-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(signature-collection): Admin create lists fix #16491
Conversation
WalkthroughThe pull request introduces modifications to the OpenAPI specification for the MedmaeliRestAPI, adding a new endpoint Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (3)
51-52
: LGTM! Consider using array destructuring for a slight optimization.The introduction of
areaId
is a good addition, aligning with the changes mentioned in the summary. The logic for finding the corresponding area ID is correct.For a slight optimization, consider using array destructuring:
const [areaId] = collection.areas.filter((a) => a.name === constituencyName).map(a => a.id);This approach eliminates the need for optional chaining and directly assigns the ID or
undefined
if no match is found.
131-134
: LGTM! Consider using prop spreading for cleaner code.The addition of the
areaId
prop to theCreateCollection
component is consistent with the earlier changes and aligns with the summary. This change enhances the component's functionality by providing area-specific data.For cleaner and more maintainable code, consider using prop spreading:
<CreateCollection {...{ collectionId: collection?.id, areaId, }} />This approach makes it easier to add or remove props in the future without changing the component invocation structure.
Line range hint
1-234
: Overall, the changes enhance area-specific functionality and maintain code quality.The modifications to the
Constituency
component, including the introduction ofareaId
and its integration with theCreateCollection
component, align well with the PR objectives and the AI-generated summary. These changes improve the handling of area-specific data in the signature collection process.The code adheres to the provided coding guidelines, particularly in terms of TypeScript usage for props and maintaining the reusability of components across different NextJS apps.
To further improve the code:
- Consider extracting the
areaId
logic into a custom hook for reusability across components.- Evaluate the possibility of using React Context for sharing the collection data, which could simplify prop passing and make the component more maintainable as it grows.
libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (1)
224-227
: LGTM! Consider adding a comment for clarity.The addition of the
areaId
prop to theCreateCollection
component enhances its flexibility while maintaining backwards compatibility. This change aligns well with the library's goal of reusability across different NextJS apps.Consider adding a brief comment explaining why
areaId
is set toundefined
for presidential lists. This would improve code readability and maintainability. For example:<CreateCollection collectionId={collection?.id} areaId={undefined} // Presidential lists are not area-specific />libs/clients/signature-collection/src/clientConfig.json (1)
966-968
: Good addition of summary, but consider enhancing the description.The addition of a summary for the
/Medmaelalistar
POST endpoint improves the API documentation. However, the description field is now empty. Consider adding a more detailed description to provide additional context and usage information for this endpoint.libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx (2)
23-29
: Use optional parameter syntax for theareaId
propSimplify the prop definition by using the optional parameter syntax
areaId?: string
instead ofareaId: string | undefined
, which is more idiomatic in TypeScript.Apply this diff to update the prop definition:
const CreateCollection = ({ collectionId, areaId, }: { collectionId: string - areaId: string | undefined + areaId?: string }) => {
Line range hint
63-73
: Incorrect error message displayed on collection creation failureIn the
createNewCollection
function, when the collection creation fails (in theelse
branch), the error toast is displaying the success message. This could confuse users.Apply this diff to fix the error message:
} else { - toast.error(formatMessage(m.createCollectionSuccess)) + toast.error(formatMessage(m.createCollectionError)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- libs/clients/signature-collection/src/clientConfig.json (2 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/Constituency/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-presidential/AllLists/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/clients/signature-collection/src/clientConfig.json (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/clients/signature-collection/src/lib/signature-collection-admin.service.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/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-presidential/AllLists/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/createCollection/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 (5)
libs/clients/signature-collection/src/clientConfig.json (2)
115-149
: New admin endpoint for creating recommendation lists looks good.The new
/Admin/Medmaelalisti
POST endpoint is well-structured and consistent with the existing API patterns. It provides a clear purpose for adding new recommendation lists to an established proposal, uses appropriate DTOs, and is correctly placed under the Admin section for proper access control.
115-149
: Overall, these changes enhance the API's recommendation list management capabilities.The addition of the new admin endpoint and the update to the existing endpoint for recommendation lists are consistent and well-structured. They maintain the separation between admin and non-admin functionalities, use appropriate DTOs, and follow existing API patterns. These changes should improve the flexibility and usability of the API for managing recommendation lists.
Also applies to: 966-968
libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx (3)
56-56
: LGTMThe addition of the
areas
field correctly incorporatesareaId
into the mutation variables when provided.
59-61
: Ensurerevalidate
is appropriately triggeredCalling
revalidate()
in theonCompleted
callback ensures that the data is refreshed after the mutation. Just make sure that this aligns with your data-fetching strategy and doesn't cause unnecessary re-renders.
172-179
: Consider ensuringconstituencyName
is reliably obtainedWhen rendering the candidate area input,
constituencyName
is obtained viauseParams()
. Ensure thatconstituencyName
is consistently available whenareaId
is present to prevent potential undefined values.
libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx
Show resolved
Hide resolved
libs/portals/admin/signature-collection/src/shared-components/createCollection/index.tsx
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16491 +/- ##
==========================================
- Coverage 36.78% 36.78% -0.01%
==========================================
Files 6845 6845
Lines 141740 141747 +7
Branches 40378 40380 +2
==========================================
Hits 52139 52139
- Misses 89601 89608 +7
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 ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
New Features
areaId
support to theCreateCollection
component for improved data handling.Bug Fixes
Refactor
Lists
component to enhance list management.Style
CreateCollection
component to conditionally display area information.