-
Notifications
You must be signed in to change notification settings - Fork 14
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
#1851 - Enable File Upload for Public Institution User #1893
#1851 - Enable File Upload for Public Institution User #1893
Conversation
- Remove upload file button. - Disable links to download/view the file. - Wrote E2E tests.
sources/packages/backend/libs/test-utils/src/factories/student-file-uploads.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/test-utils/src/factories/student-file-uploads.ts
Outdated
Show resolved
Hide resolved
...ollers/student/_tests_/e2e/student.institutions.controller.getStudentFileUploads.e2e-spec.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/route-controllers/student/models/student.dto.ts
Outdated
Show resolved
Hide resolved
...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts
Outdated
Show resolved
Hide resolved
sources/packages/web/src/components/common/students/StudentFileUploads.vue
Outdated
Show resolved
Hide resolved
sources/packages/web/src/components/common/students/StudentFileUploads.vue
Outdated
Show resolved
Hide resolved
...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts
Outdated
Show resolved
Hide resolved
...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,206 @@ | |||
<template> | |||
<tab-container> |
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 moving it to components.
sources/packages/web/src/components/common/students/StudentFileUploads.vue
Show resolved
Hide resolved
* Get all student documents for Institution user. | ||
* @return list of student documents. | ||
*/ | ||
async getInstitutionStudentFiles( |
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.
As per https://github.com/bcgov/SIMS/pull/1893/files#r1171588207 we can avoid creating a new method.
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.
sources/packages/web/src/components/common/students/StudentFileUploads.vue
Outdated
Show resolved
Hide resolved
...ollers/student/_tests_/e2e/student.institutions.controller.getStudentFileUploads.e2e-spec.ts
Show resolved
Hide resolved
Thanks for making the changes. Added more comments. |
sources/packages/backend/apps/api/src/route-controllers/student/student.controller.service.ts
Show resolved
Hide resolved
sources/packages/web/src/components/common/students/StudentFileUploads.vue
Outdated
Show resolved
Hide resolved
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.
Please take a look at a few minor comments. I believe that the below one is the one that requires more attention #1893 (comment)
}; | ||
|
||
const uploadFile = () => { | ||
context.emit("uploadFile", fileUploadModal, () => |
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.
I didn't follow why are we emitting back to parent here?
I technically what happen between here and parent and feel here that coupling is tight, but are we going to follow this approach moving forward?
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.
I have updated this part of the code as suggested by @andrewsignori-aot
Instead of passing the entire fileUploadModal
as argument, only associatedFiles
array is being passed to the emitted event handler. We are emitting uploadFile
to the AEST specific file upload component to allow for the API call saveAESTUploadedFilesToStudent
to be made by the consumer since it is a consumer specific operation.
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.
@dheepak-aot considering also the last explanation given by @sh16011993 is there anything here to be considered as a new approach?
If the point was about the modal as a parameter, that was exactly what I asked to be changed but besides that, is there anything else?
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.
No I am good.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Good work @sh16011993 👍 Thanks for doing the changes
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.
Pls take a look at the failing test cases
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 doing the changes, looks good 👍
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.
LGTM
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.
LGTM, nice work @sh16011993
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 job!
The following tasks were completed as a part of this task:
Screenshot showing the removed upload file button and disabled links to download/view the file: