MDL-58138 completion: Fixes for a number of small issues.

backup/moodle2/restore_stepslib.php

@@ -5441,13 +5441,6 @@ class restore_completion_defaults_structure_step extends restore_structure_step
      * To conditionally decide if this step must be executed.
      */
     protected function execute_condition() {
-        global $CFG;
-
-        // Completion disabled in this site, don't execute.
-        if (empty($CFG->enablecompletion)) {
-            return false;
-        }
-
         // No completion on the front page.
         if ($this->get_courseid() == SITEID) {
             return false;

completion/classes/bulkedit_form.php

@@ -83,13 +83,9 @@ protected function get_module_form() {
 
         // Initialise the form but discard all JS requirements it adds, our form has already added them.
         $mformclassname = 'mod_'.$modname.'_mod_form';
-        if (!defined('PHPUNIT_TEST') || !PHPUNIT_TEST) {
-            $PAGE->start_collecting_javascript_requirements();
-        }
+        $PAGE->start_collecting_javascript_requirements();
         $this->_moduleform = new $mformclassname($data, 0, $cmrec, $course);
-        if (!defined('PHPUNIT_TEST') || !PHPUNIT_TEST) {
-            $PAGE->end_collecting_javascript_requirements();
-        }
+        $PAGE->end_collecting_javascript_requirements();
 
         return $this->_moduleform;
     }

completion/classes/defaultedit_form.php

@@ -83,13 +83,9 @@ protected function get_module_form() {
 
         // Initialise the form but discard all JS requirements it adds, our form has already added them.
         $mformclassname = 'mod_'.$modname.'_mod_form';
-        if (!defined('PHPUNIT_TEST') || !PHPUNIT_TEST) {
-            $PAGE->start_collecting_javascript_requirements();
-        }
+        $PAGE->start_collecting_javascript_requirements();
         $this->_moduleform = new $mformclassname($data, 0, $cmrec, $course);
-        if (!defined('PHPUNIT_TEST') || !PHPUNIT_TEST) {
-            $PAGE->end_collecting_javascript_requirements();
-        }
+        $PAGE->end_collecting_javascript_requirements();
 
         return $this->_moduleform;
     }

completion/classes/manager.php

@@ -110,8 +110,9 @@ public function get_activities($cmids, $withcompletiondetails = false) {
             } else {
                 $moduleobject->completionstatus = ['icon' => null, 'string' => null];
             }
-
-            $activities[] = $moduleobject;
+            if (self::can_edit_bulk_completion($this->courseid, $mod)) {
+                $activities[] = $moduleobject;
+            }
         }
         return $activities;
     }
@@ -216,7 +217,7 @@ public function get_activities_and_resources() {
             $module->icon = $OUTPUT->image_url('icon', $module->name)->out();
             $module->formattedname = format_string(get_string('modulenameplural', 'mod_' . $module->name),
                 true, ['context' => $coursecontext]);
-            $module->canmanage = $canmanage && \course_allowed_module($course, $module->name);
+            $module->canmanage = $canmanage && course_allowed_module($course, $module->name);
             $defaults = self::get_default_completion($course, $module, false);
             $defaults->modname = $module->name;
             $module->completionstatus = $this->get_completion_detail($defaults);
@@ -390,8 +391,12 @@ public function apply_default_completion($data, $updatecustomrules) {
         if (!$modids = $data->modids) {
             return;
         }
-        $defaults = ['completion' => COMPLETION_DISABLED, 'completionview' => COMPLETION_VIEW_NOT_REQUIRED,
-            'completionexpected' => 0, 'completionusegrade' => 0];
+        $defaults = [
+            'completion' => COMPLETION_DISABLED,
+            'completionview' => COMPLETION_VIEW_NOT_REQUIRED,
+            'completionexpected' => 0,
+            'completionusegrade' => 0
+        ];
 
         $data = (array)$data;
 
@@ -407,22 +412,30 @@ public function apply_default_completion($data, $updatecustomrules) {
         $params[] = 1;
         $modules = $DB->get_records_select_menu('modules', 'id ' . $modidssql . ' and visible = ?', $params, '', 'id, name');
 
+        // Get an associative array of [module_id => course_completion_defaults_id].
+        list($in, $params) = $DB->get_in_or_equal($modids);
+        $params[] = $courseid;
+        $defaultsids = $DB->get_records_select_menu('course_completion_defaults', 'module ' . $in . ' and course = ?', $params, '',
+                                                      'module, id');
+
         foreach ($modids as $modid) {
             if (!array_key_exists($modid, $modules)) {
                 continue;
             }
-            if ($defaultsid = $DB->get_field('course_completion_defaults', 'id', ['course' => $courseid, 'module' => $modid])) {
-                $DB->update_record('course_completion_defaults', $data + ['id' => $defaultsid]);
+            if (isset($defaultsids[$modid])) {
+                $DB->update_record('course_completion_defaults', $data + ['id' => $defaultsids[$modid]]);
             } else {
-                $defaultsid = $DB->insert_record('course_completion_defaults', $data + ['course' => $courseid, 'module' => $modid]);
+                $defaultsids[$modid] = $DB->insert_record('course_completion_defaults', $data + ['course' => $courseid,
+                                                                                                 'module' => $modid]);
             }
             // Trigger event.
             \core\event\completion_defaults_updated::create([
-                'objectid' => $defaultsid,
+                'objectid' => $defaultsids[$modid],
                 'context' => $coursecontext,
                 'other' => ['modulename' => $modules[$modid]],
             ])->trigger();
         }
+
         // Add notification.
         \core\notification::add(get_string('defaultcompletionupdated', 'completion'), \core\notification::SUCCESS);
     }

completion/tests/behat/bulk_edit_activity_completion.feature

@@ -58,4 +58,3 @@ Feature: Allow teachers to bulk edit activity completion rules in a course.
     And I should see "Student must receive a grade to complete this activity" in the "//div[contains(concat(' ', normalize-space(@class), ' '), ' row ')][.//*[text() = 'Test assignment two']]" "xpath_element"
     And I should see "Student must submit to this activity to complete it" in the "//div[contains(concat(' ', normalize-space(@class), ' '), ' row ')][.//*[text() = 'Test assignment two']]" "xpath_element"
     And I should not see "Completion expected on" in the "//div[contains(concat(' ', normalize-space(@class), ' '), ' row ')][.//*[text() = 'Test assignment two']]" "xpath_element"
-

completion/tests/behat/default_activity_completion.feature

@@ -1,14 +1,14 @@
 @core @core_completion
-Feature: Allow teachers to bulk edit activity completion rules in a course.
-  In order to avoid editing single activities
+Feature: Allow teachers to edit the default activity completion rules in a course.
+  In order to set the activity completion defaults for new activities
   As a teacher
   I need to be able to edit the completion rules for a group of activities.
 
   # Given I am a teacher in a course with completion tracking enabled and activities present.
   # When I edit activity completion defaults for activity types.
   # Then the completion rule defaults should apply only to activities created from that point onwards.
   @javascript
-  Scenario: Bulk edit activity completion rules
+  Scenario: Bulk edit activity completion default rules
     Given the following "courses" exist:
       | fullname | shortname | category |
       | Course 1 | C1 | 0 |
@@ -50,4 +50,3 @@ Feature: Allow teachers to bulk edit activity completion rules in a course.
     And I should see "Student must receive a grade to complete this activity" in the "//div[contains(concat(' ', normalize-space(@class), ' '), ' row ')][.//*[text() = 'Assignments']]" "xpath_element"
     And I should see "Student must submit to this activity to complete it" in the "//div[contains(concat(' ', normalize-space(@class), ' '), ' row ')][.//*[text() = 'Assignments']]" "xpath_element"
     And I should not see "Completion expected on" in the "//div[contains(concat(' ', normalize-space(@class), ' '), ' row ')][.//*[text() = 'Assignments']]" "xpath_element"
-

course/lib.php

@@ -3867,11 +3867,14 @@ function course_get_user_navigation_options($context, $course = null) {
 function course_get_user_administration_options($course, $context) {
     global $CFG;
     $isfrontpage = $course->id == SITEID;
+    $completionenabled = $CFG->enablecompletion && $course->enablecompletion;
+    $hascompletiontabs = count(core_completion\manager::get_available_completion_tabs($course, $context)) > 0;
 
     $options = new stdClass;
     $options->update = has_capability('moodle/course:update', $context);
-    $options->editcompletion = $CFG->enablecompletion && $course->enablecompletion &&
-        ($options->update || count(core_completion\manager::get_available_completion_tabs($course, $context)) > 0);
+    $options->editcompletion = $CFG->enablecompletion &&
+                               $course->enablecompletion &&
+                               ($options->update || $hascompletiontabs);
     $options->filters = has_capability('moodle/filter:manage', $context) &&
                         count(filter_get_available_in_context($context)) > 0;
     $options->reports = has_capability('moodle/site:viewreports', $context);

course/moodleform_mod.php

@@ -1078,7 +1078,7 @@ public function apply_admin_defaults($datetimeoffsets = array()) {
      *
      * @param stdClass $data passed by reference
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
     }
 
     /**

course/templates/activityinstance.mustache

@@ -34,20 +34,20 @@
 }}
 {{#activities}}
 <div class="row m-b-1 row-fluid">
-    <div class="activityinstance col-sm-6 span6">
+    <div class="activityinstance col-xs-6 span6">
         <div class="mod-indent-outer"></div>
         <div>
-                {{#canmanage}}
-                <label class="accesshide" for="selectactivity_{{cmid}}">Select {{modname}}</label>
-                <input type="checkbox" id="selectactivity_{{cmid}}" class="m-r-1" name="cmid[]" data-section="{{sectionnumber}}" value="{{cmid}}" aria-label="{{#str}}checkactivity, completion, {{modname}}{{/str}}">
-                {{/canmanage}}
-                <a href="{{url}}">
-                <img src="{{icon}}" class="iconlarge activityicon" alt=" " role="presentation" />
-                <span class="instancename">{{modname}}</span>
-                </a>
+            {{#canmanage}}
+            <label class="accesshide" for="selectactivity_{{cmid}}">{{#str}}select, completion{{/str}} {{modname}}</label>
+            <input type="checkbox" id="selectactivity_{{cmid}}" class="m-r-1" name="cmid[]" data-section="{{sectionnumber}}" value="{{cmid}}" aria-label="{{#str}}checkactivity, completion, {{modname}}{{/str}}">
+            {{/canmanage}}
+            <a href="{{url}}">
+            <img src="{{icon}}" class="iconlarge activityicon" alt=" " role="presentation" />
+            <span class="instancename">{{modname}}</span>
+            </a>
         </div>
     </div>
-    <div class="activity-completionstatus col-sm-6 span6" id="completionstatus_{{cmid}}">
+    <div class="activity-completionstatus col-xs-6 span6" id="completionstatus_{{cmid}}">
         <div class="col-sm-1 span1  p-l-0">
             {{#completionstatus.icon}}
                 {{{completionstatus.icon}}}

course/templates/bulkactivitycompletion.mustache

@@ -48,27 +48,29 @@
             <input type="submit" value="{{#str}}edit{{/str}}" class="btn btn-primary" name="submitbutton" aria-label="{{#str}}updateactivities, completion{{/str}}" disabled/>
         </div>
     </div>
-    <div class="top-section row m-b-1">
-        <div class="col-sm-6 span6">
+    <div class="row m-b-1">
+        <div class="col-xs-6 span6">
             <input type="checkbox" class="mastercheck m-r-1" aria-label="{{#str}}checkall, completion{{/str}}">
             <label class="font-weight-bold">{{#str}}activitieslabel, core_completion{{/str}}</label>
         </div>
-        <div class="col-sm-6">
+        <div class="col-xs-6 span6">
             <label class="font-weight-bold">{{#str}}completion, core_completion{{/str}}</label>
             <span>{{{helpicon}}}</span>
         </div>
     </div>
+    <hr class="row">
     <div class="topics">
         {{#sections}}
-                <div class="topic-section m-b-1">
-                    <div class="row m-b-1">
+                <div class="m-b-1">
+                    <div class="row m-b-1 row-fluid">
                         <div class="col-sm-12">
                             <input type="checkbox" data-section-master="{{sectionnumber}}" class="m-r-1" aria-label="{{#str}}checkallsection, completion, {{name}}{{/str}}">
-                            <h3>{{name}}</h3>
+                            <h3 class="d-inline-block">{{name}}</h3>
                         </div>
                     </div>
                     {{> core_course/activityinstance}}
                 </div>
+                <hr class="row">
         {{/sections}}
     </div>
     <input type="hidden" name="id" value="{{courseid}}" />

course/templates/defaultactivitycompletion.mustache

@@ -45,28 +45,29 @@
             <input type="submit" value="{{#str}}edit{{/str}}" class="btn btn-primary" name="submitbutton" aria-label="{{#str}}updateactivities, completion{{/str}}" disabled/>
         </div>
     </div>
-    <div class="top-section row m-b-1">
-        <div class="col-sm-6">
+    <div class="row m-b-1">
+        <div class="col-xs-6 span6">
             <input type="checkbox" class="mastercheck m-r-1" aria-label="{{#str}}checkall, completion{{/str}}">
             <label class="font-weight-bold">{{#str}}activitieslabel, core_completion{{/str}}</label>
         </div>
-        <div class="col-sm-6">
+        <div class="col-xs-6 span6">
             <label class="font-weight-bold">{{#str}}completion, core_completion{{/str}}</label>
             <span>{{{helpicon}}}</span>
         </div>
     </div>
+    <hr class="row">
     <div class="modules">
         {{#modules}}
             {{#canmanage}}
-             <div class="module-section m-b-1">
+             <div class="m-b-1">
                 <div class="row m-b-1 row-fluid">
-                    <div class="col-sm-6 span6">
-                        <label class="accesshide" for="modtype_{{id}}">Select {{formattedname}}</label>
+                    <div class="col-xs-6 span6">
+                        <label class="accesshide" for="modtype_{{id}}">{{#str}}select, core_completion{{/str}} {{formattedname}}</label>
                         <input id="modtype_{{id}}" type="checkbox" class="m-r-1" name="modids[]" value="{{id}}" aria-label="{{#str}}checkactivity, completion, {{formattedname}}{{/str}}">
                         <img src="{{icon}}" alt=" " role="presentation" />
                         <span>{{formattedname}}</span>
                     </div>
-                    <div class="activity-completionstatus col-sm-6 span6">
+                    <div class="activity-completionstatus col-xs-6 span6">
                         <div class="col-sm-1 span1 p-l-0">
                             {{#completionstatus.icon}}
                                 {{{completionstatus.icon}}}
@@ -81,14 +82,15 @@
                     </div>
                 </div>
             </div>
+            <hr class="row">
             {{/canmanage}}
         {{/modules}}
     </div>
     <input type="hidden" name="id" value="{{courseid}}" />
     <input type="hidden" name="sesskey" value="{{sesskey}}" />
     <div class="row">
         <div class="col">
-            <input type="submit" value="{{#str}}edit{{/str}}" class="btn btn-primary" name="submitbutton" />
+            <input type="submit" value="{{#str}}edit{{/str}}" class="btn btn-primary" name="submitbutton" disabled/>
         </div>
     </div>
 </form>

lang/en/completion.php

@@ -190,6 +190,7 @@
 $string['roleidnotfound'] = 'Role ID {$a} not found';
 $string['saved'] = 'Saved';
 $string['seedetails'] = 'See details';
+$string['select'] = 'Select';
 $string['self'] = 'Self';
 $string['selfcompletion'] = 'Self completion';
 $string['showinguser'] = 'Showing user';

lib/pagelib.php

@@ -908,6 +908,7 @@ public function end_collecting_javascript_requirements() {
             throw new coding_exception('JavaScript collection has not been started.');
         }
         $this->_requires = $this->savedrequires;
+        $this->savedrequires = null;
     }
 
     /**

mod/choice/mod_form.php

@@ -130,9 +130,9 @@ function data_preprocessing(&$default_values){
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         // Set up completion section even if checkbox is not ticked
         if (!empty($data->completionunlocked)) {

mod/data/mod_form.php

@@ -164,9 +164,9 @@ public function data_preprocessing(&$defaultvalues) {
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         if (!empty($data->completionunlocked)) {
             $autocompletion = !empty($data->completion) && $data->completion == COMPLETION_TRACKING_AUTOMATIC;

mod/feedback/mod_form.php

@@ -166,9 +166,9 @@ public function data_preprocessing(&$default_values) {
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         if (isset($data->page_after_submit_editor)) {
             $data->page_after_submitformat = $data->page_after_submit_editor['format'];

mod/forum/mod_form.php

@@ -298,9 +298,9 @@ function completion_rule_enabled($data) {
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         // Turn off completion settings if the checkboxes aren't ticked
         if (!empty($data->completionunlocked)) {

mod/glossary/mod_form.php

@@ -208,9 +208,9 @@ function completion_rule_enabled($data) {
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         if (!empty($data->completionunlocked)) {
             // Turn off completion settings if the checkboxes aren't ticked

mod/lesson/mod_form.php

@@ -438,9 +438,9 @@ public function completion_rule_enabled($data) {
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         // Turn off completion setting if the checkbox is not ticked.
         if (!empty($data->completionunlocked)) {

mod/scorm/mod_form.php

@@ -550,9 +550,9 @@ public function completion_rule_enabled($data) {
      *
      * Only available on moodleform_mod.
      *
-     * @param stdClass $data passed by reference
+     * @param stdClass $data the form data to be modified.
      */
-    public function data_postprocessing(&$data) {
+    public function data_postprocessing($data) {
         parent::data_postprocessing($data);
         // Convert completionstatusrequired to a proper integer, if any.
         $total = 0;