From 274db7f747fd24a07df5a0418cc58283012c71b2 Mon Sep 17 00:00:00 2001 From: Sara Arjona Date: Tue, 11 Jul 2023 11:43:19 +0200 Subject: [PATCH] MDL-78528 completion: Add suffix support to avoid duplicated ids --- completion/classes/defaultedit_form.php | 37 +++++- completion/classes/edit_base_form.php | 13 +++ completion/classes/form/form_trait.php | 144 ++++++++++++++++-------- course/modlib.php | 5 +- course/tests/modlib_test.php | 44 ++++++++ course/upgrade.txt | 1 + 6 files changed, 194 insertions(+), 50 deletions(-) diff --git a/completion/classes/defaultedit_form.php b/completion/classes/defaultedit_form.php index ec088370f7c..2d0431160c6 100644 --- a/completion/classes/defaultedit_form.php +++ b/completion/classes/defaultedit_form.php @@ -27,6 +27,25 @@ class core_completion_defaultedit_form extends core_completion_edit_base_form { /** @var array */ protected $_modnames; + public function __construct( + $action = null, + $customdata = null, + $method = 'post', + $target = '', + $attributes = null, + $editable = true, + $ajaxformdata = null + ) { + $this->modules = $customdata['modules']; + if ($modname = $this->get_module_name()) { + // Set the form suffix to the module name so that the form identifier is unique for each module type. + $this->set_suffix('_' . $modname); + } + + parent::__construct($action, $customdata, $method, $target, $attributes, $editable, $ajaxformdata); + } + + /** * Returns list of types of selected modules * @@ -66,7 +85,7 @@ class core_completion_defaultedit_form extends core_completion_edit_base_form { throw new \moodle_exception('noformdesc'); } - list($module, $context, $cw, $cmrec, $data) = prepare_new_moduleinfo_data($course, $modname, 0); + list($module, $context, $cw, $cmrec, $data) = prepare_new_moduleinfo_data($course, $modname, 0, $this->get_suffix()); $data->return = 0; $data->sr = 0; $data->add = $modname; @@ -75,6 +94,7 @@ class core_completion_defaultedit_form extends core_completion_edit_base_form { $mformclassname = 'mod_'.$modname.'_mod_form'; $PAGE->start_collecting_javascript_requirements(); $this->_moduleform = new $mformclassname($data, 0, $cmrec, $course); + $this->_moduleform->set_suffix('_' . $modname); $PAGE->end_collecting_javascript_requirements(); return $this->_moduleform; @@ -101,7 +121,12 @@ class core_completion_defaultedit_form extends core_completion_edit_base_form { $modnames = array_keys($this->get_module_names()); $modname = $modnames[0]; // Pre-fill the form with the current completion rules of the first selected module type. - list($module, $context, $cw, $cmrec, $data) = prepare_new_moduleinfo_data($this->course, $modname, 0); + list($module, $context, $cw, $cmrec, $data) = prepare_new_moduleinfo_data( + $this->course, + $modname, + 0, + $this->get_suffix() + ); $data = (array)$data; $modform->data_preprocessing($data); // Unset fields that will conflict with this form and set data to this form. @@ -121,4 +146,12 @@ class core_completion_defaultedit_form extends core_completion_edit_base_form { protected function get_cm(): ?\stdClass { return null; } + + /** + * This method has been overridden because the form identifier must be unique for each module type. + * Otherwise, the form will display the same data for each module type once it's submitted. + */ + protected function get_form_identifier() { + return parent::get_form_identifier() . $this->get_suffix(); + } } diff --git a/completion/classes/edit_base_form.php b/completion/classes/edit_base_form.php index c7cf23446f0..52de135b51b 100644 --- a/completion/classes/edit_base_form.php +++ b/completion/classes/edit_base_form.php @@ -111,6 +111,19 @@ abstract class core_completion_edit_base_form extends moodleform { $moduleform->_form = $this->_form; if ($customcompletionelements = $moduleform->add_completion_rules()) { $this->hascustomrules = true; + foreach ($customcompletionelements as $customcompletionelement) { + // Instead of checking for the suffix at the end of the element name, we need to check for its presence + // because some modules, like SCORM, are adding things at the end. + if (!str_contains($customcompletionelement, $this->get_suffix())) { + debugging( + 'Custom completion rule ' . $customcompletionelement . ' of module ' . $modnames[0] . + ' has wrong suffix and has been removed from the form. This has to be fixed by the developer', + DEBUG_DEVELOPER + ); + $moduleform->_form->removeElement($customcompletionelement); + } + } + } return $customcompletionelements; } catch (Exception $e) { diff --git a/completion/classes/form/form_trait.php b/completion/classes/form/form_trait.php index 9faf5b23d66..166f010d23c 100644 --- a/completion/classes/form/form_trait.php +++ b/completion/classes/form/form_trait.php @@ -28,6 +28,9 @@ use core_grades\component_gradeitems; */ trait form_trait { + /** @var string The suffix to be added to the completion elements when creating them (for example, 'completion_assign'). */ + protected $suffix = ''; + /** * Called during validation. * Override this method to indicate, based on the data, whether a custom completion rule is selected or not. @@ -59,6 +62,24 @@ trait form_trait { throw new \coding_exception('This class does not have a _form property. Please, add it or override the get_form() method.'); } + /** + * Set the suffix to be added to the completion elements when creating them (for example, 'completion_assign'). + * + * @param string $suffix + */ + public function set_suffix(string $suffix): void { + $this->suffix = $suffix; + } + + /** + * Get the suffix to be added to the completion elements when creating them (for example, 'completion_assign'). + * + * @return string The suffix + */ + public function get_suffix(): string { + return $this->suffix; + } + /** * Get the cm (course module) associated to this class. * This method must be overriden by the class using this trait if it doesn't include a _cm property. @@ -109,6 +130,7 @@ trait form_trait { } // Unlock button if people have completed it. The button will be removed later in definition_after_data if they haven't. + // The unlock buttons don't need suffix because they are only displayed in the module settings page. $mform->addElement('submit', 'unlockcompletion', get_string('unlockcompletion', 'completion')); $mform->registerNoSubmitButton('unlockcompletion'); $mform->addElement('hidden', 'completionunlocked', 0); @@ -125,27 +147,32 @@ trait form_trait { } } + // Get the sufix to add to the completion elements name. + $suffix = $this->get_suffix(); + + $completionel = 'completion' . $suffix; $mform->addElement( 'select', - 'completion', + $completionel, get_string('completion', 'completion'), [ COMPLETION_TRACKING_NONE => get_string('completion_none', 'completion'), COMPLETION_TRACKING_MANUAL => get_string('completion_manual', 'completion'), ] ); - $mform->setDefault('completion', $trackingdefault); - $mform->addHelpButton('completion', 'completion', 'completion'); + $mform->setDefault($completionel, $trackingdefault); + $mform->addHelpButton($completionel, 'completion', 'completion'); // Automatic completion once you view it. $autocompletionpossible = false; if ($supportviews) { - $mform->addElement('checkbox', 'completionview', get_string('completionview', 'completion'), + $completionviewel = 'completionview' . $suffix; + $mform->addElement('checkbox', $completionviewel, get_string('completionview', 'completion'), get_string('completionview_desc', 'completion')); - $mform->hideIf('completionview', 'completion', 'ne', COMPLETION_TRACKING_AUTOMATIC); + $mform->hideIf($completionviewel, $completionel, 'ne', COMPLETION_TRACKING_AUTOMATIC); // Check by default if automatic completion tracking is set. if ($trackingdefault == COMPLETION_TRACKING_AUTOMATIC) { - $mform->setDefault('completionview', 1); + $mform->setDefault($completionviewel, 1); } $autocompletionpossible = true; } @@ -164,23 +191,24 @@ trait form_trait { if ($customcompletionelements !== null) { foreach ($customcompletionelements as $element) { - $mform->hideIf($element, 'completion', 'ne', COMPLETION_TRACKING_AUTOMATIC); + $mform->hideIf($element, $completionel, 'ne', COMPLETION_TRACKING_AUTOMATIC); } $autocompletionpossible = $autocompletionpossible || count($customcompletionelements) > 0; } // Automatic option only appears if possible. if ($autocompletionpossible) { - $mform->getElement('completion')->addOption( + $mform->getElement($completionel)->addOption( get_string('completion_automatic', 'completion'), COMPLETION_TRACKING_AUTOMATIC); } // Completion expected at particular date? (For progress tracking). - $mform->addElement('date_time_selector', 'completionexpected', get_string('completionexpected', 'completion'), + $completionexpectedel = 'completionexpected' . $suffix; + $mform->addElement('date_time_selector', $completionexpectedel, get_string('completionexpected', 'completion'), ['optional' => true]); - $mform->addHelpButton('completionexpected', 'completionexpected', 'completion'); - $mform->hideIf('completionexpected', 'completion', 'eq', COMPLETION_TRACKING_NONE); + $mform->addHelpButton($completionexpectedel, 'completionexpected', 'completion'); + $mform->hideIf($completionexpectedel, $completionel, 'eq', COMPLETION_TRACKING_NONE); } /** @@ -195,46 +223,52 @@ trait form_trait { ): void { $mform = $this->get_form(); - $completionelementexists = $mform->elementExists('completion'); + // Get the sufix to add to the completion elements name. + $suffix = $this->get_suffix(); + + $completionel = 'completion' . $suffix; + $completionelementexists = $mform->elementExists($completionel); $component = "mod_{$modname}"; $itemnames = component_gradeitems::get_itemname_mapping_for_component($component); if (count($itemnames) === 1) { // Only one gradeitem in this activity. // We use the completionusegrade field here. + $completionusegradeel = 'completionusegrade' . $suffix; $mform->addElement( 'checkbox', - 'completionusegrade', + $completionusegradeel, get_string('completionusegrade', 'completion'), get_string('completionusegrade_desc', 'completion') ); - $mform->addHelpButton('completionusegrade', 'completionusegrade', 'completion'); + $mform->addHelpButton($completionusegradeel, 'completionusegrade', 'completion'); // Complete if the user has reached the pass grade. + $completionpassgradeel = 'completionpassgrade' . $suffix; $mform->addElement( 'checkbox', - 'completionpassgrade', null, + $completionpassgradeel, null, get_string('completionpassgrade_desc', 'completion') ); - $mform->disabledIf('completionpassgrade', 'completionusegrade', 'notchecked'); - $mform->addHelpButton('completionpassgrade', 'completionpassgrade', 'completion'); + $mform->disabledIf($completionpassgradeel, $completionusegradeel, 'notchecked'); + $mform->addHelpButton($completionpassgradeel, 'completionpassgrade', 'completion'); if ($completionelementexists) { - $mform->hideIf('completionpassgrade', 'completion', 'ne', COMPLETION_TRACKING_AUTOMATIC); - $mform->hideIf('completionusegrade', 'completion', 'ne', COMPLETION_TRACKING_AUTOMATIC); + $mform->hideIf($completionpassgradeel, $completionel, 'ne', COMPLETION_TRACKING_AUTOMATIC); + $mform->hideIf($completionusegradeel, $completionel, 'ne', COMPLETION_TRACKING_AUTOMATIC); } // The disabledIf logic differs between ratings and other grade items due to different field types. if ($rating) { // If using the rating system, there is no grade unless ratings are enabled. - $mform->disabledIf('completionusegrade', 'assessed', 'eq', 0); - $mform->disabledIf('completionpassgrade', 'assessed', 'eq', 0); + $mform->disabledIf($completionusegradeel, 'assessed', 'eq', 0); + $mform->disabledIf($completionusegradeel, 'assessed', 'eq', 0); } else { // All other field types use the '$gradefieldname' field's modgrade_type. $itemnumbers = array_keys($itemnames); $itemnumber = array_shift($itemnumbers); $gradefieldname = component_gradeitems::get_field_name_for_itemnumber($component, $itemnumber, 'grade'); - $mform->disabledIf('completionusegrade', "{$gradefieldname}[modgrade_type]", 'eq', 'none'); - $mform->disabledIf('completionpassgrade', "{$gradefieldname}[modgrade_type]", 'eq', 'none'); + $mform->disabledIf($completionusegradeel, "{$gradefieldname}[modgrade_type]", 'eq', 'none'); + $mform->disabledIf($completionusegradeel, "{$gradefieldname}[modgrade_type]", 'eq', 'none'); } } else if (count($itemnames) > 1) { // There are multiple grade items in this activity. @@ -246,25 +280,27 @@ trait form_trait { $options[$itemnumber] = get_string("grade_{$itemname}_name", $component); } + $completiongradeitemnumberel = 'completiongradeitemnumber' . $suffix; $mform->addElement( 'select', - 'completiongradeitemnumber', + $completiongradeitemnumberel, get_string('completionusegrade', 'completion'), $options ); // Complete if the user has reached the pass grade. + $completionpassgradeel = 'completionpassgrade' . $suffix; $mform->addElement( 'checkbox', - 'completionpassgrade', null, + $completionpassgradeel, null, get_string('completionpassgrade_desc', 'completion') ); - $mform->disabledIf('completionpassgrade', 'completiongradeitemnumber', 'eq', ''); - $mform->addHelpButton('completionpassgrade', 'completionpassgrade', 'completion'); + $mform->disabledIf($completionpassgradeel, $completiongradeitemnumberel, 'eq', ''); + $mform->addHelpButton($completionpassgradeel, 'completionpassgrade', 'completion'); if ($completionelementexists) { - $mform->hideIf('completiongradeitemnumber', 'completion', 'ne', COMPLETION_TRACKING_AUTOMATIC); - $mform->hideIf('completionpassgrade', 'completion', 'ne', COMPLETION_TRACKING_AUTOMATIC); + $mform->hideIf($completiongradeitemnumberel, $completionel, 'ne', COMPLETION_TRACKING_AUTOMATIC); + $mform->hideIf($completionpassgradeel, $completionel, 'ne', COMPLETION_TRACKING_AUTOMATIC); } } } @@ -278,20 +314,29 @@ trait form_trait { protected function validate_completion(array $data): array { $errors = []; + // Get the sufix to add to the completion elements name. + $suffix = $this->get_suffix(); + + $completionel = 'completion' . $suffix; // Completion: Don't let them choose automatic completion without turning on some conditions. - $automaticcompletion = array_key_exists('completion', $data) && $data['completion'] == COMPLETION_TRACKING_AUTOMATIC; + $automaticcompletion = array_key_exists($completionel, $data) && $data[$completionel] == COMPLETION_TRACKING_AUTOMATIC; // Ignore this check when completion settings are locked, as the options are then disabled. + // The unlock buttons don't need suffix because they are only displayed in the module settings page. $automaticcompletion = $automaticcompletion && !empty($data['completionunlocked']); if ($automaticcompletion) { // View to complete. - $rulesenabled = !empty($data['completionview']); + $completionviewel = 'completionview' . $suffix; + $rulesenabled = !empty($data[$completionviewel]); // Use grade to complete (only one grade item). - $rulesenabled = $rulesenabled || !empty($data['completionusegrade']) || !empty($data['completionpassgrade']); + $completionusegradeel = 'completionusegrade' . $suffix; + $completionpassgradeel = 'completionpassgrade' . $suffix; + $rulesenabled = $rulesenabled || !empty($data[$completionusegradeel]) || !empty($data[$completionpassgradeel]); // Use grade to complete (specific grade item). - if (!$rulesenabled && isset($data['completiongradeitemnumber'])) { - $rulesenabled = $data['completiongradeitemnumber'] != ''; + $completiongradeitemnumberel = 'completiongradeitemnumber' . $suffix; + if (!$rulesenabled && isset($data[$completiongradeitemnumberel])) { + $rulesenabled = $data[$completiongradeitemnumberel] != ''; } // Module-specific completion rules. @@ -299,7 +344,7 @@ trait form_trait { if (!$rulesenabled) { // No rules are enabled. Can't set automatically completed without rules. - $errors['completion'] = get_string('badautocompletion', 'completion'); + $errors[$completionel] = get_string('badautocompletion', 'completion'); } } @@ -311,16 +356,18 @@ trait form_trait { */ protected function definition_after_data_completion(): void { global $COURSE; - $mform = $this->get_form(); $completion = new \completion_info($COURSE); if ($completion->is_enabled()) { + $suffix = $this->get_suffix(); + // If anybody has completed the activity, these options will be 'locked'. $cm = $this->get_cm(); $completedcount = empty($cm) ? 0 : $completion->count_user_data($cm); $freeze = false; if (!$completedcount) { + // The unlock buttons don't need suffix because they are only displayed in the module settings page. if ($mform->elementExists('unlockcompletion')) { $mform->removeElement('unlockcompletion'); } @@ -355,25 +402,30 @@ trait form_trait { } if ($freeze) { - $mform->freeze('completion'); - if ($mform->elementExists('completionview')) { + $completionel = 'completion' . $suffix; + $mform->freeze($completionel); + $completionviewel = 'completionview' . $suffix; + if ($mform->elementExists($completionviewel)) { // Don't use hardFreeze or checkbox value gets lost. - $mform->freeze('completionview'); + $mform->freeze($completionviewel); } - if ($mform->elementExists('completionusegrade')) { - $mform->freeze('completionusegrade'); + $completionusegradeel = 'completionusegrade' . $suffix; + if ($mform->elementExists($completionusegradeel)) { + $mform->freeze($completionusegradeel); } - if ($mform->elementExists('completionpassgrade')) { - $mform->freeze('completionpassgrade'); + $completionpassgradeel = 'completionpassgrade' . $suffix; + if ($mform->elementExists($completionpassgradeel)) { + $mform->freeze($completionpassgradeel); // Has the completion pass grade completion criteria been set? If it has, then we shouldn't change // the gradepass field. - if ($mform->exportValue('completionpassgrade')) { + if ($mform->exportValue($completionpassgradeel)) { $mform->freeze('gradepass'); } } - if ($mform->elementExists('completiongradeitemnumber')) { - $mform->freeze('completiongradeitemnumber'); + $completiongradeitemnumberel = 'completiongradeitemnumber' . $suffix; + if ($mform->elementExists($completiongradeitemnumberel)) { + $mform->freeze($completiongradeitemnumberel); } if (property_exists($this, '_customcompletionelements')) { $mform->freeze($this->_customcompletionelements); diff --git a/course/modlib.php b/course/modlib.php index eea0e6e848e..fb2dc17c9b0 100644 --- a/course/modlib.php +++ b/course/modlib.php @@ -846,10 +846,11 @@ function get_moduleinfo_data($cm, $course) { * @param stdClass $course course object * @param string $modulename module name * @param int $section section number + * @param string $suffix the suffix to add to the name of the completion rules. * @return array module information about other required data * @since Moodle 3.2 */ -function prepare_new_moduleinfo_data($course, $modulename, $section) { +function prepare_new_moduleinfo_data($course, $modulename, $section, string $suffix = '') { global $CFG; list($module, $context, $cw) = can_add_moduleinfo($course, $modulename, $section); @@ -870,7 +871,7 @@ function prepare_new_moduleinfo_data($course, $modulename, $section) { $data->downloadcontent = DOWNLOAD_COURSE_CONTENT_ENABLED; // Apply completion defaults. - $defaults = \core_completion\manager::get_default_completion($course, $module); + $defaults = \core_completion\manager::get_default_completion($course, $module, true, $suffix); foreach ($defaults as $key => $value) { $data->$key = $value; } diff --git a/course/tests/modlib_test.php b/course/tests/modlib_test.php index d021888fac0..6d541ae4926 100644 --- a/course/tests/modlib_test.php +++ b/course/tests/modlib_test.php @@ -80,6 +80,50 @@ class modlib_test extends \advanced_testcase { prepare_new_moduleinfo_data($course, $assignmodule->name, $sectionnumber); } + /** + * Test prepare_new_moduleinfo_data with suffix (which is currently only used by the completion rules). + * @covers ::prepare_new_moduleinfo_data + */ + public function test_prepare_new_moduleinfo_data_with_suffix() { + global $DB; + $this->resetAfterTest(true); + + $this->setAdminUser(); + $course = self::getDataGenerator()->create_course(); + $coursecontext = \context_course::instance($course->id); + // Test with a complex module, like assign. + $assignmodule = $DB->get_record('modules', ['name' => 'assign'], '*', MUST_EXIST); + $sectionnumber = 1; + + $suffix = 'mysuffix'; + [$module, $context, $cw, $cm, $data] = prepare_new_moduleinfo_data($course, $assignmodule->name, $sectionnumber, $suffix); + $this->assertEquals($assignmodule, $module); + $this->assertEquals($coursecontext, $context); + $this->assertNull($cm); // Not cm yet. + + $expecteddata = new \stdClass(); + $expecteddata->section = $sectionnumber; + $expecteddata->visible = 1; + $expecteddata->course = $course->id; + $expecteddata->module = $module->id; + $expecteddata->modulename = $module->name; + $expecteddata->groupmode = $course->groupmode; + $expecteddata->groupingid = $course->defaultgroupingid; + $expecteddata->id = ''; + $expecteddata->instance = ''; + $expecteddata->coursemodule = ''; + $expecteddata->advancedgradingmethod_submissions = ''; // Not grading methods enabled by default. + $expecteddata->{'completion' . $suffix} = 0; + $expecteddata->downloadcontent = DOWNLOAD_COURSE_CONTENT_ENABLED; + + // Unset untestable. + unset($data->introeditor); + unset($data->_advancedgradingdata); + + $this->assertEquals($expecteddata, $data); + $this->assertFalse(property_exists($data, 'completion')); + } + /** * Test get_moduleinfo_data */ diff --git a/course/upgrade.txt b/course/upgrade.txt index 514061e2815..5300481c74f 100644 --- a/course/upgrade.txt +++ b/course/upgrade.txt @@ -5,6 +5,7 @@ information provided here is intended especially for developers. * The `core_course_renderer::course_section_cm_completion` method has been removed, and can no longer be used * External function core_course_external::get_course_contents() now returns a new field activitybadge with the data to display the activity badge when the module implements it. +* prepare_new_moduleinfo_data() now accepts a parameter "suffix" that will be added to the name of the completion rules. === 4.2 === * course/mod.php now accepts parameter beforemod for adding course modules. It contains the course module id