#5816 - Appeals/Forms history view, data connection, and institution view - History#5873
Conversation
andrewsignori-aot
added
Student
There was a problem hiding this comment.
This component is not completely new and received just minor changes after it started to be retrieved from the API.
| formSubmissionId, | ||
| studentId, | ||
| { applicationId: options?.applicationId }, | ||
| ): Promise<FormSubmissionAPIOutDTO[]> { |
There was a problem hiding this comment.
should you be using FormSubmissionsAPIOutDTO?
There was a problem hiding this comment.
This controller service is used by API endpoints that return one or multiple records.
The API endpoints are returning the specific DTOs. What would be the benefit at this moment of having this one returning the FormSubmissionsAPIOutDTO?
There was a problem hiding this comment.
Thats fine, i just spotted the different returns, i guess my confusion was why have a separate FormSubmissionsAPIOutDTO and not use the array FormSubmissionAPIOutDTO[] directly?
There was a problem hiding this comment.
We try to avoid using the direct array return as a pattern because what is an array today may be a property in the DTO that would need to be expanded in the future.
...backend/apps/api/src/route-controllers/form-submission/form-submission.controller.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/form-submission/form-submission.service.ts
Show resolved
Hide resolved
...backend/apps/api/src/route-controllers/form-submission/form-submission.controller.service.ts
Outdated
Show resolved
Hide resolved
| await this.formSubmissionControllerService.getFormSubmissions(studentId, { | ||
| includeBasicDecisionDetails: true, | ||
| keepPendingDecisionsWhilePendingFormSubmission: true, | ||
| locationIds: userToken.authorizations.getLocationsIds(), |
| ): Promise<FormSubmissionsAPIOutDTO> { | ||
| const submissions = | ||
| await this.formSubmissionControllerService.getFormSubmissions(studentId, { | ||
| includeBasicDecisionDetails: true, |
There was a problem hiding this comment.
I see for ministry that includeBasicDecisionDetails: false but true for institution. Couldn't follow that part. I believe for history, it must be false.
There was a problem hiding this comment.
This method is consumed by different API endpoints for the three clients. When the Ministry calls, this method is for rendering the history, which does not require additional notes to be returned.
There was a problem hiding this comment.
Added comments and minor refactor to getFormSubmissions hope that it helps.
|
|
||
| @AllowAuthorizedParty(AuthorizedParties.institution) | ||
| @IsBCPublicInstitution() | ||
| @HasStudentDataAccess("studentId") |
There was a problem hiding this comment.
While using the location ids, isn't this check redundant?
There was a problem hiding this comment.
The HasStudentDataAccess will ensure the student has an application under the institution, while the locations' IDs will ensure the user's access only for application-related forms associated with the locations.
The combination of both will ensure the forms with and without an application scope will be authorized.
As a general rule, I believe the @HasStudentDataAccess("studentId") should be enforced every time the student data is accessed to ensure the same minimal check every time.
| includeBasicDecisionDetails: false, | ||
| keepPendingDecisionsWhilePendingFormSubmission: false, |
There was a problem hiding this comment.
Is it not as good as not passing the options?
There was a problem hiding this comment.
Added comments and minor refactor to getFormSubmissions hope that it helps.
andrewsignori-aot
changed the title
| async getFormSubmission( | ||
| formSubmissionId: number, | ||
| options?: { studentId?: number; applicationId?: number; itemId?: number }, | ||
| options?: { studentId?: number; itemId?: number }, |
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
dheepak-aot
left a comment
dheepak-aot
left a comment
There was a problem hiding this comment.
Thanks for making the changes. Looks good 👍
andrewsignori-aot
deleted the
feature/#5190-new-unified-data-path-for-appeals-and-forms-history
branch
PR Goal
API Changes
service methodgetFormSubmissionsthat allows retrieving data for:FormSubmissionApprovalService.getFormSubmissionthat was accepting an application ID had such application ID removed to allow the data retrieval for an application or for multiple applications, executing the authorization based on the locations' IDs.Ministry
Navigation from the history
Navigation from pending list
Institution
Navigation from history
Student
Navigation from history
Navigation from application
Non-PR related changes
:fluid="true"tov-containerto fix the extra margin in some pages.