make course advanced setting invite_only configurable site-wide #27252

asadiqbal08 · 2021-04-06T07:48:13Z

Fixes mitodl#135 and reference to previous PR# https://github.com/edx/edx-platform/pull/20596

Background:

We would like to make the ~~default~~ value of the invitation_required attribute of the courseware object configurable via a feature flag, to be named ~~COURSE_DEFAULT_INVITE_ONLY~~ COURSES_INVITE_ONLY

As it is currently, this is a manually updated flag in the course advanced settings and we would like to be able to manage the ~~default~~ value across all courses in a given deployment. The default value of the added setting should remain the same as the current setting of False, but allow us to set it to True through the existing settings mechanism.

We have merged this solution in our forked repository mitodl#126.

openedx-webhooks · 2021-04-06T07:48:24Z

Thanks for the pull request, @asadiqbal08! I've created OSPR-5719 to keep track of it in JIRA, where we prioritize reviews. 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:

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.

natabene · 2021-04-06T15:29:45Z

@asadiqbal08 Thank you for your contribution. Looks like this is ready for our review.

pdpinch · 2021-04-06T17:29:44Z

lms/djangoapps/courseware/views/views.py

@@ -928,7 +930,7 @@ def course_about(request, course_id):

        # Used to provide context to message to student if enrollment not allowed
        can_enroll = bool(request.user.has_perm(ENROLL_IN_COURSE, course))
-        invitation_only = course.invitation_only
+        invitation_only = is_course_default_invite_only_enabled() or course.invitation_only


Will course.invitation_only always be True or False, or can it be not set (None)?

It strikes me that with the logic here, if the toggle is set to True, it will never be possible to override it for one course. All courses will be invite only.

@pdpinch It either be True/False.

It strikes me that with the logic here, if the toggle is set to True, it will never be possible to override it for one course. All courses will be invite only.

hmmm but what I get yet, We are giving precedence to COURSE_DEFAULT_INVITE_ONLY flag in the task.
I mean, If is_course_default_invite_only_enabled is True then all courses will be invite only. Is it not the requirement here ? mean, reference to earlier merge: PR

As is, the PR meets the MIT ODL use case, but I feel like the setting name suggests a different behavior. COURSE_DEFAULT_INVITE_ONLY sounds like it should only change the default setting, instead of overriding the setting for all courses.

We could either:

Change the logic, or

Change the setting to COURSES_INVITE_ONLY

pdpinch · 2021-04-09T15:39:45Z

I updated the PR description and subject to reflect the new setting name and expected behavior. This should be reflected in the commit message as well.

pdpinch

LGTM!

sarina

Reviewed this from a naive perspective of the feature. Just a few comments for clarity, but the code looks good.

sarina · 2021-04-09T17:08:14Z

lms/djangoapps/courseware/tests/test_access.py

@@ -500,6 +501,39 @@ def test__has_access_course_can_enroll(self):
        )
        assert not access._has_access_course(user, 'enroll', course)

+    @override_settings(COURSES_INVITE_ONLY=False)
+    def test__course_default_invite_only_flag_false(self):
+        """Tests that default value of COURSES_INVITE_ONLY as False."""


As someone rather new to these features, the tests are somewhat unclear. Look at the above test - there's lots of comments around the assert statements & mock course creation to explain what's going on. Could you please add such comments for clarity? I wouldn't hate a more descriptive docstring, either.

Also a nit on the docstring format to do:

""" Tests that default value of COURSES_INVITE_ONLY as False. """

sarina · 2021-04-09T17:09:17Z

lms/djangoapps/courseware/tests/test_access.py

+        course = self._mock_course(invitation=False)
+        self.assertFalse(access._has_access_course(user, 'enroll', course))
+
+    def _mock_course(self, invitation):


Since this is specific to your tests, I wonder if it should have a more specific name such as _mock_course_with_invitation or something better than that, ha

sarina · 2021-04-09T17:10:31Z

lms/djangoapps/courseware/toggles.py

+# .. toggle_implementation: SettingToggle
+# .. toggle_type: feature_flag
+# .. toggle_default: False
+# .. toggle_description: Set this to manage the default value for INVITE_ONLY across all courses in a given deployment


I'm not sure I 100% follow this description, and I'm not sure why. Maybe Setting this sets the default value of INVITE_ONLY across all courses in a given deployment

edx-status-bot · 2021-04-12T12:09:58Z

Your PR has finished running tests. There were no failures.

sarina

Please squash & rebase then myself or @pdpinch can merge

edit: please also use Conventional Commits

openedx-webhooks · 2021-04-14T12:06:50Z

@asadiqbal08 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

sarina · 2021-04-14T12:25:32Z

@pdpinch please be sure to use Conventional Commits before merging in the future - thanks!

Setting COURSES_INVITE_ONLY to True overrides the INVITE_ONLY setting across all courses in a given deployment. Co-authored-by: tasawernawaz <[email protected]> Co-authored-by: asadiqbal08 <[email protected]> (cherry picked from commit 80298ae)

pdpinch · 2021-09-10T23:01:35Z

Sorry @sarina I missed your comment https://github.com/edx/edx-platform/pull/27252#issuecomment-819477760 -- but I thought this was a conventional commit: edx/edx-platform@80298ae

Did I overlook something?

sarina · 2021-09-13T12:14:18Z

@pdpinch huh. I just looked at the list of commits on the PR and saw none of them were conventional:

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Apr 6, 2021

openedx-webhooks added awaiting prioritization and removed needs triage labels Apr 6, 2021

pdpinch reviewed Apr 6, 2021

View reviewed changes

make default value of invite_only configurable

7d71607

asadiqbal08 force-pushed the asadiqbal08/issue-123 branch from 6b9c553 to 7d71607 Compare April 8, 2021 10:36

asadiqbal08 and others added 2 commits April 8, 2021 16:41

Change the setting to COURSES_INVITE_ONLY

273fb0f

Merge branch 'master' into asadiqbal08/issue-123

fa43038

pdpinch self-assigned this Apr 8, 2021

pdpinch changed the title ~~make default value of invite_only configurable~~ make course advanced setting invite_only configurable site-wide Apr 9, 2021

pdpinch approved these changes Apr 9, 2021

View reviewed changes

sarina requested changes Apr 9, 2021

View reviewed changes

asadiqbal08 added 4 commits April 12, 2021 15:18

Feedback for the tests docstrings

418c50b

Feedback over the toggle description

d5c8d44

Quality fixes

c51ddf6

Update test_access.py

998d6e9

asadiqbal08 requested a review from sarina April 12, 2021 12:11

sarina approved these changes Apr 12, 2021

View reviewed changes

openedx-webhooks added changes requested and removed awaiting prioritization labels Apr 13, 2021

pdpinch merged commit 80298ae into openedx:master Apr 14, 2021

pdpinch deleted the asadiqbal08/issue-123 branch April 14, 2021 12:06

openedx-webhooks added merged and removed changes requested labels Apr 14, 2021

pdpinch mentioned this pull request Apr 14, 2021

Residential Lilac Release Epic mitodl/edx-platform#253

Closed

12 tasks

pdpinch mentioned this pull request May 25, 2021

Cherry pick: make course advanced setting invite_only configurable site-wide mitodl/edx-platform#263

Closed

farhaanbukhsh mentioned this pull request Sep 18, 2024

feat: DEFAULT_COURSE_INVITATION_ONLY allow for invitation_only new co… #34504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make course advanced setting invite_only configurable site-wide #27252

make course advanced setting invite_only configurable site-wide #27252

asadiqbal08 commented Apr 6, 2021 •

edited by pdpinch

Loading

openedx-webhooks commented Apr 6, 2021

natabene commented Apr 6, 2021

pdpinch Apr 6, 2021

asadiqbal08 Apr 7, 2021

pdpinch Apr 7, 2021

pdpinch commented Apr 9, 2021

pdpinch left a comment

sarina left a comment

sarina Apr 9, 2021

sarina Apr 9, 2021

sarina Apr 9, 2021

edx-status-bot commented Apr 12, 2021

sarina left a comment •

edited

Loading

openedx-webhooks commented Apr 14, 2021

sarina commented Apr 14, 2021

pdpinch commented Sep 10, 2021

sarina commented Sep 13, 2021

make course advanced setting invite_only configurable site-wide #27252

make course advanced setting invite_only configurable site-wide #27252

Conversation

asadiqbal08 commented Apr 6, 2021 • edited by pdpinch Loading

openedx-webhooks commented Apr 6, 2021

natabene commented Apr 6, 2021

pdpinch Apr 6, 2021

Choose a reason for hiding this comment

asadiqbal08 Apr 7, 2021

Choose a reason for hiding this comment

pdpinch Apr 7, 2021

Choose a reason for hiding this comment

pdpinch commented Apr 9, 2021

pdpinch left a comment

Choose a reason for hiding this comment

sarina left a comment

Choose a reason for hiding this comment

sarina Apr 9, 2021

Choose a reason for hiding this comment

sarina Apr 9, 2021

Choose a reason for hiding this comment

sarina Apr 9, 2021

Choose a reason for hiding this comment

edx-status-bot commented Apr 12, 2021

sarina left a comment • edited Loading

Choose a reason for hiding this comment

openedx-webhooks commented Apr 14, 2021

sarina commented Apr 14, 2021

pdpinch commented Sep 10, 2021

sarina commented Sep 13, 2021

asadiqbal08 commented Apr 6, 2021 •

edited by pdpinch

Loading

sarina left a comment •

edited

Loading