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

Updates the code behind the More Dates selector to work without the course page #1900

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

jkachel
Copy link
Contributor

@jkachel jkachel commented Sep 18, 2023

The course page was removed from the courserun API response in an earlier PR, but the More Dates selector depended on it being there to determine which course run to show you. This PR changes that logic to something more simple - either you've selected a course run or you haven't.

What are the relevant tickets?

Fixes mitol/hq#2396 partially

Description (What does it do?)

If a course run has been selected, use that instead of the default (which is just the first courserun that appears in the list when we go to fetch them).

How can this be tested?

Set up a course with multiple concurrent runs. (For my testing, I made two with run tags ending in A and B so it'd be obvious which is which.) Then, load the course page. You should see the More Dates selector.

Clicking Enroll Now without choosing a course in the More Dates selector should enroll you in the first course on the list.

Choosing a course from the More Dates selector and then clicking Enroll Now should enroll you in that specific course run.

Additional Context

Your Enroll Now button may not change to Enrolled depending on whether or not you have a working edX instance and a corresponding course there for your test course runs. For example, I am enrolled in both these courses. However, Tutor isn't aware of either - I set one manually to be EdX Enrolled = True. (These screenshots also demonstrate that the selector works.)

Screenshot 2023-09-18 at 1 14 18 PM
Screenshot 2023-09-18 at 1 14 26 PM

Copy link
Contributor

@collinpreston collinpreston left a comment

Choose a reason for hiding this comment

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

Removed comment

Copy link
Contributor

@collinpreston collinpreston left a comment

Choose a reason for hiding this comment

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

Should the "more dates" section only show CourseRuns which have live = True?

@jkachel
Copy link
Contributor Author

jkachel commented Sep 18, 2023

Should the "more dates" section only show CourseRuns which have live = True?

Don't think so - I updated the template to filter out runs that aren't live in 8081f98

@jkachel jkachel force-pushed the jkachel/hq-2396-fix-for-more-dates-dropdown branch from fda9ce3 to c7e16d3 Compare September 18, 2023 19:33
@jkachel
Copy link
Contributor Author

jkachel commented Sep 18, 2023

Pulled the live filtering - we can address that in a separate PR (per Slack convo) since the better way to do this potentially changes other things.

Copy link
Contributor

@collinpreston collinpreston left a comment

Choose a reason for hiding this comment

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

Nice find and fix!

@jkachel jkachel merged commit 5845581 into main Sep 18, 2023
@odlbot odlbot mentioned this pull request Sep 18, 2023
11 tasks
@jkachel jkachel deleted the jkachel/hq-2396-fix-for-more-dates-dropdown branch September 20, 2023 19:39
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.

2 participants