fix: main page course listing

[dyudyunov]

Problem

Course is visible in the main page right after creation. So anonymous users can see them and access course about page for the courses without valid data (e.g. they will see default course overview)

Root cause

When courses list filtering processed it checks the see_exists permission for the anonymous user. Actually, see_exists means can_load OR can_enroll.

can_load is False in our case because course start is in the future.

But can_enroll returns True because course's enrollment_start and enrollment_end dates are blank:

enrollment_start = courselike.enrollment_start or datetime.min.replace(tzinfo=UTC)
enrollment_end = courselike.enrollment_end or datetime.max.replace(tzinfo=UTC)
if enrollment_start < now < enrollment_end:
    debug("Allow: in enrollment period")
    return ACCESS_GRANTED

Fix

Set the enrollment_start the same as a course start by default

Known Issues:

• this requires the https://github.com/openedx/openedx-demo-course/pull/38 to be merged. Otherwise, it will break the clean (first) deployment while trying to enroll audit and verified users in the Demo Course

Notes

• this fix will work for the course catalog page when the course discovery disabled (FEATURES['ENABLE_COURSE_DISCOVERY'] = False)
• I wounder why DEFAULT_START_DATE is hardcoded (1 Jan 2030)? How and when is it updated?

[openedx-webhooks]

Thanks for the pull request, @dyudyunov! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[dyudyunov]

@natabene this one is ready for the review

[nedbat]

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to keep passing required checks.

We added a new required check, "Tests Successful," that this PR does not yet run. Rebasing will get it started.

If you have any questions, please reach out to the Architecture team (either #architecture on Open edX Slack or #external-architecture on edX internal Slack).

[dyudyunov]

@nedbat rebase is done

[mariajgrimaldi]

I can take this one :)

[mphilbrick211]

Hi @dyudyunov! Your CLA check looks stuck - would you mind rebasing to see if that helps it go through?

Also, friendly ping @mariajgrimaldi for review :)

[mariajgrimaldi]

@mphilbrick211: thanks for the reminder :)

[mariajgrimaldi]

I tested before and after this PR:

Before

After

The course was still visible when accessing the main listing page, but the enrollment was closed for an anonymous user. I first thought the course would be removed from the main page listing. Is this the behavior you expected? If yes, we should change the PR name since it suggests we're fixing the main page listing instead of the enrollment for brand-new courses.

[dyudyunov]

@mariajgrimaldi no, it's not the expected behaviour

Please, confirm that:

• your COURSE_CATALOG_VISIBILITY_PERMISSION setting is set to see_exist (the default one)
• you have cleared the cache after your change in course dates or removed the @cache_if_anonymous() decorator for the view

[mariajgrimaldi]

@dyudyunov: it's not working for me, the course is still visible in the course listing. I'll be researching what's wrong with my setup.

[dyudyunov]

@mariajgrimaldi try this way:

• create a brand new course
• check that it has the enrollment start by default
• check the localhost:18000/ page to verify it is not shown there (I've tested with the anonymous user in the incognito window)

NOTE: the changes are aimed at the _can_enroll_courselike method. So checking it I supposed you're using the staff account or your user could be in the CourseEnrollmentAllowed table.

Also, I want to remind you that the https://github.com/openedx/openedx-demo-course/pull/38 should be merged first to avoid the first-deployment issues related to test users enrollments in the demo course (the honor user will not be able to enroll because enrollment_start is in the future)

[mariajgrimaldi]

@dyudyunov: good! thanks for the guidance. It's working now.

@mphilbrick211: can we ping someone to review the dependent PR (https://github.com/openedx/openedx-demo-course/pull/38)? I can review but I don't have the right permissions for merging.

[mphilbrick211]

Thanks for flagging, @mariajgrimaldi! I have pinged Tim and Jeremy in #38 https://github.com/openedx/openedx-demo-course/pull/38

[itsjeyd]

Hey @dyudyunov! While https://github.com/openedx/openedx-demo-course/pull/38 is waiting for review (left another ping there just now), could you please fix merge conflicts here?

CC @mphilbrick211 @mariajgrimaldi

[dyudyunov]

Hey @dyudyunov! While openedx/openedx-demo-course#38 is waiting for review (left another ping there just now), could you please fix merge conflicts here?

CC @mphilbrick211 @mariajgrimaldi

Done

[itsjeyd]

Thanks @dyudyunov!

I'm marking this as blocked on other work again -- technical review for https://github.com/openedx/openedx-demo-course/pull/38 is complete now (cf. https://github.com/openedx/openedx-demo-course/pull/38#pullrequestreview-1238989191) but that PR also needs product review.

CC @mphilbrick211 @mariajgrimaldi

[mphilbrick211]

@mariajgrimaldi - now that #38 is merged, would you mind reviewing/merging this one? It was blocked by #38.

[mariajgrimaldi]

@mphilbrick211 of course! thanks for the reminder :)

[mariajgrimaldi]

I've been thinking hard about this PR. Now, enrollment starts will have a default making courses unavailable for anonymous users after creation, which sounds good, IMO. My issue relies on the fact that this PR doesn't sound like a bug fix but a behavior change we need to study a bit more.

@mphilbrick211: can we get a hand from the product side? I'm not sure this new behavior is what we want for the platform.

[mphilbrick211]

@jmakowski1123 flagging this for you.

[jmakowski1123]

Hi @dyudyunov , Hi @mariajgrimaldi

Thanks for this contribution! I agree with @mariajgrimaldi , that this PR is proposing a behavior change to the platform, rather than fixing a bug. There are more details I'd like to understand from a product perspective.

Because this is a product-oriented proposal and the actual scope may be bigger than this single PR, I've created a Feature Ticket here. I'd like to shift the communication about product questions to this ticket. I wrote some questions in the comments that need some clarification. @dyudyunov , can we continue chatting here?

[mphilbrick211]

@jmakowski1123 any update on this?

[mphilbrick211]

@jmakowski1123 any update on this?

@jmakowski1123 - disregard this - I pinged Steve in the roadmap ticket.

[mphilbrick211]

Hi @mariajgrimaldi! Product review is complete on this - would you mind reviewing and merging if you're able? Thanks!

[mariajgrimaldi]

I'll review this again today. Sorry for the delay

[mphilbrick211]

Hi @dyudyunov! We've had a few new tests pop up on this pull request (shellcheck). Would you mind running these? Thanks!

CC: @mariajgrimaldi

[mariajgrimaldi]

In case anyone wants to replicate the testing of this PR, this is what I did. First, I configured my testing environment by doing the following:

1. Ensure FEATURES['ENABLE_COURSE_DISCOVERY'] = False
2. Ensure COURSE_CATALOG_VISIBILITY_PERMISSION = 'see_exists' (it's not the default in tutor dev)
3. Disable cache from course listing page
4. Create brand new course

Then, I tested two case scenarios:

1. Courses with enrollment date in the future are not visible from the main course page: after creating the course, check it has default enrollment date and it's not visible from the main course listing.

2. Courses with enrollment date in the future are visible from the main course page: after creating the course, check it has default enrollment date and it's visible from the main course listing. Besides the basic setup we did previously, for this test case I:

• Enabled course waffle flag: course_experience.pre_start_access
• Ensured visible_to_staff_only = False
• Ensured course is not expired

Which made the course visible although its enrollment date was far in the future.

[mariajgrimaldi]

@jmakowski1123 @mphilbrick211: hello there! before merging this PR, should we notify someone of these changes?

[dyudyunov]

@dyudyunov: this looks good! Thank you for being so patient. Can you change the PR name for something more descriptive of the changes?

Hi @mariajgrimaldi Thanks for the precise review 💪

I've changed the title, please let me know if it's not descriptive enough

[dyudyunov]

@mphilbrick211 hi

I've rebased on the master to trigger the tests run