-
Notifications
You must be signed in to change notification settings - Fork 13
#4941 - Approved exception does not repeat - Student Application Form #5051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#4941 - Approved exception does not repeat - Student Application Form #5051
Conversation
…d-exception-does-not-repeat-form-definition-ft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR resolves issue #4941 by preventing duplicate exception approvals in the Student Application Form and implementing structural improvements to exception handling.
- Increased JSON payload size limit from 200KB to 300KB to accommodate larger form data
- Restructured exception fields into container components with dedicated description fields
- Updated workflow to support data from both new container and legacy field structures
- Enhanced exception note handling with better categorization and messaging
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sfaa2025-26-ft.json | Restructured all exception fields into containers with dedicated description fields and full name context |
| load-assessment-consolidated-data.bpmn | Added fallback logic to read data from both new container and legacy field structures |
| application-exception.service.ts | Changed note type from General to Application and improved exception count messaging |
| form.dto.ts | Updated JSON size validation from 200KB to 300KB limit |
| class-validator-constants.ts | Increased JSON constant from 200KB to 300KB |
| app.module.ts | Updated body parser limit to use new 300KB constant |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
...es/packages/backend/workflow/src/workflow-definitions/load-assessment-consolidated-data.bpmn
Show resolved
Hide resolved
sources/packages/forms/src/form-definitions/sfaa2025-26-ft.json
Outdated
Show resolved
Hide resolved
| "lockKey": true, | ||
| "customConditional": "show = data.studyEndDateBeforeSixWeeksFromToday || data.selectedStudyEndDateBeforeSixWeeksFromToday;", | ||
| "calculateServer": true | ||
| "customConditional": "show = !data.isProgramSectionReadOnly && !!data.selectedStudyEndDateBeforeSixWeeksFromToday;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| "type": "columns", | ||
| "columns": [ | ||
| "label": "Exception Description - Dependant's income tax", | ||
| "calculateValue": "value = `Dependant's income tax - ${data.dependants[rowIndex].fullName}`;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will row.fullName name work instead? same in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, the container adds some isolation that makes this not work.
| "type": "columns", | ||
| "columns": [ | ||
| "label": "Exception Description - Dependant's income tax", | ||
| "calculateValue": "value = `Dependant's income tax - ${data.dependants[rowIndex].fullName}`;", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a business consideration to include the date of birth to the description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so far.
sh16011993
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @andrewsignori-aot 👍 Thanks for the doubt clarification.
...es/packages/backend/workflow/src/workflow-definitions/load-assessment-consolidated-data.bpmn
Show resolved
Hide resolved
...es/packages/backend/workflow/src/workflow-definitions/load-assessment-consolidated-data.bpmn
Show resolved
Hide resolved
|
Great work and great progress with the new exception framework. Please take a look at the comments. |
|
dheepak-aot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes. Looks good 👍



Uh oh!
There was an error while loading. Please reload this page.