-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix: don't consider elapsed enrollBy in assignable course runs #1313
Conversation
const enrollByDate = dayjs(enrollBy); | ||
// Determine eligibility based on the provided enrollBy is and the subsidy expiration date - refund threshold | ||
isEligibleForEnrollment = ( | ||
!isDateBeforeToday(enrollByDate) |
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.
Maybe add a comment that we'll want to eventually loop back to this and allow a certain buffer to allow enrollBy to be a few days before today?
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.
hm, the code below seems to consider late enrollment buffer days, so I just might be misunderstanding something...
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.
oh okay that overrides this, LGTM
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.
Isn't this the use case for late enrollment, handled below, though? If late enrollment is not enabled for a budget, I'm not sure we'd want to expose runs that are otherwise not enrollable without late enrollment enabled? 🤔
(edit: given late enrollment is not quite the happy path; it's opt-in and will be disabled for most budgets).
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.
Ha, sorry didn't see your updated comments until after submitting the above comment :)
Agreed this should be adequate.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1313 +/- ##
=======================================
Coverage 85.56% 85.57%
=======================================
Files 567 567
Lines 12598 12601 +3
Branches 2634 2672 +38
=======================================
+ Hits 10780 10783 +3
+ Misses 1757 1756 -1
- Partials 61 62 +1 ☔ View full report in Codecov by Sentry. |
For all changes
Only if submitting a visual change