fix(admin): sc collection statuses #16501
Conversation
WalkthroughThe pull request introduces several modifications across multiple components in the signature collection library. Key changes include the addition of Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
libs/portals/admin/signature-collection/src/shared-components/completeCollectionProcessing/index.tsx (1)
Line range hint
24-33: Consider improvements for error handling, error messages, and accessibility.While the component functions correctly, consider the following improvements:
- Use consistent error handling: Either use try/catch throughout or switch to .then()/.catch() for better readability.
- Format the error message in the catch block for more meaningful user feedback.
- Provide a descriptive label for the modal's close button to enhance accessibility.
Here's an example of how you could improve the error handling and message formatting:
const completeProcessing = async () => { try { const res = await processCollectionMutation({ variables: { input: { collectionId } }, }) if (res.data?.signatureCollectionAdminProcess.success) { toast.success(formatMessage(m.toggleCollectionProcessSuccess)) setModalSubmitReviewIsOpen(false) revalidate() } else { toast.error(formatMessage(m.toggleCollectionProcessError)) } } catch (e) { - toast.error(e.message) + toast.error(formatMessage(m.genericErrorMessage, { error: e.message })) } }And for the modal's close button:
<Modal id="reviewComplete" isVisible={modalSubmitReviewIsOpen} title={formatMessage(m.completeCollectionProcessing)} label={formatMessage(m.completeCollectionProcessing)} onClose={() => setModalSubmitReviewIsOpen(false)} - closeButtonLabel={''} + closeButtonLabel={formatMessage(m.closeModalLabel)} >Make sure to add the necessary message keys in your messages file.
Also applies to: 67-67
libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1)
90-95: LGTM! Consider enhancing readability.The changes to the
typeprop logic for theListInfocomponent look good. IncludingListStatus.Inactivein the condition for setting the type to 'success' aligns with the expected behavior.To improve readability, consider using an array and the
includesmethod:type={ [ListStatus.Reviewed, ListStatus.Inactive].includes(listStatus) ? 'success' : undefined }This approach makes it easier to add or remove statuses in the future without changing the structure of the condition.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (3)
38-39: LGTM:collectionStatusadded touseLoaderDatadestructuring.The addition of
collectionStatusto the destructured return value ofuseLoaderDatais correct and aligns with TypeScript usage guidelines.Consider adding a type annotation to improve code readability:
const { collection, collectionStatus, allLists }: ListsLoaderReturn = useLoaderData();This change would make the expected return type more explicit.
185-221: LGTM: Improved mapping logic for collection areas.The refactoring of the
collection?.areas.map()function improves code readability and maintains the same functionality. The introduction ofareaListsmakes the code more explicit and easier to understand.Consider memoizing the
areaListscalculation to optimize performance, especially ifallListsis large:const memoizedAreaLists = useMemo(() => { return collection?.areas.map(area => ({ area, lists: allLists.filter(l => l.area.name === area.name) })); }, [collection?.areas, allLists]);Then use
memoizedAreaListsin your rendering logic. This change would prevent unnecessary recalculations on re-renders.
224-226: LGTM: New conditional rendering forActionCompleteCollectionProcessing.The addition of conditional rendering for the
ActionCompleteCollectionProcessingcomponent based oncollectionStatusis correct and follows React best practices.To improve code clarity, consider extracting the condition into a descriptive constant:
const isCollectionInInitialReview = collectionStatus === CollectionStatus.InInitialReview; {isCollectionInInitialReview && ( <ActionCompleteCollectionProcessing collectionId={collection?.id} /> )}This change would make the code more self-documenting and easier to understand at a glance.
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (1)
134-141: LGTM: Conditional rendering logic updated appropriately.The addition of the
collectionStatus === CollectionStatus.InInitialReviewcondition enhances the component's rendering logic as expected. This change aligns well with the PR objectives and maintains existing functionality.Consider extracting the condition into a separate variable for improved readability:
const showCreateCollection = constituencyLists?.length > 0 && allowedToProcess && collectionStatus === CollectionStatus.InInitialReview; // Then in the JSX: {showCreateCollection && ( <CreateCollection collectionId={collection?.id} areaId={areaId} /> )}This refactoring would make the rendering logic more explicit and easier to maintain.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (3 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (3 hunks)
- libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/completeCollectionProcessing/index.tsx (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/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/screens-parliamentary/List/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/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/completeCollectionProcessing/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."
🔇 Additional comments (4)
libs/portals/admin/signature-collection/src/shared-components/completeCollectionProcessing/index.tsx (1)
Line range hint
1-82: Component adheres to coding guidelines.The
ActionCompleteCollectionProcessingcomponent follows the prescribed coding guidelines:
- It uses TypeScript for prop definitions.
- It imports and uses shared UI components, promoting reusability across different NextJS apps.
- The component structure supports effective tree-shaking and bundling.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (1)
27-28: LGTM: New imports added correctly.The new imports for
CollectionStatusandActionCompleteCollectionProcessingare correctly added and follow the coding guidelines for reusability across different NextJS apps.libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (2)
33-36: LGTM: Import changes are appropriate.The addition of
CollectionStatusimport is necessary for the new functionality. Grouping imports from the same module enhances code readability and maintainability.
47-48: LGTM: useLoaderData hook updated correctly.The addition of
collectionStatusto the destructured return value is consistent with the new functionality.Please ensure that the
ListsLoaderReturntype definition has been updated to include thecollectionStatusproperty. You can verify this by running:✅ Verification successful
Verification Complete:
ListsLoaderReturnincludescollectionStatus.
- Defined in
libs/portals/admin/signature-collection/src/loaders/AllLists.loader.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the ListsLoaderReturn type definition rg -A 10 "type ListsLoaderReturn"Length of output: 35
Script:
#!/bin/bash # Search for the ListsLoaderReturn type or interface definition rg 'type\s+ListsLoaderReturn\b|interface\s+ListsLoaderReturn\b'Length of output: 173
Script:
#!/bin/bash # Verify if 'collectionStatus' is present in ListsLoaderReturn interface rg 'collectionStatus' libs/portals/admin/signature-collection/src/loaders/AllLists.loader.tsLength of output: 457
...tals/admin/signature-collection/src/shared-components/completeCollectionProcessing/index.tsx
Show resolved
Hide resolved
…_admin_collection_statuses
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1)
28-28: LGTM: Good use ofuseRevalidatorfor data freshness.The
useRevalidatorhook is correctly implemented andrevalidateis called appropriately after a successful upload. This ensures that the component's data remains up-to-date.Consider also calling
revalidate()in theonErrorcallback to ensure consistent state even in case of errors.Also applies to: 81-81
libs/portals/admin/signature-collection/src/lib/messages.ts (1)
445-465: LGTM with a minor suggestion.The new messages are well-structured and provide clear information about the collection review and processing stages. They follow the existing pattern in the file and are properly internationalized.
However, there's a minor inconsistency in the
idofcollectionProcessedMessage. It currently uses the id forcollectionReviewedTitle.Consider updating the
idforcollectionProcessedMessage:collectionProcessedMessage: { - id: 'admin-portal.signature-collection:collectionReviewedTitle', + id: 'admin-portal.signature-collection:collectionProcessedMessage', defaultMessage: 'Nú er hægt að framlengja stökum listum.', description: '', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- libs/portals/admin/signature-collection/src/lib/messages.ts (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (4 hunks)
- libs/portals/admin/signature-collection/src/shared-components/completeCollectionProcessing/index.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/index.tsx
- libs/portals/admin/signature-collection/src/shared-components/completeCollectionProcessing/index.tsx
🧰 Additional context used
📓 Path-based instructions (3)
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/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."
🔇 Additional comments (8)
libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (2)
23-23: LGTM: Import statement is correctly added.The import of
useRevalidatorfrom 'react-router-dom' is properly placed and follows good practices for named imports.
Line range hint
1-200: Overall: Changes align with coding guidelines and improve functionality.The modifications to this file adhere to the coding guidelines for
libs/**/*:
- The component remains reusable across different NextJS apps.
- TypeScript is used effectively for props and types.
- The changes don't negatively impact tree-shaking or bundling practices.
The addition of
useRevalidatorenhances the component's ability to maintain up-to-date data after mutations, improving overall functionality.libs/portals/admin/signature-collection/src/lib/messages.ts (1)
Line range hint
1-465: Overall, the changes enhance the messaging capabilities for signature collection processing.The new messages added to this file improve the communication of collection review and processing statuses. They follow the existing patterns, use proper internationalization, and contribute to the reusability of components across different NextJS apps. The changes adhere to the coding guidelines for libs/**/* files and use TypeScript for defining messages.
These additions will help provide clearer feedback to users about the state of the signature collection process for parliamentary candidacy.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (5)
12-12: Import statement is appropriate and follows project conventionsThe addition of
AlertMessageto the imports from@island.is/island-ui/coreis correct and aligns with the project's import patterns.
28-29: Imports enhance component functionality and maintain reusabilityImporting
CollectionStatusandActionCompleteCollectionProcessingcorrectly integrates necessary types and components, supporting reusability across different NextJS apps.
186-222: Rendering logic forActionCardcomponents is clear and effectiveThe mapping over
collection?.areasand filteringallListsto createareaListsis well-structured. Conditional rendering within theActionCardcomponents enhances user experience by displaying relevant information based on the data.
238-255: Conditional rendering ofAlertMessagecomponents enhances user feedbackDisplaying success messages based on
collectionStatusprovides clear communication to the user about the status of the collection processing and review stages.
224-237:⚠️ Potential issueVerify logic for enabling processing actions when
allListsis emptyWhen
allListsis empty,allLists.every((l) => l.reviewed === true)returnstrue, which may inadvertently enable theActionCompleteCollectionProcessingcomponent even when there's nothing to process.Consider updating the condition to ensure that
canProcessis onlytruewhen there are lists to review:canProcess={ - allLists.every((l) => l.reviewed === true) + allLists.length > 0 && allLists.every((l) => l.reviewed === true) }To verify if
allListscan be empty, run the following script:
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (2)
229-238: LGTM: New ActionCompleteCollectionProcessing component with effective conditional rendering.The addition of the ActionCompleteCollectionProcessing component with conditional rendering based on CollectionStatus enhances the functionality of the ParliamentaryRoot component. The use of derived values for the canProcess prop is good for performance.
Consider extracting the condition for canProcess into a separate variable or memoized value for improved readability:
const canProcessCollection = useMemo(() => !!allLists.length && allLists.every((l) => l.reviewed === true), [allLists] ); // Then use it in the component prop canProcess={canProcessCollection}This change would make the code more readable and potentially more performant if the calculation is expensive.
250-258: LGTM: Added user feedback for collection under review status.The addition of the AlertMessage component for the InReview collection status provides clear feedback to the user about the current state of the collection. This change improves the overall user experience.
Consider extracting the AlertMessage components into a separate function to reduce duplication and improve maintainability:
const renderAlertMessage = (status: CollectionStatus) => { switch (status) { case CollectionStatus.Processed: return ( <AlertMessage type="success" title={formatMessage(m.collectionProcessedTitle)} message={formatMessage(m.collectionProcessedMessage)} /> ); case CollectionStatus.InReview: return ( <AlertMessage type="success" title={formatMessage(m.collectionReviewedTitle)} message={formatMessage(m.collectionReviewedMessage)} /> ); default: return null; } }; // Then use it in the component {renderAlertMessage(collectionStatus)}This refactoring would make the code more maintainable and easier to extend in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (3)
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx (3)
12-12: LGTM: New imports enhance functionality and follow guidelines.The added imports for
AlertMessage,CollectionStatus, andActionCompleteCollectionProcessingare appropriate for the new functionality introduced in the component. They adhere to the coding guideline of promoting reusability across different NextJS apps.Also applies to: 28-29
224-228: LGTM: Improved conditional rendering of CompareLists.The addition of the condition
collectionStatus !== CollectionStatus.InReviewfor rendering CompareLists provides more granular control over the component's visibility. This change enhances the user experience by showing the comparison only when appropriate.
241-249: LGTM: Added user feedback for processed collection status.The addition of the AlertMessage component for the Processed collection status provides valuable feedback to the user. This change enhances the user experience by clearly communicating the current state of the collection.
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx
Show resolved
Hide resolved
libs/portals/admin/signature-collection/src/screens-parliamentary/index.tsx
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16501 +/- ##
=======================================
Coverage 36.77% 36.77%
=======================================
Files 6847 6847
Lines 141873 141873
Branches 40424 40424
=======================================
Hits 52167 52167
Misses 89706 89706
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 84 Passed, 0 Skipped, 32.63s Total Time |
…and-is/island.is into sc_admin_collection_statuses
Checklist:
Summary by CodeRabbit
New Features
collectionStatus.ActionCompleteCollectionProcessingcomponent for improved collection processing feedback.Bug Fixes
Chores