feat(new-primary-school): Siblings relation column removal #17450
Conversation
WalkthroughThe pull request involves the removal of sibling relation-related functionality across multiple files in the new primary school application template. The changes primarily focus on eliminating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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: 0
🧹 Nitpick comments (2)
libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/siblingsSubSection.ts (1)
7-9: Consider removing unused imports.The changes look good, but there might be additional imports that can be removed if they were only used for the relation field functionality.
Run this script to verify unused imports:
#!/bin/bash # Check for potentially unused imports rg -l "import.*from.*constants'" | xargs ast-grep --pattern 'import { $_ } from "./constants"' -A 3libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts (1)
58-58: Consider extracting environment check to a constant.While the change is correct, consider improving readability by extracting the environment check to a named constant.
- (isRunningOnEnvironment('local') || isRunningOnEnvironment('dev')) && + const isTestEnvironment = isRunningOnEnvironment('local') || isRunningOnEnvironment('dev'); + isTestEnvironment &&
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts(1 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/Siblings.tsx(2 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/siblingsSubSection.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/dataSchema.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.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/application/templates/new-primary-school/src/fields/Review/review-groups/Siblings.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/application/templates/new-primary-school/src/lib/dataSchema.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/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/siblingsSubSection.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."
📓 Learnings (1)
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts (1)
Learnt from: birkirkristmunds
PR: island-is/island.is#16096
File: libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts:0-0
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the TypeScript service `NewPrimarySchoolService` in `libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts`, the `validChildren` array in the `getChildren` method is used for developing and testing purposes.
🔇 Additional comments (2)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/Siblings.tsx (1)
20-20: LGTM! Row data structure correctly simplified.The changes align with the PR objective, correctly simplifying the sibling information to include only the full name and national ID.
libs/application/templates/new-primary-school/src/lib/dataSchema.ts (1)
5-5: LGTM! Schema updates correctly reflect sibling data structure changes.The removal of the relation field from the siblings schema maintains type safety while aligning with the PR objective.
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17450 +/- ##
==========================================
- Coverage 35.66% 35.65% -0.01%
==========================================
Files 6977 6977
Lines 149705 149686 -19
Branches 42784 42777 -7
==========================================
- Hits 53390 53371 -19
Misses 96315 96315
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
helgifr
force-pushed
the
feat/mms-siblings-page-relation-removal
branch
from
17f36a0 to
d4f6a2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (1)
Line range hint
23-122: Consider enhancing type safety in utility functionsWhile the implementation is solid, consider these improvements for better type safety:
- Replace type assertions (
as string) with runtime type checks- Add explicit return types to functions
- Consider using TypeScript's nullish coalescing operator (??) for default values
Example for getApplicationAnswers:
export const getApplicationAnswers = ( answers: Application['answers'] ): ApplicationAnswers => { const childNationalId = getValueViaPath(answers, 'childNationalId') ?? ''; // ... rest of the implementation } type ApplicationAnswers = { childNationalId: string; // ... other return types };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/siblingsSubSection.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/constants.ts(0 hunks)libs/application/templates/new-primary-school/src/lib/dataSchema.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/application/templates/new-primary-school/src/lib/constants.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/primarySchoolSection/siblingsSubSection.ts
- libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.utils.ts
- libs/application/templates/new-primary-school/src/lib/dataSchema.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (1)
Learnt from: veronikasif
PR: island-is/island.is#16096
File: libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts:12-18
Timestamp: 2024-11-12T15:15:20.157Z
Learning: Be cautious when suggesting removal of imports; ensure all imports are thoroughly checked to confirm whether they are unused.
🔇 Additional comments (2)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (2)
Line range hint
1-20: Well-structured TypeScript implementation!The imports and type definitions are well-organized, promoting proper tree-shaking and type safety. The explicit type imports and their usage in function signatures enhance code maintainability and reusability across NextJS apps.
Line range hint
183-238: Clean and robust helper functions implementation!The helper functions demonstrate:
- Proper null safety with optional chaining
- Clean localization handling
- Pure function implementation promoting reusability
Removed relation column and also remove mock users for dev since we are getting them from Þjóðskrá
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
Summary by CodeRabbit
Release Notes
Feature Modification
User Impact