#1786 - Enable Search Student for Institutions - part 1 #1857

andrepestana-aot · 2023-04-01T00:32:43Z

In this PR

moved SearchStudents.vue to SearchStudentsCommon.vue to reuse the same UI for institutions and ministry users;
added link for BC Public institutions;
created API to search students by institutions;

I will create another PR to:

create e2e tests
fix bug on UI (responsiveness)
check an issue with the search when the user application has any pending exception. In that case the location_id is not defined and the user won't be found by the institution.

BC Public Institution

Other Institutions

guru-aot · 2023-04-05T16:10:40Z

...es/packages/backend/apps/api/src/route-controllers/student/student.institution.controller.ts

+@AllowAuthorizedParty(AuthorizedParties.institution)
+@Controller("student")
+@ApiTags(`${ClientTypeBaseRoute.Institution}-student`)
+@Injectable()


Do we need an injectable here?

When @Controller is used, @Injectable is not required.

looks like its used in quite a few controllers, here are few

I just moved this code, it already this way. Thanks!

@Controller does not require the @Injectable because they are not injected anywhere. They were probably added by mistake someday and kept being added. Anyone that finds one can have them removed or create a PR to remove them all.

I just moved this code, it already this way. Thanks!

I did not get the part that states that this code was "moved". This is a brand-new file, right?
If the file is new and the code was copied, the statement "it already this way" does not seem accurate, IMO.

It was already that way in the other file, I just moved the code from there to this one. I'm not saying that I should not have changed it and made it right. Sorry!

guru-aot · 2023-04-05T16:16:28Z

sources/packages/backend/apps/api/src/services/institution/institution.service.ts

+   * @param institutionId institution id.
+   * @returns whether the institution type is public.
+   */
+  async isBCPublicInstitution(institutionId: number): Promise<boolean> {


Just a suggestion, we already have a method to check if the institution is private, which has one reference. Can we just create a single method that return either the institution is INSTITUTION_TYPE_BC_PRIVATE or INSTITUTION_TYPE_BC_PUBLIC?. Thoughts @ann-aot @andrewsignori-aot @sh16011993 @dheepak-aot

I see the point @guru-aot.
Maybe we could create read-only properties a the entity model that will do the check based on the constants INSTITUTION_TYPE_BC_PRIVATE and INSTITUTION_TYPE_BC_PUBLIC.
IMO this is something that can be considered an entity-level concern. Would be like an emulation of a DB computed property.

I agree that it should just be a entity level concern.

@guru-aot I get what you are saying, but to have a single method returning private or public, we need to have a thrid category to pull all these institution types.

I get the point team, I was just talking about combining 2 methods and returning a single value either public or private. I guess its going way out of the scope of this PR. Pausing the discussion for now.

@guru-aot I am exactly trying to comprehend your idea. for e.g. if there is a method called checkIfPrivateOrPublic(): Private | Public, what will it return if the institution type is International? let me know if I am missing somehthing.

@guru-aot feel free to pause the discussion, from your end, please leave the comment open.
@andrepestana-aot I believe that this method will no longer be needed because we will build the decorator in the future.
If it is still needed let's move it to the entity model alongside the isBCPrivate.

I removed the method isBCPublicInstitution

guru-aot · 2023-04-05T16:17:57Z

sources/packages/backend/apps/api/src/services/student/student.service.ts

+      .leftJoin("studentAssessment.offering", "offering")
+      .leftJoin("offering.institutionLocation", "offeringLocation")
+      .leftJoin("offeringLocation.institution", "offeringInstitution")
+


please remove empty line here and above.

sources/packages/backend/apps/api/src/app.institutions.module.ts

sources/packages/backend/apps/api/src/route-controllers/index.ts

guru-aot · 2023-04-05T16:24:03Z

sources/packages/backend/apps/api/src/services/student/student.service.ts

+   * lastName, appNumber or sin.
+   * @returns list of students.
+   */
+  async searchStudentApplicationForInstitution(


Cant we reuse the existing query searchStudentApplication to use the instituion id as an optional parameter and return?

I don't like the idea of doing that because it would make a simple thing like a query to a more complex method with conditions that will be harder to maintain. Thoughts @ann-aot @dheepak-aot @andrewsignori-aot @sh16011993 ?

Maybe I am wrong and I did not tested it, but would the change be something about moving the inner/left and one where inside an if like below?

if (institutionId) { searchQuery .innerJoin("application.location", "pirLocation") .innerJoin("pirLocation.institution", "pirInstitution") .andWhere( "(offeringInstitution.id = :institutionId or pirInstitution.id = :institutionId)", { institutionId }, ); }

Ok. Changed.

I noticed the if (institutionId) twice in the method. Was it because the proposed code on the comment didn't work or because it looks cleaner?

sources/packages/backend/apps/api/src/route-controllers/student/models/student.dto.ts

ann-aot · 2023-04-05T16:25:14Z

sources/packages/backend/apps/api/src/route-controllers/student/models/student.dto.ts

@@ -93,27 +93,27 @@ export class UpdateStudentAPIInDTO
 /**
 * Student AEST search parameters.


pls update the comment too

...es/packages/backend/apps/api/src/route-controllers/student/student.institution.controller.ts

ann-aot · 2023-04-05T18:08:44Z

...es/packages/backend/apps/api/src/route-controllers/student/student.institution.controller.ts

+  StudentSearchAPIInDTO,
+  SearchStudentAPIOutDTO,
+} from "./models/student.dto";
+


pls remove the extra line

andrewsignori-aot · 2023-04-05T18:29:56Z

...es/packages/backend/apps/api/src/route-controllers/student/student.institution.controller.ts

+        userToken.authorizations.institutionId,
+        searchCriteria,
+      );
+    return searchStudentApplications.map((eachStudent: Student) => ({


If AEST and student are sharing the same DTO, the "map" logic can be centralized also on student.controller.service.ts.

sources/packages/backend/apps/api/src/services/student/student.service.ts

andrewsignori-aot · 2023-04-05T19:00:36Z

sources/packages/backend/apps/api/src/services/student/student.service.ts

+        "application.student.id = student.id",
+      )
+
+      .innerJoin("application.location", "pirLocation")


Maybe it would be safe to use left join here since we still are not saving the PIR location right on submission.
Even if we start saving, I believe that it would be better to not rely on it using the INNER.

sources/packages/web/src/components/common/SearchStudentsCommon.vue

dheepak-aot · 2023-04-06T17:02:32Z

...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts

+
+  /**
+   * Search students based on the search criteria.
+   * TODO add decorator to restrict to BC Public institutions.


dheepak-aot · 2023-04-06T17:10:30Z

sources/packages/web/src/components/common/SearchStudents.vue

+                color="primary"
+                class="p-button-raised"
+                data-cy="viewStudent"
+                @click="$emit('goToStudentView', slotProps.data.id)"


As an alternate approach because the params are same between both routes, the component can receive a a route prop and do the navigation here.
The benefit it is avoid the emit. <search-students :view-student-route="InstitutionRoutesConst.STUDENT_PROFILE">

Just a suggestion.

Yes, there are many ways of doing that. I'm following #1857 (review)

dheepak-aot · 2023-04-06T17:20:03Z

sources/packages/backend/apps/api/src/services/student/student.service.ts

+          "application",
+          "application.student.id = student.id",
+        )
+        .leftJoin("application.location", "pirLocation")


Are we changing the submit logic to save the location in a different PR?

And if that is going to happen, this whole part can be removed right?

.leftJoin("application.currentAssessment", "studentAssessment") .leftJoin("studentAssessment.offering", "offering") .leftJoin("offering.institutionLocation", "offeringLocation") .leftJoin("offeringLocation.institution", "offeringInstitution");

Yes, I'm changing the submit logic to save the location in a different PR.

Are we changing the submit logic to save the location in a different PR?

And if that is going to happen, this whole part can be removed right?

.leftJoin("application.currentAssessment", "studentAssessment") .leftJoin("studentAssessment.offering", "offering") .leftJoin("offering.institutionLocation", "offeringLocation") .leftJoin("offeringLocation.institution", "offeringInstitution");

During previous conversations, we had in mind doing both checks, but since we will save the application.location during the application submission and assuming that the location can only be changed by editing the application, it could be a nice simplification of the query.

dheepak-aot · 2023-04-06T17:23:29Z

Thanks for making the changes. Just one outstanding question.

andrewsignori-aot · 2023-04-06T17:45:10Z

...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts

+export class StudentInstitutionsController extends BaseController {
+  constructor(
+    private readonly studentService: StudentService,
+    private readonly institutionService: InstitutionService,


Please remove the institutionService if it is no longer used.

andrewsignori-aot · 2023-04-06T17:49:27Z

...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts

+    @UserToken() userToken: IInstitutionUserToken,
+    @Body() searchCriteria: StudentSearchAPIInDTO,
+  ): Promise<SearchStudentAPIOutDTO[]> {
+    return await this.studentService.searchStudentApplication(


No need to await.

andrewsignori-aot · 2023-04-06T17:51:20Z

...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts

+    @UserToken() userToken: IInstitutionUserToken,
+    @Body() searchCriteria: StudentSearchAPIInDTO,
+  ): Promise<SearchStudentAPIOutDTO[]> {
+    return await this.studentService.searchStudentApplication(


Maybe I am missing something but is the service result directly returned through the API?
The results must be converted to a DTO.

Now I see that the service is returning the DTO directly. The services, unless they are controller services, do not return DTO directly. API DTOs are supposed to be a controller concern only, that is why we usually deal with them at controller or service controller only.

andrewsignori-aot · 2023-04-06T17:53:02Z

sources/packages/backend/apps/api/src/route-controllers/student/student.aest.controller.ts

-      birthDate: getISODateOnlyString(eachStudent.birthDate),
-      sin: eachStudent.sinValidation.sin,
-    }));
+    return await this.studentService.searchStudentApplication(searchCriteria);


Same comments from https://github.com/bcgov/SIMS/pull/1857/files#r1160095243

sh16011993

@andrepestana-aot Looks good to me.

dheepak-aot

Thanks for doing the changes. LGTM 👍

guru-aot

LGTM, nice work @andrepestana-aot

sources/packages/backend/apps/api/src/route-controllers/institution/models/institution.dto.ts

andrewsignori-aot

Thanks for doing the changes, looks good 👍

sonarqubecloud · 2023-04-06T21:14:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

github-actions · 2023-04-06T21:15:32Z

Backend Unit Tests Coverage Report

Totals
Statements:	17.99% ( 1960 / 10892 )
Methods:	8.12% ( 115 / 1417 )
Lines:	20.71% ( 1716 / 8286 )
Branches:	10.85% ( 129 / 1189 )

github-actions · 2023-04-06T21:15:45Z

E2E Workflow Workers Coverage Report

Totals
Statements:	32.41% ( 176 / 543 )
Methods:	21.25% ( 17 / 80 )
Lines:	39.25% ( 157 / 400 )
Branches:	3.17% ( 2 / 63 )

github-actions · 2023-04-06T21:16:20Z

E2E Queue Consumers Coverage Report

Totals
Statements:	56.72% ( 308 / 543 )
Methods:	46.38% ( 32 / 69 )
Lines:	59.48% ( 276 / 464 )
Branches:	0% ( 0 / 10 )

github-actions · 2023-04-06T21:19:29Z

E2E SIMS API Coverage Report

Totals
Statements:	38.08% ( 2580 / 6775 )
Methods:	29.82% ( 263 / 882 )
Lines:	43.7% ( 2208 / 5053 )
Branches:	12.98% ( 109 / 840 )

andrepestana-aot added 4 commits March 31, 2023 17:32

initial commit

a48a1cf

removed unused import

3a1a3b3

changed SearchStudents from ministry to use the same component

946ac02

reverted conf to display queries on logs

0640a2b

andrepestana-aot changed the title ~~Draft~~ #1786 - Enable Search Student for Institutions - part 1 Apr 4, 2023

andrepestana-aot self-assigned this Apr 4, 2023

andrepestana-aot marked this pull request as ready for review April 4, 2023 22:25

andrepestana-aot requested review from sh16011993, guru-aot, dheepak-aot, andrewsignori-aot, ann-aot and JasonCTang April 4, 2023 22:25

andrepestana-aot added User Story Institution Institution Features labels Apr 4, 2023

guru-aot reviewed Apr 5, 2023

View reviewed changes

dheepak-aot reviewed Apr 5, 2023

View reviewed changes

sources/packages/backend/apps/api/src/app.institutions.module.ts Outdated Show resolved Hide resolved

dheepak-aot reviewed Apr 5, 2023

View reviewed changes

sources/packages/backend/apps/api/src/route-controllers/index.ts Outdated Show resolved Hide resolved

guru-aot reviewed Apr 5, 2023

View reviewed changes

dheepak-aot reviewed Apr 5, 2023

View reviewed changes

sources/packages/backend/apps/api/src/route-controllers/student/models/student.dto.ts Outdated Show resolved Hide resolved

ann-aot reviewed Apr 5, 2023

View reviewed changes

dheepak-aot reviewed Apr 5, 2023

View reviewed changes

...es/packages/backend/apps/api/src/route-controllers/student/student.institution.controller.ts Outdated Show resolved Hide resolved

ann-aot reviewed Apr 5, 2023

View reviewed changes

andrewsignori-aot reviewed Apr 5, 2023

View reviewed changes

sources/packages/backend/apps/api/src/services/student/student.service.ts Outdated Show resolved Hide resolved

andrewsignori-aot reviewed Apr 5, 2023

View reviewed changes

sources/packages/web/src/components/common/SearchStudentsCommon.vue Outdated Show resolved Hide resolved

andrewsignori-aot reviewed Apr 5, 2023

View reviewed changes

sources/packages/web/src/components/common/SearchStudentsCommon.vue Outdated Show resolved Hide resolved

review issues

c475d9f

dheepak-aot reviewed Apr 6, 2023

View reviewed changes

andrewsignori-aot reviewed Apr 6, 2023

View reviewed changes

sh16011993 approved these changes Apr 6, 2023

View reviewed changes

andrepestana-aot added 3 commits April 6, 2023 11:12

review issues

ac40949

review issues

8d5da8d

review issues

624aa25

dheepak-aot approved these changes Apr 6, 2023

View reviewed changes

guru-aot approved these changes Apr 6, 2023

View reviewed changes

andrewsignori-aot reviewed Apr 6, 2023

View reviewed changes

sources/packages/backend/apps/api/src/route-controllers/institution/models/institution.dto.ts Outdated Show resolved Hide resolved

andrepestana-aot added 2 commits April 6, 2023 13:39

review issues

aff6447

review issues

302f762

andrewsignori-aot approved these changes Apr 6, 2023

View reviewed changes

review issues

26873d3

andrepestana-aot merged commit 007adef into main Apr 6, 2023

andrepestana-aot temporarily deployed to DEV April 6, 2023 21:31 — with GitHub Actions Inactive

andrepestana-aot deleted the 1786_enable_search_student_for_institutions branch April 6, 2023 21:31

andrepestana-aot temporarily deployed to DEV April 6, 2023 21:32 — with GitHub Actions Inactive

andrepestana-aot temporarily deployed to DEV April 6, 2023 21:42 — with GitHub Actions Inactive

		@@ -93,27 +93,27 @@ export class UpdateStudentAPIInDTO
		/**
		* Student AEST search parameters.

#1786 - Enable Search Student for Institutions - part 1 #1857

#1786 - Enable Search Student for Institutions - part 1 #1857

Conversation

andrepestana-aot commented Apr 1, 2023 • edited Loading

BC Public Institution

Other Institutions

guru-aot Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guru-aot Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheepak-aot Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dheepak-aot commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sh16011993 left a comment

Choose a reason for hiding this comment

dheepak-aot left a comment

Choose a reason for hiding this comment

guru-aot left a comment

Choose a reason for hiding this comment

andrewsignori-aot left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 6, 2023

github-actions bot commented Apr 6, 2023

Backend Unit Tests Coverage Report

github-actions bot commented Apr 6, 2023

E2E Workflow Workers Coverage Report

github-actions bot commented Apr 6, 2023

E2E Queue Consumers Coverage Report

github-actions bot commented Apr 6, 2023

E2E SIMS API Coverage Report

andrepestana-aot commented Apr 1, 2023 •

edited

Loading

guru-aot Apr 5, 2023 •

edited

Loading

guru-aot Apr 5, 2023 •

edited

Loading

dheepak-aot Apr 6, 2023 •

edited

Loading