fix(j-s): Digital mailbox API #16301
Conversation
WalkthroughThe changes in this pull request primarily enhance the error handling and logging capabilities within the Changes
Possibly related PRs
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
|
…nd-is/island.is into j-s/fix-digital-mailbox-api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/internal/internalCase.response.ts (1)
Line range hint
1-47: Consider broader impact and next steps.The changes to the
Subpoenainterface improve type safety and provide more detailed information about subpoena status, aligning with the PR objectives. However, consider the following next steps:
- Update any components or API routes that use the
Subpoenainterface to handle the newserviceStatusproperty.- Ensure that the
ServiceStatusenum is properly defined in the@island.is/judicial-system/typesmodule.- Update relevant documentation to reflect these changes.
- Consider adding unit tests to verify the correct usage of the new
serviceStatusproperty.These steps will ensure that the changes are fully integrated into the NextJS application and maintain overall code quality.
Would you like assistance in identifying the components or API routes that might need updates based on these changes?
libs/judicial-system/types/src/lib/defendant.ts (2)
45-47: Approved with suggestion: Type-safe check for successful service statusThe
isSuccessfulServiceStatusfunction is a valuable addition, providing a reusable way to check if a service status is successful. It's well-typed and concise, adhering to the coding guidelines for thelibsdirectory.However, to improve type safety, consider updating the
successfulServiceStatusconstant to useServiceStatus[]instead ofstring[]. This change would ensure that only validServiceStatusvalues can be included in the array.Here's the suggested change:
-export const successfulServiceStatus: string[] = [ +export const successfulServiceStatus: ServiceStatus[] = [ ServiceStatus.ELECTRONICALLY, ServiceStatus.DEFENDER, ServiceStatus.IN_PERSON, ]This modification will make the
isSuccessfulServiceStatusfunction more robust and prevent potential errors from invalid status strings.
38-47: Summary: Valuable additions for handling service statusesThe new exports,
successfulServiceStatusandisSuccessfulServiceStatus, are valuable additions to thedefendant.tsfile. They provide a clear, reusable way to define and check successful service statuses, which aligns well with the PR objectives of including more detailed information regarding defender choices and API responses.These additions adhere to the coding guidelines for the
libsdirectory, ensuring reusability across different NextJS apps and proper TypeScript usage. They will likely improve the consistency and reliability of service status checks throughout the judicial system components.Consider documenting these new exports in the relevant API documentation to ensure other developers are aware of their availability and proper usage.
libs/judicial-system/types/src/index.ts (1)
Line range hint
1-11: LGTM! Consider adding JSDoc comment for the new export.The addition of
isSuccessfulServiceStatusis consistent with the existing exports and adheres to the coding guidelines for thelibsdirectory. It enhances the module's functionality without breaking existing exports.To improve documentation, consider adding a JSDoc comment for the new export in the
./lib/defendantfile. For example:/** * Checks if the given service status is considered successful. * @param status The service status to check * @returns True if the status is successful, false otherwise */ export function isSuccessfulServiceStatus(status: ServiceStatus): boolean;This will provide better context for developers using this function across different NextJS apps.
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/case.response.ts (1)
47-50: Improved logic for determininghasBeenServedThe update to use
isSuccessfulServiceStatusenhances the accuracy of determining if a case has been served. This change aligns well with NextJS best practices by improving data transformation logic.For better readability, consider using optional chaining:
hasBeenServed: isSuccessfulServiceStatus(subpoenas[0]?.serviceStatus) ?? false,This would eliminate the need for the length check and make the code more concise.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts (0 hunks)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/case.response.ts (2 hunks)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/internal/internalCase.response.ts (2 hunks)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (4 hunks)
- libs/judicial-system/types/src/index.ts (1 hunks)
- libs/judicial-system/types/src/lib/defendant.ts (1 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/subpoena/subpoena.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/case.response.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/internal/internalCase.response.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
libs/judicial-system/types/src/index.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/judicial-system/types/src/lib/defendant.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."
🪛 Biome
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts
[error] 122-125: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🔇 Additional comments (5)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/internal/internalCase.response.ts (2)
Line range hint
1-7: LGTM: Import statement updated correctly.The addition of
ServiceStatusto the import statement is consistent with its usage in theSubpoenainterface and follows TypeScript best practices.
Line range hint
42-47: Approve change with verification suggestion.The replacement of
acknowledged: booleanwithserviceStatus?: ServiceStatusprovides a more detailed representation of the subpoena's status, aligning with the PR objectives. However, asserviceStatusis optional, it's important to ensure proper handling in the code that uses this interface.To verify the proper usage of the new
serviceStatusproperty, please run the following script:This will help identify areas where the
Subpoenainterface orserviceStatusproperty is used, allowing you to verify that the optional nature ofserviceStatusis properly handled.libs/judicial-system/types/src/lib/defendant.ts (1)
39-43: LGTM: Well-defined constant for successful service statusesThe
successfulServiceStatusconstant is a good addition. It provides a clear, reusable definition of what constitutes a successful service status, which can be easily used across different parts of the application. The use of TypeScript for explicit typing (string[]) is appropriate and aligns with the coding guidelines for thelibsdirectory.apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/case.response.ts (1)
4-7: LGTM: Import changes are well-structuredThe addition of
isSuccessfulServiceStatusimport is properly grouped with related imports from the same module. This follows good TypeScript practices and improves code organization.apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (1)
9-13: Approved: ImportingisSuccessfulServiceStatusThe addition of
isSuccessfulServiceStatusimport is appropriate and necessary for the functionality implemented.
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts
Outdated
Show resolved
Hide resolved
unakb
dismissed
coderabbitai[bot]’s stale review
To get changes through. Ive already resolved all comments but git doesnt seem to recognize it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16301 +/- ##
==========================================
- Coverage 36.86% 36.84% -0.02%
==========================================
Files 6798 6796 -2
Lines 140495 140394 -101
Branches 39945 39917 -28
==========================================
- Hits 51792 51729 -63
+ Misses 88703 88665 -38
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 73 Total Test Services: 0 Failed, 71 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (5)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (2)
66-71: LGTM: New properties align with PR objectives.The added properties
hasChosenDefenderanddefaultDefenderChoicein theSubpoenaDataclass provide the necessary information about defender choices as per the PR objectives. The use of appropriate types and@ApiPropertydecorators is commendable.For consistency, consider adding a description to the
@ApiPropertydecorators:@ApiProperty({ type: Boolean, description: 'Indicates if a defender has been chosen' }) hasChosenDefender?: boolean @ApiProperty({ enum: DefenderChoice, description: 'Default choice for the defender' }) defaultDefenderChoice?: DefenderChoiceThis will enhance the API documentation.
123-127: SimplifyhasChosenDefenderlogic and LGTM fordefaultDefenderChoice.The addition of
hasChosenDefenderanddefaultDefenderChoiceto the returned object aligns well with the PR objectives. However, thehasChosenDefenderlogic can be simplified:hasChosenDefender: defendantInfo?.defenderChoice !== undefined && defendantInfo.defenderChoice !== DefenderChoice.DELAY,This eliminates the need for the
Booleanconstructor and makes the logic more straightforward.The
defaultDefenderChoice: DefenderChoice.DELAYis correct and aligns with the PR objectives.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (1)
Pattern
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/judicial-system/digital-mailbox-api/src/app/modules/cases/models/subpoena.response.ts (2)
9-13: LGTM: New imports are relevant and necessary.The added imports from '@island.is/judicial-system/types' are appropriate for the changes made in this file. They introduce necessary types and functions used in the updated code.
100-104: LGTM: Improved handling of subpoenas and service status.The changes in this segment effectively address previous concerns and improve the code:
- Renaming
subpoenatosubpoenasbetter reflects the handling of multiple subpoenas.- The added check
subpoenas.length > 0prevents potential undefined access, addressing a previous review comment.- Using
isSuccessfulServiceStatuscentralizes the logic for checking service status, improving maintainability.These changes enhance the robustness and clarity of the code.
* feat(j-s): Block create subpoena on staging and dev * Update subpoena.service.ts * fix(j-s): Fix mailbox API * remove changes not meant for this branch * Update subpoena.service.ts * fix(j-s): reverting changes from other branch * Update subpoena.response.ts * Update subpoena.response.ts * Update subpoena.response.ts * Update subpoena.response.ts --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…-pages (#16234) * Service portal removal. Add portals my pages * minor fixes * Fix * path fix * fix(portals-admin): locklist (#16279) * fix(portals-admin): locklist * tweak * msg id fix * tweak --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(service-portal): feature flag resolver for documents (#16285) * fix: def info and alert * feat: add feature flag to resolver * fix: move ff call to seperate function --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(vehicles-bulk-mileage): Fixes after testing review (#16295) * fix: testing fixes v1 * fix: testing comments v2 * fix: better message * fix: function name * fix: duplicate loading --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(tests): New @island/testing/e2e library (#16287) * Add @swc-node/register and @swc/core * Add testing/e2e library * update project.json for testing/e2e * fix import for libTestingE2e --------- Co-authored-by: Kristofer <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(parental-leave): ApplicationRights (#15901) * feat(parental-leave): ApplicationRights Added applicationRights to parental-leave when sending application. Since we are using a new way of calculating periods * Fix days used by period calculation * Tests for new periods * rename function with proper camelCase * Refactor: Made duplicate code into a function * Make ApplicationRights nullable * refactor: function instead of duplicate code * remove console.log * error handling for period data * clientConfig nullable fix * Fixes for calculation of months. And using clamp to get correct value of daysLeft * Multiply amount of months by 30 for period calculation with month durations * Fix old calculation of endDate with months --------- Co-authored-by: hfhelgason <[email protected]> Co-authored-by: veronikasif <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(passport-application): Updated readme (#16296) * updated readme * updated readme * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(regulations-admin): date format signature, remove self affect, disclaimer text (#16288) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(regulations-admin): No diff no addition in appendix (#16293) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(web): Global alert banner - Handle null case (#16298) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(web): Change custom syslumenn pages config for header (#16299) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(j-s): Digital mailbox API (#16301) * feat(j-s): Block create subpoena on staging and dev * Update subpoena.service.ts * fix(j-s): Fix mailbox API * remove changes not meant for this branch * Update subpoena.service.ts * fix(j-s): reverting changes from other branch * Update subpoena.response.ts * Update subpoena.response.ts * Update subpoena.response.ts * Update subpoena.response.ts --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collection): Fix list reviewed toggle (#16300) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * chore(scripts): Stricter shell script checking (#16242) * Set style level for shellcheck * Linting & formatting scripts * Remove _podman.sh script * Format all scripts * Add reviewdog/action-shfmt step * Configure shfmt * Merge from main * Linting * Move shfmt to before lint * Remove reviewdog * Allow external sources in shellcheck * Use Reviewdog for shellcheck * Set version for Reviewdog --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * chore(new-primary-school): Update messages namespace (#16302) Co-authored-by: veronikasif <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(driving-license): check if 65+ renewal is possible (#16292) * check if 65 renewal is possible * remove console log * cleanup * coderabbit tweaks * coderabbit changes * quick fix * add type? --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * feat(service-portal): default defender and has chosen fields for subpoena (#16306) * fix: def info and alert * feat: add feature flag to resolver * fix: move ff call to seperate function * feat: add default choices ans has chosen + loading states * fix: use type * fix: undefined type issue * fix: simplify check * Update service setup for my pages infra * chore: charts update dirty files * Remove from infra * undo rename --------- Co-authored-by: albinagu <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Ásdís Erna Guðmundsdóttir <[email protected]> Co-authored-by: Þorkell Máni Þorkelsson <[email protected]> Co-authored-by: Svanhildur Einarsdóttir <[email protected]> Co-authored-by: Kristofer <[email protected]> Co-authored-by: helgifr <[email protected]> Co-authored-by: hfhelgason <[email protected]> Co-authored-by: veronikasif <[email protected]> Co-authored-by: Rafn Árnason <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Rúnar Vestmann <[email protected]> Co-authored-by: mannipje <[email protected]> Co-authored-by: unakb <[email protected]> Co-authored-by: juni-haukur <[email protected]> Co-authored-by: birkirkristmunds <[email protected]> Co-authored-by: Kristján Albert <[email protected]>
What
Fix digital mailbox API after db changes and added 2 new values to return object
Why
Because it's not returning correct information and we need more concrete info about defender choices
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores