#5845 - Make institution location code optional#5900
#5845 - Make institution location code optional#5900tiago-graf wants to merge 9 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates institution location handling to allow missing location codes during location maintenance while preventing designation approval of locations that do not yet have a code.
Changes:
- Makes
institutionCodenullable/optional across backend entity/service/models and key web DTOs, and updates the Form.io “institutionLocation” form with an explicit “no code” flow. - Extends AEST designation workflow data to include
institutionCodeper location and adds UI validation to block approving locations without a code. - Performs template prop/event casing cleanup in several Vue components.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/views/institution/AddInstitutionLocation.vue | Prop/event casing updates for header and submit UI. |
| sources/packages/web/src/views/aest/institution/DesignationAESTView.vue | Adds institutionCode to designation approval location items. |
| sources/packages/web/src/services/http/dto/InstitutionLocation.dto.ts | Adjusts web DTOs to support optional code / “no code” flag. |
| sources/packages/web/src/services/http/dto/DesignationAgreement.dto.ts | Replaces any with unknown and formatting cleanup. |
| sources/packages/web/src/components/partial-view/DesignationAgreement/DesignationAgreementForm.models.ts | Adds optional institutionCode to location list model. |
| sources/packages/web/src/components/aest/institution/modals/ApproveDenyDesignationModal.vue | Displays location code and blocks approving locations with missing code. |
| sources/packages/forms/src/form-definitions/institutionlocation.json | Makes code optional with checkbox + conditional validation + banner. |
| sources/packages/forms/package-lock.json | Lockfile updates (peer metadata). |
| sources/packages/backend/libs/sims-db/src/entities/institution-location.model.ts | Makes institutionCode nullable at the ORM level. |
| sources/packages/backend/apps/api/src/services/institution-location/institution-location.service.ts | Skips duplicate checks when code is absent; persists null when no code. |
| sources/packages/backend/apps/api/src/services/institution-location/institution-location.models.ts | Makes institutionCode optional and introduces noInstitutionCode flag. |
| sources/packages/backend/apps/api/src/route-controllers/institution-locations/models/institution-location.dto.ts | Allows optional input code; adjusts output typing for nullable codes. |
Files not reviewed (1)
- sources/packages/forms/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
sources/packages/web/src/services/http/dto/InstitutionLocation.dto.ts:47
InstitutionLocationAPIInDTOstill declaresinstitutionCodeas a requiredstring, but the location code is now optional (and the backend AEST DTO acceptsstring | null). This forces callers to fabricate empty strings and makes the contract inconsistent; update the DTO to acceptinstitutionCode?: string | null(and consider exposingnoInstitutionCodeif the form submits it).
export class InstitutionLocationAPIInDTO
implements AddressDetailsFormAPIDTO, InstitutionLocationPrimaryContactAPIInDTO
{
@Expose()
locationName: string;
@Expose()
institutionCode: string;
// From InstitutionLocationPrimaryContactAPIInDTO.
sources/packages/web/src/services/http/dto/InstitutionLocation.dto.ts:86
InstitutionLocationFormAPIOutDTOstill requiresinstitutionCode: string, but the form can now legitimately represent a location without a code. This will makeformDatatyping inaccurate and can cause downstream code to assume a value exists; update it toinstitutionCode?: string | null(and optionally includenoInstitutionCode?: booleanfor round-tripping).
export interface InstitutionLocationFormAPIOutDTO extends AddressDetailsFormAPIDTO {
locationName: string;
institutionCode: string;
primaryContactFirstName: string;
primaryContactLastName: string;
primaryContactEmail: string;
primaryContactPhone: string;
}
You can also share your feedback on Copilot code review. Take the survey.
sources/packages/web/src/components/aest/institution/modals/ApproveDenyDesignationModal.vue
Outdated
Show resolved
Hide resolved
...kend/apps/api/src/route-controllers/institution-locations/models/institution-location.dto.ts
Show resolved
Hide resolved
.../packages/backend/apps/api/src/services/institution-location/institution-location.service.ts
Outdated
Show resolved
Hide resolved
| "value": "" | ||
| } | ||
| ], | ||
| "content": "<strong class=\"ml-2\">Missing institution location code</strong><br><span>Please note: You will be able to create this location without an institution location code. However, you will not be able to designate this location until you obtain a federally issued code from StudentAid BC. Once you submit your location please email designat@gov.bc.ca and request they provide you with the code and update the location information.</span>", |
There was a problem hiding this comment.
Confirmed with @Joshua-Lakusta that this email is accurate
There was a problem hiding this comment.
Pull request overview
Updates institution location handling across the stack to allow saving locations without an institution location code, while preventing designation approvals for locations that do not have a code.
Changes:
- Make Institution Location Code optional in the institution Form.io form (adds “no code” checkbox + warning banner + conditional validation/disable).
- Update backend persistence to allow nullable
institution_code, adjust duplicate checks, and add validation to block approving designations for approved locations without a code. - Propagate optional code through web/AEST views and add new API e2e coverage for AEST location get/update and designation agreement update.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/web/src/views/institution/AddInstitutionLocation.vue | Template prop casing adjustments for consistency (kebab-case). |
| sources/packages/web/src/views/aest/institution/DesignationAESTView.vue | Includes institutionCode in the designation locations model for modal display/validation. |
| sources/packages/web/src/services/http/dto/InstitutionLocation.dto.ts | Adds noInstitutionCode to input DTOs and makes location code optional in some outputs. |
| sources/packages/web/src/services/http/dto/DesignationAgreement.dto.ts | Tightens typing (any → unknown) and formatting. |
| sources/packages/web/src/components/partial-view/DesignationAgreement/DesignationAgreementForm.models.ts | Extends designation location list item to include optional institutionCode. |
| sources/packages/web/src/components/aest/institution/modals/ApproveDenyDesignationModal.vue | Blocks approving a location without a code in the AEST modal UI and updates modal typing. |
| sources/packages/forms/src/form-definitions/institutionlocation.json | Makes code conditionally required and adds “no code” checkbox + warning banner. |
| sources/packages/forms/package-lock.json | Lockfile metadata updates. |
| sources/packages/backend/libs/sims-db/src/entities/institution-location.model.ts | Marks institution_code as nullable in the entity. |
| sources/packages/backend/apps/api/src/services/institution-location/institution-location.service.ts | Trims code, conditionally checks duplicates, and adds validation helper for designation approval. |
| sources/packages/backend/apps/api/src/services/institution-location/institution-location.models.ts | Makes institutionCode optional in the service model and adds noInstitutionCode. |
| sources/packages/backend/apps/api/src/route-controllers/institution-locations/models/institution-location.dto.ts | Updates DTO validation/typing to allow null/optional institution codes. |
| sources/packages/backend/apps/api/src/route-controllers/institution-locations/tests/e2e/institution-location.aest.controller.update.e2e-spec.ts | New e2e coverage for updating with empty code and duplicate code validation. |
| sources/packages/backend/apps/api/src/route-controllers/institution-locations/tests/e2e/institution-location.aest.controller.getInstitutionLocation.e2e-spec.ts | New e2e coverage ensuring null vs present institutionCode in responses. |
| sources/packages/backend/apps/api/src/route-controllers/designation-agreement/designation-agreement.aest.controller.ts | Enforces institution code presence for approved locations (unprocessable entity with typed error). |
| sources/packages/backend/apps/api/src/route-controllers/designation-agreement/tests/e2e/designation-agreement.aest.controller.updateDesignationAgreement.e2e-spec.ts | New e2e coverage for designation approval/decline and missing-code rejection. |
| sources/packages/backend/apps/api/src/constants/error-code.constants.ts | Adds new error code MISSING_INSTITUTION_LOCATION_CODE. |
Files not reviewed (1)
- sources/packages/forms/package-lock.json: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
.../packages/backend/apps/api/src/services/institution-location/institution-location.service.ts
Show resolved
Hide resolved
.../packages/backend/apps/api/src/services/institution-location/institution-location.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/sims-db/src/entities/institution-location.model.ts
Show resolved
Hide resolved
...ent/_tests_/e2e/designation-agreement.aest.controller.updateDesignationAgreement.e2e-spec.ts
Show resolved
Hide resolved
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
||
| it("Should approve a designation agreement when all payload locations belong to the institution.", async () => { | ||
| // Arrange | ||
| const { institution } = await getAuthRelatedEntities( |
There was a problem hiding this comment.
We don't have to get the data-seeded institution here. We can have a new institution created this way.
const institution = await db.institution.save(createFakeInstitution());| const updatedDesignation = await db.designationAgreement.findOneBy({ | ||
| id: savedDesignation.id, | ||
| }); | ||
| expect(updatedDesignation.designationStatus).toBe( | ||
| DesignationAgreementStatus.Approved, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
At least for one test we can assert the possible updated fields.
const updatedDesignation = await db.designationAgreement.findOne({
select: {
id: true,
designationStatus: true,
startDate: true,
endDate: true,
designationAgreementLocations: {
id: true,
institutionLocation: { id: true },
approved: true,
},
institution: {
id: true,
notes: { id: true, noteType: true, description: true },
},
assessedBy: { id: true },
assessedDate: true,
},
relations: {
designationAgreementLocations: { institutionLocation: true },
institution: { notes: true },
assessedBy: true,
},
where: { id: savedDesignation.id },
loadEagerRelations: false,
});
const auditUser = await getAESTUser(
db.dataSource,
AESTGroups.BusinessAdministrators,
);
expect(updatedDesignation.designationStatus).toEqual({
id: savedDesignation.id,
designationStatus: DesignationAgreementStatus.Approved,
startDate: "2026-01-01",
endDate: "2027-12-31",
designationAgreementLocations: [
{
id: expect.any(Number),
institutionLocation: { id: savedLocation.id },
approved: true,
},
],
institution: {
id: institution.id,
notes: [
{
id: expect.any(Number),
noteType: NoteType.Designation,
description: "Designation approved.",
},
],
},
assessedBy: { id: auditUser.id },
assessedDate: expect.any(Date),
});|
|
||
| it("Should decline a designation agreement.", async () => { | ||
| // Arrange | ||
| const { institution } = await getAuthRelatedEntities( |
There was a problem hiding this comment.
| designationStatus: DesignationAgreementStatus.Declined, | ||
| note: "Some note.", | ||
| }) | ||
| .expect(HttpStatus.NOT_FOUND); |
| await db.designationAgreement.save(fakeDesignation); | ||
| // Create a location that belongs to a different institution. | ||
| const unrelatedLocation = createFakeInstitutionLocation(); | ||
| const savedUnrelatedLocation = |
There was a problem hiding this comment.
Minor, when await db.institutionLocation.save(unrelatedLocation); is executed the unrelatedLocation is updated automatically and not required to re-assign to a new const.
| ], | ||
| note: "Attempt with unrelated location.", | ||
| }) | ||
| .expect(HttpStatus.UNPROCESSABLE_ENTITY); |
There was a problem hiding this comment.
Please assert the complete response.
| const fakeLocation = createFakeInstitutionLocation({ institution }); | ||
| fakeLocation.institutionCode = null; | ||
| const savedLocation = await db.institutionLocation.save(fakeLocation); | ||
| const fakeUser = await db.user.save(createFakeUser()); |
There was a problem hiding this comment.
If fake user is required across many tests, we can have one created globally for that sake.
| fakeInstitutionLocations: [savedLocation], | ||
| fakeUser, | ||
| }); | ||
| fakeDesignation.designationStatus = DesignationAgreementStatus.Pending; |
There was a problem hiding this comment.
To set the designationStatus we can use the initialValues in factory.
const fakeDesignation = createFakeDesignationAgreement(
{
fakeInstitution: institution,
fakeInstitutionLocations: [savedLocation],
fakeUser,
},
{
initialValue: { designationStatus: DesignationAgreementStatus.Pending },
},
);| }) | ||
| .expect(HttpStatus.UNPROCESSABLE_ENTITY) | ||
| .expect({ | ||
| message: |
There was a problem hiding this comment.
Thanks for creating the E2E tests with good coverage.
| * @param locationIds location IDs to validate. | ||
| * @returns true when all locations have a non-null institution location code. | ||
| */ | ||
| async validateLocationsHaveInstitutionCode( |
There was a problem hiding this comment.
If our intention was to see if there is any location without code, we can check for the opposite.
async hasAnyLocationWithoutCode(locationIds: number[]): Promise<boolean> {
const locationWithoutCode = await this.repo
.createQueryBuilder("location")
.select("1")
.where("location.id IN (:...locationIds)", { locationIds })
.andWhere("location.institutionCode IS NULL")
.limit(1)
.getRawOne();
return !!locationWithoutCode;
}| const approvedLocationIds = payload.locationsDesignations | ||
| .filter((location) => location.approved) | ||
| .map((location) => location.locationId); | ||
| if (approvedLocationIds.length > 0) { |
There was a problem hiding this comment.
Based on the suggestion from the comment, https://github.com/bcgov/SIMS/pull/5900/changes#r2949170341 this condition won't be required.
| "One or more approved locations are missing an institution location code.", | ||
| MISSING_INSTITUTION_LOCATION_CODE, |
There was a problem hiding this comment.
Does it need to be an API Process error? We use API process error to show a different error message other than the generic one (e.g. unexpected error when approving/declining the designation).
| expect(response.body.institutionCode).toBeNull(); | ||
| }); | ||
|
|
||
| it("Should return institution location with the institution code when one is assigned.", async () => { |
There was a problem hiding this comment.
For one of the tests, can we assert the complete payload?
| db = createE2EDataSources(dataSource); | ||
| }); | ||
|
|
||
| it("Should save institution location with null code when an empty institution code is submitted.", async () => { |
There was a problem hiding this comment.
As per business requirement, the ministry should not be able to save the institution location as NULL.
| @IsNotEmpty() | ||
| institutionCode: string; | ||
| @IsOptional() | ||
| institutionCode?: string | null; |
There was a problem hiding this comment.
institutionCode?: string; should be good enough.
| export class InstitutionLocationDetailsAPIOutDTO extends AddressDetailsAPIOutDTO { | ||
| locationName: string; | ||
| institutionCode: string; | ||
| institutionCode: string | null; |
There was a problem hiding this comment.
institutionCode?: string ?
Same in other places.
| @Allow() | ||
| institutionCode: string; | ||
| @Allow() | ||
| noInstitutionCode?: boolean; |
There was a problem hiding this comment.
Is the boolean value not suppose to be present always?
| locationName: string; | ||
| institutionCode: string; | ||
| institutionCode?: string; | ||
| noInstitutionCode?: boolean; |
There was a problem hiding this comment.
InstitutionLocationModel is used for update location. The update location operation is ministry specific and ministry isn't expected to either see the checkbox or use the checkbox. Hence these model updates aren't relevant.
There was a problem hiding this comment.
For Update can use a model derived from InstitutionLocationModel.
export type InstitutionLocationUpdateModel = Omit<
InstitutionLocationModel,
"noInstitutionCode"
> & {
institutionCode: string;
};| }, | ||
| institution: institution, | ||
| institutionCode: data.institutionCode, | ||
| institutionCode: hasInstitutionCode ? normalizedInstitutionCode : null, |
There was a problem hiding this comment.
institutionCode: normalizedInstitutionCode, should be good enough
| const hasInstitutionCode = | ||
| !!institutionLocationData.institutionCode?.trim(); | ||
| const normalizedInstitutionCode = hasInstitutionCode | ||
| ? institutionLocationData.institutionCode.trim() | ||
| : null; |
There was a problem hiding this comment.
During update location, location code must be present.
| nullable: true, | ||
| }) | ||
| institutionCode: string; | ||
| institutionCode: string | null; |
There was a problem hiding this comment.
institutionCode?: string;
| "value": "" | ||
| } | ||
| ], | ||
| "content": "<strong class=\"ml-2\">Missing institution location code</strong><br><span>Please note: You will be able to create this location without an institution location code. However, you will not be able to designate this location until you obtain a federally issued code from StudentAid BC. Once you submit your location please email designat@gov.bc.ca and request they provide you with the code and update the location information.</span>", |
| "minLength": 4, | ||
| "maxLength": 4, | ||
| "pattern": "[A-Z]*" | ||
| "pattern": "[A-Z]*", |
| "pattern": "[A-Z]*", | ||
| "custom": "valid = (!data.noInstitutionCode && !input) ? 'Institution location code is required.' : true;" | ||
| }, | ||
| "logic": [ |
There was a problem hiding this comment.
Here are the business expectations for the institution location code as per my understanding.
- The institution location code is not a mandatory field only if the new checkbox is selected and if the checkbox is not selected, then it is still a required field. This action is only applicable for Institutions create a new location and NOT for ministry editing/updating a location.(Note: Institution create/view location, Ministry update location share the same form)
- When ministry update institution location code, the institution location is a required field and checkbox must not be visible for ministry user.
| "defaultValue": false, | ||
| "key": "noInstitutionCode", | ||
| "attributes": { | ||
| "data-cy": "noInstitutionCode" |
There was a problem hiding this comment.
data-cy is not relevant anymore.
| "refreshOnChange": true, | ||
| "hidden": true, | ||
| "key": "noInstitutionCodeBanner", | ||
| "customConditional": "show = data.noInstitutionCode === true;", |
There was a problem hiding this comment.
Please use a simple condition.
| @@ -15,6 +15,8 @@ export class InstitutionLocationFormAPIInDTO extends AddressDetailsFormAPIDTO { | |||
| @Expose() | |||
| institutionCode: string; | |||
There was a problem hiding this comment.
institutionCode?: string;
| startDate: string; | ||
| endDate: string; | ||
| submittedData: any; | ||
| submittedData: unknown; |
| "integrity": "sha512-jl1vZzPDinLr9eUt3J/t7V6FgNEw9QjvBPdysz9KfQDD41fQrC2Y4vKQdiaUpFT4bXlb1RHhLpp8wtm6M5TgSw==", | ||
| "dev": true | ||
| "dev": true, | ||
| "peer": true |
There was a problem hiding this comment.
Is it due to the node update?
There was a problem hiding this comment.
I'm probably using a different node version, anyway, i'm reverting the changes
| </v-row> | ||
| <v-row v-if="item.approved && !item.institutionCode" class="mt-0"> | ||
| <v-col class="text-right pt-0"> | ||
| <span class="text-error d-block" |
There was a problem hiding this comment.
I was following the wireframes, happy to remove
|
Nice work @tiago-graf. Please take a look at the comments. |
|








Summary
Make institution location code to make location optional on location edit form.
Adds validation to approve designation modal to block approval on locations without code.