diff --git a/completion/classes/external.php b/completion/classes/external.php index 3d46b072f42..1cd714c5ae0 100644 --- a/completion/classes/external.php +++ b/completion/classes/external.php @@ -118,7 +118,7 @@ class core_completion_external extends external_api { * Describes the parameters for override_activity_completion_status. * * @return external_external_function_parameters - * @since Moodle 3.1 + * @since Moodle 3.4 */ public static function override_activity_completion_status_parameters() { return new external_function_parameters ( @@ -136,7 +136,7 @@ class core_completion_external extends external_api { * @param int $cmid Course module id * @param int $newstate Activity completion * @return array Result and possible warnings - * @since Moodle 3.1 + * @since Moodle 3.4 * @throws moodle_exception */ public static function override_activity_completion_status($userid, $cmid, $newstate) { @@ -149,8 +149,6 @@ class core_completion_external extends external_api { $cmid = $params['cmid']; $newstate = $params['newstate']; - $warnings = array(); - $context = context_module::instance($cmid); self::validate_context($context); @@ -191,9 +189,8 @@ class core_completion_external extends external_api { ($cm->completion == COMPLETION_TRACKING_AUTOMATIC ? 'auto' : 'manual'). '-'.$completiontype; - $overridebyuser = $DB->get_record('user', array('id' => $USER->id), '*', MUST_EXIST); - $describe = get_string('completion-' . $completiontype, 'completion', fullname($overridebyuser)); - $user = $DB->get_record('user', array('id' => $userid), '*', MUST_EXIST); + $describe = get_string('completion-' . $completiontype, 'completion', fullname($USER)); + $user = \core_user::get_user($userid, '*', MUST_EXIST); $a = new StdClass; $a->state = $describe; $a->date = $date; @@ -203,15 +200,11 @@ class core_completion_external extends external_api { $img = $OUTPUT->pix_icon('i/' . $completionicon, s($fulldescribe)); - // Set data values for next completion change. - $otherstate = ($state == COMPLETION_COMPLETE) ? COMPLETION_INCOMPLETE : COMPLETION_COMPLETE; - $changecompl = $userid . '-' . $cmid . '-' . $otherstate; - - $result = array(); - $result['status'] = true; - $result['warnings'] = $warnings; - $result['changecompl'] = $changecompl; - $result['img'] = $img; + // Return the current state of completion. + $result = [ + 'completionstate' => $state, + 'img' => $img + ]; return $result; } @@ -219,15 +212,13 @@ class core_completion_external extends external_api { * Describes the override_activity_completion_status return value. * * @return external_single_structure - * @since Moodle 3.1 + * @since Moodle 3.4 */ public static function override_activity_completion_status_returns() { return new external_single_structure( array( - 'status' => new external_value(PARAM_BOOL, 'Status, true if success'), - 'warnings' => new external_warnings(), - 'changecompl' => new external_value(PARAM_ALPHANUMEXT, 'The new completion change data'), + 'completionstate' => new external_value(PARAM_BOOL, 'The current completion state.'), 'img' => new external_value(PARAM_RAW, 'Image element to replace existing one'), ) ); diff --git a/completion/tests/externallib_test.php b/completion/tests/externallib_test.php index ce3f5981368..27d526a7a3f 100644 --- a/completion/tests/externallib_test.php +++ b/completion/tests/externallib_test.php @@ -202,6 +202,7 @@ class core_completion_externallib_testcase extends externallib_advanced_testcase $this->getDataGenerator()->enrol_user($student->id, $course->id, $studentrole->id); $teacherrole = $DB->get_record('role', array('shortname' => 'teacher')); $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $teacherrole->id); + $coursecontext = context_course::instance($course->id); // Create 2 activities, one with manual completion (data), one with automatic completion triggered by viewiung it (forum). $data = $this->getDataGenerator()->create_module('data', ['course' => $course->id], ['completion' => 1]); @@ -219,28 +220,45 @@ class core_completion_externallib_testcase extends externallib_advanced_testcase $this->setUser($teacher); $result = core_completion_external::override_activity_completion_status($student->id, $data->cmid, COMPLETION_INCOMPLETE); $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result); - $this->assertTrue($result['status']); + $this->assertEquals($result['completionstate'], COMPLETION_INCOMPLETE); $completiondata = $completion->get_data($cmdata, false, $student->id); $this->assertEquals(COMPLETION_INCOMPLETE, $completiondata->completionstate); // Test overriding the status of the manual-completion-activity back to 'complete'. $result = core_completion_external::override_activity_completion_status($student->id, $data->cmid, COMPLETION_COMPLETE); $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result); - $this->assertTrue($result['status']); + $this->assertEquals($result['completionstate'], COMPLETION_COMPLETE); $completiondata = $completion->get_data($cmdata, false, $student->id); $this->assertEquals(COMPLETION_COMPLETE, $completiondata->completionstate); // Test overriding the status of the auto-completion-activity to 'complete'. $result = core_completion_external::override_activity_completion_status($student->id, $forum->cmid, COMPLETION_COMPLETE); $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result); - $this->assertTrue($result['status']); + $this->assertEquals($result['completionstate'], COMPLETION_COMPLETE); $completionforum = $completion->get_data($cmforum, false, $student->id); $this->assertEquals(COMPLETION_COMPLETE, $completionforum->completionstate); // Test overriding the status of the auto-completion-activity to 'incomplete'. $result = core_completion_external::override_activity_completion_status($student->id, $forum->cmid, COMPLETION_INCOMPLETE); $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result); - $this->assertTrue($result['status']); + $this->assertEquals($result['completionstate'], COMPLETION_INCOMPLETE); + $completionforum = $completion->get_data($cmforum, false, $student->id); + $this->assertEquals(COMPLETION_INCOMPLETE, $completionforum->completionstate); + + // Test overriding the status of the auto-completion-activity to an invalid state. It should remain incomplete. + $this->expectException('moodle_exception'); + $result = core_completion_external::override_activity_completion_status($student->id, $forum->cmid, 3); + $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result); + $this->assertEquals($result['completionstate'], COMPLETION_INCOMPLETE); + $completionforum = $completion->get_data($cmforum, false, $student->id); + $this->assertEquals(COMPLETION_INCOMPLETE, $completionforum->completionstate); + + // Test overriding the status of the auto-completion-activity for a user without capabilities. It should remain incomplete. + $this->expectException('moodle_exception'); + unassign_capability('moodle/course:overridecompletion', $teacherrole->id, $coursecontext); + $result = core_completion_external::override_activity_completion_status($student->id, $forum->cmid, 1); + $result = external_api::clean_returnvalue(core_completion_external::override_activity_completion_status_returns(), $result); + $this->assertEquals($result['completionstate'], COMPLETION_INCOMPLETE); $completionforum = $completion->get_data($cmforum, false, $student->id); $this->assertEquals(COMPLETION_INCOMPLETE, $completionforum->completionstate); } diff --git a/course/renderer.php b/course/renderer.php index 55cc100ef67..64b2575d4a8 100644 --- a/course/renderer.php +++ b/course/renderer.php @@ -510,7 +510,7 @@ class core_course_renderer extends plugin_renderer_base { if ($completiondata->overrideby) { $args = new stdClass(); $args->modname = $formattedname; - $overridebyuser = $DB->get_record('user', array('id' => $completiondata->overrideby), '*', MUST_EXIST); + $overridebyuser = \core_user::get_user($completiondata->overrideby, '*', MUST_EXIST); $args->overrideuser = fullname($overridebyuser); $imgalt = get_string('completion-alt-' . $completionicon, 'completion', $args); } else { diff --git a/lib/classes/event/course_module_completion_updated.php b/lib/classes/event/course_module_completion_updated.php index 7ac1ff91490..ab03bb94865 100644 --- a/lib/classes/event/course_module_completion_updated.php +++ b/lib/classes/event/course_module_completion_updated.php @@ -127,6 +127,7 @@ class course_module_completion_updated extends base { public static function get_other_mapping() { $othermapped = array(); $othermapped['relateduserid'] = array('db' => 'user', 'restore' => 'user'); + $othermapped['overrideby'] = array('db' => 'user', 'restore' => 'user'); return $othermapped; } diff --git a/lib/completionlib.php b/lib/completionlib.php index a965364185c..89827fb25f8 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -519,6 +519,16 @@ class completion_info { return $ccompletion->is_complete(); } + /** + * Check whether the supplied user can override the activity completion statuses within the current course. + * + * @param stdClass $user The user object. + * @return bool True if the user can override, false otherwise. + */ + public function user_can_override_completion($user) { + return has_capability('moodle/course:overridecompletion', context_course::instance($this->course_id), $user); + } + /** * Updates (if necessary) the completion state of activity $cm for the given * user. @@ -550,6 +560,7 @@ class completion_info { * @param int $userid User ID to be updated. Default 0 = current user * @param bool $override Whether manually overriding the existing completion state. * @return void + * @throws moodle_exception if trying to override without permission. */ public function update_state($cm, $possibleresult=COMPLETION_UNKNOWN, $userid=0, $override = false) { global $USER; @@ -559,6 +570,14 @@ class completion_info { return; } + // If we're processing an override and the current user isn't allowed to do so, then throw an exception. + if ($override) { + if (!$this->user_can_override_completion($USER)) { + throw new required_capability_exception(context_course::instance($this->course_id), + 'moodle/course:overridecompletion', 'nopermission', ''); + } + } + // Get current value of completion state and do nothing if it's same as // the possible result of this change. If the change is to COMPLETE and the // current value is one of the COMPLETE_xx subtypes, ignore that as well diff --git a/lib/tests/completionlib_test.php b/lib/tests/completionlib_test.php index 00bd597ff79..cc2dfbb57c2 100644 --- a/lib/tests/completionlib_test.php +++ b/lib/tests/completionlib_test.php @@ -128,10 +128,10 @@ class core_completionlib_testcase extends advanced_testcase { $this->mock_setup(); $mockbuilder = $this->getMockBuilder('completion_info'); - $mockbuilder->setMethods(array('is_enabled', 'get_data', 'internal_get_state', 'internal_set_data')); + $mockbuilder->setMethods(array('is_enabled', 'get_data', 'internal_get_state', 'internal_set_data', + 'user_can_override_completion')); $mockbuilder->setConstructorArgs(array((object)array('id' => 42))); $c = $mockbuilder->getMock(); - $cm = (object)array('id'=>13, 'course'=>42); // Not enabled, should do nothing. @@ -229,7 +229,10 @@ class core_completionlib_testcase extends advanced_testcase { ->method('is_enabled') ->with($cm) ->will($this->returnValue(true)); - $c->expects($this->at(1)) + $c->expects($this->at(1)) // Pretend the user has the required capability for overriding completion statuses. + ->method('user_can_override_completion') + ->will($this->returnValue(true)); + $c->expects($this->at(2)) ->method('get_data') ->with($cm, false, 100) ->will($this->returnValue($current)); @@ -239,7 +242,7 @@ class core_completionlib_testcase extends advanced_testcase { $changed->overrideby = 314159; $comparewith = new phpunit_constraint_object_is_equal_with_exceptions($changed); $comparewith->add_exception('timemodified', 'assertGreaterThanOrEqual'); - $c->expects($this->at(2)) + $c->expects($this->at(3)) ->method('internal_set_data') ->with($cm, $comparewith); $c->update_state($cm, COMPLETION_COMPLETE, 100, true); @@ -258,7 +261,10 @@ class core_completionlib_testcase extends advanced_testcase { ->method('is_enabled') ->with($cm) ->will($this->returnValue(true)); - $c->expects($this->at(1)) + $c->expects($this->at(1)) // Pretend the user has the required capability for overriding completion statuses. + ->method('user_can_override_completion') + ->will($this->returnValue(true)); + $c->expects($this->at(2)) ->method('get_data') ->with($cm, false, 100) ->will($this->returnValue($current)); @@ -268,7 +274,7 @@ class core_completionlib_testcase extends advanced_testcase { $changed->overrideby = 314159; $comparewith = new phpunit_constraint_object_is_equal_with_exceptions($changed); $comparewith->add_exception('timemodified', 'assertGreaterThanOrEqual'); - $c->expects($this->at(2)) + $c->expects($this->at(3)) ->method('internal_set_data') ->with($cm, $comparewith); $c->update_state($cm, COMPLETION_COMPLETE, 100, true); @@ -294,7 +300,10 @@ class core_completionlib_testcase extends advanced_testcase { ->method('is_enabled') ->with($cm) ->will($this->returnValue(true)); - $c->expects($this->at(1)) + $c->expects($this->at(1)) // Pretend the user has the required capability for overriding completion statuses. + ->method('user_can_override_completion') + ->will($this->returnValue(true)); + $c->expects($this->at(2)) ->method('get_data') ->with($cm, false, 100) ->will($this->returnValue($current)); @@ -304,7 +313,7 @@ class core_completionlib_testcase extends advanced_testcase { $changed->overrideby = 314159; $comparewith = new phpunit_constraint_object_is_equal_with_exceptions($changed); $comparewith->add_exception('timemodified', 'assertGreaterThanOrEqual'); - $c->expects($this->at(2)) + $c->expects($this->at(3)) ->method('internal_set_data') ->with($cm, $comparewith); $c->update_state($cm, COMPLETION_INCOMPLETE, 100, true); diff --git a/report/progress/amd/build/completion_override.min.js b/report/progress/amd/build/completion_override.min.js index fb55bac3aad..70785adb4ba 100644 --- a/report/progress/amd/build/completion_override.min.js +++ b/report/progress/amd/build/completion_override.min.js @@ -1 +1 @@ -define(["jquery","core/ajax","core/str","core/modal_factory","core/modal_events","core/notification","core/custom_interaction_events","core/templates"],function(a,b,c,d,e,f,g,h){var i,j=function(a){h.render("core/loading",{}).then(function(c){return i.append(c),b.call([{methodname:"core_completion_override_activity_completion_status",args:a}])[0]}).then(function(a){i.find(".loading-icon").remove(),i.data("changecompl",a.changecompl),i.attr("data-changecompl",a.changecompl),i.children("img").replaceWith(a.img)})["catch"](f.exception)},k=function(b,g){g.originalEvent.preventDefault(),b.preventDefault(),i=a(b.currentTarget);var h=i.data("changecompl").split("-"),k={userid:h[0],cmid:h[1],newstate:h[2]},l=1==k.newstate?"completion-y":"completion-n";c.get_strings([{key:l,component:"completion"}]).then(function(a){return c.get_strings([{key:"confirm",component:"moodle"},{key:"areyousureoverridecompletion",component:"completion",param:a[0]}])}).then(function(a){return d.create({type:d.types.CONFIRM,title:a[0],body:a[1]})}).then(function(a){a.getRoot().on(e.yes,function(){j(k)}),a.getRoot().on(e.hidden,function(){i.focus(),a.destroy()}),a.show()})["catch"](f.exception)},l=function(){a("#completion-progress a.changecompl").each(function(a,b){g.define(b,[g.events.activate])}),a("#completion-progress").on(g.events.activate,"a.changecompl",function(a,b){k(a,b)})};return{init:l}}); \ No newline at end of file +define(["jquery","core/ajax","core/str","core/modal_factory","core/modal_events","core/notification","core/custom_interaction_events","core/templates"],function(a,b,c,d,e,f,g,h){var i,j=function(a){h.render("core/loading",{}).then(function(c){return i.append(c),b.call([{methodname:"core_completion_override_activity_completion_status",args:a}])[0]}).then(function(b){var c=b.completionstate?0:1;i.find(".loading-icon").remove(),i.data("changecompl",a.userid+"-"+a.cmid+"-"+c),i.attr("data-changecompl",a.userid+"-"+a.cmid+"-"+c),i.children("img").replaceWith(b.img)})["catch"](f.exception)},k=function(b,g){g.originalEvent.preventDefault(),g.originalEvent.stopPropagation(),b.preventDefault(),b.stopPropagation(),i=a(b.currentTarget);var h=i.data("changecompl").split("-"),k={userid:h[0],cmid:h[1],newstate:h[2]},l=1==k.newstate?"completion-y":"completion-n";c.get_strings([{key:l,component:"completion"}]).then(function(a){return c.get_strings([{key:"confirm",component:"moodle"},{key:"areyousureoverridecompletion",component:"completion",param:a[0]}])}).then(function(a){return d.create({type:d.types.CONFIRM,title:a[0],body:a[1]})}).then(function(a){a.getRoot().on(e.yes,function(){j(k)}),a.getRoot().on(e.hidden,function(){i.focus(),a.destroy()}),a.show()})["catch"](f.exception)},l=function(){a("#completion-progress a.changecompl").each(function(a,b){g.define(b,[g.events.activate])}),a("#completion-progress").on(g.events.activate,"a.changecompl",function(a,b){k(a,b)})};return{init:l}}); \ No newline at end of file diff --git a/report/progress/amd/src/completion_override.js b/report/progress/amd/src/completion_override.js index 7d6689a7b9a..3454e9dc61d 100644 --- a/report/progress/amd/src/completion_override.js +++ b/report/progress/amd/src/completion_override.js @@ -51,9 +51,10 @@ define(['jquery', 'core/ajax', 'core/str', 'core/modal_factory', 'core/modal_eve }])[0]; }).then(function(results) { // Update the DOM accordingly. + var flipState = results.completionstate ? 0 : 1; triggerElement.find('.loading-icon').remove(); - triggerElement.data('changecompl', results.changecompl); - triggerElement.attr('data-changecompl', results.changecompl); + triggerElement.data('changecompl', override.userid + '-' + override.cmid + '-' + flipState); + triggerElement.attr('data-changecompl', override.userid + '-' + override.cmid + '-' + flipState); triggerElement.children("img").replaceWith(results.img); return; }).catch(Notification.exception); @@ -68,7 +69,9 @@ define(['jquery', 'core/ajax', 'core/str', 'core/modal_factory', 'core/modal_eve */ var userConfirm = function(e, data) { data.originalEvent.preventDefault(); + data.originalEvent.stopPropagation(); e.preventDefault(); + e.stopPropagation(); triggerElement = $(e.currentTarget); var elemData = triggerElement.data('changecompl').split('-'); diff --git a/report/progress/index.php b/report/progress/index.php index af33b5118ed..7b3e9812da9 100644 --- a/report/progress/index.php +++ b/report/progress/index.php @@ -51,9 +51,6 @@ $sifirst = optional_param('sifirst', 'all', PARAM_NOTAGS); $silast = optional_param('silast', 'all', PARAM_NOTAGS); $start = optional_param('start', 0, PARAM_INT); -// Action. -$changecompl = optional_param('changecompl', '', PARAM_ALPHANUMEXT); - // Whether to show extra user identity information $extrafields = get_extra_user_fields($context); $leftcols = 1 + count($extrafields); @@ -103,20 +100,6 @@ $reportsurl = $CFG->wwwroot.'/course/report.php?id='.$course->id; $completion = new completion_info($course); $activities = $completion->get_activities(); -if ($changecompl) { - if ($changecompl) { - require_capability('moodle/course:overridecompletion', $context); - require_sesskey(); - list($userid, $cmid, $newstate) = preg_split('/-/', $changecompl, 3); - // Make sure the activity and user are tracked. - if (isset($activities[$cmid]) && - $completion->get_num_tracked_users('u.id = :userid', array('userid' => (int)$userid), $group)) { - $completion->update_state($activities[$cmid], $newstate, $userid, true); - } - redirect($PAGE->url); - } -} - if ($sifirst !== 'all') { set_user_preference('ifirst', $sifirst); } @@ -416,7 +399,7 @@ foreach($progress as $user) { '-'.$completiontype; if ($overrideby) { - $overridebyuser = $DB->get_record('user', array('id' => $overrideby), '*', MUST_EXIST); + $overridebyuser = \core_user::get_user($overrideby, '*', MUST_EXIST); $describe = get_string('completion-' . $completiontype, 'completion', fullname($overridebyuser)); } else { $describe = get_string('completion-' . $completiontype, 'completion'); @@ -436,8 +419,7 @@ foreach($progress as $user) { $state != COMPLETION_COMPLETE_PASS && $state != COMPLETION_COMPLETE_FAIL) { $newstate = ($state == COMPLETION_COMPLETE) ? COMPLETION_INCOMPLETE : COMPLETION_COMPLETE; $changecompl = $user->id . '-' . $activity->id . '-' . $newstate; - $url = new moodle_url($PAGE->url, array('sesskey' => sesskey(), - 'changecompl' => $changecompl)); + $url = new moodle_url($PAGE->url, ['sesskey' => sesskey()]); $celltext = html_writer::link($url, $celltext, array('class' => 'changecompl', 'data-changecompl' => $changecompl, 'aria-role' => 'button')); } diff --git a/report/progress/styles.css b/report/progress/styles.css index 425d0674ef8..ede47d068dc 100644 --- a/report/progress/styles.css +++ b/report/progress/styles.css @@ -59,9 +59,4 @@ #page-report-progress-index .modicon { padding-top: 5px; } - -#page-report-progress-index #completion-progress td a .ajaxworking { - height: 16px; - background: url([[pix:i/ajaxloader]]) no-repeat; -} /*rtl:end:ignore*/