feat(signature-collection): Support adding constituancies for parliamentary collection #16056
Conversation
WalkthroughThe pull request introduces a new GraphQL input type 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
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Outside diff range and nitpick comments (3)
libs/clients/signature-collection/src/lib/signature-collection.service.ts (1)
215-216: Remove console.log statements and commented-out code.Console.log statements and commented-out code can clutter the codebase and reduce maintainability. Consider removing them to keep the code clean.
Apply this diff to remove the console logs and commented code:
async createParliamentaryLists( { collectionId, candidateId, areas }: AddListsInput, auth: User, ): Promise<Success> { const { id, isActive, isPresidential, areas: collectionAreas, } = await this.currentCollection() // check if collectionId is current collection and current collection is open if (collectionId !== id.toString() || !isActive) { throw new Error('Collection is not open') } // check if user is already owner of lists const { canCreate, canCreateInfo, name } = await this.getSignee(auth) - console.log('canCreate', canCreate) - console.log('canCreateInfo', canCreateInfo) if (!canCreate) { // allow parliamentary owners to add more areas to their collection if ( !isPresidential && !( canCreateInfo?.length === 1 && canCreateInfo[0] === ReasonKey.AlreadyOwner ) ) { return { success: false, reasons: canCreateInfo } } } const filteredAreas = areas ? collectionAreas.filter((area) => areas.flatMap((a) => a.areaId).includes(area.id), ) : collectionAreas - console.log(areas) - console.log(filteredAreas) const promises = filteredAreas.map((area) => this.getApiWithAuth(this.listsApi, auth).medmaelalistarPost({ medmaelalistarRequestDTO: { frambodID: parseInt(candidateId), medmaelalisti: { svaediID: parseInt(area.id), listiNafn: `${name} - ${area.name}`, }, }, }), ) const lists = await Promise.all(promises) - // const lists = await this.getApiWithAuth( - // this.listsApi, - // auth, - // ).medmaelalistarPost({ - // medmaelalistarRequestDTO: { - // frambodID: parseInt(id), - // medmaelalisti: filteredAreas.map((area) => ({ - // svaediID: parseInt(area.id), - // listiNafn: `${name} - ${area.name}`, - // })), - // }, - // }) if (filteredAreas.length !== lists.length) { throw new Error('Not all lists created') } return { success: true } }Also applies to: 252-263
libs/clients/signature-collection/src/clientConfig.json (2)
556-556: Clarify the summary to provide actionable informationThe summary "Ath. þarfnast skoðunar" is vague and does not inform API consumers about the endpoint's purpose. Please provide a clear and descriptive summary explaining what this endpoint does or any specific concerns.
593-593: Clarify the summary to provide actionable informationSimilarly, the summary for
/Frambod/{ID}/RemoveUmbod/{Kennitala}lacks detail. Update it with a meaningful description to aid developers interacting with your API.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (7)
- libs/api/domains/signature-collection/src/lib/dto/addLists.input.ts (1 hunks)
- libs/api/domains/signature-collection/src/lib/guards/userAccess.guard.ts (2 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (2 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (2 hunks)
- libs/clients/signature-collection/src/clientConfig.json (17 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (3 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.types.ts (1 hunks)
Additional context used
Path-based instructions (7)
libs/api/domains/signature-collection/src/lib/dto/addLists.input.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/api/domains/signature-collection/src/lib/guards/userAccess.guard.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/clients/signature-collection/src/lib/signature-collection.types.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/api/domains/signature-collection/src/lib/signatureCollection.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/api/domains/signature-collection/src/lib/signatureCollection.resolver.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/clients/signature-collection/src/lib/signature-collection.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/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."
Additional comments not posted (6)
libs/api/domains/signature-collection/src/lib/dto/addLists.input.ts (1)
6-9: LGTM!The
SignatureCollectionAddListsInputclass is correctly defined, and thecandidateIdfield is appropriately validated.libs/api/domains/signature-collection/src/lib/guards/userAccess.guard.ts (1)
9-9: LGTM!The addition of the
isProcurationHoldercheck is correctly implemented, enhancing the access control logic.Also applies to: 38-40
libs/clients/signature-collection/src/lib/signature-collection.types.ts (1)
33-39: LGTM!The
AddListsInputinterface is correctly defined for the upcoming refactoring to the new ÞÍ endpoints.libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (1)
17-18: LGTM!The
addmethod is correctly implemented to facilitate the addition of parliamentary lists, and the import statement is appropriately added.Also applies to: 124-132
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (1)
150-160: Verify access requirements for the new mutation.Ensure that the combination of
@Scopesand@AccessRequirementdecorators correctly enforces the intended access control forsignatureCollectionAddAreas.libs/clients/signature-collection/src/lib/signature-collection.service.ts (1)
189-195: LGTM!The updated return value in
createParliamentaryCandidacynow appropriately uses thegetSlugfunction for slug generation.
libs/api/domains/signature-collection/src/lib/dto/addLists.input.ts
Outdated
Show resolved
Hide resolved
juni-haukur
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
libs/clients/signature-collection/src/lib/signature-collection.service.ts (2)
229-233: Optimize area filtering for better performanceTo improve the performance of filtering areas, especially with larger datasets, consider using a
Setfor faster lookup times.Here's how you might modify the code:
const filteredAreas = areas - ? collectionAreas.filter((area) => - areas.flatMap((a) => a.areaId).includes(area.id), - ) + ? (() => { + const areaIdSet = new Set(areas.map((a) => a.areaId)) + return collectionAreas.filter((area) => areaIdSet.has(area.id)) + })() : collectionAreasThis change uses a
Setto storeareaIds, allowinghaslookups with O(1) time complexity.
197-250: Ensure consistent error handling and messagingThe method
createParliamentaryListsthrows generic errors with messages like 'Collection is not open' and 'Not all lists created'. For better error handling, consider using custom error classes or returning structured error responses.Implementing custom error classes or standardized error responses can help with consistent error handling across the application. This improves maintainability and makes debugging easier.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (3 hunks)
Additional context used
Path-based instructions (1)
libs/clients/signature-collection/src/lib/signature-collection.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."
Additional comments not posted (1)
libs/clients/signature-collection/src/lib/signature-collection.service.ts (1)
189-194: LGTMThe updated return statement correctly generates the slug using
getSlugwith the candidacy ID and election type. The nullish coalescing operators (?? '') handle potentialundefinedvalues appropriately.
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Outdated
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
libs/clients/signature-collection/src/lib/signature-collection.service.ts (1)
209-211: Address the TODO: ImplementApplicationTemplateErrorThere is a TODO comment indicating the need to create
ApplicationTemplateError. Implementing this custom error class will enhance error handling by providing more specific error information and improving consistency across the application.Would you like assistance in creating the
ApplicationTemplateErrorclass, or should I open a new GitHub issue to track this task?
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- libs/api/domains/signature-collection/src/lib/dto/addLists.input.ts (1 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- libs/api/domains/signature-collection/src/lib/dto/addLists.input.ts
Additional context used
Path-based instructions (1)
libs/clients/signature-collection/src/lib/signature-collection.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."
Additional comments not posted (2)
libs/clients/signature-collection/src/lib/signature-collection.service.ts (2)
189-194: EnsuregetSlugHandles Empty Strings AppropriatelyIn the return statement,
getSlugis called with potential empty strings ifcandidacy.idorcandidacy.medmaelasofnun?.kosningTegundarenullorundefined. Verify thatgetSlugcan handle empty strings without causing errors or unintended slugs.
321-326: LGTM: Asynchronous Deletions Properly AwaitedThe use of
Promise.allwithawaitensures that all deletion promises are properly handled, preventing unexecuted deletions. This addresses the previous issue regarding unawaited asynchronous operations.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- libs/clients/signature-collection/src/clientConfig.json (20 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- libs/clients/signature-collection/src/clientConfig.json
Additional context used
Path-based instructions (1)
libs/clients/signature-collection/src/lib/signature-collection.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."
Additional comments not posted (1)
libs/clients/signature-collection/src/lib/signature-collection.service.ts (1)
215-226: Conditional logic could be simplified for better readabilityThe nested conditional statements between lines 215-226 are complex and may be hard to maintain. Refactoring the logic can improve clarity.
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Outdated
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Outdated
Show resolved
Hide resolved
….service.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (3 hunks)
Additional context used
Path-based instructions (1)
libs/clients/signature-collection/src/lib/signature-collection.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/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
libs/clients/signature-collection/src/lib/signature-collection.service.ts
Show resolved
Hide resolved
juni-haukur
added
deprecated:automerge
...
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
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Improvements