-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: default stale start dates to today #1180
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1180 +/- ##
==========================================
+ Coverage 88.36% 88.39% +0.02%
==========================================
Files 396 396
Lines 8406 8407 +1
Branches 2025 2061 +36
==========================================
+ Hits 7428 7431 +3
+ Misses 936 933 -3
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. |
2366eab
to
16d38f7
Compare
16d38f7
to
61c8f25
Compare
@@ -73,7 +73,7 @@ export default function useEnterpriseCourseEnrollments(queryOptions = {}) { | |||
}, | |||
enabled: isEnabled, | |||
}); | |||
|
|||
// TODO: Talk about how we don't have access to weeksToComplete on the dashboard page. |
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.
[curious] Should we backlog a ticket as a follow-up story to make the Dashboard start dates have parity to other start dates with regard to weeksToComplete
? Maybe include the ticket number in the TODO comment?
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.
We can tackle this in the follow-up PR for coverage tomorrow morning.
if (isCourseSelfPaced({ pacingType })) { | ||
if (hasTimeToComplete({ end, weeksToComplete }) || isWithinMinimumStartDateThreshold({ start })) { | ||
// always today's date (incentives enrollment) | ||
return todayToIso; |
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.
Do we want test coverage on this 2 code paths?
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.
Yep, as discussed synchronously, the coverage can be a followup post testing on stage. Since the pipeline will be paused for both admin and learner portal, the coverage can be brought up to date with subsequent PRs tomorrow to get started on testing.
That being said there is a component that uses identical logic useCourseRunCardHeading
which reuses the same logic which I will piggyback off of for testing 👍🏽
Based on a heuristic, default stale course start dates to today to display in various locations in the learner journey
For all changes
Only if submitting a visual change