Skip to content
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: Update threshold dates for displaying course runs #1317

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

brobro10000
Copy link
Member

Updates the logic which determines the course runs displayed in the learner credit assignment dropdown.
Ensures the following logic:

  • Given an existing enrollBy date for a course run, the enrollBy date is after today's date. If late enrollment is allowed, the enrollBy date is after today - LATE_ENROLLMENTS_BUFFER_DAYS
  • Given an existing enrollmentStart and start date for a course run, the minimum date between an enrollmentStart and start date must be before the subsidyExpirationDate - MAX_ALLOWABLE_REFUND_THRESHOLD_DAYS

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@@ -570,6 +570,29 @@ export const minimumEnrollByDateFromToday = ({ subsidyExpirationDatetime }) => M
dayjs(subsidyExpirationDatetime).subtract(MAX_ALLOWABLE_REFUND_THRESHOLD_DAYS, 'days').toDate(),
);

const subsidyExpirationRefundCutoffDate = ({ subsidyExpirationDatetime }) => dayjs(subsidyExpirationDatetime).subtract(MAX_ALLOWABLE_REFUND_THRESHOLD_DAYS, 'days').toDate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could re-use this directly above in minimumEnrollByDateFromToday. Actually now that I'm looking at that function, is the min() doing anything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the min portion was left in incase additional criteria would need to be added, for right now its determining a fallback enrollby date to display within getNormalizedEnrollByDate.

src/components/learner-credit-management/data/utils.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 85.62%. Comparing base (b8cd0c7) to head (6c684de).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...components/learner-credit-management/data/utils.js 84.21% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1317      +/-   ##
==========================================
+ Coverage   85.57%   85.62%   +0.05%     
==========================================
  Files         570      570              
  Lines       12666    12676      +10     
  Branches     2690     2690              
==========================================
+ Hits        10839    10854      +15     
+ Misses       1765     1761       -4     
+ Partials       62       61       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@iloveagent57 iloveagent57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't let my nits stop you from merging when you're comfortable.

Comment on lines +699 to 703
if (hasEnrollBy && isLateRedemptionAllowed && isDateBeforeToday(enrollBy)) {
return isLateEnrollmentEligible && isEligibleForEnrollment;
}
// General courseware filter
return isActive && isEligibleForEnrollment;
Copy link
Contributor

@pwnage101 pwnage101 Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: isEligibleForEnrollment is a common consideration, maybe return early if that's false? That way we can end the entire function with a sequence of checks and comments for better readability. Furthermore, you don't need to check hasEnrollBy or isDateBeforeToday(enrollBy) anymore because that's already rolled into isEligibleForEnrollment.

Suggested change
if (hasEnrollBy && isLateRedemptionAllowed && isDateBeforeToday(enrollBy)) {
return isLateEnrollmentEligible && isEligibleForEnrollment;
}
// General courseware filter
return isActive && isEligibleForEnrollment;
if (!isEligibleForEnrollment) {
// Basic checks against this content's critical dates and their relation to
// the current date and subsidy expiration date have failed.
return false;
}
if (isLateRedemptionAllowed) {
// Special case: late enrollment has been enabled by ECS for this budget, and
// isEligibleForEnrollment already succeeded, so we know that late enrollment
// would be happy given enrollment deadline of the course. Now all we need
// to do is make sure the run itself is generally eligible for late enrollment
// (e.g. it is published and not archived, etc.)
return isLateEnrollmentEligible;
}
// General courseware filter. Content that's active is generally eligible for
// enrollment (e.g. it is published and not archived and generally enrollable,
// etc.).
return isActive;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely leaving this open to put into consideration for a refined implementation. 👍🏽

Copy link
Contributor

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment is also non-blocking, but for the sake of readability I'd like my Nit addressed eventually.

@brobro10000 brobro10000 merged commit 9aef684 into master Sep 24, 2024
5 of 6 checks passed
@brobro10000 brobro10000 deleted the hu/ent-enrollby-start branch September 24, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants