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')); } /** 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/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')); 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/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/edit_form.php b/mod/lti/edit_form.php index 1aab2c5c8ac..5eb80648664 100644 --- a/mod/lti/edit_form.php +++ b/mod/lti/edit_form.php @@ -180,23 +180,26 @@ 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_PRECONFIGURED); + $mform->addElement('hidden', 'lti_coursevisible', LTI_COURSEVISIBLE_ACTIVITYCHOOSER); } $mform->setType('lti_coursevisible', PARAM_INT); 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 81b583fa22f..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,7 @@ 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'; $string['lti:grade'] = 'View grades returned by the external tool'; @@ -620,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/lib.php b/mod/lti/lib.php index bf7b379eb22..4a8d1edc360 100644 --- a/mod/lti/lib.php +++ b/mod/lti/lib.php @@ -240,23 +240,12 @@ 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() - )]; + // 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; } - - // 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) { // Append the help link to the help text. @@ -270,6 +259,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 +287,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 +313,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/locallib.php b/mod/lti/locallib.php index 71d88d7f0bb..d8f1ec033fe 100644 --- a/mod/lti/locallib.php +++ b/mod/lti/locallib.php @@ -2310,47 +2310,18 @@ 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) { - global $DB, $SITE; + debugging(__FUNCTION__ . '() is deprecated. Please use \mod_lti\local\types_helper::get_lti_types_by_course() instead.', + DEBUG_DEVELOPER); - 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 ?? []); } /** @@ -2359,15 +2330,13 @@ 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; - $types = array(); - if (has_capability('mod/lti:addmanualinstance', context_course::instance($COURSE->id))) { - $types[0] = (object)array('name' => get_string('automatic', 'lti'), 'course' => 0, 'toolproxyid' => null); - } + // 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]]; - foreach ($admintypes as $type) { + $preconfiguredtypes = \mod_lti\local\types_helper::get_lti_types_by_course($COURSE->id, $USER->id); + foreach ($preconfiguredtypes as $type) { $types[$type->id] = $type; } @@ -2382,11 +2351,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; @@ -2643,6 +2613,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; 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); + } } }, diff --git a/mod/lti/mod_form.php b/mod/lti/mod_form.php index 3984a97bbee..b1a88b15558 100644 --- a/mod/lti/mod_form.php +++ b/mod/lti/mod_form.php @@ -53,31 +53,91 @@ 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. + * + * 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) { + + // 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); + + // 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); + } + 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); + // 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); - $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) { $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; } } else { - $showtypes = !$typeid; + $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')); @@ -117,53 +177,79 @@ class mod_lti_mod_form extends moodleform_mod { $noncontentitemtypes = []; if ($showtypes) { - $tooltypes = $mform->addElement('select', 'typeid', get_string('external_tool_type', 'lti')); - if ($typeid) { - $mform->getElement('typeid')->setValue($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); - foreach (lti_get_types_for_add_instance() 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(); + $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]); + + $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', $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 +264,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'; } } @@ -189,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'); @@ -237,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'); @@ -314,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($typeid)) { + 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/addtool.feature b/mod/lti/tests/behat/addtool.feature index 9833e5b6f82..f30782ef81b 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,129 @@ 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 + 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 8d2869bbec2..00000000000 --- a/mod/lti/tests/behat/addtype.feature +++ /dev/null @@ -1,56 +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 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 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 - 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/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..3158f086793 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 @@ -48,48 +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 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" - # 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/behat/managecoursetools.feature b/mod/lti/tests/behat/managecoursetools.feature index 572047c33b8..6a50146f973 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 | @@ -128,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" 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/generator/behat_mod_lti_generator.php b/mod/lti/tests/generator/behat_mod_lti_generator.php index b635e45aad4..ce48bc59f59 100644 --- a/mod/lti/tests/generator/behat_mod_lti_generator.php +++ b/mod/lti/tests/generator/behat_mod_lti_generator.php @@ -44,13 +44,55 @@ 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', '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; + } + + /** + * 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); + } } diff --git a/mod/lti/tests/generator/lib.php b/mod/lti/tests/generator/lib.php index ac037ce6e77..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); } @@ -88,28 +88,59 @@ 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 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. + $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' => (object) $type, 'config' => (object) $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); } /** * 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'])) { @@ -118,7 +149,28 @@ 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. - lti_add_type((object) $type, (object) $config); + + $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. + + // 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; + + // 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_load_type_if_cartridge($config); + lti_add_type(type: $type, config: $config); } } diff --git a/mod/lti/tests/lib_test.php b/mod/lti/tests/lib_test.php index 4b84a153b64..d8173017769 100644 --- a/mod/lti/tests/lib_test.php +++ b/mod/lti/tests/lib_test.php @@ -412,43 +412,44 @@ 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'). + // 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); - $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/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); + } +} diff --git a/mod/lti/tests/locallib_test.php b/mod/lti/tests/locallib_test.php index a704ad8f87c..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); } /** @@ -2248,4 +2264,118 @@ 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(0, $coursetooltypes); + $this->unassignUserCapability('mod/lti:addpreconfiguredinstance', (\core\context\course::instance($course->id))->id, + $teacherrole->id); + } } 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(); diff --git a/mod/lti/upgrade.txt b/mod/lti/upgrade.txt index 2b84c3f2887..b3d2abb0e8b 100644 --- a/mod/lti/upgrade.txt +++ b/mod/lti/upgrade.txt @@ -3,6 +3,11 @@ 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. +* 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;