feat(regulations-admin): Add change appendix diff #15811
Conversation
WalkthroughThe changes introduce a new optional property 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15811 +/- ##
==========================================
- Coverage 36.70% 36.69% -0.02%
==========================================
Files 6778 6778
Lines 139605 139671 +66
Branches 39688 39719 +31
==========================================
Hits 51247 51247
- Misses 88358 88424 +66
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
|
Affected services are: api,download-service,regulations-admin-backend,portals-admin, Deployed services: portals-admin,api,download-service,regulations-admin-backend,consultation-portal,web. |
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 3 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
apps/services/regulations-admin-backend/src/app/modules/draft_regulation/models/appendix.model.ts (1)
Line range hint
1-12: LGTM! Consider grouping related properties.The changes look good and align with NestJS best practices and the PR objectives. The new
diffproperty is correctly implemented as an optional field with proper Swagger documentation.A minor suggestion for improved code organization:
Consider grouping related properties together. You could move the
diffproperty right after thetextproperty since they are both of typeHTMLText. This would improve readability and make the relationship between these properties more apparent.export class AppendixModel { @ApiProperty() title!: PlainText @ApiProperty() text!: HTMLText + @ApiPropertyOptional() + diff?: HTMLText - - @ApiPropertyOptional() - diff?: HTMLText }libs/api/domains/regulations-admin/src/lib/graphql/dto/updateDraftRegulationChange.input.ts (1)
11-13: LGTM! Consider grouping related fields.The addition of the
difffield is well-implemented and consistent with the existing structure. It adheres to TypeScript typing and NestJS/GraphQL conventions.Consider grouping related fields together for better code organization. You could move the
difffield right after thetextfield, as they are closely related:@Field(() => String, { nullable: true }) text!: HTMLText @Field(() => String, { nullable: true }) diff?: HTMLText @Field(() => String, { nullable: true }) title!: PlainTextThis arrangement might improve readability and make the relationship between
textanddiffmore apparent.libs/api/domains/regulations-admin/src/lib/graphql/dto/createDraftRegulationChange.input.ts (1)
11-13: LGTM! Consider adding a comment for the newdifffield.The addition of the
difffield is well-implemented and aligns with the PR objectives. It correctly uses TypeScript and the@Fielddecorator, adhering to the coding guidelines for thelibsdirectory.For consistency and improved documentation, consider adding a brief comment explaining the purpose of the
difffield, similar to how you might have comments for other fields in your codebase.Example:
@Field(() => String, { nullable: true }) // Represents the difference in text for the appendix diff?: HTMLTextlibs/api/domains/regulations-admin/src/lib/graphql/models/draftRegulationChange.model.ts (1)
12-13: LGTM! Consider adding a comment for consistency.The addition of the
difffield is well-implemented and consistent with the existing codebase. It correctly uses TypeScript and the@Fielddecorator from@nestjs/graphql.For consistency with the
textfield, consider adding a non-null assertion (!) to thedifffield:- diff?: HTMLText + diff!: HTMLTextThis change would make the field consistent with the
textfield above it, assuming that the behavior should be the same for both fields.libs/portals/admin/regulations-admin/src/utils/formatAmendingUtils.ts (2)
91-97: LGTM! Consider a minor readability improvement.The
hasAnyChangefunction is well-implemented and efficiently checks for the presence of changes in the diff. It adheres to TypeScript usage and is exported for reusability across different NextJS apps.For improved readability, consider using object shorthand notation:
- return hasDeletion || hasInsert + return { hasDeletion, hasInsert }This change would provide more detailed information about the specific types of changes present.
99-116: LGTM! Consider refactoring for improved conciseness.The
updateAppendixWordingfunction correctly handles the replacement of "fylgiskjal" and "viðauki" with their appropriate forms. It's exported for reusability and adheres to TypeScript usage.Consider refactoring the function for improved conciseness and maintainability:
export const updateAppendixWording = (input: string): string => { const replacements: { [key: string]: string } = { fylgiskjal: 'fylgiskjali', viðauki: 'viðauka', }; return input.replace(/fylgiskjal|viðauki/gi, (match) => { const replacement = replacements[match.toLowerCase()]; return match[0] === match[0].toUpperCase() ? replacement.charAt(0).toUpperCase() + replacement.slice(1) : replacement; }); };This refactored version reduces repetition and improves maintainability by using a replacements object.
libs/portals/admin/regulations-admin/src/components/impacts/ReferenceText.tsx (1)
90-93: Approve changes with a minor suggestion for consistencyThe addition of the
s.diffclass to theHTMLDumpcomponents is consistent with the PR objective of adding change appendix diff functionality. The use of thecnfunction for combining class names is a good practice.For consistency, consider applying the same change to the
HTMLDumpcomponent in the comments section (around line 110).<HTMLDump - className={ed.classes.editor} + className={cn(ed.classes.editor, s.diff)} html={comments as HTMLText} />libs/portals/admin/regulations-admin/src/state/types.ts (1)
50-50: LGTM! Consider adding a brief comment for thediffproperty.The addition of the optional
diffproperty of typeHtmlDraftFieldto theAppendixDraftFormtype is well-structured and consistent with the existing codebase. It enhances the form to potentially include difference information.Consider adding a brief comment explaining the purpose of the
diffproperty, similar to the existing comment for therevokedproperty. This would improve code documentation and maintainability. For example:/** * Represents the difference in the appendix content. * This is used to show changes between versions of the appendix. */ diff?: HtmlDraftFieldlibs/portals/admin/regulations-admin/src/components/EditBasics.tsx (2)
141-159: UseforEachinstead ofmapwhen not using the returned arrayIn the
updateAppendixesfunction, you're usingmapfor side effects without utilizing the returned array. This can be misleading and may affect readability.Consider replacing
mapwithforEachto reflect the intent more clearly.Apply this diff to make the change:
- amendingArray.map((item) => { - item.appendixes.map((apx, idx) => { + amendingArray.forEach((item) => { + item.appendixes.forEach((apx, idx) => { // Rest of the code remains the same }) })
144-144: Avoid hardcoding strings; use localization for internationalizationThe default title
Viðauki ${idx + 1}is hardcoded in Icelandic. To support internationalization and maintain consistency across locales, consider using a localization function liketto handle this string.libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (1)
419-445: Use.forEachinstead of.mapwhen not using the returned arrayThe
appendixes?.mapfunction is used, but its return value is not utilized. For better readability and to indicate that the iteration is for side effects (modifyingadditionArray), consider using.forEachinstead.Apply this change:
- appendixes?.map((apx, idx) => { + appendixes?.forEach((apx, idx) => {libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (2)
223-229: TypeScript Optional Parameters ClarificationIn the
getDiffHtmlfunction, you're using optional parameterspreviousandcurrent. Since these are optional, it's good practice to explicitly define their types asHTMLText | undefinedfor better type safety and clarity.
287-298: Improve Comment Placement for ReadabilityThe multi-line comment within the
appendixesmapping could affect code readability. Placing comments inside object literals may make the code harder to read and maintain.Consider moving the comment above the mapping function to enhance clarity:
+ /** + * These changes are needed to both display the diff in the regulation editor and + * to display the appendix in the change regulation: + * + * IF DIFF => Show diff with something similar to getDiffHtml() + * IF NO DIFF => Then 'diff' is undefined + * IF NEW APPENDIX => 'diff' is <div data-diff="new"></div> + */ appendixes: activeChange.appendixes.map((apx, i) => ({ title: apx.title.value, text: apx.text.value, - /** - * These changes are needed to both display the diff in the regulation editor and - * to display the appendix in the change regulation: - * - * IF DIFF => Show diff with something similar to getDiffHtml() - * IF NO DIFF => Then 'diff' is undefined - * IF NEW APPENDIX => 'diff' is <div data-diff="new"></div> - */ diff: getAppendixDiffHtml(i), })),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
- apps/services/regulations-admin-backend/src/app/modules/draft_regulation/models/appendix.model.ts (2 hunks)
- apps/services/regulations-admin-backend/src/main.ts (1 hunks)
- libs/api/domains/regulations-admin/src/lib/graphql/dto/createDraftRegulationChange.input.ts (1 hunks)
- libs/api/domains/regulations-admin/src/lib/graphql/dto/updateDraftRegulationChange.input.ts (1 hunks)
- libs/api/domains/regulations-admin/src/lib/graphql/models/draftRegulationChange.model.ts (1 hunks)
- libs/portals/admin/regulations-admin/src/components/EditBasics.tsx (5 hunks)
- libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (3 hunks)
- libs/portals/admin/regulations-admin/src/components/impacts/ReferenceText.tsx (1 hunks)
- libs/portals/admin/regulations-admin/src/state/makeFields.ts (1 hunks)
- libs/portals/admin/regulations-admin/src/state/types.ts (1 hunks)
- libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (5 hunks)
- libs/portals/admin/regulations-admin/src/utils/formatAmendingUtils.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
apps/services/regulations-admin-backend/src/app/modules/draft_regulation/models/appendix.model.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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/services/regulations-admin-backend/src/main.ts (2)
Pattern
apps/services/**/*: "Confirm that the code adheres to the following:
- NestJS architecture, including modules, services, and controllers.
- Dependency injection patterns and service encapsulation.
- Integration and unit testing coverage and practices."
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/api/domains/regulations-admin/src/lib/graphql/dto/createDraftRegulationChange.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/regulations-admin/src/lib/graphql/dto/updateDraftRegulationChange.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/regulations-admin/src/lib/graphql/models/draftRegulationChange.model.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/regulations-admin/src/components/EditBasics.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/regulations-admin/src/components/impacts/EditChange.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/regulations-admin/src/components/impacts/ReferenceText.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/regulations-admin/src/state/makeFields.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/regulations-admin/src/state/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/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.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/regulations-admin/src/utils/formatAmendingUtils.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 (18)
apps/services/regulations-admin-backend/src/main.ts (2)
Line range hint
1-11: Ensure comprehensive testing for the increased payload limit.The changes adhere to the NestJS architecture and dependency injection patterns as required by the coding guidelines. However, it's crucial to:
- Update integration tests to cover scenarios with larger payloads (up to 400kb).
- Ensure that error handling for oversized payloads is still functioning correctly.
- Test the impact on API response times with larger payloads.
To verify the testing coverage, please run the following script:
#!/bin/bash # Description: Check for tests related to payload size handling # Search for tests related to payload size or JSON body limit echo "Tests related to payload size or JSON body limit:" rg --type typescript -e 'payload|body.*size|jsonBodyLimit' -g '*spec.ts' apps/services/regulations-admin-backend/src # Check for any TODOs related to testing in the spec files echo "TODOs in test files:" rg --type typescript -e 'TODO' -g '*spec.ts' apps/services/regulations-admin-backend/srcThis script will help identify existing tests related to payload handling and any TODOs that might need addressing in the test suite.
10-10: Consider the implications of increasing the JSON body limit.The
jsonBodyLimithas been increased from '300kb' to '400kb'. While this change allows for larger JSON payloads, it's important to consider the following:
- Performance: Larger payloads may impact server performance and response times.
- Security: Increased limit could potentially expose the server to larger malicious payloads.
- Scalability: Ensure that this change doesn't mask underlying issues with payload size management.
To understand the necessity of this change, please run the following script to check for any recent large payloads that might have triggered this increase:
This script will help verify if there have been issues with payload sizes that justify this increase.
Consider implementing a more dynamic approach to handle varying payload sizes:
- Use streaming for large payloads instead of increasing the limit.
- Implement pagination or chunking for large data sets.
- Optimize the data structure to reduce payload size.
These approaches can provide more scalable solutions without compromising security or performance.
libs/api/domains/regulations-admin/src/lib/graphql/dto/updateDraftRegulationChange.input.ts (2)
Line range hint
1-38: Adherence to coding guidelines confirmed.The code in this file follows the specified guidelines for
libs/**/*files:
- It uses TypeScript for defining props and exporting types.
- The input types are structured in a way that promotes reusability across different NextJS apps.
- There are no apparent issues with tree-shaking or bundling practices.
Line range hint
1-1: Clean up imports and verify consistency.
- The import statement includes unused types. Consider removing unused imports to keep the code clean:
- import { HTMLText, ISODate, PlainText } from '@island.is/regulations' + import { HTMLText } from '@island.is/regulations'
- The
UpdateDraftRegulationChangeInputclass also has adifffield (line 34), which wasn't mentioned in the AI summary. Please verify if this addition is intentional and consistent with the changes made toUpdateChangeAppendixInput.Also applies to: 34-34
libs/api/domains/regulations-admin/src/lib/graphql/dto/createDraftRegulationChange.input.ts (2)
Line range hint
15-41: Good design allowing easy extensionThe
CreateDraftRegulationChangeInputclass demonstrates good design principles. It usesCreateChangeAppendixInputin itsappendixesfield, allowing it to automatically incorporate the newdifffield without requiring any changes. This approach promotes reusability and maintainability, aligning well with the coding guidelines for thelibsdirectory.
Line range hint
1-41: Overall, excellent implementation adhering to coding guidelinesThe changes in this file demonstrate a high-quality implementation that adheres to the coding guidelines for the
libsdirectory:
- The code promotes reusability of components across different NextJS apps.
- TypeScript is effectively used for defining props and exporting types.
- The structure supports effective tree-shaking and bundling practices.
The minimal and focused changes to add the
difffield show a good understanding of the existing codebase and maintain its overall design integrity.libs/api/domains/regulations-admin/src/lib/graphql/models/draftRegulationChange.model.ts (1)
Line range hint
1-54: Overall assessment: The changes adhere to coding guidelines and best practices.The addition of the
difffield to theChangeAppendixclass is well-implemented and consistent with the existing codebase. The change:
- Maintains reusability across different NextJS apps.
- Correctly uses TypeScript for defining props and exporting types.
- Doesn't negatively impact tree-shaking or bundling practices.
- Adheres to the coding guidelines for files in the
libsdirectory.libs/portals/admin/regulations-admin/src/utils/formatAmendingUtils.ts (1)
Line range hint
1-116: Adherence to coding guidelines confirmed.The file adheres to the coding guidelines for the
libs/**/*pattern:
- The exported functions are reusable across different NextJS apps.
- TypeScript is used effectively for defining parameter types and return types.
- The file structure supports effective tree-shaking and bundling practices.
libs/portals/admin/regulations-admin/src/state/makeFields.ts (2)
Line range hint
1-224: Adherence to coding guidelines confirmed.The changes and the overall file structure in
makeFields.tscomply with the coding guidelines forlibs/**/*files:
- The code maintains reusability across different NextJS apps.
- TypeScript is used effectively for defining props and exporting types.
- The changes preserve effective tree-shaking and bundling practices.
156-161: LGTM! Changes are consistent and well-implemented.The modifications to the
makeDraftAppendixFormfunction are correct and align with the PR objectives. The newdiffproperty is properly handled, maintaining type safety and reusability. These changes adhere to the coding guidelines for files in thelibsdirectory.To ensure consistency across the codebase, let's verify the usage of the updated
makeDraftAppendixFormfunction:✅ Verification successful
Verified: All usages of
makeDraftAppendixFormcorrectly handle the optionaldiffproperty.The
diffproperty is optional and is appropriately handled in all instances wheremakeDraftAppendixFormis called. No issues detected with the current implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of makeDraftAppendixForm function # Expected: All calls to makeDraftAppendixForm should include the possibility of a 'diff' property rg --type typescript -A 5 'makeDraftAppendixForm\('Length of output: 88
Script:
#!/bin/bash # Description: Verify the usage of makeDraftAppendixForm function with correct file type # Expected: All calls to makeDraftAppendixForm should include the possibility of a 'diff' property rg --type ts -A 5 'makeDraftAppendixForm\('Length of output: 3423
libs/portals/admin/regulations-admin/src/state/types.ts (1)
Line range hint
1-284: Excellent TypeScript usage and structure for reusability.The file demonstrates proper use of TypeScript for defining and exporting types, which is crucial for reusability across different NextJS apps. The modular structure of types (e.g.,
AppendixDraftForm,RegDraftForm) promotes code reuse and maintainability.Key points:
- Appropriate use of TypeScript features like union types, generics, and mapped types.
- Clear and consistent naming conventions.
- Proper export of types, enhancing modularity.
- No apparent issues with tree-shaking or bundling practices.
libs/portals/admin/regulations-admin/src/components/EditBasics.tsx (3)
42-42: State variablehasUpdatedAppendixinitialized correctlyThe state variable is properly initialized and follows naming conventions.
51-51:startTextExpandedconstant correctly setThe
startTextExpandedconstant is set totrue, simplifying the initial state logic as intended.
273-273: Properly accessingapx.diff?.valueThe use of optional chaining to access
apx.diff?.valueensures that the code safely handles cases whereapx.diffmay be undefined.libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts (3)
2-6: Imports updated appropriatelyThe necessary types
AppendixDraftForm,GroupedDraftImpactForms, andRegDraftFormare imported, ensuring proper TypeScript type definitions.
17-20: Additional utility functions imported correctlyThe utility functions
hasAnyChange,isGildisTaka,removeRegPrefix, andupdateAppendixWordingare imported, which are used later in the code for text processing.
182-182: Function signature updated with optionalappendixesparameterThe
formatAmendingRegBodyfunction now includes an optionalappendixesparameter of typeAppendixDraftForm[]. This allows the function to process appendixes when provided.libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx (1)
261-264: Validate Diff Generation for AppendixesWhen mapping over
activeChange.appendixes, you're generating diffs usinggetAppendixDiffHtml(i). It's important to ensure that the diffs are correctly generated, especially when appendixes have been added or removed.Run the following script to verify that diffs are generated correctly for all appendixes:
libs/portals/admin/regulations-admin/src/utils/formatAmendingRegulation.ts
Show resolved
Hide resolved
libs/portals/admin/regulations-admin/src/components/impacts/EditChange.tsx
Show resolved
Hide resolved
thordurhhh
added
automerge
* Add change appendix diff * fix appendix type temp * fix * Fix appendix change update * minor refactor * Update EditChange.tsx --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Add diff for change appendixes.
Why
Addition requested.
Checklist:
Summary by CodeRabbit
New Features
difffield to various models and input classes, enhancing the capability to track changes in appendices.Bug Fixes
Documentation
Chores