-
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
#1786 - Enable Search Student for Institutions part 2 #1887
#1786 - Enable Search Student for Institutions part 2 #1887
Conversation
andrepestana-aot
commented
Apr 13, 2023
•
edited
Loading
edited
- Added BC Public institution (COLLF) to test seed data;
- Added COLLF users to test-utils;
- Adjusted query to not get either Cancelled applications that weren't assessed or Draft applications;
- Persisted institution location at the submit time;
- Removed application update for institution location from PIR process;
- Updated comment on sims.applications.location_id;
- Added E2E testes for the search student API.
- Fixed responsiveness for the form:
- Added COLLF users to test-utils; - Adjusted query to not get Cancelled applications that weren't assessed; - Persisted institution location at the submit time; - Removed application update for institution location from PIR process; - Updated comment on application.location_id;
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/application/application.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/student/student.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/db-migrations/src/sql/Applications/Update-comment-location-id.sql
Outdated
Show resolved
Hide resolved
...backend/apps/workers/src/controllers/program-info-request/program-info-request.controller.ts
Outdated
Show resolved
Hide resolved
I am not sure whether to see an archived application done by a student came as a requirement for us. the better one to answer would be @JasonCTang and business |
.leftJoin("offering.institutionLocation", "offeringLocation") | ||
.leftJoin("offeringLocation.institution", "offeringInstitution"); | ||
.innerJoin("application.location", "institutionLocation") | ||
.innerJoin("institutionLocation.institution", "institution"); |
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.
👍
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 @andrepestana-aot
* TODO add decorator to restrict to BC Public institutions. | ||
* @param searchCriteria criteria to be used in the search. | ||
* @returns searched student details. | ||
*/ | ||
@Post("search") | ||
@HttpCode(HttpStatus.OK) |
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.
👍
let collegeFLocation: InstitutionLocation; | ||
let studentRepo: Repository<Student>; | ||
let applicationRepo: Repository<Application>; | ||
let collegeFInstitutionUserToken; |
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.
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Show resolved
Hide resolved
...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts
Show resolved
Hide resolved
@@ -154,5 +192,8 @@ export async function saveFakeApplicationDisbursements( | |||
fakeOriginalAssessment, | |||
); | |||
savedApplication.currentAssessment = savedOriginalAssessment; | |||
if (options?.applicationStatus === ApplicationStatus.Draft) { |
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.
Do we really need to make it null at this point? If so, we need also to take care of the savedApplication.currentAssessment
for instance.
If the problem is related to having an intitutionLocation associated with the offering and application location, we can also receive two locations on the relations options.
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. Few minor comments only.
sources/packages/backend/apps/api/src/services/application/application.service.ts
Show resolved
Hide resolved
Thanks for doing the changes @andrepestana-aot. Added one more comment. |
sources/packages/backend/apps/api/src/services/application/application.service.ts
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed!
|
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 @andrepestana-aot . 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.
Thanks for doing the changes and really nice coverage on the search scenarios for E2E tests.
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.
Nice work @andrepestana-aot 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.
Looks good to me 👍