-
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 - Student Changes #4049
#4013 - Legacy Restriction Mapping - Student Changes #4049
Conversation
"LGCYAAAA", | ||
"LGCYBBBB", | ||
"LGCYCCCC", |
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.
😂
code: studentRestriction.restriction.restrictionCode, | ||
type: studentRestriction.restriction.notificationType, | ||
})); | ||
if (legacyRestrictions.length) { | ||
const notificationOrder = Object.values(RestrictionNotificationType); |
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.
Suggestion. Will it be better to use a map instead of the order how enums are declared?
const notificationOrderMap = {
[RestrictionNotificationType.Warning]: 1,
[RestrictionNotificationType.Error]: 2,
[RestrictionNotificationType.NoEffect]: 3,
};
// If any legacy restriction is present, create a generic LGCY restriction
// with the highest notification type.
studentRestrictions.sort(
(a, b) =>
notificationOrderMap[b.restriction.notificationType] -
notificationOrderMap[a.restriction.notificationType],
);
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.
I can see benefits in both ways.
Now that we do not have NoEffect
anymore, maybe I can add a priority only to Error
and Warning
and the suggested code would be better and clear.
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.
Adjusting as below.
/**
* Restriction notifications priority order.
* Priority 1 indicates the most important notification.
*/
const NOTIFICATION_PRIORITY_ORDER_MAP = {
[RestrictionNotificationType.Error]: 1,
[RestrictionNotificationType.Warning]: 2,
};
studentRestrictions.sort(
(a, b) =>
NOTIFICATION_PRIORITY_ORDER_MAP[a.restriction.notificationType] -
NOTIFICATION_PRIORITY_ORDER_MAP[b.restriction.notificationType],
);
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.
Code update, please take a look.
|
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.
Great Job. Thanks for catch to filter no effect
restrictions. 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.
LGTM, nice 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 <61259237+andrewsignori-aot@users.noreply.github.com> Co-authored-by: Dheepak Ramanathan <54600590+dheepak-aot@users.noreply.github.com>
LGCY_*
restrictions into a singleLGCY
restriction code to be returned to the student. The error code returned will be the highest one present in the list of theLGCY_*
being returned.LGCY_*
in the DB seed to allow the E2E to use them.db:seed:test:clean
that was not removing thecreate_history_entry
function leading to an error when the DB was populated again.app.close()
) to avoid thedb:seed:test:clean
anddb:seed:test
to be hanging and waiting.Ministry visualization for the new legacy map
SQL to be manually added during release