-
Notifications
You must be signed in to change notification settings - Fork 3
Change "next_run" to "best_run" and refactor how next_start_date is calculated #2586
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
Conversation
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.
👍
learning_resources/models.py
Outdated
published_runs = self.runs.filter(published=True) | ||
now = now_in_utc() | ||
# Find the most recent run with a currently active enrollment period | ||
current_run = ( |
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.
Should this be best_run? It seems to start w/ current run, but fallback to future or older.
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.
Will rename to best_lr_run
to avoid any confusion with the property name itself.
learning_resources/etl/loaders.py
Outdated
) | ||
if best_run: | ||
now = now_in_utc() | ||
if resource.published: |
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 have about 3k resources locally from ocw, xpro, mitxonline, see, and mit_edx. (I think thats all of them?) All published resources have a best_run 👍, but some unpublished ones don't.
If somehow a published resource had no best run, we get errors below, right? Should we keep the best_run check here? What happens if errors below occur? Just that 1 resource fails ETL, right?
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.
That should only happen if a resource has no published runs at all, in which case the resource itself shouldn't be published. I'll add the best_run check back just in case.
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.
👍 Thanks for the explanation!
337e693
to
2de3225
Compare
What are the relevant tickets?
Description (What does it do?)
Replaces
LearningResource.next_run
withLearningResource.best_run
and changes the criteria for selecting that run. This way, an enrollable run that is currently ongoing (start date in the past, end date in the future) should be selected instead of the next enrollable run with a future start date.How can this be tested?
XPRO_*
env variables in yourbackend.local.env
file./manage.py backpopulate_xpro_data
Screenshots
This branch:
Main branch, RC/Prod: