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

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.

https://github.com/scriby/moodle/commit/ddba72261f888600a3d9a4af806d1cdb773782e0
https://github.com/scriby/moodle/commit/f5dc0b00ed308ab3016d4e74c10b63c223cfe8db

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

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.

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.

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

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

(We suck)

Please can you give testing instructions?

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.

Thanks Kris,

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

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!

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.

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.

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.

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)

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.

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

Reuploading plugin - somehow the old one was corrupt.

Hmm tracker is eating files. Do you like zips?

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...

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!

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

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

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

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

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?

Nah, I need to rework this patch a little.

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

+10 to keep this going, ranked to top.

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

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.

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

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.

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.

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.

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.

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...

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!

Hi, Kris.

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

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?