Add the default enrollment start date on course creation #30954

dyudyunov · 2022-09-07T07:33:44Z

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 refactor: add enrollment start openedx-demo-course#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 is disabled (FEATURES['ENABLE_COURSE_DISCOVERY'] = False)
I wonder why DEFAULT_START_DATE is hardcoded (1 Jan 2030)? How and when is it updated?
The proposed behavior is togglable and is disabled by default. To enable it - update your settings.FEATURES for cms:
FEATURES['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE'] = True

openedx-webhooks · 2022-09-07T07:33:47Z

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 · 2022-09-07T13:11:46Z

@natabene this one is ready for the review

nedbat · 2022-10-31T19:51:35Z

📣 💥 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 · 2022-11-02T13:42:59Z

@nedbat rebase is done

mariajgrimaldi · 2022-11-17T14:38:29Z

I can take this one :)

mphilbrick211 · 2022-12-12T21:26:55Z

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

This looks pretty good! I just left a question.

Also, I had the same question regarding the DEFAULT_START_DATE constant, and maybe this is the reason. I was going to suggest changing it, but maybe that should be part of another effort.

xmodule/tests/test_course_module.py

mariajgrimaldi · 2022-12-13T19:36:11Z

@mphilbrick211: thanks for the reminder :)

mariajgrimaldi · 2022-12-19T11:32:17Z

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 · 2022-12-19T13:07:15Z

@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 · 2022-12-19T14:27:30Z

@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.

mariajgrimaldi · 2023-07-24T15:25:23Z

I'll review this during the week! Thanks

mariajgrimaldi · 2023-08-09T20:47:44Z

Sorry - it's been crazy lately. This is on my review queue! Thanks for your patience.

dyudyunov · 2023-08-10T06:50:46Z

It would be great to create a backport PR to Palm. I'm going to do so as soon as this one will be accepted

mariajgrimaldi

Thanks again for the contribution, @dyudyunov! I tested this again by ensuring the default enrollment start date was used as default if the flag was turned on.

I'll post a message in the edx-platform CC slack channel so people know about this change! See you there.

robrap

Just added some comments about the toggle. Thank you.

robrap · 2023-08-15T14:04:52Z

xmodule/course_block.py

+# .. toggle_implementation: SettingDictToggle
+# .. toggle_default: False
+# .. toggle_description: When enabled the newly created courses will have the enrollment_start_date
+#   set to DEFAULT_START_DATE.


Could you add something like: "The default behavior, when this is disabled, is ...".

Done, please check

robrap · 2023-08-15T14:08:02Z

xmodule/course_block.py

+#   set to DEFAULT_START_DATE.
+# .. toggle_use_cases: open_edx
+# .. toggle_creation_date: 2023-06-22
+# .. toggle_target_removal_date: None


You should drop this annotation altogether.

Suggested change

# .. toggle_target_removal_date: None

In the future, another annotation will make it clear that this is a permanent toggle, but for now, you could something like: "This is intended to be a permanent option."

Replaced with the proposed description

robrap · 2023-08-15T14:11:09Z

xmodule/course_block.py

@@ -57,6 +58,17 @@
 COURSE_VIDEO_SHARING_PER_VIDEO = 'per-video'
 COURSE_VIDEO_SHARING_ALL_VIDEOS = 'all-on'
 COURSE_VIDEO_SHARING_NONE = 'all-off'
+# .. toggle_name: FEATURES['CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE']


No change is being requested in this comment.

[inform] There has been an unofficial move away from using the FEATURES dict. We should probably have a DEPR to at least discuss it.
FYI: @feanil

[inform] I created #33026 to discuss this topic. Again, no request for change.

robrap

Thanks @dyudyunov. I added some comments about the toggle annotations. Hopefully they are clear. Please don't be blocked on any re-review by me unless you wish to. I am not adding an approval simply because I didn't review anything other than this annotation. Thanks for your updates.

robrap · 2023-08-16T13:09:31Z

xmodule/course_block.py

+# .. toggle_description: The default behavior, when this is disabled, is that newly created course has no
+#   enrollment_start date set. When the feature is enabled - the newly created courses will have the
+#   enrollment_start_date set to DEFAULT_START_DATE.


I fixed a typo and moved the the sentence about it being a permanent option to the description.

Suggested change

# .. toggle_description: The default behavior, when this is disabled, is that newly created course has no

# enrollment_start date set. When the feature is enabled - the newly created courses will have the

# enrollment_start_date set to DEFAULT_START_DATE.

# .. toggle_description: The default behavior, when this is disabled, is that a newly created course has no

# enrollment_start date set. When the feature is enabled - the newly created courses will have the

# enrollment_start_date set to DEFAULT_START_DATE. This is intended to be a permanent option.

My understanding is that this toggle was added because some orgs (2U) did not want this toggled on. I do not know much about this, and if I were an operator, I would wonder why I should choose one option over the option:
i. What is the consequence of having no start date set? Without knowing anything, that seems like it would be bad, but somehow it works for 2U. Why?
ii. I think I read that DEFAULT_START_DATE is intended to be a date in the distant future so courses will not appear. Is that accurate, or only if you are using this toggle? Is the default used for other things? Maybe you could speak to that a bit more here?

Please update my first point, because it is simple. Regarding my second point, I am not the owner of this particular area of the code, so this is just a suggestion. You can decide how much you update the toggle docs, but I personally would rather longer docs that help me understand more completely what it is really for. That said, if you think most operators would know all of these things, maybe you've provided enough.

Thanks for the detailed review

What is the consequence of having no start date set?

The toggle has no effect on the start date but only on the enrollment start date, but I've got your point, I'll think about how to describe the toggle behavior with more details

I think I read that DEFAULT_START_DATE is intended to be a date in the distant future so courses will not appear. Is that accurate, or only if you are using this toggle? Is the default used for other things? Maybe you could speak to that a bit more here?

I just reused the DEFAULT_START_DATE constant. It was originally used only as a default value for the course start date and no changes were made there

robrap · 2023-08-16T13:28:04Z

xmodule/course_block.py

+#   enrollment_start_date set to DEFAULT_START_DATE.
+# .. toggle_use_cases: open_edx
+# .. toggle_creation_date: 2023-06-22
+# .. toggle_target_removal_date: This is intended to be a permanent option.


Apologies that I wasn't clear. This annotation should not be used for permanent options. The sentence I added in the description is to help make the permanence of this toggle more explicit.

Suggested change

# .. toggle_target_removal_date: This is intended to be a permanent option.

In the future, openedx/edx-toggles#284 proposes atoggle_permanent_justification annotation, that would make this explicit.

The course is visible on the main page right after creation. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is 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 the course start in the future. But `can_enroll` returns True because the 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 ``` Set the enrollment_start the same as a course start by default - this fix will work for the course catalog page when the course discovery is disabled (FEATURES['ENABLE_COURSE_DISCOVERY'] = False) - I wonder why DEFAULT_START_DATE is hardcoded (1 Jan 2030)? How and when is it updated?

dyudyunov · 2023-08-21T13:01:21Z

@mariajgrimaldi hi 👋

Is there something else blocking us from merging?)

mariajgrimaldi · 2023-08-22T14:41:00Z

I'll be merging this now!

openedx-webhooks · 2023-08-22T14:42:22Z

@dyudyunov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-08-22T15:50:59Z

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-08-22T16:11:48Z

2U Release Notice: This PR has been deployed to the edX production environment.

* fix: main page course listing The course is visible on the main page right after creation when the feature toggle `CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE` is on. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is 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 the course start in the future. But `can_enroll` returns True because the 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 ``` Set the enrollment_start the same as a course start by default

mariajgrimaldi · 2023-08-31T21:09:36Z

@dyudyunov: let us know when you have the backport PR ready :)

This is a backport from the master branch: openedx#30954 The course is visible on the main page right after creation. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is 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 the course start in the future. But `can_enroll` returns True because the 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 ``` Set the enrollment_start the same as a course start by default if the CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE feature toggle is enabled (default is False).

This is a backport from the master branch: #30954 The course is visible on the main page right after creation. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is 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 the course start in the future. But `can_enroll` returns True because the 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 ``` Set the enrollment_start the same as a course start by default if the CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE feature toggle is enabled (default is False).

…dx#33150) This is a backport from the master branch: openedx#30954 The course is visible on the main page right after creation. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is 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 the course start in the future. But `can_enroll` returns True because the 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 ``` Set the enrollment_start the same as a course start by default if the CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE feature toggle is enabled (default is False).

This is a backport from the master branch: openedx/edx-platform#30954 The course is visible on the main page right after creation. So anonymous users can see them and access the course about page for the courses without valid data (e.g. they will see the default course overview) When courses list filtering is 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 the course start in the future. But `can_enroll` returns True because the 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 ``` Set the enrollment_start the same as a course start by default if the CREATE_COURSE_WITH_DEFAULT_ENROLLMENT_START_DATE feature toggle is enabled (default is False).

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 7, 2022

dyudyunov force-pushed the fix-main-page-course-visibility branch 9 times, most recently from 7faad23 to 00a98c0 Compare September 7, 2022 12:45

dyudyunov mentioned this pull request Oct 4, 2022

refactor: add enrollment start openedx/openedx-demo-course#38

Merged

dyudyunov force-pushed the fix-main-page-course-visibility branch from 00a98c0 to f3149f5 Compare November 2, 2022 12:30

mariajgrimaldi self-assigned this Nov 17, 2022

mariajgrimaldi self-requested a review November 17, 2022 14:38

mariajgrimaldi reviewed Dec 13, 2022

View reviewed changes

xmodule/tests/test_course_module.py Outdated Show resolved Hide resolved

mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 16, 2022

dyudyunov force-pushed the fix-main-page-course-visibility branch from f3149f5 to d659f87 Compare December 19, 2022 13:11

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 19, 2022

dyudyunov force-pushed the fix-main-page-course-visibility branch from d659f87 to b4b1977 Compare December 19, 2022 13:28

dyudyunov requested a review from mariajgrimaldi December 19, 2022 13:28

mariajgrimaldi self-requested a review July 24, 2023 15:25

mariajgrimaldi approved these changes Aug 15, 2023

View reviewed changes

robrap reviewed Aug 15, 2023

View reviewed changes

dyudyunov force-pushed the fix-main-page-course-visibility branch from f15f6e2 to 4eb68c7 Compare August 16, 2023 07:11

robrap reviewed Aug 16, 2023

View reviewed changes

mariajgrimaldi mentioned this pull request Aug 16, 2023

Backport(s) before palm.3 release openedx/wg-build-test-release#302

Closed

32 tasks

dyudyunov added 5 commits August 16, 2023 17:45

feat: make default enrollment start togglable

af6b86d

fix: disable linting warnings for reimport

fe84a4e

docs: format toggle annotation

caee0a4

docs: update toggle docstring

a581223

dyudyunov force-pushed the fix-main-page-course-visibility branch from 45da7b9 to a581223 Compare August 16, 2023 14:45

mariajgrimaldi merged commit 4340a83 into openedx:master Aug 22, 2023
42 checks passed

dyudyunov mentioned this pull request Sep 1, 2023

Add the default enrollment start date on course creation #33150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the default enrollment start date on course creation #30954

Add the default enrollment start date on course creation #30954

dyudyunov commented Sep 7, 2022 •

edited

Loading

openedx-webhooks commented Sep 7, 2022 •

edited

Loading

dyudyunov commented Sep 7, 2022

nedbat commented Oct 31, 2022

dyudyunov commented Nov 2, 2022

mariajgrimaldi commented Nov 17, 2022

mphilbrick211 commented Dec 12, 2022

mariajgrimaldi left a comment

mariajgrimaldi commented Dec 13, 2022

mariajgrimaldi commented Dec 19, 2022

dyudyunov commented Dec 19, 2022 •

edited

Loading

mariajgrimaldi commented Dec 19, 2022

mariajgrimaldi commented Jul 24, 2023

mariajgrimaldi commented Aug 9, 2023

dyudyunov commented Aug 10, 2023

mariajgrimaldi left a comment

robrap left a comment

robrap Aug 15, 2023

dyudyunov Aug 16, 2023

robrap Aug 15, 2023

dyudyunov Aug 16, 2023

robrap Aug 15, 2023

robrap Aug 16, 2023

robrap left a comment

robrap Aug 16, 2023

dyudyunov Aug 16, 2023

robrap Aug 16, 2023

dyudyunov commented Aug 21, 2023

mariajgrimaldi commented Aug 22, 2023

openedx-webhooks commented Aug 22, 2023

edx-pipeline-bot commented Aug 22, 2023

edx-pipeline-bot commented Aug 22, 2023

mariajgrimaldi commented Aug 31, 2023

Add the default enrollment start date on course creation #30954

Add the default enrollment start date on course creation #30954

Conversation

dyudyunov commented Sep 7, 2022 • edited Loading

Problem

Root cause

Fix

Known Issues:

Notes

openedx-webhooks commented Sep 7, 2022 • edited Loading

dyudyunov commented Sep 7, 2022

nedbat commented Oct 31, 2022

dyudyunov commented Nov 2, 2022

mariajgrimaldi commented Nov 17, 2022

mphilbrick211 commented Dec 12, 2022

mariajgrimaldi left a comment

Choose a reason for hiding this comment

mariajgrimaldi commented Dec 13, 2022

mariajgrimaldi commented Dec 19, 2022

dyudyunov commented Dec 19, 2022 • edited Loading

mariajgrimaldi commented Dec 19, 2022

mariajgrimaldi commented Jul 24, 2023

mariajgrimaldi commented Aug 9, 2023

dyudyunov commented Aug 10, 2023

mariajgrimaldi left a comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyudyunov commented Aug 21, 2023

mariajgrimaldi commented Aug 22, 2023

openedx-webhooks commented Aug 22, 2023

edx-pipeline-bot commented Aug 22, 2023

edx-pipeline-bot commented Aug 22, 2023

mariajgrimaldi commented Aug 31, 2023

dyudyunov commented Sep 7, 2022 •

edited

Loading

openedx-webhooks commented Sep 7, 2022 •

edited

Loading

dyudyunov commented Dec 19, 2022 •

edited

Loading