Merge branch 'MDL-68800-lti-gbs-patch-fixes' of https://github.com/cengage/moodle
authorJake Dallimore <jake@moodle.com>
Thu, 4 Jun 2020 06:55:58 +0000 (14:55 +0800)
committerJake Dallimore <jake@moodle.com>
Thu, 4 Jun 2020 06:55:58 +0000 (14:55 +0800)
mod/lti/launch.php
mod/lti/locallib.php
mod/lti/service/gradebookservices/backup/moodle2/restore_ltiservice_gradebookservices_subplugin.class.php
mod/lti/service/gradebookservices/classes/local/resources/lineitem.php
mod/lti/service/gradebookservices/classes/local/resources/lineitems.php
mod/lti/service/gradebookservices/classes/local/service/gradebookservices.php
mod/lti/service/gradebookservices/tests/gradebookservices_test.php
mod/lti/view.php

index 6da25d5..3ae0440 100644 (file)
@@ -57,6 +57,9 @@ $cm = get_coursemodule_from_id('lti', $id, 0, false, MUST_EXIST);
 $lti = $DB->get_record('lti', array('id' => $cm->instance), '*', MUST_EXIST);
 
 $typeid = $lti->typeid;
+if (empty($typeid) && ($tool = lti_get_tool_by_url_match($lti->toolurl))) {
+    $typeid = $tool->id;
+}
 if ($typeid) {
     $config = lti_get_type_type_config($typeid);
     if ($config->lti_ltiversion === LTI_VERSION_1P3) {
index 95f5059..ff43403 100644 (file)
@@ -613,9 +613,9 @@ function lti_get_launch_data($instance, $nonce = '') {
 
     $launchcontainer = lti_get_launch_container($instance, $typeconfig);
     $returnurlparams = array('course' => $course->id,
-                             'launch_container' => $launchcontainer,
-                             'instanceid' => $instance->id,
-                             'sesskey' => sesskey());
+        'launch_container' => $launchcontainer,
+        'instanceid' => $instance->id,
+        'sesskey' => sesskey());
 
     // Add the return URL. We send the launch container along to help us avoid frames-within-frames when the user returns.
     $url = new \moodle_url('/mod/lti/return.php', $returnurlparams);
@@ -1185,7 +1185,7 @@ function lti_build_content_item_selection_request($id, $course, moodle_url $retu
         $services = lti_get_services();
         foreach ($services as $service) {
             $serviceparameters = $service->get_launch_parameters('ContentItemSelectionRequest',
-                    $course->id, $USER->id , $id);
+                $course->id, $USER->id , $id);
             foreach ($serviceparameters as $paramkey => $paramvalue) {
                 $requestparams['custom_' . $paramkey] = lti_parse_custom_parameter($toolproxy, $tool, $requestparams, $paramvalue,
                     $islti2);
index 7d39b59..b11d19e 100644 (file)
@@ -213,25 +213,24 @@ class restore_ltiservice_gradebookservices_subplugin extends restore_subplugin {
                 $DB->update_record('ltiservice_gradebookservices', $gbs);
             }
         }
-        // Pre 3.9 backups did not include a gradebookservices record. We create one here if idnumber is set.
-        $gradeitems = $DB->get_records('grade_items', array('itemtype' => 'mod', 'itemmodule' => 'lti', 'courseid' => $courseid));
-        foreach ($gradeitems as $gi) {
-            if (isset($gi->idnumber) && !empty(trim($gi->idnumber))) {
-                $gbs = $DB->get_records('ltiservice_gradebookservices', ['gradeitemid' => $gi->id]);
-                if (empty($gbs)  && !empty($gi->iteminstance)) {
-                    // We did not find an entry for an LTI grade item with an idnumber, so let's create a gbs entry.
-                    if ($instance = $DB->get_record('lti', array('id' => $gi->iteminstance))) {
-                        if ($tool = lti_get_instance_type($instance)) {
-                            $DB->insert_record('ltiservice_gradebookservices', (object) array(
-                                'gradeitemid' => $gi->id,
-                                'courseid' => $courseid,
-                                'toolproxyid' => $tool->toolproxyid,
-                                'ltilinkid' => $gi->iteminstance,
-                                'typeid' => $tool->id,
-                                'baseurl' => $tool->baseurl,
-                                'resourceid' => $gi->idnumber
-                            ));
-                        }
+        // Pre 3.9 backups did not include a gradebookservices record. Adding one here if missing for the restored instance.
+        $gi = $DB->get_record('grade_items', array('itemtype' => 'mod', 'itemmodule' => 'lti', 'courseid' => $courseid,
+            'iteminstance' => $this->task->get_activityid()));
+        if ($gi) {
+            $gbs = $DB->get_records('ltiservice_gradebookservices', ['gradeitemid' => $gi->id]);
+            if (empty($gbs)) {
+                // The currently restored LTI link has a grade item but no gbs, so let's create a gbs entry.
+                if ($instance = $DB->get_record('lti', array('id' => $gi->iteminstance))) {
+                    if ($tool = lti_get_instance_type($instance)) {
+                        $DB->insert_record('ltiservice_gradebookservices', (object) array(
+                            'gradeitemid' => $gi->id,
+                            'courseid' => $courseid,
+                            'toolproxyid' => $tool->toolproxyid,
+                            'ltilinkid' => $gi->iteminstance,
+                            'typeid' => $tool->id,
+                            'baseurl' => $tool->baseurl,
+                            'resourceid' => $gi->idnumber
+                        ));
                     }
                 }
             }
index 8595853..7427ca4 100644 (file)
@@ -127,7 +127,7 @@ class lineitem extends resource_base {
 
         $response->set_content_type($this->formats[0]);
         $lineitem = gradebookservices::item_for_json($item, substr(parent::get_endpoint(),
-                0, strrpos(parent::get_endpoint(), "/", -10)), $typeid);
+            0, strrpos(parent::get_endpoint(), "/", -10)), $typeid);
         $response->set_body(json_encode($lineitem));
 
     }
index 86deee9..5fd278c 100644 (file)
@@ -89,7 +89,7 @@ class lineitems extends resource_base {
             $typeid = $this->get_service()->get_type()->id;
             if (empty($contextid) || !($container ^ ($response->get_request_method() === self::HTTP_POST)) ||
                     (!empty($contenttype) && !in_array($contenttype, $this->formats))) {
-                    throw new \Exception('No context or unsupported content type', 400);
+                throw new \Exception('No context or unsupported content type', 400);
             }
             if (!($course = $DB->get_record('course', array('id' => $contextid), 'id', IGNORE_MISSING))) {
                 throw new \Exception("Not Found: Course {$contextid} doesn't exist", 404);
@@ -267,16 +267,8 @@ class lineitems extends resource_base {
             $baseurl = lti_get_type_type_config($typeid)->lti_toolurl;
         }
         $gradebookservices = new gradebookservices();
-        $id = $gradebookservices->add_standalone_lineitem($contextid,
-                                                         $json->label,
-                                                         $max,
-                                                         $baseurl,
-                                                         $ltilinkid,
-                                                         $resourceid,
-                                                         $tag,
-                                                         $typeid,
-                                                         $toolproxyid);
-
+        $id = $gradebookservices->add_standalone_lineitem($contextid, $json->label,
+            $max, $baseurl, $ltilinkid, $resourceid, $tag, $typeid, $toolproxyid);
         if (is_null($typeid)) {
             $json->id = parent::get_endpoint() . "/{$id}/lineitem";
         } else {
index 43039f4..9d35849 100644 (file)
@@ -155,7 +155,7 @@ class gradebookservices extends service_base {
         // Only inject parameters if the service is enabled for this tool.
         if (isset($this->get_typeconfig()['ltiservice_gradesynchronization'])) {
             if ($this->get_typeconfig()['ltiservice_gradesynchronization'] == self::GRADEBOOKSERVICES_READ ||
-                $this->get_typeconfig()['ltiservice_gradesynchronization'] == self::GRADEBOOKSERVICES_FULL) {
+                    $this->get_typeconfig()['ltiservice_gradesynchronization'] == self::GRADEBOOKSERVICES_FULL) {
                 // Check for used in context is only needed because there is no explicit site tool - course relation.
                 if ($this->is_allowed_in_context($typeid, $courseid)) {
                     $id = null;
@@ -235,12 +235,10 @@ class gradebookservices extends service_base {
                             array_push($lineitemstoreturn, $lineitem);
                         }
                     }
-                } else if (($lineitem->itemtype == 'mod'
-                             && $lineitem->itemmodule == 'lti'
-                             && !isset($resourceid)
-                             && !isset($tag)
-                             && (!isset($ltilinkid) || (isset($ltilinkid)
-                             && $lineitem->iteminstance == $ltilinkid)))) {
+                } else if (($lineitem->itemtype == 'mod' && $lineitem->itemmodule == 'lti'
+                        && !isset($resourceid) && !isset($tag)
+                        && (!isset($ltilinkid) || (isset($ltilinkid)
+                        && $lineitem->iteminstance == $ltilinkid)))) {
                     // We will need to check if the activity related belongs to our tool proxy.
                     $ltiactivity = $DB->get_record('lti', array('id' => $lineitem->iteminstance));
                     if (($ltiactivity) && (isset($ltiactivity->typeid))) {
@@ -343,15 +341,9 @@ class gradebookservices extends service_base {
      *
      * @return int id of the created gradeitem
      */
-    public function add_standalone_lineitem(string $courseid,
-                                            string $label,
-                                            float $maximumscore,
-                                            string $baseurl,
-                                            ?int $ltilinkid,
-                                            ?string $resourceid,
-                                            ?string $tag,
-                                            int $typeid,
-                                            int $toolproxyid = null) : int {
+    public function add_standalone_lineitem(string $courseid, string $label, float $maximumscore,
+            string $baseurl, ?int $ltilinkid, ?string $resourceid, ?string $tag, int $typeid,
+            int $toolproxyid = null) : int {
         global $DB;
         $params = array();
         $params['itemname'] = $label;
@@ -428,7 +420,7 @@ class gradebookservices extends service_base {
         }
         $feedbackformat = FORMAT_MOODLE;
         $feedback = null;
-        if (isset($score->comment) && !empty($score->comment)) {
+        if (!empty($score->comment)) {
             $feedback = $score->comment;
             $feedbackformat = FORMAT_PLAIN;
         }
@@ -447,9 +439,8 @@ class gradebookservices extends service_base {
             $grade->feedback = $feedback;
             $grade->rawgrade = $finalgrade;
             $status = grade_update($source, $gradeitem->courseid,
-                         $gradeitem->itemtype, $gradeitem->itemmodule,
-                         $gradeitem->iteminstance, $gradeitem->itemnumber,
-                         $grade);
+                $gradeitem->itemtype, $gradeitem->itemmodule,
+                $gradeitem->iteminstance, $gradeitem->itemnumber, $grade);
 
             $result = ($status == GRADE_UPDATE_OK);
         }
@@ -621,9 +612,7 @@ class gradebookservices extends service_base {
      * @param string|null $tag The tag to apply to the lineitem. If empty string which will be stored as null.
      *
      */
-    public static function update_coupled_gradebookservices(object $ltiinstance,
-                                                            ?string $resourceid,
-                                                            ?string $tag) : void {
+    public static function update_coupled_gradebookservices(object $ltiinstance, ?string $resourceid, ?string $tag) : void {
         global $DB;
 
         if ($ltiinstance && $ltiinstance->typeid) {
index d97c233..da713e9 100644 (file)
@@ -160,12 +160,8 @@ class mod_lti_gradebookservices_testcase extends advanced_testcase {
      * @param string|null $resourceid resourceid the line item should have
      * @param string|null $tag tag the line item should have
      */
-    private function assert_lineitems(object $course,
-                                      int $typeid,
-                                      string $label,
-                                      ?object $ltiinstance,
-                                      ?string $resourceid,
-                                      ?string $tag) : void {
+    private function assert_lineitems(object $course, int $typeid,
+            string $label, ?object $ltiinstance, ?string $resourceid, ?string $tag) : void {
         $gbservice = new gradebookservices();
         $gradeitems = $gbservice->get_lineitems($course->id, null, null, null, null, null, $typeid);
 
@@ -211,11 +207,11 @@ class mod_lti_gradebookservices_testcase extends advanced_testcase {
     private function create_graded_lti(int $typeid, object $course, ?string $resourceid, ?string $tag) : object {
 
         $lti = ['course' => $course->id,
-                'typeid' => $typeid,
-                'instructorchoiceacceptgrades' => LTI_SETTING_ALWAYS,
-                'grade' => 10,
-                'lineitemresourceid' => $resourceid,
-                'lineitemtag' => $tag];
+            'typeid' => $typeid,
+            'instructorchoiceacceptgrades' => LTI_SETTING_ALWAYS,
+            'grade' => 10,
+            'lineitemresourceid' => $resourceid,
+            'lineitemtag' => $tag];
 
         return $this->getDataGenerator()->create_module('lti', $lti, array());
     }
@@ -231,8 +227,8 @@ class mod_lti_gradebookservices_testcase extends advanced_testcase {
     private function create_notgraded_lti(int $typeid, object $course) : object {
 
         $lti = ['course' => $course->id,
-                'typeid' => $typeid,
-                'instructorchoiceacceptgrades' => LTI_SETTING_NEVER];
+            'typeid' => $typeid,
+            'instructorchoiceacceptgrades' => LTI_SETTING_NEVER];
 
         return $this->getDataGenerator()->create_module('lti', $lti, array());
     }
@@ -247,11 +243,8 @@ class mod_lti_gradebookservices_testcase extends advanced_testcase {
      * @param int|null $ltiinstanceid Id of the LTI instance the standalone line item will be related to.
      *
      */
-    private function create_standalone_lineitem(int $courseid,
-                                                int $typeid,
-                                                ?string $resourceid,
-                                                ?string $tag,
-                                                int $ltiinstanceid = null) : void {
+    private function create_standalone_lineitem(int $courseid, int $typeid, ?string $resourceid,
+            ?string $tag, int $ltiinstanceid = null) : void {
         $gbservice = new gradebookservices();
         $gbservice->add_standalone_lineitem($courseid,
             "manualtest",
index 06bc452..5bdceb5 100644 (file)
@@ -66,10 +66,12 @@ if ($l) {  // Two ways to specify the module.
 
 $course = $DB->get_record('course', array('id' => $cm->course), '*', MUST_EXIST);
 
-if (!empty($lti->typeid)) {
-    $toolconfig = lti_get_type_config($lti->typeid);
-} else if ($tool = lti_get_tool_by_url_match($lti->toolurl)) {
-    $toolconfig = lti_get_type_config($tool->id);
+$typeid = $lti->typeid;
+if (empty($typeid) && ($tool = lti_get_tool_by_url_match($lti->toolurl))) {
+    $typeid = $tool->id;
+}
+if ($typeid) {
+    $toolconfig = lti_get_type_config($typeid);
 } else {
     $toolconfig = array();
 }
@@ -116,7 +118,6 @@ if ($lti->showdescriptionlaunch && $lti->intro) {
     echo $OUTPUT->box(format_module_intro('lti', $lti, $cm->id), 'generalbox description', 'intro');
 }
 
-$typeid = $lti->typeid;
 if ($typeid) {
     $config = lti_get_type_type_config($typeid);
 } else {