feat: feature flag to disable Advanced Settings #32015

0x29a · 2023-03-31T11:54:21Z

Description

Adds the new Studio feature flag, DISABLE_ADVANCED_SETTINGS, which disables access to course advanced settings for non-staff users. Also, it removes "Advanced Settings" from the "Settings" dropdown.

Testing instructions

Apply this patch:

diff --git a/cms/envs/common.py b/cms/envs/common.py
index 56ba9187c6..f6dce2edb8 100644
--- a/cms/envs/common.py
+++ b/cms/envs/common.py
@@ -524,7 +524,7 @@ FEATURES = {
     # .. toggle_use_cases: open_edx
     # .. toggle_creation_date: 2023-03-31
     # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32015
-    'DISABLE_ADVANCED_SETTINGS': False,
+    'DISABLE_ADVANCED_SETTINGS': True,
 }
 
 # .. toggle_name: ENABLE_COPPA_COMPLIANCE

Verify that that any user except Django admin staff neither can see the "Advanced Settings" dropdown element nor access the page directly.

private-ref

openedx-webhooks · 2023-03-31T11:54:27Z

Thanks for the pull request, @0x29a! 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.

farhaanbukhsh

👍

@0x29a I have added few nits, but it looks good overall!

✅ I tested this on master devstack
✅ I read through the code
❌ I checked for accessibility issues
✅ Includes documentation
❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

farhaanbukhsh · 2023-04-03T08:00:25Z

cms/djangoapps/contentstore/views/course.py

@@ -752,6 +762,7 @@ def course_index(request, course_key):
            'frontend_app_publisher_url': frontend_app_publisher_url,
            'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_block.id),
            'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id),


Suggested change

'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id),

'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id) if has_advanced_settings_access(request.user) else None,

@farhaanbukhsh, I did the same initially, but then I noticed this code above in this same file:

edx-platform/cms/templates/widgets/header.html

Lines 27 to 48 in 9caa31b

% if context_course:

<%

course_key = context_course.id

url_encoded_course_key = quote(six.text_type(course_key).encode('utf-8'), safe='')

index_url = reverse('course_handler', kwargs={'course_key_string': six.text_type(course_key)})

course_team_url = reverse('course_team_handler', kwargs={'course_key_string': six.text_type(course_key)})

assets_url = reverse('assets_handler', kwargs={'course_key_string': six.text_type(course_key)})

textbooks_url = reverse('textbooks_list_handler', kwargs={'course_key_string': six.text_type(course_key)})

videos_url = reverse('videos_handler', kwargs={'course_key_string': six.text_type(course_key)})

import_url = reverse('import_handler', kwargs={'course_key_string': six.text_type(course_key)})

course_info_url = reverse('course_info_handler', kwargs={'course_key_string': six.text_type(course_key)})

export_url = reverse('export_handler', kwargs={'course_key_string': six.text_type(course_key)})

settings_url = reverse('settings_handler', kwargs={'course_key_string': six.text_type(course_key)})

grading_url = reverse('grading_handler', kwargs={'course_key_string': six.text_type(course_key)})

advanced_settings_url = reverse('advanced_settings_handler', kwargs={'course_key_string': six.text_type(course_key)})

tabs_url = reverse('tabs_handler', kwargs={'course_key_string': six.text_type(course_key)})

certificates_url = ''

if settings.FEATURES.get("CERTIFICATES_HTML_VIEW") and context_course.cert_html_view_enabled:

certificates_url = reverse('certificates_list_handler', kwargs={'course_key_string': six.text_type(course_key)})

checklists_url = reverse('checklists_handler', kwargs={'course_key_string': six.text_type(course_key)})

pages_and_resources_mfe_enabled = ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND.is_enabled(context_course.id)

%>

So, if context_course is True, the button will be showed despite DISABLE_ADVANCED_SETTINGS set to True.

Makes sense :)

farhaanbukhsh · 2023-04-03T08:00:56Z

cms/templates/widgets/header.html

@@ -120,9 +120,11 @@ <h3 class="title"><span class="label"><span class="label-prefix sr">${_("Course"
                    <a href="${mfe_proctored_exam_settings_url}">${_("Proctored Exam Settings")}</a>
                  </li>
                  % endif
+                  % if advance_settings_access:


Suggested change

% if advance_settings_access:

% if advanced_settings_url:

Same concern as #32015 (comment).

cms/djangoapps/contentstore/tests/test_course_settings.py

farhaanbukhsh

Few nits ;)

farhaanbukhsh · 2023-04-03T09:22:36Z

cms/djangoapps/contentstore/views/course.py

+    return (
+        not settings.FEATURES.get('DISABLE_ADVANCED_SETTINGS', False)
+        or user.is_staff
+    )


Should we also add global admin to this? Doesn't make sense to have staff but not admin!

Done, @farhaanbukhsh.

farhaanbukhsh · 2023-04-03T20:15:21Z

cms/djangoapps/contentstore/views/course.py

@@ -752,6 +762,7 @@ def course_index(request, course_key):
            'frontend_app_publisher_url': frontend_app_publisher_url,
            'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_block.id),
            'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id),


Makes sense :)

mphilbrick211 · 2023-04-03T23:31:19Z

Hi @0x29a @farhaanbukhsh - is this ready to merge? If so, @farhaanbukhsh are you able to do so?

pdpinch · 2023-04-05T13:57:02Z

cms/envs/common.py

+    # .. toggle_name: FEATURES['DISABLE_ADVANCED_SETTINGS']
+    # .. toggle_implementation: DjangoSetting
+    # .. toggle_default: False
+    # .. toggle_description: Set to True to disable the advanced settings page in Studio for all users except staff.


Can you clarify what is meant by 'staff' here? Is it django staff, or course staff?

@pdpinch, done.

0x29a · 2023-04-05T16:51:17Z

Hi @mphilbrick211, neither me nor @farhaanbukhsh have merge rights. We just do our internal review before asking core contributors to make a final review.

farhaanbukhsh · 2023-04-05T22:40:42Z

Hi @mphilbrick211, neither me nor @farhaanbukhsh have merge rights. We just do our internal review before asking core contributors to make a final review.

@0x29a @mphilbrick211 I do have the merge access :)

openedx-webhooks · 2023-04-06T13:04:27Z

@0x29a 🎉 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-04-06T17:11:53Z

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

edx-pipeline-bot · 2023-04-06T17:32:48Z

EdX Release Notice: This PR has been deployed to the production environment.

pdpinch · 2023-04-12T13:53:05Z

After this was merged, we saw some unexpected behavior on our QA deployment with the advanced settings in Studio -- users who used to have access don't anymore.

Can you clarify:

If we don't have a value set for DISABLE_ADVANCED_SETTINGS what is the expected behavior?
What, exactly, do you mean by "non-staff" users? Staff could mean "django admin staff" or "course staff". Will course admins still have access?

0x29a · 2023-04-12T14:38:40Z

@pdpinch, thank you for reporting this. Looking at the code, I can't immediately tell where is the issue. Should we create a revert PR?

If we don't have a value set for DISABLE_ADVANCED_SETTINGS what is the expected behavior?

If it's not set, course admins and course staff should have access.

What, exactly, do you mean by "non-staff" users? Staff could mean "django admin staff" or "course staff". Will course admins still have access?

I updated the PR description. No, course admins won't have access. Only Django admin staff and superusers.

This reverts commit 63d49d3.

0x29a · 2023-04-12T14:55:01Z

@pdpinch, upd: I think I could have overlooked some templates that lack advanced_settings_url context variable. ☹️ If that's the case, I'll prepare a fix. For now, let's revert this: #32062. Sorry for the inconvenience.

farhaanbukhsh · 2023-04-17T12:05:18Z

@0x29a I should have been more careful about testing this PR, also this points that we don't have enough test coverage we should make sure to add more tests in the new PR as well. cc: @pdpinch

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 31, 2023

0x29a force-pushed the 0x29a/bb7220/disable-advanced-settings branch 2 times, most recently from ca7f438 to 3ef9955 Compare April 1, 2023 09:38

farhaanbukhsh approved these changes Apr 3, 2023

View reviewed changes

0x29a force-pushed the 0x29a/bb7220/disable-advanced-settings branch 2 times, most recently from 97c326e to 22974b2 Compare April 3, 2023 11:53

0x29a mentioned this pull request Apr 3, 2023

feat: feature flag to disable Advanced Settings [BACKPORT] open-craft/edx-platform#530

Merged

farhaanbukhsh approved these changes Apr 3, 2023

View reviewed changes

0x29a force-pushed the 0x29a/bb7220/disable-advanced-settings branch from 22974b2 to e207c9d Compare April 3, 2023 20:37

pdpinch reviewed Apr 5, 2023

View reviewed changes

0x29a force-pushed the 0x29a/bb7220/disable-advanced-settings branch from e207c9d to 60a5b5a Compare April 5, 2023 16:49

feat: feature flag to disable Advanced Settings

f45e8d1

0x29a force-pushed the 0x29a/bb7220/disable-advanced-settings branch from 60a5b5a to f45e8d1 Compare April 5, 2023 16:52

farhaanbukhsh merged commit 63d49d3 into openedx:master Apr 6, 2023

farhaanbukhsh deleted the 0x29a/bb7220/disable-advanced-settings branch April 6, 2023 13:04

0x29a added a commit to open-craft/edx-platform that referenced this pull request Apr 12, 2023

Revert "feat: feature flag to disable Advanced Settings (openedx#32015)"

4001729

This reverts commit 63d49d3.

0x29a mentioned this pull request Apr 12, 2023

Revert "feat: feature flag to disable Advanced Settings (#32015)" #32062

Closed

0x29a mentioned this pull request Apr 19, 2023

fix: missing advance_settings_access template variable #32097

Merged

itsjeyd mentioned this pull request May 10, 2023

Improvements to Roles and Permissions openedx/platform-roadmap#246

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: feature flag to disable Advanced Settings #32015

feat: feature flag to disable Advanced Settings #32015

0x29a commented Mar 31, 2023 •

edited

Loading

openedx-webhooks commented Mar 31, 2023

farhaanbukhsh left a comment •

edited

Loading

farhaanbukhsh Apr 3, 2023

0x29a Apr 3, 2023 •

edited

Loading

farhaanbukhsh Apr 3, 2023

farhaanbukhsh Apr 3, 2023

0x29a Apr 3, 2023

farhaanbukhsh left a comment

farhaanbukhsh Apr 3, 2023

0x29a Apr 3, 2023

farhaanbukhsh Apr 3, 2023

mphilbrick211 commented Apr 3, 2023

pdpinch Apr 5, 2023

0x29a Apr 5, 2023

0x29a commented Apr 5, 2023

farhaanbukhsh commented Apr 5, 2023

openedx-webhooks commented Apr 6, 2023

edx-pipeline-bot commented Apr 6, 2023

edx-pipeline-bot commented Apr 6, 2023

pdpinch commented Apr 12, 2023

0x29a commented Apr 12, 2023

0x29a commented Apr 12, 2023

farhaanbukhsh commented Apr 17, 2023 •

edited

Loading

	'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id),
	'advance_settings_url': reverse_course_url('advanced_settings_handler', course_block.id) if has_advanced_settings_access(request.user) else None,

	% if context_course:
	<%
	course_key = context_course.id
	url_encoded_course_key = quote(six.text_type(course_key).encode('utf-8'), safe='')
	index_url = reverse('course_handler', kwargs={'course_key_string': six.text_type(course_key)})
	course_team_url = reverse('course_team_handler', kwargs={'course_key_string': six.text_type(course_key)})
	assets_url = reverse('assets_handler', kwargs={'course_key_string': six.text_type(course_key)})
	textbooks_url = reverse('textbooks_list_handler', kwargs={'course_key_string': six.text_type(course_key)})
	videos_url = reverse('videos_handler', kwargs={'course_key_string': six.text_type(course_key)})
	import_url = reverse('import_handler', kwargs={'course_key_string': six.text_type(course_key)})
	course_info_url = reverse('course_info_handler', kwargs={'course_key_string': six.text_type(course_key)})
	export_url = reverse('export_handler', kwargs={'course_key_string': six.text_type(course_key)})
	settings_url = reverse('settings_handler', kwargs={'course_key_string': six.text_type(course_key)})
	grading_url = reverse('grading_handler', kwargs={'course_key_string': six.text_type(course_key)})
	advanced_settings_url = reverse('advanced_settings_handler', kwargs={'course_key_string': six.text_type(course_key)})
	tabs_url = reverse('tabs_handler', kwargs={'course_key_string': six.text_type(course_key)})
	certificates_url = ''
	if settings.FEATURES.get("CERTIFICATES_HTML_VIEW") and context_course.cert_html_view_enabled:
	certificates_url = reverse('certificates_list_handler', kwargs={'course_key_string': six.text_type(course_key)})
	checklists_url = reverse('checklists_handler', kwargs={'course_key_string': six.text_type(course_key)})
	pages_and_resources_mfe_enabled = ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND.is_enabled(context_course.id)
	%>

feat: feature flag to disable Advanced Settings #32015

feat: feature flag to disable Advanced Settings #32015

Conversation

0x29a commented Mar 31, 2023 • edited Loading

Description

Testing instructions

openedx-webhooks commented Mar 31, 2023

farhaanbukhsh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x29a Apr 3, 2023 • edited Loading

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

farhaanbukhsh 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

mphilbrick211 commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x29a commented Apr 5, 2023

farhaanbukhsh commented Apr 5, 2023

openedx-webhooks commented Apr 6, 2023

edx-pipeline-bot commented Apr 6, 2023

edx-pipeline-bot commented Apr 6, 2023

pdpinch commented Apr 12, 2023

0x29a commented Apr 12, 2023

0x29a commented Apr 12, 2023

farhaanbukhsh commented Apr 17, 2023 • edited Loading

0x29a commented Mar 31, 2023 •

edited

Loading

farhaanbukhsh left a comment •

edited

Loading

0x29a Apr 3, 2023 •

edited

Loading

farhaanbukhsh commented Apr 17, 2023 •

edited

Loading