-
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
#1837 - Fix missing details on submitting Designation Request #1848
Changes from 4 commits
14df4a9
5f4f95d
118aa41
13501f6
cc5be85
2057e67
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,11 @@ import { | |
IsInstitutionAdmin, | ||
UserToken, | ||
} from "../../auth/decorators"; | ||
import { DesignationAgreementService, FormService } from "../../services"; | ||
import { | ||
DesignationAgreementService, | ||
FormService, | ||
InstitutionService, | ||
} from "../../services"; | ||
import { | ||
DesignationAgreementAPIOutDTO, | ||
DesignationAgreementDetailsAPIOutDTO, | ||
|
@@ -45,6 +49,7 @@ export class DesignationAgreementInstitutionsController extends BaseController { | |
constructor( | ||
private readonly designationAgreementService: DesignationAgreementService, | ||
private readonly formService: FormService, | ||
private readonly institutionService: InstitutionService, | ||
private readonly designationAgreementControllerService: DesignationAgreementControllerService, | ||
) { | ||
super(); | ||
|
@@ -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 | ||
payload.isBCPrivate = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK, it is private vs "other types". Updated my comment line in the code |
||
await this.institutionService.checkIfInstitutionIsPrivate( | ||
userToken.authorizations.institutionId, | ||
); | ||
|
||
// Validate the dynamic data submission. | ||
const submissionResult = await this.formService.dryRunSubmission( | ||
FormNames.DesignationAgreementDetails, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,7 @@ import { | |
INSTITUTION_USER_ALREADY_EXISTS, | ||
LEGAL_SIGNING_AUTHORITY_EXIST, | ||
} from "../../constants"; | ||
import { INSTITUTION_TYPE_BC_PRIVATE } from "@sims/sims-db/constant"; | ||
import { InstitutionUserAuthService } from "../institution-user-auth/institution-user-auth.service"; | ||
import { UserService } from "../user/user.service"; | ||
|
||
|
@@ -694,6 +695,37 @@ export class InstitutionService extends RecordDataModelService<Institution> { | |
.getOne(); | ||
} | ||
|
||
/** | ||
* Check if institution is private | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a minor change that I missed in the previous review, a method like this usually will be named as |
||
const institutionType = await this.getInstitutionTypeById(institutionId); | ||
return INSTITUTION_TYPE_BC_PRIVATE === institutionType.institutionType.id; | ||
} | ||
|
||
/** | ||
* Get the institutionType by institution id. | ||
* @param institutionId Institution id. | ||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the where is just an id, we can use findonebyId instead of findOne, just a suggestion There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
findOneBy(where: FindOptionsWhere<Entity> | FindOptionsWhere<Entity>[]): Promise<Entity | null> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, I will leave it as it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The findOneBy here wouldn't bring the institutionType. AFAIK findOneBy doesn't allow you to specify the relations. |
||
select: { | ||
institutionType: { | ||
id: true, | ||
}, | ||
}, | ||
relations: { | ||
institutionType: true, | ||
}, | ||
where: { | ||
id: institutionId, | ||
}, | ||
}); | ||
} | ||
|
||
/** | ||
* Get the basic info of the institution by ID. | ||
* @param institutionId Institution id. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { MigrationInterface, QueryRunner } from "typeorm"; | ||
import { getSQLFileData } from "../utilities/sqlLoader"; | ||
|
||
export class DesignationAgreementsTableAlterSubmittedData1680120153698 | ||
implements MigrationInterface | ||
{ | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor thing. We never use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
"DesignationAgreements", | ||
), | ||
); | ||
} | ||
|
||
public async down(queryRunner: QueryRunner): Promise<void> { | ||
await queryRunner.query( | ||
getSQLFileData( | ||
"Update-col-submitted_data-add-not-null.sql", | ||
"DesignationAgreements", | ||
), | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
-- Updated the submitted_data column from NULL to NOT NULL | ||
ALTER TABLE | ||
sims.designation_agreements | ||
ALTER COLUMN | ||
submitted_data | ||
SET | ||
NOT NULL; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
-- Updated the submitted_data column from NOT NULL to NULL | ||
ALTER TABLE | ||
sims.designation_agreements | ||
ALTER COLUMN | ||
submitted_data DROP NOT NULL; |
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 add the period to the end of sentences.