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. Done! (has been agreed to keep it as "lti" and "External Tool", see MDL-20534). The name: "lti". 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. Done! (upgrade and moodle1 code deleted). 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. Done! (Oauth and lang stuff ignored, and still some warns remaining - line lengths... - but I'm not going to fix those now).. 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. Done! (all uses have now one TODO to MDL-30149). We don't use php namespaces in Moodle
5. Done!. 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.
6. Done! (see MDL-30175). Point B3 below needs to be decided (reverted and or implemented in an alternative way) before integrating.
7. Done!. Take rid of build and deploy shell scripts, TODO.txt and lang/en/help
8. lib.php cannot include locallib.php @ global scope. Only within the functions requiring it.

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. Done! (see MDL-30175). /lib/outputlib.php - Patching pix_url() that way is NOT 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. Or move that stuff supporting external urls to another layer... as a less-bad solution... 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.
7. Implement backup & restore. Seems incomplete. Take special attention to how types are going to be handled (is baseurl ok to uniquely detect any type, or do we need to introduce some sort of hash with other attributes and configuration).
8. lti_request_is_using_ssl() uses $ME, that only contains the path (not the scheme nor host).
9. lti/mod/log.php is missing the definitions of logging actions. That needs to be completed and uses revisited.
10. launch.php missing any capability check? Others scripts too?
11. lang strings:
    • the strings should be ordered alphabetically by keys
    • the help strings should be markdown format, not html.
    • Those linefeeds at the beginning and end of a lot of strings are not really usual in moodleland.
12. YUI2 is being used, it must be moved to YUI3.

C. Things to fix in 2.3

1. MDL-30149: 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. Done! (it seems to be the cleaner way, only blocks can end out from view). 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. Better if the module has icon.png instead of gif
6. 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!