From 4d801fe7a8d05f1cde01e525aae204c16028e0de Mon Sep 17 00:00:00 2001 From: Matjaz Gregoric Date: Mon, 23 Aug 2021 16:38:01 +0200 Subject: [PATCH 01/19] fix: monkey-patch django db introspection to avoid performance issues (cherry picked from commit d393d6eda9a007c5b1e49270e1e87636620407d4 AND fixed linting issues) (cherry picked from commit 216a206e416f3de238e2d9b9a9e80dc8714f17ba) --- cms/__init__.py | 26 ++++++++++++++++++++++++++ lms/__init__.py | 27 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/cms/__init__.py b/cms/__init__.py index f9ed0bb3cea1..d1bf27534315 100644 --- a/cms/__init__.py +++ b/cms/__init__.py @@ -6,6 +6,12 @@ isort:skip_file """ +# FAL-2248: Monkey patch django's get_storage_engine to work around long migrations times. +# This fixes a performance issue with database migrations in Ocim. We will need to keep +# this patch in our opencraft-release/* branches until edx-platform upgrades to Django 4.* +# which will include this commit: +# https://github.com/django/django/commit/518ce7a51f994fc0585d31c4553e2072bf816f76 +import django.db.backends.mysql.introspection # We monkey patch Kombu's entrypoints listing because scanning through this # accounts for the majority of LMS/Studio startup time for tests, and we don't @@ -22,3 +28,23 @@ # that shared_task will use this app, and also ensures that the celery # singleton is always configured for the CMS. from .celery import APP as CELERY_APP # lint-amnesty, pylint: disable=wrong-import-position + + +def get_storage_engine(self, cursor, table_name): + """ + This is a patched version of `get_storage_engine` that fixes a + performance issue with migrations. For more info see FAL-2248 and + https://github.com/django/django/pull/14766 + """ + cursor.execute(""" + SELECT engine + FROM information_schema.tables + WHERE table_name = %s + AND table_schema = DATABASE()""", [table_name]) + result = cursor.fetchone() + if not result: + return self.connection.features._mysql_storage_engine # pylint: disable=protected-access + return result[0] + + +django.db.backends.mysql.introspection.DatabaseIntrospection.get_storage_engine = get_storage_engine diff --git a/lms/__init__.py b/lms/__init__.py index 008640ac7147..05a30f4ffad4 100644 --- a/lms/__init__.py +++ b/lms/__init__.py @@ -18,3 +18,30 @@ # that shared_task will use this app, and also ensures that the celery # singleton is always configured for the LMS. from .celery import APP as CELERY_APP # lint-amnesty, pylint: disable=wrong-import-position + +# FAL-2248: Monkey patch django's get_storage_engine to work around long migrations times. +# This fixes a performance issue with database migrations in Ocim. We will need to keep +# this patch in our opencraft-release/* branches until edx-platform upgrades to Django 4.* +# which will include this commit: +# https://github.com/django/django/commit/518ce7a51f994fc0585d31c4553e2072bf816f76 +import django.db.backends.mysql.introspection + + +def get_storage_engine(self, cursor, table_name): + """ + This is a patched version of `get_storage_engine` that fixes a + performance issue with migrations. For more info see FAL-2248 and + https://github.com/django/django/pull/14766 + """ + cursor.execute(""" + SELECT engine + FROM information_schema.tables + WHERE table_name = %s + AND table_schema = DATABASE()""", [table_name]) + result = cursor.fetchone() + if not result: + return self.connection.features._mysql_storage_engine # pylint: disable=protected-access + return result[0] + + +django.db.backends.mysql.introspection.DatabaseIntrospection.get_storage_engine = get_storage_engine From 43c635b8466830c09f8f414e03fda2b9c9987221 Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee Date: Thu, 23 Dec 2021 07:18:58 +0530 Subject: [PATCH 02/19] feat: change studio schedule datetime inputs to user timezone (cherry picked from commit ed4c7cc6f3771488830c1b52f83b64d037fc44e0) (cherry picked from commit 1c228db635b44293d3696901d9fe5ea08eb323ad) --- cms/djangoapps/contentstore/views/course.py | 7 +++ cms/static/cms/js/spec/main.js | 1 + cms/static/js/utils/date_utils.js | 64 ++++++++++++++++++--- 3 files changed, 63 insertions(+), 9 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 023cb07199fe..b196225d1273 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -74,6 +74,7 @@ from openedx.core.djangoapps.credit.tasks import update_credit_course_requirements from openedx.core.djangoapps.models.course_details import CourseDetails from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from openedx.core.djangoapps.user_api.models import UserPreference from openedx.core.djangolib.js_utils import dump_js_escaped_json from openedx.core.lib.course_tabs import CourseTabPluginManager from openedx.core.lib.courses import course_image_url @@ -1249,6 +1250,12 @@ def settings_handler(request, course_key_string): # lint-amnesty, pylint: disab elif 'application/json' in request.META.get('HTTP_ACCEPT', ''): # pylint: disable=too-many-nested-blocks if request.method == 'GET': course_details = CourseDetails.fetch(course_key) + + # Fetch the prefered timezone setup by the user + # and pass it as part of Json response + user_timezone = UserPreference.get_value(request.user, 'time_zone') + course_details.user_timezone = user_timezone + return JsonResponse( course_details, # encoder serializes dates, old locations, and instances diff --git a/cms/static/cms/js/spec/main.js b/cms/static/cms/js/spec/main.js index f5aa089b0438..384eb7b83350 100644 --- a/cms/static/cms/js/spec/main.js +++ b/cms/static/cms/js/spec/main.js @@ -47,6 +47,7 @@ 'jquery.simulate': 'xmodule_js/common_static/js/vendor/jquery.simulate', 'datepair': 'xmodule_js/common_static/js/vendor/timepicker/datepair', 'date': 'xmodule_js/common_static/js/vendor/date', + 'moment-timezone': 'common/js/vendor/moment-timezone-with-data', moment: 'common/js/vendor/moment-with-locales', 'text': 'xmodule_js/common_static/js/vendor/requirejs/text', 'underscore': 'common/js/vendor/underscore', diff --git a/cms/static/js/utils/date_utils.js b/cms/static/js/utils/date_utils.js index a335adb01b62..a55ac575bc91 100644 --- a/cms/static/js/utils/date_utils.js +++ b/cms/static/js/utils/date_utils.js @@ -1,5 +1,5 @@ -define(['jquery', 'date', 'js/utils/change_on_enter', 'jquery.ui', 'jquery.timepicker'], - function($, date, TriggerChangeEventOnEnter) { +define(['jquery', 'date', 'js/utils/change_on_enter', 'moment-timezone', 'jquery.ui', 'jquery.timepicker'], + function($, date, TriggerChangeEventOnEnter, moment) { 'use strict'; function getDate(datepickerInput, timepickerInput) { @@ -67,15 +67,51 @@ define(['jquery', 'date', 'js/utils/change_on_enter', 'jquery.ui', 'jquery.timep return obj; } - function setupDatePicker(fieldName, view, index) { + /** + * Calculates the utc offset in miliseconds for given + * timezone and subtracts it from given localized time + * to get time in UTC + * + * @param {Date} localTime JS Date object in Local Time + * @param {string} timezone IANA timezone name ex. "Australia/Brisbane" + * @returns JS Date object in UTC + */ + function convertLocalizedDateToUTC(localTime, timezone) { + const localTimeMS = localTime.getTime(); + const utcOffset = moment.tz(localTime, timezone)._offset; + return new Date(localTimeMS - (utcOffset * 60 *1000)); + } + + /** + * Returns the timezone abbreviation for given + * timezone name + * + * @param {string} timezone IANA timezone name ex. "Australia/Brisbane" + * @returns Timezone abbreviation ex. "AEST" + */ + function getTZAbbreviation(timezone) { + return moment(new Date()).tz(timezone).format('z'); + } + + /** + * Converts the given datetime string from UTC to localized time + * + * @param {string} utcDateTime JS Date object with UTC datetime + * @param {string} timezone IANA timezone name ex. "Australia/Brisbane" + * @returns Formatted datetime string with localized timezone + */ + function getLocalizedCurrentDate(utcDateTime, timezone) { + const localDateTime = moment(utcDateTime).tz(timezone); + return localDateTime.format('YYYY-MM-DDTHH[:]mm[:]ss'); + }function setupDatePicker(fieldName, view, index) { var cacheModel; var div; var datefield; - var timefield; + var timefield;var tzfield; var cacheview; var setfield; var currentDate; - if (typeof index !== 'undefined' && view.hasOwnProperty('collection')) { + var timezone;if (typeof index !== 'undefined' && view.hasOwnProperty('collection')) { cacheModel = view.collection.models[index]; div = view.$el.find('#' + view.collectionSelector(cacheModel.cid)); } else { @@ -84,11 +120,16 @@ define(['jquery', 'date', 'js/utils/change_on_enter', 'jquery.ui', 'jquery.timep } datefield = $(div).find('input.date'); timefield = $(div).find('input.time'); - cacheview = view; + tzfield = $(div).find('span.timezone'); + cacheview = view; + + timezone = cacheModel.get('user_timezone'); setfield = function(event) { var newVal = getDate(datefield, timefield); - // Setting to null clears the time as well, as date and time are linked. + if (timezone) { + newVal = convertLocalizedDateToUTC(newVal, timezone); + }// Setting to null clears the time as well, as date and time are linked. // Note also that the validation logic prevents us from clearing the start date // (start date is required by the back end). cacheview.clearValidationErrors(); @@ -109,8 +150,13 @@ define(['jquery', 'date', 'js/utils/change_on_enter', 'jquery.ui', 'jquery.timep if (cacheModel) { currentDate = cacheModel.get(fieldName); } - // timepicker doesn't let us set null, so check that we have a time - if (currentDate) { + if (timezone) { + const tz = getTZAbbreviation(timezone); + $(tzfield).text("("+tz+")"); + } // timepicker doesn't let us set null, so check that we have a time + if (currentDate) {if (timezone) { + currentDate = getLocalizedCurrentDate(currentDate, timezone); + } setDate(datefield, timefield, currentDate); } else { // but reset fields either way From 653d353fcd28f98f184b14f6993506dbdfb56b95 Mon Sep 17 00:00:00 2001 From: Sandeep Kumar Choudhary Date: Mon, 7 Jun 2021 05:52:39 +0000 Subject: [PATCH 03/19] feat: Allow delete course content in Studio only for admin users (cherry picked from commit c812a6c1d5c0961900507a6e7abe3d0f3b8a7570) (cherry picked from commit 004f2fe3a4560428e00e97a86bb0a62b79f18e22) --- cms/djangoapps/contentstore/config/waffle.py | 10 ++ cms/djangoapps/contentstore/permissions.py | 10 ++ cms/djangoapps/contentstore/views/block.py | 9 +- .../contentstore/views/tests/test_block.py | 147 ++++++++++++++++++ .../spec/views/pages/course_outline_spec.js | 22 ++- cms/static/js/views/xblock_outline.js | 4 +- cms/templates/js/course-outline.underscore | 2 +- 7 files changed, 195 insertions(+), 9 deletions(-) create mode 100644 cms/djangoapps/contentstore/permissions.py diff --git a/cms/djangoapps/contentstore/config/waffle.py b/cms/djangoapps/contentstore/config/waffle.py index 8f431fa49327..16719e61bf29 100644 --- a/cms/djangoapps/contentstore/config/waffle.py +++ b/cms/djangoapps/contentstore/config/waffle.py @@ -53,3 +53,13 @@ # .. toggle_warning: Flag course_experience.relative_dates should also be active for relative dates functionalities to work. # .. toggle_tickets: https://openedx.atlassian.net/browse/AA-844 CUSTOM_RELATIVE_DATES = CourseWaffleFlag(f'{WAFFLE_NAMESPACE}.custom_relative_dates', __name__) + +# .. toggle_name: studio.prevent_staff_structure_deletion +# .. toggle_implementation: WaffleFlag +# .. toggle_default: False +# .. toggle_description: Prevents staff from deleting course structures +# .. toggle_use_cases: opt_in +# .. toggle_creation_date: 2021-06-25 +PREVENT_STAFF_STRUCTURE_DELETION = WaffleFlag( + f'{WAFFLE_NAMESPACE}.prevent_staff_structure_deletion', __name__, LOG_PREFIX +) diff --git a/cms/djangoapps/contentstore/permissions.py b/cms/djangoapps/contentstore/permissions.py new file mode 100644 index 000000000000..14fe40c09ca7 --- /dev/null +++ b/cms/djangoapps/contentstore/permissions.py @@ -0,0 +1,10 @@ +""" +Permission definitions for the contentstore djangoapp +""" + +from bridgekeeper import perms + +from lms.djangoapps.courseware.rules import HasRolesRule + +DELETE_COURSE_CONTENT = 'contentstore.delete_course_content' +perms[DELETE_COURSE_CONTENT] = HasRolesRule('instructor') diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index c65aaace82d8..65052ae30941 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -32,7 +32,8 @@ from xblock.core import XBlock from xblock.fields import Scope -from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG +from cms.djangoapps.contentstore.config.waffle import PREVENT_STAFF_STRUCTURE_DELETION, SHOW_REVIEW_RULES_FLAG +from cms.djangoapps.contentstore.permissions import DELETE_COURSE_CONTENT from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW from common.djangoapps.edxmako.services import MakoService @@ -1372,6 +1373,12 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F else: xblock_info['staff_only_message'] = False + xblock_info['show_delete_button'] = True + if PREVENT_STAFF_STRUCTURE_DELETION.is_enabled(): + xblock_info['show_delete_button'] = ( + user.has_perm(DELETE_COURSE_CONTENT, xblock) if user is not None else False + ) + xblock_info['has_partition_group_components'] = has_children_visible_to_specific_partition_groups( xblock ) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 7f5cbfe2be85..510c30d858ee 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -16,6 +16,7 @@ from openedx_events.content_authoring.signals import XBLOCK_DUPLICATED from openedx_events.tests.utils import OpenEdxEventsTestMixin from edx_proctoring.exceptions import ProctoredExamNotFoundException +from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys import InvalidKeyError from opaque_keys.edx.asides import AsideUsageKeyV2 from opaque_keys.edx.keys import CourseKey, UsageKey @@ -49,6 +50,8 @@ from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.utils import reverse_course_url, reverse_usage_url from cms.djangoapps.contentstore.views import block as item_module +from cms.djangoapps.contentstore.config.waffle import PREVENT_STAFF_STRUCTURE_DELETION +from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, CourseCreatorRole from common.djangoapps.student.tests.factories import StaffFactory, UserFactory from common.djangoapps.xblock_django.models import ( XBlockConfiguration, @@ -3473,3 +3476,147 @@ def test_self_paced_item_visibility_state(self, store_type): # Check that in self paced course content has live state now xblock_info = self._get_xblock_info(chapter.location) self._verify_visibility_state(xblock_info, VisibilityState.live) + + def test_staff_show_delete_button(self): + """ + Test delete button is *not visible* to user with CourseStaffRole + """ + # Add user as course staff + CourseStaffRole(self.course_key).add_users(self.user) + + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=self.user + ) + self.assertTrue(xblock_info['show_delete_button']) + + def test_staff_show_delete_button_with_waffle(self): + """ + Test delete button is *not visible* to user with CourseStaffRole and + PREVENT_STAFF_STRUCTURE_DELETION waffle set + """ + # Add user as course staff + CourseStaffRole(self.course_key).add_users(self.user) + + with override_waffle_flag(PREVENT_STAFF_STRUCTURE_DELETION, active=True): + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=self.user + ) + + self.assertFalse(xblock_info['show_delete_button']) + + def test_no_user_show_delete_button(self): + """ + Test delete button is *visible* when user attribute is not set on + xblock. This happens with ajax requests. + """ + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=None + ) + self.assertTrue(xblock_info['show_delete_button']) + + def test_no_user_show_delete_button_with_waffle(self): + """ + Test delete button is *visible* when user attribute is not set on + xblock (this happens with ajax requests) and PREVENT_STAFF_STRUCTURE_DELETION waffle set. + """ + + with override_waffle_flag(PREVENT_STAFF_STRUCTURE_DELETION, active=True): + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=None + ) + + self.assertFalse(xblock_info['show_delete_button']) + + def test_instructor_show_delete_button(self): + """ + Test delete button is *visible* to user with CourseInstructorRole only + """ + # Add user as course instructor + CourseInstructorRole(self.course_key).add_users(self.user) + + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=self.user + ) + self.assertTrue(xblock_info['show_delete_button']) + + def test_instructor_show_delete_button_with_waffle(self): + """ + Test delete button is *visible* to user with CourseInstructorRole only + and PREVENT_STAFF_STRUCTURE_DELETION waffle set + """ + # Add user as course instructor + CourseInstructorRole(self.course_key).add_users(self.user) + + with override_waffle_flag(PREVENT_STAFF_STRUCTURE_DELETION, active=True): + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=self.user + ) + + self.assertTrue(xblock_info['show_delete_button']) + + def test_creator_show_delete_button(self): + """ + Test delete button is *visible* to user with CourseInstructorRole only + """ + # Add user as course creator + CourseCreatorRole(self.course_key).add_users(self.user) + + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=self.user + ) + self.assertTrue(xblock_info['show_delete_button']) + + def test_creator_show_delete_button_with_waffle(self): + """ + Test delete button is *visible* to user with CourseInstructorRole only + and PREVENT_STAFF_STRUCTURE_DELETION waffle set + """ + # Add user as course creator + CourseCreatorRole(self.course_key).add_users(self.user) + + with override_waffle_flag(PREVENT_STAFF_STRUCTURE_DELETION, active=True): + # Get xblock outline + xblock_info = create_xblock_info( + self.course, + include_child_info=True, + course_outline=True, + include_children_predicate=lambda xblock: not xblock.category == 'vertical', + user=self.user + ) + + self.assertFalse(xblock_info['show_delete_button']) diff --git a/cms/static/js/spec/views/pages/course_outline_spec.js b/cms/static/js/spec/views/pages/course_outline_spec.js index 7b0abb53ee25..efa20d850e6a 100644 --- a/cms/static/js/spec/views/pages/course_outline_spec.js +++ b/cms/static/js/spec/views/pages/course_outline_spec.js @@ -41,7 +41,8 @@ describe('CourseOutlinePage', function() { user_partitions: [], user_partition_info: {}, highlights_enabled: true, - highlights_enabled_for_messaging: false + highlights_enabled_for_messaging: false, + show_delete_button: true }, options, {child_info: {children: children}}); }; @@ -68,7 +69,8 @@ describe('CourseOutlinePage', function() { show_review_rules: true, user_partition_info: {}, highlights_enabled: true, - highlights_enabled_for_messaging: false + highlights_enabled_for_messaging: false, + show_delete_button: true }, options, {child_info: {children: children}}); }; @@ -93,7 +95,8 @@ describe('CourseOutlinePage', function() { group_access: {}, user_partition_info: {}, highlights: [], - highlights_enabled: true + highlights_enabled: true, + show_delete_button: true }, options, {child_info: {children: children}}); }; @@ -123,7 +126,8 @@ describe('CourseOutlinePage', function() { }, user_partitions: [], group_access: {}, - user_partition_info: {} + user_partition_info: {}, + show_delete_button: true }, options, {child_info: {children: children}}); }; @@ -141,7 +145,8 @@ describe('CourseOutlinePage', function() { edited_by: 'MockUser', user_partitions: [], group_access: {}, - user_partition_info: {} + user_partition_info: {}, + show_delete_button: true }, options); }; @@ -862,6 +867,13 @@ describe('CourseOutlinePage', function() { expect(outlinePage.$('[data-locator="mock-section-2"]')).toExist(); }); + it('remains un-visible if show_delete_button is false ', function() { + createCourseOutlinePage(this, createMockCourseJSON({show_delete_button: false}, [ + createMockSectionJSON({show_delete_button: false}) + ])); + expect(getItemHeaders('section').find('.delete-button').first()).not.toExist(); + }); + it('can be deleted if it is the only section', function() { var promptSpy = EditHelpers.createPromptSpy(); createCourseOutlinePage(this, mockSingleSectionCourseJSON); diff --git a/cms/static/js/views/xblock_outline.js b/cms/static/js/views/xblock_outline.js index 1f1538e502fe..37d7881045f1 100644 --- a/cms/static/js/views/xblock_outline.js +++ b/cms/static/js/views/xblock_outline.js @@ -109,8 +109,8 @@ function($, _, gettext, BaseView, ViewUtils, XBlockViewUtils, XBlockStringFieldE includesChildren: this.shouldRenderChildren(), hasExplicitStaffLock: this.model.get('has_explicit_staff_lock'), staffOnlyMessage: this.model.get('staff_only_message'), - course: course - }; + course: course, + showDeleteButton: this.model.get('show_delete_button')}; }, renderChildren: function() { diff --git a/cms/templates/js/course-outline.underscore b/cms/templates/js/course-outline.underscore index 4550d102a6ad..7f82410196bb 100644 --- a/cms/templates/js/course-outline.underscore +++ b/cms/templates/js/course-outline.underscore @@ -179,7 +179,7 @@ if (is_proctored_exam) { <% } %> - <% if (xblockInfo.isDeletable()) { %> + <% if (xblockInfo.isDeletable() && showDeleteButton) { %>
  • From 58ecc18547276250037b764389dae4430ec822e1 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 23 Jun 2023 11:19:37 +0530 Subject: [PATCH 04/19] temp: Ignore incorrect import for now The commit for "feat: Allow delete course content in Studio only for admin users" introduces an incorrect import of an lms package from cms. Fixing this requires a larger refactoring that doesn't make sense as part of this rebase. The upsteam version also has this issue at this time: https://github.com/openedx/edx-platform/pull/27857 This can be dropped in Quince if the above is merged, or updated if the above no longer has the issue. --- setup.cfg | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.cfg b/setup.cfg index 7b1be0be92e8..fc5a2dee3e83 100644 --- a/setup.cfg +++ b/setup.cfg @@ -73,3 +73,4 @@ ignore_imports = cms.djangoapps.contentstore.views.preview -> lms.djangoapps.lms_xblock.field_data cms.envs.common -> lms.djangoapps.lms_xblock.mixin cms.envs.test -> lms.envs.test + cms.djangoapps.contentstore.permissions -> lms.djangoapps.courseware.rules From 613d84550213faf316ba67f56aef07e17d2d43e3 Mon Sep 17 00:00:00 2001 From: pkulkark Date: Tue, 1 Feb 2022 12:12:56 +0530 Subject: [PATCH 05/19] feat: Make course description editable in certs Adds the ability to edit the default course description shown in certificates. (cherry picked from commit a89baafe0575c94fac0cb4ce9db1d38ce0b71bc6) (cherry picked from commit b16c266b79dafc2468ea4462fc5276dcd71e9c49) --- .../contentstore/views/certificates.py | 2 ++ .../js/certificates/models/certificate.js | 1 + .../certificates/views/certificate_editor.js | 13 +++++++++++++ .../js/certificate-details.underscore | 6 ++++++ cms/templates/js/certificate-editor.underscore | 5 +++++ .../certificates/tests/test_webview_views.py | 18 +++--------------- lms/djangoapps/certificates/views/webview.py | 5 ++++- 7 files changed, 34 insertions(+), 16 deletions(-) diff --git a/cms/djangoapps/contentstore/views/certificates.py b/cms/djangoapps/contentstore/views/certificates.py index 6535e0a85718..d27a648c91e9 100644 --- a/cms/djangoapps/contentstore/views/certificates.py +++ b/cms/djangoapps/contentstore/views/certificates.py @@ -231,6 +231,8 @@ def serialize_certificate(certificate): # Some keys are not required, such as the title override... if certificate_data.get('course_title'): certificate_response["course_title"] = certificate_data['course_title'] + if certificate_data.get('course_description'): + certificate_response['course_description'] = certificate_data['course_description'] return certificate_response diff --git a/cms/static/js/certificates/models/certificate.js b/cms/static/js/certificates/models/certificate.js index 97a2735d7159..2b2fbf6eaec0 100644 --- a/cms/static/js/certificates/models/certificate.js +++ b/cms/static/js/certificates/models/certificate.js @@ -18,6 +18,7 @@ function(_, Backbone, BackboneRelational, BackboneAssociations, gettext, CoffeeS defaults: { // Metadata fields currently displayed in web forms course_title: '', + course_description: '', // Metadata fields not currently displayed in web forms name: 'Name of the certificate', diff --git a/cms/static/js/certificates/views/certificate_editor.js b/cms/static/js/certificates/views/certificate_editor.js index 6b0365ffbf65..afe0e0c473d0 100644 --- a/cms/static/js/certificates/views/certificate_editor.js +++ b/cms/static/js/certificates/views/certificate_editor.js @@ -24,6 +24,7 @@ function($, _, Backbone, gettext, 'change .collection-name-input': 'setName', 'change .certificate-description-input': 'setDescription', 'change .certificate-course-title-input': 'setCourseTitle', + 'change .certificate-course-description-input': 'setCourseDescription', 'focus .input-text': 'onFocus', 'blur .input-text': 'onBlur', submit: 'setAndClose', @@ -103,6 +104,7 @@ function($, _, Backbone, gettext, name: this.model.get('name'), description: this.model.get('description'), course_title: this.model.get('course_title'), + course_description: this.model.get('course_description'), org_logo_path: this.model.get('org_logo_path'), is_active: this.model.get('is_active'), isNew: this.model.isNew() @@ -143,11 +145,22 @@ function($, _, Backbone, gettext, ); }, + setCourseDescription: function(event) { + // Updates the indicated model field (still requires persistence on server) + if (event && event.preventDefault) { event.preventDefault(); } + this.model.set( + 'course_description', + this.$('.certificate-course-description-input').val(), + {silent: true} + ); + }, + setValues: function() { // Update the specified values in the local model instance this.setName(); this.setDescription(); this.setCourseTitle(); + this.setCourseDescription(); return this; } }); diff --git a/cms/templates/js/certificate-details.underscore b/cms/templates/js/certificate-details.underscore index a09a3baf897c..3401fd175a39 100644 --- a/cms/templates/js/certificate-details.underscore +++ b/cms/templates/js/certificate-details.underscore @@ -29,6 +29,12 @@ <%- course_title %>

    <% } %> + <% if (course_description) { %> +

    + <%- gettext('Course Description') %>: + <%- course_description %> +

    + <% } %>
    diff --git a/cms/templates/js/certificate-editor.underscore b/cms/templates/js/certificate-editor.underscore index 513113b80500..3b1d90969b5a 100644 --- a/cms/templates/js/certificate-editor.underscore +++ b/cms/templates/js/certificate-editor.underscore @@ -31,6 +31,11 @@ " value="<%- course_title %>" aria-describedby="certificate-course-title-<%-uniqueId %>-tip" /> <%- gettext("Specify an alternative to the official course title to display on certificates. Leave blank to use the official course title.") %>
    +
    + + " value="<%- course_description %>" aria-describedby="certificate-course-description-<%-uniqueId %>-tip" /> + <%- gettext("Specify an alternative to the official course description to display on certificates. Leave blank to use default text.") %> +

    <%- gettext("Certificate Signatories") %>

    diff --git a/lms/djangoapps/certificates/tests/test_webview_views.py b/lms/djangoapps/certificates/tests/test_webview_views.py index 96832e3cb694..a61fcd6dd7c4 100644 --- a/lms/djangoapps/certificates/tests/test_webview_views.py +++ b/lms/djangoapps/certificates/tests/test_webview_views.py @@ -140,6 +140,7 @@ def _add_course_certificates(self, count=1, signatory_count=0, is_active=True): 'name': 'Name ' + str(i), 'description': 'Description ' + str(i), 'course_title': 'course_title_' + str(i), + 'course_description': 'course_description_' + str(i), 'org_logo_path': f'/t4x/orgX/testX/asset/org-logo-{i}.png', 'signatories': signatories, 'version': 1, @@ -460,11 +461,6 @@ def test_rendering_course_organization_data(self): uuid=self.cert.verify_uuid ) response = self.client.get(test_url) - self.assertContains( - response, - 'a course of study offered by test_organization, an online learning initiative of test organization', - ) - self.assertNotContains(response, 'a course of study offered by testorg') self.assertContains(response, f'test_organization {self.course.number} Certificate |') self.assertContains(response, 'logo_test1.png') @@ -549,21 +545,13 @@ def test_rendering_maximum_data(self): self.assertContains(response, '<a class="logo" href="http://test_site.localhost">') # Test an item from course info self.assertContains(response, 'course_title_0') + # Test an item from course description + self.assertContains(response, 'course_description_0') # Test an item from user info self.assertContains(response, f"{self.user.profile.name}, you earned a certificate!") # Test an item from social info self.assertContains(response, "Post on Facebook") self.assertContains(response, "Share on Twitter") - # Test an item from certificate/org info - self.assertContains( - response, - "a course of study offered by {partner_short_name}, " - "an online learning initiative of " - "{partner_long_name}.".format( - partner_short_name=short_org_name, - partner_long_name=long_org_name, - ), - ) # Test item from badge info self.assertContains(response, "Add to Mozilla Backpack") # Test item from site configuration diff --git a/lms/djangoapps/certificates/views/webview.py b/lms/djangoapps/certificates/views/webview.py index b859cfe92ca2..e9f9afd34470 100644 --- a/lms/djangoapps/certificates/views/webview.py +++ b/lms/djangoapps/certificates/views/webview.py @@ -254,7 +254,10 @@ def _update_course_context(request, context, course, platform_name): course_number = course.display_coursenumber if course.display_coursenumber else course.number context['course_number'] = course_number context['idv_enabled_for_certificates'] = settings.FEATURES.get('ENABLE_CERTIFICATES_IDV_REQUIREMENT') - if context['organization_long_name']: + course_description_override = context['certificate_data'].get('course_description', '') + if course_description_override: + context['accomplishment_copy_course_description'] = course_description_override + elif context['organization_long_name']: # Translators: This text represents the description of course context['accomplishment_copy_course_description'] = _('a course of study offered by {partner_short_name}, ' 'an online learning initiative of ' From dbe7b04c5729a751236984f9e593393660bccca7 Mon Sep 17 00:00:00 2001 From: Farhaan Bukhsh <farhaan@opencraft.com> Date: Tue, 26 Apr 2022 02:36:41 +0530 Subject: [PATCH 06/19] feat: Added date configuration to Schedule & Details settings page This feature help to configure the date formatt in Schedule and Details settings page. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com> Co-authored-by: Joseph Curtin <jbcurtin@opencraft.com> (cherry picked from commit 64d44a86d7b3d6951159e576ec1537b684496d99) (cherry picked from commit f2cbb133709f55f1cf5581aab7bcd09b6ef0a7b1) --- cms/djangoapps/contentstore/views/course.py | 8 ++++++++ cms/envs/common.py | 5 +++++ cms/static/js/utils/date_utils.js | 7 ++++++- cms/templates/settings.html | 13 +++++++------ 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b196225d1273..a755d930bfad 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1192,6 +1192,13 @@ def settings_handler(request, course_key_string): # lint-amnesty, pylint: disab verified_mode = CourseMode.verified_mode_for_course(course_key, include_expired=True) upgrade_deadline = (verified_mode and verified_mode.expiration_datetime and verified_mode.expiration_datetime.isoformat()) + + date_placeholder_format = configuration_helpers.get_value_for_org( + course_block.location.org, + 'SCHEDULE_DETAIL_FORMAT', + settings.SCHEDULE_DETAIL_FORMAT + ).upper() + settings_context = { 'context_course': course_block, 'course_locator': course_key, @@ -1216,6 +1223,7 @@ def settings_handler(request, course_key_string): # lint-amnesty, pylint: disab 'enable_extended_course_details': enable_extended_course_details, 'upgrade_deadline': upgrade_deadline, 'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_block.id), + 'date_placeholder_format': date_placeholder_format, } if is_prerequisite_courses_enabled(): courses, in_process_course_actions = get_courses_accessible_to_user(request) diff --git a/cms/envs/common.py b/cms/envs/common.py index 70b4f0781a0a..316434d77c5d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -182,6 +182,11 @@ # templates. STUDIO_NAME = _("Your Platform Studio") STUDIO_SHORT_NAME = _("Studio") + +# .. setting_name: SCHEDULE_DETAIL_FORMAT +# .. setting_default: 'MM/DD/YYYY' +# .. setting_description: Settings to configure the date format in Schedule & Details page +SCHEDULE_DETAIL_FORMAT = 'MM/DD/YYYY' FEATURES = { 'GITHUB_PUSH': False, diff --git a/cms/static/js/utils/date_utils.js b/cms/static/js/utils/date_utils.js index a55ac575bc91..f8e71dff018e 100644 --- a/cms/static/js/utils/date_utils.js +++ b/cms/static/js/utils/date_utils.js @@ -138,7 +138,12 @@ define(['jquery', 'date', 'js/utils/change_on_enter', 'moment-timezone', 'jquery // instrument as date and time pickers timefield.timepicker({timeFormat: 'H:i'}); - datefield.datepicker(); + var placeholder = datefield.attr('placeholder'); + if (placeholder == 'DD/MM/YYYY') { + datefield.datepicker({dateFormat: 'dd/mm/yy'}); + } else { + datefield.datepicker(); + } // Using the change event causes setfield to be triggered twice, but it is necessary // to pick up when the date is typed directly in the field. diff --git a/cms/templates/settings.html b/cms/templates/settings.html index b7ef368a0911..2f99cbfa0c2b 100644 --- a/cms/templates/settings.html +++ b/cms/templates/settings.html @@ -224,7 +224,7 @@ <h2 class="title-2">${_('Course Schedule')}</h2> <li class="field-group field-group-course-start" id="course-start"> <div class="field date" id="field-course-start-date"> <label for="course-start-date">${_("Course Start Date")}</label> - <input type="text" class="start-date date start datepicker" id="course-start-date" placeholder="MM/DD/YYYY" autocomplete="off" /> + <input type="text" class="start-date date start datepicker" id="course-start-date" placeholder="${date_placeholder_format}" autocomplete="off" /> <span class="icon icon-inline fa fa-calendar-check-o datepicker-icon" aria-hidden="true"></span> <span class="tip tip-stacked">${_("First day the course begins")}</span> </div> @@ -239,7 +239,7 @@ <h2 class="title-2">${_('Course Schedule')}</h2> <li class="field-group field-group-course-end" id="course-end"> <div class="field date" id="field-course-end-date"> <label for="course-end-date">${_("Course End Date")}</label> - <input type="text" class="end-date date end" id="course-end-date" placeholder="MM/DD/YYYY" autocomplete="off" /> + <input type="text" class="end-date date end" id="course-end-date" placeholder="${date_placeholder_format}" autocomplete="off" /> <span class="icon icon-inline fa fa-calendar-check-o datepicker-icon" aria-hidden="true"></span> <span class="tip tip-stacked">${_("Last day your course is active")}</span> </div> @@ -305,8 +305,9 @@ <h2 class="title-2">${_('Course Schedule')}</h2> <div class="field date" id="field-certificate-available-date" > % endif <label for="certificate-available-date">${_("Certificates Available Date")}</label> - <input type="text" class="certificate-available-date date start datepicker" id="certificate-available-date" placeholder="MM/DD/YYYY" autocomplete="off" /> + <input type="text" class="certificate-available-date date start datepicker" id="certificate-available-date" placeholder="${date_placeholder_format}" autocomplete="off" /> <span class="icon icon-inline fa fa-calendar-check-o datepicker-icon" aria-hidden="true"></span> + <span class="tip tip-stacked">${_("By default, 48 hours after course end date")}</span> </div> </li> </ol> @@ -316,7 +317,7 @@ <h2 class="title-2">${_('Course Schedule')}</h2> <li class="field-group field-group-enrollment-start" id="enrollment-start"> <div class="field date" id="field-enrollment-start-date"> <label for="course-enrollment-start-date">${_("Enrollment Start Date")}</label> - <input type="text" class="start-date date start" id="course-enrollment-start-date" placeholder="MM/DD/YYYY" autocomplete="off" /> + <input type="text" class="start-date date start" id="course-enrollment-start-date" placeholder="${date_placeholder_format}" autocomplete="off" /> <span class="icon icon-inline fa fa-calendar-check-o datepicker-icon" aria-hidden="true"></span> <span class="tip tip-stacked">${_("First day students can enroll")}</span> </div> @@ -334,7 +335,7 @@ <h2 class="title-2">${_('Course Schedule')}</h2> <li class="field-group field-group-enrollment-end" id="enrollment-end"> <div class="field date ${enrollment_end_editable_class}" id="field-enrollment-end-date"> <label for="course-enrollment-end-date">${_("Enrollment End Date")}</label> - <input type="text" class="end-date date end" id="course-enrollment-end-date" placeholder="MM/DD/YYYY" autocomplete="off" ${enrollment_end_readonly} /> + <input type="text" class="end-date date end" id="course-enrollment-end-date" placeholder="${date_placeholder_format}" autocomplete="off" ${enrollment_end_readonly} /> <span class="icon icon-inline fa fa-calendar-check-o datepicker-icon" aria-hidden="true"></span> <span class="tip tip-stacked"> ${_("Last day students can enroll.")} @@ -357,7 +358,7 @@ <h2 class="title-2">${_('Course Schedule')}</h2> <li class="field-group field-group-upgrade-deadline" id="upgrade-deadline"> <div class="field date is-not-editable" id="field-upgrade-deadline-date"> <label for="course-upgrade-deadline-date">${_("Upgrade Deadline Date")}</label> - <input type="text" class="date upgrade-deadline" id="course-upgrade-deadline-date" placeholder="MM/DD/YYYY" autocomplete="off" readonly aria-readonly="true" /> + <input type="text" class="date upgrade-deadline" id="course-upgrade-deadline-date" placeholder="${date_placeholder_format}" autocomplete="off" readonly aria-readonly="true" /> <span class="tip tip-stacked"> ${_("Last day students can upgrade to a verified enrollment.")} ${_("Contact your {platform_name} partner manager to update these settings.").format(platform_name=settings.PLATFORM_NAME)} From 7245bdc7f73d9693f15acc885bee4e0d00be8245 Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee <kaustav@opencraft.com> Date: Mon, 8 Aug 2022 21:08:36 +0530 Subject: [PATCH 07/19] feat: allow switching anonymous user ID hashing algorithm from shake to md5 The hashing algorithm has been changed in cd60646. However, there are Open edX operators who maintain backward compatibility of anonymous user IDs after past rotations of their Django secret key. For them, altering the hashing algorithm was a breaking change that made their analytics inconsistent. (cherry picked from commit 746e4fed1bfeb731b2f6288fe3ba4ea056397fdb) (cherry picked from commit ff6d92fd892dff5e66bbf77eadf9b91ed2189a24) --- cms/envs/common.py | 11 +++++++++++ common/djangoapps/student/models/user.py | 13 +++++++++++-- common/djangoapps/student/tests/tests.py | 11 +++++++++++ lms/envs/common.py | 11 +++++++++++ 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 316434d77c5d..c0104badced7 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -531,6 +531,17 @@ # .. toggle_creation_date: 2023-03-31 # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32015 'DISABLE_ADVANCED_SETTINGS': False, + + # .. toggle_name: FEATURES['ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the legacy MD5 hashing algorithm to generate anonymous user id + # instead of the newer SHAKE128 hashing algorithm + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2022-08-08 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' + 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, } # .. toggle_name: ENABLE_COPPA_COMPLIANCE diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index f29480e0ce02..f9f60c16ddd9 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -150,12 +150,21 @@ def anonymous_id_for_user(user, course_id): # function: Rotate at will, since the hashes are stored and # will not change. # include the secret key as a salt, and to make the ids unique across different LMS installs. - hasher = hashlib.shake_128() + legacy_hash_enabled = settings.FEATURES.get('ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID', False) + if legacy_hash_enabled: + # Use legacy MD5 algorithm if flag enabled + hasher = hashlib.md5() + else: + hasher = hashlib.shake_128() hasher.update(settings.SECRET_KEY.encode('utf8')) hasher.update(str(user.id).encode('utf8')) if course_id: hasher.update(str(course_id).encode('utf-8')) - anonymous_user_id = hasher.hexdigest(16) + + if legacy_hash_enabled: + anonymous_user_id = hasher.hexdigest() + else: + anonymous_user_id = hasher.hexdigest(16) # pylint: disable=too-many-function-args try: AnonymousUserId.objects.create( diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index b41ad2f856d6..735706304e11 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -1050,6 +1050,17 @@ def test_anonymous_id_secret_key_changes_result_in_diff_values_for_same_new_user assert anonymous_id != new_anonymous_id assert self.user == user_by_anonymous_id(new_anonymous_id) + def test_enable_legacy_hash_flag(self): + """Test that different anonymous id returned if ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID enabled.""" + CourseEnrollment.enroll(self.user, self.course.id) + anonymous_id = anonymous_id_for_user(self.user, self.course.id) + with patch.dict(settings.FEATURES, ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID=True): + # Recreate user object to clear cached anonymous id. + self.user = User.objects.get(pk=self.user.id) + AnonymousUserId.objects.filter(user=self.user).filter(course_id=self.course.id).delete() + new_anonymous_id = anonymous_id_for_user(self.user, self.course.id) + assert anonymous_id != new_anonymous_id + @skip_unless_lms @patch('openedx.core.djangoapps.programs.utils.get_programs') diff --git a/lms/envs/common.py b/lms/envs/common.py index c0182d47355a..2b2af836c059 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1028,6 +1028,17 @@ # .. toggle_creation_date: 2022-06-06 # .. toggle_tickets: 'https://github.com/edx/edx-platform/pull/29538' 'DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED': False, + + # .. toggle_name: FEATURES['ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the legacy MD5 hashing algorithm to generate anonymous user id + # instead of the newer SHAKE128 hashing algorithm + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2022-08-08 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' + 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API From 3101bb7e59ceebd16f25495b088909df160026cf Mon Sep 17 00:00:00 2001 From: Kshitij Sobti <kshitij@sobti.in> Date: Mon, 6 Feb 2023 14:53:15 +0530 Subject: [PATCH 08/19] feat: Add toggle to allow redirecting to courseware after enrollment. This change adds a new waffle switch to redirect a student to coursware after enrolment instead of the dashboard. (cherry picked from commit dbce3225c63c25f07349b0f8693122eff051f4ce) --- common/djangoapps/student/tests/tests.py | 14 ++++++++++++++ common/djangoapps/student/toggles.py | 18 ++++++++++++++++++ common/djangoapps/student/views/management.py | 7 +++++-- 3 files changed, 37 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 735706304e11..8f952831f12b 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -15,6 +15,7 @@ from django.test import TestCase, override_settings from django.test.client import Client from django.urls import reverse +from edx_toggles.toggles.testutils import override_waffle_switch from markupsafe import escape from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import CourseLocator @@ -33,6 +34,7 @@ user_by_anonymous_id ) from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory +from common.djangoapps.student.toggles import REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT from common.djangoapps.student.views import complete_course_mode_info from common.djangoapps.util.model_utils import USER_SETTINGS_CHANGED_EVENT_NAME from common.djangoapps.util.testing import EventTestMixin @@ -893,6 +895,7 @@ def test_change_enrollment_modes(self): @skip_unless_lms +@ddt.ddt class ChangeEnrollmentViewTest(ModuleStoreTestCase): """Tests the student.views.change_enrollment view""" @@ -913,6 +916,17 @@ def _enroll_through_view(self, course): ) return response + @ddt.data( + (True, 'courseware'), + (False, None), + ) + @ddt.unpack + def test_enrollment_url(self, waffle_flag_enabled, returned_view): + with override_waffle_switch(REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT, waffle_flag_enabled): + response = self._enroll_through_view(self.course) + data = reverse(returned_view, args=[str(self.course.id)]) if returned_view else '' + assert response.content.decode('utf8') == data + def test_enroll_as_default(self): """Tests that a student can successfully enroll through this view""" response = self._enroll_through_view(self.course) diff --git a/common/djangoapps/student/toggles.py b/common/djangoapps/student/toggles.py index 46e9b0408fbf..4b773449e5c9 100644 --- a/common/djangoapps/student/toggles.py +++ b/common/djangoapps/student/toggles.py @@ -2,6 +2,7 @@ Toggles for Dashboard page. """ from edx_toggles.toggles import WaffleFlag +from edx_toggles.toggles import WaffleSwitch # Namespace for student waffle flags. WAFFLE_FLAG_NAMESPACE = 'student' @@ -75,3 +76,20 @@ def should_show_2u_recommendations(): def should_send_enrollment_email(): return ENROLLMENT_CONFIRMATION_EMAIL.is_enabled() + +# Waffle flag to enable control redirecting after enrolment. +# .. toggle_name: student.redirect_to_courseware_after_enrollment +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: Redirect to courseware after enrollment instead of dashboard. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2023-02-06 +# .. toggle_target_removal_date: None +# .. toggle_warning: None +REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT = WaffleSwitch( + f'{WAFFLE_FLAG_NAMESPACE}.redirect_to_courseware_after_enrollment', __name__ +) + + +def should_redirect_to_courseware_after_enrollment(): + return REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT.is_enabled() diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index d131575193b4..cb3df1627fd9 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -38,6 +38,7 @@ from rest_framework.decorators import api_view, authentication_classes, permission_classes from rest_framework.permissions import IsAuthenticated +from common.djangoapps.student.toggles import should_redirect_to_courseware_after_enrollment from common.djangoapps.track import views as track_views from lms.djangoapps.bulk_email.models import Optout from common.djangoapps.course_modes.models import CourseMode @@ -400,8 +401,10 @@ def change_enrollment(request, check_access=True): reverse("course_modes_choose", kwargs={'course_id': str(course_id)}) ) - # Otherwise, there is only one mode available (the default) - return HttpResponse() + if should_redirect_to_courseware_after_enrollment(): + return HttpResponse(reverse('courseware', args=[str(course_id)])) + else: + return HttpResponse() elif action == "unenroll": if configuration_helpers.get_value( "DISABLE_UNENROLLMENT", From 17dd0ec3d58bff995b73ef8118d4d08705b5ba30 Mon Sep 17 00:00:00 2001 From: Pooja Kulkarni <13742492+pkulkark@users.noreply.github.com> Date: Thu, 23 Feb 2023 12:09:46 -0500 Subject: [PATCH 09/19] feat: add new endpoint for cloning course (#520) (cherry picked from commit 9d9e3eb7c58a01e3fe62a946d892bd6e50586442) --- .../api/v1/serializers/course_runs.py | 33 ++++++++++++ .../v1/tests/test_views/test_course_runs.py | 51 +++++++++++++++++++ cms/djangoapps/api/v1/views/course_runs.py | 8 +++ 3 files changed, 92 insertions(+) diff --git a/cms/djangoapps/api/v1/serializers/course_runs.py b/cms/djangoapps/api/v1/serializers/course_runs.py index cbd4d09e2181..6bbbce96dd42 100644 --- a/cms/djangoapps/api/v1/serializers/course_runs.py +++ b/cms/djangoapps/api/v1/serializers/course_runs.py @@ -5,6 +5,7 @@ from django.db import transaction from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey from rest_framework import serializers from rest_framework.fields import empty @@ -203,3 +204,35 @@ def update(self, instance, validated_data): course_run = get_course_and_check_access(new_course_run_key, user) self.update_team(course_run, team) return course_run + + +class CourseCloneSerializer(serializers.Serializer): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring + source_course_id = serializers.CharField() + destination_course_id = serializers.CharField() + + def validate(self, attrs): + source_course_id = attrs.get('source_course_id') + destination_course_id = attrs.get('destination_course_id') + store = modulestore() + source_key = CourseKey.from_string(source_course_id) + dest_key = CourseKey.from_string(destination_course_id) + + # Check if the source course exists + if not store.has_course(source_key): + raise serializers.ValidationError('Source course does not exist.') + + # Check if the destination course already exists + if store.has_course(dest_key): + raise serializers.ValidationError('Destination course already exists.') + return attrs + + def create(self, validated_data): + source_course_id = validated_data.get('source_course_id') + destination_course_id = validated_data.get('destination_course_id') + user_id = self.context['request'].user.id + store = modulestore() + source_key = CourseKey.from_string(source_course_id) + dest_key = CourseKey.from_string(destination_course_id) + with store.default_store('split'): + new_course = store.clone_course(source_key, dest_key, user_id) + return new_course diff --git a/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py b/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py index 49589a473878..8366ef72941e 100644 --- a/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py +++ b/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py @@ -402,3 +402,54 @@ def test_rerun_invalid_number(self): assert response.data == {'non_field_errors': [ 'Invalid key supplied. Ensure there are no special characters in the Course Number.' ]} + + def test_clone_course(self): + course = CourseFactory() + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': str(course.id), + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 201 + self.assertEqual(response.data, {"message": "Course cloned successfully."}) + + def test_clone_course_with_missing_source_id(self): + url = reverse('api:v1:course_run-clone') + data = { + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + self.assertEqual(response.data, {'source_course_id': ['This field is required.']}) + + def test_clone_course_with_missing_dest_id(self): + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': 'course-v1:source+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + self.assertEqual(response.data, {'destination_course_id': ['This field is required.']}) + + def test_clone_course_with_nonexistent_source_course(self): + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': 'course-v1:nonexistent+source+course_id', + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + assert str(response.data.get('non_field_errors')[0]) == 'Source course does not exist.' + + def test_clone_course_with_existing_dest_course(self): + url = reverse('api:v1:course_run-clone') + course = CourseFactory() + existing_dest_course = CourseFactory() + data = { + 'source_course_id': str(course.id), + 'destination_course_id': str(existing_dest_course.id), + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + assert str(response.data.get('non_field_errors')[0]) == 'Destination course already exists.' diff --git a/cms/djangoapps/api/v1/views/course_runs.py b/cms/djangoapps/api/v1/views/course_runs.py index a0415d4e06dc..fb1671ebef04 100644 --- a/cms/djangoapps/api/v1/views/course_runs.py +++ b/cms/djangoapps/api/v1/views/course_runs.py @@ -13,6 +13,7 @@ from cms.djangoapps.contentstore.views.course import _accessible_courses_iter, get_course_and_check_access from ..serializers.course_runs import ( + CourseCloneSerializer, CourseRunCreateSerializer, CourseRunImageSerializer, CourseRunRerunSerializer, @@ -93,3 +94,10 @@ def rerun(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=miss new_course_run = serializer.save() serializer = self.get_serializer(new_course_run) return Response(serializer.data, status=status.HTTP_201_CREATED) + + @action(detail=False, methods=['post']) + def clone(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=missing-function-docstring, unused-argument + serializer = CourseCloneSerializer(data=request.data, context=self.get_serializer_context()) + serializer.is_valid(raise_exception=True) + serializer.save() + return Response({"message": "Course cloned successfully."}, status=status.HTTP_201_CREATED) From 64d2883e468b71e168297fccccae4a15baebd273 Mon Sep 17 00:00:00 2001 From: Paulo Viadanna <paulo@opencraft.com> Date: Thu, 30 Mar 2023 11:12:35 -0300 Subject: [PATCH 10/19] feat: implements SHOW_REGISTRATION_LINKS feature toggle (#528) (cherry picked from commit 3025ab5fe6f6f53d6af5b36681355efafa37c74b) --- cms/envs/common.py | 3 +++ cms/templates/howitworks.html | 2 +- cms/templates/widgets/header.html | 2 +- lms/envs/common.py | 9 +++++++++ lms/static/js/student_account/views/AccessView.js | 13 ++++++++----- lms/static/js/student_account/views/FormView.js | 2 ++ lms/static/js/student_account/views/LoginView.js | 1 + lms/static/js/student_account/views/RegisterView.js | 4 ++-- lms/templates/header/navbar-not-authenticated.html | 2 +- .../navigation/navbar-not-authenticated.html | 2 +- lms/templates/student_account/form_field.underscore | 6 ++++-- .../core/djangoapps/user_authn/views/login_form.py | 1 + 12 files changed, 34 insertions(+), 13 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index c0104badced7..5ede0b2acb94 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -347,6 +347,9 @@ # Allow public account creation 'ALLOW_PUBLIC_ACCOUNT_CREATION': True, + # Allow showing the registration links + 'SHOW_REGISTRATION_LINKS': True, + # Whether or not the dynamic EnrollmentTrackUserPartition should be registered. 'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': True, diff --git a/cms/templates/howitworks.html b/cms/templates/howitworks.html index b04a207eba6f..d21dff2d66a5 100644 --- a/cms/templates/howitworks.html +++ b/cms/templates/howitworks.html @@ -154,7 +154,7 @@ <h4 class="title">${_("Work in Teams")}</h4> </section> </div> -% if static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')): +% if static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')) and settings.FEATURES.get('SHOW_REGISTRATION_LINKS', True): <div class="wrapper-content-cta wrapper"> <section class="content content-cta"> <header> diff --git a/cms/templates/widgets/header.html b/cms/templates/widgets/header.html index 48b818da95a5..6f110caf2e88 100644 --- a/cms/templates/widgets/header.html +++ b/cms/templates/widgets/header.html @@ -268,7 +268,7 @@ <h2 class="sr-only">${_("Account Navigation")}</h2> <li class="nav-item nav-not-signedin-help"> <a href="${get_online_help_info(online_help_token)['doc_url']}" title="${_('Contextual Online Help')}" rel="noopener" target="_blank">${_("Help")}</a> </li> - % if static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')): + % if static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')) and settings.FEATURES.get('SHOW_REGISTRATION_LINKS', True): <li class="nav-item nav-not-signedin-signup"> <a class="action action-signup" href="${settings.FRONTEND_REGISTER_URL}?next=${quote_plus(request.build_absolute_uri(settings.LOGIN_URL))}">${_("Sign Up")}</a> </li> diff --git a/lms/envs/common.py b/lms/envs/common.py index 2b2af836c059..2b1f8ce22e35 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -777,6 +777,15 @@ # .. toggle_tickets: https://openedx.atlassian.net/browse/YONK-513 'ALLOW_PUBLIC_ACCOUNT_CREATION': True, + # .. toggle_name: FEATURES['SHOW_REGISTRATION_LINKS'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: True + # .. toggle_description: Allow registration links. If this is disabled, users will no longer see buttons to the + # the signup page. + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2023-03-27 + 'SHOW_REGISTRATION_LINKS': True, + # .. toggle_name: FEATURES['ENABLE_COOKIE_CONSENT'] # .. toggle_implementation: DjangoSetting # .. toggle_default: False diff --git a/lms/static/js/student_account/views/AccessView.js b/lms/static/js/student_account/views/AccessView.js index 7a2e1a858d36..6c5b3d2be696 100644 --- a/lms/static/js/student_account/views/AccessView.js +++ b/lms/static/js/student_account/views/AccessView.js @@ -75,7 +75,8 @@ this.platformName = options.platform_name; this.supportURL = options.support_link; this.passwordResetSupportUrl = options.password_reset_support_link; - this.createAccountOption = options.account_creation_allowed; + this.createAccountOption = options.account_creation_allowed && options.register_links_allowed; + this.showRegisterLinks = options.register_links_allowed this.hideAuthWarnings = options.hide_auth_warnings || false; this.pipelineUserDetails = options.third_party_auth.pipeline_user_details; this.enterpriseName = options.enterprise_name || ''; @@ -161,7 +162,7 @@ supportURL: this.supportURL, passwordResetSupportUrl: this.passwordResetSupportUrl, createAccountOption: this.createAccountOption, - hideAuthWarnings: this.hideAuthWarnings, + showRegisterLinks: this.showRegisterLinks,hideAuthWarnings: this.hideAuthWarnings, pipelineUserDetails: this.pipelineUserDetails, enterpriseName: this.enterpriseName, enterpriseSlugLoginURL: this.enterpriseSlugLoginURL, @@ -186,8 +187,9 @@ this.subview.passwordHelp = new PasswordResetView({ fields: data.fields, - model: this.resetModel - }); + model: this.resetModel, + showRegisterLinks: this.showRegisterLinks + }); // Listen for 'password-email-sent' event to toggle sub-views this.listenTo(this.subview.passwordHelp, 'password-email-sent', this.passwordEmailSent); @@ -211,7 +213,8 @@ hideAuthWarnings: this.hideAuthWarnings, is_require_third_party_auth_enabled: this.is_require_third_party_auth_enabled, enableCoppaCompliance: this.enable_coppa_compliance, - }); + showRegisterLinks: this.showRegisterLinks + }); // Listen for 'auth-complete' event so we can enroll/redirect the user appropriately. this.listenTo(this.subview.register, 'auth-complete', this.authComplete); diff --git a/lms/static/js/student_account/views/FormView.js b/lms/static/js/student_account/views/FormView.js index d5cbb75868bd..5a9a269612af 100644 --- a/lms/static/js/student_account/views/FormView.js +++ b/lms/static/js/student_account/views/FormView.js @@ -35,6 +35,7 @@ initialize: function(data) { this.model = data.model; + this.showRegisterLinks = data.showRegisterLinks; this.preRender(data); this.tpl = $(this.tpl).html(); @@ -97,6 +98,7 @@ supplementalText: data[i].supplementalText || '', supplementalLink: data[i].supplementalLink || '', loginIssueSupportLink: data[i].loginIssueSupportLink || '', + showRegisterLinks: this.showRegisterLinks, isEnterpriseEnable: this.isEnterpriseEnable }))); } diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js index 10c6bc309cdd..8357ac6fa885 100644 --- a/lms/static/js/student_account/views/LoginView.js +++ b/lms/static/js/student_account/views/LoginView.js @@ -52,6 +52,7 @@ this.supportURL = data.supportURL; this.passwordResetSupportUrl = data.passwordResetSupportUrl; this.createAccountOption = data.createAccountOption; + this.showRegisterLinks = data.showRegisterLinks; this.accountActivationMessages = data.accountActivationMessages; this.accountRecoveryMessages = data.accountRecoveryMessages; this.hideAuthWarnings = data.hideAuthWarnings; diff --git a/lms/static/js/student_account/views/RegisterView.js b/lms/static/js/student_account/views/RegisterView.js index af6353ac407a..f463c6bfd2f6 100644 --- a/lms/static/js/student_account/views/RegisterView.js +++ b/lms/static/js/student_account/views/RegisterView.js @@ -88,8 +88,8 @@ requiredStr: this.requiredStr, optionalStr: fields[i].name === 'marketing_emails_opt_in' ? '' : this.optionalStr, supplementalText: fields[i].supplementalText || '', - supplementalLink: fields[i].supplementalLink || '' - }))); + supplementalLink: fields[i].supplementalLink || '', + showRegisterLinks: this.showRegisterLinks}))); } html.push('</div>'); return html; diff --git a/lms/templates/header/navbar-not-authenticated.html b/lms/templates/header/navbar-not-authenticated.html index 47aa9abd2ced..945e494bb0a7 100644 --- a/lms/templates/header/navbar-not-authenticated.html +++ b/lms/templates/header/navbar-not-authenticated.html @@ -19,7 +19,7 @@ courses_are_browsable = settings.FEATURES.get('COURSES_ARE_BROWSABLE') allows_login = not settings.FEATURES['DISABLE_LOGIN_BUTTON'] and not combined_login_and_register can_discover_courses = settings.FEATURES.get('ENABLE_COURSE_DISCOVERY') - allow_public_account_creation = static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')) + allow_public_account_creation = static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')) and settings.FEATURES.get('SHOW_REGISTRATION_LINKS', True) should_redirect_to_authn_mfe = should_redirect_to_authn_microfrontend() %> <nav class="nav-links" aria-label=${_("Supplemental Links")}> diff --git a/lms/templates/navigation/navbar-not-authenticated.html b/lms/templates/navigation/navbar-not-authenticated.html index a82a24e87834..418419550929 100644 --- a/lms/templates/navigation/navbar-not-authenticated.html +++ b/lms/templates/navigation/navbar-not-authenticated.html @@ -34,7 +34,7 @@ <a class="btn" href="/courses">${_("Explore Courses")}</a> </li> %endif - % if static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')): + % if static.get_value('ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION')) and settings.FEATURES.get('SHOW_REGISTRATION_LINKS', True): <li class="item nav-global-04"> <a class="btn btn-neutral btn-register" href="/register${login_query()}">${_("Register")}</a> </li> diff --git a/lms/templates/student_account/form_field.underscore b/lms/templates/student_account/form_field.underscore index e64f9af4e4c4..bcd488ae24a2 100644 --- a/lms/templates/student_account/form_field.underscore +++ b/lms/templates/student_account/form_field.underscore @@ -147,10 +147,12 @@ <button type="button" class="enterprise-login field-link"><%- gettext("Sign in with your company or school") %></button> <% } %> <% } %> - <% if( form === 'password-reset' && name === 'email' ) { %> + <% if( (showRegisterLinks || loginIssueSupportLink) && form === 'password-reset' && name === 'email' ) { %> <button type="button" class="reset-help field-link" ><i class="fa fa-caret-right" /><%- gettext("Need other help signing in?") %></button> <div id="reset-help" style="display:none"> - <button type="button" class="field-link form-toggle" data-type="register"><%- gettext("Create an account") %></button> + <% if ( showRegisterLinks ) { %> + <button type="button" class="field-link form-toggle" data-type="register"><%- gettext("Create an account") %></button> + <% } %> <% if ( loginIssueSupportLink ) { %> <span><a class="field-link" href="<%- loginIssueSupportLink %>"><%- gettext("Other sign-in issues") %></a></span> <% } %> diff --git a/openedx/core/djangoapps/user_authn/views/login_form.py b/openedx/core/djangoapps/user_authn/views/login_form.py index ee04123ddf9a..bb78a9df1a3c 100644 --- a/openedx/core/djangoapps/user_authn/views/login_form.py +++ b/openedx/core/djangoapps/user_authn/views/login_form.py @@ -258,6 +258,7 @@ def login_and_registration_form(request, initial_mode="login"): 'password_reset_form_desc': json.loads(form_descriptions['password_reset']), 'account_creation_allowed': configuration_helpers.get_value( 'ALLOW_PUBLIC_ACCOUNT_CREATION', settings.FEATURES.get('ALLOW_PUBLIC_ACCOUNT_CREATION', True)), + 'register_links_allowed': settings.FEATURES.get('SHOW_REGISTRATION_LINKS', True), 'is_account_recovery_feature_enabled': is_secondary_email_feature_enabled(), 'enterprise_slug_login_url': get_enterprise_slug_login_url(), 'is_enterprise_enable': enterprise_enabled(), From c41021213b2e752a0b02d3c86e07d1643b23cc3c Mon Sep 17 00:00:00 2001 From: Fox Danger Piacenti <fox@opencraft.com> Date: Tue, 11 Apr 2023 17:51:18 -0500 Subject: [PATCH 11/19] refactor: Duplicate and update primitives made available. (cherry picked from commit a91b495cae0ebb7f2a8fe2225a700a3826f01a06) --- .../contentstore/tests/test_libraries.py | 6 +- cms/djangoapps/contentstore/views/block.py | 115 ++++++++++----- .../contentstore/views/tests/test_block.py | 133 +++++++++++++++++- xmodule/modulestore/split_mongo/split.py | 14 +- xmodule/modulestore/store_utilities.py | 27 +++- .../modulestore/tests/test_store_utilities.py | 46 +++++- 6 files changed, 286 insertions(+), 55 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 3e850e4794d7..13da5cee2b54 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -16,7 +16,7 @@ from cms.djangoapps.contentstore.tests.utils import AjaxEnabledTestClient, parse_json from cms.djangoapps.contentstore.utils import reverse_library_url, reverse_url, reverse_usage_url -from cms.djangoapps.contentstore.views.block import _duplicate_block +from cms.djangoapps.contentstore.views.block import duplicate_block from cms.djangoapps.contentstore.views.preview import _load_preview_block from cms.djangoapps.contentstore.views.tests.test_library import LIBRARY_REST_URL from cms.djangoapps.course_creators.views import add_user_with_status_granted @@ -947,7 +947,7 @@ def test_persistent_overrides(self, duplicate): if duplicate: # Check that this also works when the RCB is duplicated. self.lc_block = modulestore().get_item( - _duplicate_block(self.course.location, self.lc_block.location, self.user) + duplicate_block(self.course.location, self.lc_block.location, self.user) ) self.problem_in_course = modulestore().get_item(self.lc_block.children[0]) else: @@ -1006,7 +1006,7 @@ def test_duplicated_version(self): # Duplicate self.lc_block: duplicate = store.get_item( - _duplicate_block(self.course.location, self.lc_block.location, self.user) + duplicate_block(self.course.location, self.lc_block.location, self.user) ) # The duplicate should have identical children to the original: self.assertEqual(len(duplicate.children), 1) diff --git a/cms/djangoapps/contentstore/views/block.py b/cms/djangoapps/contentstore/views/block.py index 65052ae30941..7305bc9adc89 100644 --- a/cms/djangoapps/contentstore/views/block.py +++ b/cms/djangoapps/contentstore/views/block.py @@ -242,11 +242,11 @@ def xblock_handler(request, usage_key_string=None): status=400 ) - dest_usage_key = _duplicate_block( + dest_usage_key = duplicate_block( parent_usage_key, duplicate_source_usage_key, request.user, - request.json.get('display_name'), + display_name=request.json.get('display_name'), ) return JsonResponse({ 'locator': str(dest_usage_key), @@ -879,47 +879,88 @@ def _move_item(source_usage_key, target_parent_usage_key, user, target_index=Non return JsonResponse(context) -def _duplicate_block(parent_usage_key, duplicate_source_usage_key, user, display_name=None, is_child=False): +def gather_block_attributes(source_item, display_name=None, is_child=False): + """ + Gather all the attributes of the source block that need to be copied over to a new or updated block. + """ + # Update the display name to indicate this is a duplicate (unless display name provided). + # Can't use own_metadata(), b/c it converts data for JSON serialization - + # not suitable for setting metadata of the new block + duplicate_metadata = {} + for field in source_item.fields.values(): + if field.scope == Scope.settings and field.is_set_on(source_item): + duplicate_metadata[field.name] = field.read_from(source_item) + + if is_child: + display_name = display_name or source_item.display_name or source_item.category + + if display_name is not None: + duplicate_metadata['display_name'] = display_name + else: + if source_item.display_name is None: + duplicate_metadata['display_name'] = _("Duplicate of {0}").format(source_item.category) + else: + duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) + + asides_to_create = [] + for aside in source_item.runtime.get_asides(source_item): + for field in aside.fields.values(): + if field.scope in (Scope.settings, Scope.content,) and field.is_set_on(aside): + asides_to_create.append(aside) + break + + for aside in asides_to_create: + for field in aside.fields.values(): + if field.scope not in (Scope.settings, Scope.content,): + field.delete_from(aside) + return duplicate_metadata, asides_to_create + + +def update_from_source(*, source_block, destination_block, user_id): + """ + Update a block to have all the settings and attributes of another source. + + Copies over all attributes and settings of a source block to a destination + block. Blocks must be the same type. This function does not modify or duplicate + children. + """ + duplicate_metadata, asides = gather_block_attributes(source_block, display_name=source_block.display_name) + for key, value in duplicate_metadata.items(): + setattr(destination_block, key, value) + for key, value in source_block.get_explicitly_set_fields_by_scope(Scope.content).items(): + setattr(destination_block, key, value) + modulestore().update_item( + destination_block, + user_id, + metadata=duplicate_metadata, + asides=asides, + ) + + +def duplicate_block( + parent_usage_key, + duplicate_source_usage_key, + user, + dest_usage_key=None, + display_name=None, + shallow=False, + is_child=False +): """ Duplicate an existing xblock as a child of the supplied parent_usage_key. """ store = modulestore() with store.bulk_operations(duplicate_source_usage_key.course_key): source_item = store.get_item(duplicate_source_usage_key) - # Change the blockID to be unique. - dest_usage_key = source_item.location.replace(name=uuid4().hex) - category = dest_usage_key.block_type - - # Update the display name to indicate this is a duplicate (unless display name provided). - # Can't use own_metadata(), b/c it converts data for JSON serialization - - # not suitable for setting metadata of the new block - duplicate_metadata = {} - for field in source_item.fields.values(): - if field.scope == Scope.settings and field.is_set_on(source_item): - duplicate_metadata[field.name] = field.read_from(source_item) - - if is_child: - display_name = display_name or source_item.display_name or source_item.category - - if display_name is not None: - duplicate_metadata['display_name'] = display_name - else: - if source_item.display_name is None: - duplicate_metadata['display_name'] = _("Duplicate of {0}").format(source_item.category) - else: - duplicate_metadata['display_name'] = _("Duplicate of '{0}'").format(source_item.display_name) + if not dest_usage_key: + # Change the blockID to be unique. + dest_usage_key = source_item.location.replace(name=uuid4().hex) - asides_to_create = [] - for aside in source_item.runtime.get_asides(source_item): - for field in aside.fields.values(): - if field.scope in (Scope.settings, Scope.content,) and field.is_set_on(aside): - asides_to_create.append(aside) - break + category = dest_usage_key.block_type - for aside in asides_to_create: - for field in aside.fields.values(): - if field.scope not in (Scope.settings, Scope.content,): - field.delete_from(aside) + duplicate_metadata, asides_to_create = gather_block_attributes( + source_item, display_name=display_name, is_child=is_child, + ) dest_block = store.create_item( user.id, @@ -943,10 +984,10 @@ def _duplicate_block(parent_usage_key, duplicate_source_usage_key, user, display # Children are not automatically copied over (and not all xblocks have a 'children' attribute). # Because DAGs are not fully supported, we need to actually duplicate each child as well. - if source_item.has_children and not children_handled: + if source_item.has_children and not shallow and not children_handled: dest_block.children = dest_block.children or [] for child in source_item.children: - dupe = _duplicate_block(dest_block.location, child, user=user, is_child=True) + dupe = duplicate_block(dest_block.location, child, user=user, is_child=True) if dupe not in dest_block.children: # _duplicate_block may add the child for us. dest_block.children.append(dupe) store.update_item(dest_block, user.id) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 510c30d858ee..716b0d9a70b5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -70,7 +70,7 @@ _get_source_index, _xblock_type_and_display_name, add_container_page_publishing_info, - create_xblock_info, + create_xblock_info, duplicate_block, update_from_source, ) @@ -789,6 +789,29 @@ def verify_name(source_usage_key, parent_usage_key, expected_name, display_name= # Now send a custom display name for the duplicate. verify_name(self.seq_usage_key, self.chapter_usage_key, "customized name", display_name="customized name") + def test_shallow_duplicate(self): + """ + Test that shallow_duplicate creates a new block. + """ + source_course = CourseFactory() + user = UserFactory.create() + source_chapter = BlockFactory(parent=source_course, category='chapter', display_name='Source Chapter') + BlockFactory(parent=source_chapter, category='html', display_name='Child') + # Refresh. + source_chapter = self.store.get_item(source_chapter.location) + self.assertEqual(len(source_chapter.get_children()), 1) + destination_course = CourseFactory() + destination_location = duplicate_block( + parent_usage_key=destination_course.location, duplicate_source_usage_key=source_chapter.location, + user=user, + display_name=source_chapter.display_name, + shallow=True, + ) + # Refresh here, too, just to be sure. + destination_chapter = self.store.get_item(destination_location) + self.assertEqual(len(destination_chapter.get_children()), 0) + self.assertEqual(destination_chapter.display_name, 'Source Chapter') + @ddt.ddt class TestMoveItem(ItemTest): @@ -3620,3 +3643,111 @@ def test_creator_show_delete_button_with_waffle(self): ) self.assertFalse(xblock_info['show_delete_button']) + + +@patch('xmodule.modulestore.split_mongo.caching_descriptor_system.CachingDescriptorSystem.applicable_aside_types', + lambda self, block: ['test_aside']) +class TestUpdateFromSource(ModuleStoreTestCase): + """ + Test update_from_source. + """ + + def setUp(self): + """ + Set up the runtime for tests. + """ + super().setUp() + key_store = DictKeyValueStore() + field_data = KvsFieldData(key_store) + self.runtime = TestRuntime(services={'field-data': field_data}) + + def create_source_block(self, course): + """ + Create a chapter with all the fixings. + """ + source_block = BlockFactory( + parent=course, + category='course_info', + display_name='Source Block', + metadata={'due': datetime(2010, 11, 22, 4, 0, tzinfo=UTC)}, + ) + + def_id = self.runtime.id_generator.create_definition('html') + usage_id = self.runtime.id_generator.create_usage(def_id) + + aside = AsideTest(scope_ids=ScopeIds('user', 'html', def_id, usage_id), runtime=self.runtime) + aside.field11 = 'html_new_value1' + + # The data attribute is handled in a special manner and should be updated. + source_block.data = '<div>test</div>' + # This field is set on the content scope (definition_data), which should be updated. + source_block.items = ['test', 'beep'] + + self.store.update_item(source_block, self.user.id, asides=[aside]) + + # quick sanity checks + source_block = self.store.get_item(source_block.location) + self.assertEqual(source_block.due, datetime(2010, 11, 22, 4, 0, tzinfo=UTC)) + self.assertEqual(source_block.display_name, 'Source Block') + self.assertEqual(source_block.runtime.get_asides(source_block)[0].field11, 'html_new_value1') + self.assertEqual(source_block.data, '<div>test</div>') + self.assertEqual(source_block.items, ['test', 'beep']) + + return source_block + + def check_updated(self, source_block, destination_key): + """ + Check that the destination block has been updated to match our source block. + """ + revised = self.store.get_item(destination_key) + self.assertEqual(source_block.display_name, revised.display_name) + self.assertEqual(source_block.due, revised.due) + self.assertEqual(revised.data, source_block.data) + self.assertEqual(revised.items, source_block.items) + + self.assertEqual( + revised.runtime.get_asides(revised)[0].field11, + source_block.runtime.get_asides(source_block)[0].field11, + ) + + @XBlockAside.register_temp_plugin(AsideTest, 'test_aside') + def test_update_from_source(self): + """ + Test that update_from_source updates the destination block. + """ + course = CourseFactory() + user = UserFactory.create() + + source_block = self.create_source_block(course) + + destination_block = BlockFactory(parent=course, category='course_info', display_name='Destination Problem') + update_from_source(source_block=source_block, destination_block=destination_block, user_id=user.id) + self.check_updated(source_block, destination_block.location) + + @XBlockAside.register_temp_plugin(AsideTest, 'test_aside') + def test_update_clobbers(self): + """ + Verify that our update clobbers everything. + """ + course = CourseFactory() + user = UserFactory.create() + + source_block = self.create_source_block(course) + + destination_block = BlockFactory( + parent=course, + category='course_info', + display_name='Destination Chapter', + metadata={'due': datetime(2025, 10, 21, 6, 5, tzinfo=UTC)}, + ) + + def_id = self.runtime.id_generator.create_definition('html') + usage_id = self.runtime.id_generator.create_usage(def_id) + aside = AsideTest(scope_ids=ScopeIds('user', 'html', def_id, usage_id), runtime=self.runtime) + aside.field11 = 'Other stuff' + destination_block.data = '<div>other stuff</div>' + destination_block.items = ['other stuff', 'boop'] + self.store.update_item(destination_block, user.id, asides=[aside]) + + update_from_source(source_block=source_block, destination_block=destination_block, user_id=user.id) + self.check_updated(source_block, destination_block.location) diff --git a/xmodule/modulestore/split_mongo/split.py b/xmodule/modulestore/split_mongo/split.py index 8e7999ae75bc..7982ba46723c 100644 --- a/xmodule/modulestore/split_mongo/split.py +++ b/xmodule/modulestore/split_mongo/split.py @@ -57,7 +57,6 @@ import copy import datetime -import hashlib import logging from collections import defaultdict from importlib import import_module @@ -102,7 +101,7 @@ ) from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope from xmodule.modulestore.split_mongo.mongo_connection import DuplicateKeyError, DjangoFlexPersistenceBackend -from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES +from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES, derived_key from xmodule.partitions.partitions_service import PartitionService from xmodule.util.misc import get_library_or_course_attribute @@ -2438,7 +2437,6 @@ def _copy_from_template( for usage_key in source_keys: src_course_key = usage_key.course_key - hashable_source_id = src_course_key.for_version(None) block_key = BlockKey(usage_key.block_type, usage_key.block_id) source_structure = source_structures[src_course_key] @@ -2446,15 +2444,7 @@ def _copy_from_template( raise ItemNotFoundError(usage_key) source_block_info = source_structure['blocks'][block_key] - # Compute a new block ID. This new block ID must be consistent when this - # method is called with the same (source_key, dest_structure) pair - unique_data = "{}:{}:{}".format( - str(hashable_source_id).encode("utf-8"), - block_key.id, - new_parent_block_key.id, - ) - new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20] - new_block_key = BlockKey(block_key.type, new_block_id) + new_block_key = derived_key(src_course_key, block_key, new_parent_block_key) # Now clone block_key to new_block_key: new_block_info = copy.deepcopy(source_block_info) diff --git a/xmodule/modulestore/store_utilities.py b/xmodule/modulestore/store_utilities.py index 45082ff43e75..964b4b459832 100644 --- a/xmodule/modulestore/store_utilities.py +++ b/xmodule/modulestore/store_utilities.py @@ -1,5 +1,5 @@ # lint-amnesty, pylint: disable=missing-module-docstring - +import hashlib import logging import re import uuid @@ -7,6 +7,8 @@ from xblock.core import XBlock +from xmodule.modulestore.split_mongo import BlockKey + DETACHED_XBLOCK_TYPES = {name for name, __ in XBlock.load_tagged_classes("detached")} @@ -104,3 +106,26 @@ def get_draft_subtree_roots(draft_nodes): for draft_node in draft_nodes: if draft_node.parent_url not in urls: yield draft_node + + +def derived_key(courselike_source_key, block_key, parent): + """ + Return a new reproducible block ID for a given root, source block, and destination parent. + + When recursively copying a block structure, we need to generate new block IDs for the + blocks. We don't want to use the exact same IDs as we might copy blocks multiple times. + However, we do want to be able to reproduce the same IDs when copying the same block + so that if we ever need to re-copy the block from its source (that is, to update it with + upstream changes) we don't affect any data tied to the ID, such as grades. + """ + hashable_source_id = courselike_source_key.for_version(None) + + # Compute a new block ID. This new block ID must be consistent when this + # method is called with the same (source_key, dest_structure) pair + unique_data = "{}:{}:{}".format( + str(hashable_source_id).encode("utf-8"), + block_key.id, + parent.id, + ) + new_block_id = hashlib.sha1(unique_data.encode('utf-8')).hexdigest()[:20] + return BlockKey(block_key.type, new_block_id) diff --git a/xmodule/modulestore/tests/test_store_utilities.py b/xmodule/modulestore/tests/test_store_utilities.py index 1c1c86f4fcd3..a0b5cfcbdbb6 100644 --- a/xmodule/modulestore/tests/test_store_utilities.py +++ b/xmodule/modulestore/tests/test_store_utilities.py @@ -4,11 +4,15 @@ import unittest +from unittest import TestCase from unittest.mock import Mock import ddt -from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots +from opaque_keys.edx.keys import CourseKey + +from xmodule.modulestore.split_mongo import BlockKey +from xmodule.modulestore.store_utilities import draft_node_constructor, get_draft_subtree_roots, derived_key @ddt.ddt @@ -82,3 +86,43 @@ def test_get_draft_subtree_roots(self, node_arguments_list, expected_roots_urls) subtree_roots_urls = [root.url for root in get_draft_subtree_roots(block_nodes)] # check that we return the expected urls assert set(subtree_roots_urls) == set(expected_roots_urls) + + +mock_block = Mock() +mock_block.id = CourseKey.from_string('course-v1:Beeper+B33P+BOOP') + + +derived_key_scenarios = [ + { + 'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + 'block_key': BlockKey('chapter', 'interactive_demonstrations'), + 'parent': mock_block, + 'expected': BlockKey( + 'chapter', '5793ec64e25ed870a7dd', + ), + }, + { + 'courselike_source_key': CourseKey.from_string('course-v1:edX+DemoX+Demo_Course'), + 'block_key': BlockKey('chapter', 'interactive_demonstrations'), + 'parent': BlockKey( + 'chapter', 'thingy', + ), + 'expected': BlockKey( + 'chapter', '599792a5622d85aa41e6', + ), + } +] + + +@ddt.ddt +class TestDerivedKey(TestCase): + """ + Test reproducible block ID generation. + """ + @ddt.data(*derived_key_scenarios) + @ddt.unpack + def test_derived_key(self, courselike_source_key, block_key, parent, expected): + """ + Test that derived_key returns the expected value. + """ + self.assertEqual(derived_key(courselike_source_key, block_key, parent), expected) From 75aa91de50f9560888ad412129e9bfaf68898509 Mon Sep 17 00:00:00 2001 From: Agrendalath <piotr@surowiec.it> Date: Fri, 21 Apr 2023 16:26:40 +0200 Subject: [PATCH 12/19] fix: support files with a unicode signature in the Instructor Dashboard API Without this, files with BOM (byte order mark; generated e.g., by Microsoft Excel) cannot be read properly. (cherry picked from commit 7d8a5029f38f758a0deb176dc0d869228ca10ad3) --- lms/djangoapps/instructor/tests/test_api.py | 32 ++++++--------------- lms/djangoapps/instructor/views/api.py | 6 ++-- 2 files changed, 11 insertions(+), 27 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 54adcb3e3638..bb6b69eb9f03 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -590,6 +590,7 @@ def test_instructor_level(self): @patch.dict(settings.FEATURES, {'ALLOW_AUTOMATED_SIGNUPS': True}) +@ddt.ddt class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCase, LoginEnrollmentTestCase): """ Test Bulk account creation and enrollment from csv file @@ -641,32 +642,15 @@ def setUp(self): ) @patch('lms.djangoapps.instructor.views.api.log.info') - def test_account_creation_and_enrollment_with_csv(self, info_log): - """ - Happy path test to create a single new user - """ - csv_content = b"test_student@example.com,test_student_1,tester1,USA" - uploaded_file = SimpleUploadedFile("temp.csv", csv_content) - response = self.client.post(self.url, {'students_list': uploaded_file, 'email-students': True}) - assert response.status_code == 200 - data = json.loads(response.content.decode('utf-8')) - assert len(data['row_errors']) == 0 - assert len(data['warnings']) == 0 - assert len(data['general_errors']) == 0 - - manual_enrollments = ManualEnrollmentAudit.objects.all() - assert manual_enrollments.count() == 1 - assert manual_enrollments[0].state_transition == UNENROLLED_TO_ENROLLED - - # test the log for email that's send to new created user. - info_log.assert_called_with('email sent to new created user at %s', 'test_student@example.com') - - @patch('lms.djangoapps.instructor.views.api.log.info') - def test_account_creation_and_enrollment_with_csv_with_blank_lines(self, info_log): + @ddt.data( + b"test_student@example.com,test_student_1,tester1,USA", # Typical use case. + b"\ntest_student@example.com,test_student_1,tester1,USA\n\n", # Blank lines. + b"\xef\xbb\xbftest_student@example.com,test_student_1,tester1,USA", # Unicode signature (BOM). + ) + def test_account_creation_and_enrollment_with_csv(self, csv_content, info_log): """ Happy path test to create a single new user """ - csv_content = b"\ntest_student@example.com,test_student_1,tester1,USA\n\n" uploaded_file = SimpleUploadedFile("temp.csv", csv_content) response = self.client.post(self.url, {'students_list': uploaded_file, 'email-students': True}) assert response.status_code == 200 @@ -4385,7 +4369,7 @@ def call_add_users_to_cohorts(self, csv_data, suffix='.csv'): """ # this temporary file will be removed in `self.tearDown()` __, file_name = tempfile.mkstemp(suffix=suffix, dir=self.tempdir) - with open(file_name, 'w') as file_pointer: + with open(file_name, 'w', encoding='utf-8-sig') as file_pointer: file_pointer.write(csv_data) with open(file_name) as file_pointer: url = reverse('add_users_to_cohorts', kwargs={'course_id': str(self.course.id)}) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index eb0d6aa8a5a7..c83af8e1a430 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -339,7 +339,7 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man try: upload_file = request.FILES.get('students_list') if upload_file.name.endswith('.csv'): - students = list(csv.reader(upload_file.read().decode('utf-8').splitlines())) + students = list(csv.reader(upload_file.read().decode('utf-8-sig').splitlines())) course = get_course_by_id(course_id) else: general_errors.append({ @@ -1519,7 +1519,7 @@ def _cohorts_csv_validator(file_storage, file_to_validate): Verifies that the expected columns are present in the CSV used to add users to cohorts. """ with file_storage.open(file_to_validate) as f: - reader = csv.reader(f.read().decode('utf-8').splitlines()) + reader = csv.reader(f.read().decode('utf-8-sig').splitlines()) try: fieldnames = next(reader) @@ -3332,7 +3332,7 @@ def build_row_errors(key, _user, row_count): try: upload_file = request.FILES.get('students_list') if upload_file.name.endswith('.csv'): - students = list(csv.reader(upload_file.read().decode('utf-8').splitlines())) + students = list(csv.reader(upload_file.read().decode('utf-8-sig').splitlines())) else: general_errors.append(_('Make sure that the file you upload is in CSV format with no ' 'extraneous characters or rows.')) From 923a2d85ab1290a973867768f3bd72cf3b85d12c Mon Sep 17 00:00:00 2001 From: Agrendalath <piotr@surowiec.it> Date: Wed, 19 Apr 2023 14:48:11 +0200 Subject: [PATCH 13/19] fix: remove relative due date limit It was an artificial limit enforced by the frontend. The backend does not have limitations that would prevent it from using a higher value. (cherry picked from commit 08e9ec342d161f39d71e56a74ff5f644eb1c1b4a) --- cms/static/js/spec/views/pages/course_outline_spec.js | 10 +--------- cms/static/js/views/modals/course_outline_modals.js | 11 +++-------- .../js/self-paced-due-date-editor.underscore | 6 +----- 3 files changed, 5 insertions(+), 22 deletions(-) diff --git a/cms/static/js/spec/views/pages/course_outline_spec.js b/cms/static/js/spec/views/pages/course_outline_spec.js index efa20d850e6a..074a77c47324 100644 --- a/cms/static/js/spec/views/pages/course_outline_spec.js +++ b/cms/static/js/spec/views/pages/course_outline_spec.js @@ -2258,13 +2258,6 @@ describe('CourseOutlinePage', function() { it('shows validation error on relative date', function() { outlinePage.$('.outline-subsection .configure-button').click(); - // when due number of weeks goes over 18 - selectRelativeWeeksSubsection('19'); - expect($('#relative_weeks_due_warning_max').css('display')).not.toBe('none'); - expect($('#relative_weeks_due_warning_max')).toContainText('The maximum number of weeks this subsection can be due in is 18 weeks from the learner enrollment date.'); - expect($('.wrapper-modal-window .action-save').prop('disabled')).toBe(true); - expect($('.wrapper-modal-window .action-save').hasClass('is-disabled')).toBe(true); - // when due number of weeks is less than 1 selectRelativeWeeksSubsection('-1'); expect($('#relative_weeks_due_warning_min').css('display')).not.toBe('none'); @@ -2273,8 +2266,7 @@ describe('CourseOutlinePage', function() { expect($('.wrapper-modal-window .action-save').hasClass('is-disabled')).toBe(true); // when no validation error should show up - selectRelativeWeeksSubsection('10'); - expect($('#relative_weeks_due_warning_max').css('display')).toBe('none'); + selectRelativeWeeksSubsection('19'); expect($('#relative_weeks_due_warning_min').css('display')).toBe('none'); expect($('.wrapper-modal-window .action-save').prop('disabled')).toBe(false); expect($('.wrapper-modal-window .action-save').hasClass('is-disabled')).toBe(false); diff --git a/cms/static/js/views/modals/course_outline_modals.js b/cms/static/js/views/modals/course_outline_modals.js index 855af813a256..a10a4f23749d 100644 --- a/cms/static/js/views/modals/course_outline_modals.js +++ b/cms/static/js/views/modals/course_outline_modals.js @@ -420,16 +420,11 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', validateDueIn: function() { this.$('#relative_weeks_due_projected').hide(); - if (this.getValue() > 18){ - this.$('#relative_weeks_due_warning_max').show(); - BaseModal.prototype.disableActionButton.call(this.parent, 'save'); - } - else if (this.getValue() < 1){ - this.$('#relative_weeks_due_warning_min').show() + if (this.getValue() < 1){ + this.$('#relative_weeks_due_warning_min').show(); BaseModal.prototype.disableActionButton.call(this.parent, 'save'); } else { - this.$('#relative_weeks_due_warning_max').hide(); this.$('#relative_weeks_due_warning_min').hide(); this.showProjectedDate(); BaseModal.prototype.enableActionButton.call(this.parent, 'save'); @@ -455,7 +450,7 @@ define(['jquery', 'backbone', 'underscore', 'gettext', 'js/views/baseview', // Grab all the subsections, map them to their block_ids, then return as an Array var subsectionIds = $('.outline-subsection').map(function(){return this.id;}).get() var relative_weeks_due = null; - if (this.getValue() < 19 && this.getValue() > 0 && $('#grading_type').val() !== 'notgraded') { + if (this.getValue() > 0 && $('#grading_type').val() !== 'notgraded') { relative_weeks_due = this.getValue() } window.analytics.track('edx.bi.studio.relative_date.saved', { diff --git a/cms/templates/js/self-paced-due-date-editor.underscore b/cms/templates/js/self-paced-due-date-editor.underscore index ceaf5e6873ce..086471aa4ba6 100644 --- a/cms/templates/js/self-paced-due-date-editor.underscore +++ b/cms/templates/js/self-paced-due-date-editor.underscore @@ -4,7 +4,7 @@ <!-- Translators: Please use a generic pluralization that makes sense in most contexts for the second half of the sentence "weeks from learner enrollment date" which is preceded by "Due in: [input entry]" --> <label for="due_in"><%- gettext('Due in:') %></label> <input type="number" id="due_in" name="due_in" value="" - placeholder="" autocomplete="off" min="1" max="18" style="width:20%"/> + placeholder="" autocomplete="off" min="1" style="width:20%"/> <%- gettext('weeks from learner enrollment date')%> </li> </ul> @@ -19,10 +19,6 @@ %> </div> - <div id="relative_weeks_due_warning_max" class="message-status error"> - <%- gettext('The maximum number of weeks this subsection can be due in is 18 weeks from the learner enrollment date.') %> - </div> - <div id="relative_weeks_due_warning_min" class="message-status error"> <%- gettext('The minimum number of weeks this subsection can be due in is 1 week from the learner enrollment date.') %> </div> From 837e9edc78804811a4d4cb25e3f9a40c4854a156 Mon Sep 17 00:00:00 2001 From: Piotr Surowiec <piotr@surowiec.it> Date: Thu, 4 May 2023 16:01:10 +0200 Subject: [PATCH 14/19] feat: add Waffle Flag to disable resetting self-paced deadlines by learners (cherry picked from commit 025897146fe7500d588dd5aded1a4552e8a9743a) --- .../features/course_experience/__init__.py | 10 +++++ .../api/v1/tests/test_views.py | 38 ++++++++++++++----- openedx/features/course_experience/utils.py | 10 ++++- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index 52b870033a26..df8179623593 100644 --- a/openedx/features/course_experience/__init__.py +++ b/openedx/features/course_experience/__init__.py @@ -50,6 +50,16 @@ # .. toggle_tickets: https://openedx.atlassian.net/browse/AA-27 RELATIVE_DATES_FLAG = CourseWaffleFlag(f'{WAFFLE_FLAG_NAMESPACE}.relative_dates', __name__) # lint-amnesty, pylint: disable=toggle-missing-annotation +# .. toggle_name: course_experience.relative_dates_disable_reset +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Waffle flag to disable resetting deadlines by learners in self-paced courses. The 'Dates' tab +# will no longer show a banner about missed deadlines. The deadlines banner will also be hidden on unit pages. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2023-04-27 +# .. toggle_warning: For this toggle to have an effect, the RELATIVE_DATES_FLAG toggle must be enabled, too. +RELATIVE_DATES_DISABLE_RESET_FLAG = CourseWaffleFlag(f'{WAFFLE_FLAG_NAMESPACE}.relative_dates_disable_reset', __name__) + # .. toggle_name: course_experience.calendar_sync # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False diff --git a/openedx/features/course_experience/api/v1/tests/test_views.py b/openedx/features/course_experience/api/v1/tests/test_views.py index f67768c38f76..afc671dbf91f 100644 --- a/openedx/features/course_experience/api/v1/tests/test_views.py +++ b/openedx/features/course_experience/api/v1/tests/test_views.py @@ -2,17 +2,19 @@ Tests for reset deadlines endpoint. """ import datetime -import ddt +import ddt from django.urls import reverse from django.utils import timezone +from edx_toggles.toggles.testutils import override_waffle_flag from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.util.testing import EventTestMixin -from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin from lms.djangoapps.course_home_api.tests.utils import BaseCourseHomeTests +from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin from openedx.core.djangoapps.schedules.models import Schedule +from openedx.features.course_experience import RELATIVE_DATES_DISABLE_RESET_FLAG, RELATIVE_DATES_FLAG from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order @@ -25,17 +27,22 @@ def setUp(self): # pylint: disable=arguments-differ # Need to supply tracker name for the EventTestMixin. Also, EventTestMixin needs to come # first in class inheritance so the setUp call here appropriately works super().setUp('openedx.features.course_experience.api.v1.views.tracker') + self.course = CourseFactory.create(self_paced=True, start=timezone.now() - datetime.timedelta(days=1000)) def test_reset_deadlines(self): - CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) + enrollment = CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) + enrollment.schedule.start_date = timezone.now() - datetime.timedelta(days=100) + enrollment.schedule.save() # Test body with incorrect body param (course_key is required) response = self.client.post(reverse('course-experience-reset-course-deadlines'), {'course': self.course.id}) assert response.status_code == 400 + assert enrollment.schedule == Schedule.objects.get(id=enrollment.schedule.id) self.assert_no_events_were_emitted() # Test correct post body response = self.client.post(reverse('course-experience-reset-course-deadlines'), {'course_key': self.course.id}) assert response.status_code == 200 + assert enrollment.schedule.start_date < Schedule.objects.get(id=enrollment.schedule.id).start_date self.assert_event_emitted( 'edx.ui.lms.reset_deadlines.clicked', courserun_key=str(self.course.id), @@ -45,33 +52,44 @@ def test_reset_deadlines(self): user_id=self.user.id, ) + @override_waffle_flag(RELATIVE_DATES_FLAG, active=True) + @override_waffle_flag(RELATIVE_DATES_DISABLE_RESET_FLAG, active=True) + def test_reset_deadlines_disabled(self): + enrollment = CourseEnrollment.enroll(self.user, self.course.id, CourseMode.VERIFIED) + enrollment.schedule.start_date = timezone.now() - datetime.timedelta(days=100) + enrollment.schedule.save() + + response = self.client.post(reverse('course-experience-reset-course-deadlines'), {'course_key': self.course.id}) + assert response.status_code == 200 + assert enrollment.schedule == Schedule.objects.get(id=enrollment.schedule.id) + self.assert_no_events_were_emitted() + def test_reset_deadlines_with_masquerade(self): """ Staff users should be able to masquerade as a learner and reset the learner's schedule """ - course = CourseFactory.create(self_paced=True, start=timezone.now() - datetime.timedelta(days=1)) student_username = self.user.username student_user_id = self.user.id - student_enrollment = CourseEnrollment.enroll(self.user, course.id) + student_enrollment = CourseEnrollment.enroll(self.user, self.course.id) student_enrollment.schedule.start_date = timezone.now() - datetime.timedelta(days=100) student_enrollment.schedule.save() - staff_enrollment = CourseEnrollment.enroll(self.staff_user, course.id) + staff_enrollment = CourseEnrollment.enroll(self.staff_user, self.course.id) staff_enrollment.schedule.start_date = timezone.now() - datetime.timedelta(days=30) staff_enrollment.schedule.save() self.switch_to_staff() - self.update_masquerade(course=course, username=student_username) + self.update_masquerade(course=self.course, username=student_username) - self.client.post(reverse('course-experience-reset-course-deadlines'), {'course_key': course.id}) + self.client.post(reverse('course-experience-reset-course-deadlines'), {'course_key': self.course.id}) updated_schedule = Schedule.objects.get(id=student_enrollment.schedule.id) assert updated_schedule.start_date.date() == datetime.datetime.today().date() updated_staff_schedule = Schedule.objects.get(id=staff_enrollment.schedule.id) assert updated_staff_schedule.start_date == staff_enrollment.schedule.start_date self.assert_event_emitted( 'edx.ui.lms.reset_deadlines.clicked', - courserun_key=str(course.id), + courserun_key=str(self.course.id), is_masquerading=True, is_staff=False, - org_key=course.org, + org_key=self.course.org, user_id=student_user_id, ) diff --git a/openedx/features/course_experience/utils.py b/openedx/features/course_experience/utils.py index 451d769eeade..d58b54f6139f 100644 --- a/openedx/features/course_experience/utils.py +++ b/openedx/features/course_experience/utils.py @@ -9,7 +9,7 @@ from lms.djangoapps.course_blocks.api import get_course_blocks from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.cache_utils import request_cached -from openedx.features.course_experience import RELATIVE_DATES_FLAG +from openedx.features.course_experience import RELATIVE_DATES_DISABLE_RESET_FLAG, RELATIVE_DATES_FLAG from common.djangoapps.student.models import CourseEnrollment from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order @@ -155,6 +155,14 @@ def dates_banner_should_display(course_key, user): if not RELATIVE_DATES_FLAG.is_enabled(course_key): return False, False + if RELATIVE_DATES_DISABLE_RESET_FLAG.is_enabled(course_key): + # The `missed_deadlines` value is ignored by `reset_course_deadlines` views. Instead, they check the value of + # `missed_gated_content` to determine if learners can reset the deadlines by themselves. + # We could have added this logic directly to `reset_self_paced_schedule`, but this function is used in other + # places (e.g., when an enrollment mode is changed). We want this flag to affect only the use case when + # learners try to reset their deadlines. + return False, True + course_overview = CourseOverview.objects.get(id=str(course_key)) # Only display the banner for self-paced courses From 012aa22993947a1be089bd012323196738717dba Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee <kaustav@opencraft.com> Date: Tue, 9 May 2023 10:30:07 +0530 Subject: [PATCH 15/19] feat: tpa automatic logout (#538) (cherry picked from commit 2a5f33a5f06b96ada78efe5c4dca4af210b9d260) (cherry picked from commit 0e9cd01c60c184414dc914408dbc9d7ebc34a552) --- cms/envs/common.py | 14 ++++++++++ lms/envs/common.py | 13 +++++++++ .../djangoapps/user_authn/views/logout.py | 12 +++++++++ .../user_authn/views/tests/test_logout.py | 27 +++++++++++++++++++ 4 files changed, 66 insertions(+) diff --git a/cms/envs/common.py b/cms/envs/common.py index 5ede0b2acb94..cf69ac5f1c77 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -2268,6 +2268,20 @@ # Maximum of 6 retries before giving up. SOFTWARE_SECURE_RETRY_MAX_ATTEMPTS = 6 + +# .. toggle_name: TPA_AUTOMATIC_LOGOUT_ENABLED +# .. toggle_implementation: DjangoSetting +# .. toggle_default: False +# .. toggle_description: Redirect the user to the TPA logout URL if this flag is enabled, the +# TPA logout URL is configured, and the user logs in through TPA +# .. toggle_use_cases: open_edx +# .. toggle_warning: Enabling this toggle skips rendering logout.html, which is used to log the user out +# from the different IDAs. To ensure the user is logged out of all the IDAs be sure to redirect +# back to <LMS>/logout after logging out of the TPA. +# .. toggle_creation_date: 2023-05-07 +# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32193 +TPA_AUTOMATIC_LOGOUT_ENABLED = False + ############## DJANGO-USER-TASKS ############## # How long until database records about the outcome of a task and its artifacts get deleted? diff --git a/lms/envs/common.py b/lms/envs/common.py index 2b1f8ce22e35..23c335cee154 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1235,6 +1235,19 @@ TPA_PROVIDER_BURST_THROTTLE = '10/min' TPA_PROVIDER_SUSTAINED_THROTTLE = '50/hr' +# .. toggle_name: TPA_AUTOMATIC_LOGOUT_ENABLED +# .. toggle_implementation: DjangoSetting +# .. toggle_default: False +# .. toggle_description: Redirect the user to the TPA logout URL if this flag is enabled, the +# TPA logout URL is configured, and the user logs in through TPA. +# .. toggle_use_cases: open_edx +# .. toggle_warning: Enabling this toggle skips rendering logout.html, which is used to log the user out +# from the different IDAs. To ensure the user is logged out of all the IDAs be sure to redirect +# back to <LMS>/logout after logging out of the TPA. +# .. toggle_creation_date: 2023-05-07 +# .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32193 +TPA_AUTOMATIC_LOGOUT_ENABLED = False + ################################## TEMPLATE CONFIGURATION ##################################### # Mako templating import tempfile # pylint: disable=wrong-import-position,wrong-import-order diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index c00fa2a5dada..1fd9a3e47e8c 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -8,6 +8,7 @@ import bleach from django.conf import settings from django.contrib.auth import logout +from django.shortcuts import redirect from django.utils.http import urlencode from django.views.generic import TemplateView from oauth2_provider.models import Application @@ -83,6 +84,17 @@ def dispatch(self, request, *args, **kwargs): delete_logged_in_cookies(response) mark_user_change_as_expected(None) + + # Redirect to tpa_logout_url if TPA_AUTOMATIC_LOGOUT_ENABLED is set to True and if + # tpa_logout_url is configured. + # + # NOTE: This step skips rendering logout.html, which is used to log the user out from the + # different IDAs. To ensure the user is logged out of all the IDAs be sure to redirect + # back to <LMS>/logout after logging out of the TPA. + if settings.TPA_AUTOMATIC_LOGOUT_ENABLED: + if self.tpa_logout_url: + return redirect(self.tpa_logout_url) + return response def _build_logout_url(self, url): diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py index 139491d03d99..7d10fe1021ef 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py @@ -196,6 +196,33 @@ def test_learner_portal_logout_having_idp_logout_url(self): } self.assertDictContainsSubset(expected, response.context_data) + @mock.patch('django.conf.settings.TPA_AUTOMATIC_LOGOUT_ENABLED', True) + def test_automatic_tpa_logout_url_redirect(self): + """ + Test user automatically redirected to tpa logout_url + when TPA_AUTOMATIC_LOGOUT is set to True. + """ + idp_logout_url = 'http://mock-idp.com/logout' + client = self._create_oauth_client() + + with mock.patch( + 'openedx.core.djangoapps.user_authn.views.logout.tpa_pipeline.get_idp_logout_url_from_running_pipeline' + ) as mock_idp_logout_url: + mock_idp_logout_url.return_value = idp_logout_url + self._authenticate_with_oauth(client) + response = self.client.get(reverse('logout')) + assert response.status_code == 302 + assert response.url == idp_logout_url + + @mock.patch('django.conf.settings.TPA_AUTOMATIC_LOGOUT_ENABLED', True) + def test_no_automatic_tpa_logout_without_logout_url(self): + """ + Test user is NOT automatically redirected when tpa logout_url is not set + even if TPA_AUTOMATIC_LOGOUT is set to True. + """ + client = self._create_oauth_client() + self._assert_session_logged_out(client) + @ddt.data( ('%22%3E%3Cscript%3Ealert(%27xss%27)%3C/script%3E', 'edx.org'), ) From 8940cb3830c8b629a6e761d47549f2dccb9b13da Mon Sep 17 00:00:00 2001 From: Pooja Kulkarni <13742492+pkulkark@users.noreply.github.com> Date: Thu, 18 May 2023 15:02:57 -0400 Subject: [PATCH 16/19] feat: Extend settings handler to be accessible via api (#533) (cherry picked from commit bca8d34144f16416a4a886464d38a800f81ed925) --- .../v0/tests/test_details_settings.py | 74 +++++++++++ .../contentstore/rest_api/v0/urls.py | 13 +- .../rest_api/v0/views/__init__.py | 1 + .../rest_api/v0/views/details_settings.py | 69 ++++++++++ cms/djangoapps/contentstore/views/course.py | 119 +++++++++--------- common/djangoapps/util/json_request.py | 12 +- 6 files changed, 229 insertions(+), 59 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v0/tests/test_details_settings.py create mode 100644 cms/djangoapps/contentstore/rest_api/v0/views/details_settings.py diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_details_settings.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_details_settings.py new file mode 100644 index 000000000000..a6e2d6b19aea --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_details_settings.py @@ -0,0 +1,74 @@ +""" +Tests for the course advanced settings API. +""" +import json + +import ddt +from django.urls import reverse +from rest_framework import status + +from cms.djangoapps.contentstore.tests.utils import CourseTestCase + + +@ddt.ddt +class CourseDetailsSettingViewTest(CourseTestCase): + """ + Tests for DetailsSettings API View. + """ + + def setUp(self): + super().setUp() + self.url = reverse( + "cms.djangoapps.contentstore:v0:course_details_settings", + kwargs={"course_id": self.course.id}, + ) + + def get_and_check_developer_response(self, response): + """ + Make basic asserting about the presence of an error response, and return the developer response. + """ + content = json.loads(response.content.decode("utf-8")) + assert "developer_message" in content + return content["developer_message"] + + def test_permissions_unauthenticated(self): + """ + Test that an error is returned in the absence of auth credentials. + """ + self.client.logout() + response = self.client.get(self.url) + error = self.get_and_check_developer_response(response) + assert error == "Authentication credentials were not provided." + + def test_permissions_unauthorized(self): + """ + Test that an error is returned if the user is unauthorised. + """ + client, _ = self.create_non_staff_authed_user_client() + response = client.get(self.url) + error = self.get_and_check_developer_response(response) + assert error == "You do not have permission to perform this action." + + def test_get_course_details(self): + """ + Test for get response + """ + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + + def test_patch_course_details(self): + """ + Test for patch response + """ + data = { + "start_date": "2030-01-01T00:00:00Z", + "end_date": "2030-01-31T00:00:00Z", + "enrollment_start": "2029-12-01T00:00:00Z", + "enrollment_end": "2030-01-01T00:00:00Z", + "course_title": "Test Course", + "short_description": "This is a test course", + "overview": "This course is for testing purposes", + "intro_video": None + } + response = self.client.patch(self.url, data, content_type='application/json') + self.assertEqual(response.status_code, status.HTTP_200_OK) diff --git a/cms/djangoapps/contentstore/rest_api/v0/urls.py b/cms/djangoapps/contentstore/rest_api/v0/urls.py index eb435ef338d8..d2db1471868b 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v0/urls.py @@ -3,7 +3,13 @@ from django.urls import re_path from openedx.core.constants import COURSE_ID_PATTERN -from .views import AdvancedCourseSettingsView, CourseTabSettingsView, CourseTabListView, CourseTabReorderView +from .views import ( + AdvancedCourseSettingsView, + CourseDetailsSettingsView, + CourseTabSettingsView, + CourseTabListView, + CourseTabReorderView +) app_name = "v0" @@ -28,4 +34,9 @@ CourseTabReorderView.as_view(), name="course_tab_reorder", ), + re_path( + fr"^details_settings/{COURSE_ID_PATTERN}$", + CourseDetailsSettingsView.as_view(), + name="course_details_settings", + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v0/views/__init__.py index 91f4388cd8db..1cc7a9ca249f 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/__init__.py @@ -2,4 +2,5 @@ Views for v0 contentstore API. """ from .advanced_settings import AdvancedCourseSettingsView +from .details_settings import CourseDetailsSettingsView from .tabs import CourseTabSettingsView, CourseTabListView, CourseTabReorderView diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/details_settings.py b/cms/djangoapps/contentstore/rest_api/v0/views/details_settings.py new file mode 100644 index 000000000000..3d2392c9d132 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v0/views/details_settings.py @@ -0,0 +1,69 @@ +""" API Views for course details settings """ + +import edx_api_doc_tools as apidocs +from opaque_keys.edx.keys import CourseKey +from rest_framework.request import Request +from rest_framework.views import APIView +from xmodule.modulestore.django import modulestore + +from cms.djangoapps.models.settings.encoder import CourseSettingsEncoder +from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access +from common.djangoapps.util.json_request import JsonResponse, expect_json +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, verify_course_exists, view_auth_classes +from openedx.core.djangoapps.models.course_details import CourseDetails + +from ....views.course import update_course_details_settings + + +@view_auth_classes(is_authenticated=True) +@expect_json +class CourseDetailsSettingsView(DeveloperErrorViewMixin, APIView): + """ + View for getting and setting the details settings for a course. + """ + + @apidocs.schema( + parameters=[ + apidocs.string_parameter("course_id", apidocs.ParameterLocation.PATH, description="Course ID"), + ], + responses={ + 401: "The requester is not authenticated.", + 403: "The requester cannot access the specified course.", + 404: "The requested course does not exist.", + }, + ) + @verify_course_exists() + def get(self, request: Request, course_id: str): + """ + Get an object containing all the details settings in a course. + """ + course_key = CourseKey.from_string(course_id) + if not has_studio_read_access(request.user, course_key): + self.permission_denied(request) + course_details = CourseDetails.fetch(course_key) + return JsonResponse( + course_details, + # encoder serializes dates, old locations, and instances + encoder=CourseSettingsEncoder + ) + + @apidocs.schema( + parameters=[ + apidocs.string_parameter("course_id", apidocs.ParameterLocation.PATH, description="Course ID"), + ], + responses={ + 401: "The requester is not authenticated.", + 403: "The requester cannot access the specified course.", + 404: "The requested course does not exist.", + }, + ) + @verify_course_exists() + def patch(self, request: Request, course_id: str): + """ + Update a course's details settings. + """ + course_key = CourseKey.from_string(course_id) + if not has_studio_write_access(request.user, course_key): + self.permission_denied(request) + course_block = modulestore().get_course(course_key) + return update_course_details_settings(course_key, course_block, request) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index a755d930bfad..a3714195296c 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1271,63 +1271,70 @@ def settings_handler(request, course_key_string): # lint-amnesty, pylint: disab ) # For every other possible method type submitted by the caller... else: - # if pre-requisite course feature is enabled set pre-requisite course - if is_prerequisite_courses_enabled(): - prerequisite_course_keys = request.json.get('pre_requisite_courses', []) - if prerequisite_course_keys: - if not all(is_valid_course_key(course_key) for course_key in prerequisite_course_keys): - return JsonResponseBadRequest({"error": _("Invalid prerequisite course key")}) - set_prerequisite_courses(course_key, prerequisite_course_keys) - else: - # None is chosen, so remove the course prerequisites - course_milestones = milestones_api.get_course_milestones( - course_key=course_key, - relationship="requires", - ) - for milestone in course_milestones: - entrance_exam_namespace = generate_milestone_namespace( - get_namespace_choices().get('ENTRANCE_EXAM'), - course_key - ) - if milestone["namespace"] != entrance_exam_namespace: - remove_prerequisite_course(course_key, milestone) - - # If the entrance exams feature has been enabled, we'll need to check for some - # feature-specific settings and handle them accordingly - # We have to be careful that we're only executing the following logic if we actually - # need to create or delete an entrance exam from the specified course - if core_toggles.ENTRANCE_EXAMS.is_enabled(): - course_entrance_exam_present = course_block.entrance_exam_enabled - entrance_exam_enabled = request.json.get('entrance_exam_enabled', '') == 'true' - ee_min_score_pct = request.json.get('entrance_exam_minimum_score_pct', None) - # If the entrance exam box on the settings screen has been checked... - if entrance_exam_enabled: - # Load the default minimum score threshold from settings, then try to override it - entrance_exam_minimum_score_pct = float(settings.ENTRANCE_EXAM_MIN_SCORE_PCT) - if ee_min_score_pct: - entrance_exam_minimum_score_pct = float(ee_min_score_pct) - if entrance_exam_minimum_score_pct.is_integer(): - entrance_exam_minimum_score_pct = entrance_exam_minimum_score_pct / 100 - # If there's already an entrance exam defined, we'll update the existing one - if course_entrance_exam_present: - exam_data = { - 'entrance_exam_minimum_score_pct': entrance_exam_minimum_score_pct - } - update_entrance_exam(request, course_key, exam_data) - # If there's no entrance exam defined, we'll create a new one - else: - create_entrance_exam(request, course_key, entrance_exam_minimum_score_pct) - - # If the entrance exam box on the settings screen has been unchecked, - # and the course has an entrance exam attached... - elif not entrance_exam_enabled and course_entrance_exam_present: - delete_entrance_exam(request, course_key) - - # Perform the normal update workflow for the CourseDetails model - return JsonResponse( - CourseDetails.update_from_json(course_key, request.json, request.user), - encoder=CourseSettingsEncoder + return update_course_details_settings(course_key, course_block, request) + + +def update_course_details_settings(course_key, course_block: CourseBlock, request): + """ + Helper function to update course details settings from API data + """ + # if pre-requisite course feature is enabled set pre-requisite course + if is_prerequisite_courses_enabled(): + prerequisite_course_keys = request.json.get('pre_requisite_courses', []) + if prerequisite_course_keys: + if not all(is_valid_course_key(course_key) for course_key in prerequisite_course_keys): + return JsonResponseBadRequest({"error": _("Invalid prerequisite course key")}) + set_prerequisite_courses(course_key, prerequisite_course_keys) + else: + # None is chosen, so remove the course prerequisites + course_milestones = milestones_api.get_course_milestones( + course_key=course_key, + relationship="requires", + ) + for milestone in course_milestones: + entrance_exam_namespace = generate_milestone_namespace( + get_namespace_choices().get('ENTRANCE_EXAM'), + course_key ) + if milestone["namespace"] != entrance_exam_namespace: + remove_prerequisite_course(course_key, milestone) + + # If the entrance exams feature has been enabled, we'll need to check for some + # feature-specific settings and handle them accordingly + # We have to be careful that we're only executing the following logic if we actually + # need to create or delete an entrance exam from the specified course + if core_toggles.ENTRANCE_EXAMS.is_enabled(): + course_entrance_exam_present = course_block.entrance_exam_enabled + entrance_exam_enabled = request.json.get('entrance_exam_enabled', '') == 'true' + ee_min_score_pct = request.json.get('entrance_exam_minimum_score_pct', None) + # If the entrance exam box on the settings screen has been checked... + if entrance_exam_enabled: + # Load the default minimum score threshold from settings, then try to override it + entrance_exam_minimum_score_pct = float(settings.ENTRANCE_EXAM_MIN_SCORE_PCT) + if ee_min_score_pct: + entrance_exam_minimum_score_pct = float(ee_min_score_pct) + if entrance_exam_minimum_score_pct.is_integer(): + entrance_exam_minimum_score_pct = entrance_exam_minimum_score_pct / 100 + # If there's already an entrance exam defined, we'll update the existing one + if course_entrance_exam_present: + exam_data = { + 'entrance_exam_minimum_score_pct': entrance_exam_minimum_score_pct + } + update_entrance_exam(request, course_key, exam_data) + # If there's no entrance exam defined, we'll create a new one + else: + create_entrance_exam(request, course_key, entrance_exam_minimum_score_pct) + + # If the entrance exam box on the settings screen has been unchecked, + # and the course has an entrance exam attached... + elif not entrance_exam_enabled and course_entrance_exam_present: + delete_entrance_exam(request, course_key) + + # Perform the normal update workflow for the CourseDetails model + return JsonResponse( + CourseDetails.update_from_json(course_key, request.json, request.user), + encoder=CourseSettingsEncoder + ) @login_required diff --git a/common/djangoapps/util/json_request.py b/common/djangoapps/util/json_request.py index 3c9646b130e5..d71620020771 100644 --- a/common/djangoapps/util/json_request.py +++ b/common/djangoapps/util/json_request.py @@ -8,6 +8,8 @@ from django.core.serializers.json import DjangoJSONEncoder from django.db.models.query import QuerySet from django.http import HttpResponse, HttpResponseBadRequest +from django.utils.decorators import method_decorator +from django.views import View class EDXJSONEncoder(DjangoJSONEncoder): @@ -40,7 +42,6 @@ def expect_json(view_function): CONTENT_TYPE is application/json, parses the json dict from request.body, and updates request.POST with the contents. """ - @wraps(view_function) def parse_json_into_request(request, *args, **kwargs): # cdodge: fix postback errors in CMS. The POST 'content-type' header can include additional information # e.g. 'charset', so we can't do a direct string compare @@ -54,7 +55,14 @@ def parse_json_into_request(request, *args, **kwargs): return view_function(request, *args, **kwargs) - return parse_json_into_request + if isinstance(view_function, type) and issubclass(view_function, View): + view_function.dispatch = method_decorator(expect_json)(view_function.dispatch) + return view_function + else: + @wraps(view_function) + def wrapper(request, *args, **kwargs): + return parse_json_into_request(request, *args, **kwargs) + return wrapper class JsonResponse(HttpResponse): From 0c831dc390f63ea9259668ec400df9c4bccd95c8 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti <kshitij@sobti.in> Date: Wed, 24 May 2023 13:58:55 +0530 Subject: [PATCH 17/19] temp: Add configuration option to redirect to external site when TAP account is unlinked (cherry picked from commit e83a8c8f82849644cf95534cde3fe149e4f11916) --- lms/static/js/student_account/views/LoginView.js | 3 +++ openedx/core/djangoapps/user_authn/views/login.py | 10 ++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js index 8357ac6fa885..3fe530db907a 100644 --- a/lms/static/js/student_account/views/LoginView.js +++ b/lms/static/js/student_account/views/LoginView.js @@ -213,6 +213,7 @@ saveError: function(error) { var errorCode; var msg; + var redirectURL; if (error.status === 0) { msg = gettext('An error has occurred. Check your Internet connection and try again.'); } else if (error.status === 500) { @@ -242,6 +243,7 @@ } else if (error.responseJSON !== undefined) { msg = error.responseJSON.value; errorCode = error.responseJSON.error_code; + redirectURL = error.responseJSON.redirect_url; } else { msg = gettext('An unexpected error has occurred.'); } @@ -263,6 +265,7 @@ this.clearFormErrors(); this.renderThirdPartyAuthWarning(); } + window.location.href = redirectURL; } else { this.renderErrors(this.defaultFormErrorsTitle, this.errors); } diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 79cb78a2727c..5c5fe63e3ef6 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -76,7 +76,7 @@ def _do_third_party_auth(request): try: return pipeline.get_authenticated_user(requested_provider, username, third_party_uid) - except USER_MODEL.DoesNotExist: + except USER_MODEL.DoesNotExist as err: AUDIT_LOG.info( "Login failed - user with username {username} has no social auth " "with backend_name {backend_name}".format( @@ -98,7 +98,13 @@ def _do_third_party_auth(request): ) ) - raise AuthFailedError(message, error_code='third-party-auth-with-no-linked-account') # lint-amnesty, pylint: disable=raise-missing-from + redirect_url = configuration_helpers.get_value('OC_REDIRECT_ON_TPA_UNLINKED_ACCOUNT', None) + + raise AuthFailedError( + message, + error_code='third-party-auth-with-no-linked-account', + redirect_url=redirect_url + ) from err def _get_user_by_email(email): From 3ca85eac1228f281212b306f3e7ddfded96f7dd8 Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee <kaustav@opencraft.com> Date: Mon, 12 Jun 2023 13:37:20 +0530 Subject: [PATCH 18/19] feat: default grade designations configurable from settings (#541) (cherry picked from commit 7a7af8cdc16416ec8e3289f9482e734b58145b0e) (cherry picked from commit 94754317c3f8709b7cd8980cbf2f9e5181494fb0) --- cms/djangoapps/contentstore/views/course.py | 1 + cms/envs/common.py | 8 ++++++++ cms/static/js/factories/settings_graders.js | 5 +++-- cms/static/js/views/settings/grading.js | 9 ++++++--- cms/static/sass/views/_settings.scss | 10 +++++----- cms/templates/settings_graders.html | 3 ++- 6 files changed, 25 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index a3714195296c..17ce3eea18b1 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1365,6 +1365,7 @@ def grading_handler(request, course_key_string, grader_index=None): 'grading_url': reverse_course_url('grading_handler', course_key), 'is_credit_course': is_credit_course(course_key), 'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_block.id), + 'default_grade_designations': settings.DEFAULT_GRADE_DESIGNATIONS }) elif 'application/json' in request.META.get('HTTP_ACCEPT', ''): if request.method == 'GET': diff --git a/cms/envs/common.py b/cms/envs/common.py index cf69ac5f1c77..8bce01ff1c52 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -2338,6 +2338,14 @@ # Rate limit for regrading tasks that a grading policy change can kick off POLICY_CHANGE_TASK_RATE_LIMIT = '900/h' +# .. setting_name: DEFAULT_GRADE_DESIGNATIONS +# .. setting_default: ['A', 'B', 'C', 'D'] +# .. setting_description: The default 'pass' grade cutoff designations to be used. The failure grade +# is always 'F' and should not be included in this list. +# .. setting_warning: The DEFAULT_GRADE_DESIGNATIONS list must have more than one designation, +# or else ['A', 'B', 'C', 'D'] will be used as the default grade designations. +DEFAULT_GRADE_DESIGNATIONS = ['A', 'B', 'C', 'D'] + ############## Settings for CourseGraph ############################ # .. setting_name: COURSEGRAPH_JOB_QUEUE diff --git a/cms/static/js/factories/settings_graders.js b/cms/static/js/factories/settings_graders.js index 2be17111e0e2..b59961acbd42 100644 --- a/cms/static/js/factories/settings_graders.js +++ b/cms/static/js/factories/settings_graders.js @@ -2,7 +2,7 @@ define([ 'jquery', 'js/views/settings/grading', 'js/models/settings/course_grading_policy' ], function($, GradingView, CourseGradingPolicyModel) { 'use strict'; - return function(courseDetails, gradingUrl) { + return function(courseDetails, gradingUrl, gradeDesignations) { var model, editor; $('form :input') @@ -17,7 +17,8 @@ define([ model.urlRoot = gradingUrl; editor = new GradingView({ el: $('.settings-grading'), - model: model + model: model, + gradeDesignations: gradeDesignations }); editor.render(); }; diff --git a/cms/static/js/views/settings/grading.js b/cms/static/js/views/settings/grading.js index 4320204035ac..684545cc730e 100644 --- a/cms/static/js/views/settings/grading.js +++ b/cms/static/js/views/settings/grading.js @@ -24,7 +24,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { 'focus :input': 'inputFocus', 'blur :input': 'inputUnfocus' }, - initialize: function() { + initialize: function(options) { // load template for grading view var self = this; this.template = HtmlUtils.template( @@ -34,6 +34,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { $('#course_grade_cutoff-tpl').text() ); this.setupCutoffs(); + this.setupGradeDesignations(options.gradeDesignations); this.listenTo(this.model, 'invalid', this.handleValidationError); this.listenTo(this.model, 'change', this.showNotificationBar); @@ -297,7 +298,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { addNewGrade: function(e) { e.preventDefault(); var gradeLength = this.descendingCutoffs.length; // cutoffs doesn't include fail/f so this is only the passing grades - if (gradeLength > 3) { + if (gradeLength > this.GRADES.length - 1) { // TODO shouldn't we disable the button return; } @@ -377,7 +378,9 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { this.descendingCutoffs = _.sortBy(this.descendingCutoffs, function(gradeEle) { return -gradeEle.cutoff; }); }, - revertView: function() { + setupGradeDesignations: function(gradeDesignations) { + if (Array.isArray(gradeDesignations) && gradeDesignations.length > 1) { this.GRADES = gradeDesignations; } + },revertView: function() { var self = this; this.model.fetch({ success: function() { diff --git a/cms/static/sass/views/_settings.scss b/cms/static/sass/views/_settings.scss index 10d2ccc3272a..a786f6f2082a 100644 --- a/cms/static/sass/views/_settings.scss +++ b/cms/static/sass/views/_settings.scss @@ -777,23 +777,23 @@ height: 17px; } - &:nth-child(1) { + &:nth-child(5n+1) { background: #4fe696; } - &:nth-child(2) { + &:nth-child(5n+2) { background: #ffdf7e; } - &:nth-child(3) { + &:nth-child(5n+3) { background: #ffb657; } - &:nth-child(4) { + &:nth-child(5n+4) { background: #ef54a1; } - &:nth-child(5), + &:nth-child(5n+5), &.bar-fail { background: #fb336c; } diff --git a/cms/templates/settings_graders.html b/cms/templates/settings_graders.html index ac7119177978..d8b7d3bc0ebd 100644 --- a/cms/templates/settings_graders.html +++ b/cms/templates/settings_graders.html @@ -34,7 +34,8 @@ SettingsGradersFactory( _.extend(${dump_js_escaped_json(course_details, cls=CourseSettingsEncoder) | n, decode.utf8}, {is_credit_course: ${is_credit_course | n, dump_js_escaped_json}}), - "${grading_url | n, js_escaped_string}" + "${grading_url | n, js_escaped_string}", + ${default_grade_designations | n, dump_js_escaped_json}, ); }); </%block> From f55297379276ddcdfd140b43bdc54d19128744d9 Mon Sep 17 00:00:00 2001 From: 0x29a <demid@opencraft.com> Date: Mon, 26 Jun 2023 11:08:26 +0200 Subject: [PATCH 19/19] fix: give superusers all studio permissions (cherry picked from commit 8ef55754f4a529cc6b784298320fcdb8b415bd83) (cherry picked from commit 8e281a9535b8317e24f8d2b3b5d840a3eb852865) --- common/djangoapps/student/roles.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 1fcd9887ea07..3749bc05035a 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -120,7 +120,7 @@ class GlobalStaff(AccessRole): The global staff role """ def has_user(self, user): - return bool(user and user.is_staff) + return bool(user and (user.is_superuser or user.is_staff)) def add_users(self, *users): for user in users: