#1837 - Fix missing details on submitting Designation Request #1848
Conversation
sh16011993
added
Bug
sh16011993
requested review from
dheepak-aot,
hellolynn-tbtb,
andrewsignori-aot,
guru-aot,
ann-aot,
andrepestana-aot and
JasonCTang
sh16011993
changed the title
...src/route-controllers/designation-agreement/designation-agreement.institutions.controller.ts
Outdated
Show resolved
Hide resolved
...src/route-controllers/designation-agreement/designation-agreement.institutions.controller.ts
Outdated
Show resolved
Hide resolved
...ackages/backend/apps/api/src/route-controllers/institution/institution.controller.service.ts
Outdated
Show resolved
Hide resolved
...ackages/backend/apps/api/src/route-controllers/institution/institution.controller.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/institution/institution.service.ts
Outdated
Show resolved
Hide resolved
...end/apps/api/src/route-controllers/designation-agreement/models/designation-agreement.dto.ts
Outdated
Show resolved
Hide resolved
...end/apps/api/src/route-controllers/designation-agreement/models/designation-agreement.dto.ts
Outdated
Show resolved
Hide resolved
|
Nice work @sh16011993 . As @andrewsignori-aot mentioned, making submitted_data as nullable is better than {}. |
| * @returns Institution retrieved, if found, otherwise returns null. | ||
| */ | ||
| async getInstitutionTypeById(institutionId: number): Promise<Institution> { | ||
| return this.repo.findOne({ |
There was a problem hiding this comment.
if the where is just an id, we can use findonebyId instead of findOne, just a suggestion
There was a problem hiding this comment.
findOneById is marked as deprecated. The one to be used instead would be findOneBy but it does not seem to have the select options only the where
findOneBy(where: FindOptionsWhere<Entity> | FindOptionsWhere<Entity>[]): Promise<Entity | null>There was a problem hiding this comment.
yes just check its deprecated, yes we can use findOneBy but in this scenario, its kind of selects everything. I am okay to have it as it is.
There was a problem hiding this comment.
In that case, I will leave it as it is.
There was a problem hiding this comment.
The findOneBy here wouldn't bring the institutionType. AFAIK findOneBy doesn't allow you to specify the relations.
There was a problem hiding this comment.
Good work @sh16011993 . same comments as that @andrewsignori-aot . Curious to know, how it worked before. do you have any idea? did we remove the IsBcprivate from dry run payload or is it something else
We have added |
| @@ -81,6 +86,13 @@ export class DesignationAgreementInstitutionsController extends BaseController { | |||
| "User does not have the rights to create a designation agreement.", | |||
| ); | |||
| } | |||
|
|
|||
| // Check if institution is private or public and append it to the payload | |||
| payload.isBCPrivate = | |||
There was a problem hiding this comment.
I think that the comment sounds like there are only those 2 types: public and private but we're checking if it is "BC Private", right? It can be a private "International" institution.
There was a problem hiding this comment.
AFAIK, it is private vs "other types". Updated my comment line in the code
| * @returns Institution retrieved, if found, otherwise returns null. | ||
| */ | ||
| async getInstitutionTypeById(institutionId: number): Promise<Institution> { | ||
| return this.repo.findOne({ |
There was a problem hiding this comment.
The findOneBy here wouldn't bring the institutionType. AFAIK findOneBy doesn't allow you to specify the relations.
sh16011993
requested review from
andrewsignori-aot,
guru-aot,
ann-aot and
andrepestana-aot
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| getSQLFileData( | ||
| "Update-col-submitted_data-drop-not-null.sql", |
There was a problem hiding this comment.
Minor thing. We never use _ for sql file names. (I saw there is only one file which is exceptional).
| @@ -81,6 +86,13 @@ export class DesignationAgreementInstitutionsController extends BaseController { | |||
| "User does not have the rights to create a designation agreement.", | |||
| ); | |||
| } | |||
|
|
|||
| // Check if institution is private and append it to the payload | |||
There was a problem hiding this comment.
Please add the period to the end of sentences.
| * @param institutionId is the institution id. | ||
| * @returns boolean true if institution is private, else false. | ||
| */ | ||
| async checkIfInstitutionIsPrivate(institutionId: number): Promise<boolean> { |
There was a problem hiding this comment.
Just a minor change that I missed in the previous review, a method like this usually will be named as isPrivateInstitution, IMO.
There was a problem hiding this comment.
Please adjust also the entity model to reflect the DB change. If possible change the any to unknown unless it causes side effects.
|
Kudos, SonarCloud Quality Gate passed!
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
* #1837 - Fixed the Designation Request bug * Update code with review comments * Updated code with review comments * Updated code with review comments * Updated code with review comments * Updated code with the review comments
The following was fixed as a part of this PR:
When the private institution user submits a Designation Request as below:
The Pending and the Approved Designation Request can be viewed by the institution user with all the details properly populated.
When the ministry user logs in to approve this Designation Request, they can see all the details related to the Designation Request being properly populated.
For the public institution user, verified that their Designation Request View remains as before.