Moodle
  1. Moodle
  2. MDL-36224

External Tool - Add support for pluggable extensions to add new services

    Details

    • Testing Instructions:
      Hide

      Test 1

      Go to site admin > plugins > activity modules > LTI and click "Add external tool configuration"

      base URL: https://www.edu-apps.org
      Shared secret and consumer secret can be left blank.

      Go to a course and add an external tool.

      Launch URL: https://www.edu-apps.org/tool_redirect?id=khan_academy

      Viewing the activity should result in a khan academy video being displayed.

      Test 2

      1. Download dummy.zip attachment from this ticket
      2. Unpack to wwwroot/mod/lti/source
      3. Navigate to course, turn editing on
      4. Verify that there are 2 external tools:
        1. General tool
        2. Dummy LTI source
      Show
      Test 1 Go to site admin > plugins > activity modules > LTI and click "Add external tool configuration" base URL: https://www.edu-apps.org Shared secret and consumer secret can be left blank. Go to a course and add an external tool. Launch URL: https://www.edu-apps.org/tool_redirect?id=khan_academy Viewing the activity should result in a khan academy video being displayed. Test 2 Download dummy.zip attachment from this ticket Unpack to wwwroot/mod/lti/source Navigate to course, turn editing on Verify that there are 2 external tools: General tool Dummy LTI source
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36224-master
    • Rank:
      94

      Description

      This change is necessary to support integration of the xpLor repository with Moodle.

      It adds a "sources" directory under mod/lti that can contain plugins to extend LTI messages that can be received.

        Issue Links

          Activity

          Hide
          Chris Scribner added a comment -

          I'm not really sure which version of Moodle this should go into. I made the changes against 2.3 stable, but I think they should merge cleanly with master as well.

          Show
          Chris Scribner added a comment - I'm not really sure which version of Moodle this should go into. I made the changes against 2.3 stable, but I think they should merge cleanly with master as well.
          Show
          Chris Scribner added a comment - https://github.com/scriby/moodle/commit/ddba72261f888600a3d9a4af806d1cdb773782e0 https://github.com/scriby/moodle/commit/f5dc0b00ed308ab3016d4e74c10b63c223cfe8db
          Hide
          Dan Poltawski added a comment -

          Assigning this to the 'peer review pool', as i'm full up with integration right now.

          Show
          Dan Poltawski added a comment - Assigning this to the 'peer review pool', as i'm full up with integration right now.
          Hide
          Dan Poltawski added a comment -

          Hi Chris,

          Sorry, we're awful at peer reviews timescales, clearly.

          Could you explain a bit about this change? Its not clear to me why you are adding the subplugin support.

          Show
          Dan Poltawski added a comment - Hi Chris, Sorry, we're awful at peer reviews timescales, clearly. Could you explain a bit about this change? Its not clear to me why you are adding the subplugin support.
          Hide
          Mark Nielsen added a comment -

          This change allows for support for subplugins that can extend LTI messages for deeper integrations with LTI providers. Note, this does not include any such plugin, just support for such a use case.

          Eventually, we will be releasing a subplugin that'll provide a richer integration with xpLor.

          Show
          Mark Nielsen added a comment - This change allows for support for subplugins that can extend LTI messages for deeper integrations with LTI providers. Note, this does not include any such plugin, just support for such a use case. Eventually, we will be releasing a subplugin that'll provide a richer integration with xpLor.
          Hide
          Martin Dougiamas added a comment -

          Can you guys check this and make sure it will merge cleanly with master?

          If we are going to rush it into 2.5 (after code freeze) it has to be absolutely perfect

          Show
          Martin Dougiamas added a comment - Can you guys check this and make sure it will merge cleanly with master? If we are going to rush it into 2.5 (after code freeze) it has to be absolutely perfect
          Hide
          Mark Nielsen added a comment -

          Update branches to based off of master. Didn't run into any conflicts.

          Show
          Mark Nielsen added a comment - Update branches to based off of master. Didn't run into any conflicts.
          Hide
          Dan Poltawski added a comment -

          (We suck)

          Please can you give testing instructions?

          Show
          Dan Poltawski added a comment - (We suck) Please can you give testing instructions?
          Hide
          Kris Stokking added a comment -

          Hey Dan, many thanks for your help with this late integration! Before I draft up the testing instructions I want to make sure they match what you might expect. The new code allows for LTI sub plugins to hook into callbacks in two places:

          1. When creating a new instance of the external tool. For example, a sub plugin could add a new option in the activity chooser. When selected, it could launch out to an external interface directly rather than requiring the content creator to plug in the launch URL and other complicated settings. This will make it much easier to create new instances of the same tool type.
          2. Sub plugins can hook into custom services directly, rather than requiring an event handler from an unrelated plugin type.

          Since no sub plugins will be included in the initial rollout (likely, these will be handled entirely through the plugins database), we should simply need to test for backwards compatibility only. Similarly, since custom services require event handlers from other plugins which are also not included in Core, we can also likely ignore those as well. I think a simple smoke test to ensure that an external tool can be created, launched, and support a return of grades will suffice - let me know how this sounds for testing. I'll also get working on developer docs, so others can build upon the new functionality.

          Show
          Kris Stokking added a comment - Hey Dan, many thanks for your help with this late integration! Before I draft up the testing instructions I want to make sure they match what you might expect. The new code allows for LTI sub plugins to hook into callbacks in two places: 1. When creating a new instance of the external tool. For example, a sub plugin could add a new option in the activity chooser. When selected, it could launch out to an external interface directly rather than requiring the content creator to plug in the launch URL and other complicated settings. This will make it much easier to create new instances of the same tool type. 2. Sub plugins can hook into custom services directly, rather than requiring an event handler from an unrelated plugin type. Since no sub plugins will be included in the initial rollout (likely, these will be handled entirely through the plugins database), we should simply need to test for backwards compatibility only. Similarly, since custom services require event handlers from other plugins which are also not included in Core, we can also likely ignore those as well. I think a simple smoke test to ensure that an external tool can be created, launched, and support a return of grades will suffice - let me know how this sounds for testing. I'll also get working on developer docs, so others can build upon the new functionality.
          Hide
          Damyon Wiese added a comment -

          Thanks Kris,

          Ideally to test this properly we should also confirm that the new functionality works. A dummy test plugin would be nice.

          Show
          Damyon Wiese added a comment - Thanks Kris, Ideally to test this properly we should also confirm that the new functionality works. A dummy test plugin would be nice.
          Hide
          Kris Stokking added a comment -

          Damyon - I'd agree, but that is easier said than done. It would be easy to create the sub-plugin but we (currently) have no tool provider to connect to. I'm still thinking about how we can accomplish that... any recommendations would be welcome!

          Show
          Kris Stokking added a comment - Damyon - I'd agree, but that is easier said than done. It would be easy to create the sub-plugin but we (currently) have no tool provider to connect to. I'm still thinking about how we can accomplish that... any recommendations would be welcome!
          Hide
          Damyon Wiese added a comment -

          Hi Kris,

          See the dummy plugin I uploaded - it doesn't do anything - but it does show that you need to fix the get_types function to return subtypes to the activity chooser properly. The only example for this is the old assignment - the first entry in the list needs to be a dummy entry for the heading and needs have the typestr prefixed with '--' (which will be stripped) and this becomes the heading for the group.

          There are also whitespace errors in mod/lti/mod_form.php

          The last thing is that your add_instance hook does not look useful because it does not accept the form as a parameter - so there is no way to set defaults or add settings to the form.

          It would be great to see these hooks with enough functionality to add settings to the form and save them to the database both on add/update. The current hooks seem designed only for one really specific usecase.

          Show
          Damyon Wiese added a comment - Hi Kris, See the dummy plugin I uploaded - it doesn't do anything - but it does show that you need to fix the get_types function to return subtypes to the activity chooser properly. The only example for this is the old assignment - the first entry in the list needs to be a dummy entry for the heading and needs have the typestr prefixed with '--' (which will be stripped) and this becomes the heading for the group. There are also whitespace errors in mod/lti/mod_form.php The last thing is that your add_instance hook does not look useful because it does not accept the form as a parameter - so there is no way to set defaults or add settings to the form. It would be great to see these hooks with enough functionality to add settings to the form and save them to the database both on add/update. The current hooks seem designed only for one really specific usecase.
          Hide
          Mark Nielsen added a comment -

          Updated the code and posting this on the behalf of Kris:

          Hey Damyon,

          Thanks for the extra work with the dummy plugin!  We actually specifically avoided going that route in the activity chooser as it honestly needed a little work to make it look right.  From a backwards compatibility standpoint, it looks redundant to list subtypes when there's only one of them, and the current hook will automatically create a group if the callback exists - there's no ability to override that.  The new version will have a group for External Tools, and then a single option for General Tool, which looks less than ideal.  I'd recommend we clean that up, but it will require a change to API's outside of LTI.  I can create a separate ticket for that and supply a fix for the following stable release.

          Our next update will have the whitespace errors fixed.

          Regarding the add_instance hook - the intention of this functionality is to allow the subtype to launch immediately into the external tool provider when creating the instance, instead of requiring that to happen in a Moodle form.  The tool provider can then send the required data to a custom service in the subtype which will actually create the course module in Moodle.

          Show
          Mark Nielsen added a comment - Updated the code and posting this on the behalf of Kris: Hey Damyon, Thanks for the extra work with the dummy plugin!  We actually specifically avoided going that route in the activity chooser as it honestly needed a little work to make it look right.  From a backwards compatibility standpoint, it looks redundant to list subtypes when there's only one of them, and the current hook will automatically create a group if the callback exists - there's no ability to override that.  The new version will have a group for External Tools, and then a single option for General Tool, which looks less than ideal.  I'd recommend we clean that up, but it will require a change to API's outside of LTI.  I can create a separate ticket for that and supply a fix for the following stable release. Our next update will have the whitespace errors fixed. Regarding the add_instance hook - the intention of this functionality is to allow the subtype to launch immediately into the external tool provider when creating the instance, instead of requiring that to happen in a Moodle form.  The tool provider can then send the required data to a custom service in the subtype which will actually create the course module in Moodle.
          Hide
          Damyon Wiese added a comment -

          Hi Guys,

          I don't agree with the change to course/lib.php to use the help from the module for a subplugin - this only makes sense for LTI because you are trying to fudge an extra subplugin into the list (the standard one). It is better to make the help required so we get unique useful help for each subplugin.

          So - time is running out so I converted that last patch to use a real subplugin for the default lti support with it's own help.

          See this branch: https://github.com/damyon/moodle/branches/MDL-36224-master

          I'll ping another integrator to check my changes in the new branch.

          Show
          Damyon Wiese added a comment - Hi Guys, I don't agree with the change to course/lib.php to use the help from the module for a subplugin - this only makes sense for LTI because you are trying to fudge an extra subplugin into the list (the standard one). It is better to make the help required so we get unique useful help for each subplugin. So - time is running out so I converted that last patch to use a real subplugin for the default lti support with it's own help. See this branch: https://github.com/damyon/moodle/branches/MDL-36224-master I'll ping another integrator to check my changes in the new branch.
          Hide
          Damyon Wiese added a comment - - edited

          Also - the activity user could do with some padding fixes for modules with subtypes if this gets integrated to make the heading line up with the submodule list. (This should be done as a separate issue)

          Show
          Damyon Wiese added a comment - - edited Also - the activity user could do with some padding fixes for modules with subtypes if this gets integrated to make the heading line up with the submodule list. (This should be done as a separate issue)
          Hide
          Martin Dougiamas added a comment -

          Yes the chooser looks awful right now, we'd not worried about it much before because we'd been trying to get rid of subtypes in the activity chooser completely.

          Aligning will help a lot. I changed the nonoption class to have padding-left of 3.2 em, and the subtype left-padding to 3.9em and it looked better.

          However we probably need a completely new way of handling subtyoes in that list, perhaps they should fold out on demand like form sections do now.

          Show
          Martin Dougiamas added a comment - Yes the chooser looks awful right now, we'd not worried about it much before because we'd been trying to get rid of subtypes in the activity chooser completely. Aligning will help a lot. I changed the nonoption class to have padding-left of 3.2 em, and the subtype left-padding to 3.9em and it looked better. However we probably need a completely new way of handling subtyoes in that list, perhaps they should fold out on demand like form sections do now.
          Hide
          Damyon Wiese added a comment -

          Adding link to new bug for formatting in the activity chooser.

          Show
          Damyon Wiese added a comment - Adding link to new bug for formatting in the activity chooser.
          Hide
          Damyon Wiese added a comment -

          Reuploading plugin - somehow the old one was corrupt.

          Show
          Damyon Wiese added a comment - Reuploading plugin - somehow the old one was corrupt.
          Hide
          Damyon Wiese added a comment -

          Hmm tracker is eating files. Do you like zips?

          Show
          Damyon Wiese added a comment - Hmm tracker is eating files. Do you like zips?
          Hide
          Kris Stokking added a comment -

          Damyon - that's a great approach, thanks for making the changes. We've added your changes to our branch, as I was having trouble testing with yours - I kept getting fatal errors when accessing a course. Note: the requires version is now a bit out of sync in our branch, but that shouldn't be a problem for integration.

          Martin - I think it makes sense to use the existing sub-type behavior for LTI in the activity chooser in the interest of time. However, as a final solution, I'm not convinced that LTI should follow the same rules as other activities that support sub-types. While the functionality for the different assignment sub-types is all very similar, the functionality for external tools will be wildly different. The new capability to install plug-ins directly from moodle.org will make it easy for admins to deploy dozens of tools installed on a single site - we'll likely want to make those options more pronounced and easily distinguishable (such as each provider having their own icon). Food for thought in a later iteration...

          Show
          Kris Stokking added a comment - Damyon - that's a great approach, thanks for making the changes. We've added your changes to our branch, as I was having trouble testing with yours - I kept getting fatal errors when accessing a course. Note: the requires version is now a bit out of sync in our branch, but that shouldn't be a problem for integration. Martin - I think it makes sense to use the existing sub-type behavior for LTI in the activity chooser in the interest of time. However, as a final solution, I'm not convinced that LTI should follow the same rules as other activities that support sub-types. While the functionality for the different assignment sub-types is all very similar, the functionality for external tools will be wildly different. The new capability to install plug-ins directly from moodle.org will make it easy for admins to deploy dozens of tools installed on a single site - we'll likely want to make those options more pronounced and easily distinguishable (such as each provider having their own icon). Food for thought in a later iteration...
          Hide
          Damyon Wiese added a comment -

          Hi Kris,

          We have gone round on this one and in the end we don't feel there is enough time left to polish and review this for 2.5. The subtype support in the activity chooser is ugly (and is ugly for Assignment 2.2) but looks worse when used with LTI and there is only one subtype - the subtype takes up 2 rows and looks all wonky - given that there are no existing implementations of this subtype yet - it seems pointless to make the UI look worse for no immediate benefit. Possible solutions to the subtypes thing are padding or a mechanism to collapse the subtypes into a single row when there is only one - but there is not enough time to agree and implement these changes.

          I also have some lingering doubts about the callbacks you have implemented - but assuming you will redirect the user away from the activity creation page and create the module with a callback, you are will be making it impossible to configure the standard coursemodule settings like restricted access etc.

          In short I (and others) think this needs more work and rushing it into 2.5 would be a mistake.

          Thanks for the work on this - we should get this sorted early for 2.6 so you can progress with your plugin and have the possibility of backporting the changes.

          Cheers!

          Show
          Damyon Wiese added a comment - Hi Kris, We have gone round on this one and in the end we don't feel there is enough time left to polish and review this for 2.5. The subtype support in the activity chooser is ugly (and is ugly for Assignment 2.2) but looks worse when used with LTI and there is only one subtype - the subtype takes up 2 rows and looks all wonky - given that there are no existing implementations of this subtype yet - it seems pointless to make the UI look worse for no immediate benefit. Possible solutions to the subtypes thing are padding or a mechanism to collapse the subtypes into a single row when there is only one - but there is not enough time to agree and implement these changes. I also have some lingering doubts about the callbacks you have implemented - but assuming you will redirect the user away from the activity creation page and create the module with a callback, you are will be making it impossible to configure the standard coursemodule settings like restricted access etc. In short I (and others) think this needs more work and rushing it into 2.5 would be a mistake. Thanks for the work on this - we should get this sorted early for 2.6 so you can progress with your plugin and have the possibility of backporting the changes. Cheers!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Martin Dougiamas added a comment -

          Waking up this issue which depends on MDL-40248 .... 2.6?

          Show
          Martin Dougiamas added a comment - Waking up this issue which depends on MDL-40248 .... 2.6?
          Hide
          Kris Stokking added a comment -

          It looks like MDL-40248 will be integrated soon - can we please revisit this issue?

          Show
          Kris Stokking added a comment - It looks like MDL-40248 will be integrated soon - can we please revisit this issue?
          Hide
          Martin Dougiamas added a comment -

          We can, if the patch is updated for master and ready to go

          Show
          Martin Dougiamas added a comment - We can, if the patch is updated for master and ready to go
          Hide
          Damyon Wiese added a comment -

          Well - I just checked and the patch merges with minor conflicts.

          My concern is that it looks like MDL-40248 did not implement support for promoting the first subtype when there is only one, so for every single moodle install this will show as:

          "External Tools"

          • General tool

          in the activity chooser. Is this really what you are intending?

          Show
          Damyon Wiese added a comment - Well - I just checked and the patch merges with minor conflicts. My concern is that it looks like MDL-40248 did not implement support for promoting the first subtype when there is only one, so for every single moodle install this will show as: "External Tools" General tool in the activity chooser. Is this really what you are intending?
          Hide
          Mark Nielsen added a comment -

          Nah, I need to rework this patch a little.

          Show
          Mark Nielsen added a comment - Nah, I need to rework this patch a little.
          Hide
          Mark Nielsen added a comment -

          Updated the pull branch for master, please take another look. For default Moodle installs, there should be no change to the activity chooser.

          Show
          Mark Nielsen added a comment - Updated the pull branch for master, please take another look. For default Moodle installs, there should be no change to the activity chooser.
          Hide
          Martin Dougiamas added a comment -

          +10 to keep this going, ranked to top.

          Show
          Martin Dougiamas added a comment - +10 to keep this going, ranked to top.
          Hide
          Michael de Raadt added a comment -

          The status of this issue was "Peer review in progress" but I believe it should have been "Waiting for peer review".

          Show
          Michael de Raadt added a comment - The status of this issue was "Peer review in progress" but I believe it should have been "Waiting for peer review".
          Hide
          Andrew Davis added a comment -

          The big outstanding thing to do with this issue is writing some testing instructions. I haven't been involved with this issue before now so I am not confident to write them myself. If anyone who is more familiar with this issue reads this, please jump in and write some testing instructions.

          From a code perspective, this looks great. There is nothing wrong there that warrants fixing at this stage.

          Show
          Andrew Davis added a comment - The big outstanding thing to do with this issue is writing some testing instructions. I haven't been involved with this issue before now so I am not confident to write them myself. If anyone who is more familiar with this issue reads this, please jump in and write some testing instructions. From a code perspective, this looks great. There is nothing wrong there that warrants fixing at this stage.
          Hide
          Andrew Davis added a comment -

          I'm going to have a go at writing some testing instructions for this so it can be moved on to integration.

          Show
          Andrew Davis added a comment - I'm going to have a go at writing some testing instructions for this so it can be moved on to integration.
          Hide
          Andrew Davis added a comment -

          I've added some testing instructions that just use the pre-existing functionality.

          I do have a question that has come out of trying to exercise the new code. Where is lti_get_types() called from? Its a new function but I cannot find where it is called.

          I'm putting this up for integraton but, Mark (or anyone else), if you have more information about how we can exercise the new code that would be great.

          Show
          Andrew Davis added a comment - I've added some testing instructions that just use the pre-existing functionality. I do have a question that has come out of trying to exercise the new code. Where is lti_get_types() called from? Its a new function but I cannot find where it is called. I'm putting this up for integraton but, Mark (or anyone else), if you have more information about how we can exercise the new code that would be great.
          Hide
          Andrew Davis added a comment -

          I am replacing the git branch. The original was https://github.com/moodlerooms/moodle/compare/moodle:master...moodlerooms:MDL-36224_master

          I haven't made any code changes. I've just rebased onto latest master.

          Show
          Andrew Davis added a comment - I am replacing the git branch. The original was https://github.com/moodlerooms/moodle/compare/moodle:master...moodlerooms:MDL-36224_master I haven't made any code changes. I've just rebased onto latest master.
          Hide
          Kris Stokking added a comment -

          Andrew - The lti_get_types is called by a new function that was added in MDL-40248. Many thanks for putting testing instructions on this ticket. I think you nailed it for backwards compatibility, and I've added another test to verify that LTI sub-types are grouped in the activity picker correctly.

          Show
          Kris Stokking added a comment - Andrew - The lti_get_types is called by a new function that was added in MDL-40248 . Many thanks for putting testing instructions on this ticket. I think you nailed it for backwards compatibility, and I've added another test to verify that LTI sub-types are grouped in the activity picker correctly.
          Hide
          Damyon Wiese added a comment -

          Thanks everyone, this issue has been integrated to master only. I am satisfied that it does not affect the UI for anyone not using an LTI sub plugin and provides a benefit to someone who specifically wants to use this.

          Show
          Damyon Wiese added a comment - Thanks everyone, this issue has been integrated to master only. I am satisfied that it does not affect the UI for anyone not using an LTI sub plugin and provides a benefit to someone who specifically wants to use this.
          Hide
          Rajesh Taneja added a comment -

          Thanks for improving LTI, Mark,

          Works as mentioned, I was able to Khan academy folder and video.
          Also, installed Dummy LTI module and was able to create an instance of it.

          Passing...

          Show
          Rajesh Taneja added a comment - Thanks for improving LTI, Mark, Works as mentioned, I was able to Khan academy folder and video. Also, installed Dummy LTI module and was able to create an instance of it. Passing...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It's Friday, I'm tired so I won't be very imaginative today.

          No matter of that, yes, you did it! Thanks for your collaboration!

          Closing this as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - It's Friday, I'm tired so I won't be very imaginative today. No matter of that, yes, you did it! Thanks for your collaboration! Closing this as fixed!
          Hide
          Michael de Raadt added a comment -

          Hi, Kris.

          This issue is labelled with dev_docs_required and you mentioned you were starting to write dev docs. Did you complete this?

          Show
          Michael de Raadt added a comment - Hi, Kris. This issue is labelled with dev_docs_required and you mentioned you were starting to write dev docs. Did you complete this?
          Hide
          Juan Leyva added a comment -

          Hi Kris Stokking, with this new feature, I suppose it will be easy to create a subplugin for supporting the membership service extension, right? http://developers.imsglobal.org/ext_membership.html and also the ones used by canvas? https://www.edu-apps.org/extensions/index.html

          I'm asking because I'm missing hooks for adding extra launch parameters (needed by the provider tool), is it in your plans add this type of hooks?

          Show
          Juan Leyva added a comment - Hi Kris Stokking , with this new feature, I suppose it will be easy to create a subplugin for supporting the membership service extension, right? http://developers.imsglobal.org/ext_membership.html and also the ones used by canvas? https://www.edu-apps.org/extensions/index.html I'm asking because I'm missing hooks for adding extra launch parameters (needed by the provider tool), is it in your plans add this type of hooks?

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: