IMS-LTI consumer module review 2011-11

Basic References

• Repository: (undefined, not sure where the public repo for that is / exists, review is being done against private repo) Repo for versions 1.9, 2.0 and 2.1 : http://code.google.com/p/basiclti4moodle/ .
• Docs: (non exiting, sure we need it ASAP in 2.2 Docs - once landed)
• Tracker: MDL-20534
• Forums: http://groups.google.com/group/ims-dev

Intro and Organization

This page sums-up efforts from Martin and Eloy over the last days (sigh, really too late, blame Eloy for that, sorry!) to review the IMS-LTI consumer module aiming to Moodle 2.2 as target version to get it added to core.

Just some basic rules:

• Please use the letter + number (A1, B4...) to reference to each point everywhere.
• After landing upstream, use new issues (subtasks of MDL-20534) in the Tracker to perform each change.
• On completion, each item below should be marked as "Done!".

The review

A. Things to fix now before integration next week

1. The name, I think "basic" is out from IMS-slang recently, so np with that, but, for consistency with current "imscp" one we should call this "imslti". Not really critical, but it is now or never, so I need to address it here.
2. Should this have any upgrade path from older imslti modules? Or do we assume it's brand-new-version-1.0 release (no previous stuff to migrate).
3. Run the code-checker, http://moodle.org/plugins/view.php?plugin=local_codechecker
   • tons of 3-slashes comments
   • wrong if/for spaces
   • missing phpdocs
   • double-quoted strings...
   • llegal whitespace here and there.
4. We don't use php namespaces in Moodle
5. Abusing Heredoc/Nowdoc is not a good idea. Only should be used if necessary. Oh, specially for lang strings, we never have used them there. Recommend replacing with strings like most Moodle code.

B. Things to fix before 2.2

1. Add MDLQA functionality tests (Moodle or MR)
2. service.php requires special attention. As far as it's the script receiving any interaction from the provider needs serious review:
   • Seems to be processing the request is an highly "uncommon" way.
   • Handling of exceptions surely need polish too.
   • Check code does not accept any sort of injection (everything cleaned)
3. /lib/outputlib.php - Patching pix_url() that way is correct and we can't do it. That function is exclusively for internal urls. Sizes are not controlled either. Better if mod_lti pulls in the icon and stores it internally in resized form suitable for Moode to display much like the 'file' module does. BTW the default icon is awful too, LOL!
4. Not sure but it seems to exist some problem with lis_result_sourcedid and the IMS LTI 1.0 PHP basic provider, being incorrectly handled so no grades can go back properly to Moodle. Perhaps information should be encoded in another, safer way, instead of json_encode structure, by simple concat of needed info with separators (sounds to me that the original imp. used this, like Sakai, with ':::' as separators, or base64 encoding the json structure… or whatever.
5. Admin stuff.. what is all that pending/rejected stuff? And organizations? How is it related with the types created when editing one imslti activity? There is something there not fitting well IMO.
6. Moodle: Create the IMS-LTI consumer component in the Tracker.

C. Things to fix in 2.3

1. OAuth stuff should go to /lib (original code, with original ©) as part of the wrapper structure that can use PHP's PECL OAuth extension if it exists, and also with our own oauthlib.php implementation. We should only have one OAuth API in Moodle.
2. Add unit-tests and also contain a comprehensible collection of MDLQA tests with examples, expected behaviors...
3. Using embed visualization disables window main scroll-bars. It can lead to problems with items in navigation/settings blocks not being accessible (really minor but annoyed me twice). Surely it's on purpose to avoid the double scroll bars..
4. Also related with visualization, there is one (default) option which meaning is not clear (or I've not been able to find).
5. All the subtasks of MDL-20534 and new issues detected for that component once it lands.

D. Still to do: review request handling, cleaning, SQL, Install/Upgrade, module features, backup & restore

(in progress @ the time of publishing this. Note that, depending of the nature of the new issues found, some of them may land to previous sections)

Conclusions

Overall we like it. And I think it's important enough to have it integrated ASAP. Ideally next week (2nd week Nov 2011), but with the points exposed here fixed. Along the rest of the month I'm sure that everything else (bugs and TODO above) should be fixable (depending of devs availability, of course). Great work!