fix(secondary school): More fixes + stop user if no programs are open for admission + handle if only 1 school or program available #17522
Conversation
WalkthroughThis pull request introduces comprehensive changes to the secondary school application template, focusing on enhancing admission and school selection processes. The modifications span multiple files and involve adding new validation logic for school admissions, updating type definitions, and improving user interface components. The changes primarily address how schools are retrieved, filtered, and displayed based on student status and admission availability, with a particular emphasis on freshman and general application types. Changes
Suggested Labels
Suggested Reviewers
Possibly Related PRs
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
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17522 +/- ##
==========================================
+ Coverage 35.58% 35.64% +0.05%
==========================================
Files 7027 7015 -12
Lines 150397 150005 -392
Branches 42924 42811 -113
==========================================
- Hits 53517 53467 -50
+ Misses 96880 96538 -342
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 108 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
libs/application/templates/secondary-school/src/lib/messages/error.ts (1)
Line range hint
8-15: Fix incorrect message ID in errorValidateCanCreateDescription.The ID references the title message instead of the description message.
errorValidateCanCreateDescription: { - id: 'ss.application:error.errorValidateCanCreateTitle', + id: 'ss.application:error.errorValidateCanCreateDescription', defaultMessage: 'Þú ert með aðra opna umsókn í gangi, vinsamlegast eyðið henni áður en opnað er nýja umsókn', description: 'Error validate can create description', },
🧹 Nitpick comments (13)
libs/application/template-api-modules/src/lib/modules/templates/secondary-school/secondary-school.service.ts (1)
55-59: Consider renaming the variable for clarity.The variable
schoolIsOpenForAdmissionholds aSecondarySchoolobject rather than a boolean value. Renaming it toavailableSchooloropenSchoolForAdmissionmay improve code readability.Suggested change:
- const schoolIsOpenForAdmission = schools.find((x) => + const availableSchool = schools.find((x) =>libs/clients/secondary-school/src/lib/secondarySchoolClient.types.ts (1)
16-17: LGTM! Consider adding JSDoc comments.The new boolean properties are well-typed and follow naming conventions. Consider adding JSDoc comments to document the purpose and usage of these properties for better maintainability.
export interface SecondarySchool { + /** Indicates if the school is accepting general admission applications */ isOpenForAdmissionGeneral: boolean + /** Indicates if the school is accepting freshman admission applications */ isOpenForAdmissionFreshman: booleanlibs/application/templates/secondary-school/src/lib/dataSchema.ts (1)
126-140: Enhance refinement with custom error messages.The schema validation is correct, but adding custom error messages would improve the user experience.
applicationType: z .object({ value: z.nativeEnum(ApplicationType), isOpenForAdmissionFreshman: z.boolean().optional(), isOpenForAdmissionGeneral: z.boolean().optional(), }) .refine( ({ value, isOpenForAdmissionFreshman, isOpenForAdmissionGeneral }) => { if (value === ApplicationType.FRESHMAN) return isOpenForAdmissionFreshman else if (value === ApplicationType.GENERAL_APPLICATION) return isOpenForAdmissionGeneral return true }, + { + message: "School is not open for the selected application type", + path: ["value"] + } ),libs/application/template-api-modules/src/lib/modules/templates/secondary-school/utils.ts (1)
119-119: LGTM! Consider using an enum for priorities.The change to start priorities from 1 instead of 0 is more intuitive. However, to improve maintainability and prevent magic numbers, consider defining an enum for priority values.
+export enum Priority { + FIRST = 1, + SECOND = 2, + THIRD = 3, +} let schoolPriority = Priority.FIRST // ... programs: [ { - priority: 1, + priority: Priority.FIRST, programId: selectionItem.firstProgram.id, }, { - priority: 2, + priority: Priority.SECOND, programId: selectionItem.secondProgram?.include ? selectionItem.secondProgram.id || '' : '', }, ]Also applies to: 137-141
libs/application/templates/secondary-school/src/forms/secondarySchoolForm/userInformationSection/personalSubSection.ts (1)
108-158: Consider extracting duplicate admission status checks.The validation logic for checking if schools are open for admission is well-implemented but contains duplicate code. Consider extracting the common logic into a helper function.
+const isSchoolOpenForAdmission = ( + externalData: object, + type: ApplicationType, +): boolean => { + const schools = getValueViaPath<SecondarySchool[]>( + externalData, + 'schools.data', + ) + return !!schools?.find( + (x) => + type === ApplicationType.FRESHMAN + ? x.isOpenForAdmissionFreshman + : x.isOpenForAdmissionGeneral, + ) +} buildHiddenInput({ id: 'applicationType.isOpenForAdmissionFreshman', condition: (_, externalData) => - !!getValueViaPath<SecondarySchool[]>( - externalData, - 'schools.data', - )?.find((x) => x.isOpenForAdmissionFreshman), + isSchoolOpenForAdmission(externalData, ApplicationType.FRESHMAN), defaultValue: true, }), buildHiddenInput({ id: 'applicationType.isOpenForAdmissionGeneral', condition: (_, externalData) => - !!getValueViaPath<SecondarySchool[]>( - externalData, - 'schools.data', - )?.find((x) => x.isOpenForAdmissionGeneral), + isSchoolOpenForAdmission(externalData, ApplicationType.GENERAL_APPLICATION), defaultValue: true, }),libs/application/templates/secondary-school/src/fields/schoolSelection/index.tsx (4)
23-26: Consider using a custom hook for school data retrieval.The schools data fetching logic could be moved to a custom hook to improve reusability and separation of concerns.
- const schools = getValueViaPath<SecondarySchool[]>( - application.externalData, - 'schools.data', - ) + const schools = useSchoolData(application.externalData)
33-38: Consider consolidating related state variables.The component has multiple related state variables for selection visibility. Consider using a single state object to manage these related states.
- const [showSecondAddRemoveButtons, setShowSecondAddRemoveButtons] = useState<boolean>(false) - const [showThirdAddRemoveButtons, setShowThirdAddRemoveButtons] = useState<boolean>(false) - const [showSecondSelection, setShowSecondSelection] = useState<boolean>(false) - const [showThirdSelection, setShowThirdSelection] = useState<boolean>(false) + const [selectionState, setSelectionState] = useState({ + showSecondAddRemoveButtons: false, + showThirdAddRemoveButtons: false, + showSecondSelection: false, + showThirdSelection: false, + })
141-143: Consider extracting the filter logic to a utility function.The filtering logic for schools could be moved to a separate utility function for better reusability and testability.
- const filteredSchools = schools?.filter((x) => - isFreshman ? x.isOpenForAdmissionFreshman : x.isOpenForAdmissionGeneral, - ) + const filteredSchools = filterAvailableSchools(schools, isFreshman)
198-243: Consider extracting selection UI into a separate component.The second selection UI block is complex and could be extracted into a reusable component to improve maintainability.
+interface SelectionBlockProps { + showSelection: boolean + showButtons: boolean + onAdd: () => void + onRemove: () => void + field: FieldBaseProps['field'] + otherFieldIds: string[] +} + +const SelectionBlock: FC<SelectionBlockProps> = ({ ... }) => { + // Extract the selection block UI here +} - {(showSecondSelection || showSecondAddRemoveButtons) && ( - <> - <Text variant="h5" marginTop={2}> - {showSecondSelection - ? formatMessage(school.secondSelection.subtitle) - : formatMessage(school.secondSelection.addSubtitle)} - </Text> - ... - </> - )} + <SelectionBlock + showSelection={showSecondSelection} + showButtons={showSecondAddRemoveButtons} + onAdd={onClickAddSecond} + onRemove={onClickRemoveSecond} + field={props.field} + otherFieldIds={[`${props.field.id}.first`, `${props.field.id}.third`]} + />libs/application/templates/secondary-school/src/fields/schoolSelection/SelectionItem.tsx (2)
38-38: Consider using derived state for second program visibility.The
showSecondProgramstate could be derived from props and other state variables instead of being managed separately.- const [showSecondProgram, setShowSecondProgram] = useState<boolean>(true) + const showSecondProgram = useMemo(() => + isFreshman && programOptions.length > 1, + [isFreshman, programOptions.length] + )
261-264: Extract program visibility logic to a custom hook.The effect for managing second program visibility could be moved to a custom hook for better reusability.
+const useSecondProgramVisibility = ( + isFreshman: boolean, + programOptions: Program[], + fieldId: string, + setValue: (path: string, value: any) => void +) => { + useEffect(() => { + const showSecondProgram = isFreshman && programOptions.length !== 1 + setValue(`${fieldId}.secondProgram.include`, showSecondProgram) + return showSecondProgram + }, [isFreshman, programOptions.length, fieldId, setValue]) +} - useEffect(() => { - const showSecondProgram = isFreshman && programOptions.length !== 1 - setValue(`${props.field.id}.secondProgram.include`, showSecondProgram) - setShowSecondProgram(showSecondProgram) - }, [isFreshman, programOptions.length, props.field.id, setValue]) + const showSecondProgram = useSecondProgramVisibility( + isFreshman, + programOptions, + props.field.id, + setValue + )libs/clients/secondary-school/src/clientConfig.json (2)
323-350: Consider adding query parameters for admission type.The new endpoint for checking open admissions could benefit from optional query parameters to check specific admission types.
Consider adding query parameters like this:
"/v1/schools/admissions/open": { "get": { "tags": ["Schools"], "summary": "[GET] Check if any programme is open for admission for any school.", + "parameters": [ + { + "name": "admissionType", + "in": "query", + "description": "Type of admission to check (general/freshman)", + "schema": { + "type": "string", + "enum": ["general", "freshman"] + } + } + ], "responses": {
934-941: Add examples to the admission status properties.The new admission status properties in SchoolDto would benefit from examples in the schema.
"anyOpenForAdmissionGeneral": { "type": "boolean", - "description": "If any programmes are open for admission" + "description": "If any programmes are open for admission", + "example": true }, "anyOpenForAdmissionFreshman": { "type": "boolean", - "description": "If any programmes are open for admission for freshman" + "description": "If any programmes are open for admission for freshman", + "example": false },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
libs/application/template-api-modules/src/lib/modules/templates/secondary-school/secondary-school.service.ts(1 hunks)libs/application/template-api-modules/src/lib/modules/templates/secondary-school/utils.ts(2 hunks)libs/application/templates/secondary-school/src/fields/schoolSelection/SelectionItem.tsx(6 hunks)libs/application/templates/secondary-school/src/fields/schoolSelection/index.tsx(5 hunks)libs/application/templates/secondary-school/src/forms/secondarySchoolForm/extraInformationSection/index.ts(2 hunks)libs/application/templates/secondary-school/src/forms/secondarySchoolForm/userInformationSection/personalSubSection.ts(5 hunks)libs/application/templates/secondary-school/src/lib/SecondarySchoolTemplate.ts(1 hunks)libs/application/templates/secondary-school/src/lib/dataSchema.ts(1 hunks)libs/application/templates/secondary-school/src/lib/messages/error.ts(1 hunks)libs/application/templates/secondary-school/src/lib/messages/extraInformation.ts(2 hunks)libs/application/templates/secondary-school/src/utils/constants/index.ts(1 hunks)libs/application/templates/secondary-school/src/utils/types/index.ts(1 hunks)libs/clients/secondary-school/src/clientConfig.json(4 hunks)libs/clients/secondary-school/src/lib/secondarySchoolClient.service.ts(1 hunks)libs/clients/secondary-school/src/lib/secondarySchoolClient.types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/secondary-school/src/lib/messages/extraInformation.ts
🧰 Additional context used
📓 Path-based instructions (14)
libs/application/templates/secondary-school/src/utils/types/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/application/templates/secondary-school/src/utils/constants/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/clients/secondary-school/src/lib/secondarySchoolClient.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/application/template-api-modules/src/lib/modules/templates/secondary-school/utils.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/secondary-school/src/lib/secondarySchoolClient.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/secondary-school/src/lib/messages/error.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/secondary-school/src/fields/schoolSelection/SelectionItem.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/secondary-school/src/lib/SecondarySchoolTemplate.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/secondary-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/template-api-modules/src/lib/modules/templates/secondary-school/secondary-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/clients/secondary-school/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/application/templates/secondary-school/src/forms/secondarySchoolForm/userInformationSection/personalSubSection.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/secondary-school/src/forms/secondarySchoolForm/extraInformationSection/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/application/templates/secondary-school/src/fields/schoolSelection/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."
📓 Learnings (1)
libs/application/templates/secondary-school/src/lib/dataSchema.ts (1)
Learnt from: veronikasif
PR: island-is/island.is#17513
File: libs/application/templates/new-primary-school/src/lib/dataSchema.ts:155-157
Timestamp: 2025-01-16T16:06:48.438Z
Learning: In Zod schemas for the new-primary-school application, optional fields that accept YES/NO values should use `z.string().optional()` instead of `z.enum([YES, NO]).optional()` as the latter doesn't work correctly for optional fields. Validation of YES/NO values should be handled through refinements.
🔇 Additional comments (8)
libs/application/template-api-modules/src/lib/modules/templates/secondary-school/secondary-school.service.ts (1)
60-68: Good use of error handling when no schools are open for admission.Throwing a
TemplateApiErrorwith a clear title and summary enhances the user experience by providing meaningful feedback.libs/application/templates/secondary-school/src/utils/constants/index.ts (1)
3-4: Language code constants added correctly.Defining
LANGUAGE_CODE_DANISHas'da'andLANGUAGE_CODE_ICELANDICas'is'improves maintainability and avoids hardcoding string literals.libs/application/templates/secondary-school/src/utils/types/index.ts (1)
16-17: New properties enhance theSecondarySchooltype.Adding
isOpenForAdmissionGeneralandisOpenForAdmissionFreshmanas boolean properties appropriately extends theSecondarySchoolinterface to include admission status.libs/application/templates/secondary-school/src/lib/messages/error.ts (1)
20-29: LGTM! Error messages are well-structured.The new error messages follow the established pattern and provide clear feedback for the no-schools-open scenario.
libs/application/templates/secondary-school/src/forms/secondarySchoolForm/extraInformationSection/index.ts (1)
56-56: LGTM! Good use of constants.Replacing the hardcoded language code with a constant improves maintainability and follows best practices.
libs/clients/secondary-school/src/lib/secondarySchoolClient.service.ts (1)
56-57: LGTM! Good addition of admission status flags.The new properties correctly handle undefined values and provide essential information about school admission status for both general and freshman applications.
libs/application/templates/secondary-school/src/forms/secondarySchoolForm/userInformationSection/personalSubSection.ts (1)
44-48: LGTM! Consistent handling of freshman status.The refactored conditions using
isFreshmanExternalDataimprove code consistency and readability.Also applies to: 58-62, 80-84
libs/application/templates/secondary-school/src/lib/SecondarySchoolTemplate.ts (1)
234-240: Verify organization reviewer submission permissions.Adding submission actions for organization reviewers is a significant workflow change. Please verify:
- This aligns with business requirements
- Proper access controls are in place
- The submission process handles both applicant and reviewer submissions correctly
Run the following script to check role permissions across the codebase:
✅ Verification successful
Organization reviewer submission permissions are properly implemented.
The implementation includes:
- Full read/write permissions for organization reviewers
- Proper state machine transitions for submissions
- Standard submission workflow consistent with other templates
- Correct assignee management through state machine actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check role permissions and access controls for organization reviewers # Check for role definitions and permissions rg -A 5 "ORGANISATION_REVIEWER" # Check for submission handling ast-grep --pattern 'ApiActions.submitApplication'Length of output: 9578
Script:
#!/bin/bash # Check for validation logic and state machine configurations rg -A 5 "validateCanCreate|validateSubmission|canSubmit" libs/application/templates/secondary-school/ # Check for state machine configurations specific to organization reviewers ast-grep --pattern 'stateMachineConfig = { $$$ }' # Look for submission middleware or handlers rg -A 5 "submitApplication.*handler|submitHandler|onSubmit"Length of output: 71828
Script:
#!/bin/bash # Check state machine configurations and role validation rg -A 10 "stateMachineConfig|stateMachineOptions" libs/application/templates/secondary-school/ # Look for role-based validation ast-grep --pattern 'role === $_ || roles.includes($_)' # Check for application state transitions rg -A 5 "transition|state.*draft|state.*submitted" libs/application/templates/secondary-school/Length of output: 5372
libs/application/templates/secondary-school/src/fields/schoolSelection/index.tsx
Show resolved
Hide resolved
libs/application/template-api-modules/src/lib/modules/templates/secondary-school/utils.ts
Show resolved
Hide resolved
...
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
Bug Fixes
Documentation
Chores