From 6f4555d31d3b5b03100e74c8296282e4a7745e70 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 25 Jul 2023 11:00:08 +0800 Subject: [PATCH 01/21] MDL-78597 mod_lti: improve DB reads on coursetooledit page This was hitting the lti_types table twice, via both lti_get_type and lti_get_type_type_config. This change adds course to the stdClass returned by the latter, so we can just make a single call to that. This also permits the use of that course property in other places where we'd like to have access to it, such as when editing a pre- configured tool at site level. --- mod/lti/coursetooledit.php | 5 +++-- mod/lti/locallib.php | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/mod/lti/coursetooledit.php b/mod/lti/coursetooledit.php index c99809baa8e..bc97a1e851d 100644 --- a/mod/lti/coursetooledit.php +++ b/mod/lti/coursetooledit.php @@ -35,15 +35,16 @@ $typeid = optional_param('typeid', null, PARAM_INT); require_login($courseid, false); require_capability('mod/lti:addcoursetool', context_course::instance($courseid)); if (!empty($typeid)) { - $type = lti_get_type($typeid); + $type = lti_get_type_type_config($typeid); if ($type->course != $courseid || $type->course == get_site()->id) { throw new moodle_exception('You do not have permissions to edit this tool type.'); } +} else { + $type = (object) ['lti_clientid' => null]; } // Page setup. $url = new moodle_url('/mod/lti/coursetooledit.php', ['courseid' => $courseid]); -$type = !empty($typeid) ? lti_get_type_type_config($typeid) : (object) ['lti_clientid' => null]; $pageheading = !empty($typeid) ? get_string('courseexternaltooledit', 'mod_lti', $type->lti_typename) : get_string('courseexternaltooladd', 'mod_lti'); diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php index 71d88d7f0bb..1f549e1d4ea 100644 --- a/mod/lti/locallib.php +++ b/mod/lti/locallib.php @@ -2643,6 +2643,8 @@ function lti_get_type_type_config($id) { $type->typeid = $basicltitype->id; + $type->course = $basicltitype->course; + $type->toolproxyid = $basicltitype->toolproxyid; $type->lti_toolurl = $basicltitype->baseurl; From 8ed8b6e9b92496e8af5e77a3344105d6890efa93 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 25 Jul 2023 18:12:00 +0800 Subject: [PATCH 02/21] MDL-78597 mod_lti: make new course tools show in activity chooser This will only impact newly created course tools, not existing tools. Existing tools will be listed in the LTI External tools page and teachers will be able to add them to the activity chooser from there. --- mod/lti/edit_form.php | 2 +- mod/lti/tests/behat/addtool.feature | 43 +++++++++++++++++++---------- mod/lti/tests/generator/lib.php | 3 +- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/mod/lti/edit_form.php b/mod/lti/edit_form.php index 1aab2c5c8ac..b47256445ef 100644 --- a/mod/lti/edit_form.php +++ b/mod/lti/edit_form.php @@ -196,7 +196,7 @@ class mod_lti_edit_types_form extends moodleform { $mform->addHelpButton('lti_coursevisible', $stringname, 'lti'); $mform->setDefault('lti_coursevisible', '1'); } else { - $mform->addElement('hidden', 'lti_coursevisible', LTI_COURSEVISIBLE_PRECONFIGURED); + $mform->addElement('hidden', 'lti_coursevisible', LTI_COURSEVISIBLE_ACTIVITYCHOOSER); } $mform->setType('lti_coursevisible', PARAM_INT); diff --git a/mod/lti/tests/behat/addtool.feature b/mod/lti/tests/behat/addtool.feature index 9833e5b6f82..ebfd359e2ee 100644 --- a/mod/lti/tests/behat/addtool.feature +++ b/mod/lti/tests/behat/addtool.feature @@ -2,7 +2,7 @@ Feature: Add tools In order to provide activities for learners As a teacher - I need to be able to add external tools to a course + I need to be able to add instances of external tools to a course Background: Given the following "users" exist: @@ -14,30 +14,45 @@ Feature: Add tools And the following "course enrolments" exist: | user | course | role | | teacher1 | C1 | editingteacher | - And I log in as "admin" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - And I follow "Manage preconfigured tools" - And I follow "Add preconfigured tool" - And I set the following fields to these values: - | Tool name | Teaching Tool 1 | - | Tool configuration usage | Show in activity chooser and as a preconfigured tool | - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/tool_provider.php" - And I press "Save changes" - And I log out + # A site tool configured to show as a preconfigured tool and in the activity chooser. + And the following "mod_lti > tool types" exist: + | name | baseurl | coursevisible | state | + | Teaching Tool 1 | /mod/lti/tests/fixtures/tool_provider.php | 2 | 1 | + # A course tool in course 1. + And the following "mod_lti > course tools" exist: + | name | baseurl | course | + | Course tool 1 | /mod/lti/tests/fixtures/tool_provider.php | C1 | @javascript - Scenario: Add a tool via the activity picker + Scenario: Add a site tool via the activity picker Given I log in as "teacher1" And I am on "Course 1" course homepage with editing mode on - And I add a "Teaching Tool 1" to section "1" + When I add a "Teaching Tool 1" to section "1" # For tool that does not support Content-Item message type, the Select content button must be disabled. And I set the field "Activity name" to "Test tool activity 1" And I expand all fieldsets And I set the field "Launch container" to "Embed" And the "Select content" "button" should be disabled And I press "Save and return to course" - When I open "Test tool activity 1" actions menu + And I open "Test tool activity 1" actions menu And I choose "Edit settings" in the open action menu Then the field "Preconfigured tool" matches value "Teaching Tool 1" And the "Select content" "button" should be disabled And the "Tool URL" "field" should be disabled + + @javascript + Scenario: Add a course tool via the activity picker + Given I log in as "teacher1" + And I am on "Course 1" course homepage with editing mode on + When I add a "Course tool 1" to section "1" + # For tool that does not support Content-Item message type, the Select content button must be disabled. + And I set the field "Activity name" to "Test tool activity 2" + And I expand all fieldsets + And I set the field "Launch container" to "Embed" + And the "Select content" "button" should be disabled + And I press "Save and return to course" + And I open "Test tool activity 2" actions menu + And I choose "Edit settings" in the open action menu + Then the field "Preconfigured tool" matches value "Course tool 1" + And the "Select content" "button" should be disabled + And the "Tool URL" "field" should be disabled diff --git a/mod/lti/tests/generator/lib.php b/mod/lti/tests/generator/lib.php index ac037ce6e77..bfded14fd74 100644 --- a/mod/lti/tests/generator/lib.php +++ b/mod/lti/tests/generator/lib.php @@ -118,7 +118,8 @@ class mod_lti_generator extends testing_module_generator { if (!isset($type['course']) || $type['course'] == $SITE->id) { throw new coding_exception('Must specify a non-site course when creating a course tool type.'); } - $type['coursevisible'] = LTI_COURSEVISIBLE_PRECONFIGURED; // The default for course tools. + $type['coursevisible'] = LTI_COURSEVISIBLE_ACTIVITYCHOOSER; // The default for course tools. + $type['state'] = LTI_TOOL_STATE_CONFIGURED; // The default for course tools. lti_add_type((object) $type, (object) $config); } } From 8825c0a0fe9f9308a25422c79064e1658f63643d Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 26 Jul 2023 12:13:57 +0800 Subject: [PATCH 03/21] MDL-78597 mod_lti: remove tool configuration usage for course tools This change removes the 'Tool configuration usage' control for course tools being edited via site admin. All course tools are, at a minimum, considered to be preconfigured tools and are visible in the course. The visibility of course tools in the activity chooser will be controlled via the LTI External tools course page in future. --- mod/lti/edit_form.php | 31 +++++++++++++++++-------------- mod/lti/typessettings.php | 14 +++++++++++--- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/mod/lti/edit_form.php b/mod/lti/edit_form.php index b47256445ef..5eb80648664 100644 --- a/mod/lti/edit_form.php +++ b/mod/lti/edit_form.php @@ -180,21 +180,24 @@ class mod_lti_edit_types_form extends moodleform { $mform->setForceLtr('lti_customparameters'); if (!empty($this->_customdata->isadmin)) { - $options = array( - LTI_COURSEVISIBLE_NO => get_string('show_in_course_no', 'lti'), - LTI_COURSEVISIBLE_PRECONFIGURED => get_string('show_in_course_preconfigured', 'lti'), - LTI_COURSEVISIBLE_ACTIVITYCHOOSER => get_string('show_in_course_activity_chooser', 'lti'), - ); - if ($istool) { - // LTI2 tools can not be matched by URL, they have to be either in preconfigured tools or in activity chooser. - unset($options[LTI_COURSEVISIBLE_NO]); - $stringname = 'show_in_course_lti2'; - } else { - $stringname = 'show_in_course_lti1'; + // Only site-level preconfigured tools allow the control of course visibility in the site admin tool type form. + if (!$this->_customdata->iscoursetool) { + $options = array( + LTI_COURSEVISIBLE_NO => get_string('show_in_course_no', 'lti'), + LTI_COURSEVISIBLE_PRECONFIGURED => get_string('show_in_course_preconfigured', 'lti'), + LTI_COURSEVISIBLE_ACTIVITYCHOOSER => get_string('show_in_course_activity_chooser', 'lti'), + ); + if ($istool) { + // LTI2 tools can not be matched by URL, they have to be either in preconfigured tools or in activity chooser. + unset($options[LTI_COURSEVISIBLE_NO]); + $stringname = 'show_in_course_lti2'; + } else { + $stringname = 'show_in_course_lti1'; + } + $mform->addElement('select', 'lti_coursevisible', get_string($stringname, 'lti'), $options); + $mform->addHelpButton('lti_coursevisible', $stringname, 'lti'); + $mform->setDefault('lti_coursevisible', '1'); } - $mform->addElement('select', 'lti_coursevisible', get_string($stringname, 'lti'), $options); - $mform->addHelpButton('lti_coursevisible', $stringname, 'lti'); - $mform->setDefault('lti_coursevisible', '1'); } else { $mform->addElement('hidden', 'lti_coursevisible', LTI_COURSEVISIBLE_ACTIVITYCHOOSER); } diff --git a/mod/lti/typessettings.php b/mod/lti/typessettings.php index 956bafdd340..91c2cd82011 100644 --- a/mod/lti/typessettings.php +++ b/mod/lti/typessettings.php @@ -127,9 +127,17 @@ if (lti_request_is_using_ssl() && !empty($type->lti_secureicon)) { $type->oldicon = $type->lti_icon; } -$form = new mod_lti_edit_types_form($pageurl, - (object)array('isadmin' => true, 'istool' => false, 'id' => $id, 'clientid' => $type->lti_clientid, - 'coursecategories' => $type->lti_coursecategories)); +$form = new mod_lti_edit_types_form( + $pageurl, + (object) [ + 'isadmin' => true, + 'istool' => false, + 'id' => $id, + 'clientid' => $type->lti_clientid, + 'coursecategories' => $type->lti_coursecategories, + 'iscoursetool' => !empty($id) && $type->course !== get_site()->id + ] +); if ($data = $form->get_data()) { $type = new stdClass(); From b3ef113509acfda0b9f0d2cae1d33d5dccb29f6d Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 26 Jul 2023 17:01:38 +0800 Subject: [PATCH 04/21] MDL-78597 mod_lti: set instance vars in mod_edit form This introduces a form constructor and sets up instance vars for the object there, allowing removal of the optional_param() calls in other methods. --- mod/lti/mod_form.php | 47 ++++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/mod/lti/mod_form.php b/mod/lti/mod_form.php index 3984a97bbee..da84293d44f 100644 --- a/mod/lti/mod_form.php +++ b/mod/lti/mod_form.php @@ -53,16 +53,37 @@ require_once($CFG->dirroot.'/mod/lti/locallib.php'); class mod_lti_mod_form extends moodleform_mod { + /** @var int|null the typeid or null if the instance form is being created for a manually configured tool instance.*/ + protected ?int $typeid; + + /** @var string|null type */ + protected ?string $type; + + /** + * Constructor. + * + * @param \stdClass $current the current form data. + * @param string $section the section number. + * @param \stdClass $cm the course module object. + * @param \stdClass $course the course object. + */ + public function __construct($current, $section, $cm, $course) { + + // Setup some of the pieces used to control display in the form definition() method. + // Type ID parameter being passed when adding an preconfigured tool from activity chooser. + $this->typeid = optional_param('typeid', null, PARAM_INT); + $this->type = optional_param('type', null, PARAM_ALPHA); + + parent::__construct($current, $section, $cm, $course); + } + public function definition() { global $PAGE, $OUTPUT, $COURSE; - if ($type = optional_param('type', false, PARAM_ALPHA)) { - component_callback("ltisource_$type", 'add_instance_hook'); + if ($this->type) { + component_callback("ltisource_$this->type", 'add_instance_hook'); } - // Type ID parameter being passed when adding an preconfigured tool from activity chooser. - $typeid = optional_param('typeid', false, PARAM_INT); - $showoptions = has_capability('mod/lti:addmanualinstance', $this->context); // Show configuration details only if not preset (when new) or user has the capabilities to do so (when editing). if ($this->_instance) { @@ -73,7 +94,7 @@ class mod_lti_mod_form extends moodleform_mod { $showtypes = false; } } else { - $showtypes = !$typeid; + $showtypes = !$this->typeid; } $mform =& $this->_form; @@ -118,8 +139,8 @@ class mod_lti_mod_form extends moodleform_mod { if ($showtypes) { $tooltypes = $mform->addElement('select', 'typeid', get_string('external_tool_type', 'lti')); - if ($typeid) { - $mform->getElement('typeid')->setValue($typeid); + if ($this->typeid) { + $mform->getElement('typeid')->setValue($this->typeid); } $mform->addHelpButton('typeid', 'external_tool_type', 'lti'); @@ -160,10 +181,10 @@ class mod_lti_mod_form extends moodleform_mod { $tooltypes->addOption($type->name, $id, $attributes); } } else { - $mform->addElement('hidden', 'typeid', $typeid); + $mform->addElement('hidden', 'typeid', $this->typeid); $mform->setType('typeid', PARAM_INT); - if ($typeid) { - $config = lti_get_type_config($typeid); + if ($this->typeid) { + $config = lti_get_type_config($this->typeid); if (!empty($config['contentitem'])) { $mform->addElement('hidden', 'contentitem', 1); $mform->setType('contentitem', PARAM_INT); @@ -178,7 +199,7 @@ class mod_lti_mod_form extends moodleform_mod { 'data-contentitemurl' => $contentitemurl->out(false) ]; if (!$showtypes) { - if (!$typeid || empty(lti_get_type_config($typeid)['contentitem'])) { + if (!$this->typeid || empty(lti_get_type_config($this->typeid)['contentitem'])) { $contentbuttonattributes['disabled'] = 'disabled'; } } @@ -353,7 +374,7 @@ class mod_lti_mod_form extends moodleform_mod { ), ); - if (!empty($typeid)) { + if (!empty($this->typeid)) { $mform->setAdvanced('typeid'); $mform->setAdvanced('toolurl'); } From 7d1cd36e8919d6825fddf252624a4150a8ffcdac Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Sat, 29 Jul 2023 15:16:27 +0800 Subject: [PATCH 05/21] MDL-78597 mod_lti: add full field support to tool_types generator This method is used by behat only, where only a single arg is received. The method lti_add_type() expects two params: type and config, but will only ever receive one. This means not all the fields can be set when creating an lti type. This change: - Removes the superfluous param which the method won't receive - Improves the logic for handling type and type config data, making it match what happens when creating types via mforms. - Adds relative URL support to the baseurl field, allowing behat features to create types using local tool fixtures. - Sets sensible default for missing config data, allowing the created tool type to be used in launches in places like behat. --- mod/lti/tests/generator/lib.php | 42 +++++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/mod/lti/tests/generator/lib.php b/mod/lti/tests/generator/lib.php index bfded14fd74..1a521ca7936 100644 --- a/mod/lti/tests/generator/lib.php +++ b/mod/lti/tests/generator/lib.php @@ -88,17 +88,49 @@ class mod_lti_generator extends testing_module_generator { lti_add_tool_proxy((object) $config); } + /** + * Split type creation data into 'type' and 'config' components, based on input array key prefixes. + * + * The $data array contains both the type data and config data that will be passed to lti_add_type(). This must be split into + * two params (type, config) based on the array key prefixes ({@see lti_add_type()} for how the two params are handled): + * - NO prefix: denotes 'type' data. + * - 'lti_' prefix: denotes 'config' data. + * - 'ltiservice_' prefix: denotes 'config' data, specifically config for service plugins. + * + * @param array $data array of type and config data containing prefixed keys. + * @return array containing separated type and config data. E.g. ['type' = [...], 'config' => [...]] + */ + protected function get_type_and_config_from_data(array $data): array { + // Grab any non-prefixed fields; these are the type fields. The rest is considered config. + $type = array_filter( + $data, + fn($val, $key) => !str_contains($key, 'lti_') && !str_contains($key, 'ltiservice_'), + ARRAY_FILTER_USE_BOTH + ); + $config = array_diff_key($data, $type); + + return ['type' => $type, 'config' => $config]; + } + /** * Create a tool type. * - * @param array $type - * @param array|null $config + * @param array $data */ - public function create_tool_types(array $type, ?array $config = null) { - if (!isset($type['baseurl'])) { + public function create_tool_types(array $data) { + if (!isset($data['baseurl'])) { throw new coding_exception('Must specify baseurl when creating a LTI tool type.'); } - lti_add_type((object) $type, (object) $config); + $data['baseurl'] = (new moodle_url($data['baseurl']))->out(false); // Permits relative URLs in behat features. + + // Sensible defaults permitting the tool type to be used in a launch. + $data['lti_acceptgrades'] = $data['lti_acceptgrades'] ?? LTI_SETTING_ALWAYS; + $data['lti_sendname'] = $data['lti_sendname'] ?? LTI_SETTING_ALWAYS; + $data['lti_sendemailaddr'] = $data['lti_sendname'] ?? LTI_SETTING_ALWAYS; + + ['type' => $type, 'config' => $config] = $this->get_type_and_config_from_data($data); + + lti_add_type(type: (object) $type, config: (object) $config); } /** From 5862fe2607eaf0287ac4ab3201ac991663cfc6f5 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Sat, 29 Jul 2023 16:29:09 +0800 Subject: [PATCH 06/21] MDL-78597 mod_lti: add full field support to course_tool_types generator This method is used by behat only, where only a single arg is received. The method lti_add_type() expects two params: type and config, but will only ever receive one. This means not all the fields can be set when creating an lti type. This change: - Removes the superfluous param which the method won't receive - Improves the logic for handling type and type config data, making it match what happens when creating types via mforms. - Adds relative URL support to the baseurl field, allowing behat features to create types using local tool fixtures. - Sets sensible default for missing config data, allowing the created tool type to be used in launches in places like behat. --- mod/lti/tests/generator/lib.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/mod/lti/tests/generator/lib.php b/mod/lti/tests/generator/lib.php index 1a521ca7936..5525de65e32 100644 --- a/mod/lti/tests/generator/lib.php +++ b/mod/lti/tests/generator/lib.php @@ -137,11 +137,10 @@ class mod_lti_generator extends testing_module_generator { * Create a course tool type. * * @param array $type the type info. - * @param array|null $config the type configuration. * @return void * @throws coding_exception if any required fields are missing. */ - public function create_course_tool_types(array $type, ?array $config = null): void { + public function create_course_tool_types(array $type): void { global $SITE; if (!isset($type['baseurl'])) { @@ -150,8 +149,18 @@ class mod_lti_generator extends testing_module_generator { if (!isset($type['course']) || $type['course'] == $SITE->id) { throw new coding_exception('Must specify a non-site course when creating a course tool type.'); } + + $type['baseurl'] = (new moodle_url($type['baseurl']))->out(false); // Permits relative URLs in behat features. $type['coursevisible'] = LTI_COURSEVISIBLE_ACTIVITYCHOOSER; // The default for course tools. $type['state'] = LTI_TOOL_STATE_CONFIGURED; // The default for course tools. - lti_add_type((object) $type, (object) $config); + + // Sensible defaults permitting the tool type to be used in a launch. + $type['lti_acceptgrades'] = $type['lti_acceptgrades'] ?? LTI_SETTING_ALWAYS; + $type['lti_sendname'] = $type['lti_sendname'] ?? LTI_SETTING_ALWAYS; + $type['lti_sendemailaddr'] = $type['lti_sendemailaddr'] ?? LTI_SETTING_ALWAYS; + + ['type' => $type, 'config' => $config] = $this->get_type_and_config_from_data($type); + + lti_add_type(type: (object) $type, config: (object) $config); } } From 36900136a81f7e41fc7fec31bb05cb56abbcd941 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Sun, 30 Jul 2023 13:50:47 +0800 Subject: [PATCH 07/21] MDL-78597 mod_lti: add instance creation to behat creatable entities This allows behat tests which have already created either a site or course tool type, to then link it to an activity instance using a generator. This depends on the ability to name match a type. --- .../generator/behat_mod_lti_generator.php | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/mod/lti/tests/generator/behat_mod_lti_generator.php b/mod/lti/tests/generator/behat_mod_lti_generator.php index b635e45aad4..b887525b954 100644 --- a/mod/lti/tests/generator/behat_mod_lti_generator.php +++ b/mod/lti/tests/generator/behat_mod_lti_generator.php @@ -50,7 +50,28 @@ class behat_mod_lti_generator extends behat_generator_base { 'datagenerator' => 'course_tool_types', 'required' => ['baseurl', 'course'], 'switchids' => ['course' => 'course'] - ] + ], + 'tool instances' => [ + 'singular' => 'instance', + 'datagenerator' => 'instance', + 'required' => ['course', 'tool'], + 'switchids' => ['course' => 'course', 'tool' => 'typeid'] + ], ]; } + + /** + * Handles the switchid ['tool' => 'typeid'] for finding a tool by name. + * + * @param string $name the name of the tool. + * @return int the id of the tool type identified by the name $name. + */ + protected function get_tool_id(string $name): int { + global $DB; + + if (!$id = $DB->get_field('lti_types', 'id', ['name' => $name])) { + throw new coding_exception('The specified tool with name "' . $name . '" does not exist'); + } + return (int) $id; + } } From ca10084639d6a4f4a35f4f015bdb736078818d01 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 26 Jul 2023 14:14:12 +0800 Subject: [PATCH 08/21] MDL-78597 mod_lti: remove the ability to add manual instances of tools In summary: - Remove the 'External tool' item from get_course_content_items() hook, which results in its removal from the activity chooser. - Remove the 'External tool' item from get_all_content_items() hook, which results in its removal from the admin activity chooser recommendations page. - Prevent use of the edit_form for creation of new manual instances. - Retain the ability to edit existing manual instances. - Fix tests expecting external tool. --- mod/lti/lang/en/lti.php | 1 + mod/lti/lib.php | 35 +++---------- mod/lti/mod_form.php | 12 +++++ mod/lti/tests/behat/backup_restore.feature | 57 ++++++++-------------- mod/lti/tests/behat/contentitem.feature | 36 +++++--------- mod/lti/tests/lib_test.php | 18 +++---- mod/lti/upgrade.txt | 2 + 7 files changed, 58 insertions(+), 103 deletions(-) diff --git a/mod/lti/lang/en/lti.php b/mod/lti/lang/en/lti.php index 81b583fa22f..f2350b98ce8 100644 --- a/mod/lti/lang/en/lti.php +++ b/mod/lti/lang/en/lti.php @@ -289,6 +289,7 @@ $string['launchoptions'] = 'Launch options'; $string['lti'] = 'LTI'; $string['lti:addcoursetool'] = 'Add course-specific tool configurations'; $string['lti:addmanualinstance'] = 'Add a manually-configured tool'; +$string['lti:addmanualinstanceprohibitederror'] = 'The manual creation of tools without a course tool definition is no longer supported. Please create a course tool first and then use that to create activity instances.'; $string['lti:addinstance'] = 'Add a new external tool'; $string['lti:addpreconfiguredinstance'] = 'Add a preconfigured tool'; $string['lti:grade'] = 'View grades returned by the external tool'; diff --git a/mod/lti/lib.php b/mod/lti/lib.php index bf7b379eb22..b75f0aa4a1b 100644 --- a/mod/lti/lib.php +++ b/mod/lti/lib.php @@ -240,22 +240,6 @@ function lti_get_course_content_items(\core_course\local\entity\content_item $de $types = []; - // The 'External tool' entry (the main module content item), should always take the id of 1. - if (has_capability('mod/lti:addmanualinstance', context_course::instance($course->id), $user)) { - $types = [new \core_course\local\entity\content_item( - 1, - $defaultmodulecontentitem->get_name(), - $defaultmodulecontentitem->get_title(), - $defaultmodulecontentitem->get_link(), - $defaultmodulecontentitem->get_icon(), - $defaultmodulecontentitem->get_help(), - $defaultmodulecontentitem->get_archetype(), - $defaultmodulecontentitem->get_component_name(), - $defaultmodulecontentitem->get_purpose() - )]; - } - - // Other, preconfigured tools take their own id + 1, so we'll never clash with the module's entry. $preconfiguredtools = lti_get_configured_types($course->id, $defaultmodulecontentitem->get_link()->param('sr')); foreach ($preconfiguredtools as $preconfiguredtool) { @@ -270,6 +254,9 @@ function lti_get_course_content_items(\core_course\local\entity\content_item $de $preconfiguredtool->help = ''; } + // Preconfigured tools take their own id + 1. This logic exists because, previously, the entry permitting manual instance + // creation (the $defaultmodulecontentitem, or 'External tool' item) was included and had the id 1. This logic prevented id + // collisions. $types[] = new \core_course\local\entity\content_item( $preconfiguredtool->id + 1, $preconfiguredtool->name, @@ -295,18 +282,7 @@ function mod_lti_get_all_content_items(\core_course\local\entity\content_item $d global $OUTPUT, $CFG; require_once($CFG->dirroot . '/mod/lti/locallib.php'); // For access to constants. - // The 'External tool' entry (the main module content item), should always take the id of 1. - $types = [new \core_course\local\entity\content_item( - 1, - $defaultmodulecontentitem->get_name(), - $defaultmodulecontentitem->get_title(), - $defaultmodulecontentitem->get_link(), - $defaultmodulecontentitem->get_icon(), - $defaultmodulecontentitem->get_help(), - $defaultmodulecontentitem->get_archetype(), - $defaultmodulecontentitem->get_component_name(), - $defaultmodulecontentitem->get_purpose() - )]; + $types = []; foreach (lti_get_lti_types() as $ltitype) { if ($ltitype->coursevisible != LTI_COURSEVISIBLE_ACTIVITYCHOOSER) { @@ -332,6 +308,9 @@ function mod_lti_get_all_content_items(\core_course\local\entity\content_item $d } $type->link = new moodle_url('/course/modedit.php', array('add' => 'lti', 'return' => 0, 'typeid' => $ltitype->id)); + // Preconfigured tools take their own id + 1. This logic exists because, previously, the entry permitting manual instance + // creation (the $defaultmodulecontentitem, or 'External tool' item) was included and had the id 1. This logic prevented id + // collisions. $types[] = new \core_course\local\entity\content_item( $type->id + 1, $type->name, diff --git a/mod/lti/mod_form.php b/mod/lti/mod_form.php index da84293d44f..39d4f157e9c 100644 --- a/mod/lti/mod_form.php +++ b/mod/lti/mod_form.php @@ -62,10 +62,13 @@ class mod_lti_mod_form extends moodleform_mod { /** * Constructor. * + * Throws an exception if trying to init the form for a new manual instance of a tool, which is not supported in 4.3 onward. + * * @param \stdClass $current the current form data. * @param string $section the section number. * @param \stdClass $cm the course module object. * @param \stdClass $course the course object. + * @throws moodle_exception if trying to init the form for the creation of a manual instance, which is no longer supported. */ public function __construct($current, $section, $cm, $course) { @@ -74,6 +77,15 @@ class mod_lti_mod_form extends moodleform_mod { $this->typeid = optional_param('typeid', null, PARAM_INT); $this->type = optional_param('type', null, PARAM_ALPHA); + // Only permit construction if the form deals with editing an existing instance (current->id not empty), or creating an + // instance from a preconfigured tool type ($this->typeid not empty). + global $PAGE; + if ($PAGE->has_set_url() && str_contains($PAGE->url, '/course/modedit.php')) { + if (empty($this->typeid) && empty($current->id)) { + throw new moodle_exception('lti:addmanualinstanceprohibitederror', 'mod_lti'); + } + } + parent::__construct($current, $section, $cm, $course); } diff --git a/mod/lti/tests/behat/backup_restore.feature b/mod/lti/tests/behat/backup_restore.feature index 9da97b66c9d..d956ce0d4d7 100644 --- a/mod/lti/tests/behat/backup_restore.feature +++ b/mod/lti/tests/behat/backup_restore.feature @@ -15,30 +15,20 @@ Feature: Restoring Moodle 2 backup restores LTI configuration | teacher1 | C2 | editingteacher | Scenario: Backup and restore course with preconfigured site LTI tool on the same site - When I log in as "admin" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - And I follow "Manage preconfigured tools" - And I follow "Add preconfigured tool" - And I set the following fields to these values: - | Tool name | My site tool | - | Tool URL | https://www.moodle.org | - | lti_coursevisible | 1 | - And I press "Save changes" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - And "This tool has not yet been used" "text" should exist in the "//div[contains(@id,'tool-card-container') and contains(., 'My site tool')]" "xpath_element" - And I am on site homepage - And I am on "Course 1" course homepage - And I turn editing mode on - And I add a "External tool" to section "1" and I fill the form with: - | Activity name | My LTI module | - | Preconfigured tool | My site tool | - | Launch container | Embed | + Given the following "mod_lti > tool types" exist: + | name | description | baseurl | coursevisible | state | + | My site tool | Site tool description | https://www.moodle.org | 2 | 1 | + And the following "mod_lti > tool instances" exist: + | name | tool | course | + | My LTI module | My site tool | C1 | + And I am on the "Course 1" course page logged in as admin And I should see "My LTI module" - And I backup "Course 1" course using this options: + When I backup "Course 1" course using this options: | Confirmation | Filename | test_backup.mbz | And I restore "test_backup.mbz" backup into a new course using this options: And I am on site homepage And I follow "Course 1 copy 1" + And I turn editing mode on And I open "My LTI module" actions menu And I choose "Edit settings" in the open action menu Then the field "Preconfigured tool" matches value "My site tool" @@ -47,29 +37,20 @@ Feature: Restoring Moodle 2 backup restores LTI configuration @javascript @_switch_window Scenario: Backup and restore course with preconfigured course LTI tool on the same site - When I log in as "teacher1" - And I am on "Course 1" course homepage with editing mode on - # In the first course create an LTI module that uses a course preconfigured toolю - And I add a "External tool" to section "1" - And I set the following fields to these values: - | Activity name | Test tool activity 2 | - And I follow "Add preconfigured tool" - And I switch to "add_tool" window - And I set the field "Tool name" to "My course tool" - And I set the field "Tool URL" to "http://www.example.com/lti/provider.php" - And I set the field "Consumer key" to "my key" - And I set the field "Shared secret" to "my secret" - And I set the field "Default launch container" to "Existing window" - And I press "Save changes" - And I switch to the main window - And I press "Save and return to course" + Given the following "mod_lti > course tools" exist: + | name | description | baseurl | course | lti_resourcekey | lti_password | lti_launchcontainer | + | My course tool | Example description | http://www.example.com/lti/provider.php | C1 | my key | my secret | 5 | + # In the first course create an LTI module that uses a course preconfigured tool + And the following "mod_lti > tool instances" exist: + | name | tool | course | + | Test tool activity 2 | My course tool | C1 | + And I am on the "Course 1" course page logged in as admin # Backup course and restore into another course And I backup "Course 1" course using this options: | Confirmation | Filename | test_backup.mbz | And I restore "test_backup.mbz" backup into "Course 2" course using this options: - And I am on site homepage - And I follow "Course 2" - # Make sure the copy of the preconfigured tool was created in the second course with both encrtypted and non-encrypted properties. + And I am on "Course 2" course homepage with editing mode on + # Make sure the copy of the preconfigured tool was created in the second course with both encrypted and non-encrypted properties. And I open "Test tool activity 2" actions menu And I choose "Edit settings" in the open action menu Then the field "Preconfigured tool" matches value "My course tool" diff --git a/mod/lti/tests/behat/contentitem.feature b/mod/lti/tests/behat/contentitem.feature index fe56ef5e081..51efb901b77 100644 --- a/mod/lti/tests/behat/contentitem.feature +++ b/mod/lti/tests/behat/contentitem.feature @@ -14,18 +14,9 @@ Feature: Content-Item support And the following "course enrolments" exist: | user | course | role | | teacher1 | C1 | editingteacher | - And I log in as "admin" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - # Create tool type that supports deep linking. - And I follow "configure a tool manually" - And I set the field "Tool name" to "Teaching Tool 1" - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/tool_provider.php" - And I set the field "Tool configuration usage" to "Show in activity chooser and as a preconfigured tool" - And I expand all fieldsets - And I set the field "Supports Deep Linking (Content-Item Message)" to "1" - And I press "Save changes" - And I should see "Teaching Tool 1" - And I log out + And the following "mod_lti > tool types" exist: + | name | description | baseurl | coursevisible | state | lti_contentitem | + | Teaching Tool 1 | Tool 1 description | /mod/lti/tests/fixtures/tool_provider.php | 2 | 1 | 1 | @javascript Scenario: Tool that supports Deep Linking should be able to configure a tool via the Select content button @@ -51,19 +42,14 @@ Feature: Content-Item support @javascript Scenario: Changing preconfigured tool selection - Given I log in as "admin" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - And I follow "configure a tool manually" - And I set the field "Tool name" to "Teaching Tool 2" - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/tool_provider.php" - And I set the field "Tool configuration usage" to "Show in activity chooser and as a preconfigured tool" - And I expand all fieldsets - And I press "Save changes" - And I should see "Teaching Tool 2" - And I log out - When I log in as "teacher1" - And I am on "Course 1" course homepage with editing mode on - And I add a "External tool" to section "1" + Given the following "mod_lti > tool types" exist: + | name | description | baseurl | coursevisible | state | + | Teaching Tool 2 | Another description | /mod/lti/tests/fixtures/tool_provider.php | 2 | 1 | + # Create a manually configured instance using the generator (this isn't possible via the UI any more). + And the following "activities" exist: + | activity | course | name | typeid | toolurl | + | lti | C1 | Test tool activity 1 | 0 | /mod/lti/tests/fixtures/tool_provider.php | + When I am on the "Test tool activity 1" "lti activity editing" page logged in as teacher1 # On load with no preconfigured tool selected: Select content button - disabled, Tool URL - enabled. And the field "Preconfigured tool" matches value "Automatic, based on tool URL" And I set the field "Activity name" to "Test tool activity 1" diff --git a/mod/lti/tests/lib_test.php b/mod/lti/tests/lib_test.php index 4b84a153b64..2cd1acb6e8c 100644 --- a/mod/lti/tests/lib_test.php +++ b/mod/lti/tests/lib_test.php @@ -412,43 +412,37 @@ class lib_test extends \advanced_testcase { // The lti_get_lti_types_by_course method (used by the callbacks) assumes the global user. $this->setUser($teacher); - // Teacher in course1 should be able to see the default module item ('external tool'), - // the site preconfigured tool and the tool created in course1. + // Teacher in course1 should be able to see the site preconfigured tool and the tool created in course1. $courseitems = lti_get_course_content_items($defaultmodulecontentitem, $teacher, $course); - $this->assertCount(3, $courseitems); + $this->assertCount(2, $courseitems); $ids = []; foreach ($courseitems as $item) { $ids[] = $item->get_id(); } - $this->assertContains(1, $ids); $this->assertContains($sitetoolrecord->id + 1, $ids); $this->assertContains($course1toolrecord->id + 1, $ids); $this->assertNotContains($sitetoolrecordnonchooser->id + 1, $ids); - // The content items for teacher2 in course2 include the default module content item ('external tool'), - // the site preconfigured tool and the tool created in course2. + // The content items for teacher2 in course2 include the site preconfigured tool and the tool created in course2. $this->setUser($teacher2); $course2items = lti_get_course_content_items($defaultmodulecontentitem, $teacher2, $course2); - $this->assertCount(3, $course2items); + $this->assertCount(2, $course2items); $ids = []; foreach ($course2items as $item) { $ids[] = $item->get_id(); } - $this->assertContains(1, $ids); $this->assertContains($sitetoolrecord->id + 1, $ids); $this->assertContains($course2toolrecord->id + 1, $ids); $this->assertNotContains($sitetoolrecordnonchooser->id + 1, $ids); - // When fetching all content items, we expect to see all items available in activity choosers (in any course), - // plus the default module content item ('external tool'). + // When fetching all content items, we expect to see all items available in activity choosers (in any course). $this->setAdminUser(); $allitems = mod_lti_get_all_content_items($defaultmodulecontentitem); - $this->assertCount(4, $allitems); + $this->assertCount(3, $allitems); $ids = []; foreach ($allitems as $item) { $ids[] = $item->get_id(); } - $this->assertContains(1, $ids); $this->assertContains($sitetoolrecord->id + 1, $ids); $this->assertContains($course1toolrecord->id + 1, $ids); $this->assertContains($course2toolrecord->id + 1, $ids); diff --git a/mod/lti/upgrade.txt b/mod/lti/upgrade.txt index 2b84c3f2887..94b2b4562cd 100644 --- a/mod/lti/upgrade.txt +++ b/mod/lti/upgrade.txt @@ -3,6 +3,8 @@ This files describes API changes in the lti code. === 4.3 === * The `lti_libxml_disable_entity_loader` method is deprecated, as it is no longer required from PHP 8.0 +* The `mod_lti_mod_form` constructor will now throw an exception if called without passing a typeid as manual configuration of + instances is now unsupported. === 4.2 === From 6ed29fc35b84091dbb67009a46e17eb928c79c17 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 26 Jul 2023 17:19:28 +0800 Subject: [PATCH 09/21] MDL-78597 mod_lti: prevent manual instance use in edit form This change ensures tool instances which are currently using preconfigured tools (site or course level) are not able to be switched back to use the manual 'Automatic, based on tool URL' option. That option is reserved for legacy tool support of manual instances and domain-matched site tools, but must not be selectable otherwise. --- mod/lti/mod_form.php | 14 +++++++++++++- mod/lti/tests/behat/addtool.feature | 2 ++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/mod/lti/mod_form.php b/mod/lti/mod_form.php index 39d4f157e9c..95a5ea1c1d5 100644 --- a/mod/lti/mod_form.php +++ b/mod/lti/mod_form.php @@ -156,7 +156,19 @@ class mod_lti_mod_form extends moodleform_mod { } $mform->addHelpButton('typeid', 'external_tool_type', 'lti'); - foreach (lti_get_types_for_add_instance() as $id => $type) { + // To prevent the use of manually configured instances, existing instances which are using a preconfigured tool will not + // display the option "Automatic, based on tool URL" in the preconfigured tools select. This prevents switching from an + // instance configured using a preconfigured tool to an instance that is manually configured. + // Exceptions are made for: + // - Existing manually configured instances (i.e. no type set). + // - Instances using a tool type which isn't visible in the course preconfigured tools selector, such as when a + // site-level tool type is domain-matched. + $instancetypes = lti_get_types_for_add_instance(); + if (!empty($this->current->typeid) && array_key_exists($this->current->typeid, $instancetypes)) { + unset($instancetypes[0]); + } + + foreach ($instancetypes as $id => $type) { if (!empty($type->toolproxyid)) { $toolproxy[] = $type->id; $attributes = array('globalTool' => 1, 'toolproxy' => 1); diff --git a/mod/lti/tests/behat/addtool.feature b/mod/lti/tests/behat/addtool.feature index ebfd359e2ee..32233c6a800 100644 --- a/mod/lti/tests/behat/addtool.feature +++ b/mod/lti/tests/behat/addtool.feature @@ -56,3 +56,5 @@ Feature: Add tools Then the field "Preconfigured tool" matches value "Course tool 1" And the "Select content" "button" should be disabled And the "Tool URL" "field" should be disabled + And I click on "Preconfigured tool" "select" + And I should not see "Automatic, based on tool URL" From 5b01727107a0202ec702c678b9ced5aaf0de0191 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Thu, 27 Jul 2023 15:29:19 +0800 Subject: [PATCH 10/21] MDL-78597 mod_lti: retain selected option when arranging into optgroups Fixes a bug with YUI, resulting in the selected option not being set properly again after the options have been reorganised into optgroups. --- mod/lti/mod_form.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/mod/lti/mod_form.js b/mod/lti/mod_form.js index 639823688b9..507bd6e1b94 100644 --- a/mod/lti/mod_form.js +++ b/mod/lti/mod_form.js @@ -354,6 +354,13 @@ if(courseOptions.size() > 0){ typeSelector.append(courseGroup); } + + // Fixes what is presumably a bug in YUI, in which the selected option is not properly set after reorganising the + // options into optgroups. + var selectedOption = typeSelector.one('[selected]'); + if (selectedOption) { + selectedOption.set('selected', true); + } } }, From 85679c47da7dc608001e1c44536efae0c749326c Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 1 Aug 2023 12:10:26 +0800 Subject: [PATCH 11/21] MDL-78597 mod_lti: let content item callbacks obey preconfigured cap Users without the 'mod/lti:addpreconfiguredinstance' capability won't be able to see any mod_lti content items in the activity chooser. Note: this patch doesn't address the underlying locallib method, lti_get_lti_types_by_course(), which will be addressed elsewhere. --- mod/lti/lib.php | 5 +++++ mod/lti/tests/lib_test.php | 7 +++++++ 2 files changed, 12 insertions(+) diff --git a/mod/lti/lib.php b/mod/lti/lib.php index b75f0aa4a1b..4a8d1edc360 100644 --- a/mod/lti/lib.php +++ b/mod/lti/lib.php @@ -240,7 +240,12 @@ function lti_get_course_content_items(\core_course\local\entity\content_item $de $types = []; + // Use of a tool type, whether site or course level, is controlled by the following cap. + if (!has_capability('mod/lti:addpreconfiguredinstance', \core\context\course::instance($course->id), $user)) { + return $types; + } $preconfiguredtools = lti_get_configured_types($course->id, $defaultmodulecontentitem->get_link()->param('sr')); + foreach ($preconfiguredtools as $preconfiguredtool) { // Append the help link to the help text. diff --git a/mod/lti/tests/lib_test.php b/mod/lti/tests/lib_test.php index 2cd1acb6e8c..d8173017769 100644 --- a/mod/lti/tests/lib_test.php +++ b/mod/lti/tests/lib_test.php @@ -435,6 +435,13 @@ class lib_test extends \advanced_testcase { $this->assertContains($course2toolrecord->id + 1, $ids); $this->assertNotContains($sitetoolrecordnonchooser->id + 1, $ids); + // Removing the capability to use preconfigured (site or course level) tools, should result in no content items. + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + assign_capability('mod/lti:addpreconfiguredinstance', CAP_PROHIBIT, $teacherrole->id, + \core\context\course::instance($course2->id)); + $course2items = lti_get_course_content_items($defaultmodulecontentitem, $teacher2, $course2); + $this->assertCount(0, $course2items); + // When fetching all content items, we expect to see all items available in activity choosers (in any course). $this->setAdminUser(); $allitems = mod_lti_get_all_content_items($defaultmodulecontentitem); From afa44dd2885348188983c90a4c77e72908735377 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 1 Aug 2023 12:15:41 +0800 Subject: [PATCH 12/21] MDL-78597 core_course: fix content item tests covering get_all cases Fixes two tests which use the lti module as an example of a core mod implementing the 'get_all_content_items' callback. The setup of the mod in these tests, including course and site tools, needed to be updated in light of the removal of the 'external tool' content item. --- .../content_item_readonly_repository_test.php | 27 +++++++++---- .../services_content_item_service_test.php | 39 ++++++++++++------- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/course/tests/content_item_readonly_repository_test.php b/course/tests/content_item_readonly_repository_test.php index e055209f85e..681851bd5ce 100644 --- a/course/tests/content_item_readonly_repository_test.php +++ b/course/tests/content_item_readonly_repository_test.php @@ -78,26 +78,39 @@ class content_item_readonly_repository_test extends \advanced_testcase { */ public function test_find_all() { $this->resetAfterTest(); - global $DB; - // We'll compare our results to those which are course-specific. + global $DB, $CFG; + require_once($CFG->dirroot . '/mod/lti/tests/generator/lib.php'); + require_once($CFG->dirroot . '/mod/lti/locallib.php'); + + // We'll compare our results to those which are course-specific, using mod_lti as an example. $course = $this->getDataGenerator()->create_course(); $user = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + /** @var \mod_lti_generator $ltigenerator */ + $ltigenerator = $this->getDataGenerator()->get_plugin_generator('mod_lti'); + $ltigenerator->create_tool_types([ + 'name' => 'site tool', + 'baseurl' => 'http://example.com', + 'coursevisible' => LTI_COURSEVISIBLE_ACTIVITYCHOOSER, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); - assign_capability('mod/lti:addmanualinstance', CAP_PROHIBIT, $teacherrole->id, \context_course::instance($course->id)); + assign_capability('mod/lti:addpreconfiguredinstance', CAP_PROHIBIT, $teacherrole->id, + \core\context\course::instance($course->id)); $cir = new content_item_readonly_repository(); + $this->setUser($user); // This is needed since the underlying lti code needs the global user despite the api accepting user. - // Course specific - lti won't be returned as the user doesn't have the required cap. + // Course specific - the tool won't be returned as the user doesn't have the capability required to use preconfigured tools. $forcourse = $cir->find_all_for_course($course, $user); $forcourse = array_filter($forcourse, function($contentitem) { - return $contentitem->get_name() === 'lti'; + return str_contains($contentitem->get_name(), 'lti_type'); }); $this->assertEmpty($forcourse); - // All - all items are returned, including lti. + // All - all items are returned, including the lti site tool. $all = $cir->find_all(); $all = array_filter($all, function($contentitem) { - return $contentitem->get_name() === 'lti'; + return str_contains($contentitem->get_name(), 'lti_type'); }); $this->assertCount(1, $all); } diff --git a/course/tests/services_content_item_service_test.php b/course/tests/services_content_item_service_test.php index edccbd4d66b..e438a8dcaa9 100644 --- a/course/tests/services_content_item_service_test.php +++ b/course/tests/services_content_item_service_test.php @@ -111,28 +111,37 @@ class services_content_item_service_test extends \advanced_testcase { $this->resetAfterTest(); global $DB; - // Create a user in a course. + // Create a user in a course and set up a site-level LTI tool, configured to display in the activity chooser. $course = $this->getDataGenerator()->create_course(); $user = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); - + /** @var \mod_lti_generator $ltigenerator */ + $ltigenerator = $this->getDataGenerator()->get_plugin_generator('mod_lti'); + $ltigenerator->create_tool_types([ + 'name' => 'site tool', + 'baseurl' => 'http://example.com', + 'coursevisible' => LTI_COURSEVISIBLE_ACTIVITYCHOOSER, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); $cis = new content_item_service(new content_item_readonly_repository()); - $allcontentitems = $cis->get_all_content_items($user); - $coursecontentitems = $cis->get_content_items_for_user_in_course($user, $course); + $this->setUser($user); // This is needed since the underlying lti code needs the global user despite the api accepting user. // The call to get_all_content_items() should return the same items as for the course, - // given the user in an editing teacher and can add manual lti instances. - $this->assertContains('lti', array_column($coursecontentitems, 'name')); - $this->assertContains('lti', array_column($allcontentitems, 'name')); - - // Now removing the cap 'mod/lti:addinstance'. This will restrict those items returned by the course-specific method. - $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); - assign_capability('mod/lti:addinstance', CAP_PROHIBIT, $teacherrole->id, \context_course::instance($course->id)); - - // Verify that all items, including lti, are still returned by the get_all_content_items() call. + // given the user is an editing teacher and can add preconfigured lti instances. $allcontentitems = $cis->get_all_content_items($user); $coursecontentitems = $cis->get_content_items_for_user_in_course($user, $course); - $this->assertNotContains('lti', array_column($coursecontentitems, 'name')); - $this->assertContains('lti', array_column($allcontentitems, 'name')); + $this->assertContains('site tool', array_column($coursecontentitems, 'title')); + $this->assertContains('site tool', array_column($allcontentitems, 'title')); + + // Now removing the cap 'mod/lti:addpreconfiguredinstance', restricting those items returned by the course-specific method. + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + assign_capability('mod/lti:addpreconfiguredinstance', CAP_PROHIBIT, $teacherrole->id, + \core\context\course::instance($course->id)); + + // Verify that all items, including the tool, are still returned by the get_all_content_items() call. + $allcontentitems = $cis->get_all_content_items($user); + $coursecontentitems = $cis->get_content_items_for_user_in_course($user, $course); + $this->assertNotContains('site tool', array_column($coursecontentitems, 'title')); + $this->assertContains('site tool', array_column($allcontentitems, 'title')); } /** From 7ca26f1460d73623628f897fad4d3c30cf664b51 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 1 Aug 2023 13:48:23 +0800 Subject: [PATCH 13/21] MDL-78597 mod_lti: add \mod_lti\local\types_helper class This will serve as a location to put new tool-type-centric methods. --- mod/lti/classes/local/types_helper.php | 72 ++++++++++ mod/lti/tests/local/types_helper_test.php | 154 ++++++++++++++++++++++ 2 files changed, 226 insertions(+) create mode 100644 mod/lti/classes/local/types_helper.php create mode 100644 mod/lti/tests/local/types_helper_test.php diff --git a/mod/lti/classes/local/types_helper.php b/mod/lti/classes/local/types_helper.php new file mode 100644 index 00000000000..45dd6bd3e3b --- /dev/null +++ b/mod/lti/classes/local/types_helper.php @@ -0,0 +1,72 @@ +. + +namespace mod_lti\local; + +use core\context\course; + +/** + * Helper class specifically dealing with LTI types (preconfigured tools). + * + * @package mod_lti + * @copyright 2023 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class types_helper { + + /** + * Returns all LTI tool types (preconfigured tools) visible in the given course and for the given user. + * + * This list will contain both site level tools and course-level tools. + * + * @param int $courseid the id of the course. + * @param int $userid the id of the user. + * @param array $coursevisible options for 'coursevisible' field, which will default to + * [LTI_COURSEVISIBLE_PRECONFIGURED, LTI_COURSEVISIBLE_ACTIVITYCHOOSER] if omitted. + * @return \stdClass[] the array of tool type objects. + */ + public static function get_lti_types_by_course(int $courseid, int $userid, array $coursevisible = []): array { + global $DB, $SITE; + + if (!has_capability('mod/lti:addpreconfiguredinstance', course::instance($courseid), $userid)) { + return []; + } + + if (empty($coursevisible)) { + $coursevisible = [LTI_COURSEVISIBLE_PRECONFIGURED, LTI_COURSEVISIBLE_ACTIVITYCHOOSER]; + } + list($coursevisiblesql, $coursevisparams) = $DB->get_in_or_equal($coursevisible, SQL_PARAMS_NAMED, 'coursevisible'); + + $coursecond = implode(" OR ", ["t.course = :courseid", "t.course = :siteid"]); + $coursecategory = $DB->get_field('course', 'category', ['id' => $courseid]); + $query = "SELECT t.* + FROM {lti_types} t + LEFT JOIN {lti_types_categories} tc ON t.id = tc.typeid + WHERE t.coursevisible $coursevisiblesql + AND ($coursecond) + AND t.state = :active + AND (tc.id IS NULL OR tc.categoryid = :categoryid) + ORDER BY t.name ASC"; + + return $DB->get_records_sql($query, + [ + 'siteid' => $SITE->id, + 'courseid' => $courseid, + 'active' => LTI_TOOL_STATE_CONFIGURED, + 'categoryid' => $coursecategory + ] + $coursevisparams); + } +} diff --git a/mod/lti/tests/local/types_helper_test.php b/mod/lti/tests/local/types_helper_test.php new file mode 100644 index 00000000000..30283f38299 --- /dev/null +++ b/mod/lti/tests/local/types_helper_test.php @@ -0,0 +1,154 @@ +. +// +// This file is part of BasicLTI4Moodle +// +// BasicLTI4Moodle is an IMS BasicLTI (Basic Learning Tools for Interoperability) +// consumer for Moodle 1.9 and Moodle 2.0. BasicLTI is a IMS Standard that allows web +// based learning tools to be easily integrated in LMS as native ones. The IMS BasicLTI +// specification is part of the IMS standard Common Cartridge 1.1 Sakai and other main LMS +// are already supporting or going to support BasicLTI. This project Implements the consumer +// for Moodle. Moodle is a Free Open source Learning Management System by Martin Dougiamas. +// BasicLTI4Moodle is a project iniciated and leaded by Ludo(Marc Alier) and Jordi Piguillem +// at the GESSI research group at UPC. +// SimpleLTI consumer for Moodle is an implementation of the early specification of LTI +// by Charles Severance (Dr Chuck) htp://dr-chuck.com , developed by Jordi Piguillem in a +// Google Summer of Code 2008 project co-mentored by Charles Severance and Marc Alier. +// +// BasicLTI4Moodle is copyright 2009 by Marc Alier Forment, Jordi Piguillem and Nikolas Galanis +// of the Universitat Politecnica de Catalunya http://www.upc.edu +// Contact info: Marc Alier Forment granludo @ gmail.com or marc.alier @ upc.edu. + +namespace mod_lti\local; + +use mod_lti_testcase; + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once($CFG->dirroot . '/mod/lti/locallib.php'); +require_once($CFG->dirroot . '/mod/lti/tests/mod_lti_testcase.php'); + +/** + * Types helper tests. + * + * @package mod_lti + * @copyright 2023 Jake Dallimore + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @coversDefaultClass \mod_lti\local\types_helper + */ +class types_helper_test extends mod_lti_testcase { + + /** + * Test fetching tool types for a given course and user. + * + * @covers ::get_lti_types_by_course + * @return void. + */ + public function test_get_lti_types_by_course(): void { + $this->resetAfterTest(); + + global $DB; + $coursecat1 = $this->getDataGenerator()->create_category(); + $coursecat2 = $this->getDataGenerator()->create_category(); + $course = $this->getDataGenerator()->create_course(['category' => $coursecat1->id]); + $course2 = $this->getDataGenerator()->create_course(['category' => $coursecat2->id]); + $teacher = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + $teacher2 = $this->getDataGenerator()->create_and_enrol($course2, 'editingteacher'); + + // Create the following tool types for testing: + // - Site tool configured as "Do not show" (LTI_COURSEVISIBLE_NO). + // - Site tool configured as "Show as a preconfigured tool only" (LTI_COURSEVISIBLE_PRECONFIGURED). + // - Site tool configured as "Show as a preconfigured tool and in the activity chooser" (LTI_COURSEVISIBLE_ACTIVITYCHOOSER). + // - Course tool which, by default, is configured as LTI_COURSEVISIBLE_ACTIVITYCHOOSER). + // - Site tool configured to "Show as a preconfigured tool and in the activity chooser" but restricted to a category. + + /** @var \mod_lti_generator $ltigenerator */ + $ltigenerator = $this->getDataGenerator()->get_plugin_generator('mod_lti'); + $ltigenerator->create_tool_types([ + 'name' => 'site tool do not show', + 'baseurl' => 'http://example.com/tool/1', + 'coursevisible' => LTI_COURSEVISIBLE_NO, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); + $ltigenerator->create_tool_types([ + 'name' => 'site tool preconfigured only', + 'baseurl' => 'http://example.com/tool/2', + 'coursevisible' => LTI_COURSEVISIBLE_PRECONFIGURED, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); + $ltigenerator->create_tool_types([ + 'name' => 'site tool preconfigured and activity chooser', + 'baseurl' => 'http://example.com/tool/3', + 'coursevisible' => LTI_COURSEVISIBLE_ACTIVITYCHOOSER, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); + $ltigenerator->create_course_tool_types([ + 'name' => 'course tool preconfigured and activity chooser', + 'baseurl' => 'http://example.com/tool/4', + 'course' => $course->id + ]); + $ltigenerator->create_tool_types([ + 'name' => 'site tool preconfigured and activity chooser, restricted to category 2', + 'baseurl' => 'http://example.com/tool/5', + 'coursevisible' => LTI_COURSEVISIBLE_ACTIVITYCHOOSER, + 'state' => LTI_TOOL_STATE_CONFIGURED, + 'lti_coursecategories' => $coursecat2->id + ]); + + // Request using the default 'coursevisible' param will include all tools except the one configured as "Do not show" and + // the tool restricted to category 2. + $coursetooltypes = types_helper::get_lti_types_by_course($course->id, $teacher->id); + $this->assertCount(3, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/2', 'http://example.com/tool/3', 'http://example.com/tool/4'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for only those tools configured to show in the activity chooser for the teacher. + $coursetooltypes = types_helper::get_lti_types_by_course($course->id, $teacher->id, + [LTI_COURSEVISIBLE_ACTIVITYCHOOSER]); + $this->assertCount(2, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/3', 'http://example.com/tool/4'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for only those tools configured to show as a preconfigured tool for the teacher. + $coursetooltypes = types_helper::get_lti_types_by_course($course->id, $teacher->id, + [LTI_COURSEVISIBLE_PRECONFIGURED]); + $this->assertCount(1, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/2'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for teacher2 in course2 (course category 2). + $coursetooltypes = types_helper::get_lti_types_by_course($course2->id, $teacher2->id); + $this->assertCount(3, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/2', 'http://example.com/tool/3', 'http://example.com/tool/5'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for a teacher who cannot use preconfigured tools in the course. + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + assign_capability('mod/lti:addpreconfiguredinstance', CAP_PROHIBIT, $teacherrole->id, + \core\context\course::instance($course->id)); + $coursetooltypes = types_helper::get_lti_types_by_course($course->id, $teacher->id); + $this->assertCount(0, $coursetooltypes); + } +} From d8e2067b6686e6294a96bdd88cd120ae121e2d20 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 1 Aug 2023 13:51:09 +0800 Subject: [PATCH 14/21] MDL-78597 mod_lti: deprecate lti_get_lti_types_by_course() This method includes broken legacy behaviour in which the inclusion of course and site tools could be controlled independently, based on the capabilities 'mod/lti:addmanualinstance' (to include course tools) and 'mod/lti:addpreconfiguredinstance' (to include site tools). This behaviour is deprecated in 4.3, so this method is also deprecated. See the replacement method types_helper::get_lti_types_by_course. --- mod/lti/locallib.php | 21 +++--- mod/lti/tests/locallib_test.php | 122 ++++++++++++++++++++++++++++++++ mod/lti/upgrade.txt | 1 + 3 files changed, 136 insertions(+), 8 deletions(-) diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php index 1f549e1d4ea..a443133e2da 100644 --- a/mod/lti/locallib.php +++ b/mod/lti/locallib.php @@ -2310,12 +2310,16 @@ function lti_filter_tool_types(array $tools, $state) { /** * Returns all lti types visible in this course * + * @deprecated since Moodle 4.3 * @param int $courseid The id of the course to retieve types for * @param array $coursevisible options for 'coursevisible' field, * default [LTI_COURSEVISIBLE_PRECONFIGURED, LTI_COURSEVISIBLE_ACTIVITYCHOOSER] * @return stdClass[] All the lti types visible in the given course */ function lti_get_lti_types_by_course($courseid, $coursevisible = null) { + debugging(__FUNCTION__ . '() is deprecated. Please use \mod_lti\local\types_helper::get_lti_types_by_course() instead.', + DEBUG_DEVELOPER); + global $DB, $SITE; if ($coursevisible === null) { @@ -2359,15 +2363,15 @@ function lti_get_lti_types_by_course($courseid, $coursevisible = null) { * @return array Array of lti types */ function lti_get_types_for_add_instance() { - global $COURSE; - $admintypes = lti_get_lti_types_by_course($COURSE->id); + global $COURSE, $USER; + $preconfiguredtypes = \mod_lti\local\types_helper::get_lti_types_by_course($COURSE->id, $USER->id); - $types = array(); + $types = []; if (has_capability('mod/lti:addmanualinstance', context_course::instance($COURSE->id))) { $types[0] = (object)array('name' => get_string('automatic', 'lti'), 'course' => 0, 'toolproxyid' => null); } - foreach ($admintypes as $type) { + foreach ($preconfiguredtypes as $type) { $types[$type->id] = $type; } @@ -2382,11 +2386,12 @@ function lti_get_types_for_add_instance() { * @return array Array of lti types. Each element is object with properties: name, title, icon, help, helplink, link */ function lti_get_configured_types($courseid, $sectionreturn = 0) { - global $OUTPUT; - $types = array(); - $admintypes = lti_get_lti_types_by_course($courseid, [LTI_COURSEVISIBLE_ACTIVITYCHOOSER]); + global $OUTPUT, $USER; + $types = []; + $preconfiguredtypes = \mod_lti\local\types_helper::get_lti_types_by_course($courseid, $USER->id, + [LTI_COURSEVISIBLE_ACTIVITYCHOOSER]); - foreach ($admintypes as $ltitype) { + foreach ($preconfiguredtypes as $ltitype) { $type = new stdClass(); $type->id = $ltitype->id; $type->modclass = MOD_CLASS_ACTIVITY; diff --git a/mod/lti/tests/locallib_test.php b/mod/lti/tests/locallib_test.php index a704ad8f87c..e78e2b2496f 100644 --- a/mod/lti/tests/locallib_test.php +++ b/mod/lti/tests/locallib_test.php @@ -2248,4 +2248,126 @@ MwIDAQAB return ['proxies' => $proxies, 'types' => $types]; } + + /** + * Test for lti_get_lti_types_by_course. + * + * Note: This includes verification of the broken legacy behaviour in which the inclusion of course and site tools could be + * controlled independently, based on the capabilities 'mod/lti:addmanualinstance' (to include course tools) and + * 'mod/lti:addpreconfiguredinstance' (to include site tools). This behaviour is deprecated in 4.3 and all preconfigured tools + * are controlled by the single capability 'mod/lti:addpreconfiguredinstance'. + * + * @covers ::lti_get_lti_types_by_course() + * @return void + */ + public function test_lti_get_lti_types_by_course(): void { + $this->resetAfterTest(); + + global $DB; + $coursecat1 = $this->getDataGenerator()->create_category(); + $coursecat2 = $this->getDataGenerator()->create_category(); + $course = $this->getDataGenerator()->create_course(['category' => $coursecat1->id]); + $course2 = $this->getDataGenerator()->create_course(['category' => $coursecat2->id]); + $teacher = $this->getDataGenerator()->create_and_enrol($course, 'editingteacher'); + $teacher2 = $this->getDataGenerator()->create_and_enrol($course2, 'editingteacher'); + + // Create the following tool types for testing: + // - Site tool configured as "Do not show" (LTI_COURSEVISIBLE_NO). + // - Site tool configured as "Show as a preconfigured tool only" (LTI_COURSEVISIBLE_PRECONFIGURED). + // - Site tool configured as "Show as a preconfigured tool and in the activity chooser" (LTI_COURSEVISIBLE_ACTIVITYCHOOSER). + // - Course tool which, by default, is configured as LTI_COURSEVISIBLE_ACTIVITYCHOOSER). + // - Site tool configured to "Show as a preconfigured tool and in the activity chooser" but restricted to a category. + + /** @var \mod_lti_generator $ltigenerator */ + $ltigenerator = $this->getDataGenerator()->get_plugin_generator('mod_lti'); + $ltigenerator->create_tool_types([ + 'name' => 'site tool do not show', + 'baseurl' => 'http://example.com/tool/1', + 'coursevisible' => LTI_COURSEVISIBLE_NO, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); + $ltigenerator->create_tool_types([ + 'name' => 'site tool preconfigured only', + 'baseurl' => 'http://example.com/tool/2', + 'coursevisible' => LTI_COURSEVISIBLE_PRECONFIGURED, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); + $ltigenerator->create_tool_types([ + 'name' => 'site tool preconfigured and activity chooser', + 'baseurl' => 'http://example.com/tool/3', + 'coursevisible' => LTI_COURSEVISIBLE_ACTIVITYCHOOSER, + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); + $ltigenerator->create_course_tool_types([ + 'name' => 'course tool preconfigured and activity chooser', + 'baseurl' => 'http://example.com/tool/4', + 'course' => $course->id + ]); + $ltigenerator->create_tool_types([ + 'name' => 'site tool preconfigured and activity chooser, restricted to category 2', + 'baseurl' => 'http://example.com/tool/5', + 'coursevisible' => LTI_COURSEVISIBLE_ACTIVITYCHOOSER, + 'state' => LTI_TOOL_STATE_CONFIGURED, + 'lti_coursecategories' => $coursecat2->id + ]); + + $this->setUser($teacher); // Important: this deprecated method depends on the global user for cap checks. + + // Request using the default 'coursevisible' param will include all tools except the one configured as "Do not show". + $coursetooltypes = lti_get_lti_types_by_course($course->id); + $this->assertDebuggingCalled(); + $this->assertCount(3, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/2', 'http://example.com/tool/3', 'http://example.com/tool/4'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for only those tools configured to show in the activity chooser for the teacher. + $coursetooltypes = lti_get_lti_types_by_course($course->id, [LTI_COURSEVISIBLE_ACTIVITYCHOOSER]); + $this->assertDebuggingCalled(); + $this->assertCount(2, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/3', 'http://example.com/tool/4'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for only those tools configured to show as a preconfigured tool for the teacher. + $coursetooltypes = lti_get_lti_types_by_course($course->id, [LTI_COURSEVISIBLE_PRECONFIGURED]); + $this->assertDebuggingCalled(); + $this->assertCount(1, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/2'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for teacher2 in course2 (course category 2). + $this->setUser($teacher2); + $coursetooltypes = lti_get_lti_types_by_course($course2->id); + $this->assertDebuggingCalled(); + $this->assertCount(3, $coursetooltypes); + $this->assertEmpty(array_diff( + ['http://example.com/tool/2', 'http://example.com/tool/3', 'http://example.com/tool/5'], + array_column($coursetooltypes, 'baseurl') + )); + + // Request for a teacher who cannot use preconfigured tools in the course. + // No tools are available. + $this->setUser($teacher); + $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher')); + assign_capability('mod/lti:addpreconfiguredinstance', CAP_PROHIBIT, $teacherrole->id, + \core\context\course::instance($course->id)); + $coursetooltypes = lti_get_lti_types_by_course($course->id); + $this->assertDebuggingCalled(); + $this->assertCount(1, $coursetooltypes); + $this->unassignUserCapability('mod/lti:addpreconfiguredinstance', (\core\context\course::instance($course->id))->id, + $teacherrole->id); + + // Request for a teacher who cannot use manually configured tools in the course. + // Only return site tools, which is broken legacy behaviour. + assign_capability('mod/lti:addmanualinstance', CAP_PROHIBIT, $teacherrole->id, + \context_course::instance($course->id)); + $coursetooltypes = lti_get_lti_types_by_course($course->id); + $this->assertDebuggingCalled(); + $this->assertCount(2, $coursetooltypes); + } } diff --git a/mod/lti/upgrade.txt b/mod/lti/upgrade.txt index 94b2b4562cd..a45dcac1ab6 100644 --- a/mod/lti/upgrade.txt +++ b/mod/lti/upgrade.txt @@ -5,6 +5,7 @@ This files describes API changes in the lti code. * The `lti_libxml_disable_entity_loader` method is deprecated, as it is no longer required from PHP 8.0 * The `mod_lti_mod_form` constructor will now throw an exception if called without passing a typeid as manual configuration of instances is now unsupported. +* The `lti_get_lti_types_by_course` method is deprecated. Please use mod_lti\local\types_helper::get_lti_types_by_course instead. === 4.2 === From 93707d8cf5ca7846f91636f72e422ec6697a1292 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 4 Aug 2023 12:20:58 +0800 Subject: [PATCH 15/21] MDL-78597 mod_lti: deprecate mod/lti:addmanualinstance and lock config This change: - removes all uses of mod/lti:addmanualinstance and deprecates it. - updates the activity instance edit form, specifically the way it deals with existing manually-configured instances, removing the ability to edit tool configuration and adding a notice to the user. --- mod/lti/db/access.php | 18 +- mod/lti/lang/en/deprecated.txt | 1 + mod/lti/lang/en/lti.php | 9 +- mod/lti/locallib.php | 47 +--- mod/lti/mod_form.php | 239 +++++++++++------- mod/lti/tests/behat/managecoursetools.feature | 1 - mod/lti/tests/locallib_test.php | 10 +- mod/lti/upgrade.txt | 2 + mod/lti/version.php | 2 +- 9 files changed, 176 insertions(+), 153 deletions(-) create mode 100644 mod/lti/lang/en/deprecated.txt diff --git a/mod/lti/db/access.php b/mod/lti/db/access.php index 6722495b971..244f812e86f 100644 --- a/mod/lti/db/access.php +++ b/mod/lti/db/access.php @@ -102,17 +102,6 @@ $capabilities = array( 'clonepermissionsfrom' => 'mod/lti:addinstance', ), - // The ability to add a manual instance (i.e. not from a preconfigured tool) to the course. - 'mod/lti:addmanualinstance' => array( - 'captype' => 'write', - 'contextlevel' => CONTEXT_COURSE, - 'archetypes' => array( - 'editingteacher' => CAP_ALLOW, - 'manager' => CAP_ALLOW - ), - 'clonepermissionsfrom' => 'mod/lti:addinstance', - ), - // The ability to request the administrator to configure a particular // External tool globally. 'mod/lti:requesttooladd' => array( @@ -124,3 +113,10 @@ $capabilities = array( ) ) ); +$deprecatedcapabilities = [ + // The ability to add a manual instance (i.e. not from a preconfigured tool) to the course. + 'mod/lti:addmanualinstance' => [ + 'message' => 'Manual instance configuration is deprecated. Please create a course tool (mod/lti:addcoursetool) and ensure '. + 'users are able to add an instance of the course tool via the activity chooser (mod/lti:addpreconfiguredinstance).' + ], +]; diff --git a/mod/lti/lang/en/deprecated.txt b/mod/lti/lang/en/deprecated.txt new file mode 100644 index 00000000000..1175f1a017c --- /dev/null +++ b/mod/lti/lang/en/deprecated.txt @@ -0,0 +1 @@ +lti:addmanualinstance,mod_lti diff --git a/mod/lti/lang/en/lti.php b/mod/lti/lang/en/lti.php index f2350b98ce8..d74124d0d3b 100644 --- a/mod/lti/lang/en/lti.php +++ b/mod/lti/lang/en/lti.php @@ -191,6 +191,11 @@ $string['dynreg_update_btn_update'] = 'Update'; $string['dynreg_update_btn_new'] = 'Register as a new external tool'; $string['duplicateregurl'] = 'This registration URL is already in use'; $string['editdescription'] = 'Click here to give this tool a description'; +$string['editmanualinstancedeprecationwarning'] = 'Manually configured External tool activities are no longer supported. Don\'t worry, this activity will still work as it is, but you can\'t make changes to the tool configuration here anymore. +

+To make any changes to the tool, or to create new activities with it, the tool needs to be added to your course in Course > More > LTI External tools. Then, you will be able to create new activities, selecting the tool directly in the Activity chooser. +

+You can read more about adding LTI External tools in the documentation External tool.'; $string['edittype'] = 'Edit preconfigured tool'; $string['embed'] = 'Embed'; $string['embed_no_blocks'] = 'Embed, without blocks'; @@ -288,7 +293,6 @@ real estate to the tool, and others provide a more integrated feel with the Mood $string['launchoptions'] = 'Launch options'; $string['lti'] = 'LTI'; $string['lti:addcoursetool'] = 'Add course-specific tool configurations'; -$string['lti:addmanualinstance'] = 'Add a manually-configured tool'; $string['lti:addmanualinstanceprohibitederror'] = 'The manual creation of tools without a course tool definition is no longer supported. Please create a course tool first and then use that to create activity instances.'; $string['lti:addinstance'] = 'Add a new external tool'; $string['lti:addpreconfiguredinstance'] = 'Add a preconfigured tool'; @@ -621,3 +625,6 @@ $string['using_tool_cartridge'] = 'Using tool cartridge'; $string['using_tool_configuration'] = 'Using tool configuration: '; $string['validurl'] = 'A valid URL must start with http(s)://'; $string['viewsubmissions'] = 'View submissions and grading screen'; + +// Deprecated since Moodle 4.3. +$string['lti:addmanualinstance'] = 'Add a manually-configured tool'; diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php index a443133e2da..d8f1ec033fe 100644 --- a/mod/lti/locallib.php +++ b/mod/lti/locallib.php @@ -2320,41 +2320,8 @@ function lti_get_lti_types_by_course($courseid, $coursevisible = null) { debugging(__FUNCTION__ . '() is deprecated. Please use \mod_lti\local\types_helper::get_lti_types_by_course() instead.', DEBUG_DEVELOPER); - global $DB, $SITE; - - if ($coursevisible === null) { - $coursevisible = [LTI_COURSEVISIBLE_PRECONFIGURED, LTI_COURSEVISIBLE_ACTIVITYCHOOSER]; - } - - list($coursevisiblesql, $coursevisparams) = $DB->get_in_or_equal($coursevisible, SQL_PARAMS_NAMED, 'coursevisible'); - $courseconds = []; - if (has_capability('mod/lti:addmanualinstance', context_course::instance($courseid))) { - $courseconds[] = "t.course = :courseid"; - } - if (has_capability('mod/lti:addpreconfiguredinstance', context_course::instance($courseid))) { - $courseconds[] = "t.course = :siteid"; - } - if (!$courseconds) { - return []; - } - $coursecond = implode(" OR ", $courseconds); - $coursecategory = $DB->get_field('course', 'category', ['id' => $courseid]); - $query = "SELECT t.* - FROM {lti_types} t - LEFT JOIN {lti_types_categories} tc on t.id = tc.typeid - WHERE t.coursevisible $coursevisiblesql - AND ($coursecond) - AND t.state = :active - AND (tc.id IS NULL OR tc.categoryid = :categoryid) - ORDER BY t.name ASC"; - - return $DB->get_records_sql($query, - [ - 'siteid' => $SITE->id, - 'courseid' => $courseid, - 'active' => LTI_TOOL_STATE_CONFIGURED, - 'categoryid' => $coursecategory - ] + $coursevisparams); + global $USER; + return \mod_lti\local\types_helper::get_lti_types_by_course($courseid, $USER->id, $coursevisible ?? []); } /** @@ -2364,13 +2331,11 @@ function lti_get_lti_types_by_course($courseid, $coursevisible = null) { */ function lti_get_types_for_add_instance() { global $COURSE, $USER; + + // Always return the 'manual' type option, despite manual config being deprecated, so that we have it for legacy instances. + $types = [(object) ['name' => get_string('automatic', 'lti'), 'course' => 0, 'toolproxyid' => null]]; + $preconfiguredtypes = \mod_lti\local\types_helper::get_lti_types_by_course($COURSE->id, $USER->id); - - $types = []; - if (has_capability('mod/lti:addmanualinstance', context_course::instance($COURSE->id))) { - $types[0] = (object)array('name' => get_string('automatic', 'lti'), 'course' => 0, 'toolproxyid' => null); - } - foreach ($preconfiguredtypes as $type) { $types[$type->id] = $type; } diff --git a/mod/lti/mod_form.php b/mod/lti/mod_form.php index 95a5ea1c1d5..b1a88b15558 100644 --- a/mod/lti/mod_form.php +++ b/mod/lti/mod_form.php @@ -96,11 +96,18 @@ class mod_lti_mod_form extends moodleform_mod { component_callback("ltisource_$this->type", 'add_instance_hook'); } - $showoptions = has_capability('mod/lti:addmanualinstance', $this->context); + // Since 'mod/lti:addmanualinstance' capability is deprecated, determining which users may have had access to the certain + // form fields (the manual config fields) isn't straightforward. Users without 'mod/lti:addmanualinstance' would have only + // been permitted to edit the basic instance fields (name, etc.), so care must be taken not to display the config fields to + // these users. Users who can add/edit course tools (mod/lti:addcoursetool) are able to view tool information anyway, via + // the tool definitions, so this capability is used as a replacement, to control access to these tool config fields. + $canviewmanualconfig = has_capability('mod/lti:addcoursetool', $this->context); + $manualinstance = empty($this->current->typeid) && empty($this->typeid); + // Show configuration details only if not preset (when new) or user has the capabilities to do so (when editing). if ($this->_instance) { $showtypes = has_capability('mod/lti:addpreconfiguredinstance', $this->context); - if (!$showoptions && $this->current->typeid == 0) { + if ($manualinstance && !$canviewmanualconfig) { // If you cannot add a manual instance and this is already a manual instance, then // remove the 'types' selector. $showtypes = false; @@ -109,8 +116,28 @@ class mod_lti_mod_form extends moodleform_mod { $showtypes = !$this->typeid; } + // Determine whether this tool instance is using a tool which is not visible at the course level, but which does exist. + // This indicates that the instance has either: + // - Been configured manually, and was domain matched to a site tool in the past. + // - Been configured using a preconfigured tool that is now no longer visible in the course. + // In the case of the domain matched tool, tool URL will be set. + $instancetypes = lti_get_types_for_add_instance(); + $matchestoolnotavailabletocourse = false; + if (!$manualinstance && !empty($this->current->toolurl) && lti_get_type_config($this->current->typeid)) { + // Type was found, so it's likely been domain matched. + $matchestoolnotavailabletocourse = !in_array($this->current->typeid, array_keys($instancetypes)); + } + $mform =& $this->_form; + // Show the deprecation notice when displaying any manually configured instance, regardless of whether the user can view + // the tool configuration details or not. They will still see locked privacy fields and should be told why that is. + if ($manualinstance || $matchestoolnotavailabletocourse) { + $mform->addElement('html', $OUTPUT->notification( + get_string('editmanualinstancedeprecationwarning', 'mod_lti', get_docs_url('External_tool')), + \core\output\notification::NOTIFY_WARNING, false)); + } + // Adding the "general" fieldset, where all the common settings are shown. $mform->addElement('html', ""); $mform->addElement('header', 'general', get_string('general', 'form')); @@ -150,59 +177,73 @@ class mod_lti_mod_form extends moodleform_mod { $noncontentitemtypes = []; if ($showtypes) { - $tooltypes = $mform->addElement('select', 'typeid', get_string('external_tool_type', 'lti')); - if ($this->typeid) { - $mform->getElement('typeid')->setValue($this->typeid); - } - $mform->addHelpButton('typeid', 'external_tool_type', 'lti'); + if ($manualinstance) { + // Legacy, manually configured instances: only freeze the element (not hardFreeze) so that disabledIf() still works. + // The data in the select is restricted so that only the current value is deemed valid, preventing DOM-edit changes, + // which are possible with frozen elements. + $tooltypes = $mform->addElement('select', 'typeid', get_string('external_tool_type', 'lti')); + $mform->addHelpButton('typeid', 'external_tool_type', 'lti'); + $manualinstanceoption = $instancetypes[0]; // The 'Automatic, based on tool URL' option. + $tooltypes->addOption($manualinstanceoption->name, 0, []); + $mform->freeze('typeid'); + } else if ($matchestoolnotavailabletocourse) { + // Legacy instances domain-matched to site tools: use a hidden field for typeid and a static visual element when + // displaying these instances so that the string value of typeid is still visible when the element is frozen. + // This gets around the fact that a frozen select without a selected option will display nothing. + $mform->addElement('hidden', 'typeid', $this->current->typeid); + $mform->setType('typeid', PARAM_INT); - // To prevent the use of manually configured instances, existing instances which are using a preconfigured tool will not - // display the option "Automatic, based on tool URL" in the preconfigured tools select. This prevents switching from an - // instance configured using a preconfigured tool to an instance that is manually configured. - // Exceptions are made for: - // - Existing manually configured instances (i.e. no type set). - // - Instances using a tool type which isn't visible in the course preconfigured tools selector, such as when a - // site-level tool type is domain-matched. - $instancetypes = lti_get_types_for_add_instance(); - if (!empty($this->current->typeid) && array_key_exists($this->current->typeid, $instancetypes)) { + $manualinstanceoption = $instancetypes[0]; // The 'Automatic, based on tool URL' option. + $mform->addElement('static', 'typeiddisplayonly', get_string('external_tool_type', 'lti'), + $manualinstanceoption->name); + } else { + // To prevent the use of manually configured instances, existing instances which are using a preconfigured tool will + // not display the option "Automatic, based on tool URL" in the preconfigured tools select. This prevents switching + // from an instance configured using a preconfigured tool to an instance that is manually configured. unset($instancetypes[0]); - } - foreach ($instancetypes as $id => $type) { - if (!empty($type->toolproxyid)) { - $toolproxy[] = $type->id; - $attributes = array('globalTool' => 1, 'toolproxy' => 1); - $enabledcapabilities = explode("\n", $type->enabledcapability); - if (!in_array('Result.autocreate', $enabledcapabilities) || - in_array('BasicOutcome.url', $enabledcapabilities)) { - $attributes['nogrades'] = 1; - } - if (!in_array('Person.name.full', $enabledcapabilities) && - !in_array('Person.name.family', $enabledcapabilities) && - !in_array('Person.name.given', $enabledcapabilities)) { - $attributes['noname'] = 1; - } - if (!in_array('Person.email.primary', $enabledcapabilities)) { - $attributes['noemail'] = 1; - } - } else if ($type->course == $COURSE->id) { - $attributes = array('editable' => 1, 'courseTool' => 1, 'domain' => $type->tooldomain); - } else if ($id != 0) { - $attributes = array('globalTool' => 1, 'domain' => $type->tooldomain); - } else { - $attributes = array(); + $tooltypes = $mform->addElement('select', 'typeid', get_string('external_tool_type', 'lti')); + if ($this->typeid) { + $mform->getElement('typeid')->setValue($this->typeid); } + $mform->addHelpButton('typeid', 'external_tool_type', 'lti'); - if ($id) { - $config = lti_get_type_config($id); - if (!empty($config['contentitem'])) { - $attributes['data-contentitem'] = 1; - $attributes['data-id'] = $id; + foreach ($instancetypes as $id => $type) { + if (!empty($type->toolproxyid)) { + $toolproxy[] = $type->id; + $attributes = array('globalTool' => 1, 'toolproxy' => 1); + $enabledcapabilities = explode("\n", $type->enabledcapability); + if (!in_array('Result.autocreate', $enabledcapabilities) || + in_array('BasicOutcome.url', $enabledcapabilities)) { + $attributes['nogrades'] = 1; + } + if (!in_array('Person.name.full', $enabledcapabilities) && + !in_array('Person.name.family', $enabledcapabilities) && + !in_array('Person.name.given', $enabledcapabilities)) { + $attributes['noname'] = 1; + } + if (!in_array('Person.email.primary', $enabledcapabilities)) { + $attributes['noemail'] = 1; + } + } else if ($type->course == $COURSE->id) { + $attributes = array('editable' => 1, 'courseTool' => 1, 'domain' => $type->tooldomain); + } else if ($id != 0) { + $attributes = array('globalTool' => 1, 'domain' => $type->tooldomain); } else { - $noncontentitemtypes[] = $id; + $attributes = array(); } + + if ($id) { + $config = lti_get_type_config($id); + if (!empty($config['contentitem'])) { + $attributes['data-contentitem'] = 1; + $attributes['data-id'] = $id; + } else { + $noncontentitemtypes[] = $id; + } + } + $tooltypes->addOption($type->name, $id, $attributes); } - $tooltypes->addOption($type->name, $id, $attributes); } } else { $mform->addElement('hidden', 'typeid', $this->typeid); @@ -234,9 +275,14 @@ class mod_lti_mod_form extends moodleform_mod { $allnoncontentitemtypes = $noncontentitemtypes; $allnoncontentitemtypes[] = '0'; // Add option value for "Automatic, based on tool URL". $mform->disabledIf('selectcontent', 'typeid', 'in', $allnoncontentitemtypes); + + // Always disable select content for legacy tool instances domain-matched to site tools. + if ($matchestoolnotavailabletocourse) { + $mform->disabledIf('selectcontent', 'typeid', 'in', [$this->current->typeid]); + } } - if ($showoptions) { + if ($canviewmanualconfig) { $mform->addElement('text', 'toolurl', get_string('launch_url', 'lti'), array('size' => '64')); $mform->setType('toolurl', PARAM_URL); $mform->addHelpButton('toolurl', 'launch_url', 'lti'); @@ -282,7 +328,7 @@ class mod_lti_mod_form extends moodleform_mod { $mform->addHelpButton('launchcontainer', 'launchinpopup', 'lti'); $mform->setAdvanced('launchcontainer'); - if ($showoptions) { + if ($canviewmanualconfig) { $mform->addElement('text', 'resourcekey', get_string('resourcekey', 'lti')); $mform->setType('resourcekey', PARAM_TEXT); $mform->setAdvanced('resourcekey'); @@ -359,51 +405,66 @@ class mod_lti_mod_form extends moodleform_mod { array('sesskey' => sesskey(), 'course' => $COURSE->id)); $ajaxurl = new moodle_url('/mod/lti/ajax.php'); - // All these icon uses are incorrect. LTI JS needs updating to use AMD modules and templates so it can use - // the mustache pix helper - until then LTI will have inconsistent icons. - $jsinfo = (object)array( - 'edit_icon_url' => (string)$OUTPUT->image_url('t/edit'), - 'add_icon_url' => (string)$OUTPUT->image_url('t/add'), - 'delete_icon_url' => (string)$OUTPUT->image_url('t/delete'), - 'green_check_icon_url' => (string)$OUTPUT->image_url('i/valid'), - 'warning_icon_url' => (string)$OUTPUT->image_url('warning', 'lti'), - 'instructor_tool_type_edit_url' => $editurl->out(false), - 'ajax_url' => $ajaxurl->out(true), - 'courseId' => $COURSE->id - ); - - $module = array( - 'name' => 'mod_lti_edit', - 'fullpath' => '/mod/lti/mod_form.js', - 'requires' => array('base', 'io', 'querystring-stringify-simple', 'node', 'event', 'json-parse'), - 'strings' => array( - array('addtype', 'lti'), - array('edittype', 'lti'), - array('deletetype', 'lti'), - array('delete_confirmation', 'lti'), - array('cannot_edit', 'lti'), - array('cannot_delete', 'lti'), - array('global_tool_types', 'lti'), - array('course_tool_types', 'lti'), - array('using_tool_configuration', 'lti'), - array('using_tool_cartridge', 'lti'), - array('domain_mismatch', 'lti'), - array('custom_config', 'lti'), - array('tool_config_not_found', 'lti'), - array('tooltypeadded', 'lti'), - array('tooltypedeleted', 'lti'), - array('tooltypenotdeleted', 'lti'), - array('tooltypeupdated', 'lti'), - array('forced_help', 'lti') - ), - ); - if (!empty($this->typeid)) { $mform->setAdvanced('typeid'); $mform->setAdvanced('toolurl'); } - $PAGE->requires->js_init_call('M.mod_lti.editor.init', array(json_encode($jsinfo)), true, $module); + if ($manualinstance || $matchestoolnotavailabletocourse) { + $mform->hardFreeze([ + 'toolurl', + 'securetoolurl', + 'launchcontainer', + 'resourcekey', + 'password', + 'instructorcustomparameters', + 'icon', + 'secureicon', + 'instructorchoicesendname', + 'instructorchoicesendemailaddr', + 'instructorchoiceacceptgrades' + ]); + } else { + // All these icon uses are incorrect. LTI JS needs updating to use AMD modules and templates so it can use + // the mustache pix helper - until then LTI will have inconsistent icons. + $jsinfo = (object)array( + 'edit_icon_url' => (string)$OUTPUT->image_url('t/edit'), + 'add_icon_url' => (string)$OUTPUT->image_url('t/add'), + 'delete_icon_url' => (string)$OUTPUT->image_url('t/delete'), + 'green_check_icon_url' => (string)$OUTPUT->image_url('i/valid'), + 'warning_icon_url' => (string)$OUTPUT->image_url('warning', 'lti'), + 'instructor_tool_type_edit_url' => $editurl->out(false), + 'ajax_url' => $ajaxurl->out(true), + 'courseId' => $COURSE->id + ); + + $module = array( + 'name' => 'mod_lti_edit', + 'fullpath' => '/mod/lti/mod_form.js', + 'requires' => array('base', 'io', 'querystring-stringify-simple', 'node', 'event', 'json-parse'), + 'strings' => array( + array('addtype', 'lti'), + array('edittype', 'lti'), + array('deletetype', 'lti'), + array('delete_confirmation', 'lti'), + array('cannot_edit', 'lti'), + array('cannot_delete', 'lti'), + array('global_tool_types', 'lti'), + array('course_tool_types', 'lti'), + array('using_tool_configuration', 'lti'), + array('using_tool_cartridge', 'lti'), + array('domain_mismatch', 'lti'), + array('custom_config', 'lti'), + array('tool_config_not_found', 'lti'), + array('tooltypeadded', 'lti'), + array('tooltypedeleted', 'lti'), + array('tooltypenotdeleted', 'lti'), + array('tooltypeupdated', 'lti'), + array('forced_help', 'lti') + ), + ); + $PAGE->requires->js_init_call('M.mod_lti.editor.init', array(json_encode($jsinfo)), true, $module); + } } /** diff --git a/mod/lti/tests/behat/managecoursetools.feature b/mod/lti/tests/behat/managecoursetools.feature index 572047c33b8..5053431836b 100644 --- a/mod/lti/tests/behat/managecoursetools.feature +++ b/mod/lti/tests/behat/managecoursetools.feature @@ -63,7 +63,6 @@ Feature: Manage course tools Given the following "role capability" exists: | role | editingteacher | | mod/lti:addcoursetool | allow | - | mod/lti:addmanualinstance | allow | | mod/lti:addpreconfiguredinstance | prohibit | And the following "mod_lti > course tools" exist: | name | description | baseurl | course | diff --git a/mod/lti/tests/locallib_test.php b/mod/lti/tests/locallib_test.php index e78e2b2496f..8853c4be68b 100644 --- a/mod/lti/tests/locallib_test.php +++ b/mod/lti/tests/locallib_test.php @@ -2358,16 +2358,8 @@ MwIDAQAB \core\context\course::instance($course->id)); $coursetooltypes = lti_get_lti_types_by_course($course->id); $this->assertDebuggingCalled(); - $this->assertCount(1, $coursetooltypes); + $this->assertCount(0, $coursetooltypes); $this->unassignUserCapability('mod/lti:addpreconfiguredinstance', (\core\context\course::instance($course->id))->id, $teacherrole->id); - - // Request for a teacher who cannot use manually configured tools in the course. - // Only return site tools, which is broken legacy behaviour. - assign_capability('mod/lti:addmanualinstance', CAP_PROHIBIT, $teacherrole->id, - \context_course::instance($course->id)); - $coursetooltypes = lti_get_lti_types_by_course($course->id); - $this->assertDebuggingCalled(); - $this->assertCount(2, $coursetooltypes); } } diff --git a/mod/lti/upgrade.txt b/mod/lti/upgrade.txt index a45dcac1ab6..b3d2abb0e8b 100644 --- a/mod/lti/upgrade.txt +++ b/mod/lti/upgrade.txt @@ -6,6 +6,8 @@ This files describes API changes in the lti code. * The `mod_lti_mod_form` constructor will now throw an exception if called without passing a typeid as manual configuration of instances is now unsupported. * The `lti_get_lti_types_by_course` method is deprecated. Please use mod_lti\local\types_helper::get_lti_types_by_course instead. +* The capability `mod/lti:addmanualinstance` is now deprecated. Since manual instance creation is no longer supported, there is no + substitute for this capability. === 4.2 === diff --git a/mod/lti/version.php b/mod/lti/version.php index fb336cb1ae8..542dcd5b45d 100644 --- a/mod/lti/version.php +++ b/mod/lti/version.php @@ -48,7 +48,7 @@ defined('MOODLE_INTERNAL') || die; -$plugin->version = 2023070501; // The current module version (Date: YYYYMMDDXX). +$plugin->version = 2023081100; // The current module version (Date: YYYYMMDDXX). $plugin->requires = 2023041800; // Requires this Moodle version. $plugin->component = 'mod_lti'; // Full name of the plugin (used for diagnostics). $plugin->cron = 0; From dfd215cad2c6fa75f277e37f92cdb5a3c1463c0d Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 9 Aug 2023 16:12:21 +0800 Subject: [PATCH 16/21] MDL-78597 mod_lti: add cartridge support to course tool generator Allow the generator to create course tools based on cartridge URLs. --- mod/lti/tests/generator/lib.php | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/mod/lti/tests/generator/lib.php b/mod/lti/tests/generator/lib.php index 5525de65e32..6e7bb954af3 100644 --- a/mod/lti/tests/generator/lib.php +++ b/mod/lti/tests/generator/lib.php @@ -98,7 +98,7 @@ class mod_lti_generator extends testing_module_generator { * - 'ltiservice_' prefix: denotes 'config' data, specifically config for service plugins. * * @param array $data array of type and config data containing prefixed keys. - * @return array containing separated type and config data. E.g. ['type' = [...], 'config' => [...]] + * @return array containing separated objects for type and config data. E.g. ['type' = stdClass, 'config' => stdClass] */ protected function get_type_and_config_from_data(array $data): array { // Grab any non-prefixed fields; these are the type fields. The rest is considered config. @@ -109,7 +109,7 @@ class mod_lti_generator extends testing_module_generator { ); $config = array_diff_key($data, $type); - return ['type' => $type, 'config' => $config]; + return ['type' => (object) $type, 'config' => (object) $config]; } /** @@ -159,8 +159,18 @@ class mod_lti_generator extends testing_module_generator { $type['lti_sendname'] = $type['lti_sendname'] ?? LTI_SETTING_ALWAYS; $type['lti_sendemailaddr'] = $type['lti_sendemailaddr'] ?? LTI_SETTING_ALWAYS; + // Required for cartridge processing support. + $type['lti_toolurl'] = $type['baseurl']; + $type['lti_description'] = $type['description'] ?? ''; + $type['lti_icon'] = $type['icon'] ?? ''; + $type['lti_secureicon'] = $type['secureicon'] ?? ''; + if (!empty($type['name'])) { + $type['lti_typename'] = $type['name']; + } + ['type' => $type, 'config' => $config] = $this->get_type_and_config_from_data($type); - lti_add_type(type: (object) $type, config: (object) $config); + lti_load_type_if_cartridge($config); + lti_add_type(type: $type, config: $config); } } From 9914870b7dcdf78c1699abb07da49a345b149325 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 9 Aug 2023 16:23:04 +0800 Subject: [PATCH 17/21] MDL-78597 mod_lti: cover creation of course tools using cartridge URLs This replaces the legacy scenario, which used the '+' on the activity instance edit form, with a scenario covering the cartridge use on the new course tools (LTI External tools) edit form. --- mod/lti/tests/behat/addtype.feature | 23 +------------------ mod/lti/tests/behat/managecoursetools.feature | 20 ++++++++++++++++ 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/mod/lti/tests/behat/addtype.feature b/mod/lti/tests/behat/addtype.feature index 8d2869bbec2..3e87dd1f74b 100644 --- a/mod/lti/tests/behat/addtype.feature +++ b/mod/lti/tests/behat/addtype.feature @@ -18,7 +18,7 @@ Feature: Add preconfigured tools via teacher interface | activity | course | name | typeid | toolurl | | lti | C1 | Test tool | 0 | /mod/lti/tests/fixtures/ims_cartridge_basic_lti_link.xml | - Scenario: Add a tool from a cartridge + Scenario: Add a tool activity instance from a cartridge Given I am on the "Test tool" "lti activity editing" page logged in as teacher1 And I expand all fieldsets Then the field "Tool URL" matches value "http://www.example.com/lti/provider.php" @@ -26,27 +26,6 @@ Feature: Add preconfigured tools via teacher interface And the field "Icon URL" matches value "http://download.moodle.org/unittest/test.jpg" And the field "Secure icon URL" matches value "https://download.moodle.org/unittest/test.jpg" - @javascript @_switch_window - Scenario: Add a preconfigured tool from a cartridge - Given I am on the "Test tool" "lti activity editing" page logged in as teacher1 - And I follow "Add preconfigured tool" - And I switch to "add_tool" window - And I set the field "Tool name" to "Placeholder" - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/ims_cartridge_basic_lti_link.xml" - And I press "Save changes" - And I switch to the main window - And I wait "2" seconds - And I follow "Edit preconfigured tool" - When I switch to "edit_tool" window - Then the field "Tool URL" matches value "http://www.example.com/lti/provider.php" - And the field "Icon URL" matches value "http://download.moodle.org/unittest/test.jpg" - And the field "Secure icon URL" matches value "https://download.moodle.org/unittest/test.jpg" - And I press "Cancel" - And I switch to the main window - And I press "Save and display" - And I am on the "Test tool" "lti activity editing" page - And the field "Preconfigured tool" matches value "Placeholder" - @javascript @_switch_window Scenario: Add and use a preconfigured tool Given I am on the "Test tool" "lti activity editing" page logged in as teacher1 diff --git a/mod/lti/tests/behat/managecoursetools.feature b/mod/lti/tests/behat/managecoursetools.feature index 5053431836b..6a50146f973 100644 --- a/mod/lti/tests/behat/managecoursetools.feature +++ b/mod/lti/tests/behat/managecoursetools.feature @@ -127,3 +127,23 @@ Feature: Manage course tools And I click on "Delete" "button" in the "Delete Test tool" "dialogue" And I should see "Test tool removed" And I should not see "Test tool" in the "reportbuilder-table" "table" + + @javascript + Scenario: Add a course tool using a cartridge URL + Given I am on the "Course 1" course page logged in as teacher1 + And I navigate to "LTI External tools" in current page administration + When I click on "Add tool" "link" + And I set the following fields to these values: + | Tool name | Test tool 1 | + | Tool description | Test tool 1 description | + And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/ims_cartridge_basic_lti_link.xml" + And I press "Save changes" + Then I should see "Test tool 1" in the "reportbuilder-table" "table" + # The cartridge description, if set, overrides the description set in the type edit form (bug?). + And I should see "Example tool description" in the "Test tool 1" "table_row" + And I open the action menu in "Test tool 1" "table_row" + And I choose "Edit" in the open action menu + And the field "Tool name" matches value "Test tool 1" + And the field "Tool URL" matches value "http://www.example.com/lti/provider.php" + And the field "Icon URL" matches value "http://download.moodle.org/unittest/test.jpg" + And the field "Secure icon URL" matches value "https://download.moodle.org/unittest/test.jpg" From 5c737525d8a17602f84b82fdb25ed05fb2e87771 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Thu, 10 Aug 2023 09:29:57 +0800 Subject: [PATCH 18/21] MDL-78597 mod_lti: Add test for manually configured instance editing This adds a single scenario covering all the expected behavior for those existing, manually configured tools. It also removes any tests dealing with creation of manually configured instances, which is not supported any more. --- mod/lti/tests/behat/addtool.feature | 82 +++++++++++++++++++++++++ mod/lti/tests/behat/addtype.feature | 35 ----------- mod/lti/tests/behat/contentitem.feature | 40 ------------ mod/lti/tests/generator/lib.php | 2 +- 4 files changed, 83 insertions(+), 76 deletions(-) delete mode 100644 mod/lti/tests/behat/addtype.feature diff --git a/mod/lti/tests/behat/addtool.feature b/mod/lti/tests/behat/addtool.feature index 32233c6a800..f30782ef81b 100644 --- a/mod/lti/tests/behat/addtool.feature +++ b/mod/lti/tests/behat/addtool.feature @@ -58,3 +58,85 @@ Feature: Add tools And the "Tool URL" "field" should be disabled And I click on "Preconfigured tool" "select" And I should not see "Automatic, based on tool URL" + + @javascript + Scenario: Editing a (deprecated) manually configured activity instance, confirming that config changes aren't possible + Given the following "activities" exist: + | activity | name | course | toolurl | + | lti | A manual tool | C1 | /mod/lti/tests/fixtures/ims_cartridge_basic_lti_link.xml | + # Add a course tool with the same URL as that of the manually configured instance (the tool URL found in the above cartridge). + # This would normally be domain-matched during edit, resulting in the assignment of a preconfigured tool to the instance. + # In this case, because config changes and domain matching are disabled, the test confirms this doesn't take place. + And the following "mod_lti > course tools" exist: + | name | baseurl | course | lti_sendname | lti_sendemailaddr | lti_acceptgrades | + | Course tool 2 | http://www.example.com/lti/provider.php | C1 | 0 | 1 | 2 | + When I am on the "A manual tool" "lti activity editing" page logged in as teacher1 + Then I should see "Manually configured External tool activities are no longer supported" + And I follow "Show more..." + And I expand all fieldsets + # The privacy values below represent the existing values of the privacy settings, before saving and inheriting from the + # domain-matched tool values. + And the following fields match these values: + | Activity name | A manual tool | + | id_showdescription | 0 | + | Consumer key | 12345 | + | Icon URL | http://download.moodle.org/unittest/test.jpg | + | Secure icon URL | https://download.moodle.org/unittest/test.jpg | + | Tool URL | http://www.example.com/lti/provider.php | + | id_instructorchoicesendname | 1 | + | id_instructorchoicesendemailaddr | 1 | + | id_instructorchoiceacceptgrades | 1 | + And the "Activity name" "field" should be enabled + And the "Activity description" "field" should be enabled + And the "id_showdescription" "checkbox" should be enabled + And the "id_showtitlelaunch" "checkbox" should be enabled + And the "id_showdescriptionlaunch" "checkbox" should be enabled + And the "Secure tool URL" "field" should be disabled + And the "Consumer key" "field" should be disabled + And I click on "Reveal" "icon" + And I should see "secret" + And the "Custom parameters" "field" should be disabled + And the "Icon URL" "field" should be disabled + And the "Secure icon URL" "field" should be disabled + And I should see "Automatic, based on tool URL" + And the "Select content" "button" should be disabled + And the "Tool URL" "field" should be disabled + And the "id_instructorchoicesendname" "checkbox" should be disabled + And the "id_instructorchoicesendemailaddr" "checkbox" should be disabled + And the "id_instructorchoiceacceptgrades" "checkbox" should be disabled + And I set the following fields to these values: + | Activity name | A manual tool name edited | + | id_showdescription | 1 | + And I press "Save and return to course" + And I am on the "A manual tool" "lti activity editing" page logged in as teacher1 + And I follow "Show more..." + # This confirms that the instance config, while locked to user edits, still inherits privacy settings from the tool which + # it was domain-matched to. + And the following fields match these values: + | Activity name | A manual tool name edited | + | id_showdescription | 1 | + | Consumer key | 12345 | + | Icon URL | http://download.moodle.org/unittest/test.jpg | + | Secure icon URL | https://download.moodle.org/unittest/test.jpg | + | Tool URL | http://www.example.com/lti/provider.php | + | id_instructorchoicesendname | 0 | + | id_instructorchoicesendemailaddr | 1 | + | id_instructorchoiceacceptgrades | 2 | + And the "Activity name" "field" should be enabled + And the "Activity description" "field" should be enabled + And the "id_showdescription" "checkbox" should be enabled + And the "id_showtitlelaunch" "checkbox" should be enabled + And the "id_showdescriptionlaunch" "checkbox" should be enabled + And the "Secure tool URL" "field" should be disabled + And the "Consumer key" "field" should be disabled + And I click on "Reveal" "icon" + And I should see "secret" + And the "Custom parameters" "field" should be disabled + And the "Icon URL" "field" should be disabled + And the "Secure icon URL" "field" should be disabled + And I should see "Automatic, based on tool URL" + And the "Select content" "button" should be disabled + And the "Tool URL" "field" should be disabled + And the "id_instructorchoicesendname" "checkbox" should be disabled + And the "id_instructorchoicesendemailaddr" "checkbox" should be disabled + And the "id_instructorchoiceacceptgrades" "checkbox" should be disabled diff --git a/mod/lti/tests/behat/addtype.feature b/mod/lti/tests/behat/addtype.feature deleted file mode 100644 index 3e87dd1f74b..00000000000 --- a/mod/lti/tests/behat/addtype.feature +++ /dev/null @@ -1,35 +0,0 @@ -@mod @mod_lti -Feature: Add preconfigured tools via teacher interface - In order to provide reusable activities for teachers - As a teacher - I need to be able to add preconfigured tools - - Background: - Given the following "users" exist: - | username | firstname | lastname | email | - | teacher1 | Terry1 | Teacher1 | teacher1@example.com | - And the following "courses" exist: - | fullname | shortname | category | - | Course 1 | C1 | 0 | - And the following "course enrolments" exist: - | user | course | role | - | teacher1 | C1 | editingteacher | - And the following "activities" exist: - | activity | course | name | typeid | toolurl | - | lti | C1 | Test tool | 0 | /mod/lti/tests/fixtures/ims_cartridge_basic_lti_link.xml | - - Scenario: Add a tool activity instance from a cartridge - Given I am on the "Test tool" "lti activity editing" page logged in as teacher1 - And I expand all fieldsets - Then the field "Tool URL" matches value "http://www.example.com/lti/provider.php" - And the field "Secure tool URL" matches value "https://www.example.com/lti/provider.php" - And the field "Icon URL" matches value "http://download.moodle.org/unittest/test.jpg" - And the field "Secure icon URL" matches value "https://download.moodle.org/unittest/test.jpg" - - @javascript @_switch_window - Scenario: Add and use a preconfigured tool - Given I am on the "Test tool" "lti activity editing" page logged in as teacher1 - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/tool_provider.php" - And I press "Save and display" - When I switch to "contentframe" iframe - Then I should see "This represents a tool provider" diff --git a/mod/lti/tests/behat/contentitem.feature b/mod/lti/tests/behat/contentitem.feature index 51efb901b77..3158f086793 100644 --- a/mod/lti/tests/behat/contentitem.feature +++ b/mod/lti/tests/behat/contentitem.feature @@ -39,43 +39,3 @@ Feature: Content-Item support And I choose "Edit settings" in the open action menu Then the field "Preconfigured tool" matches value "Teaching Tool 1" And the "Select content" "button" should be enabled - - @javascript - Scenario: Changing preconfigured tool selection - Given the following "mod_lti > tool types" exist: - | name | description | baseurl | coursevisible | state | - | Teaching Tool 2 | Another description | /mod/lti/tests/fixtures/tool_provider.php | 2 | 1 | - # Create a manually configured instance using the generator (this isn't possible via the UI any more). - And the following "activities" exist: - | activity | course | name | typeid | toolurl | - | lti | C1 | Test tool activity 1 | 0 | /mod/lti/tests/fixtures/tool_provider.php | - When I am on the "Test tool activity 1" "lti activity editing" page logged in as teacher1 - # On load with no preconfigured tool selected: Select content button - disabled, Tool URL - enabled. - And the field "Preconfigured tool" matches value "Automatic, based on tool URL" - And I set the field "Activity name" to "Test tool activity 1" - And the "Select content" "button" should be disabled - And the "Tool URL" "field" should be enabled - # Selecting a tool that supports deep linking: Select content button - enabled, Tool URL - enabled. - And I set the field "Preconfigured tool" to "Teaching Tool 1" - And I set the field "Activity name" to "Test tool activity 1" - Then the "Select content" "button" should be enabled - And the "Tool URL" "field" should be enabled - # Selecting a tool that does not support deep linking: Select content button - disabled, Tool URL - disabled. - And I set the field "Preconfigured tool" to "Teaching Tool 2" - And I set the field "Activity name" to "Test tool activity 1" - And the "Select content" "button" should be disabled - And the "Tool URL" "field" should be disabled - # Not selecting any tool: Select content button - disabled, Tool URL - enabled. - And I set the field "Preconfigured tool" to "Automatic, based on tool URL" - And I set the field "Activity name" to "Test tool activity 1" - And the "Select content" "button" should be disabled - And the "Tool URL" "field" should be enabled - - @javascript - Scenario: Editing a manually configured external tool - Given the following "activities" exist: - | activity | course | name | typeid | toolurl | - | lti | C1 | Test tool | 0 | /mod/lti/tests/fixtures/tool_provider.php | - And I am on the "Test tool" "lti activity editing" page logged in as teacher1 - Then the field "Preconfigured tool" matches value "Automatic, based on tool URL" - And the "Select content" "button" should be disabled diff --git a/mod/lti/tests/generator/lib.php b/mod/lti/tests/generator/lib.php index 6e7bb954af3..d6837c0c30a 100644 --- a/mod/lti/tests/generator/lib.php +++ b/mod/lti/tests/generator/lib.php @@ -68,7 +68,7 @@ class mod_lti_generator extends testing_module_generator { $record->instructorchoiceacceptgrades = 1; } if (!isset($record->typeid)) { - $record->typeid = null; + $record->typeid = 0; } return parent::create_instance($record, (array)$options); } From 7cb2a78626182b320ba2a781fd541193e374cd01 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 29 Aug 2023 16:08:53 +0800 Subject: [PATCH 19/21] MDL-78597 mod_lti: add category restriction support to lti generator This is already supported in lti_add_type, via the lti_coursecategories config value, so all that's needed is to match the categories by idnumber and pass through the resulting ids. --- .../generator/behat_mod_lti_generator.php | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/mod/lti/tests/generator/behat_mod_lti_generator.php b/mod/lti/tests/generator/behat_mod_lti_generator.php index b887525b954..ce48bc59f59 100644 --- a/mod/lti/tests/generator/behat_mod_lti_generator.php +++ b/mod/lti/tests/generator/behat_mod_lti_generator.php @@ -44,6 +44,7 @@ class behat_mod_lti_generator extends behat_generator_base { 'singular' => 'tool type', 'datagenerator' => 'tool_types', 'required' => ['baseurl'], + 'switchids' => ['lti_coursecategories' => 'lti_coursecategories'] ], 'course tools' => [ 'singular' => 'course tool', @@ -74,4 +75,24 @@ class behat_mod_lti_generator extends behat_generator_base { } return (int) $id; } + + /** + * Handles the switchid ['lti_coursecategories' => 'lti_coursecategories'] for restricting a tool to certain categories. + * + * @param string $idnumbers a comma-separated string containing the course category id numbers, e.g. 'cata, catb, catc'. + * @return string a comma-separated string containing the course category ids. + * @throws coding_exception if one or more of the categories is unable to be matched by its idnumber. + */ + protected function get_lti_coursecategories_id(string $idnumbers): string { + global $DB; + $categoryids = array_map('trim', explode(',', $idnumbers)); + + [$insql, $inparams] = $DB->get_in_or_equal($categoryids); + $ids = $DB->get_fieldset_sql("SELECT id FROM {course_categories} WHERE idnumber $insql", $inparams); + if (!$ids || count($ids) != count($categoryids)) { + throw new coding_exception("One or more course categories unable to be matched using idnumbers: $idnumbers"); + } + + return implode(',', $ids); + } } From c497672778fbca0f176bf237457eb9ae29e7674d Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 29 Aug 2023 16:10:45 +0800 Subject: [PATCH 20/21] MDL-78597 mod_lti: add category restriction support to course tools list --- .../systemreports/course_external_tools_list.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/mod/lti/classes/reportbuilder/local/systemreports/course_external_tools_list.php b/mod/lti/classes/reportbuilder/local/systemreports/course_external_tools_list.php index fad07005b3a..14cab9dd3c4 100644 --- a/mod/lti/classes/reportbuilder/local/systemreports/course_external_tools_list.php +++ b/mod/lti/classes/reportbuilder/local/systemreports/course_external_tools_list.php @@ -65,12 +65,20 @@ class course_external_tools_list extends system_report { FROM {lti} $ti WHERE $ti.typeid = {$entitymainalias}.id) AS toolusage"); - // Scope the report to the course context only. + // Join the types_categories table, to include only tools available to the current course's category. + $cattablealias = database::generate_alias(); + $joinsql = "LEFT JOIN {lti_types_categories} {$cattablealias} + ON ({$cattablealias}.typeid = {$entitymainalias}.id)"; + $this->add_join($joinsql); + + // Scope the report to the course context and include only those tools available to the category. $paramprefix = database::generate_param_name(); $coursevisibleparam = database::generate_param_name(); + $categoryparam = database::generate_param_name(); [$insql, $params] = $DB->get_in_or_equal([get_site()->id, $this->course->id], SQL_PARAMS_NAMED, "{$paramprefix}_"); - $wheresql = "{$entitymainalias}.course {$insql} AND {$entitymainalias}.coursevisible NOT IN (:{$coursevisibleparam})"; - $params = array_merge($params, [$coursevisibleparam => LTI_COURSEVISIBLE_NO]); + $wheresql = "{$entitymainalias}.course {$insql} AND {$entitymainalias}.coursevisible NOT IN (:{$coursevisibleparam}) ". + "AND ({$cattablealias}.id IS NULL OR {$cattablealias}.categoryid = :{$categoryparam})"; + $params = array_merge($params, [$coursevisibleparam => LTI_COURSEVISIBLE_NO, $categoryparam => $this->course->category]); $this->add_base_condition_sql($wheresql, $params); $this->set_downloadable(false, get_string('pluginname', 'mod_lti')); From a23abb0538e198121dcc7b78e9195deddfb31615 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 29 Aug 2023 11:45:36 +0800 Subject: [PATCH 21/21] MDL-78597 mod_lti: fix those tests not using lti generators The following were added in MDL-69489 and had a few issues, which are fixed in this patch: Unit tests: - were not using the existing generator. - were making assertions inside a foreach, which doesn't test anything if the result set returned is empty (error prone). - were calling setAdminUser() unnecessarily. Behat tests: - the restricttocategory.feature file used manual steps to setup site tools. --- .../tests/behat/restricttocategory.feature | 46 +++---- mod/lti/tests/locallib_test.php | 120 ++++++++++-------- 2 files changed, 84 insertions(+), 82 deletions(-) diff --git a/mod/lti/tests/behat/restricttocategory.feature b/mod/lti/tests/behat/restricttocategory.feature index b9ae7c4ab70..a006548d499 100644 --- a/mod/lti/tests/behat/restricttocategory.feature +++ b/mod/lti/tests/behat/restricttocategory.feature @@ -1,4 +1,4 @@ -@mod @mod_lti @javascript +@mod @mod_lti Feature: Make an LTI only available to specific course categories In order to restrict which courses a tool can be used in As an administrator @@ -24,48 +24,34 @@ Feature: Make an LTI only available to specific course categories | teacher1 | C1 | editingteacher | | teacher1 | C2 | editingteacher | | teacher1 | C3 | editingteacher | - And I log in as "admin" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - And I follow "Manage preconfigured tools" - And I follow "Add preconfigured tool" - And I expand all fieldsets - And I set the following fields to these values: - | Tool name | Teaching Tool 1 | - | Tool configuration usage | Show as preconfigured tool when adding an external tool | - | catb | 1 | - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/tool_provider.php" - And I press "Save changes" - And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration - And I follow "Manage preconfigured tools" - And I follow "Add preconfigured tool" - And I expand all fieldsets - And I click on "cata" "link" - And I set the following fields to these values: - | Tool name | Teaching Tool 2 | - | Tool configuration usage | Show in activity chooser and as a preconfigured tool | - | catca | 1 | - And I set the field "Tool URL" to local url "/mod/lti/tests/fixtures/tool_provider.php" - And I press "Save changes" + And the following "mod_lti > tool types" exist: + | name | description | baseurl | coursevisible | state | lti_coursecategories | + | Teaching Tool 1 | Tool 1 description | /mod/lti/tests/fixtures/tool_provider.php | 1 | 1 | catb | + | Teaching Tool 2 | Tool 2 description | /mod/lti/tests/fixtures/tool_provider.php | 2 | 1 | catca | Scenario: Tool is set to "Show as preconfigured tool when adding an external tool" on parent category - Given I log in as "teacher1" - And I am on "Course 2" course homepage with editing mode on - And I add a "External tool" to section "1" - When I click on "Preconfigured tool" "select" - Then I should see "Teaching Tool 1" + Given I am on the "Course 2" course page logged in as teacher1 + When I navigate to "LTI External tools" in current page administration + Then I should see "Teaching Tool 1" in the "reportbuilder-table" "table" + And I should not see "Teaching Tool 2" in the "reportbuilder-table" "table" + @javascript Scenario: Tool is set to "Show in activity chooser and as preconfigured tool" on child category Given I log in as "teacher1" When I am on "Course 3" course homepage with editing mode on And I open the activity chooser Then I should see "Teaching Tool 2" in the "Add an activity or resource" "dialogue" + And I should not see "Teaching Tool 1" in the "Add an activity or resource" "dialogue" - Scenario: Tool restrict access + @javascript + Scenario: View a course in a category in which no tools are available Given I log in as "teacher1" When I am on "Course 1" course homepage with editing mode on And I open the activity chooser - Then I should not see "Teaching Tool 2" in the "Add an activity or resource" "dialogue" + Then I should not see "Teaching Tool 1" in the "Add an activity or resource" "dialogue" + And I should not see "Teaching Tool 2" in the "Add an activity or resource" "dialogue" + @javascript Scenario: Editing and saving selected parent / child categories Given I log in as "admin" And I navigate to "Plugins > Activity modules > External tool > Manage tools" in site administration diff --git a/mod/lti/tests/locallib_test.php b/mod/lti/tests/locallib_test.php index 8853c4be68b..48a9f58fa22 100644 --- a/mod/lti/tests/locallib_test.php +++ b/mod/lti/tests/locallib_test.php @@ -631,36 +631,33 @@ class locallib_test extends mod_lti_testcase { public function test_lti_get_tools_by_domain() { $this->resetAfterTest(); - $this->setAdminUser(); + /** @var \mod_lti_generator $ltigenerator */ + $ltigenerator = $this->getDataGenerator()->get_plugin_generator('mod_lti'); // Create a tool type with good domain. - $type = new \stdClass(); - $data = new \stdClass(); - $data->lti_contentitem = true; - $type->state = LTI_TOOL_STATE_CONFIGURED; - $type->name = "Test tool 1"; - $type->description = "Good example description"; - $type->tooldomain = 'example.com'; - $type->baseurl = 'https://example.com/i/am/?where=here'; - $type->course = SITEID; - $typeid = lti_add_type($type, $data); + $ltigenerator->create_tool_types([ + 'name' => 'Test tool 1', + 'description' => 'Good example description', + 'tooldomain' => 'example.com', + 'baseurl' => 'https://example.com/i/am/?where=here', + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); // Create a tool type with bad domain. - $type = new \stdClass(); - $data = new \stdClass(); - $data->lti_contentitem = true; - $type->state = LTI_TOOL_STATE_CONFIGURED; - $type->name = "Test tool 2"; - $type->description = "Bad example description"; - $type->tooldomain = 'badexample.com'; - $type->baseurl = 'https://badexample.com/i/am/?where=here'; - $type->course = SITEID; - $typeid = lti_add_type($type, $data); + $ltigenerator->create_tool_types([ + 'name' => 'Test tool 2', + 'description' => 'Bad example description', + 'tooldomain' => 'badexample.com', + 'baseurl' => 'https://badexample.com/i/am/?where=here', + 'state' => LTI_TOOL_STATE_CONFIGURED + ]); - $records = lti_get_tools_by_domain('example.com', LTI_TOOL_STATE_CONFIGURED, null); - foreach ($records as $record) { - $this->assertEquals('example.com', $record->tooldomain); - } + $records = lti_get_tools_by_domain('example.com', LTI_TOOL_STATE_CONFIGURED); + $this->assertCount(1, $records); + $this->assertEmpty(array_diff( + ['https://example.com/i/am/?where=here'], + array_column($records, 'baseurl') + )); } /** @@ -671,45 +668,64 @@ class locallib_test extends mod_lti_testcase { public function test_lti_get_tools_by_domain_restrict_types_category() { $this->resetAfterTest(); - $this->setAdminUser(); - $coursecat1 = $this->getDataGenerator()->create_category(); $coursecat2 = $this->getDataGenerator()->create_category(); $course1 = $this->getDataGenerator()->create_course(['category' => $coursecat1->id]); $course2 = $this->getDataGenerator()->create_course(['category' => $coursecat2->id]); + /** @var \mod_lti_generator $ltigenerator */ + $ltigenerator = $this->getDataGenerator()->get_plugin_generator('mod_lti'); + // Create a tool type with domain restricting to a category1. - $type = new \stdClass(); - $data = new \stdClass(); - $data->lti_contentitem = true; - $type->state = LTI_TOOL_STATE_CONFIGURED; - $type->name = "Test tool 1"; - $type->description = "Good example description"; - $type->tooldomain = 'exampleone.com'; - $type->baseurl = 'https://exampleone.com/i/am/?where=here'; - $type->course = $course1->id; - $typeid = lti_add_type($type, $data); - $typecategoryid = lti_type_add_categories($typeid, $coursecat1->id); + $ltigenerator->create_tool_types([ + 'name' => 'Test tool 1', + 'description' => 'Good example description', + 'tooldomain' => 'exampleone.com', + 'baseurl' => 'https://exampleone.com/tool/1', + 'state' => LTI_TOOL_STATE_CONFIGURED, + 'lti_coursecategories' => $coursecat1->id + ]); + + // Create another tool type using the same domain, restricted to category2. + $ltigenerator->create_tool_types([ + 'name' => 'Test tool 1', + 'description' => 'Good example description', + 'tooldomain' => 'exampleone.com', + 'baseurl' => 'https://exampleone.com/tool/2', + 'state' => LTI_TOOL_STATE_CONFIGURED, + 'lti_coursecategories' => $coursecat2->id + ]); // Create a tool type with domain restricting to a category2. - $type = new \stdClass(); - $data = new \stdClass(); - $data->lti_contentitem = true; - $type->state = LTI_TOOL_STATE_CONFIGURED; - $type->name = "Test tool 2"; - $type->description = "Good example description"; - $type->tooldomain = 'exampletwo.com'; - $type->baseurl = 'https://exampletwo.com/i/am/?where=here'; - $type->course = $course2->id; - $typeid = lti_add_type($type, $data); - $typecategoryid = lti_type_add_categories($typeid, $coursecat2->id); + $ltigenerator->create_tool_types([ + 'name' => 'Test tool 2', + 'description' => 'Good example description', + 'tooldomain' => 'exampletwo.com', + 'baseurl' => 'https://exampletwo.com/tool/3', + 'state' => LTI_TOOL_STATE_CONFIGURED, + 'lti_coursecategories' => $coursecat2->id + ]); + // Get tool types for domain 'exampleone' in course 1 and verify only the one result under course category 1 is included. $records = lti_get_tools_by_domain('exampleone.com', LTI_TOOL_STATE_CONFIGURED, $course1->id); - foreach ($records as $record) { - $this->assertEquals('exampleone.com', $record->tooldomain); - } + $this->assertCount(1, $records); + $this->assertEmpty(array_diff( + ['https://exampleone.com/tool/1'], + array_column($records, 'baseurl') + )); + // Get tool types for domain 'exampleone' in course 2 and verify only the one result under course category 2 is included. + $records = lti_get_tools_by_domain('exampleone.com', LTI_TOOL_STATE_CONFIGURED, $course2->id); + $this->assertCount(1, $records); + $this->assertEmpty(array_diff( + ['https://exampleone.com/tool/2'], + array_column($records, 'baseurl') + )); + + // Get tool types for domain 'exampletwo' in course 1 and verify that no results are found. + $records = lti_get_tools_by_domain('exampletwo.com', LTI_TOOL_STATE_CONFIGURED, $course1->id); + $this->assertCount(0, $records); } /**