feat(new-primary-school): Update key-options and api name #17809
Conversation
WalkthroughThis pull request updates the API endpoint naming across several configuration and source code files. The value of Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant FS as FriggClientService
participant XR as X-Road Service/API
C->>FS: Call getAllSchoolsByMunicipality()
FS->>XR: Request data using updated xRoadServicePath ("frigg-form-api")
XR-->>FS: Return organization list
FS-->>C: Return organization list
Possibly related PRs
Suggested labels
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)libs/application/templates/new-primary-school/src/lib/messages.ts (1)Pattern
🔇 Additional comments (5)
✨ 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
|
|
View your CI Pipeline Execution ↗ for commit 6bd02d1.
☁️ Nx Cloud last updated this comment at |
![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 (1)
libs/clients/mms/frigg/src/clientConfig.json (1)
430-432: Schema Update:FormSubmitSuccessModel
The property formerly known as"status"is now renamed to"state", and the required fields have been updated accordingly. This change should be reflected in any front-end or integration logic that relies on this schema.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
charts/islandis-services/api/values.dev.yaml(1 hunks)charts/islandis-services/api/values.prod.yaml(1 hunks)charts/islandis-services/api/values.staging.yaml(1 hunks)charts/islandis-services/application-system-api/values.dev.yaml(1 hunks)charts/islandis-services/application-system-api/values.prod.yaml(1 hunks)charts/islandis-services/application-system-api/values.staging.yaml(1 hunks)charts/islandis/values.dev.yaml(2 hunks)charts/islandis/values.prod.yaml(2 hunks)charts/islandis/values.staging.yaml(2 hunks)infra/src/dsl/xroad.ts(1 hunks)libs/application/templates/new-primary-school/src/fields/ContactsTableRepeater/index.tsx(3 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/AllergiesAndIntolerances.tsx(1 hunks)libs/application/templates/new-primary-school/src/fields/Review/review-groups/FreeSchoolMeal.tsx(1 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/allergiesAndIntolerancesSubSection.ts(2 hunks)libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/freeSchoolMealSubSection.ts(1 hunks)libs/application/templates/new-primary-school/src/lib/constants.ts(1 hunks)libs/clients/mms/frigg/project.json(1 hunks)libs/clients/mms/frigg/src/clientConfig.json(11 hunks)libs/clients/mms/frigg/src/lib/friggClient.config.ts(1 hunks)libs/clients/mms/frigg/src/lib/friggClient.service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- charts/islandis-services/api/values.prod.yaml
- infra/src/dsl/xroad.ts
- charts/islandis-services/api/values.staging.yaml
- charts/islandis/values.prod.yaml
- charts/islandis/values.dev.yaml
- charts/islandis/values.staging.yaml
🧰 Additional context used
📓 Path-based instructions (10)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/FreeSchoolMeal.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/fields/ContactsTableRepeater/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/clients/mms/frigg/src/lib/friggClient.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/forms/NewPrimarySchoolForm/differentNeedsSection/freeSchoolMealSubSection.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/mms/frigg/src/lib/friggClient.config.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/mms/frigg/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/mms/frigg/project.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/application/templates/new-primary-school/src/lib/constants.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/differentNeedsSection/allergiesAndIntolerancesSubSection.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/AllergiesAndIntolerances.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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/lib/constants.ts (2)
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/lib/constants.ts:29-31
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `libs/application/templates/new-primary-school/src/lib/constants.ts`, the enum string values in `ReasonForApplicationOptions` may match server-provided values, even if they appear to have typos. Avoid flagging these as typos.
Learnt from: veronikasif
PR: island-is/island.is#16304
File: libs/application/templates/new-primary-school/src/lib/dataSchema.ts:103-103
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/new-primary-school/src/lib/dataSchema.ts`, when comparing the `reason` field in `reasonForApplication`, which comes from the server as a string, avoid comparing it directly to `ReasonForApplicationOptions` enum values, as the enum may not include all server-provided values.
🔇 Additional comments (35)
libs/application/templates/new-primary-school/src/lib/constants.ts (2)
36-41: LGTM! Improved naming clarity.The changes to the
OptionsTypeenum improve clarity and specificity:
- Renamed
INTOLERANCEtoFOOD_ALLERGY_AND_INTOLERANCEfor better context- Added new options for school types and meals
30-30: Verify the impact of enum value change.The casing change in
SIBLINGS_IN_SAME_SCHOOLvalue from 'SiblingsInSameSchool' to 'siblingsInSameSchool' could be a breaking change if the value is used in API calls or database queries.✅ Verification successful
Verification Complete: No Impact Observed
- The search for "SiblingsInSameSchool" confirms that the updated enum value
'siblingsInSameSchool'is only defined inlibs/application/templates/new-primary-school/src/lib/constants.tsand is not referenced anywhere else as the old literal.- There are no immediate indications that API calls or database queries rely on the former casing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of the old enum value rg -i "SiblingsInSameSchool" -A 5Length of output: 560
libs/application/templates/new-primary-school/src/fields/Review/review-groups/FreeSchoolMeal.tsx (1)
37-37: LGTM! Improved semantic accuracy.Changed from
ALLERGYtoSCHOOL_MEALwhich better represents the context of the options being fetched.libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/freeSchoolMealSubSection.ts (1)
75-75: LGTM! Consistent with component changes.Changed from
ALLERGYtoSCHOOL_MEALto maintain consistency with the FreeSchoolMeal component and improve semantic accuracy.libs/application/templates/new-primary-school/src/fields/Review/review-groups/AllergiesAndIntolerances.tsx (1)
44-44: LGTM! Improved naming clarity.Changed from
INTOLERANCEtoFOOD_ALLERGY_AND_INTOLERANCEwhich better describes the type of data being fetched.libs/application/templates/new-primary-school/src/forms/NewPrimarySchoolForm/differentNeedsSection/allergiesAndIntolerancesSubSection.ts (3)
62-62: LGTM! The updated option type better reflects the field's purpose.The change from
INTOLERANCEtoFOOD_ALLERGY_AND_INTOLERANCEimproves clarity by accurately representing both food allergies and intolerances in a single field.
91-91: LGTM! The option type correctly represents non-food allergies.Keeping
OptionsType.ALLERGYfor this field maintains a clear distinction between food-related and other types of allergies.
1-206: Great implementation following best practices!The code demonstrates:
- Strong TypeScript usage with proper type imports
- Modular and reusable form building components
- Effective conditional rendering for form fields
libs/clients/mms/frigg/src/clientConfig.json (19)
28-29: Security and Tag Declaration for/formsEndpoint
The addition of the"security": [{ "bearer": [] }]and"tags": ["Frigg"]properties ensures that calls to the/formsendpoint are secured and properly categorized. Please verify that the bearer security scheme is correctly defined in the components and that downstream consumers expect these tags.
35-49: Enhancements for/forms/typesEndpoint
The modifications include updating the"operationId"to"getFormTypes", and the addition of"security","summary", and"tags"properties. These changes improve API documentation and security consistency. Ensure that consumers of this API are updated regarding the new operation identifier and description.
55-55: Operation Identifier Update for/key-options
The"operationId"is updated to"getAllKeyOptions". This helps in uniquely identifying this operation in API tooling and client generation.
78-78: Added Summary for/key-optionsEndpoint
Including the"summary": "Get all key options."property enhances the self-documentation of the API. This addition aligns with similar endpoints for better consistency.
85-85: Operation Identifier Update for/key-options/types
The"operationId"is now set to"getKeyOptionsTypes", ensuring unique identification for this endpoint. Verify that any client or server code referencing the old identifier is updated.
97-97: Added Summary for/key-options/typesEndpoint
By adding"summary": "Get list of types of key options", the endpoint is now better documented, offering a clear description for API consumers.
112-146: Detailed Query Parameters for/schoolsEndpoint
The update to the/schoolsGET endpoint now includes an operation identifier and a comprehensive set of query parameters (sort,orderBy,limit,offset) with detailed descriptions and schema definitions. This facilitates greater flexibility in data retrieval and pagination. Please ensure that these descriptions meet your API’s internationalization and validation requirements.
158-167: Enhanced Response for/schoolsEndpoint
The responses section now includes a"default"response with headers (specifically"x-total-count") to indicate the total number of records available. Verify that the header and its schema conform to your API specifications and that backend implementations return the expected header values.
169-170: Summary and Tag Addition for/schoolsEndpoint
The addition of"summary": "Get all schools."and"tags": ["Frigg"]boosts endpoint documentation and helps in categorizing the endpoint correctly.
176-176: Security and Descriptive Enhancements for/students/{nationalId}Endpoint
Updates include ensuring the"operationId"is set to"getUserBySourcedId", and adding both the"security": [{ "bearer": [] }]and"summary": "Get user by nationalId"properties along with the"tags"declaration. These adjustments improve API clarity and enforce security.Also applies to: 195-197
200-269: New Endpoint:/registrations/{orgId}
A new endpoint is introduced for retrieving registration data by organization ID. It includes detailed descriptions, query parameters for sorting/pagination, security settings, and a reference to theRegistrationModelin its response schema. Ensure that the naming conventions and parameter descriptions align with the overall API design and that your backend supports these parameters appropriately.
271-304: New Endpoint:/registrations/{id}/{orgId}
This endpoint provides registration details filtered by both a unique ID and an organization ID. The clear parameter definitions and associated response schema (RegistrationDetailModel) enhance the API’s expressiveness. Verify that the nested parameter structure is intuitive for API consumers.
487-492: Schema Update:AddressModel
TheAddressModelnow uses"address"and"postCode"instead of the previous names. Additionally, the"required"array has been updated to reflect these changes. Please ensure that all client code referencing these fields is updated to match the new property names.
582-584: Type Inconsistency inAgentModel
The"phone"and"email"properties in theAgentModelare now defined with type"object", which contrasts with similar schemas (such asAgentDtowhere"email"is a string). Please double-check if these types are intentionally set to"object"(perhaps for a complex structure) or if they should remain as simple strings for consistency.
645-672: New Schema:RegistrationModel
A comprehensive schema forRegistrationModelhas been added, including properties such as"id","nameOfChild","nationalId", and others with corresponding references toMembershipOrganizationModel. Ensure that these required fields adequately capture the registration data needed by the client and backend services.
674-715: New Schema:ReviewActionModel
The addition of theReviewActionModelenriches the API with a detailed structure for review actions. The use of enumerated values for"action"and"status"helps in enforcing valid state transitions. Confirm that the enumerations align with the business requirements.
716-740: New Schema:ReviewModel
This schema defines the structure for a review, including an array of actions and timestamps for creation and update. Verify that the"displayStatus"and"state"values are consistent with the client’s presentation logic and that the required fields are comprehensive for the review process.
741-749: New Schema:LanguageModel
TheLanguageModelschema is now available to capture language-related details, requiring"nativeLanguage","noIcelandic", and"otherLanguages". Ensure that this model meets your multilingual and localization requirements.
750-788: New Schema:RegistrationDetailModel
This schema aggregates detailed information for a registration, including nested references toReviewModel,UserModel, an array ofAgentModel, andLanguageModel. This comprehensive structure is key for endpoints that deliver full registration details. Please verify that all nested references resolve correctly and that the schema fully covers the necessary data for downstream processing.libs/clients/mms/frigg/src/lib/friggClient.config.ts (1)
16-16: LGTM! API path update is consistent.The update from 'frigg-form-service' to 'frigg-form-api' aligns with the broader API naming convention changes.
libs/clients/mms/frigg/src/lib/friggClient.service.ts (1)
29-29: Verify the necessity of empty object parameter.The addition of an empty object parameter to
getAllSchoolsByMunicipalitycall might indicate an API requirement change. Please confirm if this is intentional and required by the updated API.✅ Verification successful
Empty Object Parameter Is Intentional and Required
- The service method in
friggClient.service.tscalls the API asthis.friggApiWithAuth(user).getAllSchoolsByMunicipality({}), which adds the required empty object parameter.- The GraphQL resolver in
frigg.resolver.tscalls the service method without supplying this parameter, indicating that the empty object is intentionally injected in the service layer to meet the updated API's expected signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of getAllSchoolsByMunicipality to verify consistency ast-grep --pattern 'getAllSchoolsByMunicipality($$$)'Length of output: 55
Script:
#!/bin/bash # Search for the definition/signature of getAllSchoolsByMunicipality in TypeScript files rg --type ts -n "getAllSchoolsByMunicipality\s*\(" .Length of output: 464
libs/application/templates/new-primary-school/src/fields/ContactsTableRepeater/index.tsx (1)
55-55: LGTM! Improved semantic meaning in test identifiers.The update from 'relative-' to 'contact-' in dataTestId values better reflects the component's purpose and improves test readability.
Also applies to: 64-64, 73-73, 81-81
libs/clients/mms/frigg/project.json (1)
25-25: Verify OpenAPI endpoint accessibility.The update to use 'frigg-form-api' in the OpenAPI document URL is consistent with the API naming changes. However, please verify the endpoint accessibility.
Consider adding error handling.
Consider adding error handling and retries to the curl command for better reliability.
- "curl -H \"X-Road-Client: IS-DEV/GOV/10000/island-is-client\" http://localhost:8081/r1/IS-DEV/GOV/10066/MMS-Protected/getOpenAPI?serviceCode=frigg-form-api > src/clientConfig.json", + "curl --retry 3 --retry-delay 2 --fail -H \"X-Road-Client: IS-DEV/GOV/10000/island-is-client\" http://localhost:8081/r1/IS-DEV/GOV/10066/MMS-Protected/getOpenAPI?serviceCode=frigg-form-api > src/clientConfig.json || (echo 'Failed to fetch OpenAPI document' && exit 1)",charts/islandis-services/application-system-api/values.dev.yaml (1)
106-106: Development Environment – Updated API Endpoint Naming
TheXROAD_MMS_FRIGG_PATHvariable has been correctly updated to use the new API-oriented identifierfrigg-form-apiinstead of the legacy service-oriented name. This change aligns with the overall architectural update and maintains consistency across all environment configurations.charts/islandis-services/application-system-api/values.prod.yaml (1)
106-106: Production Environment – Frigg API Endpoint Update
The production configuration now reflects the revised endpoint by updatingXROAD_MMS_FRIGG_PATHtofrigg-form-api. This adjustment ensures consistency with the new API naming convention across all environments.charts/islandis-services/application-system-api/values.staging.yaml (1)
106-106: Staging Environment – Consistent API Endpoint Naming
The updated value forXROAD_MMS_FRIGG_PATHin staging now usesfrigg-form-apiinstead of the old service name. Please verify that any dependent systems and references have been updated accordingly for full consistency.charts/islandis-services/api/values.dev.yaml (1)
123-123: API Service Configuration – Frigg Endpoint Revision
The API configuration now updates theXROAD_MMS_FRIGG_PATHvariable to usefrigg-form-api, aligning with the changes in other configuration files. Ensure that all downstream service calls referencing this variable are updated to prevent mismatches.
[TS-971] Update key-options and api name
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
Chores
Documentation