Moodle
  1. Moodle
  2. MDL-32614

Moodle LTI tool breaks during restore

    Details

    • Testing Instructions:
      Hide

      Important note: As far as all the URLs introduced are fake, none of the activities will deploy any tool at all. That's not the goal for this testing.

      Testing this requires having one sitetool and one coursetool defined. Let's create them:

      A) As admin, go to admin->plugins->activity modules->external tool

      B) Click "Add external tool configuration"

      C) Enter:

      • Tool name: sitetool
      • Tool base URL: http://sitetool.com
      • Check "Show tool type when creating tool instances"
      • Save changes.

      (with this, the site tool is created, it doesn't matter the url is fake)

      D) Create a new course, enable edition and add one External tool activity.

      E) Name of the activity: "A) one automatic sitetool"

      F) Click the + (plus) icon in "External tool type" and fill:

      (with this, the course tool is created, it doesn't matter the url is fake)

      Let's begin testing and creating the required activities:

      1) TEST: We are back to the "Adding a new External Tool" page and the "External tool type" shows: Automatic, based in the URL, sitetool and coursetool (3 options). Name should continue being: "A) one automatic sitetool"

      2) Pick "Automatic, based in the URL" and introduce http://sitetool.com in the "Launch URL" field. And press tab.

      3) TEST: One text with "Using tool configuration: sitetool" is shown.

      4) Save and return to course.

      5) Create a new external tool activity with information:

      • Name: "B) one automatic coursetool"
      • In the dropdown: "Automatic, based in the URL"
      • Launch URL: http://coursetool.com
      • And press tab

      6) TEST: One text with "Using tool configuration: coursetool" is shown.

      7) Save and return to course.

      8) Create a new external tool activity with information:

      • Name: "C) one automatic selftool"
      • In the dropdown: "Automatic, based in the URL"
      • Launch URL: http://selftool.com
      • And press tab

      9) TEST: One text with "Tool configuration not found for this " is shown.

      10) Save and return to course.

      11) Create a new external tool activity with information:

      • Name: "D) one picked sitetool"
      • In the dropdown: "sitetool"

      12) Save and return to course

      13) Create a new external tool activity with information:

      • Name: "E) one picked coursetool"
      • In the dropdown: "coursetool"

      14) Save and return to course

      15) TEST: At this point the course has 5 LTI activities, named, ABCDE

      16) Backup the course (default settings)

      17) Restore the course (default settings) into new course in the same site.

      18) TEST: 5 activities are restored (ABCDE)

      19) TEST: A, B and C retain the introduced settings (name, external tool type and url are the same than the ones in the original course.

      20) TEST: The external tool type drop down shows only 2 options now (Automatic and sitetool), from 3 in the original course. It's one of the things to be fixed by MDL-34161.

      21) TEST: D and E now show "Automatic" in the drop down and there is no URL (lost). Note this is one of the things to be fixed by MDL-34161 too.

      22) Restore the course into different site (not having any site tool defined).

      23) TEST: A, B and C retain the introduced settings (name, external tool type and url are the same than the ones in the original course.

      24) TEST: The external tool type drop down shows only 1 option now (Automatic), from 3 in the original course. It's one of the things to be fixed by MDL-34161.

      25) FINAL TEST: Look to original course and verify that the 5 activities (ABCDE) retain the original settings as specified when created, so the restore operation has not affected them at all in any case.

      26) Tired? NP, this is the END, thanks!

      Show
      Important note: As far as all the URLs introduced are fake, none of the activities will deploy any tool at all. That's not the goal for this testing. Testing this requires having one sitetool and one coursetool defined. Let's create them: A) As admin, go to admin->plugins->activity modules->external tool B) Click "Add external tool configuration" C) Enter: Tool name: sitetool Tool base URL: http://sitetool.com Check "Show tool type when creating tool instances" Save changes. (with this, the site tool is created, it doesn't matter the url is fake) D) Create a new course, enable edition and add one External tool activity. E) Name of the activity: "A) one automatic sitetool" F) Click the + (plus) icon in "External tool type" and fill: Tool name: coursetool Tool base URL: http://coursetool.com Save changes. (with this, the course tool is created, it doesn't matter the url is fake) Let's begin testing and creating the required activities: 1) TEST: We are back to the "Adding a new External Tool" page and the "External tool type" shows: Automatic, based in the URL, sitetool and coursetool (3 options). Name should continue being: "A) one automatic sitetool" 2) Pick "Automatic, based in the URL" and introduce http://sitetool.com in the "Launch URL" field. And press tab. 3) TEST: One text with "Using tool configuration: sitetool" is shown. 4) Save and return to course. 5) Create a new external tool activity with information: Name: "B) one automatic coursetool" In the dropdown: "Automatic, based in the URL" Launch URL: http://coursetool.com And press tab 6) TEST: One text with "Using tool configuration: coursetool" is shown. 7) Save and return to course. 8) Create a new external tool activity with information: Name: "C) one automatic selftool" In the dropdown: "Automatic, based in the URL" Launch URL: http://selftool.com And press tab 9) TEST: One text with "Tool configuration not found for this " is shown. 10) Save and return to course. 11) Create a new external tool activity with information: Name: "D) one picked sitetool" In the dropdown: "sitetool" 12) Save and return to course 13) Create a new external tool activity with information: Name: "E) one picked coursetool" In the dropdown: "coursetool" 14) Save and return to course 15) TEST: At this point the course has 5 LTI activities, named, ABCDE 16) Backup the course (default settings) 17) Restore the course (default settings) into new course in the same site. 18) TEST: 5 activities are restored (ABCDE) 19) TEST: A, B and C retain the introduced settings (name, external tool type and url are the same than the ones in the original course. 20) TEST: The external tool type drop down shows only 2 options now (Automatic and sitetool), from 3 in the original course. It's one of the things to be fixed by MDL-34161 . 21) TEST: D and E now show "Automatic" in the drop down and there is no URL (lost). Note this is one of the things to be fixed by MDL-34161 too. 22) Restore the course into different site (not having any site tool defined). 23) TEST: A, B and C retain the introduced settings (name, external tool type and url are the same than the ones in the original course. 24) TEST: The external tool type drop down shows only 1 option now (Automatic), from 3 in the original course. It's one of the things to be fixed by MDL-34161 . 25) FINAL TEST: Look to original course and verify that the 5 activities (ABCDE) retain the original settings as specified when created, so the restore operation has not affected them at all in any case. 26) Tired? NP, this is the END, thanks!
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      39541

      Description

      After having restored a course containing an instance of mod_lti, the resulting LTI is broken.

      It seems that all restored database records in the mdl_lti table table have typeid = 0. Looking into the code of mod/lti/backup/moodle2/restore_lti_stepslib.php , I see around line 88 that after the restore process we have an IF statement that always evaluates to true and modifies the $basiclti->typeid to 0.

        Issue Links

          Activity

          Hide
          Nicolas Dunand added a comment -

          Actually this IF statement check all lti records after each restore to see if lti.toolurl (which doesn't exist) equals lti_types_config.toolurl (which doesn't exist either) for lti.typeid = lti_types_config.typeid .

          This always is false, so all lti records end up with lti.typeid = 0.

          I don't know what was the purpose of this - I assume it was some sort of consistency check but It doesn't seem to work any more.

          Show
          Nicolas Dunand added a comment - Actually this IF statement check all lti records after each restore to see if lti.toolurl (which doesn't exist) equals lti_types_config.toolurl (which doesn't exist either) for lti.typeid = lti_types_config.typeid . This always is false, so all lti records end up with lti.typeid = 0. I don't know what was the purpose of this - I assume it was some sort of consistency check but It doesn't seem to work any more.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this.

          Show
          Michael de Raadt added a comment - Thanks for reporting this.
          Hide
          Chris Scribner added a comment -

          I created a course with some External Tool links in it, performed a backup, then restored the backup. All the External Tool links were still in tact, and I did not observe any issues.

          It is not an error if lti.typeid = 0, as that just means "match based on the Launch URL" when a launch occurs.

          One thing to note is that backups do not contain the key or secret for security reasons. I don't know if this could be related to the issue you observed or not.

          Feel free to attach a course backup which demonstrates the problem.

          Chris

          Show
          Chris Scribner added a comment - I created a course with some External Tool links in it, performed a backup, then restored the backup. All the External Tool links were still in tact, and I did not observe any issues. It is not an error if lti.typeid = 0, as that just means "match based on the Launch URL" when a launch occurs. One thing to note is that backups do not contain the key or secret for security reasons. I don't know if this could be related to the issue you observed or not. Feel free to attach a course backup which demonstrates the problem. Chris
          Hide
          Mike Buchanon added a comment -

          I think this has to do with system level external tool definitions vs course level definitions. If you don't define a tool URL at the course level and only use the tool type id drop down, then the restore seems to fail. Worse yet, when it fails, it happily runs through the LTI table and resets the tool typeid to 0 for all entries where there's no URL (basically anything using a system level definition). I haven't had a chance yet to dive into it in great detail, but, this screencast shows what I'm seeing: http://screencast.com/t/A3aGlJXrRM. I'm sure Chris can fix it as he's one of the smartest guys I know I'll keep looking at it too. If I can be of any help, please let me know!

          Show
          Mike Buchanon added a comment - I think this has to do with system level external tool definitions vs course level definitions. If you don't define a tool URL at the course level and only use the tool type id drop down, then the restore seems to fail. Worse yet, when it fails, it happily runs through the LTI table and resets the tool typeid to 0 for all entries where there's no URL (basically anything using a system level definition). I haven't had a chance yet to dive into it in great detail, but, this screencast shows what I'm seeing: http://screencast.com/t/A3aGlJXrRM . I'm sure Chris can fix it as he's one of the smartest guys I know I'll keep looking at it too. If I can be of any help, please let me know!
          Hide
          Chris Scribner added a comment -

          Mike,

          Thanks for posting the video. I looked into the issue, and I was shocked to see what was occurring. There was some code left in from the original open source BLTI plugin that I missed which was resetting tool types for all LTI links on the system (as you observed)! I couldn't figure out why that code was there in the first place, so I just took it out and things went fine in my testing.

          This brought to light another bug. When migrating content between Moodle systems, the lti items will point to the wrong tool configuration on the other site. The tool config table uses auto-incrementing IDs, so it's likely a tool config will exist in the target system with the same ID as the original system, but associated with a different tool provider.

          Chris

          Show
          Chris Scribner added a comment - Mike, Thanks for posting the video. I looked into the issue, and I was shocked to see what was occurring. There was some code left in from the original open source BLTI plugin that I missed which was resetting tool types for all LTI links on the system (as you observed)! I couldn't figure out why that code was there in the first place, so I just took it out and things went fine in my testing. This brought to light another bug. When migrating content between Moodle systems, the lti items will point to the wrong tool configuration on the other site. The tool config table uses auto-incrementing IDs, so it's likely a tool config will exist in the target system with the same ID as the original system, but associated with a different tool provider. Chris
          Hide
          Chris Scribner added a comment -

          Fix is here: https://github.com/scriby/moodle/tree/MDL-32614

          Testing instructions:

          1. Make sure you have at least one LTI tool provider that has the setting "Show tool type when creating tool instances" selected
          2. Create a new course
          3. Create an External Tool activity, and fill out the required fields. Select a tool provider from the "External tool type" list
          4. Save the item
          5. Backup the course
          6. Restore the course
          7. Edit the LTI item in the new course and verify the "External tool type" is still selected
          8. Edit the LTI item from the original course and verify the "External tool type" is still selected

          Show
          Chris Scribner added a comment - Fix is here: https://github.com/scriby/moodle/tree/MDL-32614 Testing instructions: 1. Make sure you have at least one LTI tool provider that has the setting "Show tool type when creating tool instances" selected 2. Create a new course 3. Create an External Tool activity, and fill out the required fields. Select a tool provider from the "External tool type" list 4. Save the item 5. Backup the course 6. Restore the course 7. Edit the LTI item in the new course and verify the "External tool type" is still selected 8. Edit the LTI item from the original course and verify the "External tool type" is still selected
          Hide
          Chris Scribner added a comment -

          Dan,

          Have time to peer review?

          Thanks,

          Chris

          Show
          Chris Scribner added a comment - Dan, Have time to peer review? Thanks, Chris
          Hide
          Dan Poltawski added a comment -

          Thanks Chris for the patch, i'm submitting this for integration.

          Show
          Dan Poltawski added a comment - Thanks Chris for the patch, i'm submitting this for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Uhm, Chris..

          I've been reviewing this... and really think it's missing important functionality, both currently and also with your patch applied.

          I think there are basically 3 cases to handle on backup & restore:

          A), aka, "selfcontained") All the information about the provider is present at module level (lti table), so the typeid = 0

          B), aka, "coursetool") There is some information defined at course-tool level (lti_types table with course != 0) and one record in the module (lti table) matching the tool by course, typeid and url.

          C), aka, "sitetool") There is some information defined at site (system) level (lti_types table with course == 0) and one record in the module (lti table) matching the tool by typeid and url.

          Said that, there are various alternatives to backup and restore information, depending of which of the above cases we can to cover and which ones we want to fallback on restore to simpler ones.

          Right now, with your code, only case A) is supported, backup does not include any information about lti_types, nor restore is able to match one tool to any existing B) or C) tools. With current code (before your patch), there was one "attempt" to perform the match, although it was incorrect. With the patch, no attempt to match is performed anymore.

          So, let me explain how I see the whole thing, in a serie of points, and let's discuss if that leads to better results:

          1) In backup, we will always include all the selected "lti" records and also, the B) "lti_types", defined for the course and obviously, keeping any private information out from the equation. Not nested. First the lti_types and later the ltis.

          2) In restore we will restore all the lti_types present in 1) (remember they are coursetool ones only, not sitetools). We'll do that conditionally, so if in the target course there is already one type with the same url we'll reuse it later and, if there is not matching type, and the user has perms... a new type will be created. Note reuse means create one mapping for each restore lty_type (oldid, newid).

          3) Once we have processed all lti_types in previous point, we'll begin restoring the lti records themselves:

          3.1) If lti has typeid == 0:

          3.1.1 (Case A) It is a self-contained lti, np. create lti and done. STOP. (note this is, more or less, where we are right now, but without the 3.1 condition).

          3.2) If lti has typeid != 0:

          3.3 Look in the mappings for one "lti_types" with matching typeid.

          3.4 If found (and the user can point to coursetools, I don't remember if there is a perm for this):

          3.4.1 (Case B) lti has type pointing to coursetool. Remap typeid, create lti and done. STOP.

          3.5 If not found, it means the lti is pointing to some tool not present in backup or not created in restore so:

          3.6 Look if there is any sitetool (course = 0) matching the url.

          3.7 If found (and the user can point to sitetools, I don't remember if there is a perm for this):

          3.7.1 (Case C) lti has type pointing to sitetool. Remap typeid, create lti and done. STOP.

          3.8 If not found, it means the lti is pointing to some tool we have not been able to match so:

          3.9 (Fallback) convert to "selfcontained" case A). Set typeid = 0, create lti and done. STOP.

          And that is, lol.

          Basically it's about go, based on typeid, discerning which of the cases we must apply on restore, and offering one fallback mechanishm to case A) if B) and C) matches are not found. But to be able to achieve that, it's important to perform points 1) and 2) for better matching of case B).

          Tell me what do you think...in the mean time, I'm reopening this...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Uhm, Chris.. I've been reviewing this... and really think it's missing important functionality, both currently and also with your patch applied. I think there are basically 3 cases to handle on backup & restore: A), aka, "selfcontained") All the information about the provider is present at module level (lti table), so the typeid = 0 B), aka, "coursetool") There is some information defined at course-tool level (lti_types table with course != 0) and one record in the module (lti table) matching the tool by course, typeid and url. C), aka, "sitetool") There is some information defined at site (system) level (lti_types table with course == 0) and one record in the module (lti table) matching the tool by typeid and url. Said that, there are various alternatives to backup and restore information, depending of which of the above cases we can to cover and which ones we want to fallback on restore to simpler ones. Right now, with your code, only case A) is supported, backup does not include any information about lti_types, nor restore is able to match one tool to any existing B) or C) tools. With current code (before your patch), there was one "attempt" to perform the match, although it was incorrect. With the patch, no attempt to match is performed anymore. So, let me explain how I see the whole thing, in a serie of points, and let's discuss if that leads to better results: 1) In backup, we will always include all the selected "lti" records and also, the B) "lti_types", defined for the course and obviously, keeping any private information out from the equation. Not nested. First the lti_types and later the ltis. 2) In restore we will restore all the lti_types present in 1) (remember they are coursetool ones only, not sitetools). We'll do that conditionally, so if in the target course there is already one type with the same url we'll reuse it later and, if there is not matching type, and the user has perms... a new type will be created. Note reuse means create one mapping for each restore lty_type (oldid, newid). 3) Once we have processed all lti_types in previous point, we'll begin restoring the lti records themselves: 3.1) If lti has typeid == 0: 3.1.1 (Case A) It is a self-contained lti, np. create lti and done. STOP. (note this is, more or less, where we are right now, but without the 3.1 condition). 3.2) If lti has typeid != 0: 3.3 Look in the mappings for one "lti_types" with matching typeid. 3.4 If found (and the user can point to coursetools, I don't remember if there is a perm for this): 3.4.1 (Case B) lti has type pointing to coursetool. Remap typeid, create lti and done. STOP. 3.5 If not found, it means the lti is pointing to some tool not present in backup or not created in restore so: 3.6 Look if there is any sitetool (course = 0) matching the url. 3.7 If found (and the user can point to sitetools, I don't remember if there is a perm for this): 3.7.1 (Case C) lti has type pointing to sitetool. Remap typeid, create lti and done. STOP. 3.8 If not found, it means the lti is pointing to some tool we have not been able to match so: 3.9 (Fallback) convert to "selfcontained" case A). Set typeid = 0, create lti and done. STOP. And that is, lol. Basically it's about go, based on typeid, discerning which of the cases we must apply on restore, and offering one fallback mechanishm to case A) if B) and C) matches are not found. But to be able to achieve that, it's important to perform points 1) and 2) for better matching of case B). Tell me what do you think...in the mean time, I'm reopening this...ciao
          Hide
          Chris Scribner added a comment -

          I recommend taking this change as is and creating. New issue for further import/export work. It's a bad bug and a fix need to get in as soon as possible.

          I agree with the approach you laid out. The one thing we may want add is making it work between Moodle sites. Because the Ids are auto-incremented it's likely we'll match the wrong tool config on a different Moodle instance.

          I'm on vacation now and don't have time in my schedule to undertake this change anytime soon.

          Show
          Chris Scribner added a comment - I recommend taking this change as is and creating. New issue for further import/export work. It's a bad bug and a fix need to get in as soon as possible. I agree with the approach you laid out. The one thing we may want add is making it work between Moodle sites. Because the Ids are auto-incremented it's likely we'll match the wrong tool config on a different Moodle instance. I'm on vacation now and don't have time in my schedule to undertake this change anytime soon.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (Hi Chris, note the plan above performs sitetools matching exclusively by url, so if the tool is available in target site... it will be reused properly. Else fallback to selfcontained will happen).

          About the plan for this... I'm going to delete that code, that really can be breaking other ltis, amend code a bit to restore all ltis as selfcontained (typeid ALWAYS = 0) and then create another issue for the big refactor to perform later.

          Sounds ok? Ciao and enjoy!

          Show
          Eloy Lafuente (stronk7) added a comment - (Hi Chris, note the plan above performs sitetools matching exclusively by url, so if the tool is available in target site... it will be reused properly. Else fallback to selfcontained will happen). About the plan for this... I'm going to delete that code, that really can be breaking other ltis, amend code a bit to restore all ltis as selfcontained (typeid ALWAYS = 0) and then create another issue for the big refactor to perform later. Sounds ok? Ciao and enjoy!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I've created MDL-34161 about the fixes needed to get backup and restore supporting everything, FYI.

          Now I'm adding one commit here to, always, restore the tools as "selfcontained" (Case A). Never trying to look for course or site tools. As said, note this is one interim solution.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I've created MDL-34161 about the fixes needed to get backup and restore supporting everything, FYI. Now I'm adding one commit here to, always, restore the tools as "selfcontained" (Case A). Never trying to look for course or site tools. As said, note this is one interim solution. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sending to integration. 2 commits:

          • The original one from Chris, deleting the restore portion where mess was happening. So other ltis won't be modified anymore.
          • One extra commit from me, in order to change all the activities to be "self-contained" aka, not using course or site tools at all. Only MDL-34161 will enable that, we need more info in backup to fix that.

          Reworked testing instructions, they are long but, I hope, simple and quick to follow.

          At the end they verify that the original activities are not borked anymore by the restore operations and also that any type of tool with URL introduced will continue working, but those "picked" from the dropdown of site/course tools will lose their associations. Something to be fixed by MDL-34161.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sending to integration. 2 commits: The original one from Chris, deleting the restore portion where mess was happening. So other ltis won't be modified anymore. One extra commit from me, in order to change all the activities to be "self-contained" aka, not using course or site tools at all. Only MDL-34161 will enable that, we need more info in backup to fix that. Reworked testing instructions, they are long but, I hope, simple and quick to follow. At the end they verify that the original activities are not borked anymore by the restore operations and also that any type of tool with URL introduced will continue working, but those "picked" from the dropdown of site/course tools will lose their associations. Something to be fixed by MDL-34161 . Ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, the changes here ended up making sense after much reading and have been integrated now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks guys, the changes here ended up making sense after much reading and have been integrated now. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          Done! Passed on 2.2, 2.3 and master! Yeepee!

          Show
          Frédéric Massart added a comment - Done! Passed on 2.2, 2.3 and master! Yeepee!
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.
          Hide
          Itamar Tzadok added a comment -

          On 2.2.4 the problem still exists when restoring into a course an lti activity from the front page.

          • Opening the activity shows "Missing oauth_consumer_key in request".
          • In the tool form the 'external tool type' setting loses its value.
          • The 'using tool configuration ...' note of the launch url points to the wrong tool configuration.
          • Selecting the correct tool type pops up a message that the tool configuration cannot be edited.
          • Saving the form throws "Found more than one record in fetch".

          Perhaps this issue should be reopen.

          Show
          Itamar Tzadok added a comment - On 2.2.4 the problem still exists when restoring into a course an lti activity from the front page. Opening the activity shows "Missing oauth_consumer_key in request". In the tool form the 'external tool type' setting loses its value. The 'using tool configuration ...' note of the launch url points to the wrong tool configuration. Selecting the correct tool type pops up a message that the tool configuration cannot be edited. Saving the form throws "Found more than one record in fetch". Perhaps this issue should be reopen.
          Hide
          Dan Poltawski added a comment -

          Hi Itamar,

          We can't reopen already integrated issues, if you believe there is still an issue please can you create a new issue and link /point to this issue.

          thanks,
          Dan

          Show
          Dan Poltawski added a comment - Hi Itamar, We can't reopen already integrated issues, if you believe there is still an issue please can you create a new issue and link /point to this issue. thanks, Dan

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: