-
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: add date metadata for course run assignments on coures about page #1145
Conversation
b7da0d9
to
c956e93
Compare
c956e93
to
a518fb7
Compare
5028945
to
3211653
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1145 +/- ##
==========================================
- Coverage 88.06% 88.05% -0.02%
==========================================
Files 393 394 +1
Lines 8289 8338 +49
Branches 2027 2047 +20
==========================================
+ Hits 7300 7342 +42
- Misses 947 953 +6
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. |
…al-enterprise into hu/ent-8887
1a1580b
to
2cb01c3
Compare
9af8f6a
to
ab638fc
Compare
effe96f
to
57c2c85
Compare
const today = new Date(); | ||
const startDate = new Date(start); | ||
return startDate && today >= startDate; | ||
return dayjs(start).isBefore(dayjs()); |
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.
🎉
src/components/course/data/utils.jsx
Outdated
dateFormat = null, | ||
}) { | ||
if (!assignments?.length) { | ||
logInfo(`[sortedByExpirationDate] ${assignments} an empty array`); |
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.
[nit/curious] Is the [sortedByExpirationDate]
prefix intended to match the function name? E.g., at this point in the code path sortedByExpirationDate
does not exist (i.e., defined below line 928).
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.
nit: given assignments
is an empty array, i'm not sure how much utility there is in including the variable itself in the logInfo
?:
Would this message make sense as something like, [sortedByExpirationDate] no assignments provided in array
or similar?
Semi-related (curious for your two cents!), I'm wondering if we might want to treat this and the below logInfo
as logError
given it seems in the code path where this function is called, it's only called when there are assigned course runs, we might want to consider it as an error that could trigger an alert for observability/monitoring. For example, if we determine there are assigned course runs, and then pass in empty array or the assignments are all missing .earliestPossibleExpiration
, would we want to consider that an error? 🤔
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 had this debate with myself as well when implementing this approach. I guess I overlooked the part where we are deep enough into the code path to expect assignments in the array at this point so a change to logError
would make the most sense. Changing now 👍🏽
Also, at one point the function name was sortedByExpirationDate
but ended up changing the name and not the variable. Updated to take that into account.
src/components/course/course-header/tests/CourseImportantDates.test.jsx
Outdated
Show resolved
Hide resolved
d55a8a0
to
5a900c2
Compare
Adds course run related
enrollBy
date andcourseStart
date on the course about page for assignment based enrollments.For instances where multiple course runs are assigned to a course, the soonest
earliestExpirationDate
date is displayed along with its corresponding course start date.Properly handles tense for when a course started vs course starts.
For all changes
Only if submitting a visual change