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

mitxonline dashboard crashes when courserun has no upgrade deadline #2362

Conversation

cp-at-mit
Copy link
Contributor

What are the relevant tickets?

https://github.com/mitodl/hq/issues/5284

Description (What does it do?)

mitxonline dashboard crashes when courserun has no upgrade deadline

How can this be tested?

  1. Create a course run with no upgrade deadline defined and course run enrollment for that course run.
  2. Visit the dashboard and verify that the page loads correctly.
  3. Update the course run to have an upgrade deadline in the future.
  4. Visit the dashboard and verify that the page loads correctly.

@cp-at-mit cp-at-mit marked this pull request as ready for review August 26, 2024 18:18
@annagav annagav self-requested a review August 27, 2024 13:19
@annagav annagav self-assigned this Aug 27, 2024
@annagav
Copy link
Contributor

annagav commented Aug 27, 2024

@cp-at-mit I think this might be a related issue #2137. Does this PR fix this issue?

@cp-at-mit
Copy link
Contributor Author

@cp-at-mit I think this might be a related issue #2137. Does this PR fix this issue?

Yeah it sounds like this would resolve that requirement

cp-at-mit and others added 7 commits August 27, 2024 11:44
Copy link
Contributor

@annagav annagav left a comment

Choose a reason for hiding this comment

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

I think this PR should make "upgrade deadline" be required field on the CourseRun model.

@cp-at-mit
Copy link
Contributor Author

cp-at-mit commented Aug 27, 2024

I think this PR should make "upgrade deadline" be required field on the CourseRun model.

@annagav What date should the migration use for CourseRuns without an upgrade deadline?

@annagav
Copy link
Contributor

annagav commented Aug 28, 2024

@cp-at-mit are there there many course runs in production that are missing it? What type of courses are those?

@cp-at-mit
Copy link
Contributor Author

@cp-at-mit are there there many course runs in production that are missing it? What type of courses are those?

I think I saw 8 courses without an upgrade deadline. I think they are just normal courses.

@annagav
Copy link
Contributor

annagav commented Aug 28, 2024

So all the upgrade_deadline does is that it prevents a user from enrolling in verified more after the deadline. So if there is no deadline I am assuming no verified mode is available for the course. So we can set the missing upgrade deadline values to any date in the past or now.

@pdpinch
Copy link
Member

pdpinch commented Aug 28, 2024

I think this PR should make "upgrade deadline" be required field on the CourseRun model.

While our current business rule is that all courses should offer certificates or sale and therefore have an upgrade deadline, I'm reluctant to add a validation for this.

Where would it be applied? How will it complicate the course configuration process, which is already quite messy? What do we do if the upgrade deadline hasn't been determined at the time of courserun creation? And does it tie our hands for offering new types of courses in the future, like SPOCs (small private courses) or other courses without certificates?

@cp-at-mit cp-at-mit requested a review from annagav August 28, 2024 19:29
Copy link
Contributor

@annagav annagav left a comment

Choose a reason for hiding this comment

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

👍

@cp-at-mit cp-at-mit merged commit 21a4188 into main Aug 28, 2024
7 checks passed
@cp-at-mit cp-at-mit deleted the 5284-mitxonline-dashboard-crashes-when-courserun-has-no-upgrade-deadline branch August 28, 2024 20:26
@odlbot odlbot mentioned this pull request Sep 3, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants