-
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
#4013 - Legacy Restriction Mapping - Migrations and SQL Changes #4037
#4013 - Legacy Restriction Mapping - Migrations and SQL Changes #4037
Conversation
...ages/backend/apps/db-migrations/src/sql/SFASRestrictionMaps/Create-sfas-restriction-maps.sql
Show resolved
Hide resolved
...ages/backend/apps/db-migrations/src/sql/SFASRestrictionMaps/Create-sfas-restriction-maps.sql
Show resolved
Hide resolved
is_legacy_only BOOLEAN NOT NULL, | ||
-- Audit columns | ||
created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), | ||
UNIQUE (legacy_code, code) |
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.
👍
is_legacy_only BOOLEAN NOT NULL, | ||
-- Audit columns | ||
created_at TIMESTAMP WITH TIME ZONE NOT NULL DEFAULT NOW(), | ||
UNIQUE (legacy_code, code) |
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.
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.
Are you talking about the name convention or about executing it in an alter table?
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.
About convention of CONSTRAINT constraint_name UNIQUE (legacy_code, code)
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.
As we talked, we will be keeping this as it is.
const existingRestrictionCodes = this.restrictionRepo | ||
.createQueryBuilder("restriction") | ||
.select("restriction.restrictionCode") | ||
.getSql(); |
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.
What is suppose to be the outcome if a SIMS federal restriction code is added to mapping by mistake. I see that the system will not create new restriction on-demand. But will we insert a federal restriction as per the existing logic ?? Asking the question as configuration is dynamic now.
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.
Whatever is added to this table as "legacy only" will become a provincial restriction.
Can you clarify what is the concern here?
The table is added as a configuration option but should be managed with caution.
sources/packages/backend/libs/integrations/src/services/sfas/sfas-restriction-import.service.ts
Show resolved
Hide resolved
.select("restriction.restrictionCode") | ||
.getSql(); | ||
const missingLegacyOnlyRestrictions = | ||
await this.sfasRestrictionMapRepo.find({ |
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.
👍
sources/packages/backend/libs/integrations/src/services/sfas/sfas-restriction-import.service.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/integrations/src/services/sfas/sfas-restriction-import.service.ts
Outdated
Show resolved
Hide resolved
Great and massive work. Thanks for the quick outcome. Please take a look at the comments. |
Quality Gate passedIssues Measures |
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.
Thanks for making the changes. Looks Good 👍
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.
Done with my review, LGTM, nice quick work @andrewsignori-aot
Merge Hotfix V2.1.1 to Main ## What's Changed * #4013 - Legacy Restriction Mapping - Migrations and SQL Changes by @andrewsignori-aot in #4037 * #4041 - Ignore Cancelled Legacy Applications on SIMS Validations and Calculations by @dheepak-aot in #4042 * #4013 - Legacy Restriction Mapping - Student Changes by @andrewsignori-aot in #4049 * #4052 - Fix the legacy program year totals for program year limits by @dheepak-aot in #4058 --------- Co-authored-by: Andrew Boni Signori <[email protected]> Co-authored-by: Dheepak Ramanathan <[email protected]>
sims.restrcitions
if not present yet.is_legacy_only
creates a distinction between the SFAS-SIMS mapped restriction and the restrictions that should be added to SIMS but not managed (only resolved).Rollback