View source for IMS-LTI consumer module review 2011-11

=== Basic References ===

* '''Repositories''': 
** Repo for new 2.2 core "lti" module, available until integration @ https://github.com/scriby/moodle/compare/master...lti.
** Repo for previous 1.9, 2.0 and 2.1 contrib "basiclti" module, available @ http://code.google.com/p/basiclti4moodle/ . 
* '''Docs''': [https://docs.moodle.org/20/en/IMS-LTI_module (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 release notes|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 "<span style="color:green">Done!</span>".

=== The review ===

==== A.  Things to fix now before integration next week ====

# <span style="color:green">Done! (has been agreed to keep it as "lti" and "External Tool", see MDL-20534)</span>. 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.
# <span style="color:green">Done! (upgrade and moodle1 code deleted)</span>. 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).
# <span style="color:green">Done! (Oauth and lang stuff ignored, and still some warns remaining - line lengths... - but I'm not going to fix those now).</span>. 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.
# <span style="color:green">Done! (all uses have now one TODO to MDL-30149)</span>. We don't use php namespaces in Moodle 
# <span style="color:green">Done!</span>. 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.
# <span style="color:green">Done! (see MDL-30175)</span>. Point B3 below needs to be decided (reverted and or implemented in an alternative way) before integrating.
# <span style="color:green">Done!</span> Take rid of build and deploy shell scripts, TODO.txt and lang/en/help
# <span style="color:green">Done!</span> lib.php cannot include locallib.php @ global scope. Only within the functions requiring it.
# <span style="color:green">Done!</span> The add, edit and delete tool icons present in the mod_form are doing nothing/ending in blank popup (popups are forbidden, basically).
# <span style="color:green">Done! (Clarified, was a wrong interpretation)</span>. The $this->_customdata->isadmin condition does not seems to be working ok, logged as admin I cannot see settings like "coursevisible" or "organizations" in the instructor_edit_tool_type.php.
# <span style="color:green">Done!( Clarified, was a wrong interpretation)</span>. We should not have isadmin() deciding about features like showing "coursevisible" or "organizations". Capabilities are required for that. Also they must be observed also when processing the form data or anybody will be able to "supplant" that information easily.

==== B. Things to fix before 2.2 ====

# Add MDLQA functionality tests   (Moodle or MR). Also some user-level docs.
# 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) 
# <span style="color:green">Done! (see MDL-30175)</span>. /lib/outputlib.php - [https://github.com/stronk7/moodle/commit/e9580690b22a6f48fb55d5470d8b863261ea7182 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!
# Not sure but it seems to exist some problem with lis_result_sourcedid and the [http://www.imsglobal.org/developers/BLTI/tool.php 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.
# <span style="color:green">Done! (Clarified)</span>. 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.
# <span style="color:green">Done! (added Chris Scribner as initial leader)</span>. Moodle: Create the IMS-LTI consumer component in the Tracker.
# 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).
# MDL-30326 - lti_request_is_using_ssl() uses $ME, that only contains the path (not the scheme nor host).
# <span style="color:green">Done! (see MDL-30343)</span> - lti/mod/log.php is missing the definitions of logging actions. That needs to be completed and uses revisited.
# launch.php missing any capability check? Others scripts too?
# <span style="color:green">Done! (see MDL-30339)</span> - 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.
# YUI2 is being used, it must be moved to YUI3.
# How are grades handled? Both lti_add_instance() and database definition set it to default 100 but there is no way to specify one scale or another max value in the mod form.
# <span style="color:green">Done! (see MDL-30290)</span> - The "Display description on course page" feature is missing.
# lib.php: Clean all the functions that are not needed and review all the @TODO notes there.
# The form to create templates (types) at admin (typessettings.php) and at activity settings (instructor_edit_tool_type.php) are missing support for some things like SSL fields/icons... they should support everything supported by the module or could lead to problems when picking.
# Some minor details about the DB schema:
#* We should use only "big" texts everywhere. Sooner or later we'll be taking out that differentiation (only supported by MySQL) so if they are big since day 0, better.
#* Columns pointing to other table->id should be named "xxxx'''id'''" and one foreign key defined always and not index (createdby, userid, typeid, courseid...)
#* Regarding the url fields, since Moodle 2.2 we support varchars up to 1300cc (aprox). Surely they are enough to store all those URLs in the module. For your consideration.
# Review all the require_login() uses, they are missing module and it's mandatory to allow it to check for a lot of stuff (groups, module visibility...)
# When grading happens, we need to be sure that there is proper support for decimal separators, perhaps not important because no manual grades are entered but only percentages get re-scaled to php values, but needs confirmation.
# Any administration/configuration/write option must be protected by sesskey. Anything returning json data should include it for safety.
# There are some URLs passed in requests ('toourl'...), is that properly encoded to be sent/received properly?
# Privacy: Perhaps the initial value for all the "share xxx" in config should be "no". Specially the roster one seems "dangerous" in the meaning of one teacher playing with external tools, and "publishing" the fullname/email to unknown (non-trusted) sources. Perhaps some capability should authorize this?
# missing any RISK_XSS in capability definitions? Anything able to embed flash/javascript (text editor) should have that risk defined. And together with the Privacy point above, anything able to disclose personal information should have the RISK_PERSONAL one.
# Who can edit/delete global tools? How are the "tool requests" handled?
# format_text() missing (intro and any other text+format). That leads to no multilang support nor embed images display.
# Anything being set output as "title" or other html tag attributes should be using p() or s() for basic clean and format_strings() if are contents introduced by users requiring multilang support.
# <span style="color:green">Done! (see MDL-30354)</span> - locallib:
#* get_string('no_' . $id, 'lti') ??
#* lti_get_url_thumbprint(): Why is doing that with "www" part of the hostname?
#* review lti_filter_get_types() uses, missing $course param
#* lti_filter_print_types() unused? Also it included old output code (harcoded paths to icons, echos...) would be great to update as much as possible to current output stuff.
#* $type needs use of new stdClass(), cannot be constructed implicitly.
#* lti_post_launch_html() cannot it build that form in another way. Are all the inserted values (endpoint/params...) safe/encoded enough?
# <span style="color:green">Done! (see MDL-30341)</span> - Review and complete all the "MRTODO" placeholders in code (copyrights and brief descriptions)
# MDL-30347 - mod/lti/simpletest unit tests failing. Getting 6 passes, 5 fails and 2 exceptions

==== C.  Things to fix in 2.3 ====

# 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.
# Add unit-tests and also contain a comprehensible collection of MDLQA tests with examples, expected behaviors...
# <span style="color:green">Done! (it seems to be the cleaner way, only blocks can end out from view)</span>. 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..
# Also related with visualization, there is one (default) option which meaning is not clear (or I've not been able to find).
# Better if the module has icon.png instead of gif
# Right now lti_types->course = 1 (SITE) is used to store global "template" types, and other values are used to store "course" templates. That approach must be moved to "contextid", so CONTEXT_SYSTEM is used for global templates and CONTEXT_COURSE for course templates. That will provide a better control using capabilities and easier integration in the multi-tenants thingy. Current implementation (course based, with SITE meaning global is far from perfect).
# Although we are now officially allowing external URLs (A6/B3) to be the activity icon, this needs to go one step ahead and be able to fetch the icon from the  icon URLs and store them into "icon" filearea and serve them from there.
# All the subtasks of MDL-20534 and new issues detected for that component once it lands.

=== 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!