From 44508bfe93c391fe8bd490ca8e848ab5516793b3 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 27 May 2022 13:51:38 +0800 Subject: [PATCH 1/3] MDL-68843 enrol_lti: add deleted activity warning to published tools Adds a warning to the 'name' column of the tables, letting teachers know that the activity exposed by the method has been deleted and what their options are. --- .../table/published_resources_table.php | 35 +++++++++++++++++- enrol/lti/classes/manage_table.php | 35 +++++++++++++++++- enrol/lti/lang/en/enrol_lti.php | 3 ++ enrol/lti/lib.php | 3 +- enrol/lti/pix/enrolinstancewarning.png | Bin 0 -> 339 bytes enrol/lti/pix/enrolinstancewarning.svg | 2 + 6 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 enrol/lti/pix/enrolinstancewarning.png create mode 100644 enrol/lti/pix/enrolinstancewarning.svg diff --git a/enrol/lti/classes/local/ltiadvantage/table/published_resources_table.php b/enrol/lti/classes/local/ltiadvantage/table/published_resources_table.php index d375d9163c5..8367b634da2 100644 --- a/enrol/lti/classes/local/ltiadvantage/table/published_resources_table.php +++ b/enrol/lti/classes/local/ltiadvantage/table/published_resources_table.php @@ -92,7 +92,8 @@ class published_resources_table extends \table_sql { * @return string */ public function col_name($tool) { - $name = helper::get_name($tool); + $toolcontext = \context::instance_by_id($tool->contextid, IGNORE_MISSING); + $name = $toolcontext ? helper::get_name($tool) : $this->get_deleted_activity_name_html($tool); return $this->get_display_text($tool, $name); } @@ -215,9 +216,39 @@ class published_resources_table extends \table_sql { */ protected function get_display_text($tool, $text) { if ($tool->status != ENROL_INSTANCE_ENABLED) { - return \html_writer::tag('span', $text, array('class' => 'dimmed_text')); + return \html_writer::tag('div', $text, array('class' => 'dimmed_text')); } return $text; } + + /** + * Get a warning icon, with tooltip, describing enrolment instances sharing activities which have been deleted. + * + * @param \stdClass $tool the tool instance record. + * @return string the HTML for the name column. + */ + protected function get_deleted_activity_name_html(\stdClass $tool): string { + global $OUTPUT; + $icon = \html_writer::tag( + 'a', + $OUTPUT->pix_icon('enrolinstancewarning', get_string('deletedactivityalt' , 'enrol_lti'), 'enrol_lti'), [ + "class" => "btn btn-link p-0", + "role" => "button", + "data-container" => "body", + "data-toggle" => "popover", + "data-placement" => right_to_left() ? "left" : "right", + "data-content" => get_string('deletedactivitydescription', 'enrol_lti'), + "data-html" => "true", + "tabindex" => "0", + "data-trigger" => "focus" + ] + ); + $name = \html_writer::span($icon . get_string('deletedactivity', 'enrol_lti')); + if ($tool->name) { + $name .= \html_writer::empty_tag('br') . \html_writer::empty_tag('br') . $tool->name; + } + + return $name; + } } diff --git a/enrol/lti/classes/manage_table.php b/enrol/lti/classes/manage_table.php index bd9ccc208da..262010103ba 100644 --- a/enrol/lti/classes/manage_table.php +++ b/enrol/lti/classes/manage_table.php @@ -101,7 +101,8 @@ class manage_table extends \table_sql { * @return string */ public function col_name($tool) { - $name = helper::get_name($tool); + $toolcontext = \context::instance_by_id($tool->contextid, IGNORE_MISSING); + $name = $toolcontext ? helper::get_name($tool) : $this->get_deleted_activity_name_html($tool); return $this->get_display_text($tool, $name); } @@ -247,9 +248,39 @@ class manage_table extends \table_sql { */ protected function get_display_text($tool, $text) { if ($tool->status != ENROL_INSTANCE_ENABLED) { - return \html_writer::tag('span', $text, array('class' => 'dimmed_text')); + return \html_writer::tag('div', $text, array('class' => 'dimmed_text')); } return $text; } + + /** + * Get a warning icon, with tooltip, describing enrolment instances sharing activities which have been deleted. + * + * @param \stdClass $tool the tool instance record. + * @return string the HTML for the name column. + */ + protected function get_deleted_activity_name_html(\stdClass $tool): string { + global $OUTPUT; + $icon = \html_writer::tag( + 'a', + $OUTPUT->pix_icon('enrolinstancewarning', get_string('deletedactivityalt' , 'enrol_lti'), 'enrol_lti'), [ + "class" => "btn btn-link p-0", + "role" => "button", + "data-container" => "body", + "data-toggle" => "popover", + "data-placement" => right_to_left() ? "left" : "right", + "data-content" => get_string('deletedactivitydescription', 'enrol_lti'), + "data-html" => "true", + "tabindex" => "0", + "data-trigger" => "focus" + ] + ); + $name = \html_writer::span($icon . get_string('deletedactivity', 'enrol_lti')); + if ($tool->name) { + $name .= \html_writer::empty_tag('br') . \html_writer::empty_tag('br') . $tool->name; + } + + return $name; + } } diff --git a/enrol/lti/lang/en/enrol_lti.php b/enrol/lti/lang/en/enrol_lti.php index 80f89612bcf..5808a0334d5 100644 --- a/enrol/lti/lang/en/enrol_lti.php +++ b/enrol/lti/lang/en/enrol_lti.php @@ -36,6 +36,9 @@ $string['copytoclipboard'] = 'Copy to clipboard'; $string['couldnotestablishproxy'] = 'Could not establish proxy with consumer.'; $string['customproperties'] = 'Custom properties'; $string['deeplinkingurl'] = 'Deep linking URL'; +$string['deletedactivity'] = 'Deleted activity'; +$string['deletedactivityalt'] = 'The activity shared by this instance has been deleted'; +$string['deletedactivitydescription'] = 'The activity shared by this instance has been deleted. You can select another activity to share by editing the instance or, if no longer required, you can simply delete the instance. Deleting the instance will remove any associated user enrolments.'; $string['deploymentadd'] = 'Add a deployment'; $string['deploymentaddnotice'] = 'Deployment added'; $string['deploymentdelete'] = 'Delete deployment'; diff --git a/enrol/lti/lib.php b/enrol/lti/lib.php index 7c8f78e3251..684993a20bf 100644 --- a/enrol/lti/lib.php +++ b/enrol/lti/lib.php @@ -475,6 +475,7 @@ function enrol_lti_extend_navigation_course($navigation, $course, $context) { function enrol_lti_get_fontawesome_icon_map() { return [ 'enrol_lti:managedeployments' => 'fa-sitemap', - 'enrol_lti:platformdetails' => 'fa-pencil-square-o' + 'enrol_lti:platformdetails' => 'fa-pencil-square-o', + 'enrol_lti:enrolinstancewarning' => 'fa-exclamation-circle text-danger', ]; } diff --git a/enrol/lti/pix/enrolinstancewarning.png b/enrol/lti/pix/enrolinstancewarning.png new file mode 100644 index 0000000000000000000000000000000000000000..edb89e473ea64b0ef85a779f2229c83e26607281 GIT binary patch literal 339 zcmV-Z0j&OsP)Au7Z)wjsz?+Giu;NZy?!0{0Gz0c)++T7Lof1aJTV5t#s7oO9>8u78sR zecwNM?*{-c3Bl2O|6Wzq&jN6$91Y)$F>H)s0Jj+{n0c_)>J34j=Vhe)Ktw`B0w4?U zLMe6H5QxY}WLGD05YgJp0e<+0AdhO#djxUl*%<*)siyUAA~F%tF{p9dw%=XX0o(7M zHUt1J0FF`P>wLNp6p`zyvLw#AGk`H=HHdNU#l~9GG>^ + \ No newline at end of file From 3387e637360568cedf4d21b175e2e2af42e0aa44 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 14 Jun 2022 10:55:52 +0800 Subject: [PATCH 2/3] MDL-68843 enrol_lti: auto disable orphaned enrolment instances - pre_course_module_deletion hook to disable on mod delete - upgrade step dealing with legacy orphaned records --- enrol/lti/db/upgrade.php | 12 +++++++ enrol/lti/lib.php | 15 +++++++++ enrol/lti/tests/lib_test.php | 32 +++++++++++++++++++ .../ltiadvantage/task/sync_grades_test.php | 7 ++-- enrol/lti/version.php | 2 +- 5 files changed, 65 insertions(+), 3 deletions(-) diff --git a/enrol/lti/db/upgrade.php b/enrol/lti/db/upgrade.php index 92da6cab9b9..5cf89474132 100644 --- a/enrol/lti/db/upgrade.php +++ b/enrol/lti/db/upgrade.php @@ -450,5 +450,17 @@ function xmldb_enrol_lti_upgrade($oldversion) { // Automatically generated Moodle v4.0.0 release upgrade line. // Put any upgrade step following this. + if ($oldversion < 2022061500) { + // Disable all orphaned enrolment method instances. + $sql = "id IN (SELECT t.enrolid + FROM {enrol_lti_tools} t + LEFT JOIN {context} c ON (t.contextid = c.id) + WHERE c.id IS NULL)"; + $DB->set_field_select('enrol', 'status', 1, $sql); + + // Lti savepoint reached. + upgrade_plugin_savepoint(true, 2022061500, 'enrol', 'lti'); + } + return true; } diff --git a/enrol/lti/lib.php b/enrol/lti/lib.php index 684993a20bf..f01522cb9e1 100644 --- a/enrol/lti/lib.php +++ b/enrol/lti/lib.php @@ -479,3 +479,18 @@ function enrol_lti_get_fontawesome_icon_map() { 'enrol_lti:enrolinstancewarning' => 'fa-exclamation-circle text-danger', ]; } + +/** + * Pre-delete course module hook which disables any methods referring to the deleted module, preventing launches and allowing remap. + * + * @param stdClass $cm The deleted course module record. + */ +function enrol_lti_pre_course_module_delete(stdClass $cm) { + global $DB; + $sql = "id IN (SELECT t.enrolid + FROM {enrol_lti_tools} t + JOIN {context} c ON (t.contextid = c.id) + WHERE c.contextlevel = :contextlevel + AND c.instanceid = :cmid)"; + $DB->set_field_select('enrol', 'status', ENROL_INSTANCE_DISABLED, $sql, ['contextlevel' => CONTEXT_MODULE, 'cmid' => $cm->id]); +} diff --git a/enrol/lti/tests/lib_test.php b/enrol/lti/tests/lib_test.php index d11e0f82699..9698d5e818b 100644 --- a/enrol/lti/tests/lib_test.php +++ b/enrol/lti/tests/lib_test.php @@ -216,4 +216,36 @@ class lib_test extends \lti_advantage_testcase { // LTI enrolment has 1 enrol actions for active users -- unenrol. $this->assertCount(1, $actions); } + + /** + * Test the behaviour of an enrolment method when the activity to which it provides access is deleted. + * + * @covers \enrol_lti_pre_course_module_delete + */ + public function test_course_module_deletion() { + // Create two modules and publish them. + $course = $this->getDataGenerator()->create_course(); + $mod = $this->getDataGenerator()->create_module('assign', ['course' => $course->id]); + $mod2 = $this->getDataGenerator()->create_module('assign', ['course' => $course->id]); + $tooldata = [ + 'cmid' => $mod->cmid, + 'courseid' => $course->id, + ]; + $tool = $this->getDataGenerator()->create_lti_tool((object)$tooldata); + $tooldata['cmid'] = $mod2->cmid; + $tool2 = $this->getDataGenerator()->create_lti_tool((object)$tooldata); + + // Verify the instances are both enabled. + $modinstance = helper::get_lti_tool($tool->id); + $mod2instance = helper::get_lti_tool($tool2->id); + $this->assertEquals(ENROL_INSTANCE_ENABLED, $modinstance->status); + $this->assertEquals(ENROL_INSTANCE_ENABLED, $mod2instance->status); + + // Delete a module and verify the associated instance is disabled. + course_delete_module($mod->cmid); + $modinstance = helper::get_lti_tool($tool->id); + $mod2instance = helper::get_lti_tool($tool2->id); + $this->assertEquals(ENROL_INSTANCE_DISABLED, $modinstance->status); + $this->assertEquals(ENROL_INSTANCE_ENABLED, $mod2instance->status); + } } diff --git a/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php b/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php index e8680a5f238..76cabc0184d 100644 --- a/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php +++ b/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php @@ -419,6 +419,8 @@ class sync_grades_test extends \lti_advantage_testcase { */ public function test_sync_grades_deleted_context() { $this->resetAfterTest(); + global $DB; + [$course, $resource] = $this->create_test_environment(); $launchservice = $this->get_tool_launch_service(); @@ -427,10 +429,11 @@ class sync_grades_test extends \lti_advantage_testcase { $instructoruser = $this->getDataGenerator()->create_user(); [$userid] = $launchservice->user_launches_tool($instructoruser, $teachermocklaunch); - global $CFG; - require_once($CFG->dirroot . '/course/lib.php'); + // Delete the activity, then enable the enrolment method (it is disabled during activity deletion). $modcontext = \context::instance_by_id($resource->contextid); course_delete_module($modcontext->instanceid); + $enrol = ['id' => $resource->enrolid, 'status' => ENROL_INSTANCE_ENABLED]; + $DB->update_record('enrol', $enrol); $task = $this->get_task_with_mocked_grade_service(); $this->expectOutputRegex( diff --git a/enrol/lti/version.php b/enrol/lti/version.php index 87b900db4a1..5aa032eedb6 100644 --- a/enrol/lti/version.php +++ b/enrol/lti/version.php @@ -24,7 +24,7 @@ defined('MOODLE_INTERNAL') || die(); -$plugin->version = 2022041900; // The current plugin version (Date: YYYYMMDDXX). +$plugin->version = 2022061500; // The current plugin version (Date: YYYYMMDDXX). $plugin->requires = 2022041200; // Requires this Moodle version. $plugin->component = 'enrol_lti'; // Full name of the plugin (used for diagnostics). $plugin->dependencies = [ From 273d9d561d7b957b0d8cd9b14767340dc2cd5231 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 14 Jun 2022 10:56:07 +0800 Subject: [PATCH 3/3] MDL-68843 enrol_lti: add tests covering disabled instances in tasks Verify the expected behaviour of the sync_members and sync_grades tasks when enrolment instances are disabled, whether as a byproduct of module deletion, or as status change made by the user. --- .../ltiadvantage/task/sync_grades_test.php | 31 +++++++++++++++++- .../ltiadvantage/task/sync_members_test.php | 32 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php b/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php index 76cabc0184d..88e01c9dfd3 100644 --- a/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php +++ b/enrol/lti/tests/local/ltiadvantage/task/sync_grades_test.php @@ -412,6 +412,35 @@ class sync_grades_test extends \lti_advantage_testcase { $task->execute(); } + /** + * Test syncing grades when the enrolment instance is disabled. + * + * @covers ::execute + */ + public function test_sync_grades_disabled_instance() { + $this->resetAfterTest(); + global $DB; + + [$course, $resource, $resource2, $resource3] = $this->create_test_environment(); + + // Disable resource 1. + $enrol = (object) ['id' => $resource->enrolid, 'status' => ENROL_INSTANCE_DISABLED]; + $DB->update_record('enrol', $enrol); + + // Delete the activity being shared by resource 2, leaving resource 2 disabled as a result. + $modcontext = \context::instance_by_id($resource2->contextid); + course_delete_module($modcontext->instanceid); + + // Only the enabled resource 3 should sync grades. + $task = $this->get_task_with_mocked_grade_service(); + $this->expectOutputRegex( + "/^Starting - LTI Advantage grade sync for shared resource '$resource3->id' in course '$course->id'.\n". + "Completed - Synced grades for tool '$resource3->id' in the course '$course->id'. Processed 0 users; ". + "sent 0 grades.\n$/" + ); + $task->execute(); + } + /** * Test the grade sync when the context has been deleted in between launch and when the grade sync task is run. * @@ -432,7 +461,7 @@ class sync_grades_test extends \lti_advantage_testcase { // Delete the activity, then enable the enrolment method (it is disabled during activity deletion). $modcontext = \context::instance_by_id($resource->contextid); course_delete_module($modcontext->instanceid); - $enrol = ['id' => $resource->enrolid, 'status' => ENROL_INSTANCE_ENABLED]; + $enrol = (object) ['id' => $resource->enrolid, 'status' => ENROL_INSTANCE_ENABLED]; $DB->update_record('enrol', $enrol); $task = $this->get_task_with_mocked_grade_service(); diff --git a/enrol/lti/tests/local/ltiadvantage/task/sync_members_test.php b/enrol/lti/tests/local/ltiadvantage/task/sync_members_test.php index 7a2c3d3044c..ec265de1b91 100644 --- a/enrol/lti/tests/local/ltiadvantage/task/sync_members_test.php +++ b/enrol/lti/tests/local/ltiadvantage/task/sync_members_test.php @@ -609,6 +609,38 @@ class sync_members_test extends \lti_advantage_testcase { $this->assertCount(1, $userrepo->find_by_resource($resource->id)); } + /** + * Test syncing members when the enrolment instance is disabled. + * + * @covers ::execute + */ + public function test_sync_members_disabled_instance() { + $this->resetAfterTest(); + global $DB; + + [$course, $resource, $resource2, $resource3] = $this->create_test_environment(); + $userrepo = new user_repository(); + + // Disable resource 1. + $enrol = (object) ['id' => $resource->enrolid, 'status' => ENROL_INSTANCE_DISABLED]; + $DB->update_record('enrol', $enrol); + + // Delete the activity being shared by resource2, leaving resource 2 disabled as a result. + $modcontext = \context::instance_by_id($resource2->contextid); + course_delete_module($modcontext->instanceid); + + // Only the enabled resource 3 should sync members. + $task = $this->get_mock_task_with_users($this->get_mock_members_with_ids(range(1, 1))); + $task->execute(); + + $this->expectOutputRegex( + "/^Starting - Member sync for published resource '$resource3->id' for course '$course->id'.\n". + "Completed - Synced members for tool '$resource3->id' in the course '$course->id'. Processed 0 users; ". + "enrolled 0 members; unenrolled 0 members.\n$/" + ); + $this->assertCount(0, $userrepo->find_by_resource($resource->id)); + } + /** * Test syncing members for a membersync-enabled resource when the launch omits the NRPS service endpoints. *