#5154 - Ensure Application Creator is Set for Change Requests#5736
#5154 - Ensure Application Creator is Set for Change Requests#5736tiago-graf merged 13 commits intomainfrom
Conversation
tiago-graf
added
E2E/Unit tests
DB
There was a problem hiding this comment.
Pull request overview
This pull request addresses the first acceptance criterion of issue #5154 by ensuring the creator field is properly set for application change requests and enforcing NOT NULL constraints through a database migration.
Changes:
- Sets the
creatorfield to the student's user ID when creating application change requests - Adds a database migration to backfill NULL
creatorvalues with the student's user ID and enforce NOT NULL constraint - Updates test factories to properly set the
creatorfield in test data - Adds e2e test assertions to verify the creator field is correctly populated
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sources/packages/backend/apps/api/src/services/application/application.service.ts | Sets creator field for new application records in submitApplication and createApplicationChangeRequest methods, moving the assignment before the first save operation |
| sources/packages/backend/apps/db-migrations/src/migrations/1770252146185-SetApplicationCreatorFromStudentId.ts | Migration class that applies SQL to backfill and enforce NOT NULL constraint on creator field |
| sources/packages/backend/apps/db-migrations/src/sql/Applications/Set-creator-from-student-id.sql | SQL script to update NULL creators with student user IDs and add NOT NULL constraint |
| sources/packages/backend/apps/db-migrations/src/sql/Applications/Rollback-set-creator-from-student-id.sql | Rollback script to revert NOT NULL constraint (contains critical bug) |
| sources/packages/backend/libs/test-utils/src/factories/application.ts | Updates test factory to accept auditUser relation and use it to set the creator field in test data |
| sources/packages/backend/apps/api/src/route-controllers/application/tests/application.students.controller.applicationChangeRequest.e2e-spec.ts | Adds assertions to verify creator field is properly set in e2e tests |
| .vscode/launch.json | Adds new debug configuration for running tests by selected name (unrelated developer tooling) |
...ges/backend/apps/db-migrations/src/sql/Applications/Rollback-set-creator-from-student-id.sql
Outdated
Show resolved
Hide resolved
...ges/backend/apps/db-migrations/src/sql/Applications/Rollback-set-creator-from-student-id.sql
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| ], | ||
| "rootDir": ".", | ||
| "testRegex": ".*\\.spec\\.ts$", | ||
| "testRegex": ".*\\.(?:e2e-)?spec\\.ts$", |
There was a problem hiding this comment.
After some discussion with @weskubo-cgi and some research we found out this seemed to be missing to enable the e2e tests in the test explorer, this now makes us able to see/run individual tests but i'm still wondering if you @dheepak-aot @andrewsignori-aot have ran into this before or if you use any other test tools to run the unit tests?
There was a problem hiding this comment.
This config is used only for unit tests. If we are talking about the test explorer not being able to identify the e2e tests, I believe it is not considering the configs for e2e tests.
Let me know if I am missing something.
There was a problem hiding this comment.
I reverted the changes and made jest use the jest-e2e.json file by settings the jest command line to use the npm run test:e2e:api:local
...ces/packages/backend/apps/db-migrations/src/sql/Applications/Set-creator-from-student-id.sql
Show resolved
Hide resolved
| "Sources", | ||
| "All", | ||
| ], | ||
| "jest.jestCommandLine": "npm run test:e2e:api:local --", |
There was a problem hiding this comment.
this makes the text explorer recognize the e2e tests now, the draw back is that the unit tests are not present (we would need a combined config file if that was necessary)
There was a problem hiding this comment.
not sure why the whole file formatting changed, i'm assuming no one touched this file in a while
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
weskubo-cgi
left a comment
weskubo-cgi
left a comment
There was a problem hiding this comment.
Changes look good. And thanks for getting the Test Explorer setup. I love it.
| "jest.disabledWorkspaceFolders": [ | ||
| "Web UI", | ||
| "Forms", | ||
| "DevOps", | ||
| "Load Tests", | ||
| "Sources", |
There was a problem hiding this comment.
The tool looks great. Thanks.
dheepak-aot
left a comment
dheepak-aot
left a comment
There was a problem hiding this comment.
Thanks for making the changes. Looks good 👍
3 participants
Summary
Sets the creator field for application change requests to the student's user ID. Includes migration to backfill existing NULL creators.
Addresses first AC of #5154.