-
Notifications
You must be signed in to change notification settings - Fork 3
Use best_run_id to determine start date on frontend; bring back next_run/date #2702
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
base: main
Are you sure you want to change the base?
Conversation
ChristopherChudzicki
left a 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.
As written, I don't think this affects the DifferingRunsTable. Left a suggestion about how it could be done to affect that and deal with a date ordering issue.
| bestStart = | ||
| Date.parse(bestRun.start_date) > Date.parse(bestRun.enrollment_start) | ||
| ? bestRun.start_date | ||
| : bestRun.enrollment_start |
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.
The fallback here would be highly unexpected / bad data, right?
I just checked prod with
from learning_resources.models import LearningResourceRun
from django.db.models import F
# Filter runs where enrollment_start is greater than start_date (both fields must be non-null)
runs = LearningResourceRun.objects.filter(
enrollment_start__isnull=False,
start_date__isnull=False,
enrollment_start__gt=F('start_date')
)and runs.count() was 0. (RC did have 6 such runs, 1 of which was published. It was from MITxOnline. RC MITxOnline data is ...not great, though).
Suggestion: My inclination would be to leave a comment here that this is pretty unexpected.
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.
Right, something would have to be quite wrong for a published course not to have a best run. I'll add a comment
| if (sortedDates && bestStartDate && !anytime) { | ||
| // Replace the first date with best_start_date | ||
| sortedDates = [bestStartDate, ...sortedDates.slice(1)] |
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.
The original logic was incorrect here, I think. As currently structured, only "bestStartDate" has the possibility of being <current date>.
But sometimes the best run isn't the first run, so this ends up
// Resource 1, run start dates:
Oc 31 2024 Oct 31 2025 Oct 31 2026
↑
replaced by <current date>
// Resource 2, run start dates:
Oct 31 2025 Oct 31 2026
↑
replaced by <current date>
As currently written, Resource 1 would display (using Nov 18 as current date):
// Resource 1, displayed value (current)
Nov 18 2025 Oct 31 2025 Oct 31 2026
// Resource 1, displayed value (desired)
Oct 31 2025 Nov 18 2025 Oct 31 2026
https://learn.mit.edu/search?platform=mitxonline&q=Minds+and+Machines&sortby=upcoming&resource=2810 is a similar example, where the first date is being replaced, but it should be the third. (Click "Show more")
My suggestion is: move the "maybe show current date" logic into formatRunDate. (Maybe rename to runDisplayDate or something?) That way:
- the dates show up in correct order
- Automatically affects DifferingRunsTable, too. (Which is displayed when runs are sufficiently different that the drawer shows a table, one row per run. Mostly relevant to SEE courses, I think?)
If ☝️ makes sense / sounds good, two things worth mentioning:
- I think you would need to update LearningResourceCard / LearningResourceListCard to use
startDate = formatRunDate(runs.find(best_run)) - In LearningResourceCard / LearningResourceListCard, probably need to use NoSSR instead of LocalDate, since it will be formatted by the parent component, now.
| const daysFromToday = (days: number): string => { | ||
| const date = new Date() | ||
| date.setDate(date.getDate() + days) | ||
| return date.toISOString() | ||
| } |
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.
mentioning... faker.date.soon({ days: <x-number> }) which creates a date 0–x days in the future, iirc.
I don't think it's what you wanted here, but might be useful some other time.
542ba39 to
7365408
Compare
f9edaed to
75462fb
Compare
75462fb to
dc016f0
Compare
ChristopherChudzicki
left a 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.
👍 Working well.
I did leave one comment about potential shared logic. At the very least, I do think we should try and put those funcs in the same file. If we just move the func, I don't think we need to re-review, htough I'm happy to look again.
| } | ||
| } | ||
|
|
||
| // For the best run in dated resources, use special logic |
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'm not sure the logic needs to be special... if a dated course had two active runs, I guess we could show the current date for both of those. You could "start today" for either run.
But I think that would be highly unexpected, so this is fine. 👍
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.
This function is being applied to every run in a course. It's possible that a course might have 2 runs with past start dates, but only 1 is the "best" run, and my assumption was that's the only one that should show today's date, and any other non-best run with a start date in the past should show that past date?
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.
PS an example of this is here: https://learn.mit.edu/search?q=%22Paradox+and+Infinity%22&resource=2809
two published runs:
run 1 (not best): "start_date": "2023-11-09T16:00:00Z"
run 2 (best): "start_date": "2025-02-04T16:00:00Z",
With this PR, InfoSection will show:
Starts:
November 09, 2023|November 24, 2025
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.
Yeah, let's stick with your approach 👍
| if (run.id === bestRunId && availability === "dated" && !asTaughtIn) { | ||
| if (!run.start_date && !run.enrollment_start) return null | ||
|
|
||
| // Get the max of start_date and enrollment_start | ||
| let bestStart: string | ||
| if (run.start_date && run.enrollment_start) { | ||
| bestStart = | ||
| Date.parse(run.start_date) > Date.parse(run.enrollment_start) | ||
| ? run.start_date | ||
| : run.enrollment_start | ||
| } else { | ||
| bestStart = (run.start_date || run.enrollment_start)! | ||
| } | ||
|
|
||
| // If the best start date is in the future, show it; otherwise show today | ||
| const now = new Date() | ||
| const bestStartDate = new Date(bestStart) | ||
| if (bestStartDate > now) { | ||
| return formatDate(bestStart, "MMMM DD, YYYY") | ||
| } else { | ||
| return formatDate(new Date().toISOString(), "MMMM DD, YYYY") | ||
| } | ||
| } |
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.
The date determination is the same logic as getBestResourceStartDate, right? (This function formats it, too).
It would be great if we could share that logic. If not, maybe we could at least co-locate the two funcs in the same file. Odd that getBestResourceStartDate (and its predecessor, getResourceDate) are in the pricing.ts file.
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.
getBestResourceStartDate (formerly bestStartDate takes in a resource as an input parameter, and returns the date of the best run, in ISO string format, Used in the resource cards,
formatRunDate takes in a run as an input parameter, and is called on every run for a resource. It returns the date in a string format that is dependent on certain attributes like semester, year, asTaughtIn boolean, and now also availablity and whether or not it is the best run (because today's date should be shown for a past date only for the best run, not other runs, or at least I presumed that). It is used in the InfoSection component and DifferingRunsTable.
I could adjust getBestResourceStartDate to call formatRunDate(best_run, showStartAnytime(resource), resource.availability, best_run.id) but I was hesitant to do that because of the differences in how that function formats the date outputs, which I assume was intentional?
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.
Yeah, I think we'd have to extract the date-picking without formatting, which might not be trivial.
Let's keep as-is, but i do think co-locating the functions would be worthwhile.
Depends on #2696
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/9206
Description (What does it do?)
best_run_idfor determining what start date to display if anynext_runproperty and assignsnext_start_datebased on that (only used for search purposes now).How can this be tested?
recreate_index --alllearning_resources.tasks.update_next_start_date_and_prices()in a shell.