fix(portals-admin): locklist

[albinagu]

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• Formatting passes locally with my changes
• I have rebased against main before asking for a review

Summary by CodeRabbit

• New Features

  • Updated message definitions for improved localization, including new messages for locking a collection.
  • Introduced a modal for locking the list with user confirmation and toast notifications for success/error handling.
  • Added functionality to prevent unnecessary queries based on user input validation.
  • Implemented a new LockList component to enhance list management capabilities.

• Bug Fixes

  • Refined logic for executing queries to enhance user experience and prevent errors.

[coderabbitai]

Walkthrough

This pull request introduces several modifications primarily focused on localization and functionality enhancements in the signature collection module. Key changes include updates to message definitions in the messages.ts file, the addition of a new GraphQL mutation for locking lists, and adjustments to the ActionReviewComplete component's logic. The changes aim to improve user interactions and streamline the locking process within the application.

Changes

File	Change Summary
libs/portals/admin/signature-collection/src/lib/messages.ts	- Updated confirmListReviewedToggleBack message. - Added lockList, lockListSuccess, and lockListError messages. - Updated toggleReviewError message.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx	- Modified nationalId handling in useSignatureCollectionSignatureLookupQuery to clean non-numeric characters and adjust skip condition.
libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx	- Integrated LockList component. - Replaced toggleReview function with toggleListReview mutation, including success/error handling. - Adjusted button layout and modal content.
libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx	- Introduced ActionLockList component for locking a list with modal confirmation and mutation handling.
libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/lockList.graphql	- Added new mutation SignatureCollectionLockList with input type SignatureCollectionListIdInput.

Possibly related PRs

• feat(portals-admin): Set up for Parliamentary Lists #15892: This PR introduces new message definitions in the messages.ts file related to the signature collection feature, which aligns with the main PR's updates to message definitions for localization.
• fix(portals-admin): Parliamentary collection #16142: This PR modifies the messages.ts file, adding new messages and updating existing ones, which is relevant to the main PR's focus on enhancing message clarity and localization.
• fix(portals-admin): signees filters #16211: This PR updates the messages.ts file with new and modified messages, which complements the main PR's changes to message definitions for user interactions.
• fix(portals-admin): signature collection overview search #16268: This PR includes updates to the messages.ts file, focusing on localization strings for the admin portal, which is directly related to the main PR's changes in message definitions.

Suggested reviewers

• kksteini
• disaerna

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)

libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1)

47-47: Approve the optimization of the useCanSignQuery hook.

The addition of the !name condition in the skip property is a good optimization. It ensures that the canSign query is only executed when both the nationalIdInput is valid and the name is available. This prevents unnecessary API calls and potential errors.

For improved clarity and maintainability, consider extracting the skip condition into a separate variable:

const shouldSkipCanSignQuery = !nationalId.isValid(nationalIdInput) || !name;

const { data: canSign, loading: loadingCanSign } = useCanSignQuery({
  variables: {
    input: {
      signeeNationalId: nationalIdInput,
      listId,
    },
  },
  skip: shouldSkipCanSignQuery,
});

This refactoring would make the code more readable and easier to maintain in the future.

libs/portals/admin/signature-collection/src/lib/messages.ts (2)

426-430: LGTM: New message for locking collection

The addition of the lockList message is consistent with the existing pattern and introduces a new action for locking a collection. This enhances the functionality of the signature collection feature.

Consider adding a brief description to provide context for translators:

 lockList: {
   id: 'admin-portal.signature-collection:lockList',
   defaultMessage: 'Læsa söfnun',
-  description: '',
+  description: 'Button text for locking a signature collection',
 },

---

431-435: LGTM: Success message for locking collection

The addition of the lockListSuccess message is consistent with the existing pattern and provides user feedback for a successful lock action. This improves the user experience of the signature collection feature.

Consider adding a brief description to provide context for translators:

 lockListSuccess: {
   id: 'admin-portal.signature-collection:lockListSuccess',
   defaultMessage: 'Tókst að læsa söfnun',
-  description: '',
+  description: 'Success message shown after locking a signature collection',
 },

libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1)

83-84: Improve the modal content to provide clearer user confirmation

The text inside the modal currently displays {formatMessage(m.lockList)}, which may not provide sufficient context for the user about the action they are confirming. Consider updating the modal content to include a more descriptive message that explains the consequences of locking the list.

Suggested change:

-<Text>{formatMessage(m.lockList)}</Text>
+<Text>{formatMessage(m.lockListConfirmationMessage)}</Text>

Where m.lockListConfirmationMessage is a new localized message, such as:

lockListConfirmationMessage: 'Are you sure you want to lock this list? This action cannot be undone.',

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 47df0c0 and 8ab09e1.

📒 Files selected for processing (4)

• libs/portals/admin/signature-collection/src/lib/messages.ts (2 hunks)
• libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1 hunks)
• libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (3 hunks)
• libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList.graphql (1 hunks)

🧰 Additional context used

📓 Path-based instructions (4)

libs/portals/admin/signature-collection/src/lib/messages.ts (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList.graphql (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

🔇 Additional comments (6)

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList.graphql (2)

1-6: LGTM! Well-structured GraphQL mutation.

The SignatureCollectionLockList mutation is well-defined and follows GraphQL best practices. It's specific to signature collection, which aligns with its location in the project structure and could be reusable across different NextJS apps dealing with signature collections.

---

1-6: Verify input type and return fields in the schema.

Please ensure that the SignatureCollectionListIdInput type and the reasons and success fields are properly defined in the GraphQL schema.

libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1)

Line range hint 1-224: Overall, the code quality is high and adheres to best practices.

The PaperSignees component in this file demonstrates good adherence to coding guidelines and best practices:

1. Reusability: The component is well-structured and can be easily reused across different NextJS apps.
2. TypeScript usage: Props and types are properly defined using TypeScript, enhancing type safety and developer experience.
3. Effective bundling: The code structure supports efficient tree-shaking and bundling.
4. State management: React hooks are used effectively for managing component state and side effects.
5. API integration: GraphQL queries and mutations are implemented cleanly using generated hooks.
6. UI components: The component leverages reusable UI components from the @island.is/island-ui/core library.
7. Localization: The component uses localization hooks for internationalization support.

The recent change improves the efficiency of API calls, further enhancing the overall quality of the component.

libs/portals/admin/signature-collection/src/lib/messages.ts (3)

385-387: LGTM: Improved clarity in message wording

The change from "Aflæsa úrvinnslu" to "Opna fyrir úrvinnslu" improves the clarity of the action being described. This refinement in wording makes the user interface more intuitive.

---

441-444: LGTM: Grammatical improvement in error message

The change from "Ekki tókst loka úrvinnslu" to "Ekki tókst að loka úrvinnslu" is a grammatical improvement that enhances the readability of the error message. This minor refinement contributes to a more polished user interface.

---

Line range hint 1-743: Overall assessment: Improved functionality with minor issues

The changes to this file enhance the signature collection feature by adding new messages for locking collections and improving existing messages. The additions and modifications maintain consistency with the existing pattern, ensuring reusability and effective tree-shaking.

Key points:

1. New messages for locking collections have been added, improving functionality.
2. Existing messages have been refined for better clarity and grammar.
3. TypeScript usage is consistent throughout the file.

There's one issue that needs to be addressed:

• The id for the lockListError message is incorrect and should be fixed.

Consider adding descriptions to the new messages to provide context for translators.

These changes align well with the coding guidelines for files in the libs/**/* pattern, maintaining reusability, proper TypeScript usage, and effective tree-shaking practices.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.92%. Comparing base (d40e981) to head (358be12).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16279   +/-   ##
=======================================
  Coverage   36.92%   36.92%           
=======================================
  Files        6781     6781           
  Lines      140030   140030           
  Branches    39815    39815           
=======================================
  Hits        51705    51705           
  Misses      88325    88325           

Flag	Coverage Δ
web	1.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d40e981...358be12. Read the comment docs.

[datadog-island-is]

Datadog Report

Branch report: lock_list
Commit report: e242f82
Test service: web

✅ 0 Failed, 84 Passed, 0 Skipped, 30.84s Total Time
➡️ Test Sessions change in coverage: 1 no change

[coderabbitai]

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (5)

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/lockList.graphql (1)

3-3: Consider documenting the reasons field.

The reasons field in the mutation response could benefit from some documentation. Consider adding a comment or description in the schema to clarify its purpose and potential values.

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (2)

1-16: Remove unused import ListStatus

The ListStatus import from ../../../lib/utils is not used in this component. To maintain clean and efficient code, it's recommended to remove unused imports.

Apply this change:

-import { ListStatus } from '../../../lib/utils'

Also, since listStatus prop is of type string, consider using a more specific type if possible, such as a union of allowed status values. This would improve type safety and make the component's API more clear.

---

17-37: Hooks and state management look good, consider enhancing error handling

The use of hooks and state initialization is well-implemented. The GraphQL mutation is properly set up with appropriate callbacks.

Consider enhancing the error handling in the onError callback. Instead of a generic error message, you could log the error details or display a more specific error message to the user. For example:

onError: (error) => {
  console.error('Failed to lock list:', error);
  toast.error(formatMessage(m.lockListError, { error: error.message }));
},

This would provide more context for debugging and potentially more helpful feedback to the user.

libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1)

27-41: Consider extracting toggleListReview mutation into a custom hook

The implementation of the toggleListReview mutation is well-structured with proper error handling and success callbacks. However, to enhance reusability across different components or NextJS apps, consider encapsulating this mutation logic into a custom hook. This aligns with the project's emphasis on reusable components and hooks.

Example:

// useToggleListReview.ts
export const useToggleListReview = (listId: string) => {
  const { formatMessage } = useLocale()
  const { revalidate } = useRevalidator()

  return useToggleListReviewMutation({
    variables: { input: { listId } },
    onCompleted: () => {
      revalidate()
      toast.success(formatMessage(m.toggleReviewSuccess))
    },
    onError: () => {
      toast.error(formatMessage(m.toggleReviewError))
    },
  })
}

// In the component
const [toggleListReview, { loading }] = useToggleListReview(listId)

This refactoring would improve the overall structure and reusability of the code.

libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)

44-47: Approve with suggestion: Consider extracting the nationalId cleaning logic

The change improves input validation by ensuring only numeric characters are used for the nationalId. This is a good practice that can prevent potential errors.

To enhance code readability and maintainability, consider extracting the nationalId cleaning logic and the length check into a separate function. This would make the code more self-documenting and easier to update if requirements change. For example:

const cleanAndValidateNationalId = (id: string): string => {
  const cleaned = id.replace(/[^0-9]/g, '');
  return cleaned.length === 10 ? cleaned : '';
};

// Usage in the component:
const cleanedNationalId = cleanAndValidateNationalId(searchTerm);

const { data, loading } = useSignatureCollectionSignatureLookupQuery({
  variables: {
    input: {
      collectionId: collection?.id,
      nationalId: cleanedNationalId,
    },
  },
  skip: !cleanedNationalId,
});

This approach would also make it easier to add more sophisticated validation if needed in the future.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8ab09e1 and e3ce39c.

📒 Files selected for processing (5)

• libs/portals/admin/signature-collection/src/lib/messages.ts (2 hunks)
• libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1 hunks)
• libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (3 hunks)
• libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (1 hunks)
• libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/lockList.graphql (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• libs/portals/admin/signature-collection/src/lib/messages.ts

🧰 Additional context used

📓 Path-based instructions (4)

libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/lockList.graphql (1)

Pattern libs/**/*: "Confirm that the code adheres to the following:

• Reusability of components and hooks across different NextJS apps.
• TypeScript usage for defining props and exporting types.
• Effective tree-shaking and bundling practices."

🔇 Additional comments (6)

libs/portals/admin/signature-collection/src/shared-components/completeReview/lockList/index.tsx (3)

39-49: Button implementation looks good

The Button component is well-implemented with appropriate props for styling and functionality. The use of formatMessage for the button text ensures proper localization.

---

78-78: Component export looks good

The default export of the ActionLockList component is correct and follows common React practices.

---

1-78: Overall assessment: Well-implemented component with room for minor improvements

The ActionLockList component is generally well-implemented and adheres to the coding guidelines for the libs directory. It demonstrates good use of TypeScript, React hooks, and follows best practices for reusability.

Key strengths:

1. Proper use of TypeScript for prop definitions.
2. Effective use of React hooks and state management.
3. Integration with localization and UI components.

Areas for improvement:

1. Remove unused import and consider more specific prop types.
2. Enhance error handling in the GraphQL mutation.
3. Improve Modal implementation for better accessibility and user experience.

These suggested improvements will further enhance the component's quality, maintainability, and user-friendliness.

libs/portals/admin/signature-collection/src/shared-components/completeReview/index.tsx (3)

20-26: LGTM: Improved state management and readability

The addition of modalSubmitReviewIsOpen state and modalText variable, along with the listReviewed check based on listStatus, enhances the component's functionality and readability. The changes maintain proper TypeScript usage for props.

---

45-53: LGTM: Improved layout and functionality

The changes to the component's JSX structure are well-implemented:

1. The layout change to spaceBetween improves the visual balance.
2. The addition of the LockList component enhances modularity.
3. The conditional rendering of the Button's icon based on listReviewed status improves user experience.

These modifications align well with the project's emphasis on reusable components and enhance the overall functionality of the ActionReviewComplete component.

---

74-76: LGTM: Enhanced Modal functionality and UX

The changes to the Modal component are well-implemented:

1. The use of modalText for dynamic content improves flexibility.
2. The Button now correctly uses the toggleListReview function.
3. The conditional rendering of the Button's icon based on listReviewed status enhances user experience.

These modifications maintain proper localization practices and improve the overall functionality of the Modal.