Merged
Conversation
tiago-graf
added
DB
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enforces that sims.notes.creator is not nullable, strengthening auditability/data integrity for notes in the SIMS database layer.
Changes:
- Adds a DB migration (and rollback) to set
sims.notes.creatortoNOT NULL. - Updates the shared
RecordDataModelTypeORM relation metadata forcreatorto be non-nullable. - Adds a TypeORM migration wrapper to execute the SQL scripts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sources/packages/backend/libs/sims-db/src/entities/record.model.ts | Changes RecordDataModel.creator relation to nullable: false at the ORM level. |
| sources/packages/backend/apps/db-migrations/src/sql/Notes/Set-notes-creator-not-null.sql | Alters sims.notes.creator to NOT NULL. |
| sources/packages/backend/apps/db-migrations/src/sql/Notes/Rollback-set-notes-creator-not-null.sql | Rollback script to drop the NOT NULL constraint on sims.notes.creator. |
| sources/packages/backend/apps/db-migrations/src/migrations/1771367806224-SetNotesCreatorNotNull.ts | TypeORM migration that runs the forward/rollback SQL scripts. |
Comments suppressed due to low confidence (1)
sources/packages/backend/libs/sims-db/src/entities/record.model.ts:15
- Setting
nullable: falseonRecordDataModel.creatoraffects every entity that extendsRecordDataModel, not just notes. Many tables in the SQL migrations still definecreatoras nullable (e.g.,Create-notes.sqland several others), and some comments explicitly state NULL means "created by system". This ORM-level change will make the entity metadata inconsistent with the actual schema/expected behavior and may break inserts/updates for records created without a user. Consider reverting this change in the base class and enforcing NOT NULL only forsims.notes.creator(DB-only), or introduce a separate base model for entities that require a non-null creator.
@ManyToOne(() => User, { eager: false, nullable: false })
@JoinColumn({
name: ColumnNames.Creator,
referencedColumnName: ColumnNames.ID,
})
creator: User;
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| expect(updatedSFASApplication.wthdProcessed).toBe(false); | ||
| }); | ||
|
|
||
| it("Should create a BCSC student with HOLD restriction and note when SFAS partial match exists and note should have a creator.", async () => { |
Collaborator
There was a problem hiding this comment.
Can we renamed the test as below? The creator is the target of this ticket but the E2E itself should not have the creator as target to be tested. Does it make sense?
Should create a BCSC student with HOLD restriction and note when SFAS partial match exists
andrewsignori-aot
requested changes
Feb 20, 2026
Collaborator
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Thanks for checking PROD data. Please take a look at the latest comments.
Comment on lines
+314
to
+318
| if (!holdRestriction) { | ||
| throw new Error( | ||
| "Expected a HOLD restriction to be created for the student.", | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
Please use the expect to execute the assertions.
| noteType: NoteType.Restriction, | ||
| creator: { | ||
| id: systemUserId, | ||
| userName: expect.any(String), |
Collaborator
There was a problem hiding this comment.
No need to select the userName if it will not be asserted. Since the name is known, it should be either asserted or removed.
| SET | ||
| NOT NULL; | ||
|
|
||
| -- Recreate the foreign key with an ON DELETE behavior compatible with NOT NULL. |
Collaborator
There was a problem hiding this comment.
The comment must be adjusted to remove the "ON DELETE behavior", right?
andrewsignori-aot
approved these changes
Feb 20, 2026
Collaborator
andrewsignori-aot
left a comment
andrewsignori-aot
left a comment
There was a problem hiding this comment.
Thanks for the further investigation on PROD and for making the changes. Only minor comments, hence approving it 👍
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Creates migration to alter notes table setting the column creator to NOT NULL.
No data was found where the creator is null, so there is not need to update any records/code.
Migration/Rollback Test