#5878 - Student Forms e2e Tests - getFormSubmission#5924
#5878 - Student Forms e2e Tests - getFormSubmission#5924andrewsignori-aot wants to merge 10 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds e2e coverage for the new getFormSubmission endpoints across clients (students/institutions/ministry) and adjusts Ministry decision-visibility rules so non-approval roles can see final decision info once a submission is completed.
Changes:
- Added e2e specs for
getFormSubmissionfor Students, Institutions, and AEST, plus a small assertion style tweak in an existing student spec. - Refactored form-submission approval-role authorization into a dedicated helper and updated controller/service mapping to use role-based decision-detail restrictions.
- Updated the Ministry UI approval component to hide decision controls for users without approval authorization.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/components/form-submissions/FormSubmissionApproval.vue | Adjusts what decision UI is rendered based on approval authorization/submission status. |
| sources/packages/backend/libs/test-utils/src/factories/form-submission.ts | Adds a richer factory helper to create submissions/items/decisions for e2e tests. |
| sources/packages/backend/libs/test-utils/src/factories/form-submission-item.ts | Extends factory to support audit user/decision array initialization. |
| sources/packages/backend/libs/test-utils/src/factories/dynamic-form-configuration.ts | Extends dynamic form configuration helpers to support more flexible setup for tests. |
| sources/packages/backend/apps/api/src/services/index.ts | Exposes the new form-submission authorization helper via barrel export. |
| sources/packages/backend/apps/api/src/services/form-submission/form-submission.models.ts | Removes embedded approval-role map (moved to authorization helper). |
| sources/packages/backend/apps/api/src/services/form-submission/form-submission.authorization.ts | Introduces shared approval-authorization helper and roles map. |
| sources/packages/backend/apps/api/src/services/form-submission/form-submission-approval.service.ts | Switches authorization checks to use the shared helper. |
| sources/packages/backend/apps/api/src/route-controllers/form-submission/form-submission.controller.service.ts | Updates decision mapping logic to restrict details based on user roles + submission status. |
| sources/packages/backend/apps/api/src/route-controllers/form-submission/form-submission.aest.controller.ts | Uses shared authorization logic and controller-service decision mapping for Ministry output. |
| sources/packages/backend/apps/api/src/route-controllers/form-submission/tests/e2e/form-submission.students.controller.getSubmissionForms.e2e-spec.ts | Minor assertion style change (toStrictEqual). |
| sources/packages/backend/apps/api/src/route-controllers/form-submission/tests/e2e/form-submission.students.controller.getFormSubmission.e2e-spec.ts | New Students e2e coverage for getFormSubmission scenarios. |
| sources/packages/backend/apps/api/src/route-controllers/form-submission/tests/e2e/form-submission.institutions.controller.getFormSubmission.e2e-spec.ts | New Institutions e2e coverage for getFormSubmission scenarios. |
| sources/packages/backend/apps/api/src/route-controllers/form-submission/tests/e2e/form-submission.aest.controller.getFormSubmission.e2e-spec.ts | New AEST e2e coverage for getFormSubmission scenarios. |
Comments suppressed due to low confidence (1)
sources/packages/backend/apps/api/src/route-controllers/form-submission/form-submission.controller.service.ts:36
- The JSDoc for
getFormSubmissionsstill referenceskeepPendingDecisionsWhilePendingFormSubmission, but this option was removed from the method signature. Please update/remove this part of the comment to avoid misleading API/controller behavior documentation.
* - `userRoles` when provided, it will be used to determine the access to the decision details
* that the consumer has based on their roles and the form category.
* - `includeBasicDecisionDetails` optional flag to include basic decision details, besides
* the decision status. Used for institutions to have access to more details than the student
* to better support them. Default to false when not provided to expose less information. When keepPendingDecisionsWhilePendingFormSubmission
* is true, the decision details will not be included while the form submission is pending to avoid showing non-final decisions
* to be exposed.
You can also share your feedback on Copilot code review. Take the survey.
sources/packages/web/src/components/form-submissions/FormSubmissionApproval.vue
Show resolved
Hide resolved
sources/packages/backend/libs/test-utils/src/factories/form-submission-item.ts
Outdated
Show resolved
Hide resolved
...rs/form-submission/_tests_/e2e/form-submission.aest.controller.getFormSubmission.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...orm-submission/_tests_/e2e/form-submission.students.controller.getFormSubmission.e2e-spec.ts
Show resolved
Hide resolved
...es/backend/apps/api/src/route-controllers/form-submission/form-submission.aest.controller.ts
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/form-submission/form-submission.authorization.ts
Outdated
Show resolved
Hide resolved
|
|
||
| // Mock the user received in the token. | ||
| await mockJWTUserInfo(appModule, formSubmission.student.user); | ||
|
|
There was a problem hiding this comment.
The mockJWTUserInfo is still there. Isn't it supposed to be removed.
There was a problem hiding this comment.
I guess I got them all removed now 😉
| ); | ||
| }); | ||
|
|
||
| it("Should get a form submission as completed, and its decision statuses, including current notes when the user does not have approval authorization.", async () => { |
There was a problem hiding this comment.
Minor. current decision notes?
There was a problem hiding this comment.
Changed.
| formCategory: formSubmission.formCategory, | ||
| status: formSubmission.submissionStatus, |
There was a problem hiding this comment.
Minor and not blocker. Form category and submission status can be used as values.
There was a problem hiding this comment.
I tried to change them all across all the files.
| const collegeFAlternativeLocation = createFakeInstitutionLocation({ | ||
| institution: collegeF, | ||
| }); | ||
| const application = await saveFakeApplication(db.dataSource, { |
There was a problem hiding this comment.
This is as good as const application = await saveFakeApplication(db.dataSource); for the test and alternative location is not required.
There was a problem hiding this comment.
Yes, it would work, but the idea was to have a location under the same institution being used for the other for the same scenario.
| formCategory: FormCategory.StudentAppeal, | ||
| status: FormSubmissionStatus.Completed, | ||
| submittedDate: formSubmission.submittedDate.toISOString(), | ||
| assessedDate: formSubmission.assessedDate?.toISOString(), |
There was a problem hiding this comment.
The optional chaining operation is not required here.
There was a problem hiding this comment.
Removed the "?".
| configurations: [ | ||
| .expect(({ body }) => | ||
| expect(body.configurations).toEqual( | ||
| expect.arrayContaining([ |
There was a problem hiding this comment.
Is there a reason to use arrayContaining?
There was a problem hiding this comment.
Yes, the DB now has other dynamic configurations added along with the tests that would be retrieved by this method. The idea was to keep the validation to ensure the PROD ones are present.
| const formSubmission = await saveFakeFormSubmissionFromInputTestData(db, { | ||
| formCategory: FormCategory.StudentAppeal, | ||
| submissionStatus: FormSubmissionStatus.Pending, | ||
| auditUser: ministryUser, |
There was a problem hiding this comment.
minor: creator should be a student
There was a problem hiding this comment.
I am changing to internally use the student for the form submission creation and audit user for the Ministry scope, for instance, while saving the decision.
| !hasFormSubmissionApprovalAuthorization( | ||
| submissionItem.dynamicFormConfiguration.formCategory, | ||
| userRoles, | ||
| ) && submissionStatus === FormSubmissionStatus.Pending; |
| ): DynamicFormConfiguration { | ||
| const dynamicFormConfiguration = new DynamicFormConfiguration(); | ||
| dynamicFormConfiguration.formType = formType; | ||
| dynamicFormConfiguration.formType = formType as DynamicFormType; |
| /** | ||
| * Gets the list of form submissions for a student, | ||
| * including the individual form items and their details. | ||
| * @param studentId student ID to retrieve the form submission history for. |
There was a problem hiding this comment.
minor: missing userToken comment definition
There was a problem hiding this comment.
We refrain from adding it. There will be many endpoints missing it.
|
Nice work. Just a few minor comments. |
|



PR Goal
Add some level of API endpoints E2E tests to all new endpoints introduced during the new forms implementation. At least one scenario for each endpoint must be present, and more scenarios can be added if time allows it. Suggestions for scenarios not covered are appreciated, but will be taken care of based on criticality and time available 😉
getFormSubmission
Ministry
Institution
Students
Fix
Ministry's approval visualization
Users without approval roles were not able to see the final decision once the form submissions were completed.
The image below is what Ministry users without approval roles will be able to see, similar to BC Public Institutions.
They have access to the decision statuses and final note only when the submission is completed, following the same idea from the BC Public Institutions.
Ministry's history view
The history visualization was also adapted to behave similarly to BC Public Institutions, where the "temporary" decision status is not displayed until the final decision is made.