[RW-7110][risk=no] Change lookback for compliance training renewal #6525
Conversation
There was a problem hiding this comment.
Looks good, just some minor comments.
Also, originally you mentioned adding this to the DB access module state. This approach works well too, but for future reference it may be a good idea to add the "alternatives considered" to the PR description to capture rationale.
Perfect, that is all great feedback. I will work on addressing your comments. |
There was a problem hiding this comment.
Wouldn't it be nice if you didn't have to type same config and value in all config files.
https://precisionmedicineinitiative.atlassian.net/browse/RW-7871
| wasBypassed || | ||
| (isComplete && !isExpiringOrExpired(status?.expirationEpochMillis)) | ||
| (isComplete && | ||
| !isExpiringOrExpired(status?.expirationEpochMillis, status.moduleName)) |
There was a problem hiding this comment.
Does the new param requires optional chaining: status?.moduleName?
There was a problem hiding this comment.
moduleName has recently become a required element, so it must exist if status does
| "expiryDays": 365, | ||
| "expiryDaysWarningThresholds": [1, 3, 7, 15, 30], | ||
| "lookbackPeriod": 330 | ||
| "lookbackPeriod": 330, | ||
| "trainingLookbackPeriod": 30 |
There was a problem hiding this comment.
I got an idea for future improvement: there is a pattern here in renewal configs. Maybe one of more config values can be derived.
TOL, e.g. trainingLookbackPeriod can be derived from last element of expiryDaysWarningThresholds array. Or expiryDays minus lookbackPeriod (= 35) is what trainingLookbackPeriod should be.
| complianceTrainingHost: | ||
| type: string | ||
| description: The hostname for constructing the compliance training URL. | ||
| complianceTrainingRenewalLookback: |
There was a problem hiding this comment.
@evrii what do you think about a small followup PR for this change: group this one with accessRenewalLookback and update the description(s) to make the commonality and differences clearer.
Description:
Updated the lookback for compliance training to be 30 days.
Alternatives considered:
Adding a lookback period column to the access_module table.
Decided against it, because there is no anticipation of any more lookback periods.
Hard coding the training modules lookback period into the ui.
Decided against this approach, because it seemed less flexible.
PR checklist