From d26e7d93c9e3e195b7ccdb06167dd5192693eac3 Mon Sep 17 00:00:00 2001 From: farhan Date: Wed, 16 Apr 2025 10:59:50 +0500 Subject: [PATCH 01/23] chore: Initial setup of JS files and webpack config --- webpack.common.config.js | 1 + xmodule/assets/video/public/js/video_block_main.js | 9 +++++++++ xmodule/assets/video/public/js/video_storage.js | 5 +++++ xmodule/video_block/video_block.py | 4 ++-- 4 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 xmodule/assets/video/public/js/video_block_main.js create mode 100644 xmodule/assets/video/public/js/video_storage.js diff --git a/webpack.common.config.js b/webpack.common.config.js index 4eea3b5da9b6..06efdb5c6151 100644 --- a/webpack.common.config.js +++ b/webpack.common.config.js @@ -140,6 +140,7 @@ module.exports = Merge.smart({ ReactRenderer: './common/static/js/src/ReactRenderer.jsx', XModuleShim: './xmodule/js/src/xmodule.js', VerticalStudentView: './xmodule/assets/vertical/public/js/vertical_student_view.js', + VideoBlockMain: './xmodule/assets/video/public/js/video_block_main.js', commons: 'babel-polyfill' }, diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js new file mode 100644 index 000000000000..53c212d4e3c2 --- /dev/null +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -0,0 +1,9 @@ +import {test} from './video_storage'; + +console.log('In video_block_main.js file'); + +window.Video = function (runtime, element) { + 'use strict'; + console.log('In Video initialize method'); + test(); +} diff --git a/xmodule/assets/video/public/js/video_storage.js b/xmodule/assets/video/public/js/video_storage.js new file mode 100644 index 000000000000..38e4d2c812fd --- /dev/null +++ b/xmodule/assets/video/public/js/video_storage.js @@ -0,0 +1,5 @@ +console.log('In video_storage.js file'); + +export function test() { + console.log('In test'); +} diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index b0e01068fbf3..73ed4579ab83 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -246,8 +246,8 @@ def student_view(self, context): """ fragment = Fragment(self.get_html(context=context)) add_css_to_fragment(fragment, 'VideoBlockDisplay.css') - add_webpack_js_to_fragment(fragment, 'VideoBlockDisplay') - shim_xmodule_js(fragment, 'Video') + add_webpack_js_to_fragment(fragment, 'VideoBlockMain') + fragment.initialize_js('Video') return fragment def author_view(self, context): From 99e118caecfef45a9812f6f2322814ef1a276b0d Mon Sep 17 00:00:00 2001 From: farhan Date: Wed, 16 Apr 2025 16:08:25 +0500 Subject: [PATCH 02/23] chore: Add more refactored code in the main.js --- .../video/public/js/video_block_main.js | 183 +++++++++++++++++- .../assets/video/public/js/video_storage.js | 78 +++++++- 2 files changed, 253 insertions(+), 8 deletions(-) diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js index 53c212d4e3c2..ba711287b8a0 100644 --- a/xmodule/assets/video/public/js/video_block_main.js +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -1,9 +1,180 @@ -import {test} from './video_storage'; +// Import required modules and dependencies +import {VideoStorage} from './video_storage'; +// TODO: Uncomment the imports +// import { initialize } from './initialize'; // Assuming this function is imported +// import { +// FocusGrabber, +// VideoControl, +// VideoPlayPlaceholder, +// VideoPlayPauseControl, +// VideoProgressSlider, +// VideoSpeedControl, +// VideoVolumeControl, +// VideoQualityControl, +// VideoFullScreen, +// VideoCaption, +// VideoCommands, +// VideoContextMenu, +// VideoSaveStatePlugin, +// VideoEventsPlugin, +// VideoCompletionHandler, +// VideoTranscriptFeedback, +// VideoAutoAdvanceControl, +// VideoPlaySkipControl, +// VideoSkipControl, +// VideoEventsBumperPlugin, +// VideoPoster, +// VideoSocialSharing, +// VideoAccessibleMenu, +// VideoBumper, +// } from './video_modules'; // Assuming all necessary modules are grouped here + +'use strict'; console.log('In video_block_main.js file'); -window.Video = function (runtime, element) { - 'use strict'; - console.log('In Video initialize method'); - test(); -} +(function (require, $) { + // TODO: Following code needs to be reviewed, why we are not getting $ + if (!$) { + $ = window.jQuery; + } + var youtubeXhr = null; + var oldVideo = window.Video; + + window.Video = function (runtime, element) { + console.log('In Video initialize method'); + + const el = $(element).find('.video'); + const id = el.attr('id').replace(/video_/, ''); + const storage = new VideoStorage('VideoState', id); + const bumperMetadata = el.data('bumper-metadata'); + const autoAdvanceEnabled = el.data('autoadvance-enabled') === 'True'; + + const mainVideoModules = [] + // TODO: Uncomment the code + // const mainVideoModules = [ + // FocusGrabber, + // VideoControl, + // VideoPlayPlaceholder, + // VideoPlayPauseControl, + // VideoProgressSlider, + // VideoSpeedControl, + // VideoVolumeControl, + // VideoQualityControl, + // VideoFullScreen, + // VideoCaption, + // VideoCommands, + // VideoContextMenu, + // VideoSaveStatePlugin, + // VideoEventsPlugin, + // VideoCompletionHandler, + // VideoTranscriptFeedback, + // ].concat(autoAdvanceEnabled ? [VideoAutoAdvanceControl] : []); + + const bumperVideoModules = [ + // VideoControl, + // VideoPlaySkipControl, + // VideoSkipControl, + // VideoVolumeControl, + // VideoCaption, + // VideoCommands, + // VideoSaveStatePlugin, + // VideoTranscriptFeedback, + // VideoEventsBumperPlugin, + // VideoCompletionHandler, + ]; + + const state = { + el: el, + id: id, + metadata: el.data('metadata'), + storage: storage, + options: {}, + youtubeXhr: youtubeXhr, + modules: mainVideoModules, + }; + + const getBumperState = (metadata) => { + return $.extend(true, { + el: el, + id: id, + storage: storage, + options: {SaveStatePlugin: {events: ['language_menu:change']}}, + youtubeXhr: youtubeXhr, + modules: bumperVideoModules, + }, {metadata: metadata}); + }; + + const player = (innerState) => { + return () => { + _.extend(innerState.metadata, {autoplay: true, focusFirstControl: true}); + // TODO: Uncomment following initialize method calling + // initialize(innerState, element); + }; + }; + + // VideoAccessibleMenu(el, { + // storage: storage, + // saveStateUrl: state.metadata.saveStateUrl, + // }); + + // VideoSocialSharing(el); + + if (bumperMetadata) { + // VideoPoster(el, { + // poster: el.data('poster'), + // onClick: _.once(function () { + // const mainVideoPlayer = player(state); + // + // if (storage.getItem('isBumperShown')) { + // mainVideoPlayer(); + // } else { + // const bumperState = getBumperState(bumperMetadata); + // const bumper = new VideoBumper(player(bumperState), bumperState); + // + // state.bumperState = bumperState; + // + // bumper.getPromise().then(() => { + // delete state.bumperState; + // mainVideoPlayer(); + // }); + // } + // }), + // }); + } else { + // TODO: Uncomment following initialize method calling + // initialize(state, element); + } + + if (!youtubeXhr) { + youtubeXhr = state.youtubeXhr; + } + + el.data('video-player-state', state); + const onSequenceChange = () => { + if (state && state.videoPlayer) { + state.videoPlayer.destroy(); + } + $('.sequence').off('sequence:change', onSequenceChange); + }; + $('.sequence').on('sequence:change', onSequenceChange); + + // Because the 'state' object is only available inside this closure, we will also make it available to + // the caller by returning it. This is necessary so that we can test Video with Jasmine. + return state; + }; + + window.Video.clearYoutubeXhr = function () { + youtubeXhr = null; + }; + + // TODO: Uncomment following initialize related code + // window.Video.loadYouTubeIFrameAPI = initialize.prototype.loadYouTubeIFrameAPI; + + // Invoke the mock Video constructor so that the elements stored within it can be processed by the real + // `window.Video` constructor. + // TODO: Un comment following + // oldVideo(null, true); + +}()); + diff --git a/xmodule/assets/video/public/js/video_storage.js b/xmodule/assets/video/public/js/video_storage.js index 38e4d2c812fd..5262041b8778 100644 --- a/xmodule/assets/video/public/js/video_storage.js +++ b/xmodule/assets/video/public/js/video_storage.js @@ -1,5 +1,79 @@ +'use strict'; + console.log('In video_storage.js file'); -export function test() { - console.log('In test'); +/** + * Provides a convenient way to store key-value pairs. + * + * @param {string} namespace Namespace that is used to store data. + * @param {string} id Identifier for instance-specific storage. + * @return {object} VideoStorage API. + */ +export function VideoStorage(namespace = 'VideoStorage', id = Math.random().toString(36).slice(2)) { + /** + * Adds new value to the storage or rewrites existent. + * + * @param {string} name Identifier of the data. + * @param {any} value Data to store. + * @param {boolean} instanceSpecific Data with this flag will be added + * to instance specific storage. + */ + const setItem = (name, value, instanceSpecific) => { + if (name) { + if (instanceSpecific) { + window[namespace][id][name] = value; + } else { + window[namespace][name] = value; + } + } + }; + + /** + * Returns the current value associated with the given name. + * + * @param {string} name Identifier of the data. + * @param {boolean} instanceSpecific Data with this flag will be added + * to instance specific storage. + * @return {any} The current value associated with the given name. + * If the given key does not exist in the list + * associated with the object then this method must return null. + */ + const getItem = (name, instanceSpecific) => { + if (instanceSpecific) { + return window[namespace][id][name]; + } else { + return window[namespace][name]; + } + }; + + /** + * Removes the current value associated with the given name. + * + * @param {string} name Identifier of the data. + * @param {boolean} instanceSpecific Data with this flag will be added + * to instance specific storage. + */ + const removeItem = (name, instanceSpecific) => { + if (instanceSpecific) { + delete window[namespace][id][name]; + } else { + delete window[namespace][name]; + } + }; + + /** Clear the storage. */ + const clear = () => { + window[namespace] = {}; + window[namespace][id] = {}; + }; + + window[namespace] = window[namespace] || {}; + window[namespace][id] = window[namespace][id] || {}; + + return { + clear: clear, + getItem: getItem, + removeItem: removeItem, + setItem: setItem + }; } From df82bb2de710f812522048fee7def4b04be949fd Mon Sep 17 00:00:00 2001 From: farhan Date: Wed, 16 Apr 2025 17:04:05 +0500 Subject: [PATCH 03/23] chore: Adds poster.js --- xmodule/assets/video/public/js/poster.js | 67 +++++++++++++++++++ .../video/public/js/video_block_main.js | 58 +++++++++------- 2 files changed, 99 insertions(+), 26 deletions(-) create mode 100644 xmodule/assets/video/public/js/poster.js diff --git a/xmodule/assets/video/public/js/poster.js b/xmodule/assets/video/public/js/poster.js new file mode 100644 index 000000000000..857497c97619 --- /dev/null +++ b/xmodule/assets/video/public/js/poster.js @@ -0,0 +1,67 @@ +import $ from 'jquery'; // jQuery import +import _ from 'underscore'; + +'use strict'; + +/** + * VideoPoster function + * @param {jQuery} element + * @param {Object} options + */ +function VideoPoster(element, options) { + if (!(this instanceof VideoPoster)) { + return new VideoPoster(element, options); + } + + _.bindAll(this, 'onClick', 'destroy'); + this.element = element; + this.container = element.find('.video-player'); + this.options = options || {}; + this.initialize(); +} + +VideoPoster.moduleName = 'Poster'; +VideoPoster.prototype = { + template: _.template([ + '
', + '', + '
' + ].join('')), + + initialize: function () { + this.el = $(this.template({ + url: this.options.poster.url, + type: this.options.poster.type + })); + this.element.addClass('is-pre-roll'); + this.render(); + this.bindHandlers(); + }, + + bindHandlers: function () { + this.el.on('click', this.onClick); + this.element.on('destroy', this.destroy); + }, + + render: function () { + this.container.append(this.el); + }, + + onClick: function () { + if (_.isFunction(this.options.onClick)) { + this.options.onClick(); + } + this.destroy(); + }, + + destroy: function () { + this.element.off('destroy', this.destroy).removeClass('is-pre-roll'); + this.el.remove(); + }, +}; + +export {VideoPoster}; \ No newline at end of file diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js index ba711287b8a0..35cf53f7b825 100644 --- a/xmodule/assets/video/public/js/video_block_main.js +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -1,5 +1,8 @@ // Import required modules and dependencies +import $ from 'jquery'; +import _ from 'underscore'; import {VideoStorage} from './video_storage'; +import {VideoPoster} from './poster'; // TODO: Uncomment the imports // import { initialize } from './initialize'; // Assuming this function is imported // import { @@ -23,21 +26,24 @@ import {VideoStorage} from './video_storage'; // VideoPlaySkipControl, // VideoSkipControl, // VideoEventsBumperPlugin, -// VideoPoster, // VideoSocialSharing, // VideoAccessibleMenu, // VideoBumper, // } from './video_modules'; // Assuming all necessary modules are grouped here +// Stub gettext if the runtime doesn't provide it +if (typeof window.gettext === 'undefined') { + window.gettext = function (text) { + return text; + }; +} + + 'use strict'; console.log('In video_block_main.js file'); -(function (require, $) { - // TODO: Following code needs to be reviewed, why we are not getting $ - if (!$) { - $ = window.jQuery; - } +(function () { var youtubeXhr = null; var oldVideo = window.Video; @@ -121,26 +127,26 @@ console.log('In video_block_main.js file'); // VideoSocialSharing(el); if (bumperMetadata) { - // VideoPoster(el, { - // poster: el.data('poster'), - // onClick: _.once(function () { - // const mainVideoPlayer = player(state); - // - // if (storage.getItem('isBumperShown')) { - // mainVideoPlayer(); - // } else { - // const bumperState = getBumperState(bumperMetadata); - // const bumper = new VideoBumper(player(bumperState), bumperState); - // - // state.bumperState = bumperState; - // - // bumper.getPromise().then(() => { - // delete state.bumperState; - // mainVideoPlayer(); - // }); - // } - // }), - // }); + VideoPoster(el, { + poster: el.data('poster'), + onClick: _.once(function () { + const mainVideoPlayer = player(state); + + if (storage.getItem('isBumperShown')) { + mainVideoPlayer(); + } else { + const bumperState = getBumperState(bumperMetadata); + const bumper = new VideoBumper(player(bumperState), bumperState); + + state.bumperState = bumperState; + + bumper.getPromise().then(() => { + delete state.bumperState; + mainVideoPlayer(); + }); + } + }), + }); } else { // TODO: Uncomment following initialize method calling // initialize(state, element); From 1bb6642d0b4e2c18a2ab99aefccefb90447e5b5e Mon Sep 17 00:00:00 2001 From: farhan Date: Wed, 16 Apr 2025 17:32:51 +0500 Subject: [PATCH 04/23] chore: Adds video_accessible_menu --- xmodule/assets/video/public/js/poster.js | 2 + .../video/public/js/video_accessible_menu.js | 56 +++++++++++++++++++ .../video/public/js/video_block_main.js | 11 ++-- 3 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 xmodule/assets/video/public/js/video_accessible_menu.js diff --git a/xmodule/assets/video/public/js/poster.js b/xmodule/assets/video/public/js/poster.js index 857497c97619..ef059b81fa08 100644 --- a/xmodule/assets/video/public/js/poster.js +++ b/xmodule/assets/video/public/js/poster.js @@ -5,6 +5,8 @@ import _ from 'underscore'; /** * VideoPoster function + * + * @constructor * @param {jQuery} element * @param {Object} options */ diff --git a/xmodule/assets/video/public/js/video_accessible_menu.js b/xmodule/assets/video/public/js/video_accessible_menu.js new file mode 100644 index 000000000000..efcbff1cf63f --- /dev/null +++ b/xmodule/assets/video/public/js/video_accessible_menu.js @@ -0,0 +1,56 @@ +import _ from 'underscore'; + +/** + * Video Download Transcript control module. + * + * @constructor + * @param {jQuery} element + * @param {Object} options + */ +function VideoTranscriptDownloadHandler(element, options = {}) { + if (!(this instanceof VideoTranscriptDownloadHandler)) { + return new VideoTranscriptDownloadHandler(element, options); + } + + _.bindAll(this, 'clickHandler'); + + this.container = element; + this.options = options; + + if (this.container.find('.wrapper-downloads .wrapper-download-transcripts')) { + this.initialize(); + } +} + +VideoTranscriptDownloadHandler.prototype = { + // Initializes the module. + initialize() { + this.value = this.options.storage.getItem('transcript_download_format'); + this.el = this.container.find('.list-download-transcripts'); + this.el.on('click', '.btn-link', this.clickHandler); + }, + + // Event handler. We delay link clicks until the file type is set + clickHandler(event) { + event.preventDefault(); + + const fileType = $(event.target).data('value'); + const data = {transcript_download_format: fileType}; + const downloadUrl = $(event.target).attr('href'); + + $.ajax({ + url: this.options.saveStateUrl, + type: 'POST', + dataType: 'json', + data: data, + success: () => { + this.options.storage.setItem('transcript_download_format', fileType); + }, + complete: () => { + document.location.href = downloadUrl; + }, + }); + }, +}; + +export {VideoTranscriptDownloadHandler}; diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js index 35cf53f7b825..902c6e704f21 100644 --- a/xmodule/assets/video/public/js/video_block_main.js +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -3,6 +3,8 @@ import $ from 'jquery'; import _ from 'underscore'; import {VideoStorage} from './video_storage'; import {VideoPoster} from './poster'; +import {VideoTranscriptDownloadHandler} from './video_accessible_menu'; + // TODO: Uncomment the imports // import { initialize } from './initialize'; // Assuming this function is imported // import { @@ -27,7 +29,6 @@ import {VideoPoster} from './poster'; // VideoSkipControl, // VideoEventsBumperPlugin, // VideoSocialSharing, -// VideoAccessibleMenu, // VideoBumper, // } from './video_modules'; // Assuming all necessary modules are grouped here @@ -119,10 +120,10 @@ console.log('In video_block_main.js file'); }; }; - // VideoAccessibleMenu(el, { - // storage: storage, - // saveStateUrl: state.metadata.saveStateUrl, - // }); + VideoTranscriptDownloadHandler(el, { + storage: storage, + saveStateUrl: state.metadata.saveStateUrl, + }); // VideoSocialSharing(el); From 1b9671d2c83c78d66eacbca329a1313d064b0234 Mon Sep 17 00:00:00 2001 From: Hassan Raza Date: Wed, 9 Apr 2025 14:34:07 +0500 Subject: [PATCH 05/23] feat: Group ora staff notifications (#36440) * feat: Group ora staff notifications * fix: Revert default change for ora staff notifications * fix: update edx-ora2 version --- .../djangoapps/notifications/base_notification.py | 4 +++- .../notifications/grouping_notifications.py | 15 +++++++++++++++ requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 6 files changed, 22 insertions(+), 5 deletions(-) diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index ff2cc38c84a3..17ec8cda45d7 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -201,8 +201,10 @@ 'push': False, 'email_cadence': EmailCadence.DAILY, 'non_editable': [], - 'content_template': _('<{p}>You have a new open response submission awaiting for review for ' + 'content_template': _('<{p}>You have a new open response submission awaiting review for ' '<{strong}>{ora_name}'), + 'grouped_content_template': _('<{p}>You have multiple submissions awaiting review for ' + '<{strong}>{ora_name}'), 'content_context': { 'ora_name': 'Name of ORA in course', }, diff --git a/openedx/core/djangoapps/notifications/grouping_notifications.py b/openedx/core/djangoapps/notifications/grouping_notifications.py index 0e84ea3f109a..a1da88683fa0 100644 --- a/openedx/core/djangoapps/notifications/grouping_notifications.py +++ b/openedx/core/djangoapps/notifications/grouping_notifications.py @@ -106,6 +106,21 @@ def group(self, new_notification, old_notification): } +@NotificationRegistry.register('ora_staff_notification') +class OraStaffGrouper(BaseNotificationGrouper): + """ + Grouper for new ora staff notifications. + """ + + def group(self, new_notification, old_notification): + """ + Groups new ora staff notifications based on the xblock ID. + """ + content_context = old_notification.content_context + content_context.setdefault("grouped", True) + return content_context + + def group_user_notifications(new_notification: Notification, old_notification: Notification): """ Groups user notification based on notification type and group_id diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 3663296b289f..13f8da5438cc 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -828,7 +828,7 @@ openedx-mongodbproxy==0.2.2 # via -r requirements/edx/kernel.in optimizely-sdk==5.2.0 # via -r requirements/edx/bundled.in -ora2==6.14.7 +ora2==6.15.0 # via -r requirements/edx/bundled.in packaging==24.2 # via diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 2ceb572fbbf7..76d63f0cb0d8 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1396,7 +1396,7 @@ optimizely-sdk==5.2.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -ora2==6.14.7 +ora2==6.15.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 43d2cb5aba2a..099fafb7bd67 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1000,7 +1000,7 @@ openedx-mongodbproxy==0.2.2 # via -r requirements/edx/base.txt optimizely-sdk==5.2.0 # via -r requirements/edx/base.txt -ora2==6.14.7 +ora2==6.15.0 # via -r requirements/edx/base.txt packaging==24.2 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index d61ffb530edb..4edc93f95fee 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1058,7 +1058,7 @@ openedx-mongodbproxy==0.2.2 # via -r requirements/edx/base.txt optimizely-sdk==5.2.0 # via -r requirements/edx/base.txt -ora2==6.14.7 +ora2==6.15.0 # via -r requirements/edx/base.txt packaging==24.2 # via From e15274d0ed446601b347c6119ebb1e2806336849 Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Wed, 9 Apr 2025 16:29:44 +0500 Subject: [PATCH 06/23] feat: prevent sending goal reminder email to disabled users (#36505) --- .../commands/goal_reminder_email.py | 3 +++ .../tests/test_goal_reminder_email.py | 19 +++++++++++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py b/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py index b6840ca2c0b0..7b3f3f598bfe 100644 --- a/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py +++ b/lms/djangoapps/course_goals/management/commands/goal_reminder_email.py @@ -49,6 +49,9 @@ def send_ace_message(goal, session_id): Returns true if sent, false if it absorbed an exception and did not send """ user = goal.user + if not user.has_usable_password(): + log.info(f'Goal Reminder User is disabled {user.username} course {goal.course_key}') + return False try: course = CourseOverview.get_from_id(goal.course_key) except CourseOverview.DoesNotExist: diff --git a/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py b/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py index 214fb58219ea..46d46832a6d9 100644 --- a/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py +++ b/lms/djangoapps/course_goals/management/commands/tests/test_goal_reminder_email.py @@ -1,5 +1,5 @@ """Tests for the goal_reminder_email command""" - +import uuid from datetime import datetime from botocore.exceptions import NoCredentialsError @@ -18,7 +18,7 @@ from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory -from lms.djangoapps.course_goals.management.commands.goal_reminder_email import send_email_using_ses +from lms.djangoapps.course_goals.management.commands.goal_reminder_email import send_email_using_ses, send_ace_message from lms.djangoapps.course_goals.models import CourseGoalReminderStatus from lms.djangoapps.course_goals.tests.factories import ( CourseGoalFactory, CourseGoalReminderStatusFactory, UserActivityFactory, @@ -220,6 +220,21 @@ def test_params_without_ses(self, mock_ace): assert 'override_default_channel' not in msg.options assert 'from_address' not in msg.options + @ddt.data(True, False) + @mock.patch('lms.djangoapps.course_goals.management.commands.goal_reminder_email.ace.send') + def test_goal_reminder_email_sent_to_disable_user(self, value, mock_ace): + """ + Test that the goal reminder email is not sent to disabled users. + """ + goal = self.make_valid_goal() + if value: + goal.user.set_password("12345678") + else: + goal.user.set_unusable_password() + goal.user.save() + send_ace_message(goal, str(uuid.uuid4())) + assert mock_ace.called is value + class TestGoalReminderEmailSES(TestCase): """ From 97b7d8c789e939ee2b64ad8250a440d2a540d876 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 7 Apr 2025 14:54:50 -0400 Subject: [PATCH 07/23] docs: Make the graph render correctly. I'm not sure how this worked before but I corrected to use the `graphviz` directive and then full graphviz graph syntax which seemed to fix whatever issues it was having. --- scripts/user_retirement/docs/special_cases.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/user_retirement/docs/special_cases.rst b/scripts/user_retirement/docs/special_cases.rst index ae544c3208c6..bded2d76bcd5 100644 --- a/scripts/user_retirement/docs/special_cases.rst +++ b/scripts/user_retirement/docs/special_cases.rst @@ -19,9 +19,10 @@ re-tried. You can do this using the Django admin. In this example, a user retirement errored during forums retirement, so we manually reset their state from ``ERRORED`` to ``ENROLLMENTS_COMPLETE``. -.. digraph:: retirement_states_example +.. graphviz:: :align: center + digraph G { //rankdir=LR; // Rank Direction Left to Right ranksep = "0.3"; @@ -49,6 +50,7 @@ from ``ERRORED`` to ``ENROLLMENTS_COMPLETE``. } ERRORED -> ENROLLMENTS_COMPLETE[style="bold,dashed",color=black,label=" via django\nadmin"] + } Now, the user retirement driver scripts will automatically resume this user's retirement the next time they are executed. From b7b1f31491bc2ab7b581aeee029a71c80e80767b Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Mon, 24 Mar 2025 10:35:13 -0400 Subject: [PATCH 08/23] feat!: Remove the course sock and related APIs. DEPR: https://github.com/openedx/edx-platform/issues/36429 This change removes the course_sock and related API data. The UI it removes is on the Legacy Courseware pages which have also been replaced and have their own [deprecation ticket](https://github.com/openedx/edx-platform/issues/35803) Before this can be merged, we will need to update the frontend-app-learning MFE to no longer consume the `can_show_upgrade_sock` attribute. BREAKING CHANGE: CourseHomeMetadata, ProgressTab, OutlineTab and VerifiedMode APIs will no longer have a `can_show_upgrade_sock` attribute. --- .../outline/tests/test_view.py | 7 - lms/djangoapps/course_home_api/serializers.py | 6 - lms/djangoapps/courseware/tests/test_views.py | 2 +- lms/djangoapps/courseware/views/index.py | 4 - lms/static/sass/_build-course.scss | 3 - lms/static/sass/bootstrap/lms-main.scss | 1 - lms/static/sass/features/_course-sock.scss | 170 ------------------ lms/templates/courseware/courseware.html | 34 ---- .../djangoapps/courseware_api/serializers.py | 1 - .../core/djangoapps/courseware_api/views.py | 5 - .../features/course_experience/__init__.py | 3 - .../static/course_experience/js/CourseSock.js | 90 ---------- .../course-sock-fragment.html | 76 -------- .../tests/views/test_course_sock.py | 117 ------------ .../tests/views/test_masquerade.py | 44 +---- .../course_experience/views/course_sock.py | 47 ----- webpack.common.config.js | 11 +- 17 files changed, 7 insertions(+), 614 deletions(-) delete mode 100644 lms/static/sass/features/_course-sock.scss delete mode 100644 openedx/features/course_experience/static/course_experience/js/CourseSock.js delete mode 100644 openedx/features/course_experience/templates/course_experience/course-sock-fragment.html delete mode 100644 openedx/features/course_experience/tests/views/test_course_sock.py delete mode 100644 openedx/features/course_experience/views/course_sock.py diff --git a/lms/djangoapps/course_home_api/outline/tests/test_view.py b/lms/djangoapps/course_home_api/outline/tests/test_view.py index 76928846f080..fd1680ba533d 100644 --- a/lms/djangoapps/course_home_api/outline/tests/test_view.py +++ b/lms/djangoapps/course_home_api/outline/tests/test_view.py @@ -31,7 +31,6 @@ from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from openedx.features.course_experience import ( COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, - DISPLAY_COURSE_SOCK_FLAG, ENABLE_COURSE_GOALS ) from openedx.features.discounts.applicability import ( @@ -362,12 +361,6 @@ def test_visibility(self, is_enrolled, is_staff, course_visibility): assert (data['access_expiration'] is not None) == show_enrolled assert (data['resume_course']['url'] is not None) == show_enrolled - @ddt.data(True, False) - def test_can_show_upgrade_sock(self, sock_enabled): - with override_waffle_flag(DISPLAY_COURSE_SOCK_FLAG, active=sock_enabled): - response = self.client.get(self.url) - assert response.data['can_show_upgrade_sock'] == sock_enabled - def test_verified_mode(self): enrollment = CourseEnrollment.enroll(self.user, self.course.id) CourseDurationLimitConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) diff --git a/lms/djangoapps/course_home_api/serializers.py b/lms/djangoapps/course_home_api/serializers.py index d3aa5617331e..44023c9d50db 100644 --- a/lms/djangoapps/course_home_api/serializers.py +++ b/lms/djangoapps/course_home_api/serializers.py @@ -9,7 +9,6 @@ from lms.djangoapps.courseware.utils import verified_upgrade_deadline_link from openedx.core.djangoapps.courseware_api.utils import serialize_upgrade_info from openedx.features.content_type_gating.models import ContentTypeGatingConfig -from openedx.features.course_experience import DISPLAY_COURSE_SOCK_FLAG from openedx.features.course_experience.utils import dates_banner_should_display @@ -59,13 +58,8 @@ class VerifiedModeSerializer(ReadOnlySerializer): Requires 'course_overview', 'enrollment', and 'request' from self.context. """ - can_show_upgrade_sock = serializers.SerializerMethodField() verified_mode = serializers.SerializerMethodField() - def get_can_show_upgrade_sock(self, _): - course_overview = self.context['course_overview'] - return DISPLAY_COURSE_SOCK_FLAG.is_enabled(course_overview.id) - def get_verified_mode(self, _): """Return verified mode information, or None.""" course_overview = self.context['course_overview'] diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index da31ebccce45..6303f07cf351 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -341,7 +341,7 @@ def test_index_query_counts(self): self.client.login(username=self.user.username, password=self.user_password) CourseEnrollment.enroll(self.user, course.id) - with self.assertNumQueries(154, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): + with self.assertNumQueries(152, table_ignorelist=QUERY_COUNT_TABLE_IGNORELIST): with check_mongo_calls(3): url = reverse( 'courseware_section', diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index 7a9242f595ef..e3bd2b34634f 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -48,7 +48,6 @@ default_course_url ) from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url -from openedx.features.course_experience.views.course_sock import CourseSockFragmentView from openedx.features.enterprise_support.api import data_sharing_consent_required from ..access import has_access @@ -454,9 +453,6 @@ def _create_courseware_context(self, request): table_of_contents['chapters'], ) - courseware_context['course_sock_fragment'] = CourseSockFragmentView().render_to_fragment( - request, course=self.course) - # entrance exam data self._add_entrance_exam_to_context(courseware_context) diff --git a/lms/static/sass/_build-course.scss b/lms/static/sass/_build-course.scss index 732eb2caf888..47fde243c7f6 100644 --- a/lms/static/sass/_build-course.scss +++ b/lms/static/sass/_build-course.scss @@ -66,6 +66,3 @@ // responsive @import 'base/layouts'; // temporary spot for responsive course @import 'header'; - -// features -@import 'features/course-sock'; diff --git a/lms/static/sass/bootstrap/lms-main.scss b/lms/static/sass/bootstrap/lms-main.scss index 70bd33da28eb..1299b84fa289 100644 --- a/lms/static/sass/bootstrap/lms-main.scss +++ b/lms/static/sass/bootstrap/lms-main.scss @@ -28,7 +28,6 @@ $static-path: '../..'; @import 'features/bookmarks'; @import 'features/course-experience'; @import 'features/course-search'; -@import 'features/course-sock'; @import 'features/course-upgrade-message'; @import 'features/course-duration-limits'; diff --git a/lms/static/sass/features/_course-sock.scss b/lms/static/sass/features/_course-sock.scss deleted file mode 100644 index a3a1f1484d75..000000000000 --- a/lms/static/sass/features/_course-sock.scss +++ /dev/null @@ -1,170 +0,0 @@ -.verification-sock { - display: inline-block; - position: relative; - width: 100%; - max-width: map-get($container-max-widths, xl); - margin: $baseline auto 0; - -webkit-transition: all 0.4s ease-out; - -moz-transition: all 0.4s ease-out; - -o-transition: all 0.4s ease-out; - -ms-transition: all 0.4s ease-out; - transition: all 0.4s ease-out; - - .action-toggle-verification-sock { - @include left(50%); - @include margin-left(-1 * $baseline * 15/2); - - position: absolute; - top: (-1 * $baseline); - width: ($baseline * 15); - color: theme-color("inverse"); - background-color: theme-color("success"); - border-color: theme-color("success"); - background-image: none; - box-shadow: none; - -webkit-transition: background-color 0.5s; - transition: background-color 0.5s; - cursor: pointer; - - &.active, - &:focus, - &:hover { - color: theme-color("success"); - background-color: theme-color("inverse"); - border-color: theme-color("success"); - background-image: none; - box-shadow: none; - } - } - - .verification-main-panel { - display: none; - overflow: hidden; - border-top: 1px solid $border-color; - padding: ($baseline * 5/2) ($baseline * 2); - -webkit-transition: height ease-out; - transition: height ease-out; - - .verification-desc-panel { - color: $black-t3; - position: relative; - - h2 { - font-size: 1.5rem; - font-weight: $font-weight-bold; - } - - h3 { - font-size: 1.25rem; - } - - @media (max-width: 960px) { - .mini-cert { - display: none; - border: 1px solid $black-t0; - } - } - - .mini-cert { - @include right($baseline); - - position: absolute; - top: $baseline; - width: ($baseline * 13); - } - - .learner-story-container { - display: flex; - max-width: 630px; - - .student-image { - margin: ($baseline / 4) $baseline 0 0; - height: ($baseline * 5/2); - width: ($baseline * 5/2); - } - - .story-quote > .author { - display: block; - margin-top: ($baseline / 4); - font-weight: 600; - } - - &:not(:first-child) { - margin-top: ($baseline * 2); - } - } - - .action-upgrade-certificate { - position: absolute; - right: $baseline; - background-color: theme-color("success"); - border-color: theme-color("success"); - color: theme-color("inverse"); - background-image: none; - box-shadow: none; - cursor: pointer; - - &:hover { - background-color: theme-color("inverse"); - color: theme-color("success"); - } - - @media (max-width: 960px) { - & { - position: relative; - margin-top: ($baseline * 2); - } - } - - @media (min-width: 960px) { - &.stuck-top { - bottom: auto; - top: $baseline * (52 / 5); - } - - &.stuck-bottom { - top: auto; - bottom: $baseline * (-1 * 3/2); - } - - &.attached { - @include right($baseline); - - position: fixed; - bottom: $baseline; - top: auto; - } - } - } - } - } -} - -// Overrides for the courseware page. -.view-courseware { - .verification-sock { - margin-top: 0; - border-top: none; - border-bottom: none; - - .action-toggle-verification-sock { - top: (-1 * $baseline * 5/4); - - &:not(.active) { - color: theme-color("inverse"); - background-color: theme-color("success"); - box-shadow: none; - border: 1px solid theme-color("success"); - - &:hover { - background-color: $success-color-hover; - } - } - } - - .verification-main-panel { - border-top: 0; - border-bottom: 1px solid $border-color; - } - } -} diff --git a/lms/templates/courseware/courseware.html b/lms/templates/courseware/courseware.html index eb94fab17c91..ce9dd19b2934 100644 --- a/lms/templates/courseware/courseware.html +++ b/lms/templates/courseware/courseware.html @@ -290,7 +290,6 @@ - ${HTML(course_sock_fragment.body_html())}
% if settings.FEATURES.get("LICENSING", False): @@ -317,36 +316,3 @@ % endif % endif - -<%static:require_module_async module_name="js/commerce/track_ecommerce_events" class_name="TrackECommerceEvents"> - - var fbeLink = $("#FBE_banner"); - var welcomeLink = $("#welcome"); - var accessDeniedUpsellLink = $("#accessDeniedUpsell"); - var sockLink = $("#sock"); - - TrackECommerceEvents.trackUpsellClick(fbeLink, 'in_course_audit_access_expires', { - pageName: "in_course", - linkType: "link", - linkCategory: "FBE_banner" - }); - - TrackECommerceEvents.trackUpsellClick(welcomeLink, 'in_course_welcome', { - pageName: "in_course", - linkType: "link", - linkCategory: "welcome" - }); - - TrackECommerceEvents.trackUpsellClick(accessDeniedUpsellLink, 'in_course_upgrade', { - pageName: "in_course", - linkType: "link", - linkCategory: "(none)" - }); - - TrackECommerceEvents.trackUpsellClick(sockLink, 'in_course_sock', { - pageName: "in_course", - linkType: "button", - linkCategory: "green_upgrade" - }); - - diff --git a/openedx/core/djangoapps/courseware_api/serializers.py b/openedx/core/djangoapps/courseware_api/serializers.py index 47b977532ff6..191d832c5f51 100644 --- a/openedx/core/djangoapps/courseware_api/serializers.py +++ b/openedx/core/djangoapps/courseware_api/serializers.py @@ -80,7 +80,6 @@ class CourseInfoSerializer(serializers.Serializer): # pylint: disable=abstract- """ access_expiration = serializers.DictField() - can_show_upgrade_sock = serializers.BooleanField() content_type_gating_enabled = serializers.BooleanField() course_goals = CourseGoalsSerializer() effort = serializers.CharField() diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 07e219f83d81..576f0472b770 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -55,7 +55,6 @@ from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from openedx.core.lib.courses import get_course_by_id -from openedx.features.course_experience import DISPLAY_COURSE_SOCK_FLAG from openedx.features.course_experience import ENABLE_COURSE_GOALS from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.access import get_access_expiration_data @@ -127,10 +126,6 @@ def content_type_gating_enabled(self): course_key=self.course_key, ) - @property - def can_show_upgrade_sock(self): - return DISPLAY_COURSE_SOCK_FLAG.is_enabled(self.course_key) - @property def license(self): return self.course.license diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index 3f4c4d3fefd1..c5f11a301340 100644 --- a/openedx/features/course_experience/__init__.py +++ b/openedx/features/course_experience/__init__.py @@ -18,9 +18,6 @@ f'{WAFFLE_FLAG_NAMESPACE}.disable_course_outline_page', __name__ ) -# Waffle flag to enable the sock on the footer of the home and courseware pages. -DISPLAY_COURSE_SOCK_FLAG = CourseWaffleFlag(f'{WAFFLE_FLAG_NAMESPACE}.display_course_sock', __name__) # lint-amnesty, pylint: disable=toggle-missing-annotation - # Waffle flag to let learners access a course before its start date. COURSE_PRE_START_ACCESS_FLAG = WaffleFlag(f'{WAFFLE_FLAG_NAMESPACE}.pre_start_access', __name__) # lint-amnesty, pylint: disable=toggle-missing-annotation diff --git a/openedx/features/course_experience/static/course_experience/js/CourseSock.js b/openedx/features/course_experience/static/course_experience/js/CourseSock.js deleted file mode 100644 index 676be0a07498..000000000000 --- a/openedx/features/course_experience/static/course_experience/js/CourseSock.js +++ /dev/null @@ -1,90 +0,0 @@ -/* globals Logger */ - -export class CourseSock { // eslint-disable-line import/prefer-default-export - constructor() { - // eslint-disable-next-line no-undef - const $toggleActionButton = $('.action-toggle-verification-sock'); - // eslint-disable-next-line no-undef - const $verificationSock = $('.verification-sock .verification-main-panel'); - // eslint-disable-next-line no-undef - const $upgradeToVerifiedButton = $('.verification-sock .action-upgrade-certificate'); - // eslint-disable-next-line no-undef - const $miniCert = $('.mini-cert'); - const pageLocation = window.location.href.indexOf('courseware') > -1 - ? 'Course Content Page' : 'Home Page'; - - // Behavior to fix button to bottom of screen on scroll - const fixUpgradeButton = () => { - if (!$upgradeToVerifiedButton.is(':visible')) { return; } - - // Grab the current scroll location - // eslint-disable-next-line no-undef - const documentBottom = $(window).scrollTop() + $(window).height(); - - // Establish a sliding window in which the button is fixed - const startFixed = $verificationSock.offset().top + 320; - const endFixed = (startFixed + $verificationSock.height()) - 220; - - // Ensure update button stays in sock even when max-width is exceeded - const distRight = window.outerWidth - ($miniCert.offset().left + $miniCert.width()); - - // Update positioning when scrolling is in fixed window and screen width is sufficient - if ((documentBottom > startFixed && documentBottom < endFixed - // eslint-disable-next-line no-undef - && $(window).width() > 960)) { - $upgradeToVerifiedButton.addClass('attached'); - $upgradeToVerifiedButton.css('right', `${distRight}px`); - } else { - // If outside sliding window, reset to un-attached state - $upgradeToVerifiedButton.removeClass('attached'); - $upgradeToVerifiedButton.css('right', '20px'); - - // Add class to define absolute location - if (documentBottom < startFixed) { - $upgradeToVerifiedButton.addClass('stuck-top'); - $upgradeToVerifiedButton.removeClass('stuck-bottom'); - } else if (documentBottom > endFixed) { - $upgradeToVerifiedButton.addClass('stuck-bottom'); - $upgradeToVerifiedButton.removeClass('stuck-top'); - } - } - }; - - // Fix the sock to the screen on scroll and resize events - if ($upgradeToVerifiedButton.length) { - // eslint-disable-next-line no-undef - $(window).scroll(fixUpgradeButton).resize(fixUpgradeButton); - } - - // Open the sock when user clicks to Learn More - $toggleActionButton.on('click', () => { - const toggleSpeed = 400; - $toggleActionButton.toggleClass('active'); - $verificationSock.slideToggle(toggleSpeed, fixUpgradeButton); - - // Toggle aria-expanded attribute - const newAriaExpandedState = $toggleActionButton.attr('aria-expanded') === 'false'; - $toggleActionButton.attr('aria-expanded', newAriaExpandedState); - - // Log open and close events - const isOpening = $toggleActionButton.hasClass('active'); - const logMessage = isOpening ? 'edx.bi.course.sock.toggle_opened' - : 'edx.bi.course.sock.toggle_closed'; - window.analytics.track( - logMessage, - { - from_page: pageLocation, - }, - ); - }); - - $upgradeToVerifiedButton.on('click', () => { - Logger.log( - 'edx.course.enrollment.upgrade.clicked', - { - location: 'sock', - }, - ); - }); - } -} diff --git a/openedx/features/course_experience/templates/course_experience/course-sock-fragment.html b/openedx/features/course_experience/templates/course_experience/course-sock-fragment.html deleted file mode 100644 index ede2a7f2b297..000000000000 --- a/openedx/features/course_experience/templates/course_experience/course-sock-fragment.html +++ /dev/null @@ -1,76 +0,0 @@ -## mako - -<%page expression_filter="h"/> -<%namespace name='static' file='../static_content.html'/> - -<%! -from django.utils.translation import gettext as _ -from openedx.core.djangolib.markup import HTML, Text -from openedx.features.course_experience import DISPLAY_COURSE_SOCK_FLAG -%> - -<%block name="content"> - % if show_course_sock: - - %endif - - -<%static:webpack entry="CourseSock"> - new CourseSock({ - el:'.verification-sock' - }); - diff --git a/openedx/features/course_experience/tests/views/test_course_sock.py b/openedx/features/course_experience/tests/views/test_course_sock.py deleted file mode 100644 index 5c612e3ca85f..000000000000 --- a/openedx/features/course_experience/tests/views/test_course_sock.py +++ /dev/null @@ -1,117 +0,0 @@ -""" -Tests for course verification sock -""" - - -from unittest import mock - -import ddt -from django.urls import reverse -from edx_toggles.toggles.testutils import override_waffle_flag - -from common.djangoapps.course_modes.models import CourseMode -from lms.djangoapps.commerce.models import CommerceConfiguration -from lms.djangoapps.courseware.tests.helpers import set_preview_mode -from openedx.core.djangolib.markup import HTML -from openedx.features.course_experience import DISPLAY_COURSE_SOCK_FLAG -from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order - -from .helpers import add_course_mode - -TEST_PASSWORD = 'Password1234' -TEST_VERIFICATION_SOCK_LOCATOR = '
DISCOUNT_PRICE"), True)) - ) - def test_upgrade_message_discount(self): - response = self.get_courseware(self.verified_course) - self.assertContains(response, "DISCOUNT_PRICE") - - def assert_verified_sock_is_visible(self, course, response): # lint-amnesty, pylint: disable=unused-argument - return self.assertContains(response, TEST_VERIFICATION_SOCK_LOCATOR, html=False) - - def assert_verified_sock_is_not_visible(self, course, response): # lint-amnesty, pylint: disable=unused-argument - return self.assertNotContains(response, TEST_VERIFICATION_SOCK_LOCATOR, html=False) diff --git a/openedx/features/course_experience/tests/views/test_masquerade.py b/openedx/features/course_experience/tests/views/test_masquerade.py index 0f555e27f7e2..f0c110d8e9aa 100644 --- a/openedx/features/course_experience/tests/views/test_masquerade.py +++ b/openedx/features/course_experience/tests/views/test_masquerade.py @@ -2,11 +2,7 @@ Tests for masquerading functionality on course_experience """ -from django.urls import reverse - -from edx_toggles.toggles.testutils import override_waffle_flag -from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin, set_preview_mode -from openedx.features.course_experience import DISPLAY_COURSE_SOCK_FLAG +from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin from common.djangoapps.student.roles import CourseStaffRole from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order @@ -15,7 +11,6 @@ from xmodule.partitions.partitions_service import PartitionService # lint-amnesty, pylint: disable=wrong-import-order from .helpers import add_course_mode -from .test_course_sock import TEST_VERIFICATION_SOCK_LOCATOR TEST_PASSWORD = 'Password1234' @@ -59,40 +54,3 @@ def get_group_id_by_course_mode_name(self, course_id, mode_name): if group.name == mode_name: return group.id return None - - -@set_preview_mode(True) -class TestVerifiedUpgradesWithMasquerade(MasqueradeTestBase): - """ - Tests for the course verification upgrade messages while the user is being masqueraded. - """ - - @override_waffle_flag(DISPLAY_COURSE_SOCK_FLAG, active=True) - def test_masquerade_as_student(self): - # Elevate the staff user to be student - self.update_masquerade(course=self.verified_course, user_partition_id=ENROLLMENT_TRACK_PARTITION_ID) - response = self.client.get(reverse('courseware', kwargs={'course_id': str(self.verified_course.id)})) - self.assertContains(response, TEST_VERIFICATION_SOCK_LOCATOR, html=False) - - @override_waffle_flag(DISPLAY_COURSE_SOCK_FLAG, active=True) - def test_masquerade_as_verified_student(self): - user_group_id = self.get_group_id_by_course_mode_name( - self.verified_course.id, - 'Verified Certificate' - ) - self.update_masquerade(course=self.verified_course, group_id=user_group_id, - user_partition_id=ENROLLMENT_TRACK_PARTITION_ID) - response = self.client.get(reverse('courseware', kwargs={'course_id': str(self.verified_course.id)})) - self.assertNotContains(response, TEST_VERIFICATION_SOCK_LOCATOR, html=False) - - @override_waffle_flag(DISPLAY_COURSE_SOCK_FLAG, active=True) - def test_masquerade_as_masters_student(self): - user_group_id = self.get_group_id_by_course_mode_name( - self.masters_course.id, - 'Masters' - ) - self.update_masquerade(course=self.masters_course, group_id=user_group_id, - user_partition_id=ENROLLMENT_TRACK_PARTITION_ID) - response = self.client.get(reverse('courseware', kwargs={'course_id': str(self.masters_course.id)})) - - self.assertNotContains(response, TEST_VERIFICATION_SOCK_LOCATOR, html=False) diff --git a/openedx/features/course_experience/views/course_sock.py b/openedx/features/course_experience/views/course_sock.py deleted file mode 100644 index af3221315d24..000000000000 --- a/openedx/features/course_experience/views/course_sock.py +++ /dev/null @@ -1,47 +0,0 @@ -""" -Fragment for rendering the course's sock and associated toggle button. -""" - -from django.template.loader import render_to_string -from web_fragments.fragment import Fragment - -from lms.djangoapps.courseware.utils import ( - can_show_verified_upgrade, - verified_upgrade_deadline_link -) -from openedx.core.djangoapps.plugin_api.views import EdxFragmentView -from openedx.features.discounts.utils import format_strikeout_price -from common.djangoapps.student.models import CourseEnrollment - - -class CourseSockFragmentView(EdxFragmentView): - """ - A fragment to provide extra functionality in a dropdown sock. - """ - def render_to_fragment(self, request, course, **kwargs): # lint-amnesty, pylint: disable=arguments-differ - """ - Render the course's sock fragment. - """ - context = self.get_verification_context(request, course) - html = render_to_string('course_experience/course-sock-fragment.html', context) - return Fragment(html) - - @staticmethod - def get_verification_context(request, course): # lint-amnesty, pylint: disable=missing-function-docstring - enrollment = CourseEnrollment.get_enrollment(request.user, course.id) - show_course_sock = can_show_verified_upgrade(request.user, enrollment, course) - if show_course_sock: - upgrade_url = verified_upgrade_deadline_link(request.user, course=course) - course_price, _ = format_strikeout_price(request.user, course) - else: - upgrade_url = '' - course_price = '' - - context = { - 'show_course_sock': show_course_sock, - 'course_price': course_price, - 'course_id': course.id, - 'upgrade_url': upgrade_url, - } - - return context diff --git a/webpack.common.config.js b/webpack.common.config.js index 06efdb5c6151..89082f218524 100644 --- a/webpack.common.config.js +++ b/webpack.common.config.js @@ -130,7 +130,6 @@ module.exports = Merge.smart({ CompletionOnViewService: './lms/static/completion/js/CompletionOnViewService.js', // Features - CourseSock: './openedx/features/course_experience/static/course_experience/js/CourseSock.js', Currency: './openedx/features/course_experience/static/course_experience/js/currency.js', AnnouncementsView: './openedx/features/announcements/static/announcements/jsx/Announcements.jsx', @@ -195,19 +194,19 @@ module.exports = Merge.smart({ multiple: [ { search: defineHeader, replace: '' }, { search: defineFooter, replace: '' }, - { + { search: /(\/\* RequireJS) \*\//g, replace(match, p1, offset, string) { return p1; } }, - { + { search: /\/\* Webpack/g, replace(match, p1, offset, string) { return match + ' */'; } }, - { + { search: /text!(.*?\.underscore)/g, replace(match, p1, offset, string) { return p1; @@ -658,13 +657,13 @@ module.exports = Merge.smart({ // We used to have node: { fs: 'empty' } in this file, // that is no longer supported. Adding this based on the recommendation in // https://stackoverflow.com/questions/64361940/webpack-error-configuration-node-has-an-unknown-property-fs - // + // // With this uncommented tests fail // Tests failed in the following suites: // * lms javascript // * xmodule-webpack javascript // Error: define cannot be used indirect - // + // // fallback: { // fs: false // } From 4e48a11569bd6a256b84e48385d4be2f1340309a Mon Sep 17 00:00:00 2001 From: Kyle D McCormick Date: Mon, 7 Apr 2025 15:59:14 -0400 Subject: [PATCH 09/23] fix: Remove Two Pointless Contentstore Waffle Flags These flags had no effect, so they are being removed: * new_core_editors.use_advanced_problem_editor * new_editors.add_game_block_button Related to: https://github.com/openedx/frontend-app-authoring/pull/1753 Part of: https://github.com/openedx/edx-platform/issues/26275 --- cms/djangoapps/contentstore/toggles.py | 31 ------------------- .../xblock_storage_handlers/view_handlers.py | 6 ---- 2 files changed, 37 deletions(-) diff --git a/cms/djangoapps/contentstore/toggles.py b/cms/djangoapps/contentstore/toggles.py index af125e0d9b7a..8d1e96a55119 100644 --- a/cms/djangoapps/contentstore/toggles.py +++ b/cms/djangoapps/contentstore/toggles.py @@ -160,37 +160,6 @@ def use_new_problem_editor(): return ENABLE_NEW_PROBLEM_EDITOR_FLAG.is_enabled() -# .. toggle_name: new_core_editors.use_advanced_problem_editor -# .. toggle_implementation: WaffleFlag -# .. toggle_default: False -# .. toggle_description: This flag enables the use of the new core problem xblock advanced editor as the default -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2024-07-25 -# .. toggle_target_removal_date: 2024-08-31 -# .. toggle_tickets: TNL-11694 -# .. toggle_warning: -ENABLE_DEFAULT_ADVANCED_PROBLEM_EDITOR_FLAG = WaffleFlag('new_core_editors.use_advanced_problem_editor', __name__) - - -# .. toggle_name: new_editors.add_game_block_button -# .. toggle_implementation: WaffleFlag -# .. toggle_default: False -# .. toggle_description: This flag enables the creation of the new games block -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2023-07-26 -# .. toggle_target_removal_date: 2023-09-31 -# .. toggle_tickets: TNL-10924 -# .. toggle_warning: -ENABLE_ADD_GAME_BLOCK_FLAG = WaffleFlag('new_editors.add_game_block_button', __name__) - - -def use_add_game_block(): - """ - Returns a boolean if add game block button is enabled - """ - return ENABLE_ADD_GAME_BLOCK_FLAG.is_enabled() - - # .. toggle_name: contentstore.individualize_anonymous_user_id # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 19e20804c2fb..3506825ae357 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -33,7 +33,6 @@ from xblock.fields import Scope from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG -from cms.djangoapps.contentstore.toggles import ENABLE_DEFAULT_ADVANCED_PROBLEM_EDITOR_FLAG from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.ai_aside_summary_config import AiAsideSummaryConfig from cms.lib.xblock.upstream_sync import BadUpstream, sync_from_upstream @@ -187,11 +186,6 @@ def handle_xblock(request, usage_key_string=None): # TODO: pass fields to get_block_info and only return those with modulestore().bulk_operations(usage_key.course_key): response = get_block_info(get_xblock(usage_key, request.user)) - # TODO: remove after beta testing for the new problem editor parser - if response["category"] == "problem": - response["metadata"]["default_to_advanced"] = ( - ENABLE_DEFAULT_ADVANCED_PROBLEM_EDITOR_FLAG.is_enabled() - ) if "customReadToken" in fields: parent_children = _get_block_parent_children(get_xblock(usage_key, request.user)) response.update(parent_children) From 47bc2f4274a04cc7d459f13d5e0283b2a927a8a5 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 20 Mar 2025 14:51:20 -0400 Subject: [PATCH 10/23] refactor: Remove now-unused static asset Django settings Closes: https://github.com/openedx/edx-platform/issues/36407 --- .../contentstore/views/tests/test_block.py | 3 --- cms/envs/common.py | 18 ------------------ cms/envs/devstack.py | 3 --- cms/envs/mock.yml | 1 - lms/envs/common.py | 8 -------- lms/envs/devstack.py | 3 --- 6 files changed, 36 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index 93e382fb7fba..ef85c446c81e 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -560,9 +560,6 @@ def assert_xblock_info(xblock, xblock_info): else: self.assertNotIn("ancestors", response) xblock_info = get_block_info(xblock) - # TODO: remove after beta testing for the new problem editor parser - if xblock_info["category"] == "problem": - xblock_info["metadata"]["default_to_advanced"] = False self.assertEqual(xblock_info, response) diff --git a/cms/envs/common.py b/cms/envs/common.py index 6cc193827170..9be3eb9e0956 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1317,16 +1317,6 @@ ##### custom vendor plugin variables ##### -# .. setting_name: JS_ENV_EXTRA_CONFIG -# .. setting_default: {} -# .. setting_description: JavaScript code can access this dictionary using `process.env.JS_ENV_EXTRA_CONFIG` -# One of the current use cases for this is enabling custom TinyMCE plugins -# (TINYMCE_ADDITIONAL_PLUGINS) and overriding the TinyMCE configuration (TINYMCE_CONFIG_OVERRIDES). -# .. setting_warning: This Django setting is DEPRECATED! Starting in Sumac, Webpack will no longer -# use Django settings. Please set the JS_ENV_EXTRA_CONFIG environment variable to an equivalent JSON -# string instead. For details, see: https://github.com/openedx/edx-platform/issues/31895 -JS_ENV_EXTRA_CONFIG = json.loads(os.environ.get('JS_ENV_EXTRA_CONFIG', '{}')) - ############################### PIPELINE ####################################### PIPELINE = { @@ -1508,14 +1498,6 @@ } } -# .. setting_name: WEBPACK_CONFIG_PATH -# .. setting_default: "webpack.prod.config.js" -# .. setting_description: Path to the Webpack configuration file. Used by Paver scripts. -# .. setting_warning: This Django setting is DEPRECATED! Starting in Sumac, Webpack will no longer -# use Django settings. Please set the WEBPACK_CONFIG_PATH environment variable instead. For details, -# see: https://github.com/openedx/edx-platform/issues/31895 -WEBPACK_CONFIG_PATH = os.environ.get('WEBPACK_CONFIG_PATH', 'webpack.prod.config.js') - ############################ SERVICE_VARIANT ################################## diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index ac21c2ee01a3..63dff1fbb9c2 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -66,9 +66,6 @@ 'django.contrib.staticfiles.finders.AppDirectoriesFinder', ] -# Load development webpack donfiguration -WEBPACK_CONFIG_PATH = 'webpack.dev.config.js' - ############################ PYFS XBLOCKS SERVICE ############################# # Set configuration for Django pyfilesystem diff --git a/cms/envs/mock.yml b/cms/envs/mock.yml index 6e37b1818066..fb2233501ef4 100644 --- a/cms/envs/mock.yml +++ b/cms/envs/mock.yml @@ -501,7 +501,6 @@ IDA_LOGOUT_URI_LIST: - https://discovery.localhost/logout/ - https://commerce-coordinator.localhost/logout/ ID_VERIFICATION_SUPPORT_LINK: https://support.localhost/hc/en-us/articles/206503858-How-do-I-complete-photo-verification- -JS_ENV_EXTRA_CONFIG: {} JWT_AUTH: JWT_AUDIENCE: test_jwt_audience JWT_AUTH_COOKIE_HEADER_PAYLOAD: jwt-cookie-header-payload diff --git a/lms/envs/common.py b/lms/envs/common.py index 316dad98da3f..719e12fb8b0d 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2806,14 +2806,6 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring } } -# .. setting_name: WEBPACK_CONFIG_PATH -# .. setting_default: "webpack.prod.config.js" -# .. setting_description: Path to the Webpack configuration file. Used by Paver scripts. -# .. setting_warning: This Django setting is DEPRECATED! Starting in Sumac, Webpack will no longer -# use Django settings. Please set the WEBPACK_CONFIG_PATH environment variable instead. For details, -# see: https://github.com/openedx/edx-platform/issues/31895 -WEBPACK_CONFIG_PATH = os.environ.get('WEBPACK_CONFIG_PATH', 'webpack.prod.config.js') - ########################## DJANGO DEBUG TOOLBAR ############################### # We don't enable Django Debug Toolbar universally, but whenever we do, we want diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index d607073b9a29..e024e81edfef 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -136,9 +136,6 @@ def should_show_debug_toolbar(request): # lint-amnesty, pylint: disable=missing PIPELINE['SASS_ARGUMENTS'] = '--debug-info' -# Load development webpack configuration -WEBPACK_CONFIG_PATH = 'webpack.dev.config.js' - ########################### VERIFIED CERTIFICATES ################################# FEATURES['AUTOMATIC_VERIFY_STUDENT_IDENTITY_FOR_TESTING'] = True From 82efb8a130911d020717d0c8607143c10f742002 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Wed, 9 Apr 2025 14:10:12 -0400 Subject: [PATCH 11/23] fix: Globally Enable Studio Content REST API (#36488) There was a waffle flag `contentstore.enable_studio_content_api`, intended to gate the "experimental" REST APIs at `/api/contentstore/v{0,1,2}/*`. In practice, these APIs are no longer experimental: for the past few named releases, they have been enabled in Tutor and used to power the Authoring MFE. We are making the Authoring MFE default-on in all Open edX sites starting in Teak, with the legacy authoring frontend slated for removal by Ulmo. Therefore, we need to remove flag which is gating the REST API. We are _not_ introducing a temporary opt-out toggle, as we do need feel it is necessary. Part of: https://github.com/openedx/edx-platform/issues/36275 --- .../rest_api/v0/tests/test_assets.py | 33 ----------- .../rest_api/v0/tests/test_xblock.py | 40 ------------- .../rest_api/v0/views/api_heartbeat.py | 5 +- .../contentstore/rest_api/v0/views/assets.py | 25 -------- .../rest_api/v0/views/authoring_videos.py | 58 ------------------- .../rest_api/v0/views/transcripts.py | 18 ------ .../contentstore/rest_api/v0/views/xblock.py | 25 -------- cms/djangoapps/contentstore/toggles.py | 25 +------- 8 files changed, 2 insertions(+), 227 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_assets.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_assets.py index 46b636916df8..3772dbb64e77 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_assets.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_assets.py @@ -60,13 +60,8 @@ def send_request(self, _url, _data): } ), ) - @patch( - f"cms.djangoapps.contentstore.rest_api.{VERSION}.views.xblock.toggles.use_studio_content_api", - return_value=True, - ) def make_request( self, - mock_use_studio_content_api, mock_handle_assets, run_assertions=None, course_id=None, @@ -125,13 +120,6 @@ def assert_assets_handler_called(self, *, mock_handle_assets, response): def send_request(self, url, data): return self.client.get(url) - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.get(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_assets_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -182,13 +170,6 @@ def assert_assets_handler_called(self, *, mock_handle_assets, response): def send_request(self, url, data): return self.client.post(url, data=data, format="multipart") - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.post(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_assets_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -232,13 +213,6 @@ def assert_assets_handler_called(self, *, mock_handle_assets, response): def send_request(self, url, data): return self.client.put(url, data=data, format="json") - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.put(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_assets_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -277,13 +251,6 @@ def assert_assets_handler_called(self, *, mock_handle_assets, response): def send_request(self, url, data): return self.client.delete(url) - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.delete(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_assets_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password diff --git a/cms/djangoapps/contentstore/rest_api/v0/tests/test_xblock.py b/cms/djangoapps/contentstore/rest_api/v0/tests/test_xblock.py index e4c21eb353b1..512c92f6ffe9 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/tests/test_xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v0/tests/test_xblock.py @@ -55,13 +55,8 @@ def send_request(self, _url, _data): } ), ) - @patch( - f"cms.djangoapps.contentstore.rest_api.{VERSION}.views.xblock.toggles.use_studio_content_api", - return_value=True, - ) def make_request( self, - mock_use_studio_content_api, mock_handle_xblock, run_assertions=None, course_id=None, @@ -111,13 +106,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): def send_request(self, url, data): return self.client.get(url) - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.get(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_xblock_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -167,13 +155,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): def send_request(self, url, data): return self.client.post(url, data=data, format="json") - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.post(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_xblock_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -218,13 +199,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): def send_request(self, url, data): return self.client.put(url, data=data, format="json") - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.put(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_xblock_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -269,13 +243,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): def send_request(self, url, data): return self.client.patch(url, data=data, format="json") - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.patch(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_xblock_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password @@ -310,13 +277,6 @@ def assert_xblock_handler_called(self, *, mock_handle_xblock, response): def send_request(self, url, data): return self.client.delete(url) - def test_api_behind_feature_flag(self): - # should return 404 if the feature flag is not enabled - url = self.get_url() - - response = self.client.delete(url) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_xblock_handler_called_with_correct_arguments(self): self.client.login( username=self.course_instructor.username, password=self.password diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/api_heartbeat.py b/cms/djangoapps/contentstore/rest_api/v0/views/api_heartbeat.py index f8b539557e5e..f322f2360939 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/api_heartbeat.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/api_heartbeat.py @@ -5,7 +5,6 @@ from rest_framework.response import Response from rest_framework import status from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes -import cms.djangoapps.contentstore.toggles as toggles class APIHeartBeatView(DeveloperErrorViewMixin, APIView): @@ -43,6 +42,4 @@ def get(self, request: Request): } ``` """ - if toggles.use_studio_content_api(): - return Response({'status': 'heartbeat successful'}, status=status.HTTP_200_OK) - return Response(status=status.HTTP_403_FORBIDDEN) + return Response({'status': 'heartbeat successful'}, status=status.HTTP_200_OK) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/assets.py b/cms/djangoapps/contentstore/rest_api/v0/views/assets.py index 0c0c24aeab69..a7a495256514 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/assets.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/assets.py @@ -4,7 +4,6 @@ import logging from rest_framework.generics import CreateAPIView, RetrieveAPIView, UpdateAPIView, DestroyAPIView from django.views.decorators.csrf import csrf_exempt -from django.http import Http404 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from common.djangoapps.util.json_request import expect_json_in_class_view @@ -12,7 +11,6 @@ from cms.djangoapps.contentstore.api import course_author_access_required from cms.djangoapps.contentstore.asset_storage_handlers import handle_assets -import cms.djangoapps.contentstore.toggles as contentstore_toggles from ..serializers.assets import AssetSerializer from .utils import validate_request_with_serializer @@ -20,7 +18,6 @@ from openedx.core.lib.api.parsers import TypedFileUploadParser log = logging.getLogger(__name__) -toggles = contentstore_toggles @view_auth_classes() @@ -33,17 +30,6 @@ class AssetsCreateRetrieveView(DeveloperErrorViewMixin, CreateAPIView, RetrieveA serializer_class = AssetSerializer parser_classes = (JSONParser, MultiPartParser, FormParser, TypedFileUploadParser) - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @csrf_exempt @course_author_access_required @validate_request_with_serializer @@ -66,17 +52,6 @@ class AssetsUpdateDestroyView(DeveloperErrorViewMixin, UpdateAPIView, DestroyAPI serializer_class = AssetSerializer parser_classes = (JSONParser, MultiPartParser, FormParser, TypedFileUploadParser) - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @course_author_access_required @expect_json_in_class_view @validate_request_with_serializer diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py index 8fb66070f3bf..f97bf7a98d16 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/authoring_videos.py @@ -9,7 +9,6 @@ ) from rest_framework.parsers import (MultiPartParser, FormParser) from django.views.decorators.csrf import csrf_exempt -from django.http import Http404 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from openedx.core.lib.api.parsers import TypedFileUploadParser @@ -27,12 +26,10 @@ VideoUploadSerializer, VideoImageSerializer, ) -import cms.djangoapps.contentstore.toggles as contentstore_toggles from .utils import validate_request_with_serializer log = logging.getLogger(__name__) -toggles = contentstore_toggles @view_auth_classes() @@ -44,17 +41,6 @@ class VideosUploadsView(DeveloperErrorViewMixin, RetrieveAPIView, DestroyAPIView """ serializer_class = VideoUploadSerializer - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @course_author_access_required def retrieve(self, request, course_key, edx_video_id=None): # pylint: disable=arguments-differ return handle_videos(request, course_key.html_id(), edx_video_id) @@ -73,17 +59,6 @@ class VideosCreateUploadView(DeveloperErrorViewMixin, CreateAPIView): """ serializer_class = VideoUploadSerializer - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @csrf_exempt @course_author_access_required @expect_json_in_class_view @@ -102,17 +77,6 @@ class VideoImagesView(DeveloperErrorViewMixin, CreateAPIView): serializer_class = VideoImageSerializer parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @csrf_exempt @course_author_access_required @expect_json_in_class_view @@ -133,17 +97,6 @@ class VideoEncodingsDownloadView(DeveloperErrorViewMixin, RetrieveAPIView): # does not specify a serializer class. swagger_schema = None - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @csrf_exempt @course_author_access_required def retrieve(self, request, course_key): # pylint: disable=arguments-differ @@ -161,17 +114,6 @@ class VideoFeaturesView(DeveloperErrorViewMixin, RetrieveAPIView): # does not specify a serializer class. swagger_schema = None - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @csrf_exempt def retrieve(self, request): # pylint: disable=arguments-differ return enabled_video_features(request) diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/transcripts.py b/cms/djangoapps/contentstore/rest_api/v0/views/transcripts.py index 9a63693e12b9..3344fdbd1f43 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/transcripts.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/transcripts.py @@ -8,7 +8,6 @@ DestroyAPIView ) from django.views.decorators.csrf import csrf_exempt -from django.http import Http404 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from common.djangoapps.util.json_request import expect_json_in_class_view @@ -20,7 +19,6 @@ delete_video_transcript_or_404, handle_transcript_download, ) -import cms.djangoapps.contentstore.toggles as contentstore_toggles from ..serializers import TranscriptSerializer, YoutubeTranscriptCheckSerializer, YoutubeTranscriptUploadSerializer from rest_framework.parsers import (MultiPartParser, FormParser) from openedx.core.lib.api.parsers import TypedFileUploadParser @@ -28,7 +26,6 @@ from cms.djangoapps.contentstore.rest_api.v0.views.utils import validate_request_with_serializer log = logging.getLogger(__name__) -toggles = contentstore_toggles @view_auth_classes() @@ -42,11 +39,6 @@ class TranscriptView(DeveloperErrorViewMixin, CreateAPIView, RetrieveAPIView, De serializer_class = TranscriptSerializer parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) - def dispatch(self, request, *args, **kwargs): - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @csrf_exempt @course_author_access_required @expect_json_in_class_view @@ -81,11 +73,6 @@ class YoutubeTranscriptCheckView(DeveloperErrorViewMixin, RetrieveAPIView): serializer_class = YoutubeTranscriptCheckSerializer parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) - def dispatch(self, request, *args, **kwargs): - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @course_author_access_required def retrieve(self, request, course_key_string): # pylint: disable=arguments-differ """ @@ -104,11 +91,6 @@ class YoutubeTranscriptUploadView(DeveloperErrorViewMixin, RetrieveAPIView): serializer_class = YoutubeTranscriptUploadSerializer parser_classes = (MultiPartParser, FormParser, TypedFileUploadParser) - def dispatch(self, request, *args, **kwargs): - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - @course_author_access_required def retrieve(self, request, course_key_string): # pylint: disable=arguments-differ """ diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py index cc26619fb83a..8e678ae845e0 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py @@ -4,21 +4,18 @@ import logging from rest_framework.generics import RetrieveUpdateDestroyAPIView, CreateAPIView from django.views.decorators.csrf import csrf_exempt -from django.http import Http404 from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from common.djangoapps.util.json_request import expect_json_in_class_view from cms.djangoapps.contentstore.api import course_author_access_required from cms.djangoapps.contentstore.xblock_storage_handlers import view_handlers -import cms.djangoapps.contentstore.toggles as contentstore_toggles from ..serializers import XblockSerializer from .utils import validate_request_with_serializer log = logging.getLogger(__name__) -toggles = contentstore_toggles handle_xblock = view_handlers.handle_xblock @@ -32,17 +29,6 @@ class XblockView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView): """ serializer_class = XblockSerializer - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - # pylint: disable=arguments-differ @course_author_access_required @expect_json_in_class_view @@ -77,17 +63,6 @@ class XblockCreateView(DeveloperErrorViewMixin, CreateAPIView): """ serializer_class = XblockSerializer - def dispatch(self, request, *args, **kwargs): - # TODO: probably want to refactor this to a decorator. - """ - The dispatch method of a View class handles HTTP requests in general - and calls other methods to handle specific HTTP methods. - We use this to raise a 404 if the content api is disabled. - """ - if not toggles.use_studio_content_api(): - raise Http404 - return super().dispatch(request, *args, **kwargs) - # pylint: disable=arguments-differ @csrf_exempt @course_author_access_required diff --git a/cms/djangoapps/contentstore/toggles.py b/cms/djangoapps/contentstore/toggles.py index 8d1e96a55119..4fcc8372c594 100644 --- a/cms/djangoapps/contentstore/toggles.py +++ b/cms/djangoapps/contentstore/toggles.py @@ -5,6 +5,7 @@ from openedx.core.djangoapps.content.search import api as search_api from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag + # .. toggle_name: FEATURES['ENABLE_EXPORT_GIT'] # .. toggle_implementation: SettingDictToggle # .. toggle_default: False @@ -180,30 +181,6 @@ def individualize_anonymous_user_id(course_id): return INDIVIDUALIZE_ANONYMOUS_USER_ID.is_enabled(course_id) -# .. toggle_name: contentstore.enable_studio_content_api -# .. toggle_implementation: WaffleFlag -# .. toggle_default: False -# .. toggle_description: Enables the new (experimental and unsafe!) Studio Content REST API for course authors, -# .. which provides CRUD capabilities for course content and xblock editing. -# .. Use at your own peril - you can easily delete learner data when editing running courses. -# .. This can be triggered by deleting blocks, editing subsections, problems, assignments, discussions, -# .. creating new problems or graded sections, and by other things you do. -# .. toggle_use_cases: open_edx -# .. toggle_creation_date: 2023-05-26 -# .. toggle_tickets: TNL-10208 -ENABLE_STUDIO_CONTENT_API = WaffleFlag( - f'{CONTENTSTORE_NAMESPACE}.enable_studio_content_api', - __name__, -) - - -def use_studio_content_api(): - """ - Returns a boolean if studio editing API is enabled - """ - return ENABLE_STUDIO_CONTENT_API.is_enabled() - - # .. toggle_name: new_studio_mfe.use_new_home_page # .. toggle_implementation: WaffleFlag # .. toggle_default: False From f8b3784589bc4e7c6b65bfa9b4e2309b7b068af4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 10 Apr 2025 02:44:54 -0300 Subject: [PATCH 12/23] feat: container support for tags [FC-0083] (#36484) * feat: container tag support * fix: fixes from review * docs: fix typo --- openedx/core/djangoapps/content/search/api.py | 18 ++--- .../djangoapps/content/search/documents.py | 29 +++---- .../djangoapps/content/search/handlers.py | 19 +++-- .../content/search/tests/test_api.py | 46 ++++++++++- .../content/search/tests/test_documents.py | 81 +++++++++---------- .../rest_api/v1/tests/test_views.py | 50 +++++++++++- .../content_tagging/tests/test_api.py | 19 ++++- 7 files changed, 183 insertions(+), 79 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index a597ae51e865..d9bcc55cd9d7 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -17,6 +17,7 @@ from meilisearch import Client as MeilisearchClient from meilisearch.errors import MeilisearchApiError, MeilisearchError from meilisearch.models.task import TaskInfo +from opaque_keys import OpaqueKey from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryCollectionLocator from openedx_learning.api import authoring as authoring_api @@ -42,7 +43,7 @@ searchable_doc_for_collection, searchable_doc_for_container, searchable_doc_for_library_block, - searchable_doc_for_usage_key, + searchable_doc_for_key, searchable_doc_collections, searchable_doc_tags, searchable_doc_tags_for_collection, @@ -486,8 +487,7 @@ def index_container_batch(batch, num_done, library_key) -> int: container, ) doc = searchable_doc_for_container(container_key) - # TODO: when we add container tags - # doc.update(searchable_doc_tags_for_container(container_key)) + doc.update(searchable_doc_tags(container_key)) docs.append(doc) except Exception as err: # pylint: disable=broad-except status_cb(f"Error indexing container {container.key}: {err}") @@ -511,7 +511,7 @@ def index_container_batch(batch, num_done, library_key) -> int: collections = authoring_api.get_collections(library.learning_package_id, enabled=True) num_collections = collections.count() num_collections_done = 0 - status_cb(f"{num_collections_done + 1}/{num_collections}. Now indexing collections in library {lib_key}") + status_cb(f"{num_collections_done}/{num_collections}. Now indexing collections in library {lib_key}") paginator = Paginator(collections, 100) for p in paginator.page_range: num_collections_done = index_collection_batch( @@ -620,7 +620,7 @@ def delete_index_doc(usage_key: UsageKey) -> None: Args: usage_key (UsageKey): The usage key of the XBlock to be removed from the index """ - doc = searchable_doc_for_usage_key(usage_key) + doc = searchable_doc_for_key(usage_key) _delete_index_doc(doc[Fields.id]) @@ -811,12 +811,12 @@ def upsert_content_library_index_docs(library_key: LibraryLocatorV2) -> None: _update_index_docs(docs) -def upsert_block_tags_index_docs(usage_key: UsageKey): +def upsert_content_object_tags_index_doc(key: OpaqueKey): """ - Updates the tags data in documents for the given Course/Library block + Updates the tags data in document for the given Course/Library item """ - doc = {Fields.id: meili_id_from_opaque_key(usage_key)} - doc.update(searchable_doc_tags(usage_key)) + doc = {Fields.id: meili_id_from_opaque_key(key)} + doc.update(searchable_doc_tags(key)) _update_index_docs([doc]) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index 061015751955..c5821af34ee9 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -8,6 +8,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.utils.text import slugify +from opaque_keys import OpaqueKey from opaque_keys.edx.keys import LearningContextKey, UsageKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 from openedx_learning.api import authoring as authoring_api @@ -113,7 +114,7 @@ class PublishStatus: modified = "modified" -def meili_id_from_opaque_key(usage_key: UsageKey) -> str: +def meili_id_from_opaque_key(key: OpaqueKey) -> str: """ Meilisearch requires each document to have a primary key that's either an integer or a string composed of alphanumeric characters (a-z A-Z 0-9), @@ -124,7 +125,7 @@ def meili_id_from_opaque_key(usage_key: UsageKey) -> str: we could use PublishableEntity's primary key / UUID instead. """ # The slugified key _may_ not be unique so we append a hashed string to make it unique: - key_str = str(usage_key) + key_str = str(key) key_bin = key_str.encode() suffix = blake2b(key_bin, digest_size=4, usedforsecurity=False).hexdigest() @@ -140,12 +141,12 @@ def _meili_access_id_from_context_key(context_key: LearningContextKey) -> int: return access.id -def searchable_doc_for_usage_key(usage_key: UsageKey) -> dict: +def searchable_doc_for_key(key: OpaqueKey) -> dict: """ - Generates a base document identified by its usage key. + Generates a base document identified by its opaque key. """ return { - Fields.id: meili_id_from_opaque_key(usage_key), + Fields.id: meili_id_from_opaque_key(key), } @@ -244,7 +245,7 @@ class implementation returns only: return block_data -def _tags_for_content_object(object_id: UsageKey | LearningContextKey) -> dict: +def _tags_for_content_object(object_id: OpaqueKey) -> dict: """ Given an XBlock, course, library, etc., get the tag data for its index doc. @@ -406,7 +407,7 @@ def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetad block_published = None publish_status = PublishStatus.never - doc = searchable_doc_for_usage_key(xblock_metadata.usage_key) + doc = searchable_doc_for_key(xblock_metadata.usage_key) doc.update({ Fields.type: DocType.library_block, Fields.breadcrumbs: [], @@ -427,13 +428,13 @@ def searchable_doc_for_library_block(xblock_metadata: lib_api.LibraryXBlockMetad return doc -def searchable_doc_tags(usage_key: UsageKey) -> dict: +def searchable_doc_tags(key: OpaqueKey) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, with the tags data for the given content object. """ - doc = searchable_doc_for_usage_key(usage_key) - doc.update(_tags_for_content_object(usage_key)) + doc = searchable_doc_for_key(key) + doc.update(_tags_for_content_object(key)) return doc @@ -443,7 +444,7 @@ def searchable_doc_collections(usage_key: UsageKey) -> dict: Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, with the collections data for the given content object. """ - doc = searchable_doc_for_usage_key(usage_key) + doc = searchable_doc_for_key(usage_key) doc.update(_collections_for_content_object(usage_key)) return doc @@ -461,7 +462,7 @@ def searchable_doc_tags_for_collection( library_key, collection_key, ) - doc = searchable_doc_for_usage_key(collection_usage_key) + doc = searchable_doc_for_key(collection_usage_key) doc.update(_tags_for_content_object(collection_usage_key)) return doc @@ -473,7 +474,7 @@ def searchable_doc_for_course_block(block) -> dict: like Meilisearch or Elasticsearch, so that the given course block can be found using faceted search. """ - doc = searchable_doc_for_usage_key(block.usage_key) + doc = searchable_doc_for_key(block.usage_key) doc.update({ Fields.type: DocType.course_block, }) @@ -503,7 +504,7 @@ def searchable_doc_for_collection( collection_key, ) - doc = searchable_doc_for_usage_key(collection_usage_key) + doc = searchable_doc_for_key(collection_usage_key) try: collection = collection or lib_api.get_library_collection_from_usage_key(collection_usage_key) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index 4565165e87ea..f932c4e71c23 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -8,7 +8,7 @@ from django.dispatch import receiver from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import LibraryCollectionLocator +from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator from openedx_events.content_authoring.data import ( ContentLibraryData, ContentObjectChangedData, @@ -41,7 +41,7 @@ from .api import ( only_if_meilisearch_enabled, upsert_block_collections_index_docs, - upsert_block_tags_index_docs, + upsert_content_object_tags_index_doc, upsert_collection_tags_index_docs, ) from .tasks import ( @@ -211,15 +211,20 @@ def content_object_associations_changed_handler(**kwargs) -> None: return try: - # Check if valid if course or library block + # Check if valid course or library block usage_key = UsageKey.from_string(str(content_object.object_id)) except InvalidKeyError: try: - # Check if valid if library collection + # Check if valid library collection usage_key = LibraryCollectionLocator.from_string(str(content_object.object_id)) except InvalidKeyError: - log.error("Received invalid content object id") - return + try: + # Check if valid library container + usage_key = LibraryContainerLocator.from_string(str(content_object.object_id)) + except InvalidKeyError: + # Invalid content object id + log.error("Received invalid content object id") + return # This event's changes may contain both "tags" and "collections", but this will happen rarely, if ever. # So we allow a potential double "upsert" here. @@ -227,7 +232,7 @@ def content_object_associations_changed_handler(**kwargs) -> None: if isinstance(usage_key, LibraryCollectionLocator): upsert_collection_tags_index_docs(usage_key) else: - upsert_block_tags_index_docs(usage_key) + upsert_content_object_tags_index_doc(usage_key) if not content_object.changes or "collections" in content_object.changes: upsert_block_collections_index_docs(usage_key) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 813db0241db1..5786451d1fe2 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -234,7 +234,6 @@ def setUp(self): "modified": created_date.timestamp(), "access_id": lib_access.id, "breadcrumbs": [{"display_name": "Library"}], - # "tags" should be here but we haven't implemented them yet # "published" is not set since we haven't published it yet } @@ -262,6 +261,7 @@ def test_reindex_meilisearch(self, mock_meilisearch): doc_collection = copy.deepcopy(self.collection_dict) doc_collection["tags"] = {} doc_unit = copy.deepcopy(self.unit_dict) + doc_unit["tags"] = {} api.rebuild_index() assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 @@ -292,6 +292,7 @@ def test_reindex_meilisearch_incremental(self, mock_meilisearch): doc_collection = copy.deepcopy(self.collection_dict) doc_collection["tags"] = {} doc_unit = copy.deepcopy(self.unit_dict) + doc_unit["tags"] = {} api.rebuild_index(incremental=True) assert mock_meilisearch.return_value.index.return_value.add_documents.call_count == 4 @@ -472,8 +473,7 @@ def test_index_xblock_tags(self, mock_meilisearch): """ Test indexing an XBlock with tags. """ - - # Tag XBlock (these internally call `upsert_block_tags_index_docs`) + # Tag XBlock (these internally call `upsert_content_object_tags_index_doc`) tagging_api.tag_object(str(self.sequential.usage_key), self.taxonomyA, ["one", "two"]) tagging_api.tag_object(str(self.sequential.usage_key), self.taxonomyB, ["three", "four"]) @@ -866,3 +866,43 @@ def test_delete_collection(self, mock_meilisearch): mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([ doc_problem_without_collection, ]) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_library_container_metadata(self, mock_meilisearch): + """ + Test indexing a Library Container. + """ + api.upsert_library_container_index_doc(self.unit.container_key) + + mock_meilisearch.return_value.index.return_value.update_documents.assert_called_once_with([self.unit_dict]) + + @override_settings(MEILISEARCH_ENABLED=True) + def test_index_tags_in_containers(self, mock_meilisearch): + # Tag collection + tagging_api.tag_object(self.unit_key, self.taxonomyA, ["one", "two"]) + tagging_api.tag_object(self.unit_key, self.taxonomyB, ["three", "four"]) + + # Build expected docs with tags at each stage + doc_unit_with_tags1 = { + "id": "lctorg1libunitunit-1-e4527f7c", + "tags": { + 'taxonomy': ['A'], + 'level0': ['A > one', 'A > two'] + } + } + doc_unit_with_tags2 = { + "id": "lctorg1libunitunit-1-e4527f7c", + "tags": { + 'taxonomy': ['A', 'B'], + 'level0': ['A > one', 'A > two', 'B > four', 'B > three'] + } + } + + assert mock_meilisearch.return_value.index.return_value.update_documents.call_count == 2 + mock_meilisearch.return_value.index.return_value.update_documents.assert_has_calls( + [ + call([doc_unit_with_tags1]), + call([doc_unit_with_tags2]), + ], + any_order=True, + ) diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index a2964436d039..fb41e8efc278 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -87,6 +87,14 @@ def setUpClass(cls): "html", "text2", ) + cls.container = library_api.create_container( + cls.library.key, + container_type=library_api.ContainerType.Unit, + slug="unit1", + title="A Unit in the Search Index", + user_id=None, + ) + cls.container_usage_key = "lct:edX:2012_Fall:unit:unit1" # Add the problem block to the collection library_api.update_library_collection_components( @@ -116,6 +124,7 @@ def setUpClass(cls): tagging_api.tag_object(str(cls.html_block_key), cls.difficulty_tags, tags=["Normal"]) tagging_api.tag_object(str(cls.library_block.usage_key), cls.difficulty_tags, tags=["Normal"]) tagging_api.tag_object(cls.collection_usage_key, cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(cls.container_usage_key, cls.difficulty_tags, tags=["Normal"]) @property def toy_course_access_id(self): @@ -501,17 +510,8 @@ def test_draft_container(self): """ Test creating a search document for a draft-only container """ - created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) - with freeze_time(created_date): - container_meta = library_api.create_container( - self.library.key, - container_type=library_api.ContainerType.Unit, - slug="unit1", - title="A Unit in the Search Index", - user_id=None, - ) - - doc = searchable_doc_for_container(container_meta.container_key) + doc = searchable_doc_for_container(self.container.container_key) + doc.update(searchable_doc_tags(self.container.container_key)) assert doc == { "id": "lctedx2012_fallunitunit1-edd13a0c", @@ -529,7 +529,10 @@ def test_draft_container(self): "breadcrumbs": [{"display_name": "some content_library"}], "created": 1680674828.0, "modified": 1680674828.0, - # "tags" should be here but we haven't implemented them yet + "tags": { + "taxonomy": ["Difficulty"], + "level0": ["Difficulty > Normal"] + }, # "published" is not set since we haven't published it yet } @@ -537,23 +540,17 @@ def test_published_container(self): """ Test creating a search document for a published container """ - created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) - with freeze_time(created_date): - container_meta = library_api.create_container( - self.library.key, - container_type=library_api.ContainerType.Unit, - slug="unit1", - title="A Unit in the Search Index", - user_id=None, - ) + with freeze_time(self.container.created): + # Create a container with a block in it library_api.update_container_children( - container_meta.container_key, + self.container.container_key, [self.library_block.usage_key], user_id=None, ) library_api.publish_changes(self.library.key) - doc = searchable_doc_for_container(container_meta.container_key) + doc = searchable_doc_for_container(self.container.container_key) + doc.update(searchable_doc_tags(self.container.container_key)) assert doc == { "id": "lctedx2012_fallunitunit1-edd13a0c", @@ -571,29 +568,22 @@ def test_published_container(self): "breadcrumbs": [{"display_name": "some content_library"}], "created": 1680674828.0, "modified": 1680674828.0, + "tags": { + "taxonomy": ["Difficulty"], + "level0": ["Difficulty > Normal"] + }, "published": {"num_children": 1}, - # "tags" should be here but we haven't implemented them yet - # "published" is not set since we haven't published it yet } def test_published_container_with_changes(self): """ Test creating a search document for a published container """ - created_date = datetime(2023, 4, 5, 6, 7, 8, tzinfo=timezone.utc) - with freeze_time(created_date): - container_meta = library_api.create_container( - self.library.key, - container_type=library_api.ContainerType.Unit, - slug="unit1", - title="A Unit in the Search Index", - user_id=None, - ) - library_api.update_container_children( - container_meta.container_key, - [self.library_block.usage_key], - user_id=None, - ) + library_api.update_container_children( + self.container.container_key, + [self.library_block.usage_key], + user_id=None, + ) library_api.publish_changes(self.library.key) block_2 = library_api.create_library_block( self.library.key, @@ -602,15 +592,16 @@ def test_published_container_with_changes(self): ) # Add another component after publish - with freeze_time(created_date): + with freeze_time(self.container.created): library_api.update_container_children( - container_meta.container_key, + self.container.container_key, [block_2.usage_key], user_id=None, entities_action=authoring_api.ChildrenEntitiesAction.APPEND, ) - doc = searchable_doc_for_container(container_meta.container_key) + doc = searchable_doc_for_container(self.container.container_key) + doc.update(searchable_doc_tags(self.container.container_key)) assert doc == { "id": "lctedx2012_fallunitunit1-edd13a0c", @@ -628,9 +619,11 @@ def test_published_container_with_changes(self): "breadcrumbs": [{"display_name": "some content_library"}], "created": 1680674828.0, "modified": 1680674828.0, + "tags": { + "taxonomy": ["Difficulty"], + "level0": ["Difficulty > Normal"] + }, "published": {"num_children": 1}, - # "tags" should be here but we haven't implemented them yet - # "published" is not set since we haven't published it yet } def test_mathjax_plain_text_conversion_for_search(self): diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 5a99868ece66..6ba1049082d1 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -14,7 +14,7 @@ from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile from edx_django_utils.cache import RequestCache -from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryCollectionLocator +from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryCollectionLocator, LibraryContainerLocator from openedx_tagging.core.tagging.models import Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy from openedx_tagging.core.tagging.rest_api.v1.serializers import TaxonomySerializer @@ -114,6 +114,9 @@ def _setUp_library(self): def _setUp_collection(self): self.collection_key = str(LibraryCollectionLocator(self.content_libraryA.key, 'test-collection')) + def _setUp_container(self): + self.container_key = str(LibraryContainerLocator(self.content_libraryA.key, 'unit', 'unit1')) + def _setUp_users(self): """ Create users for testing @@ -288,6 +291,7 @@ def setUp(self): self._setUp_users() self._setUp_taxonomies() self._setUp_collection() + self._setUp_container() # Clear all request caches in between test runs to keep query counts consistent. RequestCache.clear_all_namespaces() @@ -1701,6 +1705,50 @@ def test_tag_collection(self, user_attr, taxonomy_attr, tag_values, expected_sta assert status.is_success(new_response.status_code) assert new_response.data == response.data + @ddt.data( + # staffA and staff are staff in collection and can tag using enabled taxonomies + ("user", "tA1", ["Tag 1"], status.HTTP_403_FORBIDDEN), + ("staffA", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("staff", "tA1", ["Tag 1"], status.HTTP_200_OK), + ("user", "tA1", [], status.HTTP_403_FORBIDDEN), + ("staffA", "tA1", [], status.HTTP_200_OK), + ("staff", "tA1", [], status.HTTP_200_OK), + ("user", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_403_FORBIDDEN), + ("staffA", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("staff", "multiple_taxonomy", ["Tag 1", "Tag 2"], status.HTTP_200_OK), + ("user", "open_taxonomy", ["tag1"], status.HTTP_403_FORBIDDEN), + ("staffA", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ("staff", "open_taxonomy", ["tag1"], status.HTTP_200_OK), + ) + @ddt.unpack + def test_tag_container(self, user_attr, taxonomy_attr, tag_values, expected_status): + """ + Tests that only staff and org level users can tag containers + """ + user = getattr(self, user_attr) + self.client.force_authenticate(user=user) + + taxonomy = getattr(self, taxonomy_attr) + + response = self._call_put_request(self.container_key, taxonomy.pk, tag_values) + + assert response.status_code == expected_status + if status.is_success(expected_status): + tags_by_taxonomy = response.data[str(self.container_key)]["taxonomies"] + if tag_values: + response_taxonomy = tags_by_taxonomy[0] + assert response_taxonomy["name"] == taxonomy.name + response_tags = response_taxonomy["tags"] + assert [t["value"] for t in response_tags] == tag_values + else: + assert tags_by_taxonomy == [] # No tags are set from any taxonomy + + # Check that re-fetching the tags returns what we set + url = OBJECT_TAG_UPDATE_URL.format(object_id=self.container_key) + new_response = self.client.get(url, format="json") + assert status.is_success(new_response.status_code) + assert new_response.data == response.data + @ddt.data( "staffA", "staff", diff --git a/openedx/core/djangoapps/content_tagging/tests/test_api.py b/openedx/core/djangoapps/content_tagging/tests/test_api.py index d85c87d62b17..2661c02fd15c 100644 --- a/openedx/core/djangoapps/content_tagging/tests/test_api.py +++ b/openedx/core/djangoapps/content_tagging/tests/test_api.py @@ -6,7 +6,7 @@ from django.test.testcases import TestCase from fs.osfs import OSFS from opaque_keys.edx.keys import CourseKey, UsageKey -from opaque_keys.edx.locator import LibraryLocatorV2, LibraryCollectionLocator +from opaque_keys.edx.locator import LibraryLocatorV2, LibraryCollectionLocator, LibraryContainerLocator from openedx_tagging.core.tagging.models import ObjectTag from organizations.models import Organization from .test_objecttag_export_helpers import TestGetAllObjectTagsMixin, TaggedCourseMixin @@ -397,6 +397,23 @@ def test_tag_collection(self): self.taxonomy_3.id: self.taxonomy_3, } + def test_tag_container(self): + unit_key = LibraryContainerLocator.from_string('lct:orgA:libX:unit:unit1') + + api.tag_object( + object_id=str(unit_key), + taxonomy=self.taxonomy_3, + tags=["Tag 3.1"], + ) + + with self.assertNumQueries(1): + object_tags, taxonomies = api.get_all_object_tags(unit_key) + + assert object_tags == {'lct:orgA:libX:unit:unit1': {3: ['Tag 3.1']}} + assert taxonomies == { + self.taxonomy_3.id: self.taxonomy_3, + } + class TestExportImportTags(TaggedCourseMixin): """ From 30b55779b28f8500d253e910552dee9df1a823c9 Mon Sep 17 00:00:00 2001 From: Hassan Raza Date: Thu, 10 Apr 2025 13:32:54 +0500 Subject: [PATCH 13/23] fix: Enable ORA new submissions course notification preference (#36449) * fix: Enable ORA new submissions course notification preference * fix: update edx-ora2 version --- openedx/core/djangoapps/notifications/base_notification.py | 6 +++--- openedx/core/djangoapps/notifications/models.py | 2 +- openedx/core/djangoapps/notifications/tests/test_views.py | 6 +++--- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/notifications/base_notification.py b/openedx/core/djangoapps/notifications/base_notification.py index 17ec8cda45d7..899046e440ed 100644 --- a/openedx/core/djangoapps/notifications/base_notification.py +++ b/openedx/core/djangoapps/notifications/base_notification.py @@ -191,12 +191,12 @@ 'email_template': '', 'filters': [FILTER_AUDIT_EXPIRED_USERS_WITH_NO_ROLE] }, - 'ora_staff_notification': { + 'ora_staff_notifications': { 'notification_app': 'grading', - 'name': 'ora_staff_notification', + 'name': 'ora_staff_notifications', 'is_core': False, 'info': '', - 'web': False, + 'web': True, 'email': False, 'push': False, 'email_cadence': EmailCadence.DAILY, diff --git a/openedx/core/djangoapps/notifications/models.py b/openedx/core/djangoapps/notifications/models.py index ed9c58e50698..e3bad0016ee8 100644 --- a/openedx/core/djangoapps/notifications/models.py +++ b/openedx/core/djangoapps/notifications/models.py @@ -25,7 +25,7 @@ ADDITIONAL_NOTIFICATION_CHANNEL_SETTINGS = ['email_cadence'] # Update this version when there is a change to any course specific notification type or app. -COURSE_NOTIFICATION_CONFIG_VERSION = 12 +COURSE_NOTIFICATION_CONFIG_VERSION = 13 def get_course_notification_preference_config(): diff --git a/openedx/core/djangoapps/notifications/tests/test_views.py b/openedx/core/djangoapps/notifications/tests/test_views.py index 952cda06d528..e624995b050a 100644 --- a/openedx/core/djangoapps/notifications/tests/test_views.py +++ b/openedx/core/djangoapps/notifications/tests/test_views.py @@ -369,8 +369,8 @@ def _expected_api_response(self, course=None): 'enabled': True, 'core_notification_types': [], 'notification_types': { - 'ora_staff_notification': { - 'web': False, + 'ora_staff_notifications': { + 'web': True, 'email': False, 'push': False, 'email_cadence': 'Daily', @@ -1088,7 +1088,7 @@ def setUp(self): "email": True, "email_cadence": "Daily" }, - "ora_staff_notification": { + "ora_staff_notifications": { "web": False, "push": False, "email": False, diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 13f8da5438cc..29c056d6712f 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -828,7 +828,7 @@ openedx-mongodbproxy==0.2.2 # via -r requirements/edx/kernel.in optimizely-sdk==5.2.0 # via -r requirements/edx/bundled.in -ora2==6.15.0 +ora2==6.15.1 # via -r requirements/edx/bundled.in packaging==24.2 # via diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 76d63f0cb0d8..d7f7674bd6f6 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1396,7 +1396,7 @@ optimizely-sdk==5.2.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -ora2==6.15.0 +ora2==6.15.1 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 099fafb7bd67..d3372e8dc678 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -1000,7 +1000,7 @@ openedx-mongodbproxy==0.2.2 # via -r requirements/edx/base.txt optimizely-sdk==5.2.0 # via -r requirements/edx/base.txt -ora2==6.15.0 +ora2==6.15.1 # via -r requirements/edx/base.txt packaging==24.2 # via diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 4edc93f95fee..174a06ed72f3 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1058,7 +1058,7 @@ openedx-mongodbproxy==0.2.2 # via -r requirements/edx/base.txt optimizely-sdk==5.2.0 # via -r requirements/edx/base.txt -ora2==6.15.0 +ora2==6.15.1 # via -r requirements/edx/base.txt packaging==24.2 # via From 75d66b5efe7b0a95fa1a0ab2a49a5ea513ad070c Mon Sep 17 00:00:00 2001 From: Muhammad Umar Khan <42294172+mumarkhan999@users.noreply.github.com> Date: Thu, 10 Apr 2025 15:22:28 +0500 Subject: [PATCH 14/23] chore: fix cache content size calculation error (#36511) Co-authored-by: M Umar Khan --- lms/djangoapps/courseware/courses.py | 5 +++-- openedx/core/djangoapps/content/block_structure/store.py | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 62bd6bf5c982..5dc4a7ce9107 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -795,10 +795,11 @@ def get_assignments_grades(user, course_id, cache_timeout): collected_block_structure = get_block_structure_manager(course_id).get_collected() total_bytes_in_one_mb = 1024 * 1024 - data_size_in_mbs = round(len(pickle.dumps(collected_block_structure)) / total_bytes_in_one_mb, 2) - if data_size_in_mbs < total_bytes_in_one_mb * 2: + data_size_in_bytes = len(pickle.dumps(collected_block_structure)) + if data_size_in_bytes < total_bytes_in_one_mb * 2: cache.set(cache_key, collected_block_structure, cache_timeout) else: + data_size_in_mbs = round(data_size_in_bytes / total_bytes_in_one_mb, 2) # .. custom_attribute_name: collected_block_structure_size_in_mbs # .. custom_attribute_description: contains the data chunk size in MBs. The size on which # the memcached client failed to store value in cache. diff --git a/openedx/core/djangoapps/content/block_structure/store.py b/openedx/core/djangoapps/content/block_structure/store.py index 409dae988e92..bb6359b9d3a3 100644 --- a/openedx/core/djangoapps/content/block_structure/store.py +++ b/openedx/core/djangoapps/content/block_structure/store.py @@ -137,8 +137,9 @@ def _add_to_cache(self, serialized_data, bs_model): """ cache_key = self._encode_root_cache_key(bs_model) total_bytes_in_one_mb = 1024 * 1024 - data_size_in_mbs = round(len(serialized_data) / total_bytes_in_one_mb, 2) - if data_size_in_mbs < total_bytes_in_one_mb * 2: + data_size_in_bytes = len(serialized_data) + data_size_in_mbs = round(data_size_in_bytes / total_bytes_in_one_mb, 2) + if data_size_in_bytes < total_bytes_in_one_mb * 2: self._cache.set(cache_key, serialized_data, timeout=config.cache_timeout_in_seconds()) logger.info("BlockStructure: Added to cache; %s, size: %.2fMB", bs_model, data_size_in_mbs) else: From 53a2471ca4da7cc528ce971cb53c04c7f6c85125 Mon Sep 17 00:00:00 2001 From: Irtaza Akram <51848298+irtazaakram@users.noreply.github.com> Date: Thu, 10 Apr 2025 20:01:38 +0500 Subject: [PATCH 15/23] fix: duplicate rendering of instructions in annotatable xblock (#36445) In the Annotatable XBlock, the element was appearing twice in the student view: * Once in "annotatable-instructions" (where it should be). * Again in "annotatable-content" (where annotations and other content are rendered). The _render_content method processed and rendered the entire XML data, including , without removing it. The _extract_instructions method, which is responsible for removing , was not called in _render_content, leading to duplication. This fix will: * Prevents duplicate instructions in the student view. * Maintains the expected behavior of showing instructions only in "annotatable-instructions". * No impact on existing annotation functionality. --- xmodule/annotatable_block.py | 3 +-- xmodule/tests/test_annotatable_block.py | 8 ++++++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/xmodule/annotatable_block.py b/xmodule/annotatable_block.py index dbcf0123c59a..9dcc27198d5d 100644 --- a/xmodule/annotatable_block.py +++ b/xmodule/annotatable_block.py @@ -140,9 +140,8 @@ def _render_content(self): """ Renders annotatable content with annotation spans and returns HTML. """ xmltree = etree.fromstring(self.data) - content = etree.tostring(xmltree, encoding='unicode') + self._extract_instructions(xmltree) - xmltree = etree.fromstring(content) xmltree.tag = 'div' if 'display_name' in xmltree.attrib: del xmltree.attrib['display_name'] diff --git a/xmodule/tests/test_annotatable_block.py b/xmodule/tests/test_annotatable_block.py index b9f419ee3a0f..3de253047a8d 100644 --- a/xmodule/tests/test_annotatable_block.py +++ b/xmodule/tests/test_annotatable_block.py @@ -134,3 +134,11 @@ def test_extract_instructions(self): xmltree = etree.fromstring('foo') actual = self.annotatable._extract_instructions(xmltree) # lint-amnesty, pylint: disable=protected-access assert actual is None + + def test_instruction_removal(self): + xmltree = etree.fromstring(self.sample_xml) + instructions = self.annotatable._extract_instructions(xmltree) # pylint: disable=protected-access + + assert instructions is not None + assert "Read the text." in instructions + assert xmltree.find("instructions") is None From bedcee5d0ccba7f38fc626a53e067a3553168415 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Chris=20Ch=C3=A1vez?= Date: Thu, 10 Apr 2025 17:15:53 -0500 Subject: [PATCH 16/23] feat: add restore container API, delete index when deleting container (#36464) --- openedx/core/djangoapps/content/search/api.py | 9 ++-- .../djangoapps/content/search/documents.py | 3 +- .../djangoapps/content/search/handlers.py | 18 ++++++- .../core/djangoapps/content/search/tasks.py | 19 ++++++- .../content/search/tests/test_api.py | 11 ++++ .../content_libraries/api/containers.py | 26 +++++++--- .../content_libraries/rest_api/containers.py | 20 ++++++++ .../content_libraries/tests/base.py | 5 ++ .../tests/test_containers.py | 50 +++++++++++++++++++ .../core/djangoapps/content_libraries/urls.py | 2 + 10 files changed, 148 insertions(+), 15 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index d9bcc55cd9d7..31db56bbf26d 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -17,8 +17,7 @@ from meilisearch import Client as MeilisearchClient from meilisearch.errors import MeilisearchApiError, MeilisearchError from meilisearch.models.task import TaskInfo -from opaque_keys import OpaqueKey -from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.keys import UsageKey, OpaqueKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryCollectionLocator from openedx_learning.api import authoring as authoring_api from common.djangoapps.student.roles import GlobalStaff @@ -613,14 +612,14 @@ def add_with_children(block): _update_index_docs(docs) -def delete_index_doc(usage_key: UsageKey) -> None: +def delete_index_doc(key: OpaqueKey) -> None: """ Deletes the document for the given XBlock from the search index Args: - usage_key (UsageKey): The usage key of the XBlock to be removed from the index + key (OpaqueKey): The opaque key of the XBlock/Container to be removed from the index """ - doc = searchable_doc_for_key(usage_key) + doc = searchable_doc_for_key(key) _delete_index_doc(doc[Fields.id]) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index c5821af34ee9..dbb55cb755c6 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -8,8 +8,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.utils.text import slugify -from opaque_keys import OpaqueKey -from opaque_keys.edx.keys import LearningContextKey, UsageKey +from opaque_keys.edx.keys import LearningContextKey, UsageKey, OpaqueKey from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Collection diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index f932c4e71c23..ba7e58eb800c 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -46,6 +46,7 @@ ) from .tasks import ( delete_library_block_index_doc, + delete_library_container_index_doc, delete_xblock_index_doc, update_content_library_index_docs, update_library_collection_index_doc, @@ -238,7 +239,6 @@ def content_object_associations_changed_handler(**kwargs) -> None: @receiver(LIBRARY_CONTAINER_CREATED) -@receiver(LIBRARY_CONTAINER_DELETED) @receiver(LIBRARY_CONTAINER_UPDATED) @only_if_meilisearch_enabled def library_container_updated_handler(**kwargs) -> None: @@ -263,3 +263,19 @@ def library_container_updated_handler(**kwargs) -> None: str(library_container.library_key), library_container.container_key, ]) + + +@receiver(LIBRARY_CONTAINER_DELETED) +@only_if_meilisearch_enabled +def library_container_deleted(**kwargs) -> None: + """ + Delete the index for the content library container + """ + library_container = kwargs.get("library_container", None) + if not library_container or not isinstance(library_container, LibraryContainerData): # pragma: no cover + log.error("Received null or incorrect data for event") + return + + # Update content library index synchronously to make sure that search index is updated before + # the frontend invalidates/refetches results. This is only a single document update so is very fast. + delete_library_container_index_doc.apply(args=[str(library_container.container_key)]) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index f23ca9aa304e..1c66050415e3 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -11,7 +11,11 @@ from edx_django_utils.monitoring import set_code_owner_attribute from meilisearch.errors import MeilisearchError from opaque_keys.edx.keys import UsageKey -from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2 +from opaque_keys.edx.locator import ( + LibraryContainerLocator, + LibraryLocatorV2, + LibraryUsageLocatorV2, +) from . import api @@ -124,3 +128,16 @@ def update_library_container_index_doc(library_key_str: str, container_key_str: log.info("Updating content index documents for container %s in library%s", container_key, library_key) api.upsert_library_container_index_doc(container_key) + + +@shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) +@set_code_owner_attribute +def delete_library_container_index_doc(container_key_str: str) -> None: + """ + Celery task to delete the content index document for a library block + """ + container_key = LibraryContainerLocator.from_string(container_key_str) + + log.info("Deleting content index document for library block with id: %s", container_key) + + api.delete_index_doc(container_key) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 5786451d1fe2..63317d020ff2 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -867,6 +867,17 @@ def test_delete_collection(self, mock_meilisearch): doc_problem_without_collection, ]) + @override_settings(MEILISEARCH_ENABLED=True) + def test_delete_index_container(self, mock_meilisearch): + """ + Test delete a container index. + """ + library_api.delete_container(self.unit.container_key) + + mock_meilisearch.return_value.index.return_value.delete_document.assert_called_once_with( + self.unit_dict["id"], + ) + @override_settings(MEILISEARCH_ENABLED=True) def test_index_library_container_metadata(self, mock_meilisearch): """ diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index bf519901ae57..8a80cec352aa 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -44,6 +44,7 @@ "library_container_locator", "update_container", "delete_container", + "restore_container", "update_container_children", "get_containers_contains_component", ] @@ -121,7 +122,7 @@ def library_container_locator( ) -def _get_container(container_key: LibraryContainerLocator) -> Container: +def _get_container(container_key: LibraryContainerLocator, isDeleted=False) -> Container: """ Internal method to fetch the Container object from its LibraryContainerLocator @@ -135,7 +136,7 @@ def _get_container(container_key: LibraryContainerLocator) -> Container: learning_package.id, key=container_key.container_id, ) - if container and container.versioning.draft: + if container and (isDeleted or container.versioning.draft): return container raise ContentLibraryContainerNotFound @@ -234,10 +235,7 @@ def delete_container( No-op if container doesn't exist or has already been soft-deleted. """ - try: - container = _get_container(container_key) - except ContentLibraryContainerNotFound: - return + container = _get_container(container_key) authoring_api.soft_delete_draft(container.pk) @@ -251,6 +249,22 @@ def delete_container( # TODO: trigger a LIBRARY_COLLECTION_UPDATED for each collection the container was in +def restore_container(container_key: LibraryContainerLocator) -> None: + """ + Restore the specified library container. + """ + container = _get_container(container_key, isDeleted=True) + + authoring_api.set_draft_version(container.pk, container.versioning.latest.pk) + + LIBRARY_CONTAINER_CREATED.send_event( + library_container=LibraryContainerData( + library_key=container_key.library_key, + container_key=str(container_key), + ) + ) + + def get_container_children( container_key: LibraryContainerLocator, published=False, diff --git a/openedx/core/djangoapps/content_libraries/rest_api/containers.py b/openedx/core/djangoapps/content_libraries/rest_api/containers.py index 95e468b4a43a..3c65fc006bf6 100644 --- a/openedx/core/djangoapps/content_libraries/rest_api/containers.py +++ b/openedx/core/djangoapps/content_libraries/rest_api/containers.py @@ -273,3 +273,23 @@ def patch(self, request, container_key: LibraryContainerLocator): container_key, action=authoring_api.ChildrenEntitiesAction.REPLACE, ) + + +@method_decorator(non_atomic_requests, name="dispatch") +@view_auth_classes() +class LibraryContainerRestore(GenericAPIView): + """ + View to restore soft-deleted library containers. + """ + @convert_exceptions + def post(self, request, container_key: LibraryContainerLocator) -> Response: + """ + Restores a soft-deleted library container + """ + api.require_permission_for_library_key( + container_key.library_key, + request.user, + permissions.CAN_EDIT_THIS_CONTENT_LIBRARY, + ) + api.restore_container(container_key) + return Response(None, status=HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 6adb8184ea00..3c07aabe06c4 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -34,6 +34,7 @@ URL_LIB_BLOCK_ASSET_FILE = URL_LIB_BLOCK + 'assets/{file_name}' # Get, delete, or upload a specific static asset file URL_LIB_CONTAINER = URL_PREFIX + 'containers/{container_key}/' # Get a container in this library URL_LIB_CONTAINER_COMPONENTS = URL_LIB_CONTAINER + 'children/' # Get, add or delete a component in this container +URL_LIB_CONTAINER_RESTORE = URL_LIB_CONTAINER + 'restore/' # Restore a deleted container URL_LIB_LTI_PREFIX = URL_PREFIX + 'lti/1.3/' URL_LIB_LTI_JWKS = URL_LIB_LTI_PREFIX + 'pub/jwks/' @@ -386,6 +387,10 @@ def _delete_container(self, container_key: str, expect_response=204): """ Delete a container (unit etc.) """ return self._api('delete', URL_LIB_CONTAINER.format(container_key=container_key), None, expect_response) + def _restore_container(self, container_key: str, expect_response=204): + """ Restore a deleted a container (unit etc.) """ + return self._api('post', URL_LIB_CONTAINER_RESTORE.format(container_key=container_key), None, expect_response) + def _get_container_components(self, container_key: str, expect_response=200): """ Get container components""" return self._api( diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 52546396f2bb..46d206af5008 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -330,3 +330,53 @@ def test_unit_replace_children(self): }, update_receiver.call_args_list[0].kwargs, ) + + def test_restore_unit(self): + """ + Test restore a deleted unit. + """ + lib = self._create_library(slug="containers", title="Container Test Library", description="Units and more") + lib_key = LibraryLocatorV2.from_string(lib["id"]) + + # Create a unit: + create_date = datetime(2024, 9, 8, 7, 6, 5, tzinfo=timezone.utc) + with freeze_time(create_date): + container_data = self._create_container(lib["id"], "unit", slug="u1", display_name="Test Unit") + + # Delete the unit + self._delete_container(container_data["container_key"]) + + create_receiver = mock.Mock() + LIBRARY_CONTAINER_CREATED.connect(create_receiver) + + # Restore container + self._restore_container(container_data["container_key"]) + new_container_data = self._get_container(container_data["container_key"]) + expected_data = { + "container_key": "lct:CL-TEST:containers:unit:u1", + "container_type": "unit", + "display_name": "Test Unit", + "last_published": None, + "published_by": "", + "last_draft_created": "2024-09-08T07:06:05Z", + "last_draft_created_by": 'Bob', + 'has_unpublished_changes': True, + 'created': '2024-09-08T07:06:05Z', + 'modified': '2024-09-08T07:06:05Z', + 'collections': [], + } + + self.assertDictContainsEntries(new_container_data, expected_data) + + assert create_receiver.call_count == 1 + self.assertDictContainsSubset( + { + "signal": LIBRARY_CONTAINER_CREATED, + "sender": None, + "library_container": LibraryContainerData( + lib_key, + container_key="lct:CL-TEST:containers:unit:u1", + ), + }, + create_receiver.call_args_list[0].kwargs, + ) diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 99e98a2cc424..c46cc8641cd8 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -82,6 +82,8 @@ path('', containers.LibraryContainerView.as_view()), # update components under container path('children/', containers.LibraryContainerChildrenView.as_view()), + # Restore a soft-deleted container + path('restore/', containers.LibraryContainerRestore.as_view()), # Update collections for a given container # path('collections/', views.LibraryContainerCollectionsView.as_view(), name='update-collections-ct'), # path('publish/', views.LibraryContainerPublishView.as_view()), From 6f2c21a8e6456ae7393d09b50ebc08378a373469 Mon Sep 17 00:00:00 2001 From: Jillian Date: Sat, 12 Apr 2025 00:51:58 +0930 Subject: [PATCH 17/23] refactor!: use LibraryCollectionLocator and LibraryContainerLocator [FC-0083] (#36476) efactors the content_libraries.api to use LibraryCollectionLocator and LibraryContainerLocator keys, instead of passing separate LibraryUsageKeyV2 keys along with the collection/container locators. Renames misleading uses of collection_usage_key to collection_locator, including in the content_libraries.api and content.search.api method names and parameters. This change impacts Developers, but should not affect end users. This refactoring seems reasonable to do without going through deprecation, given the minimal use of these APIs. --- openedx/core/djangoapps/content/search/api.py | 30 +++++---- .../djangoapps/content/search/documents.py | 35 ++++------ .../djangoapps/content/search/handlers.py | 12 ++-- .../core/djangoapps/content/search/tasks.py | 19 +++--- .../content/search/tests/test_api.py | 11 ++-- .../content/search/tests/test_documents.py | 25 ++++--- .../content_libraries/api/blocks.py | 22 ++++--- .../content_libraries/api/collections.py | 21 +++--- .../content_libraries/api/containers.py | 13 ++-- .../content_libraries/api/libraries.py | 7 +- .../content_libraries/library_context.py | 3 +- .../content_libraries/signal_handlers.py | 20 ++++-- .../content_libraries/tests/test_api.py | 66 ++++++++++++------- .../tests/test_containers.py | 30 +++++---- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 18 files changed, 176 insertions(+), 146 deletions(-) diff --git a/openedx/core/djangoapps/content/search/api.py b/openedx/core/djangoapps/content/search/api.py index 31db56bbf26d..49bf4be7687d 100644 --- a/openedx/core/djangoapps/content/search/api.py +++ b/openedx/core/djangoapps/content/search/api.py @@ -18,7 +18,11 @@ from meilisearch.errors import MeilisearchApiError, MeilisearchError from meilisearch.models.task import TaskInfo from opaque_keys.edx.keys import UsageKey, OpaqueKey -from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2, LibraryCollectionLocator +from opaque_keys.edx.locator import ( + LibraryCollectionLocator, + LibraryContainerLocator, + LibraryLocatorV2, +) from openedx_learning.api import authoring as authoring_api from common.djangoapps.student.roles import GlobalStaff from rest_framework.request import Request @@ -461,8 +465,9 @@ def index_collection_batch(batch, num_done, library_key) -> int: docs = [] for collection in batch: try: - doc = searchable_doc_for_collection(library_key, collection.key, collection=collection) - doc.update(searchable_doc_tags_for_collection(library_key, collection.key)) + collection_key = lib_api.library_collection_locator(library_key, collection.key) + doc = searchable_doc_for_collection(collection_key, collection=collection) + doc.update(searchable_doc_tags_for_collection(collection_key)) docs.append(doc) except Exception as err: # pylint: disable=broad-except status_cb(f"Error indexing collection {collection}: {err}") @@ -702,13 +707,13 @@ def _get_document_from_index(document_id: str) -> dict: return document -def upsert_library_collection_index_doc(library_key: LibraryLocatorV2, collection_key: str) -> None: +def upsert_library_collection_index_doc(collection_key: LibraryCollectionLocator) -> None: """ Creates, updates, or deletes the document for the given Library Collection in the search index. If the Collection is not found or disabled (i.e. soft-deleted), then delete it from the search index. """ - doc = searchable_doc_for_collection(library_key, collection_key) + doc = searchable_doc_for_collection(collection_key) update_components = False # Soft-deleted/disabled collections are removed from the index @@ -738,12 +743,11 @@ def upsert_library_collection_index_doc(library_key: LibraryLocatorV2, collectio if update_components: from .tasks import update_library_components_collections as update_task - update_task.delay(str(library_key), collection_key) + update_task.delay(str(collection_key)) def update_library_components_collections( - library_key: LibraryLocatorV2, - collection_key: str, + collection_key: LibraryCollectionLocator, batch_size: int = 1000, ) -> None: """ @@ -751,8 +755,12 @@ def update_library_components_collections( Because there may be a lot of components, we send these updates to Meilisearch in batches. """ + library_key = collection_key.library_key library = lib_api.get_library(library_key) - components = authoring_api.get_collection_components(library.learning_package_id, collection_key) + components = authoring_api.get_collection_components( + library.learning_package_id, + collection_key.collection_id, + ) paginator = Paginator(components, batch_size) for page in paginator.page_range: @@ -828,12 +836,12 @@ def upsert_block_collections_index_docs(usage_key: UsageKey): _update_index_docs([doc]) -def upsert_collection_tags_index_docs(collection_usage_key: LibraryCollectionLocator): +def upsert_collection_tags_index_docs(collection_key: LibraryCollectionLocator): """ Updates the tags data in documents for the given library collection """ - doc = searchable_doc_tags_for_collection(collection_usage_key.library_key, collection_usage_key.collection_id) + doc = searchable_doc_tags_for_collection(collection_key) _update_index_docs([doc]) diff --git a/openedx/core/djangoapps/content/search/documents.py b/openedx/core/djangoapps/content/search/documents.py index dbb55cb755c6..1c093034271d 100644 --- a/openedx/core/djangoapps/content/search/documents.py +++ b/openedx/core/djangoapps/content/search/documents.py @@ -9,7 +9,7 @@ from django.core.exceptions import ObjectDoesNotExist from django.utils.text import slugify from opaque_keys.edx.keys import LearningContextKey, UsageKey, OpaqueKey -from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator from openedx_learning.api import authoring as authoring_api from openedx_learning.api.authoring_models import Collection from rest_framework.exceptions import NotFound @@ -450,19 +450,14 @@ def searchable_doc_collections(usage_key: UsageKey) -> dict: def searchable_doc_tags_for_collection( - library_key: LibraryLocatorV2, - collection_key: str, + collection_key: LibraryCollectionLocator ) -> dict: """ Generate a dictionary document suitable for ingestion into a search engine like Meilisearch or Elasticsearch, with the tags data for the given library collection. """ - collection_usage_key = lib_api.get_library_collection_usage_key( - library_key, - collection_key, - ) - doc = searchable_doc_for_key(collection_usage_key) - doc.update(_tags_for_content_object(collection_usage_key)) + doc = searchable_doc_for_key(collection_key) + doc.update(_tags_for_content_object(collection_key)) return doc @@ -484,8 +479,7 @@ def searchable_doc_for_course_block(block) -> dict: def searchable_doc_for_collection( - library_key: LibraryLocatorV2, - collection_key: str, + collection_key: LibraryCollectionLocator, *, # Optionally provide the collection if we've already fetched one collection: Collection | None = None, @@ -498,21 +492,16 @@ def searchable_doc_for_collection( If no collection is found for the given library_key + collection_key, the returned document will contain only basic information derived from the collection usage key, and no Fields.type value will be included in the returned dict. """ - collection_usage_key = lib_api.get_library_collection_usage_key( - library_key, - collection_key, - ) - - doc = searchable_doc_for_key(collection_usage_key) + doc = searchable_doc_for_key(collection_key) try: - collection = collection or lib_api.get_library_collection_from_usage_key(collection_usage_key) + collection = collection or lib_api.get_library_collection_from_locator(collection_key) except lib_api.ContentLibraryCollectionNotFound: # Collection not found, so we can only return the base doc pass if collection: - assert collection.key == collection_key + assert collection.key == collection_key.collection_id draft_num_children = authoring_api.filter_publishable_entities( collection.entities, @@ -524,9 +513,9 @@ def searchable_doc_for_collection( ).count() doc.update({ - Fields.context_key: str(library_key), - Fields.org: str(library_key.org), - Fields.usage_key: str(collection_usage_key), + Fields.context_key: str(collection_key.library_key), + Fields.org: str(collection_key.org), + Fields.usage_key: str(collection_key), Fields.block_id: collection.key, Fields.type: DocType.collection, Fields.display_name: collection.title, @@ -537,7 +526,7 @@ def searchable_doc_for_collection( Fields.published: { Fields.published_num_children: published_num_children, }, - Fields.access_id: _meili_access_id_from_context_key(library_key), + Fields.access_id: _meili_access_id_from_context_key(collection_key.library_key), Fields.breadcrumbs: [{"display_name": collection.learning_package.title}], }) diff --git a/openedx/core/djangoapps/content/search/handlers.py b/openedx/core/djangoapps/content/search/handlers.py index ba7e58eb800c..d817de1519bd 100644 --- a/openedx/core/djangoapps/content/search/handlers.py +++ b/openedx/core/djangoapps/content/search/handlers.py @@ -187,16 +187,14 @@ def library_collection_updated_handler(**kwargs) -> None: if library_collection.background: update_library_collection_index_doc.delay( - str(library_collection.library_key), - library_collection.collection_key, + str(library_collection.collection_key), ) else: # Update collection index synchronously to make sure that search index is updated before # the frontend invalidates/refetches index. # See content_library_updated_handler for more details. update_library_collection_index_doc.apply(args=[ - str(library_collection.library_key), - library_collection.collection_key, + str(library_collection.collection_key), ]) @@ -252,16 +250,14 @@ def library_container_updated_handler(**kwargs) -> None: if library_container.background: update_library_container_index_doc.delay( - str(library_container.library_key), - library_container.container_key, + str(library_container.container_key), ) else: # Update container index synchronously to make sure that search index is updated before # the frontend invalidates/refetches index. # See content_library_updated_handler for more details. update_library_container_index_doc.apply(args=[ - str(library_container.library_key), - library_container.container_key, + str(library_container.container_key), ]) diff --git a/openedx/core/djangoapps/content/search/tasks.py b/openedx/core/djangoapps/content/search/tasks.py index 1c66050415e3..48fc6b327c0a 100644 --- a/openedx/core/djangoapps/content/search/tasks.py +++ b/openedx/core/djangoapps/content/search/tasks.py @@ -12,6 +12,7 @@ from meilisearch.errors import MeilisearchError from opaque_keys.edx.keys import UsageKey from opaque_keys.edx.locator import ( + LibraryCollectionLocator, LibraryContainerLocator, LibraryLocatorV2, LibraryUsageLocatorV2, @@ -92,38 +93,40 @@ def update_content_library_index_docs(library_key_str: str) -> None: @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def update_library_collection_index_doc(library_key_str: str, collection_key: str) -> None: +def update_library_collection_index_doc(collection_key_str: str) -> None: """ Celery task to update the content index document for a library collection """ - library_key = LibraryLocatorV2.from_string(library_key_str) + collection_key = LibraryCollectionLocator.from_string(collection_key_str) + library_key = collection_key.library_key log.info("Updating content index documents for collection %s in library%s", collection_key, library_key) - api.upsert_library_collection_index_doc(library_key, collection_key) + api.upsert_library_collection_index_doc(collection_key) @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def update_library_components_collections(library_key_str: str, collection_key: str) -> None: +def update_library_components_collections(collection_key_str: str) -> None: """ Celery task to update the "collections" field for components in the given content library collection. """ - library_key = LibraryLocatorV2.from_string(library_key_str) + collection_key = LibraryCollectionLocator.from_string(collection_key_str) + library_key = collection_key.library_key log.info("Updating document.collections for library %s collection %s components", library_key, collection_key) - api.update_library_components_collections(library_key, collection_key) + api.update_library_components_collections(collection_key) @shared_task(base=LoggedTask, autoretry_for=(MeilisearchError, ConnectionError)) @set_code_owner_attribute -def update_library_container_index_doc(library_key_str: str, container_key_str: str) -> None: +def update_library_container_index_doc(container_key_str: str) -> None: """ Celery task to update the content index document for a library container """ - library_key = LibraryLocatorV2.from_string(library_key_str) container_key = LibraryContainerLocator.from_string(container_key_str) + library_key = container_key.library_key log.info("Updating content index documents for container %s in library%s", container_key, library_key) diff --git a/openedx/core/djangoapps/content/search/tests/test_api.py b/openedx/core/djangoapps/content/search/tests/test_api.py index 63317d020ff2..bd9fb803c1aa 100644 --- a/openedx/core/djangoapps/content/search/tests/test_api.py +++ b/openedx/core/djangoapps/content/search/tests/test_api.py @@ -8,6 +8,7 @@ from datetime import datetime, timezone from unittest.mock import MagicMock, Mock, call, patch from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.locator import LibraryCollectionLocator import ddt import pytest @@ -188,11 +189,13 @@ def setUp(self): created_by=None, description="my collection description" ) - self.collection_usage_key = "lib-collection:org1:lib:MYCOL" + self.collection_key = LibraryCollectionLocator.from_string( + "lib-collection:org1:lib:MYCOL", + ) self.collection_dict = { "id": "lib-collectionorg1libmycol-5b647617", "block_id": self.collection.key, - "usage_key": self.collection_usage_key, + "usage_key": str(self.collection_key), "type": "collection", "display_name": "my_collection", "description": "my collection description", @@ -737,8 +740,8 @@ def test_delete_all_drafts(self, mock_meilisearch): @override_settings(MEILISEARCH_ENABLED=True) def test_index_tags_in_collections(self, mock_meilisearch): # Tag collection - tagging_api.tag_object(self.collection_usage_key, self.taxonomyA, ["one", "two"]) - tagging_api.tag_object(self.collection_usage_key, self.taxonomyB, ["three", "four"]) + tagging_api.tag_object(str(self.collection_key), self.taxonomyA, ["one", "two"]) + tagging_api.tag_object(str(self.collection_key), self.taxonomyB, ["three", "four"]) # Build expected docs with tags at each stage doc_collection_with_tags1 = { diff --git a/openedx/core/djangoapps/content/search/tests/test_documents.py b/openedx/core/djangoapps/content/search/tests/test_documents.py index fb41e8efc278..1f6c56f7d691 100644 --- a/openedx/core/djangoapps/content/search/tests/test_documents.py +++ b/openedx/core/djangoapps/content/search/tests/test_documents.py @@ -5,6 +5,7 @@ from datetime import datetime, timezone from freezegun import freeze_time +from opaque_keys.edx.locator import LibraryCollectionLocator, LibraryContainerLocator from openedx_learning.api import authoring as authoring_api from organizations.models import Organization @@ -81,7 +82,9 @@ def setUpClass(cls): created_by=None, description="my toy collection description" ) - cls.collection_usage_key = "lib-collection:edX:2012_Fall:TOY_COLLECTION" + cls.collection_key = LibraryCollectionLocator.from_string( + "lib-collection:edX:2012_Fall:TOY_COLLECTION", + ) cls.library_block = library_api.create_library_block( cls.library.key, "html", @@ -94,7 +97,9 @@ def setUpClass(cls): title="A Unit in the Search Index", user_id=None, ) - cls.container_usage_key = "lct:edX:2012_Fall:unit:unit1" + cls.container_key = LibraryContainerLocator.from_string( + "lct:edX:2012_Fall:unit:unit1", + ) # Add the problem block to the collection library_api.update_library_collection_components( @@ -123,8 +128,8 @@ def setUpClass(cls): tagging_api.tag_object(str(cls.html_block_key), cls.subject_tags, tags=["Chinese", "Jump Links"]) tagging_api.tag_object(str(cls.html_block_key), cls.difficulty_tags, tags=["Normal"]) tagging_api.tag_object(str(cls.library_block.usage_key), cls.difficulty_tags, tags=["Normal"]) - tagging_api.tag_object(cls.collection_usage_key, cls.difficulty_tags, tags=["Normal"]) - tagging_api.tag_object(cls.container_usage_key, cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(str(cls.collection_key), cls.difficulty_tags, tags=["Normal"]) + tagging_api.tag_object(str(cls.container_key), cls.difficulty_tags, tags=["Normal"]) @property def toy_course_access_id(self): @@ -451,13 +456,13 @@ def test_html_published_library_block(self): assert doc["publish_status"] == "modified" def test_collection_with_library(self): - doc = searchable_doc_for_collection(self.library.key, self.collection.key) - doc.update(searchable_doc_tags_for_collection(self.library.key, self.collection.key)) + doc = searchable_doc_for_collection(self.collection_key) + doc.update(searchable_doc_tags_for_collection(self.collection_key)) assert doc == { "id": "lib-collectionedx2012_falltoy_collection-d1d907a4", "block_id": self.collection.key, - "usage_key": self.collection_usage_key, + "usage_key": str(self.collection_key), "type": "collection", "org": "edX", "display_name": "Toy Collection", @@ -480,13 +485,13 @@ def test_collection_with_library(self): def test_collection_with_published_library(self): library_api.publish_changes(self.library.key) - doc = searchable_doc_for_collection(self.library.key, self.collection.key) - doc.update(searchable_doc_tags_for_collection(self.library.key, self.collection.key)) + doc = searchable_doc_for_collection(self.collection_key) + doc.update(searchable_doc_tags_for_collection(self.collection_key)) assert doc == { "id": "lib-collectionedx2012_falltoy_collection-d1d907a4", "block_id": self.collection.key, - "usage_key": self.collection_usage_key, + "usage_key": str(self.collection_key), "type": "collection", "org": "edX", "display_name": "Toy Collection", diff --git a/openedx/core/djangoapps/content_libraries/api/blocks.py b/openedx/core/djangoapps/content_libraries/api/blocks.py index 24510762267c..04649a83503f 100644 --- a/openedx/core/djangoapps/content_libraries/api/blocks.py +++ b/openedx/core/djangoapps/content_libraries/api/blocks.py @@ -59,6 +59,7 @@ LibraryBlockAlreadyExists, ) from .libraries import ( + library_collection_locator, library_component_usage_key, require_permission_for_library_key, PublishableItem, @@ -284,8 +285,7 @@ def set_library_block_olx(usage_key: LibraryUsageLocatorV2, new_olx_str: str) -> for container in affected_containers: LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( - library_key=usage_key.lib_key, - container_key=str(container.container_key), + container_key=container.container_key, background=True, ) ) @@ -527,8 +527,10 @@ def delete_library_block(usage_key: LibraryUsageLocatorV2, remove_from_parent=Tr for collection in affected_collections: LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( - library_key=library_key, - collection_key=collection.key, + collection_key=library_collection_locator( + library_key=library_key, + collection_key=collection.key, + ), background=True, ) ) @@ -540,8 +542,7 @@ def delete_library_block(usage_key: LibraryUsageLocatorV2, remove_from_parent=Tr for container in affected_containers: LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( - library_key=library_key, - container_key=str(container.container_key), + container_key=container.container_key, background=True, ) ) @@ -580,8 +581,10 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2) -> None: for collection in affected_collections: LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( - library_key=library_key, - collection_key=collection.key, + collection_key=library_collection_locator( + library_key=library_key, + collection_key=collection.key, + ), background=True, ) ) @@ -594,8 +597,7 @@ def restore_library_block(usage_key: LibraryUsageLocatorV2) -> None: for container in affected_containers: LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( - library_key=library_key, - container_key=str(container.container_key), + container_key=container.container_key, background=True, ) ) diff --git a/openedx/core/djangoapps/content_libraries/api/collections.py b/openedx/core/djangoapps/content_libraries/api/collections.py index 435804c4140f..f94c9e5401a1 100644 --- a/openedx/core/djangoapps/content_libraries/api/collections.py +++ b/openedx/core/djangoapps/content_libraries/api/collections.py @@ -32,8 +32,8 @@ "update_library_collection", "update_library_collection_components", "set_library_component_collections", - "get_library_collection_usage_key", - "get_library_collection_from_usage_key", + "library_collection_locator", + "get_library_collection_from_locator", ] @@ -218,8 +218,10 @@ def set_library_component_collections( for collection in affected_collections: LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( - library_key=library_key, - collection_key=collection.key, + collection_key=library_collection_locator( + library_key=library_key, + collection_key=collection.key, + ), background=True, ) ) @@ -227,7 +229,7 @@ def set_library_component_collections( return component -def get_library_collection_usage_key( +def library_collection_locator( library_key: LibraryLocatorV2, collection_key: str, ) -> LibraryCollectionLocator: @@ -238,15 +240,14 @@ def get_library_collection_usage_key( return LibraryCollectionLocator(library_key, collection_key) -def get_library_collection_from_usage_key( - collection_usage_key: LibraryCollectionLocator, +def get_library_collection_from_locator( + collection_locator: LibraryCollectionLocator, ) -> Collection: """ Return a Collection using the LibraryCollectionLocator """ - - library_key = collection_usage_key.library_key - collection_key = collection_usage_key.collection_id + library_key = collection_locator.library_key + collection_key = collection_locator.collection_id content_library = ContentLibrary.objects.get_by_key(library_key) # type: ignore[attr-defined] assert content_library.learning_package_id is not None # shouldn't happen but it's technically possible. try: diff --git a/openedx/core/djangoapps/content_libraries/api/containers.py b/openedx/core/djangoapps/content_libraries/api/containers.py index 8a80cec352aa..31eae20e7fa6 100644 --- a/openedx/core/djangoapps/content_libraries/api/containers.py +++ b/openedx/core/djangoapps/content_libraries/api/containers.py @@ -190,8 +190,7 @@ def create_container( LIBRARY_CONTAINER_CREATED.send_event( library_container=LibraryContainerData( - library_key=library_key, - container_key=str(container_key), + container_key=container_key, ) ) @@ -219,8 +218,7 @@ def update_container( LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( - library_key=library_key, - container_key=str(container_key), + container_key=container_key, ) ) @@ -241,8 +239,7 @@ def delete_container( LIBRARY_CONTAINER_DELETED.send_event( library_container=LibraryContainerData( - library_key=container_key.library_key, - container_key=str(container_key), + container_key=container_key, ) ) @@ -259,7 +256,6 @@ def restore_container(container_key: LibraryContainerLocator) -> None: LIBRARY_CONTAINER_CREATED.send_event( library_container=LibraryContainerData( - library_key=container_key.library_key, container_key=str(container_key), ) ) @@ -325,8 +321,7 @@ def update_container_children( LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( - library_key=library_key, - container_key=str(container_key), + container_key=container_key, ) ) diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index bf9ca6f60d29..de7be56f7fab 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -75,6 +75,7 @@ from .. import permissions from ..constants import ALL_RIGHTS_RESERVED from ..models import ContentLibrary, ContentLibraryPermission +from .collections import library_collection_locator from .exceptions import ( LibraryAlreadyExists, LibraryPermissionIntegrityError, @@ -739,8 +740,10 @@ def revert_changes(library_key: LibraryLocatorV2) -> None: for collection in authoring_api.get_collections(learning_package.id): LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( - library_key=library_key, - collection_key=collection.key, + collection_key=library_collection_locator( + library_key=library_key, + collection_key=collection.key, + ), background=True, ) ) diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index d5f121d8396e..34a5a90269fe 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -125,8 +125,7 @@ def send_container_updated_events(self, usage_key: UsageKeyV2): for container in affected_containers: LIBRARY_CONTAINER_UPDATED.send_event( library_container=LibraryContainerData( - library_key=usage_key.lib_key, - container_key=str(container.container_key), + container_key=container.container_key, background=True, ) ) diff --git a/openedx/core/djangoapps/content_libraries/signal_handlers.py b/openedx/core/djangoapps/content_libraries/signal_handlers.py index 68cc7512e540..08b30b10bba6 100644 --- a/openedx/core/djangoapps/content_libraries/signal_handlers.py +++ b/openedx/core/djangoapps/content_libraries/signal_handlers.py @@ -25,7 +25,7 @@ from lms.djangoapps.grades.api import signals as grades_signals -from .api import library_component_usage_key +from .api import library_collection_locator, library_component_usage_key from .models import ContentLibrary, LtiGradedResource log = logging.getLogger(__name__) @@ -86,15 +86,19 @@ def library_collection_saved(sender, instance, created, **kwargs): if created: LIBRARY_COLLECTION_CREATED.send_event( library_collection=LibraryCollectionData( - library_key=library.library_key, - collection_key=instance.key, + collection_key=library_collection_locator( + library_key=library.library_key, + collection_key=instance.key, + ), ) ) else: LIBRARY_COLLECTION_UPDATED.send_event( library_collection=LibraryCollectionData( - library_key=library.library_key, - collection_key=instance.key, + collection_key=library_collection_locator( + library_key=library.library_key, + collection_key=instance.key, + ), ) ) @@ -112,8 +116,10 @@ def library_collection_deleted(sender, instance, **kwargs): LIBRARY_COLLECTION_DELETED.send_event( library_collection=LibraryCollectionData( - library_key=library.library_key, - collection_key=instance.key, + collection_key=library_collection_locator( + library_key=library.library_key, + collection_key=instance.key, + ), ) ) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_api.py b/openedx/core/djangoapps/content_libraries/tests/test_api.py index 71360cde3e31..b0934768b794 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_api.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_api.py @@ -352,8 +352,10 @@ def test_create_library_collection(self): "signal": LIBRARY_COLLECTION_CREATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib2.library_key, - collection_key="COL4", + collection_key=api.library_collection_locator( + self.lib2.library_key, + collection_key="COL4", + ), ), }, event_receiver.call_args_list[0].kwargs, @@ -388,8 +390,10 @@ def test_update_library_collection(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key="COL1", + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key="COL1", + ), ), }, event_receiver.call_args_list[0].kwargs, @@ -418,8 +422,10 @@ def test_delete_library_collection(self): "signal": LIBRARY_COLLECTION_DELETED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key="COL1", + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key="COL1", + ), ), }, event_receiver.call_args_list[0].kwargs, @@ -493,8 +499,10 @@ def test_update_library_collection_components_event(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key="COL1", + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key="COL1", + ), ), }, event_receiver.call_args_list[2].kwargs, @@ -544,8 +552,10 @@ def test_set_library_component_collections(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib2.library_key, - collection_key=self.col2.key, + collection_key=api.library_collection_locator( + self.lib2.library_key, + collection_key=self.col2.key, + ), background=True, ), }, @@ -556,8 +566,10 @@ def test_set_library_component_collections(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib2.library_key, - collection_key=self.col3.key, + collection_key=api.library_collection_locator( + self.lib2.library_key, + collection_key=self.col3.key, + ), background=True, ), }, @@ -585,8 +597,10 @@ def test_delete_library_block(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key=self.col1.key, + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, + ), background=True, ), }, @@ -614,8 +628,10 @@ def test_restore_library_block(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key=self.col1.key, + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, + ), background=True, ), }, @@ -656,8 +672,10 @@ def test_add_component_and_revert(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key=self.col1.key, + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, + ), background=True, ), }, @@ -715,8 +733,10 @@ def test_delete_component_and_revert(self): "signal": LIBRARY_COLLECTION_UPDATED, "sender": None, "library_collection": LibraryCollectionData( - self.lib1.library_key, - collection_key=self.col1.key, + collection_key=api.library_collection_locator( + self.lib1.library_key, + collection_key=self.col1.key, + ), background=True, ), }, @@ -824,8 +844,7 @@ def _validate_calls_of_html_block(self, event_mock): "signal": LIBRARY_CONTAINER_UPDATED, "sender": None, "library_container": LibraryContainerData( - library_key=self.lib1.library_key, - container_key=str(self.unit1.container_key), + container_key=self.unit1.container_key, background=True, ) }, @@ -836,8 +855,7 @@ def _validate_calls_of_html_block(self, event_mock): "signal": LIBRARY_CONTAINER_UPDATED, "sender": None, "library_container": LibraryContainerData( - library_key=self.lib1.library_key, - container_key=str(self.unit2.container_key), + container_key=self.unit2.container_key, background=True, ) }, diff --git a/openedx/core/djangoapps/content_libraries/tests/test_containers.py b/openedx/core/djangoapps/content_libraries/tests/test_containers.py index 46d206af5008..85e04260cf52 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_containers.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_containers.py @@ -7,7 +7,7 @@ import ddt from freezegun import freeze_time -from opaque_keys.edx.locator import LibraryLocatorV2 +from opaque_keys.edx.locator import LibraryContainerLocator, LibraryLocatorV2 from openedx_events.content_authoring.data import LibraryContainerData from openedx_events.content_authoring.signals import ( LIBRARY_CONTAINER_CREATED, @@ -83,13 +83,15 @@ def test_unit_crud(self): self.assertDictContainsEntries(container_data, expected_data) assert create_receiver.call_count == 1 + container_key = LibraryContainerLocator.from_string( + "lct:CL-TEST:containers:unit:u1", + ) self.assertDictContainsSubset( { "signal": LIBRARY_CONTAINER_CREATED, "sender": None, "library_container": LibraryContainerData( - lib_key, - container_key="lct:CL-TEST:containers:unit:u1", + container_key, ), }, create_receiver.call_args_list[0].kwargs, @@ -114,8 +116,7 @@ def test_unit_crud(self): "signal": LIBRARY_CONTAINER_UPDATED, "sender": None, "library_container": LibraryContainerData( - lib_key, - container_key="lct:CL-TEST:containers:unit:u1", + container_key, ), }, update_receiver.call_args_list[0].kwargs, @@ -135,8 +136,7 @@ def test_unit_crud(self): "signal": LIBRARY_CONTAINER_DELETED, "sender": None, "library_container": LibraryContainerData( - lib_key, - container_key="lct:CL-TEST:containers:unit:u1", + container_key, ), }, delete_receiver.call_args_list[0].kwargs, @@ -214,8 +214,9 @@ def test_unit_add_children(self): "signal": LIBRARY_CONTAINER_UPDATED, "sender": None, "library_container": LibraryContainerData( - lib_key, - container_key=container_data["container_key"], + container_key=LibraryContainerLocator.from_string( + container_data["container_key"], + ), ), }, update_receiver.call_args_list[0].kwargs, @@ -263,8 +264,9 @@ def test_unit_remove_children(self): "signal": LIBRARY_CONTAINER_UPDATED, "sender": None, "library_container": LibraryContainerData( - lib_key, - container_key=container_data["container_key"], + container_key=LibraryContainerLocator.from_string( + container_data["container_key"], + ), ), }, update_receiver.call_args_list[0].kwargs, @@ -324,8 +326,9 @@ def test_unit_replace_children(self): "signal": LIBRARY_CONTAINER_UPDATED, "sender": None, "library_container": LibraryContainerData( - lib_key, - container_key=container_data["container_key"], + container_key=LibraryContainerLocator.from_string( + container_data["container_key"], + ), ), }, update_receiver.call_args_list[0].kwargs, @@ -374,7 +377,6 @@ def test_restore_unit(self): "signal": LIBRARY_CONTAINER_CREATED, "sender": None, "library_container": LibraryContainerData( - lib_key, container_key="lct:CL-TEST:containers:unit:u1", ), }, diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 29c056d6712f..f1ef8845df80 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -804,7 +804,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/kernel.in openedx-django-wiki==2.1.0 # via -r requirements/edx/kernel.in -openedx-events==9.20.0 +openedx-events==10.0.0 # via # -r requirements/edx/kernel.in # edx-enterprise diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index d7f7674bd6f6..5bfbca431f47 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1363,7 +1363,7 @@ openedx-django-wiki==2.1.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt -openedx-events==9.20.0 +openedx-events==10.0.0 # via # -r requirements/edx/doc.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index d3372e8dc678..7b4fb5682a82 100644 --- a/requirements/edx/doc.txt +++ b/requirements/edx/doc.txt @@ -976,7 +976,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==9.20.0 +openedx-events==10.0.0 # via # -r requirements/edx/base.txt # edx-enterprise diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 174a06ed72f3..404104e9750d 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -1034,7 +1034,7 @@ openedx-django-require==2.1.0 # via -r requirements/edx/base.txt openedx-django-wiki==2.1.0 # via -r requirements/edx/base.txt -openedx-events==9.20.0 +openedx-events==10.0.0 # via # -r requirements/edx/base.txt # edx-enterprise From 528a3eb71319a2d206187a6bdf9ec714a6cbb723 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Fri, 11 Apr 2025 18:01:36 +0000 Subject: [PATCH 18/23] feat: post component height to parent window [FC-0083] (#36477) XBlock rendered in an iframe in frontend-app-authoring needs to resize automatically based on component size. Updates xblock iframe template to post message to parent window on size change. --- common/templates/xblock_v2/xblock_iframe.html | 60 +++++++++++++++---- .../content_libraries/api/libraries.py | 13 ++-- 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/common/templates/xblock_v2/xblock_iframe.html b/common/templates/xblock_v2/xblock_iframe.html index add3c46bce55..69af639da2a8 100644 --- a/common/templates/xblock_v2/xblock_iframe.html +++ b/common/templates/xblock_v2/xblock_iframe.html @@ -180,8 +180,8 @@ - -
+ +
{{ fragment.body_html | safe }} @@ -360,7 +360,7 @@ error_message_div.html('Error: '+response.message); error_message_div.css('display', 'block'); } - }); + }); }); }); } @@ -437,21 +437,55 @@ const rootNode = document.querySelector('.xblock, .xblock-v1'); // will always return the first matching element initializeXBlockAndChildren(rootNode, () => { }); + (function() { + // If this view is rendered in an iframe within the authoring microfrontend app + // it will report the height of its contents to the parent window when the + // document loads, window resizes, or DOM mutates. + if (window !== window.parent) { + var lastHeight = window.parent[0].offsetHeight; + var lastWidth = window.parent[0].offsetWidth; - let lastHeight = -1; - function checkFrameHeight() { - const newHeight = document.documentElement.scrollHeight; - if (newHeight !== lastHeight) { - lastHeight = newHeight; + function dispatchResizeMessage(event) { + // Note: event is actually an Array of MutationRecord objects when fired from the MutationObserver + var newHeight = rootNode.scrollHeight; + var newWidth = rootNode.offsetWidth; + + window.parent.postMessage( + { + type: 'plugin.resize', + payload: { + width: newWidth, + height: newHeight, + } + }, document.referrer + ); + + lastHeight = newHeight; + lastWidth = newWidth; + + // Within the authoring microfrontend the iframe resizes to match the + // height of this document and it should never scroll. It does scroll + // ocassionally when javascript is used to focus elements on the page + // before the parent iframe has been resized to match the content + // height. This window.scrollTo is an attempt to keep the content at the + // top of the page. + window.scrollTo(0, 0); + } + + // Create an observer instance linked to the callback function + const observer = new MutationObserver(dispatchResizeMessage); + + // Start observing the target node for configured mutations + observer.observe(rootNode, { attributes: true, childList: true, subtree: true }); + + const resizeObserver = new ResizeObserver(dispatchResizeMessage); + resizeObserver.observe(rootNode); } - } - // Check the size whenever the DOM changes: - new MutationObserver(checkFrameHeight).observe(document.body, { attributes: true, childList: true, subtree: true }); - // And whenever the IFrame is resized - window.addEventListener('resize', checkFrameHeight); + }()); } window.addEventListener('load', blockFrameJS); + diff --git a/openedx/core/djangoapps/content_libraries/api/libraries.py b/openedx/core/djangoapps/content_libraries/api/libraries.py index de7be56f7fab..e6ba323b9aae 100644 --- a/openedx/core/djangoapps/content_libraries/api/libraries.py +++ b/openedx/core/djangoapps/content_libraries/api/libraries.py @@ -211,12 +211,17 @@ class LibraryXBlockMetadata(PublishableItem): Class that represents the metadata about an XBlock in a content library. """ usage_key: LibraryUsageLocatorV2 + # TODO: move tags_count to LibraryItem as all objects under a library can be tagged. + tags_count: int = 0 @classmethod def from_component(cls, library_key, component, associated_collections=None): """ Construct a LibraryXBlockMetadata from a Component object. """ + # Import content_tagging.api here to avoid circular imports + from openedx.core.djangoapps.content_tagging.api import get_object_tag_counts + last_publish_log = component.versioning.last_publish_log published_by = None @@ -227,12 +232,11 @@ def from_component(cls, library_key, component, associated_collections=None): published = component.versioning.published last_draft_created = draft.created if draft else None last_draft_created_by = draft.publishable_entity_version.created_by if draft else None + usage_key = library_component_usage_key(library_key, component) + tags = get_object_tag_counts(str(usage_key), count_implicit=True) return cls( - usage_key=library_component_usage_key( - library_key, - component, - ), + usage_key=usage_key, display_name=draft.title, created=component.created, modified=draft.created, @@ -245,6 +249,7 @@ def from_component(cls, library_key, component, associated_collections=None): has_unpublished_changes=component.versioning.has_unpublished_changes, collections=associated_collections or [], can_stand_alone=component.publishable_entity.can_stand_alone, + tags_count=tags.get(str(usage_key), 0), ) From dc09b958f9389fb2723eac45959f95807bfd11f9 Mon Sep 17 00:00:00 2001 From: Hassan Raza Date: Mon, 14 Apr 2025 15:02:25 +0500 Subject: [PATCH 19/23] fix: Update ORA notification type for grouper (#36517) --- openedx/core/djangoapps/notifications/exceptions.py | 6 ++++++ .../notifications/grouping_notifications.py | 12 +++++++++--- .../tests/test_notification_grouping.py | 4 ++++ 3 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 openedx/core/djangoapps/notifications/exceptions.py diff --git a/openedx/core/djangoapps/notifications/exceptions.py b/openedx/core/djangoapps/notifications/exceptions.py new file mode 100644 index 000000000000..2eb8bb794253 --- /dev/null +++ b/openedx/core/djangoapps/notifications/exceptions.py @@ -0,0 +1,6 @@ +""" Notification-related exceptions. """ + + +class InvalidNotificationTypeError(Exception): + """ Exception raised when an invalid notification type is passed. """ + pass # lint-amnesty, pylint: disable=unnecessary-pass diff --git a/openedx/core/djangoapps/notifications/grouping_notifications.py b/openedx/core/djangoapps/notifications/grouping_notifications.py index a1da88683fa0..eeb4400462b4 100644 --- a/openedx/core/djangoapps/notifications/grouping_notifications.py +++ b/openedx/core/djangoapps/notifications/grouping_notifications.py @@ -2,14 +2,16 @@ Notification grouping utilities for notifications """ import datetime +from abc import ABC, abstractmethod from typing import Dict, Type, Union from pytz import utc -from abc import ABC, abstractmethod - +from openedx.core.djangoapps.notifications.base_notification import COURSE_NOTIFICATION_TYPES from openedx.core.djangoapps.notifications.models import Notification +from .exceptions import InvalidNotificationTypeError + class BaseNotificationGrouper(ABC): """ @@ -42,6 +44,10 @@ def decorator(grouper_class: Type[BaseNotificationGrouper]) -> Type[BaseNotifica """ Registers the grouper class for the given notification type. """ + if notification_type not in COURSE_NOTIFICATION_TYPES: + raise InvalidNotificationTypeError( + f"'{notification_type}' is not a valid notification type." + ) cls._groupers[notification_type] = grouper_class return grouper_class @@ -106,7 +112,7 @@ def group(self, new_notification, old_notification): } -@NotificationRegistry.register('ora_staff_notification') +@NotificationRegistry.register('ora_staff_notifications') class OraStaffGrouper(BaseNotificationGrouper): """ Grouper for new ora staff notifications. diff --git a/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py b/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py index a3aace632c8b..23419e46cb64 100644 --- a/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py +++ b/openedx/core/djangoapps/notifications/tests/test_notification_grouping.py @@ -26,6 +26,10 @@ class TestNotificationRegistry(unittest.TestCase): Tests for the NotificationRegistry class """ + @patch.dict( + 'openedx.core.djangoapps.notifications.base_notification.COURSE_NOTIFICATION_TYPES', + {'test_notification': 'Test Notification'} + ) def test_register_and_get_grouper(self): """ Test that the register and get_grouper methods work as expected From 34d682e4cc58db964d113a79a4a2d372d724f8f0 Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Mon, 14 Apr 2025 18:11:01 +0500 Subject: [PATCH 20/23] feat: prevent sending course update, recurring nudge and upgrade reminder email to disabled users (#36514) --- openedx/core/djangoapps/schedules/tasks.py | 3 ++ .../djangoapps/schedules/tests/test_tasks.py | 47 ++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/schedules/tasks.py b/openedx/core/djangoapps/schedules/tasks.py index 55288d63f1a8..6a999d3dd8ed 100644 --- a/openedx/core/djangoapps/schedules/tasks.py +++ b/openedx/core/djangoapps/schedules/tasks.py @@ -276,6 +276,9 @@ def _schedule_send(msg_str, site_id, delivery_config_var, log_prefix): # lint-a msg = Message.from_string(msg_str) user = User.objects.get(id=msg.recipient.lms_user_id) + if not user.has_usable_password(): + LOG.info(f'{delivery_config_var} Scheduled email User is disabled {user.username}') + return with emulate_http_request(site=site, user=user): _annonate_send_task_for_monitoring(msg) LOG.debug('%s: Sending message = %s', log_prefix, msg_str) diff --git a/openedx/core/djangoapps/schedules/tests/test_tasks.py b/openedx/core/djangoapps/schedules/tests/test_tasks.py index 412a62521733..99f790e35b01 100644 --- a/openedx/core/djangoapps/schedules/tests/test_tasks.py +++ b/openedx/core/djangoapps/schedules/tests/test_tasks.py @@ -9,9 +9,13 @@ import ddt from django.conf import settings +from django.test import TestCase +from edx_ace.recipient import Recipient +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.schedules.message_types import InstructorLedCourseUpdate from openedx.core.djangoapps.schedules.resolvers import DEFAULT_NUM_BINS -from openedx.core.djangoapps.schedules.tasks import BinnedScheduleMessageBaseTask +from openedx.core.djangoapps.schedules.tasks import BinnedScheduleMessageBaseTask, _schedule_send from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFactory from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms @@ -73,3 +77,44 @@ def test_is_enqueue_enabled(self, enabled): self.schedule_config.enqueue_recurring_nudge = enabled self.schedule_config.save() assert self.basetask.is_enqueue_enabled(self.site) == enabled + + +@ddt.ddt +@skip_unless_lms +class TestScheduleSendForDisabledUser(TestCase): + """ + Tests email send for disabled users + """ + + def setUp(self): + super().setUp() + self.user = UserFactory() + self.site = SiteFactory.create() + ScheduleConfigFactory.create( + site=self.site, + enqueue_recurring_nudge=True, deliver_recurring_nudge=True, + enqueue_upgrade_reminder=True, deliver_upgrade_reminder=True, + enqueue_course_update=True, deliver_course_update=True, + ) + + @ddt.data(True, False) + @patch('openedx.core.djangoapps.schedules.tasks.ace.send') + def test_email_not_sent_to_disable_users(self, user_enabled, mock_send): + """ + Tests email not send for disabled users + """ + if user_enabled: + self.user.set_password("12345678") + else: + self.user.set_unusable_password() + self.user.save() + msg = InstructorLedCourseUpdate().personalize( + Recipient( + self.user.id, + self.user.email, + ), + "en", + {}, + ) + _schedule_send(str(msg), self.site.id, "deliver_course_update", "Course Update") + assert mock_send.called is user_enabled From 0a6078a8b66e3c3496652d4f90ed07a0ec76c8bf Mon Sep 17 00:00:00 2001 From: farhan Date: Thu, 17 Apr 2025 11:48:27 +0500 Subject: [PATCH 21/23] chore: Adds skip_control.js file --- .../assets/video/public/js/skip_control.js | 70 +++++++++++++++++++ .../video/public/js/video_block_main.js | 4 +- 2 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 xmodule/assets/video/public/js/skip_control.js diff --git a/xmodule/assets/video/public/js/skip_control.js b/xmodule/assets/video/public/js/skip_control.js new file mode 100644 index 000000000000..326c8e8fbb64 --- /dev/null +++ b/xmodule/assets/video/public/js/skip_control.js @@ -0,0 +1,70 @@ +import $ from 'jquery'; // jQuery import +import _ from 'underscore'; + +'use strict'; + +/** + * VideoSkipControl function + * + * @constructor + * @param {Object} state The object containing the state of the video + * @param {Object} i18n The object containing strings with translations + * @return {jQuery.Promise} Returns a resolved jQuery promise + */ +function VideoSkipControl(state, i18n) { + if (!(this instanceof VideoSkipControl)) { + return new VideoSkipControl(state, i18n); + } + + _.bindAll(this, 'onClick', 'render', 'destroy'); + this.state = state; + this.state.videoSkipControl = this; + this.i18n = i18n; + + this.initialize(); + + return $.Deferred().resolve().promise(); +} + +VideoSkipControl.prototype = { + template: [ + '', + ].join(''), + + initialize: function () { + this.el = $(this.template); + this.bindHandlers(); + }, + + /** Creates any necessary DOM elements, attach them, and set their, initial configuration. */ + render: function () { + this.state.el.find('.vcr .control').after(this.el); + }, + + /** Bind any necessary function callbacks to DOM events. */ + bindHandlers: function () { + this.el.on('click', this.onClick); + + this.state.el.on({ + 'play.skip': _.once(this.render), + 'destroy.skip': this.destroy, + }); + }, + + onClick: function (event) { + event.preventDefault(); + this.state.videoCommands.execute('skip', true); + }, + + destroy: function () { + this.el.remove(); + this.state.el.off('.skip'); + delete this.state.videoSkipControl; + }, +}; + +export {VideoSkipControl}; \ No newline at end of file diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js index 902c6e704f21..7f53138980ee 100644 --- a/xmodule/assets/video/public/js/video_block_main.js +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -4,6 +4,7 @@ import _ from 'underscore'; import {VideoStorage} from './video_storage'; import {VideoPoster} from './poster'; import {VideoTranscriptDownloadHandler} from './video_accessible_menu'; +import {VideoSkipControl} from './skip_control'; // TODO: Uncomment the imports // import { initialize } from './initialize'; // Assuming this function is imported @@ -26,7 +27,6 @@ import {VideoTranscriptDownloadHandler} from './video_accessible_menu'; // VideoTranscriptFeedback, // VideoAutoAdvanceControl, // VideoPlaySkipControl, -// VideoSkipControl, // VideoEventsBumperPlugin, // VideoSocialSharing, // VideoBumper, @@ -81,7 +81,7 @@ console.log('In video_block_main.js file'); const bumperVideoModules = [ // VideoControl, // VideoPlaySkipControl, - // VideoSkipControl, + VideoSkipControl, // VideoVolumeControl, // VideoCaption, // VideoCommands, From 4bfa224bea7cfb1f5be667f46f789d975d671882 Mon Sep 17 00:00:00 2001 From: farhan Date: Thu, 17 Apr 2025 12:02:38 +0500 Subject: [PATCH 22/23] chore: Adds VideoPlayPlaceholder --- .../video/public/js/play_placeholder.js | 87 +++++++++++++++++++ .../video/public/js/video_block_main.js | 42 +++++---- 2 files changed, 107 insertions(+), 22 deletions(-) create mode 100644 xmodule/assets/video/public/js/play_placeholder.js diff --git a/xmodule/assets/video/public/js/play_placeholder.js b/xmodule/assets/video/public/js/play_placeholder.js new file mode 100644 index 000000000000..d3d7384573bb --- /dev/null +++ b/xmodule/assets/video/public/js/play_placeholder.js @@ -0,0 +1,87 @@ +import $ from 'jquery'; // jQuery import +import _ from 'underscore'; + +'use strict'; + +/** + * Video Play placeholder control function. + * + * @constructor + * @param {Object} state - The object containing the state of the video + * @param {Object} i18n - The object containing strings with translations + * @return {jQuery.Promise} - A resolved jQuery promise + */ +function VideoPlayPlaceholder(state, i18n) { + if (!(this instanceof VideoPlayPlaceholder)) { + return new VideoPlayPlaceholder(state, i18n); + } + + _.bindAll(this, 'onClick', 'hide', 'show', 'destroy'); + this.state = state; + this.state.videoPlayPlaceholder = this; + this.i18n = i18n; + this.initialize(); + + return $.Deferred().resolve().promise(); +} + +VideoPlayPlaceholder.prototype = { + destroy: function () { + this.el.off('click', this.onClick); + this.state.el.off({ + destroy: this.destroy, + play: this.hide, + 'ended pause': this.show, + }); + this.hide(); + delete this.state.videoPlayPlaceholder; + }, + + /** + * Indicates whether the placeholder should be shown. + * We display it for HTML5 videos on iPad and Android devices. + * @return {Boolean} + */ + shouldBeShown: function () { + return /iPad|Android/i.test(this.state.isTouch[0]) && !this.state.isYoutubeType(); + }, + + /** Initializes the module. */ + initialize: function () { + if (!this.shouldBeShown()) { + return false; + } + + this.el = this.state.el.find('.btn-play'); + this.bindHandlers(); + this.show(); + }, + + /** Bind any necessary function callbacks to DOM events. */ + bindHandlers: function () { + this.el.on('click', this.onClick); + this.state.el.on({ + destroy: this.destroy, + play: this.hide, + 'ended pause': this.show, + }); + }, + + onClick: function () { + this.state.videoCommands.execute('play'); + }, + + hide: function () { + this.el + .addClass('is-hidden') + .attr({'aria-hidden': 'true', tabindex: -1}); + }, + + show: function () { + this.el + .removeClass('is-hidden') + .attr({'aria-hidden': 'false', tabindex: 0}); + } +}; + +export {VideoPlayPlaceholder}; \ No newline at end of file diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js index 7f53138980ee..ef7465d0aa86 100644 --- a/xmodule/assets/video/public/js/video_block_main.js +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -5,13 +5,13 @@ import {VideoStorage} from './video_storage'; import {VideoPoster} from './poster'; import {VideoTranscriptDownloadHandler} from './video_accessible_menu'; import {VideoSkipControl} from './skip_control'; +import {VideoPlayPlaceholder} from './play_placeholder'; // TODO: Uncomment the imports // import { initialize } from './initialize'; // Assuming this function is imported // import { // FocusGrabber, // VideoControl, -// VideoPlayPlaceholder, // VideoPlayPauseControl, // VideoProgressSlider, // VideoSpeedControl, @@ -57,26 +57,25 @@ console.log('In video_block_main.js file'); const bumperMetadata = el.data('bumper-metadata'); const autoAdvanceEnabled = el.data('autoadvance-enabled') === 'True'; - const mainVideoModules = [] - // TODO: Uncomment the code - // const mainVideoModules = [ - // FocusGrabber, - // VideoControl, - // VideoPlayPlaceholder, - // VideoPlayPauseControl, - // VideoProgressSlider, - // VideoSpeedControl, - // VideoVolumeControl, - // VideoQualityControl, - // VideoFullScreen, - // VideoCaption, - // VideoCommands, - // VideoContextMenu, - // VideoSaveStatePlugin, - // VideoEventsPlugin, - // VideoCompletionHandler, - // VideoTranscriptFeedback, - // ].concat(autoAdvanceEnabled ? [VideoAutoAdvanceControl] : []); + const mainVideoModules = [ + // FocusGrabber, + // VideoControl, + VideoPlayPlaceholder, + // VideoPlayPauseControl, + // VideoProgressSlider, + // VideoSpeedControl, + // VideoVolumeControl, + // VideoQualityControl, + // VideoFullScreen, + // VideoCaption, + // VideoCommands, + // VideoContextMenu, + // VideoSaveStatePlugin, + // VideoEventsPlugin, + // VideoCompletionHandler, + // VideoTranscriptFeedback, + // ].concat(autoAdvanceEnabled ? [VideoAutoAdvanceControl] : []); + ] const bumperVideoModules = [ // VideoControl, @@ -184,4 +183,3 @@ console.log('In video_block_main.js file'); // oldVideo(null, true); }()); - From c5874a754cb984f5c41e1c806d9cb4ca876f4dc2 Mon Sep 17 00:00:00 2001 From: farhan Date: Thu, 17 Apr 2025 12:28:42 +0500 Subject: [PATCH 23/23] chore: Adds play pause and play skip control js files --- .../video/public/js/play_pause_control.js | 95 +++++++++++++++++++ .../video/public/js/play_skip_control.js | 90 ++++++++++++++++++ .../video/public/js/video_block_main.js | 9 +- 3 files changed, 190 insertions(+), 4 deletions(-) create mode 100644 xmodule/assets/video/public/js/play_pause_control.js create mode 100644 xmodule/assets/video/public/js/play_skip_control.js diff --git a/xmodule/assets/video/public/js/play_pause_control.js b/xmodule/assets/video/public/js/play_pause_control.js new file mode 100644 index 000000000000..c2b2bdb2aa0e --- /dev/null +++ b/xmodule/assets/video/public/js/play_pause_control.js @@ -0,0 +1,95 @@ +import $ from 'jquery'; // jQuery import +import _ from 'underscore'; + +'use strict'; + +/** + * PlayPauseControl function + * + * @constructor + * @param {Object} state - The object containing the state of the video + * @param {Object} i18n - The object containing strings with translations + * @return {jQuery.Promise} - A resolved jQuery promise + */ +function VideoPlayPauseControl(state, i18n) { + if (!(this instanceof VideoPlayPauseControl)) { + return new VideoPlayPauseControl(state, i18n); + } + + _.bindAll(this, 'play', 'pause', 'onClick', 'destroy'); + this.state = state; + this.state.videoPlayPauseControl = this; + this.i18n = i18n; + + this.initialize(); + + return $.Deferred().resolve().promise(); +} + +VideoPlayPauseControl.prototype = { + template: [ + '', + ].join(''), + + /** Initializes the module. */ + initialize: function () { + this.el = $(this.template); + this.render(); + this.bindHandlers(); + }, + + /** + * Creates any necessary DOM elements, attach them, and set their, + * initial configuration. + */ + render: function () { + this.state.el.find('.vcr').prepend(this.el); + }, + + /** Bind any necessary function callbacks to DOM events. */ + bindHandlers: function () { + this.el.on('click', this.onClick); + this.state.el.on({ + play: this.play, + 'pause ended': this.pause, + destroy: this.destroy, + }); + }, + + onClick: function (event) { + event.preventDefault(); + this.state.videoCommands.execute('togglePlayback'); + }, + + play: function () { + this.el + .addClass('pause') + .removeClass('play') + .attr({title: gettext('Pause'), 'aria-label': gettext('Pause')}) + .find('.icon') + .removeClass('fa-play') + .addClass('fa-pause'); + }, + + pause: function () { + this.el + .removeClass('pause') + .addClass('play') + .attr({title: gettext('Play'), 'aria-label': gettext('Play')}) + .find('.icon') + .removeClass('fa-pause') + .addClass('fa-play'); + }, + + destroy: function () { + this.el.remove(); + this.state.el.off('destroy', this.destroy); + delete this.state.videoPlayPauseControl; + }, +}; + +export {VideoPlayPauseControl}; diff --git a/xmodule/assets/video/public/js/play_skip_control.js b/xmodule/assets/video/public/js/play_skip_control.js new file mode 100644 index 000000000000..3be7d8b05398 --- /dev/null +++ b/xmodule/assets/video/public/js/play_skip_control.js @@ -0,0 +1,90 @@ +import $ from 'jquery'; // jQuery import +import _ from 'underscore'; + +'use strict'; + +/** + * Play/skip control module + * + * @constructor + * @param {Object} state - The object containing the state of the video + * @param {Object} i18n - The object containing strings with translations + * @return {jQuery.Promise} - A resolved jQuery promise + */ +function VideoPlaySkipControl(state, i18n) { + if (!(this instanceof VideoPlaySkipControl)) { + return new VideoPlaySkipControl(state, i18n); + } + + _.bindAll(this, 'play', 'onClick', 'destroy'); + this.state = state; + this.state.videoPlaySkipControl = this; + this.i18n = i18n; + + this.initialize(); + + return $.Deferred().resolve().promise(); +} + +VideoPlaySkipControl.prototype = { + template: [ + '', + ].join(''), + + /** Initializes the module. */ + initialize: function () { + this.el = $(this.template); + this.render(); + this.bindHandlers(); + }, + + /** + * Creates any necessary DOM elements, attach them, and set their, + * initial configuration. + */ + render: function () { + this.state.el.find('.vcr').prepend(this.el); + }, + + /** Bind any necessary function callbacks to DOM events. */ + bindHandlers: function () { + this.el.on('click', this.onClick); + this.state.el.on({ + play: this.play, + destroy: this.destroy, + }); + }, + + onClick: function (event) { + event.preventDefault(); + if (this.state.videoPlayer.isPlaying()) { + this.state.videoCommands.execute('skip'); + } else { + this.state.videoCommands.execute('play'); + } + }, + + play: function () { + this.el + .removeClass('play') + .addClass('skip') + .attr('title', gettext('Skip')) + .find('.icon') + .removeClass('fa-play') + .addClass('fa-step-forward'); + // Disable possibility to pause the video. + this.state.el.find('video').off('click'); + }, + + destroy: function () { + this.el.remove(); + this.state.el.off('destroy', this.destroy); + delete this.state.videoPlaySkipControl; + }, +}; + +export {VideoPlaySkipControl}; \ No newline at end of file diff --git a/xmodule/assets/video/public/js/video_block_main.js b/xmodule/assets/video/public/js/video_block_main.js index ef7465d0aa86..90fbd47a1acd 100644 --- a/xmodule/assets/video/public/js/video_block_main.js +++ b/xmodule/assets/video/public/js/video_block_main.js @@ -6,13 +6,15 @@ import {VideoPoster} from './poster'; import {VideoTranscriptDownloadHandler} from './video_accessible_menu'; import {VideoSkipControl} from './skip_control'; import {VideoPlayPlaceholder} from './play_placeholder'; +import {VideoPlaySkipControl} from './play_skip_control'; +import {VideoPlayPauseControl} from './play_pause_control'; + // TODO: Uncomment the imports // import { initialize } from './initialize'; // Assuming this function is imported // import { // FocusGrabber, // VideoControl, -// VideoPlayPauseControl, // VideoProgressSlider, // VideoSpeedControl, // VideoVolumeControl, @@ -26,7 +28,6 @@ import {VideoPlayPlaceholder} from './play_placeholder'; // VideoCompletionHandler, // VideoTranscriptFeedback, // VideoAutoAdvanceControl, -// VideoPlaySkipControl, // VideoEventsBumperPlugin, // VideoSocialSharing, // VideoBumper, @@ -61,7 +62,7 @@ console.log('In video_block_main.js file'); // FocusGrabber, // VideoControl, VideoPlayPlaceholder, - // VideoPlayPauseControl, + VideoPlayPauseControl, // VideoProgressSlider, // VideoSpeedControl, // VideoVolumeControl, @@ -79,7 +80,7 @@ console.log('In video_block_main.js file'); const bumperVideoModules = [ // VideoControl, - // VideoPlaySkipControl, + VideoPlaySkipControl, VideoSkipControl, // VideoVolumeControl, // VideoCaption,