Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.2
    • Labels:
      None
    • Rank:
      16323

      Description

      Tim Hunt wrote: I watched an online talk about IMS LTI (Learning Tools Interoperability) last week. http://ims.wimba.com/launcher.cgi?room=_webinar_2009_0928_1107_16 - if you are able to get it. It is worth a watch (80 minutes). There is sample code we can steal, and even an example of a nice admin interface someone has done that we could probably use too. I think it would just be a new resource-like module.

      1. basiclti_tools.rtf
        2 kB
        Charles Severance
      2. BLTI_TestPlan.txt
        11 kB
        Charles Severance
      3. MDL-20534-cert.txt
        5 kB
        Charles Severance

        Issue Links

        Progress
        Unresolved Sub-Tasks

        Sub-Tasks

          Activity

          Hide
          Ludo ( Marc Alier) added a comment -

          Hi there, we (UPC wiki team) are working with Charles Severance at IMS o develop a IMS BasicLTI for Moodle 1.9. The code, documentation and tracker is at http://code.google.com/p/basiclti4moodle/
          When Moodle 2.0 and BLTI will be both a litle stable we will port the thing.

          Show
          Ludo ( Marc Alier) added a comment - Hi there, we (UPC wiki team) are working with Charles Severance at IMS o develop a IMS BasicLTI for Moodle 1.9. The code, documentation and tracker is at http://code.google.com/p/basiclti4moodle/ When Moodle 2.0 and BLTI will be both a litle stable we will port the thing.
          Hide
          Ludo ( Marc Alier) added a comment -

          IMS has approved and released the BLTI. the moodle consumer (see link in previous comment) has passed all the tests in the testing framework thar dr chuck has setted up.
          so we have a moodle 1.9 activity module that acts as a fine ims blt consumer
          now in a few weeks we will port it to moodle 2.0

          Show
          Ludo ( Marc Alier) added a comment - IMS has approved and released the BLTI. the moodle consumer (see link in previous comment) has passed all the tests in the testing framework thar dr chuck has setted up. so we have a moodle 1.9 activity module that acts as a fine ims blt consumer now in a few weeks we will port it to moodle 2.0
          Show
          Ludo ( Marc Alier) added a comment - http://www.imsglobal.org/pressreleases/pr100216.html
          Hide
          Ludo ( Marc Alier) added a comment -

          Version for Moodle 2.0 is implemented.
          Both versions for Moodle 1.9 and Moodle 2.0 pass IMS conformance tests (we can use the badges).
          Please find code at http://code.google.com/p/basiclti4moodle/
          Letting issue as Fixed waiting for revision from the big guns :-D

          Show
          Ludo ( Marc Alier) added a comment - Version for Moodle 2.0 is implemented. Both versions for Moodle 1.9 and Moodle 2.0 pass IMS conformance tests (we can use the badges). Please find code at http://code.google.com/p/basiclti4moodle/ Letting issue as Fixed waiting for revision from the big guns :-D
          Hide
          Ludo ( Marc Alier) added a comment -

          Awaiting orders...

          Show
          Ludo ( Marc Alier) added a comment - Awaiting orders...
          Hide
          Martin Dougiamas added a comment -

          Needs review but I'd like to see this in 2.1 in June.

          Show
          Martin Dougiamas added a comment - Needs review but I'd like to see this in 2.1 in June.
          Hide
          Martin Dougiamas added a comment -

          Please don't close issues until they get committed to core. Otherwise they get lost.

          Show
          Martin Dougiamas added a comment - Please don't close issues until they get committed to core. Otherwise they get lost.
          Hide
          Martin Dougiamas added a comment -

          Is anyone using this module? Can I have some feedback from a user POV?

          Show
          Martin Dougiamas added a comment - Is anyone using this module? Can I have some feedback from a user POV?
          Hide
          Charles Severance added a comment -

          Martin, the end-user UI looks like just another resource. End-users are likely not to even know that they are using Basic LTI. If you are looking for feedback for UI/usability - it is more admin types than end-user types. And even then, it is not heavily used by admins. The use it once to plug something like Wimba or LectureTools into Moodle and then they forget about it. The entire UI is in the admin-space. Here is a lits of compliant tools:

          http://www.imsglobal.org/cc/statuschart.cfm

          Here is my "welcome new developer pages" with lots of handouts, sample code, presentations, screen casts, etc.

          http://www.imsglobal.org/developers/BLTI/

          Here is a video of me certifying Moodle with this plugin a while back:

          http://www.vimeo.com/9957979

          I would also check with the folks at MoodleRooms - last I knew they were working to put this into both their 1.9 and 2.0 instances.

          /Chuck

          Show
          Charles Severance added a comment - Martin, the end-user UI looks like just another resource. End-users are likely not to even know that they are using Basic LTI. If you are looking for feedback for UI/usability - it is more admin types than end-user types. And even then, it is not heavily used by admins. The use it once to plug something like Wimba or LectureTools into Moodle and then they forget about it. The entire UI is in the admin-space. Here is a lits of compliant tools: http://www.imsglobal.org/cc/statuschart.cfm Here is my "welcome new developer pages" with lots of handouts, sample code, presentations, screen casts, etc. http://www.imsglobal.org/developers/BLTI/ Here is a video of me certifying Moodle with this plugin a while back: http://www.vimeo.com/9957979 I would also check with the folks at MoodleRooms - last I knew they were working to put this into both their 1.9 and 2.0 instances. /Chuck
          Hide
          Martin Dougiamas added a comment -

          It's not the UI that I'm worried about, it's possible bugs. We're having a nightmare dealing with the wiki code in 2.0 ...

          Show
          Martin Dougiamas added a comment - It's not the UI that I'm worried about, it's possible bugs. We're having a nightmare dealing with the wiki code in 2.0 ...
          Hide
          Steve Lay added a comment -

          I used it to demonstrate a prototype Basic LTI connection at a recent Questionmark Users Conference and it worked just fine. I was only using it on Moodle 1.9 though, so I can't vouch for how it works in 2.0.

          Show
          Steve Lay added a comment - I used it to demonstrate a prototype Basic LTI connection at a recent Questionmark Users Conference and it worked just fine. I was only using it on Moodle 1.9 though, so I can't vouch for how it works in 2.0.
          Hide
          Ludo ( Marc Alier) added a comment - - edited

          Martin, the wiki was developed by grad students. Those where the resources I had. And I'm sorry if there are still issues.

          This module has been developed by Jordi Piguillem and Nikolas Galanis, supervised by dr chuck on the lti compliance part.

          Show
          Ludo ( Marc Alier) added a comment - - edited Martin, the wiki was developed by grad students. Those where the resources I had. And I'm sorry if there are still issues. This module has been developed by Jordi Piguillem and Nikolas Galanis, supervised by dr chuck on the lti compliance part.
          Hide
          Charles Severance added a comment -

          Martin, A couple of things. The 1.9 code is several years old and has had numerous eyes on it including Marc, Nickolas, Jordi, Tom Murdock of MoodleRooms. There have been several improvements that have caused the code to be reviewed multiple times. I would guess we have at least 50 installs of the 1.9 code. The primary installer is folks developing and testing BLTI as well as a few folks deploying it. We don't know who they are since it is pretty simple and has been quite solid for a while now. Every now and again we will see something like this:

          http://www.youtube.com/watch?v=kpPZ4osXJO0

          I would also point out that the use of this tool will grow slowly as more and more tools support BLTI so unlike the Wiki, we won't have a mad rush of a few million teachers pounding on it the day after release. I am confident the code is solid and if we find something, it will be minor and easy to fix without any real panic.

          I don't think there is nearly as much experience with the 2.0 code as it was derived from the 1.9 code more recently. But even for that code Tom Murdock of MoodleRooms has gone over it. But the clear reality is that the 2.0 code is less well tested just because of the nature of the 2.0 rollout and the fact that if someone wants to whip up a quick server - they still choose 1.9 instead of 2.0.

          You might feel more comfortable if we did a bit of functionality and code review of the 2.0 version with someone at HQ - I am happy to participate in that conversation.

          I am happy to support you including this in 2.1 by volunteering to write a detailed test plan (whatever format and level of detail is expected of a Moodle feature) and then volunteer to perform the tests as the code is integrated and modified through release candidates as necessary according to your schedule. The certification tests are a pretty solid set of unit tests because they make the developer cause the Moodle to jump through a bunch of hoops.

          Hope this helps.

          Show
          Charles Severance added a comment - Martin, A couple of things. The 1.9 code is several years old and has had numerous eyes on it including Marc, Nickolas, Jordi, Tom Murdock of MoodleRooms. There have been several improvements that have caused the code to be reviewed multiple times. I would guess we have at least 50 installs of the 1.9 code. The primary installer is folks developing and testing BLTI as well as a few folks deploying it. We don't know who they are since it is pretty simple and has been quite solid for a while now. Every now and again we will see something like this: http://www.youtube.com/watch?v=kpPZ4osXJO0 I would also point out that the use of this tool will grow slowly as more and more tools support BLTI so unlike the Wiki, we won't have a mad rush of a few million teachers pounding on it the day after release. I am confident the code is solid and if we find something, it will be minor and easy to fix without any real panic. I don't think there is nearly as much experience with the 2.0 code as it was derived from the 1.9 code more recently. But even for that code Tom Murdock of MoodleRooms has gone over it. But the clear reality is that the 2.0 code is less well tested just because of the nature of the 2.0 rollout and the fact that if someone wants to whip up a quick server - they still choose 1.9 instead of 2.0. You might feel more comfortable if we did a bit of functionality and code review of the 2.0 version with someone at HQ - I am happy to participate in that conversation. I am happy to support you including this in 2.1 by volunteering to write a detailed test plan (whatever format and level of detail is expected of a Moodle feature) and then volunteer to perform the tests as the code is integrated and modified through release candidates as necessary according to your schedule. The certification tests are a pretty solid set of unit tests because they make the developer cause the Moodle to jump through a bunch of hoops. Hope this helps.
          Hide
          Ludo ( Marc Alier) added a comment -

          The code is hosted on a Mercurial (git like thing) on http://code.google.com/p/basiclti4moodle/
          is open for reading, if someone wants write access please give me your google account and I will gran permissions.
          If you move the code to a internal moodle git, please let us know where are you working so we can try to give a hand and backport the security corrections to the 2.0 version, that as dr chuck has pointed is less tested than the 1.9 one.
          L.

          Show
          Ludo ( Marc Alier) added a comment - The code is hosted on a Mercurial (git like thing) on http://code.google.com/p/basiclti4moodle/ is open for reading, if someone wants write access please give me your google account and I will gran permissions. If you move the code to a internal moodle git, please let us know where are you working so we can try to give a hand and backport the security corrections to the 2.0 version, that as dr chuck has pointed is less tested than the 1.9 one. L.
          Hide
          Ludo ( Marc Alier) added a comment -

          Hi,
          we are working on the backup/restore features:
          Issues related to the whole IMS BLTI architecture:

          • the content/service provided by the blti consumer is provided by an external app. So no content or service will be backed up really
          • part of the blti tool setup is created by the admin and do not belong in the course. this part while could be backed up, would be a security issue to restore it
          • if a blti activity is restored in a new course, the consumer will send to the external tool a diferent course id and roster, so in some tools the tool will act as in another course context - wich is fair.

          Chuck, I don't know if a future extension of blti could handle the feature of backuped/restored consumers. Many teachers use this feature to make templates of their courses.

          I will drop a comment when the backup/restore thing will be included on the version for 2.0 (the 1.9 version does not support it and will not so far)
          Cheers
          L.

          Show
          Ludo ( Marc Alier) added a comment - Hi, we are working on the backup/restore features: Issues related to the whole IMS BLTI architecture: the content/service provided by the blti consumer is provided by an external app. So no content or service will be backed up really part of the blti tool setup is created by the admin and do not belong in the course. this part while could be backed up, would be a security issue to restore it if a blti activity is restored in a new course, the consumer will send to the external tool a diferent course id and roster, so in some tools the tool will act as in another course context - wich is fair. Chuck, I don't know if a future extension of blti could handle the feature of backuped/restored consumers. Many teachers use this feature to make templates of their courses. I will drop a comment when the backup/restore thing will be included on the version for 2.0 (the 1.9 version does not support it and will not so far) Cheers L.
          Hide
          Charles Severance added a comment -

          Marc, The way to model a activity placement in a backup it is to simply store the URL and any local choices like instructorchoicesendname and all the values of the instructor* fields. The key/secret should not be backed up in any situation. On restore you use the entire URL as a logical key to reconnect the activity in the class to the admin-created instance with key/secret, copying bits back and forth as needed. Since it is the case that the URL may not exist as an admin-created tool at the moment of import, we will have to add a bit of code to the activity edit and launch to detect when the activity row is not connected to an admin row and properly reconnect things if the admin row now exists or put up a meaningful message saying, something like "This link is not fully configured, please have your administrator install a Basic LTI tool for http://www.khanacademy.org/launch" or similar.

          One nice thing is that the resolution between instructor settings and admin settings for things like sending e-mail address are late-bound and admin settings always override instructor settings (this allows admins to change the rules after activities are placed in a course). Because of this if we store some instructor settings for an activity placement in the backup and then restore it in a situation where the admin settings are set to take precedence over instructor settings, the restored instructor settings will be ignored and the instructor's edit activity will work just fine.

          This means that if a course is exported from a course where the admin has one set of settings for a Basic LTI tool and then moved to a different settings where the same URL is installed with different admin settings, the new admin settings will win. Which is as it should be.

          Show
          Charles Severance added a comment - Marc, The way to model a activity placement in a backup it is to simply store the URL and any local choices like instructorchoicesendname and all the values of the instructor* fields. The key/secret should not be backed up in any situation. On restore you use the entire URL as a logical key to reconnect the activity in the class to the admin-created instance with key/secret, copying bits back and forth as needed. Since it is the case that the URL may not exist as an admin-created tool at the moment of import, we will have to add a bit of code to the activity edit and launch to detect when the activity row is not connected to an admin row and properly reconnect things if the admin row now exists or put up a meaningful message saying, something like "This link is not fully configured, please have your administrator install a Basic LTI tool for http://www.khanacademy.org/launch " or similar. One nice thing is that the resolution between instructor settings and admin settings for things like sending e-mail address are late-bound and admin settings always override instructor settings (this allows admins to change the rules after activities are placed in a course). Because of this if we store some instructor settings for an activity placement in the backup and then restore it in a situation where the admin settings are set to take precedence over instructor settings, the restored instructor settings will be ignored and the instructor's edit activity will work just fine. This means that if a course is exported from a course where the admin has one set of settings for a Basic LTI tool and then moved to a different settings where the same URL is installed with different admin settings, the new admin settings will win. Which is as it should be.
          Hide
          Ludo ( Marc Alier) added a comment -

          Chuck,
          this is the approach we are taking. Thanks.

          I expect to have the backup/restore on the mercurial repo by monday.

          Show
          Ludo ( Marc Alier) added a comment - Chuck, this is the approach we are taking. Thanks. I expect to have the backup/restore on the mercurial repo by monday.
          Hide
          Nikolas Galanis added a comment -

          Hi all,

          I have just finished adding support for backup and restore.

          I have tried successfully:
          1. Backup and restore all basicLTI tools of a course within the same course using the "delete all course contents" option.
          2. Create a new course using a backup of a different one.

          Both cases work fine.

          As mentioned before, I am only backing up the information provided by the course creator and not the Moodle administrator, so trying to restore the backup file to a different Moodle will not work.

          The new version is up both in our hg repository and as a zip file in http://code.google.com/p/basiclti4moodle/

          Regards,

          Nikolas

          Show
          Nikolas Galanis added a comment - Hi all, I have just finished adding support for backup and restore. I have tried successfully: 1. Backup and restore all basicLTI tools of a course within the same course using the "delete all course contents" option. 2. Create a new course using a backup of a different one. Both cases work fine. As mentioned before, I am only backing up the information provided by the course creator and not the Moodle administrator, so trying to restore the backup file to a different Moodle will not work. The new version is up both in our hg repository and as a zip file in http://code.google.com/p/basiclti4moodle/ Regards, Nikolas
          Hide
          Charles Severance added a comment -

          I am reviewing this. Just looking at the backup code, I think that too much info may be placed in the ZIP file. The placement secret and several other items should not be in the backup. Let me look things over and get back.

          Show
          Charles Severance added a comment - I am reviewing this. Just looking at the backup code, I think that too much info may be placed in the ZIP file. The placement secret and several other items should not be in the backup. Let me look things over and get back.
          Hide
          Charles Severance added a comment -

          I reviewed the backup code and made some suggestions as to how to make it so the backup can be restored in a different Moodle - I sent the detail as to what I think needs to be done to Marc, and Nikolas.

          Show
          Charles Severance added a comment - I reviewed the backup code and made some suggestions as to how to make it so the backup can be restored in a different Moodle - I sent the detail as to what I think needs to be done to Marc, and Nikolas.
          Hide
          Nikolas Galanis added a comment -

          The backup code seems to be working correctly, although we are still testing it for possible bugs (thanks Dr. Chuck!). I have uploaded the latest version in our hg repository. The zip file on the project's webpage is slightly outdated so, as a general rule, I would suggest checking files out from the repository.

          Show
          Nikolas Galanis added a comment - The backup code seems to be working correctly, although we are still testing it for possible bugs (thanks Dr. Chuck!). I have uploaded the latest version in our hg repository. The zip file on the project's webpage is slightly outdated so, as a general rule, I would suggest checking files out from the repository.
          Hide
          Nikolas Galanis added a comment -

          Some final (hopefully) functionality added: Now you can chose whether to fix a misconfigured (due to a failed restore) tool either by defining a new configuration or by using an existing one.

          The project repository is updated with the new code and a new zip file.

          Show
          Nikolas Galanis added a comment - Some final (hopefully) functionality added: Now you can chose whether to fix a misconfigured (due to a failed restore) tool either by defining a new configuration or by using an existing one. The project repository is updated with the new code and a new zip file.
          Hide
          Martin Dougiamas added a comment -

          Can you please fix all the whitespace issues (should be 4-space indents everywhere, no tabs and no trailing spaces).

          Also I see some "global $DB" declarations at the top of pages etc which are not necessary.

          I'd like to see this submitted for integration before Monday, let's get these small things fixed to increase the chances of acceptance.

          I'll keep looking as I have time.

          Show
          Martin Dougiamas added a comment - Can you please fix all the whitespace issues (should be 4-space indents everywhere, no tabs and no trailing spaces). Also I see some "global $DB" declarations at the top of pages etc which are not necessary. I'd like to see this submitted for integration before Monday, let's get these small things fixed to increase the chances of acceptance. I'll keep looking as I have time.
          Hide
          Charles Severance added a comment -

          Martin, Dumb question - but is there a format for a test plan? I would like to get a test plan started. If there is no format, I will just do plain text - but if you have a format I will follow it.

          Show
          Charles Severance added a comment - Martin, Dumb question - but is there a format for a test plan? I would like to get a test plan started. If there is no format, I will just do plain text - but if you have a format I will follow it.
          Hide
          Ludo ( Marc Alier) added a comment -
          Show
          Ludo ( Marc Alier) added a comment - Chuck, see: http://docs.moodle.org/20/en/Development:Tests
          Hide
          Charles Severance added a comment -

          Thanks - Off I go int into Wiki

          Show
          Charles Severance added a comment - Thanks - Off I go int into Wiki
          Hide
          Martin Dougiamas added a comment -

          Sorry, those wiki pages are a little out of date now. Michael D is working on a new developer wiki which will clean up those old pages and separate them from the main docs, it will be at http://docs.moodle.org/dev soon.

          There's three types of testing you need to think about:

          1. Code tests. Your module should have some unit tests implemented to test specific functions in the code. See http://docs.moodle.org/20/en/Development:Unit_tests The exact tests are up to you, but it would be best to go for core functions that everything relies on.

          2. For integration, we just need point-by-point text in the "Testing instructions" field right in this issue here. This just ensures that things are working as expected. For a whole module like this just describe the major basic functionality, with some example LTI resources to connect to.

          3. For our QA testing cycle before each major release, we need a series of functional tests created in the QA area. Here are examples for Forum: http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDLQA+AND+component+%3D+Forum+AND+status+%3D+Open+ORDER+BY+priority+DESC&mode=hide

          What we do is clone MDLQA-1 and all its subtasks before each QA cycle (the next one starts in a week or so), then a team works to confirm them all.

          For LTI we need a test for each "feature" from a user point of view. You won't have access to create them there directly, but if you could write up a text file and attach it here to make it easy to cut and paste them into new MDLQA issues that would be appreciated.

          Show
          Martin Dougiamas added a comment - Sorry, those wiki pages are a little out of date now. Michael D is working on a new developer wiki which will clean up those old pages and separate them from the main docs, it will be at http://docs.moodle.org/dev soon. There's three types of testing you need to think about: 1. Code tests. Your module should have some unit tests implemented to test specific functions in the code. See http://docs.moodle.org/20/en/Development:Unit_tests The exact tests are up to you, but it would be best to go for core functions that everything relies on. 2. For integration, we just need point-by-point text in the "Testing instructions" field right in this issue here. This just ensures that things are working as expected. For a whole module like this just describe the major basic functionality, with some example LTI resources to connect to. 3. For our QA testing cycle before each major release, we need a series of functional tests created in the QA area. Here are examples for Forum: http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+MDLQA+AND+component+%3D+Forum+AND+status+%3D+Open+ORDER+BY+priority+DESC&mode=hide What we do is clone MDLQA-1 and all its subtasks before each QA cycle (the next one starts in a week or so), then a team works to confirm them all. For LTI we need a test for each "feature" from a user point of view. You won't have access to create them there directly, but if you could write up a text file and attach it here to make it easy to cut and paste them into new MDLQA issues that would be appreciated.
          Hide
          Nikolas Galanis added a comment -

          I have just uploaded the newest version of the code.

          I have fixed the indentation issues, I have removed the excess "global $DB" declarations and I have defined a minimum tool height of 400 just in case a tool instance is setup without height or a height of "0".

          Show
          Nikolas Galanis added a comment - I have just uploaded the newest version of the code. I have fixed the indentation issues, I have removed the excess "global $DB" declarations and I have defined a minimum tool height of 400 just in case a tool instance is setup without height or a height of "0".
          Hide
          Martin Dougiamas added a comment -

          Sam, the module you want is self-contained and can be dropped into a 2.1 in the mod directory.

          It's inside the zip in the folder "IMS Basic LTI for Moodle/Moodle 2/mod/basiclti"

          I've not tested it anywhere near enough yet, sorry Have a look though.

          Show
          Martin Dougiamas added a comment - Sam, the module you want is self-contained and can be dropped into a 2.1 in the mod directory. It's inside the zip in the folder "IMS Basic LTI for Moodle/Moodle 2/mod/basiclti" I've not tested it anywhere near enough yet, sorry Have a look though.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just been having a look over this, its looking real cool.
          I'm not aware of the history or decisions about this issue so hopefully I haven't missed the point of my involvement.

          I've been flat tack all day however I've read through the main body of code and made some general notes for you.
          It looks like alot but hopefully majority of them will be trivial to fix/tidy up.

          General things:

          • Misc white space issues - Tim Hunt's whitespace check is a really good way to easily identify and clean up those (https://github.com/timhunt/moodle-local_codechecker)
          • Remove the closing PHP tags on files (?>)
          • Files that arn't directly accessed need `defined('MOODLE_INTERNAL') || die();` or equiv
          • Variable names shouldn't contain underscores.
          • Requiring files after config.php should make use of $CFG->dirroot
          • error() function is deprecated, you should throw exceptions instead, or in the case of failing DB queries you can use the MUST_EXIST arg if you choose.
          • calls to require_login where you have the course object should pass the whole object not just course id, this avoids an unnessecary DB query
          • Course formats have callbacks now for getting their strings (noted $strweek & $strtopic in index.php)
          • There's alot of functions in lib.php that are still marked TODO - some assessment of these would need to be completed before this is ready to go in.
          • Functions need to be checked for unused globals

          More specific:

          • OAuth.php and TrivialStore.php appear to be coded differently to everything else, is there history behind them? (coding style, licensing etc anything we should know)
          • lib.php What is $basiclti_CONSTANT a quick grep didnt reveal its purpose.
          • lib.php Database syntax improvements:
            • ln94 No need to check that the insert succeeded, it returns the new id or throws an exception
            • ln155 Delete returns true of throws exception - no need to use $result or test success of delete.
            • ln285 Incorrect conversion of record_exists (only takes 2 args now)
          • lib.php ln341 use of deprecated object class, use stdClass instead now
          • Direct access of $_POST should be avoided at all costs where possible. Noted in lib.php::basiclti_submissions
          • Database queries should use always use params instead of directly writing vals into SQL, noted in basiclti_submissions (queries in there to get gradeitemid and gradeentry could just be using DB->get_record as the select is basic conditions)
          • Many of the functions within lib.php need their php docs updated.
          • locallib.php should not be using $COURSE but $PAGE->course - better yet the course should be passed in as an argument to the function (I see there is also instance->course...)
          • locallib.php split_custom_parameters and map_keyname need to be converted to use textlib for substr and strtolower operations to make avoid multi lang probs. I also not sure about the naming/location of these functions but that would be a trivial matter to look at closer to the time of integration.
          • locallib.php::basiclti_get_ims_role needs more work/thought renaming roles/adding new roles is common and most certainly the returned string should be coming from the language packs not hardcoded.
          • locallib.php::basiclti_filter_print_types Don't use $USER->sesskey directly call sesskey() method instead to ensure one exists. also center tag is deprecated.
          • locallib.php::basiclti_delete_type should be wrapped in a transaction
          • locallib.php::basiclti_get_type_config_from_instance missing $type = new stdClass;
          • Functions for producing output should be moved to a renderer.
          • locallib.php::postLaunchHTML
            • Function should be renamed as per coding style
            • Inline JS script needs to be removed and included through a proper JS inclusion.
            • Inline JS actions (href="javascript:...) should be removed and instead events attached through JS
          • service.php - This file looks interesting but still needs alot of work, most importantly security and conversion to 2.0 standards. This looks like it would be the perfect place to implement and utilise a custom renderer for the xml styled output.
          • settings.php - Probably needs to be an external page to avoid all that processing when generating the admin tree - plus conversion to 2.0 Api's
          • styles.php just needs to be removed
          • typessettings.php would be great to see the forms converted to mforms (if settings.php were a admin_externalpage would it be possible to combine this perhaps?)

          Also just glancing over this I have a few questions: (I'm real pressed for time otherwise I would've looked into it myself sorry)

          1. Does this still work with JS disabled?
          2. What browsers has this been tested on (particually keen to know about IE testing).

          (This isn't a full review sorry just as far as I could get in the limited time I had)

          Hope that is all useful, as I said earlier I'm not familiar with the history of this issue myself but it looks like its heading in a good direction.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been having a look over this, its looking real cool. I'm not aware of the history or decisions about this issue so hopefully I haven't missed the point of my involvement. I've been flat tack all day however I've read through the main body of code and made some general notes for you. It looks like alot but hopefully majority of them will be trivial to fix/tidy up. General things: Misc white space issues - Tim Hunt's whitespace check is a really good way to easily identify and clean up those ( https://github.com/timhunt/moodle-local_codechecker ) Remove the closing PHP tags on files (?>) Files that arn't directly accessed need `defined('MOODLE_INTERNAL') || die();` or equiv Variable names shouldn't contain underscores. Requiring files after config.php should make use of $CFG->dirroot error() function is deprecated, you should throw exceptions instead, or in the case of failing DB queries you can use the MUST_EXIST arg if you choose. calls to require_login where you have the course object should pass the whole object not just course id, this avoids an unnessecary DB query Course formats have callbacks now for getting their strings (noted $strweek & $strtopic in index.php) There's alot of functions in lib.php that are still marked TODO - some assessment of these would need to be completed before this is ready to go in. Functions need to be checked for unused globals More specific: OAuth.php and TrivialStore.php appear to be coded differently to everything else, is there history behind them? (coding style, licensing etc anything we should know) lib.php What is $basiclti_CONSTANT a quick grep didnt reveal its purpose. lib.php Database syntax improvements: ln94 No need to check that the insert succeeded, it returns the new id or throws an exception ln155 Delete returns true of throws exception - no need to use $result or test success of delete. ln285 Incorrect conversion of record_exists (only takes 2 args now) lib.php ln341 use of deprecated object class, use stdClass instead now Direct access of $_POST should be avoided at all costs where possible. Noted in lib.php::basiclti_submissions Database queries should use always use params instead of directly writing vals into SQL, noted in basiclti_submissions (queries in there to get gradeitemid and gradeentry could just be using DB->get_record as the select is basic conditions) Many of the functions within lib.php need their php docs updated. locallib.php should not be using $COURSE but $PAGE->course - better yet the course should be passed in as an argument to the function (I see there is also instance->course...) locallib.php split_custom_parameters and map_keyname need to be converted to use textlib for substr and strtolower operations to make avoid multi lang probs. I also not sure about the naming/location of these functions but that would be a trivial matter to look at closer to the time of integration. locallib.php::basiclti_get_ims_role needs more work/thought renaming roles/adding new roles is common and most certainly the returned string should be coming from the language packs not hardcoded. locallib.php::basiclti_filter_print_types Don't use $USER->sesskey directly call sesskey() method instead to ensure one exists. also center tag is deprecated. locallib.php::basiclti_delete_type should be wrapped in a transaction locallib.php::basiclti_get_type_config_from_instance missing $type = new stdClass; Functions for producing output should be moved to a renderer. locallib.php::postLaunchHTML Function should be renamed as per coding style Inline JS script needs to be removed and included through a proper JS inclusion. Inline JS actions (href="javascript:...) should be removed and instead events attached through JS service.php - This file looks interesting but still needs alot of work, most importantly security and conversion to 2.0 standards. This looks like it would be the perfect place to implement and utilise a custom renderer for the xml styled output. settings.php - Probably needs to be an external page to avoid all that processing when generating the admin tree - plus conversion to 2.0 Api's styles.php just needs to be removed typessettings.php would be great to see the forms converted to mforms (if settings.php were a admin_externalpage would it be possible to combine this perhaps?) Also just glancing over this I have a few questions: (I'm real pressed for time otherwise I would've looked into it myself sorry) Does this still work with JS disabled? What browsers has this been tested on (particually keen to know about IE testing). (This isn't a full review sorry just as far as I could get in the limited time I had) Hope that is all useful, as I said earlier I'm not familiar with the history of this issue myself but it looks like its heading in a good direction. Cheers Sam
          Hide
          Nikolas Galanis added a comment -

          Sam,

          thanks a lot for the comments. I will start working on them right away.

          Please keep in mind that the best way to get the code is through our mercurial repository (hg) as explained in:

          http://code.google.com/p/basiclti4moodle/source/checkout

          The zip file is not always up to date.

          OAuth.php and TrivialStore.php have been taken by a generic basicLTI implementation from IMS. That explains the different code style.

          Short answers:
          1. I have tested it on Firefox with JS disabled and it is working fine.
          2. We are developing and testing using Firefox. Unfortunately we have done little testing outside FF. We know that there is a problem with the object not displaying in Chrome. We will be doing tests on Safari promptly and I will check it on IE tonight at home, since it seems I am the only Windows fanboy in our group.

          Regards,
          Nikolas

          Show
          Nikolas Galanis added a comment - Sam, thanks a lot for the comments. I will start working on them right away. Please keep in mind that the best way to get the code is through our mercurial repository (hg) as explained in: http://code.google.com/p/basiclti4moodle/source/checkout The zip file is not always up to date. OAuth.php and TrivialStore.php have been taken by a generic basicLTI implementation from IMS. That explains the different code style. Short answers: 1. I have tested it on Firefox with JS disabled and it is working fine. 2. We are developing and testing using Firefox. Unfortunately we have done little testing outside FF. We know that there is a problem with the object not displaying in Chrome. We will be doing tests on Safari promptly and I will check it on IE tonight at home, since it seems I am the only Windows fanboy in our group. Regards, Nikolas
          Hide
          Sam Hemelryk added a comment -

          Ahh thanks for pointing it out about the zip Nikolas.

          Feel free to ask if you have any questions about the things I noted, otherwise I look forward to seeing this evolve.

          In regards to the testing thats OK I was just asking out of curiosity - certainly it is something that can be worked on when the code is getting to be stable.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ahh thanks for pointing it out about the zip Nikolas. Feel free to ask if you have any questions about the things I noted, otherwise I look forward to seeing this evolve. In regards to the testing thats OK I was just asking out of curiosity - certainly it is something that can be worked on when the code is getting to be stable. Cheers Sam
          Hide
          Martin Dougiamas added a comment -

          Nikolas, the code from IMS - is it GPL-compatible?

          Show
          Martin Dougiamas added a comment - Nikolas, the code from IMS - is it GPL-compatible?
          Hide
          Charles Severance added a comment -

          Martin - It is Apache 2. But I am pretty sure I can change the license since (a) I wrote the code, and (b) IMS wants folks to reuse this code. Luckily the OAuth code is MIT. Maybe I should just change the IMS code to be MIT - I will check and be back.

          Show
          Charles Severance added a comment - Martin - It is Apache 2. But I am pretty sure I can change the license since (a) I wrote the code, and (b) IMS wants folks to reuse this code. Luckily the OAuth code is MIT. Maybe I should just change the IMS code to be MIT - I will check and be back.
          Hide
          Nikolas Galanis added a comment -

          Just uploaded today's work. Did a lot of cleanup thanks to Tim Hunt's tool.

          I am currently looking into eliminating the underscores in variable names.

          Show
          Nikolas Galanis added a comment - Just uploaded today's work. Did a lot of cleanup thanks to Tim Hunt's tool. I am currently looking into eliminating the underscores in variable names.
          Hide
          Martin Dougiamas added a comment -

          Apache 2 is OK, it's compatible with GPLv3. http://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses

          The MIT License (X11 License) is also compatible.

          Nicolas, can you please make sure there is an appropriate license notice in every file in this module?

          Show
          Martin Dougiamas added a comment - Apache 2 is OK, it's compatible with GPLv3. http://www.gnu.org/licenses/license-list.html#GPLCompatibleLicenses The MIT License (X11 License) is also compatible. Nicolas, can you please make sure there is an appropriate license notice in every file in this module?
          Hide
          Charles Severance added a comment -

          Thanks Martin. I will continue to move toward making the IMS code MIT - but having 2.0 as GPL3 makes it easy.

          Nikolas - When you add the license to the files from IMS - add it as Copyright Apache 2 IMS since I don't yet have permission to change it to MIT - so the copy we have is copyright Apache 2 - IMS.

          Show
          Charles Severance added a comment - Thanks Martin. I will continue to move toward making the IMS code MIT - but having 2.0 as GPL3 makes it easy. Nikolas - When you add the license to the files from IMS - add it as Copyright Apache 2 IMS since I don't yet have permission to change it to MIT - so the copy we have is copyright Apache 2 - IMS.
          Hide
          Nikolas Galanis added a comment -

          I uploaded the newest version.

          I have changed the licenses in the two files as per our email conversation. However, I would appreciate it if you could check them out since I am completely inexperienced in licensing protocol. I am pretty sure it will need some changes.

          Apart from that, I am shaping up the code as per Sam's suggestions.

          Nikolas

          Show
          Nikolas Galanis added a comment - I uploaded the newest version. I have changed the licenses in the two files as per our email conversation. However, I would appreciate it if you could check them out since I am completely inexperienced in licensing protocol. I am pretty sure it will need some changes. Apart from that, I am shaping up the code as per Sam's suggestions. Nikolas
          Hide
          Ludo ( Marc Alier) added a comment -

          Niko, on the files where chuck has worked on he should appear as @author

          Show
          Ludo ( Marc Alier) added a comment - Niko, on the files where chuck has worked on he should appear as @author
          Hide
          Charles Severance added a comment -

          This is my first cut of a test plan written and uploaded from Delta flight 1691

          Comments on format, style, or content welcome.

          Show
          Charles Severance added a comment - This is my first cut of a test plan written and uploaded from Delta flight 1691 Comments on format, style, or content welcome.
          Hide
          Martin Dougiamas added a comment -

          Guys, this really needs to be integrated today or it's not going to make the 2.1 release. How's it looking?

          Show
          Martin Dougiamas added a comment - Guys, this really needs to be integrated today or it's not going to make the 2.1 release. How's it looking?
          Hide
          Ludo ( Marc Alier) added a comment -

          Hi,
          Niko tells me that all the bugs and issues that Sam pointed out are fixed in our mercurial at code.google.etc... Only remain two minor issues of code style such as getting rid of inline javascript.

          Show
          Ludo ( Marc Alier) added a comment - Hi, Niko tells me that all the bugs and issues that Sam pointed out are fixed in our mercurial at code.google.etc... Only remain two minor issues of code style such as getting rid of inline javascript.
          Hide
          Nikolas Galanis added a comment - - edited

          Some more details about which of Sam's suggestions are yet to be implemented.

          service.php and settings.php have not been changed yet.
          Functions producing output have still not been moved to a renderer.
          Inline JS has not been moved to a separate js file

          I think that roughly everything else is done. I am not 100% certain because today is a holiday in Spain and I have my todo checklist at the office.

          A couple of comments off the top of my head:
          . Direct access to $_POST in lib.php::basiclti_submissions is due to this function following the coding of the corresponding one in mod/assignment
          . The custom parameters handled in split_custom_parameters need some more testing because I am still having some trouble with non-international characters like the ñ.

          Show
          Nikolas Galanis added a comment - - edited Some more details about which of Sam's suggestions are yet to be implemented. service.php and settings.php have not been changed yet. Functions producing output have still not been moved to a renderer. Inline JS has not been moved to a separate js file I think that roughly everything else is done. I am not 100% certain because today is a holiday in Spain and I have my todo checklist at the office. A couple of comments off the top of my head: . Direct access to $_POST in lib.php::basiclti_submissions is due to this function following the coding of the corresponding one in mod/assignment . The custom parameters handled in split_custom_parameters need some more testing because I am still having some trouble with non-international characters like the ñ.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          grrr, holiday in Spain, and I didn't notice it, grrr! :-P :-D

          This is just to share my own opinion about this:

          1) It's looking great, 100%. And this is not a kiss for a kill (in the next points).
          2) But I still see it as sort of "work in progress"
          3) There are some bits like the oauth integration (that we have been considering for some time and perhaps should be addressed for some upcoming release in a central-way), plus some of the points commented by Sam above.
          4) We are running out of time, really, for 2.1!

          So, my +0.5 goes for:

          1) Keep that module as contrib one for the life of 2.1
          2) Integrate it into 2.2dev around, say, 1 month after 2.1 release.

          That way people could play with it more and more and, at the same time the oauth thing can be reconsidered, all the security reviewed and the module really polished.

          Of course, MD has the final decision, just sharing my personal thoughts about to include it for 2.1. Brain won this time, sorry!

          Ciao and thanks for the phenomenal stuff,

          Show
          Eloy Lafuente (stronk7) added a comment - grrr, holiday in Spain, and I didn't notice it, grrr! :-P :-D This is just to share my own opinion about this: 1) It's looking great, 100%. And this is not a kiss for a kill (in the next points). 2) But I still see it as sort of "work in progress" 3) There are some bits like the oauth integration (that we have been considering for some time and perhaps should be addressed for some upcoming release in a central-way), plus some of the points commented by Sam above. 4) We are running out of time, really, for 2.1! So, my +0.5 goes for: 1) Keep that module as contrib one for the life of 2.1 2) Integrate it into 2.2dev around, say, 1 month after 2.1 release. That way people could play with it more and more and, at the same time the oauth thing can be reconsidered, all the security reviewed and the module really polished. Of course, MD has the final decision, just sharing my personal thoughts about to include it for 2.1. Brain won this time, sorry! Ciao and thanks for the phenomenal stuff,
          Hide
          Martin Dougiamas added a comment -

          Yep, going to push this back a bit, sorry guys. We are just so flat out here finishing what we have.

          However:

          1) Yes, let's put this in master soon after we branch 2.1 so that it can be polished there for 2.2 and
          2) I'd like to feature it on the new plugins database, coming soon, as a module that easily be dropped into Moodle 2.1.

          Show
          Martin Dougiamas added a comment - Yep, going to push this back a bit, sorry guys. We are just so flat out here finishing what we have. However: 1) Yes, let's put this in master soon after we branch 2.1 so that it can be polished there for 2.2 and 2) I'd like to feature it on the new plugins database, coming soon, as a module that easily be dropped into Moodle 2.1.
          Hide
          Mark Drechsler added a comment -

          Just wondering if this is still on track to make it into 2.2 - comments anyone?

          Show
          Mark Drechsler added a comment - Just wondering if this is still on track to make it into 2.2 - comments anyone?
          Hide
          James Strong added a comment -

          I saw the following in an announcement from IMS recently:

          "If all goes as hoped, with the next release of BLTI (estimated October 1) it will simply be LTI v1.1. Included in that release will be formal conformance support for outcomes. We anticipate that in September the Common Cartridge/LTI Accredited Profile Management Group (APMG) will begin taking up the further evolution of the one LTI, drawing from the detailed technical work accomplished in the Full LTI workgroup (led by Blackboard and Pearson). From that point forward there will no longer be a BLTI or a Full LTI - only LTI."

          Perhaps this could be put on the 2.3 roadmap - I imagine there wouldn't be adequate time to get this into 2.2.

          Show
          James Strong added a comment - I saw the following in an announcement from IMS recently: "If all goes as hoped, with the next release of BLTI (estimated October 1) it will simply be LTI v1.1. Included in that release will be formal conformance support for outcomes. We anticipate that in September the Common Cartridge/LTI Accredited Profile Management Group (APMG) will begin taking up the further evolution of the one LTI, drawing from the detailed technical work accomplished in the Full LTI workgroup (led by Blackboard and Pearson). From that point forward there will no longer be a BLTI or a Full LTI - only LTI." Perhaps this could be put on the 2.3 roadmap - I imagine there wouldn't be adequate time to get this into 2.2.
          Hide
          Charles Severance added a comment -

          I think that the best way to look at this is supporting at least BLTI 1.0 in 2.2 and shooting for BLTI 1.1 in 2.3. BLTI 1.1 is a simple scope and working prototype code and not technically challenging and already has nice, complete draft - but it needs to be reviewed, etc.

          I think MoodleRooms is getting involved in making long term plans to support Basic LTI in Moodle core going forward as the spec evolves.

          Show
          Charles Severance added a comment - I think that the best way to look at this is supporting at least BLTI 1.0 in 2.2 and shooting for BLTI 1.1 in 2.3. BLTI 1.1 is a simple scope and working prototype code and not technically challenging and already has nice, complete draft - but it needs to be reviewed, etc. I think MoodleRooms is getting involved in making long term plans to support Basic LTI in Moodle core going forward as the spec evolves.
          Hide
          Curtis Fornadley added a comment -

          Here at UCLA we are on Moodle 1.9.8 and plan to fully migrate to Moodle 2.2 by June 2012

          As part of an online instruction pilot the University of California is exploring the use of LTI as a way to connect activities from Moodle and Sakai into Sakai OAE. (the UC campuses are basically half Moodle, half Sakai)

          This would require Moodle to be a producer of LTI not just a consumer.
          Is Moodle as an LTI producer is on the Moodle road map, if so what version?

          As I understand it LIS is needed to send grading information from the producer back to the consumer. Is this included in the planned implementation?

          Thanks for any info

          Show
          Curtis Fornadley added a comment - Here at UCLA we are on Moodle 1.9.8 and plan to fully migrate to Moodle 2.2 by June 2012 As part of an online instruction pilot the University of California is exploring the use of LTI as a way to connect activities from Moodle and Sakai into Sakai OAE. (the UC campuses are basically half Moodle, half Sakai) This would require Moodle to be a producer of LTI not just a consumer. Is Moodle as an LTI producer is on the Moodle road map, if so what version? As I understand it LIS is needed to send grading information from the producer back to the consumer. Is this included in the planned implementation? Thanks for any info
          Hide
          Tim Hunt added a comment -

          Search the forums at moodle.org (I think particularly the General developer forum). There has been some talk about it, but rather miscellaneous. The use case for Moodle as an LTI producer is less compelling than Moodle as an LTI consumer.

          There are plenty of issues to work out how they should be handled, primarily enrolment, I think. I expect it will require several people to knock-up proofs of concept (I expect you can do it as a local plugin) before we could find a solution that is good enough to go into core. Actually, since it is slightly a fringe requirement, it might be best to keep it as a local plugin, not in core.

          Show
          Tim Hunt added a comment - Search the forums at moodle.org (I think particularly the General developer forum). There has been some talk about it, but rather miscellaneous. The use case for Moodle as an LTI producer is less compelling than Moodle as an LTI consumer. There are plenty of issues to work out how they should be handled, primarily enrolment, I think. I expect it will require several people to knock-up proofs of concept (I expect you can do it as a local plugin) before we could find a solution that is good enough to go into core. Actually, since it is slightly a fringe requirement, it might be best to keep it as a local plugin, not in core.
          Hide
          Charles Severance added a comment -

          Curtis, there is a lot of diffuse interest in Moodle as a LTI provider. There have been several prototypes that have been built. I see Tim's point that it is not a compelling use case from the perspective of the core Moodle team. I think there is an extremely compelling use case for users like yourself and for services like MoodleRooms and RemoteLearner. I have some code that is a start on the take but it is very incomplete. I do not have the time to polish it or improve it so I am not going to publish it until there is someone who will take responsibility for the code. I think that it would make a great Google Code project but someone needs to lead it.

          Show
          Charles Severance added a comment - Curtis, there is a lot of diffuse interest in Moodle as a LTI provider. There have been several prototypes that have been built. I see Tim's point that it is not a compelling use case from the perspective of the core Moodle team. I think there is an extremely compelling use case for users like yourself and for services like MoodleRooms and RemoteLearner. I have some code that is a start on the take but it is very incomplete. I do not have the time to polish it or improve it so I am not going to publish it until there is someone who will take responsibility for the code. I think that it would make a great Google Code project but someone needs to lead it.
          Hide
          Martin Dougiamas added a comment -

          What I'd like to see is some actual detailed use cases explaining how security works.

          For example, how would LTI be added to a quiz activity, say, in a way that a student in a Sakai course can usefully take it?

          Show
          Martin Dougiamas added a comment - What I'd like to see is some actual detailed use cases explaining how security works. For example, how would LTI be added to a quiz activity, say, in a way that a student in a Sakai course can usefully take it?
          Hide
          Charles Severance added a comment -

          Actually, I think that Basic LTI Provider at the level of a per-tool is a rather limited use case. The per-class use case is much more solid. Imagine a school that is running Blackboard as their "enterprise" LMS all plugged into their SIS, etc. A few faculty might want to teach a particular course course in Moodle by putting a LTI link from their enterprise LMS to a server running Moodle with Basic LTI Provider.

          The roster is effectively transferred from Blackboard to Moodle along with roles, etc. Then the teacher can teach their class in Moodle, using only Moodle. And when we get LTI 1.1 support grades sent back, the teacher could compute a final grade column in Moodle and send it back to Blackboard for transfer to the SIS. In essence the faculty member has a minimal course in Blackboard with (a) a grade book, and (b) a link to Moodle. They do all their teaching in Moodle. Here is a demonstration of a very hacked up Provider for Blackboard where we plugged a Blackboard "course" into Sakai.

          http://www.vimeo.com/26310497

          That is just a rough demo and does not represent any kind of product commitment.

          I think that the per-tool use case is more subtle and harder to get right. But it can be made to work. Sakai does this OK - but there is no grade flow. I think a key key concept is that not all tools are suitable to be used outside the context of all their sibling other tools in Moodle. And grade flow is a little tricky.

          So in my view of the world - the first use case to address is the "whole course" use case - as that is (a) easiest (b) does not require any pedagogy adjustments, (c) keeps a coherent Moodle UI, and (d) gets more people to be able to conveniently use Moodle regardless of the institutional choice of LMS. I think this is particularly powerful for MoodleRooms and RemoteLearner who could sell single-course instances of Moodle provisioned by LTI to teachers forced to use an LMS other than Moodle by at their institution It is like an "escape hatch" that leads to Moodle.

          I am happy to talk more or answer any questions

          Show
          Charles Severance added a comment - Actually, I think that Basic LTI Provider at the level of a per-tool is a rather limited use case. The per-class use case is much more solid. Imagine a school that is running Blackboard as their "enterprise" LMS all plugged into their SIS, etc. A few faculty might want to teach a particular course course in Moodle by putting a LTI link from their enterprise LMS to a server running Moodle with Basic LTI Provider. The roster is effectively transferred from Blackboard to Moodle along with roles, etc. Then the teacher can teach their class in Moodle, using only Moodle. And when we get LTI 1.1 support grades sent back, the teacher could compute a final grade column in Moodle and send it back to Blackboard for transfer to the SIS. In essence the faculty member has a minimal course in Blackboard with (a) a grade book, and (b) a link to Moodle. They do all their teaching in Moodle. Here is a demonstration of a very hacked up Provider for Blackboard where we plugged a Blackboard "course" into Sakai. http://www.vimeo.com/26310497 That is just a rough demo and does not represent any kind of product commitment. I think that the per-tool use case is more subtle and harder to get right. But it can be made to work. Sakai does this OK - but there is no grade flow. I think a key key concept is that not all tools are suitable to be used outside the context of all their sibling other tools in Moodle. And grade flow is a little tricky. So in my view of the world - the first use case to address is the "whole course" use case - as that is (a) easiest (b) does not require any pedagogy adjustments, (c) keeps a coherent Moodle UI, and (d) gets more people to be able to conveniently use Moodle regardless of the institutional choice of LMS. I think this is particularly powerful for MoodleRooms and RemoteLearner who could sell single-course instances of Moodle provisioned by LTI to teachers forced to use an LMS other than Moodle by at their institution It is like an "escape hatch" that leads to Moodle. I am happy to talk more or answer any questions
          Hide
          Charles Severance added a comment -

          Martin - one more thing. I have GPL code from Jenzabar that does the course use case. Jenzabar has a student system/portal and an LMS product. Not all of their SIS/portal customers purchase their LMS. Some customers use the Jenzabar portal with Moodle as the LMS, and some use e-Racer (from Jenzabar) as their LMS - and still other schools have both and let teachers pick which LMS (Moodle or Jenzabar) they want for each of the courses they teach. Since it is all coming from a single portal the two LMS's live quite nicely side by side. Pretty cool.

          Show
          Charles Severance added a comment - Martin - one more thing. I have GPL code from Jenzabar that does the course use case. Jenzabar has a student system/portal and an LMS product. Not all of their SIS/portal customers purchase their LMS. Some customers use the Jenzabar portal with Moodle as the LMS, and some use e-Racer (from Jenzabar) as their LMS - and still other schools have both and let teachers pick which LMS (Moodle or Jenzabar) they want for each of the courses they teach. Since it is all coming from a single portal the two LMS's live quite nicely side by side. Pretty cool.
          Hide
          Niall Barr added a comment -

          I've also created a BasicLTI based block that allows links straight into Moodle courses. It's specific to our needs at Glasgow, so assumes all the Moodle's that are linked share an LDAP authentication system. It also includes a REST web sevice with LTI style authentication to get a list of a users courses, so each Moodle can list the courses on all other Moodles. We also plan to use it to allow links from Sharepoint into Moodle.

          I've not shared it yet because it will need a bit more work to make it suitable for other Moodle setups, however I'll be happy to share it once it's been tidyed up a bit.

          Show
          Niall Barr added a comment - I've also created a BasicLTI based block that allows links straight into Moodle courses. It's specific to our needs at Glasgow, so assumes all the Moodle's that are linked share an LDAP authentication system. It also includes a REST web sevice with LTI style authentication to get a list of a users courses, so each Moodle can list the courses on all other Moodles. We also plan to use it to allow links from Sharepoint into Moodle. I've not shared it yet because it will need a bit more work to make it suitable for other Moodle setups, however I'll be happy to share it once it's been tidyed up a bit.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi,

          here it's the document where the (still ongoing) review of the IMS-LTI consumer module is happening. Feel free to comment, using the numbers (A2, B3..) to reference any point there.

          http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11

          More coming today...ciao

          Edited: LOL, forgot the link!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi, here it's the document where the (still ongoing) review of the IMS-LTI consumer module is happening. Feel free to comment, using the numbers (A2, B3..) to reference any point there. http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11 More coming today...ciao Edited: LOL, forgot the link!
          Hide
          Martin Dougiamas added a comment -

          Note we are working from the 2.2 version done by Moodlerooms, based on the one submitted for 2.1.

          Show
          Martin Dougiamas added a comment - Note we are working from the 2.2 version done by Moodlerooms, based on the one submitted for 2.1.
          Hide
          Charles Severance added a comment -

          Cool. I hope the one from MoodleRooms is called "lti" and not "basiclti". We slowed prograss on the basiclti4moodle version once the MoodleRooms version picked up. SO I am glad to hear it and really looking forward to seeing it in action. Please let us know if there is some repo that has it - I will hop on and test it / run through certification as soon as I know where it is.

          Show
          Charles Severance added a comment - Cool. I hope the one from MoodleRooms is called "lti" and not "basiclti". We slowed prograss on the basiclti4moodle version once the MoodleRooms version picked up. SO I am glad to hear it and really looking forward to seeing it in action. Please let us know if there is some repo that has it - I will hop on and test it / run through certification as soon as I know where it is.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note in the review above the first point is about to rename it to "imslti" (to keep it sharing nomenclature with the already existing "imscp" module, mainly). But yes, I think the "basic" is out from the equation in any case.

          About the repo, I'm contacting Chris Scribner (Moodlerooms) about to move it to public space to have more eyes on it. In any case we are aiming to integrate it next week (if all the requirements in the review are fulfilled), you I guess it won't be a long wait.

          We really need to know how/when/where all those changes are going to be implemented, grrr, Fridays!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Note in the review above the first point is about to rename it to "imslti" (to keep it sharing nomenclature with the already existing "imscp" module, mainly). But yes, I think the "basic" is out from the equation in any case. About the repo, I'm contacting Chris Scribner (Moodlerooms) about to move it to public space to have more eyes on it. In any case we are aiming to integrate it next week (if all the requirements in the review are fulfilled), you I guess it won't be a long wait. We really need to know how/when/where all those changes are going to be implemented, grrr, Fridays! Ciao
          Hide
          Charles Severance added a comment -

          Thanks - imslti is perfect when it goes in, I will come buy you a beer

          Show
          Charles Severance added a comment - Thanks - imslti is perfect when it goes in, I will come buy you a beer
          Hide
          Chris Scribner added a comment -

          Thanks for putting the review together. The timing isn't great, as I'm in Japan now and travelling back to the US next Monday, then going to a Moodlerooms company retreat the next day. Is there any way we could push out the integration by a week to give me time to get through the changes?

          I'll work on getting the code into a public repository on Monday.

          Here's my initial reaction to the list:

          A.

          1. The name is currently "lti", which we're calling "External Tool" in the UI (this seems to be the new trend). I'll change the module name to "imslti" as requested.
          2. This module is standalone from any other previous LTI-like modules.
          3. Ok.
          4. There's a backstory here. The basiclti module (which gave birth to this module) includes the same OAuth libraries. So, when both modules were included in a site, it would totally break due to name conflicts. If you don't want me to use namespaces to fix the conflict, can you suggest another reasonable approach?
          5. Sorry to launch into a programmer holy war, but I'm surprised you considered this "abuse". In my opinion here/nowdoc increase readability and ease-of-editing for multiline strings. I understand they're uncommon as they're newer language elements, but I think they were introduced into PHP for a reason I also understand the value of following coding standards, so I leave it in your hands.

          Do you want me to change all use of here/nowdoc to string concats (is there another way?), or just the ones in the lang file?

          B.

          1. Ok
          2. Are there any specific recommendations for changes in this file?
          3. Would it be acceptable if I moved the patch out of pix_url, and into a "higher" layer of code? For instance, I think the code that calls pix_url can check if the url contains :// and call it or not. I'll look into how the file module does it... I didn't realize there was already anything like this.
          4. Thanks for pointing this out. I don't exactly know why there would be an issue as the json gets form URL encoded when posted (tests with another system went ok). I'll look into it and most likely switch the format to be safe.
          5. The on-page help text provided (the ( ? ) icons) attempts to answer the questions you asked, but it's probably not enough. Some high level documentation for the module from an instructor and administrator point of view are likely needed.

          C.

          3. Getting the size of the embedded content to take up the available space without adding double scroll bars in all themes was extremely challenging. You found a trade-off I had to make, which was disabling the scroll bars in the main Moodle content, which is a little funky if blocks extend down the page. That's why the default launch container is "Embed, without blocks".
          4. I didn't understand this comment.

          And, there's a few things I wanted to finish up:

          1. Add start/end dates for when the lti item can be launched. This will be useful in the case that the lti item points to an external quiz.
          2. I saw an issue recently with a name collision on OAuthResponse when entering course edit mode if both lti and basiclti were installed.
          3. Test completion options – mainly that "on grade" and "on view" work
          4. New files I added don't have "comment headers" yet. I wasn't completely sure what to put there. I also need to add MoodleRooms to the list of contributes for the other files.

          Once again, thanks for devoting time to this project!

          Chris Scribner

          Show
          Chris Scribner added a comment - Thanks for putting the review together. The timing isn't great, as I'm in Japan now and travelling back to the US next Monday, then going to a Moodlerooms company retreat the next day. Is there any way we could push out the integration by a week to give me time to get through the changes? I'll work on getting the code into a public repository on Monday. Here's my initial reaction to the list: A. 1. The name is currently "lti", which we're calling "External Tool" in the UI (this seems to be the new trend). I'll change the module name to "imslti" as requested. 2. This module is standalone from any other previous LTI-like modules. 3. Ok. 4. There's a backstory here. The basiclti module (which gave birth to this module) includes the same OAuth libraries. So, when both modules were included in a site, it would totally break due to name conflicts. If you don't want me to use namespaces to fix the conflict, can you suggest another reasonable approach? 5. Sorry to launch into a programmer holy war, but I'm surprised you considered this "abuse". In my opinion here/nowdoc increase readability and ease-of-editing for multiline strings. I understand they're uncommon as they're newer language elements, but I think they were introduced into PHP for a reason I also understand the value of following coding standards, so I leave it in your hands. Do you want me to change all use of here/nowdoc to string concats (is there another way?), or just the ones in the lang file? B. 1. Ok 2. Are there any specific recommendations for changes in this file? 3. Would it be acceptable if I moved the patch out of pix_url, and into a "higher" layer of code? For instance, I think the code that calls pix_url can check if the url contains :// and call it or not. I'll look into how the file module does it... I didn't realize there was already anything like this. 4. Thanks for pointing this out. I don't exactly know why there would be an issue as the json gets form URL encoded when posted (tests with another system went ok). I'll look into it and most likely switch the format to be safe. 5. The on-page help text provided (the ( ? ) icons) attempts to answer the questions you asked, but it's probably not enough. Some high level documentation for the module from an instructor and administrator point of view are likely needed. C. 3. Getting the size of the embedded content to take up the available space without adding double scroll bars in all themes was extremely challenging. You found a trade-off I had to make, which was disabling the scroll bars in the main Moodle content, which is a little funky if blocks extend down the page. That's why the default launch container is "Embed, without blocks". 4. I didn't understand this comment. And, there's a few things I wanted to finish up: 1. Add start/end dates for when the lti item can be launched. This will be useful in the case that the lti item points to an external quiz. 2. I saw an issue recently with a name collision on OAuthResponse when entering course edit mode if both lti and basiclti were installed. 3. Test completion options – mainly that "on grade" and "on view" work 4. New files I added don't have "comment headers" yet. I wasn't completely sure what to put there. I also need to add MoodleRooms to the list of contributes for the other files. Once again, thanks for devoting time to this project! Chris Scribner
          Hide
          Tim Hunt added a comment -

          A5. One of here/nowdoc has been in PHP since forever, it is only the other that is new. However, the Moodle way is just to use multi-line strings like https://github.com/moodle/moodle/blob/master/question/engine/datalib.php#L139. No need for concatenation (as their would be in Java, say).

          Z4. Correct boilerplate comment on files is described here: http://docs.moodle.org/dev/Coding_style#Files

          Show
          Tim Hunt added a comment - A5. One of here/nowdoc has been in PHP since forever, it is only the other that is new. However, the Moodle way is just to use multi-line strings like https://github.com/moodle/moodle/blob/master/question/engine/datalib.php#L139 . No need for concatenation (as their would be in Java, say). Z4. Correct boilerplate comment on files is described here: http://docs.moodle.org/dev/Coding_style#Files
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A.

          1. Oki, so it goes to "imslti", thanks (I'm sorry I saw in your commits already two name changes, lol, so this will be the 3rd). About the "External tool" name, I had in my internal notes that addressed too (it became lost apparently), and I think it's better to name it "IMS LTI" why not? It's exactly that.

          2. So, if we consider this a brand-new module, then surely we don't need: i) any upgrade steps, everybody is going to install it from scratch. ii) any conversion from moodle 1.x backups (I saw one commit from Darko about that). Or do you need any of i) or ii) ?

          3. Oki, if I get some time over the weekend I'll try to sort as much as possible of this and will put one commit on top of you branch (I think I've write perms there).

          4. Well, the solution here is to have one unified library @ core (see C1), so any plugin can use it. I've created MDL-30149 about that. So surely, although we don't use namespaces... until C1 is solved (for Moodle 2.3), using them is the only solution to certainly avoid any conflict. Just if you can put here and there some:

          // TODO: Switch to core oauthlib once implemented - MDL-30149
          

          5. Hehe, no war at all, I've been using here documents since ages ago, specially for shell scripts, were concat operations are horrible. But in Moodle, simply, we don't use them. Yes, there are some uses, but please, ignore them. Specially the lang strings... that stuff goes to AMOS (our system for supporting translations and language packs generation, and I bet it won't "digest" them properly at all). So, for sure, the lang ones must be out, and also the SQLs (using double/single quotes as needed). About the HTML ones, I can live with them if they require variables substitution, but no excuse if they are pure HTML IMO. Once again, if I get some time... I'll try to help here (I love tedious and repetitive tasks).

          B.

          1. Ok

          2. Are there any specific recommendations for changes in this file? Well, I must confess that the bit calling my attention was:

          $rawbody = file_get_contents("php://input");
          

          that basically means it accepts any content. And the point was about to look carefully how we are validating/using/storing that information. So nothing found yet (I'm about to start 2nd, deeper, iteration). I'll keep you informed if I find anything annoying.

          3. I think I was looking @ this some weeks ago and there were not proper solutions for the dynamic icon thing. All that stuff is controlled by our mod_info/cm_info core stuff and it will need to be able to accept "other" icons. But I'm not sure if that "other" should include remote urls or just pulled & locally stored icons. It is something to discuss and I'm 90% sure this won't be possible in Moodle 2.2. I'll try to introduce it @ HQ next week to see which are the feelings about it. Depending of that, perhaps we'll need to comment out some parts of the UI (hopefully re-introducing them ASAP).

          4. Well, perhaps the problem is in the "IMS LTI 1.0 PHP basic provider" itself (the one I used for basic testing), behaving wrongly. I agree that json format itself should be ok/safe. But, if we can make it safer just by using another, simpler, format, or by encoding in another way.. then my +1 goes to make it (we don't know what some providers outa there are going to accept).

          5. Yeah, that would be terrible welcome, I really was not able to get the point beyond that stuff. I guessed is was sort of pre-configured external tools available to all activities, but the "pending/rejected" thing, and the organizations... simply didn't fit in my (limited) brain.

          C.

          1. See A4: MDL-30149

          3. Oki, I think we can survive without those scroll bars (I sounded to me a lot to the fights I had to maintain some years ago in the IMS-CP resource type). So we can close this as "Done!" if nobody objects it.

          4. Yes, what I mean is that I get four options in the "Launch container" menu: ("default", "embed", "embed without blocks" and "new window") and it's not clear at all the meaning of the "default" one (or from where that default comes/which is its current value). That leaded me to think it that something is wrong with it.

          Finally, about the pending things, I'd recommend to, at this time, center our efforts in the "required" stuff. Where required means all we need to get this integrated, aka (A) and as most as possible of (B). Once being there, we'll have 3 more weeks for testing, fixing (B) and add new stuff (C/subtasks).

          Of course, big thanks for your hard work! Let's push, push, push! And happy good trip back from JP!

          I'm back to the 2nd review round (will try to fix trivial coding-rulezz while doing). Anything new found will be documented in the wiki and shared here.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A. 1. Oki, so it goes to "imslti", thanks (I'm sorry I saw in your commits already two name changes, lol, so this will be the 3rd). About the "External tool" name, I had in my internal notes that addressed too (it became lost apparently), and I think it's better to name it "IMS LTI" why not? It's exactly that. 2. So, if we consider this a brand-new module, then surely we don't need: i) any upgrade steps, everybody is going to install it from scratch. ii) any conversion from moodle 1.x backups (I saw one commit from Darko about that). Or do you need any of i) or ii) ? 3. Oki, if I get some time over the weekend I'll try to sort as much as possible of this and will put one commit on top of you branch (I think I've write perms there). 4. Well, the solution here is to have one unified library @ core (see C1), so any plugin can use it. I've created MDL-30149 about that. So surely, although we don't use namespaces... until C1 is solved (for Moodle 2.3), using them is the only solution to certainly avoid any conflict. Just if you can put here and there some: // TODO: Switch to core oauthlib once implemented - MDL-30149 5. Hehe, no war at all, I've been using here documents since ages ago, specially for shell scripts, were concat operations are horrible. But in Moodle, simply, we don't use them. Yes, there are some uses, but please, ignore them. Specially the lang strings... that stuff goes to AMOS (our system for supporting translations and language packs generation, and I bet it won't "digest" them properly at all). So, for sure, the lang ones must be out, and also the SQLs (using double/single quotes as needed). About the HTML ones, I can live with them if they require variables substitution, but no excuse if they are pure HTML IMO. Once again, if I get some time... I'll try to help here (I love tedious and repetitive tasks). B. 1. Ok 2. Are there any specific recommendations for changes in this file? Well, I must confess that the bit calling my attention was: $rawbody = file_get_contents( "php: //input" ); that basically means it accepts any content. And the point was about to look carefully how we are validating/using/storing that information. So nothing found yet (I'm about to start 2nd, deeper, iteration). I'll keep you informed if I find anything annoying. 3. I think I was looking @ this some weeks ago and there were not proper solutions for the dynamic icon thing. All that stuff is controlled by our mod_info/cm_info core stuff and it will need to be able to accept "other" icons. But I'm not sure if that "other" should include remote urls or just pulled & locally stored icons. It is something to discuss and I'm 90% sure this won't be possible in Moodle 2.2. I'll try to introduce it @ HQ next week to see which are the feelings about it. Depending of that, perhaps we'll need to comment out some parts of the UI (hopefully re-introducing them ASAP). 4. Well, perhaps the problem is in the "IMS LTI 1.0 PHP basic provider" itself (the one I used for basic testing), behaving wrongly. I agree that json format itself should be ok/safe. But, if we can make it safer just by using another, simpler, format, or by encoding in another way.. then my +1 goes to make it (we don't know what some providers outa there are going to accept). 5. Yeah, that would be terrible welcome, I really was not able to get the point beyond that stuff. I guessed is was sort of pre-configured external tools available to all activities, but the "pending/rejected" thing, and the organizations... simply didn't fit in my (limited) brain. C. 1. See A4: MDL-30149 3. Oki, I think we can survive without those scroll bars (I sounded to me a lot to the fights I had to maintain some years ago in the IMS-CP resource type). So we can close this as "Done!" if nobody objects it. 4. Yes, what I mean is that I get four options in the "Launch container" menu: ("default", "embed", "embed without blocks" and "new window") and it's not clear at all the meaning of the "default" one (or from where that default comes/which is its current value). That leaded me to think it that something is wrong with it. Finally, about the pending things, I'd recommend to, at this time, center our efforts in the "required" stuff. Where required means all we need to get this integrated, aka (A) and as most as possible of (B). Once being there, we'll have 3 more weeks for testing, fixing (B) and add new stuff (C/subtasks). Of course, big thanks for your hard work! Let's push, push, push! And happy good trip back from JP! I'm back to the 2nd review round (will try to fix trivial coding-rulezz while doing). Anything new found will be documented in the wiki and shared here. Ciao
          Hide
          Martin Dougiamas added a comment - - edited

          Frankly I was completely happy with it as "lti" so if it's going to take a lot of effort to rename everything in the code base I would leave it.

          In the interface it's better as "External tool", not IMS LTI. Technical jargon like that is exactly what we'll be fixing everywhere in 2.3.

          Show
          Martin Dougiamas added a comment - - edited Frankly I was completely happy with it as "lti" so if it's going to take a lot of effort to rename everything in the code base I would leave it. In the interface it's better as "External tool", not IMS LTI. Technical jargon like that is exactly what we'll be fixing everywhere in 2.3.
          Hide
          Charles Severance added a comment -

          I am happy with anything for the module name as long as it is not basiclti and I prefer "External Tool" over than "IMS LTI" in the UI.

          I agree with Eloy that there is no "update" needed because this will "appear" in 2.3 - but in that case we should rename the database tables in the lti module so folks who have installed the basiclti4moodle stuff don't end up with a surprise breakage. If we use different table names - the two modules simply pass like ships in the night.

          As long as we don't "break" things for folks that already installed basiclti4moodle, I really think there is little need in any type of conversion other than manual re-entry of tools into the new lti module as folks go through their transition to 2.3. The core Moodle code certainly does not ever need any awareness of the basiclti4moodle stuff. At some point we could imagine that the basiclti4moodle code might produce a 2.3 and later version that would at least be aware of the core code.

          Show
          Charles Severance added a comment - I am happy with anything for the module name as long as it is not basiclti and I prefer "External Tool" over than "IMS LTI" in the UI. I agree with Eloy that there is no "update" needed because this will "appear" in 2.3 - but in that case we should rename the database tables in the lti module so folks who have installed the basiclti4moodle stuff don't end up with a surprise breakage. If we use different table names - the two modules simply pass like ships in the night. As long as we don't "break" things for folks that already installed basiclti4moodle, I really think there is little need in any type of conversion other than manual re-entry of tools into the new lti module as folks go through their transition to 2.3. The core Moodle code certainly does not ever need any awareness of the basiclti4moodle stuff. At some point we could imagine that the basiclti4moodle code might produce a 2.3 and later version that would at least be aware of the core code.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          NP here, marking A1 (naming) as done. It will be "lti" and "External Tool". (don't agree, but that's not important). One less thing.

          Regarding tablenames, I don't expect any problem there as far as all the basiclti4moodle ones are prefixed "basiclti" and the ones introduced by this implementation are "lti" prefixed. A2 continues being valid and any upgrade code and/or conversion from 1.x backups can be safely deleted.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - NP here, marking A1 (naming) as done. It will be "lti" and "External Tool". (don't agree, but that's not important). One less thing. Regarding tablenames, I don't expect any problem there as far as all the basiclti4moodle ones are prefixed "basiclti" and the ones introduced by this implementation are "lti" prefixed. A2 continues being valid and any upgrade code and/or conversion from 1.x backups can be safely deleted. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Chris (et all),

          just to save some time, I've rebased (against current master, applies clean) and pushed your private branch to my public moodle.git repo. That way everybody will be able to look/check/test it immediately.

          Here it's the thing: https://github.com/stronk7/moodle/compare/master...MDL-20534

          I've added some commits on top, addressing:

          A2 - upgrade code and moodle1 backup conversion deleted.
          A3 - global coding / whitespace cleanup. The cleanup does NOT include any change in the 3 OAuth files nor in the language ones.
          A4 - in all places using namespaces, add the TODO comment.
          Others - various minor fixes here and there.

          Note for Moodlerooms & Ludosteam: While cleaning all the header comments and @ copyrights, I've added the 'MRTODO' placeholder to be reviewed by you and add there the pertinent information. All the original comments @ copyrights have been observed, of course (though the order/formatting of the lines has been accommodated). Please check everything is ok.

          So I think that, with those changes we can consider A2, A3 and A4 done, so I'm marking that in the review document @ wiki.

          In the other side, I've detected some more issues that I'm adding to the review, mainly:

          B7 - Implement backup & restore. Seems incomplete.
          B8 - lti_request_is_using_ssl() uses $ME, that only contains the path (not the scheme nor host).
          B9 - lti/mod/log.php is missing the definitions of logging actions. That needs to be completed and uses revisited.
          B10 - launch.php missing any capability check? Others scripts too?

          So, in theory we are only missing A5 for landing. I'm sure that I've missed important things in the review, but if we don't advance NOW, this is not going to land on time.

          Once @ upstream, work on Bxx and new issues can be performed easier. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Chris (et all), just to save some time, I've rebased (against current master, applies clean) and pushed your private branch to my public moodle.git repo. That way everybody will be able to look/check/test it immediately. Here it's the thing: https://github.com/stronk7/moodle/compare/master...MDL-20534 I've added some commits on top, addressing: A2 - upgrade code and moodle1 backup conversion deleted. A3 - global coding / whitespace cleanup. The cleanup does NOT include any change in the 3 OAuth files nor in the language ones. A4 - in all places using namespaces, add the TODO comment. Others - various minor fixes here and there. Note for Moodlerooms & Ludosteam: While cleaning all the header comments and @ copyrights, I've added the 'MRTODO' placeholder to be reviewed by you and add there the pertinent information. All the original comments @ copyrights have been observed, of course (though the order/formatting of the lines has been accommodated). Please check everything is ok. So I think that, with those changes we can consider A2, A3 and A4 done, so I'm marking that in the review document @ wiki. In the other side, I've detected some more issues that I'm adding to the review, mainly: B7 - Implement backup & restore. Seems incomplete. B8 - lti_request_is_using_ssl() uses $ME, that only contains the path (not the scheme nor host). B9 - lti/mod/log.php is missing the definitions of logging actions. That needs to be completed and uses revisited. B10 - launch.php missing any capability check? Others scripts too? So, in theory we are only missing A5 for landing. I'm sure that I've missed important things in the review, but if we don't advance NOW, this is not going to land on time. Once @ upstream, work on Bxx and new issues can be performed easier. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          One more thing, I've added A6 to avoid forgetting about B3. It needs to be decided before integration.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - One more thing, I've added A6 to avoid forgetting about B3. It needs to be decided before integration. Ciao
          Hide
          Chris Scribner added a comment -

          A.

          1. Great, no more name changes!
          2. We don't need any of the upgrade stuff. The commit from Darko you're referring to will be included in the common cartridge code you should be getting from MR (we should verify...)
          5. I checked a change into https://github.com/scriby/moodle for A5 (here/now doc). A few kittens around the world died as I replaced beautifully formatted strings with their escaped successors.

          B.

          3. I'll note that we consider the external icon issue to be important (for our / partners business interests) and for the success of LTI / LTI tool providers. Importing a course full of LTI links all with the same (terrible) icon makes for a subpar end user experience. I'll admit that this particular solution to the problem isn't the most desirable, but it's what we have to work with from the IMS spec. (Just for the record, my recommendation would be that the CC spec included a "type" for LTI resources, so the LMS could display its own appropriate icon. This would handle 90% of use cases elegantly).

          BTW, I'm working on getting a new icon from one of our designers... Hopefully I'll be able to drop it in next week.

          C

          4. In this case "default" means the default that was selected for the tool provider in an admin level or course level tool provider configuration. Because we determine the tool provider to use when the tool is launched, it's not easy to show what the default setting is.

          Thanks for taking the initiative to get the code cleaned up for inclusion into core!

          Chris

          Show
          Chris Scribner added a comment - A. 1. Great, no more name changes! 2. We don't need any of the upgrade stuff. The commit from Darko you're referring to will be included in the common cartridge code you should be getting from MR (we should verify...) 5. I checked a change into https://github.com/scriby/moodle for A5 (here/now doc). A few kittens around the world died as I replaced beautifully formatted strings with their escaped successors. B. 3. I'll note that we consider the external icon issue to be important (for our / partners business interests) and for the success of LTI / LTI tool providers. Importing a course full of LTI links all with the same (terrible) icon makes for a subpar end user experience. I'll admit that this particular solution to the problem isn't the most desirable, but it's what we have to work with from the IMS spec. (Just for the record, my recommendation would be that the CC spec included a "type" for LTI resources, so the LMS could display its own appropriate icon. This would handle 90% of use cases elegantly). BTW, I'm working on getting a new icon from one of our designers... Hopefully I'll be able to drop it in next week. C 4. In this case "default" means the default that was selected for the tool provider in an admin level or course level tool provider configuration. Because we determine the tool provider to use when the tool is launched, it's not easy to show what the default setting is. Thanks for taking the initiative to get the code cleaned up for inclusion into core! Chris
          Hide
          Martin Dougiamas added a comment -

          Awesome, Chris, thanks for taking care of A5.

          For the icon, I think the best approach would be similar to what "mod/resource" does. Try it with a Word document or a pdf document and you can see that the icon changes appropriately on the course page.

          Off the top of my head, I suggest that you grab the icon from the source when the module settings are saved, and store/crop/resize/convert it to a native image file in a dedicated LTI filearea. Then lti_get_coursemodule_info() in lib.php can return the correct URL to get that image (calling the pluginfile.php script). Remember also to add this filearea to backup/restore.

          Show
          Martin Dougiamas added a comment - Awesome, Chris, thanks for taking care of A5. For the icon, I think the best approach would be similar to what "mod/resource" does. Try it with a Word document or a pdf document and you can see that the icon changes appropriately on the course page. Off the top of my head, I suggest that you grab the icon from the source when the module settings are saved, and store/crop/resize/convert it to a native image file in a dedicated LTI filearea. Then lti_get_coursemodule_info() in lib.php can return the correct URL to get that image (calling the pluginfile.php script). Remember also to add this filearea to backup/restore.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          MD if I'm not wrong (I visited that code some weeks ago)... using pluginfile.php is also impossible right now. The modinfo/cm_info only works against wwwroot files,.

          IMO what we need is to perform some changes in that higher level API (modinfo, cm_info...) to allow whatever we want there, with possibilities being (one or both of):

          1) simple external URLs
          2) stored files (that the lti module should have pulled and stored previously as commented above).

          Surely 1) is the only solution implementable now (if cm_info gurus do not prevent us to do so). The 2) alternative will need, for sure more complex and deeper changes.

          So, I'd say our real possibilities right now are:

          1) allow simple external URLs in modinfo/cm_info (requires impl & approval)
          Z) hide all the icons UI in the lti module and wait until implemented properly (1 and/or 2 above)

          In any case, both possibilities imply we are going to revert current hack in pix_url(). No way that can land.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - MD if I'm not wrong (I visited that code some weeks ago)... using pluginfile.php is also impossible right now. The modinfo/cm_info only works against wwwroot files,. IMO what we need is to perform some changes in that higher level API (modinfo, cm_info...) to allow whatever we want there, with possibilities being (one or both of): 1) simple external URLs 2) stored files (that the lti module should have pulled and stored previously as commented above). Surely 1) is the only solution implementable now (if cm_info gurus do not prevent us to do so). The 2) alternative will need, for sure more complex and deeper changes. So, I'd say our real possibilities right now are: 1) allow simple external URLs in modinfo/cm_info (requires impl & approval) Z) hide all the icons UI in the lti module and wait until implemented properly (1 and/or 2 above) In any case, both possibilities imply we are going to revert current hack in pix_url(). No way that can land. Ciao
          Hide
          Sam Marshall added a comment - - edited

          Plan 1 is ok imo - cm_info could have new field $iconurl (this should be private field in 'new data available only by functions' section, with set method set_icon_url). This will then need to be implemented in get_icon_url method so that, if $iconurl is non-null, use that instead of $icon and $iconcomponent.

          I guess it (iconurl field) needs to be added to cached_cm_info as well. I think this may require a slight change in course/lib.php [where it stores any used fields from cached_cm_info in modinfo] as well as in modinfolib.

          Should be fairly straightforward to code I think. When that api is in place then you can just set it in cached_cm_info in module_get_course_module_info function, or dynamically for each request (should that be needed, hopefully not in this case) in the other functions.

          It might be nicer in the long term to support stored files for icons as well but I can see that is harder.

          Show
          Sam Marshall added a comment - - edited Plan 1 is ok imo - cm_info could have new field $iconurl (this should be private field in 'new data available only by functions' section, with set method set_icon_url). This will then need to be implemented in get_icon_url method so that, if $iconurl is non-null, use that instead of $icon and $iconcomponent. I guess it (iconurl field) needs to be added to cached_cm_info as well. I think this may require a slight change in course/lib.php [where it stores any used fields from cached_cm_info in modinfo] as well as in modinfolib. Should be fairly straightforward to code I think. When that api is in place then you can just set it in cached_cm_info in module_get_course_module_info function, or dynamically for each request (should that be needed, hopefully not in this case) in the other functions. It might be nicer in the long term to support stored files for icons as well but I can see that is harder.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Thanks for the detailed instructions, Sam!

          Guessing if this is enough to introduce support for url-icons in cm_info (seems to be working great here, cached and rebuilt on activity update):

          https://github.com/stronk7/moodle/compare/master...MDL-30175

          To quick test it (forum/lib.php):

          function forum_get_coursemodule_info($coursemodule) {
          
              $info = new cached_cm_info();
              $info->iconurl = 'http://www.euroblind.org/skin/basic/design/braille/i/' . ($coursemodule->id % 10) . '.gif';
              return $info;
          
          }
          

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Thanks for the detailed instructions, Sam! Guessing if this is enough to introduce support for url-icons in cm_info (seems to be working great here, cached and rebuilt on activity update): https://github.com/stronk7/moodle/compare/master...MDL-30175 To quick test it (forum/lib.php): function forum_get_coursemodule_info($coursemodule) { $info = new cached_cm_info(); $info->iconurl = 'http: //www.euroblind.org/skin/basic/design/braille/i/' . ($coursemodule->id % 10) . '.gif'; return $info; } Ciao
          Hide
          Sam Marshall added a comment -

          Eloy's patch looks 100% right to me except:

          1) I would probably define it as a moodle_url not a string (this is only new code so no need for legacy report)? Or, is there problem with serialising moodle_url into the modinfo cache?

          2) Trivial but there's a @var string; <-- unnecessary semicolon

          Show
          Sam Marshall added a comment - Eloy's patch looks 100% right to me except: 1) I would probably define it as a moodle_url not a string (this is only new code so no need for legacy report)? Or, is there problem with serialising moodle_url into the modinfo cache? 2) Trivial but there's a @var string; <-- unnecessary semicolon
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks Sam,

          I've created MDL-30175 to address this, so for any icon-related thing, that's the place (I'm updating the review docs to take rid of A6/B3 based on that issue).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks Sam, I've created MDL-30175 to address this, so for any icon-related thing, that's the place (I'm updating the review docs to take rid of A6/B3 based on that issue). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Nice, MDL-30175 has been integrated so now we are external-icons-aware, going to send some commits to Chris repo...

          Show
          Eloy Lafuente (stronk7) added a comment - Nice, MDL-30175 has been integrated so now we are external-icons-aware, going to send some commits to Chris repo...
          Show
          Eloy Lafuente (stronk7) added a comment - Progress going @ https://github.com/scriby/moodle/compare/master...lti Review status: http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11 Niao
          Hide
          Martin Dougiamas added a comment -

          Direct link? Am I missing something? What happens if the image is 1024x768 pixels?

          Show
          Martin Dougiamas added a comment - Direct link? Am I missing something? What happens if the image is 1024x768 pixels?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          well, it if is 1024 x 768 it will look like a horrible rectangle, I'd recommend to use squared ones instead. But yes, any size is ok, if that's the question.

          Show
          Eloy Lafuente (stronk7) added a comment - well, it if is 1024 x 768 it will look like a horrible rectangle, I'd recommend to use squared ones instead. But yes, any size is ok, if that's the question.
          Hide
          Martin Dougiamas added a comment -

          OK, the patch looks good as a way to introduce external URLs there but I would still like the LTI module to download the icon itself, resize/crop/store it and give modinfo an internal pluginfile.php URL. This is by no means urgent for 2.2, though.

          Show
          Martin Dougiamas added a comment - OK, the patch looks good as a way to introduce external URLs there but I would still like the LTI module to download the icon itself, resize/crop/store it and give modinfo an internal pluginfile.php URL. This is by no means urgent for 2.2, though.
          Hide
          Martin Dougiamas added a comment -

          Chris, to help get this into 2.2 we really need your urgent attention to the many issues that Eloy is discovering.

          http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11

          Reviewers don't usually fix everything, they push it back to the developer. He's fixing things this week only because of the lack of time before freeze, but there is a lot there (mostly related to the original code base I think).

          Show
          Martin Dougiamas added a comment - Chris, to help get this into 2.2 we really need your urgent attention to the many issues that Eloy is discovering. http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11 Reviewers don't usually fix everything, they push it back to the developer. He's fixing things this week only because of the lack of time before freeze, but there is a lot there (mostly related to the original code base I think).
          Hide
          Charles Severance added a comment -

          I grabbed a copy of Eloy's branch and started playing around doing some testing with the IMS LTI 1.1 certification suite and test harnesses and found some minor issues that are pretty easy to fix. I will let things settle down a bit (I am watching this issue) and when there is a pretty stable branch or it ends up in trunk, I will run through a suite of tests and check for the kinds of little things that a certification test exposes and provide a few patches.

          Show
          Charles Severance added a comment - I grabbed a copy of Eloy's branch and started playing around doing some testing with the IMS LTI 1.1 certification suite and test harnesses and found some minor issues that are pretty easy to fix. I will let things settle down a bit (I am watching this issue) and when there is a pretty stable branch or it ends up in trunk, I will run through a suite of tests and check for the kinds of little things that a certification test exposes and provide a few patches.
          Hide
          Chris Scribner added a comment -

          It's not really going to be possible for me to address any issues before Friday. I got back in yesterday and am flying to Baltimore this morning for a company summit. Between meetings and jet lag, I don't think there will be much room for coding. I expect to be able to move through the issues quickly once I get started on them. It's definitely not my intention to pass work off onto Eloy (just unfortunate timing)!

          Some of the issues are with the old code, and some are with the code I wrote. There are many standards / preferences I wasn't aware of, and we made many design trade-offs without input from Moodle core. We focused as much as we could on creating the best end-user experience using the module – the common use cases should "just work" well.

          I'll respond to a few items. All the others sound fine and I'll address as many as possible.

          A.

          9. Do you mean that the browser is blocking the popups or the popups themselves are blank? I haven't seen a problem here before, but I'll get a site set up with the new code to check it out.

          10-11. I think you misunderstood the purpose of the isadmin flag. It's to indicate whether the user is viewing the editor from the admin interface or the instructor interface. The admin interface gets a few fields the instructor one doesn't, but everything else is the same. It's not supposed to be capability based within a course, the options are only for site-wide configured tool providers.

          B.

          10. Do we need to add a permission for launching an LTI resource? Or just make sure the person launching is enrolled in the course?

          11. Is there a script that can re-order the localization keys somewhere?

          12. Ok. Does this include widgets, too? The YUI3 data table is still in beta (I guess the tabs aren't anymore – well, depending on what version of yui3 is in core).

          13. A scale of 0-100 is assumed. Grades received through the LTI service are percentage based and scaled across that range. I'm not very knowledgeable about how Moodle handles grades. Is there a problem here?

          14. I'm not familiar with this feature

          16. Leaving the SSL fields out of the admin screen was done on purpose. The icon one doesn't make sense, as it's a per-instance setting. The SSL launch URL is more of a Common Cartridge setting than an LTI one. The admin piece just defines associations between domains and launch credentials / settings. In the worst case (if the normal and SSL launch URLs for the tool don't share the same domain), we would have to set up two tool configurations in the admin side.

          Thanks,

          Chris

          Show
          Chris Scribner added a comment - It's not really going to be possible for me to address any issues before Friday. I got back in yesterday and am flying to Baltimore this morning for a company summit. Between meetings and jet lag, I don't think there will be much room for coding. I expect to be able to move through the issues quickly once I get started on them. It's definitely not my intention to pass work off onto Eloy (just unfortunate timing)! Some of the issues are with the old code, and some are with the code I wrote. There are many standards / preferences I wasn't aware of, and we made many design trade-offs without input from Moodle core. We focused as much as we could on creating the best end-user experience using the module – the common use cases should "just work" well. I'll respond to a few items. All the others sound fine and I'll address as many as possible. A. 9. Do you mean that the browser is blocking the popups or the popups themselves are blank? I haven't seen a problem here before, but I'll get a site set up with the new code to check it out. 10-11. I think you misunderstood the purpose of the isadmin flag. It's to indicate whether the user is viewing the editor from the admin interface or the instructor interface. The admin interface gets a few fields the instructor one doesn't, but everything else is the same. It's not supposed to be capability based within a course, the options are only for site-wide configured tool providers. B. 10. Do we need to add a permission for launching an LTI resource? Or just make sure the person launching is enrolled in the course? 11. Is there a script that can re-order the localization keys somewhere? 12. Ok. Does this include widgets, too? The YUI3 data table is still in beta (I guess the tabs aren't anymore – well, depending on what version of yui3 is in core). 13. A scale of 0-100 is assumed. Grades received through the LTI service are percentage based and scaled across that range. I'm not very knowledgeable about how Moodle handles grades. Is there a problem here? 14. I'm not familiar with this feature 16. Leaving the SSL fields out of the admin screen was done on purpose. The icon one doesn't make sense, as it's a per-instance setting. The SSL launch URL is more of a Common Cartridge setting than an LTI one. The admin piece just defines associations between domains and launch credentials / settings. In the worst case (if the normal and SSL launch URLs for the tool don't share the same domain), we would have to set up two tool configurations in the admin side. Thanks, Chris
          Hide
          Martin Dougiamas added a comment -

          Cheers Chris, thanks.

          Show
          Martin Dougiamas added a comment - Cheers Chris, thanks.
          Hide
          Charles Severance added a comment -

          I notice that this is marked as "need QA test" - Back in June, I wrote a QA test in simpletest/testlocallib.php - I am sure that it is not sufficient because I am not an expert and perhaps it needs up be updated a bit - but it is/was a start.

          Show
          Charles Severance added a comment - I notice that this is marked as "need QA test" - Back in June, I wrote a QA test in simpletest/testlocallib.php - I am sure that it is not sufficient because I am not an expert and perhaps it needs up be updated a bit - but it is/was a start.
          Hide
          Charles Severance added a comment -

          Yesterday at the IMS Quarterly Meeting in Commerce, TX USA, I demonstrated Moodle 2.2 support for launch and grade return. It worked and impressed the group watching the demos. I grabbed the code from Eloy's branch. Being not an expert developer it took me a while to get it working - but once it worked - it was very nice.

          Show
          Charles Severance added a comment - Yesterday at the IMS Quarterly Meeting in Commerce, TX USA, I demonstrated Moodle 2.2 support for launch and grade return. It worked and impressed the group watching the demos. I grabbed the code from Eloy's branch. Being not an expert developer it took me a while to get it working - but once it worked - it was very nice.
          Hide
          Martin Dougiamas added a comment -

          Chuck, our QA tests are functional tests that people do to make sure everything is working. For example: http://tracker.moodle.org/browse/MDLQA-71

          We need a few tests like that covering the LTI functionality from a user point of view, written in a detailed and reproducible way so that anyone can perform the test and make sure the module is performing correctly. I guess one to test installation/connection, one to test sending of data to the tool and one to test returned grades. If you'd write them (as a comment here is fine) that would be very helpful.

          BTW is there a list of public LTI providers around? I had trouble finding any useful examples.

          Show
          Martin Dougiamas added a comment - Chuck, our QA tests are functional tests that people do to make sure everything is working. For example: http://tracker.moodle.org/browse/MDLQA-71 We need a few tests like that covering the LTI functionality from a user point of view, written in a detailed and reproducible way so that anyone can perform the test and make sure the module is performing correctly. I guess one to test installation/connection, one to test sending of data to the tool and one to test returned grades. If you'd write them (as a comment here is fine) that would be very helpful. BTW is there a list of public LTI providers around? I had trouble finding any useful examples.
          Hide
          Charles Severance added a comment -

          Here is a list of Basic LTI tool end points.

          Show
          Charles Severance added a comment - Here is a list of Basic LTI tool end points.
          Hide
          Charles Severance added a comment -

          Martin, I just uploaded a list of BLTI endpoints in an rtf file as an attachment to this issue. These are all LTI 1.0 (Basic launch only). We have a test for LTI 1.1 (launch + grade + extensions) that is not yet public - but it available on a security-by-obscurity URL at www.imsglobal.org - Chris has the LTI 1.1 testing URL and I am happy to send it to you via E-Mail for you to give to your QA folks. You can distribute it as widely as you like - just not on the wide open Internet - at least for a few more weeks while I finalize the LTI 1.1 testing code and put it at its official location.

          In terms of a test plan - I wrote one back in June (attached to this ticket) for the basiclti4moodle code. I am not yet an expert in the Chris code so it might be best for Chris to grab my test plan (above) as his starting point, clean it up and add the new flows. Even thought it is not quite right, it is probably a better starting point than nothing.

          Show
          Charles Severance added a comment - Martin, I just uploaded a list of BLTI endpoints in an rtf file as an attachment to this issue. These are all LTI 1.0 (Basic launch only). We have a test for LTI 1.1 (launch + grade + extensions) that is not yet public - but it available on a security-by-obscurity URL at www.imsglobal.org - Chris has the LTI 1.1 testing URL and I am happy to send it to you via E-Mail for you to give to your QA folks. You can distribute it as widely as you like - just not on the wide open Internet - at least for a few more weeks while I finalize the LTI 1.1 testing code and put it at its official location. In terms of a test plan - I wrote one back in June (attached to this ticket) for the basiclti4moodle code. I am not yet an expert in the Chris code so it might be best for Chris to grab my test plan (above) as his starting point, clean it up and add the new flows. Even thought it is not quite right, it is probably a better starting point than nothing.
          Hide
          Chris Scribner added a comment -

          FYI,

          I added the 1.9 -> 2.0 converter back in (Darko's commit). The CC import code needs the file to import LTI links, because the CC import process is to convert the CC to 1.9 Moodle format, then convert that to 2.0 format.

          Show
          Chris Scribner added a comment - FYI, I added the 1.9 -> 2.0 converter back in (Darko's commit). The CC import code needs the file to import LTI links, because the CC import process is to convert the CC to 1.9 Moodle format, then convert that to 2.0 format.
          Hide
          Chris Scribner added a comment -

          All the "A" items should be addressed now.

          A.
          8. Removed locallib require and fixed breakage
          9. This should be fixed now. This broke when I added the secure launch URL to the form earlier.
          10-11. (Previous comment) I think you misunderstood the purpose of the isadmin flag. It's to indicate whether the user is viewing the editor from the admin interface or the instructor interface. The admin interface gets a few fields the instructor one doesn't, but everything else is the same. It's not supposed to be capability based within a course, the options are only for site-wide configured tool providers.

          Show
          Chris Scribner added a comment - All the "A" items should be addressed now. A. 8. Removed locallib require and fixed breakage 9. This should be fixed now. This broke when I added the secure launch URL to the form earlier. 10-11. (Previous comment) I think you misunderstood the purpose of the isadmin flag. It's to indicate whether the user is viewing the editor from the admin interface or the instructor interface. The admin interface gets a few fields the instructor one doesn't, but everything else is the same. It's not supposed to be capability based within a course, the options are only for site-wide configured tool providers.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Chris (et all),

          General:

          I've updated http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11 with the changes performed here along the last days.

          Also, I've created the "ims-lti" component in the Tracker, so this and future issues can be reported/handled properly. I've added you, Chris, as component maintainer, so you will be the one receiving the issues initially. Let me know if there is something wrong with that.

          A. Seems everything id "Done!" yay! I only would add some testing instructions here (surely basic, like install/upgrade/edit/admin/run some example...) in order to get the initial testing round defined and performed.

          B. Is still plenty, so I guess next 2-3 weeks must be intense, getting as much as possible fixed before release. Some replies to your last messages:

          10. I'm not 100% sure if we need one capability checking for this or, as you said, is enough to be enrolled. Surely the later is true, but IMO, we should at least check for the "view" capability in all the pages. Surely I'm more concerned about how the grading from the external tool lands into Moodle, and how it's checked if the user running the activity is a "gradeable" one (not a teacher, for example). lti_update_grade() seems to be missing that.

          11. No, sorry. I've asked David about that, but no tool is available for that. Anyway, if something must be delayed, I'd say this it the less critical.

          12. Everything able to run under YUI3 must be YUI3, afaik.

          13. Well, on Moodle, all the modules allow you to pick one max grade or one scale to be used. That, in practice, means that any grade coming in % from source, must be scaled to that grade setting. So if you define the max grade in the module to be "80" and you get one 50% from the external tool, it should be stored into Moodle, re-scaled, aka one "40". Same happens if scales are used (the number of items in the scale -1) is the max value to re-scale to. It's really simple, I'd recommend you to take a look to other modules because it's practically automatic and makes it better than current, fixed, 100 max value.

          14. It's a simple feature that allows the teacher to decide if he wants to show the description of the activity embed in the course page. Also automatic, all the rest of modules support it. Look for FEATURE_SHOW_DESCRIPTION

          16. Oki, I trust you, lol

          xx. I'm adding some more issues to the section B

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Chris (et all), General: I've updated http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11 with the changes performed here along the last days. Also, I've created the "ims-lti" component in the Tracker, so this and future issues can be reported/handled properly. I've added you, Chris, as component maintainer, so you will be the one receiving the issues initially. Let me know if there is something wrong with that. A. Seems everything id "Done!" yay! I only would add some testing instructions here (surely basic, like install/upgrade/edit/admin/run some example...) in order to get the initial testing round defined and performed. B. Is still plenty, so I guess next 2-3 weeks must be intense, getting as much as possible fixed before release. Some replies to your last messages: 10. I'm not 100% sure if we need one capability checking for this or, as you said, is enough to be enrolled. Surely the later is true, but IMO, we should at least check for the "view" capability in all the pages. Surely I'm more concerned about how the grading from the external tool lands into Moodle, and how it's checked if the user running the activity is a "gradeable" one (not a teacher, for example). lti_update_grade() seems to be missing that. 11. No, sorry. I've asked David about that, but no tool is available for that. Anyway, if something must be delayed, I'd say this it the less critical. 12. Everything able to run under YUI3 must be YUI3, afaik. 13. Well, on Moodle, all the modules allow you to pick one max grade or one scale to be used. That, in practice, means that any grade coming in % from source, must be scaled to that grade setting. So if you define the max grade in the module to be "80" and you get one 50% from the external tool, it should be stored into Moodle, re-scaled, aka one "40". Same happens if scales are used (the number of items in the scale -1) is the max value to re-scale to. It's really simple, I'd recommend you to take a look to other modules because it's practically automatic and makes it better than current, fixed, 100 max value. 14. It's a simple feature that allows the teacher to decide if he wants to show the description of the activity embed in the course page. Also automatic, all the rest of modules support it. Look for FEATURE_SHOW_DESCRIPTION 16. Oki, I trust you, lol xx. I'm adding some more issues to the section B
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki, review updated and everything seems ready for integration.

          Please take a look to the review in case there is anything we should sort before sending this to core git repo. Any work should continue there after landing.

          http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11

          Also, please, fill here what is required to be tested at this point.

          Awaiting for MD +1 AND testing instructions, sounds simple, isn't it?

          Thanks everybody! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki, review updated and everything seems ready for integration. Please take a look to the review in case there is anything we should sort before sending this to core git repo. Any work should continue there after landing. http://docs.moodle.org/dev/IMS-LTI_consumer_module_review_2011-11 Also, please, fill here what is required to be tested at this point. Awaiting for MD +1 AND testing instructions, sounds simple, isn't it? Thanks everybody! Ciao
          Hide
          Chris Scribner added a comment -

          Just FYI, I'm out the week of 11/21 for Thanksgiving. I'll devote as much time as I can this week to work through the B issues.

          B.

          10. I'm not sure I follow the concerns around updating the grade. The only interface into that is through the LTI service calls, which is secured through the shared secret and a salted hash for the individual activity (to make sure the tool provider isn't tamperin with ids to try to send grades for other activities). What am I missing?

          11. Ok, I'll see what I can do here. Shouldn't take too long to write a re-ordering script as none of my language strings contain semicolons (I think).

          12. Can you specifically address whether you want me to use the beta YUI 3 DataTable or not? It's a bit of a gray area and a judgment call (and we'll want to make sure the newest version of YUI 3 is in core if we go for it).

          13. I'll look into it.

          16. The SSL settings ended up being a bit of a pain. I wish the LTI spec had just said "the SSL URL will be the same as the non-SSL URL, with https instead of http (and there would have to be a separate bit flag indicating whether SSL is supported". That would have allowed us to not have to store a separate URL, add an edit box for it on the (already crowded) instructor edit form, and simpler logic around launching.

          Thanks,

          Chris

          Show
          Chris Scribner added a comment - Just FYI, I'm out the week of 11/21 for Thanksgiving. I'll devote as much time as I can this week to work through the B issues. B. 10. I'm not sure I follow the concerns around updating the grade. The only interface into that is through the LTI service calls, which is secured through the shared secret and a salted hash for the individual activity (to make sure the tool provider isn't tamperin with ids to try to send grades for other activities). What am I missing? 11. Ok, I'll see what I can do here. Shouldn't take too long to write a re-ordering script as none of my language strings contain semicolons (I think). 12. Can you specifically address whether you want me to use the beta YUI 3 DataTable or not? It's a bit of a gray area and a judgment call (and we'll want to make sure the newest version of YUI 3 is in core if we go for it). 13. I'll look into it. 16. The SSL settings ended up being a bit of a pain. I wish the LTI spec had just said "the SSL URL will be the same as the non-SSL URL, with https instead of http (and there would have to be a separate bit flag indicating whether SSL is supported". That would have allowed us to not have to store a separate URL, add an edit box for it on the (already crowded) instructor edit form, and simpler logic around launching. Thanks, Chris
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Thanks Chris,

          one more little thing I always forget to comment. In my commits, I added some "MRTODO" labels around copyright for new files and so on, if you can, plz, search and kill all them to suit your needs. TIA!

          Edited: About YUI, all I know is we have version 3.4.1 into master (2.2), pinging Sam Hemelryk (I've no idea at all about YUI).

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Thanks Chris, one more little thing I always forget to comment. In my commits, I added some "MRTODO" labels around copyright for new files and so on, if you can, plz, search and kill all them to suit your needs. TIA! Edited: About YUI, all I know is we have version 3.4.1 into master (2.2), pinging Sam Hemelryk (I've no idea at all about YUI).
          Hide
          Martin Dougiamas added a comment -

          +1 to land today!

          Show
          Martin Dougiamas added a comment - +1 to land today!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, I'm pushing this definitively to integration.git (then do some basic install/upgrade/edit tests) and will be part of the upstream (moodle.git, master branch) repository and 2.2beta version downloads in a bunch of hours.

          The very next step should be to add/suggest/request a battery of MDLQA tests to cover as much as possible and also start some user-level (teacher/admin) documentation. Then the rest of pending B issues.

          Thanks everybody... here we go, towards and lti-zed world! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, I'm pushing this definitively to integration.git (then do some basic install/upgrade/edit tests) and will be part of the upstream (moodle.git, master branch) repository and 2.2beta version downloads in a bunch of hours. The very next step should be to add/suggest/request a battery of MDLQA tests to cover as much as possible and also start some user-level (teacher/admin) documentation. Then the rest of pending B issues. Thanks everybody... here we go, towards and lti-zed world! Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This has been integrated!

          Show
          Eloy Lafuente (stronk7) added a comment - This has been integrated!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has passed basic tests (MDLQA should cover extensively this along the next weeks).

          Note I've added one extra commit raising versions and requirements of the module to today dates.

          Later today this should be available in main moodle.git repo (and github/gitorious clones) under the v2.2beta tag (and of course, in the master branch). Also via downloads of 2.2beta @ http://download.moodle.org

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has passed basic tests (MDLQA should cover extensively this along the next weeks). Note I've added one extra commit raising versions and requirements of the module to today dates. Later today this should be available in main moodle.git repo (and github/gitorious clones) under the v2.2beta tag (and of course, in the master branch). Also via downloads of 2.2beta @ http://download.moodle.org Ciao
          Hide
          Ludo ( Marc Alier) added a comment -

          Yay! Soo cool!!! :-D

          Show
          Ludo ( Marc Alier) added a comment - Yay! Soo cool!!! :-D
          Hide
          Charles Severance added a comment -

          Thanks all - I teach and am in meetings all day today - but this evening I will check it out and start my own QA / Review of it.

          Show
          Charles Severance added a comment - Thanks all - I teach and am in meetings all day today - but this evening I will check it out and start my own QA / Review of it.
          Hide
          Chris Scribner added a comment -

          Here are things that need to be tested (can serve as an outline of a test plan):

          1. In a course, create an ad-hoc External Tool. Just fill out the Launch URL, consumer key (advanced), and shared secret (advanced).

          2. Create a course-level tool configuration by clicking on the "Add" button next to the tool type dropdown in a course. Create an External Tool in a course that uses the course level tool configuration (the Launch URL should "start with" the Base URL of the tool configuration).

          3. Create a site-level tool configuration by visiting Administration -> Plugins -> Activity modules -> External Tool. Create an External Tool in a course that uses the site level tool configuration.

          4. Test launching as a student and submitting grades with all the tools created in steps 1-3.

          5. Test launching as an instructor from all the tools created in steps 1-3. Verify grades in the grade book for student launches in step 4. Check the submission page for each External Tool and verify the submissions are present.

          6. Import a Common Cartridge with LTI links and verify they work. (This is pending CC code getting integrated with core)

          Those are the most important items / use cases. There are certainly more things that could be tested at a more detailed level.

          Show
          Chris Scribner added a comment - Here are things that need to be tested (can serve as an outline of a test plan): 1. In a course, create an ad-hoc External Tool. Just fill out the Launch URL, consumer key (advanced), and shared secret (advanced). 2. Create a course-level tool configuration by clicking on the "Add" button next to the tool type dropdown in a course. Create an External Tool in a course that uses the course level tool configuration (the Launch URL should "start with" the Base URL of the tool configuration). 3. Create a site-level tool configuration by visiting Administration -> Plugins -> Activity modules -> External Tool. Create an External Tool in a course that uses the site level tool configuration. 4. Test launching as a student and submitting grades with all the tools created in steps 1-3. 5. Test launching as an instructor from all the tools created in steps 1-3. Verify grades in the grade book for student launches in step 4. Check the submission page for each External Tool and verify the submissions are present. 6. Import a Common Cartridge with LTI links and verify they work. (This is pending CC code getting integrated with core) Those are the most important items / use cases. There are certainly more things that could be tested at a more detailed level.
          Hide
          Chris Scribner added a comment -

          I added the new tool_consumer_info settings to launch based on the newest version of the LTI spec, and have a question about version...

          $requestparams['tool_consumer_info_product_family_code'] = 'moodle';
          $requestparams['tool_consumer_info_version'] = strval($CFG->version);

          For now, I set the version number to Moodle's internal version #. I think the intention of the field is to be something like "2.3", but I couldn't figure out how to get a human readable version # automatically. Is there a way to get it, or is the current implementation as good as it gets?

          Show
          Chris Scribner added a comment - I added the new tool_consumer_info settings to launch based on the newest version of the LTI spec, and have a question about version... $requestparams ['tool_consumer_info_product_family_code'] = 'moodle'; $requestparams ['tool_consumer_info_version'] = strval($CFG->version); For now, I set the version number to Moodle's internal version #. I think the intention of the field is to be something like "2.3", but I couldn't figure out how to get a human readable version # automatically. Is there a way to get it, or is the current implementation as good as it gets?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yes, you got this finally upstream, just in time for Moodle 2.2beta. Congrats and thanks! Ciao
          Hide
          Chris Scribner added a comment -

          Eloy,

          Thanks for your dedication and patience! I'm glad to see this made it in.

          How should I make changes going forward?

          Chris

          Show
          Chris Scribner added a comment - Eloy, Thanks for your dedication and patience! I'm glad to see this made it in. How should I make changes going forward? Chris
          Hide
          Tim Hunt added a comment -

          With MDL-20636 (new question engine in 2.1). After that was integrated, I made a new issue (MDL-27738) with subtasks, or linked issues, to track all the remaining problems. That seemed to work quite well.

          Show
          Tim Hunt added a comment - With MDL-20636 (new question engine in 2.1). After that was integrated, I made a new issue ( MDL-27738 ) with subtasks, or linked issues, to track all the remaining problems. That seemed to work quite well.
          Hide
          Charles Severance added a comment -

          I ran the software through the certification and caught a few nits:

          • The error return is 'failure' not 'error'
          • The spec says that it needs to return 'failure' for out of range or non-numeric grades
          • The result score needs a language tag, hard-coded as 'en'
          • Setting a grade multiplied by 100 but reading the grade did not divide by 100

          All those are now fixed with this patch as well as this bit of cruft:

          • I removed the "extension service url" as it is not implemented in service.php

          Feel free to review and adjust - probably the one place you might want to refactor is that I put code to catch out-of-range-and non-numeric in lti_parse_grade_replace_message and threw an exception on error and then caught it in service.php and sent back the 'failure' message. Feel free to refactor a bit if you see this done in a cleaner manner.

          Show
          Charles Severance added a comment - I ran the software through the certification and caught a few nits: The error return is 'failure' not 'error' The spec says that it needs to return 'failure' for out of range or non-numeric grades The result score needs a language tag, hard-coded as 'en' Setting a grade multiplied by 100 but reading the grade did not divide by 100 All those are now fixed with this patch as well as this bit of cruft: I removed the "extension service url" as it is not implemented in service.php Feel free to review and adjust - probably the one place you might want to refactor is that I put code to catch out-of-range-and non-numeric in lti_parse_grade_replace_message and threw an exception on error and then caught it in service.php and sent back the 'failure' message. Feel free to refactor a bit if you see this done in a cleaner manner.
          Hide
          Charles Severance added a comment -

          See the attachment

          MDL-20534-cert.txt

          for these fixes. Let me know if there is another place for this.

          Show
          Charles Severance added a comment - See the attachment MDL-20534 -cert.txt for these fixes. Let me know if there is another place for this.
          Hide
          Charles Severance added a comment -

          If you want to grab a copy of the latest/newest combined test harness certification code check this out:

          http://ims-dev.googlecode.com/svn/trunk/lti/web/

          I just checked this out into my MAMP / htdocs in the folder "lti". Then I can run the tool test as:

          http://localhost:8888/lti/tool.php 12345 / secret

          You can run a certification run using this info:

          http://localhost:8888/lti/cert/lmscert.php 12345/secret

          Show
          Charles Severance added a comment - If you want to grab a copy of the latest/newest combined test harness certification code check this out: http://ims-dev.googlecode.com/svn/trunk/lti/web/ I just checked this out into my MAMP / htdocs in the folder "lti". Then I can run the tool test as: http://localhost:8888/lti/tool.php 12345 / secret You can run a certification run using this info: http://localhost:8888/lti/cert/lmscert.php 12345/secret
          Hide
          Helen Foster added a comment -

          I'm removing the qa_test_required label as the attached test plan has been divided over several QA tests.

          In case anyone wishes to run these tests or add themselves as a watcher, here are links to them in the 2.2 QA cycle:

          MDLQA-1418
          MDLQA-1419
          MDLQA-1420
          MDLQA-1421

          Feedback on the QA tests is always welcome!

          Show
          Helen Foster added a comment - I'm removing the qa_test_required label as the attached test plan has been divided over several QA tests. In case anyone wishes to run these tests or add themselves as a watcher, here are links to them in the 2.2 QA cycle: MDLQA-1418 MDLQA-1419 MDLQA-1420 MDLQA-1421 Feedback on the QA tests is always welcome!
          Hide
          Charles Severance added a comment -

          Helen, Where would I send any comments on these QA plans after I review them?

          Show
          Charles Severance added a comment - Helen, Where would I send any comments on these QA plans after I review them?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          (surely helen at this domain is ok, fyi, not sure if she will read your Q)

          LOL, edited: helen at this domain!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited (surely helen at this domain is ok, fyi, not sure if she will read your Q) LOL, edited: helen at this domain!
          Hide
          Helen Foster added a comment -

          Sorry, my mistake for not watching this issue. I am now! Charles, please leave your comments here. Martin also mentioned that he hoped to find time to review the QA tests.

          Show
          Helen Foster added a comment - Sorry, my mistake for not watching this issue. I am now! Charles, please leave your comments here. Martin also mentioned that he hoped to find time to review the QA tests.
          Hide
          Chris Scribner added a comment -

          I looked at the max grade issue again (B13). The only reason I could see it would matter is if someone was manually entering grades for the LTI item and didn't want to enter them on a scale of 0-100.

          Given that LTI grades are automatically reported, that doesn't look important enough to me to add a Max grade setting on the External Tool edit form. (Consequently, I couldn't find it on the quiz settings, but I saw it on the SCORM one.) (It would be great if the value could be changed from the grade book.)

          Do you think it's worth it to add the field?

          Show
          Chris Scribner added a comment - I looked at the max grade issue again (B13). The only reason I could see it would matter is if someone was manually entering grades for the LTI item and didn't want to enter them on a scale of 0-100. Given that LTI grades are automatically reported, that doesn't look important enough to me to add a Max grade setting on the External Tool edit form. (Consequently, I couldn't find it on the quiz settings, but I saw it on the SCORM one.) (It would be great if the value could be changed from the grade book.) Do you think it's worth it to add the field?
          Hide
          Chris Scribner added a comment -

          An update on the "B" list:

          1. Is there anything you want from me here?
          2. Waiting on specific items that need to be changed in service.php
          4. Chuck fixed the issue with the test harness. I don't think it's too unreasonable for tool providers to correctly decode the form (it's built into most frameworks nowadays). Base64 decoding probably doesn't help as +, /, and = will still need to be decoded.
          5. Do you understand this better now?
          7. What's not complete here? It worked ok in my basic testing. Note that course / site level types aren't backed up.
          8. MDL-30326 (awaiting integration)
          9. MDL-30343 (awaiting integration)
          10. Still need to look at this
          11. MDL-30339 (awaiting integration)
          12. Never got feedback on whether I should try to move the YUI2 DataGrid to beta DataGrid in YUI3.
          13. See previous comment
          14. MDL-30290 (awaiting integration)
          15. I looked at the TODO's and was baffled. I don't know how those functions are used and most importantly where to test them in the UI. If provided steps for how to test through the UI I could take a stab.
          16. I think we're fine here for 2.2, but am open to adjusting if problems come up.
          17. Still need to look at this
          18. Still need to look at this
          19. I didn't understand this comment
          20. Can you point out which places specifically need to be updated?
          21. Can you point me to the place you found that? I haven't seen any problems with this through front-end testing.
          22. The roster setting is gone. The privacy issue is a tricky balance, because some tool providers will need the information to work correctly. I could see a master level setting indicating the default state for those values.
          23. Still need to look at this
          24. Any Administrator can add / edit / reject global tools. Instructors submit "tool requests" from the launch error screen. (We had originally planned on integrating tool requests with CC import, but that turned out to not be possible, and it's in a slightly strange state now.)
          25. I get the gist of this comment but don't know what specifically needs to change.
          26. Still need to look at this
          27. MDL-30354 (awaiting integration)
          28. MDL-30341 (awaiting integration)
          29. Still need to look at this

          Show
          Chris Scribner added a comment - An update on the "B" list: 1. Is there anything you want from me here? 2. Waiting on specific items that need to be changed in service.php 4. Chuck fixed the issue with the test harness. I don't think it's too unreasonable for tool providers to correctly decode the form (it's built into most frameworks nowadays). Base64 decoding probably doesn't help as +, /, and = will still need to be decoded. 5. Do you understand this better now? 7. What's not complete here? It worked ok in my basic testing. Note that course / site level types aren't backed up. 8. MDL-30326 (awaiting integration) 9. MDL-30343 (awaiting integration) 10. Still need to look at this 11. MDL-30339 (awaiting integration) 12. Never got feedback on whether I should try to move the YUI2 DataGrid to beta DataGrid in YUI3. 13. See previous comment 14. MDL-30290 (awaiting integration) 15. I looked at the TODO's and was baffled. I don't know how those functions are used and most importantly where to test them in the UI. If provided steps for how to test through the UI I could take a stab. 16. I think we're fine here for 2.2, but am open to adjusting if problems come up. 17. Still need to look at this 18. Still need to look at this 19. I didn't understand this comment 20. Can you point out which places specifically need to be updated? 21. Can you point me to the place you found that? I haven't seen any problems with this through front-end testing. 22. The roster setting is gone. The privacy issue is a tricky balance, because some tool providers will need the information to work correctly. I could see a master level setting indicating the default state for those values. 23. Still need to look at this 24. Any Administrator can add / edit / reject global tools. Instructors submit "tool requests" from the launch error screen. (We had originally planned on integrating tool requests with CC import, but that turned out to not be possible, and it's in a slightly strange state now.) 25. I get the gist of this comment but don't know what specifically needs to change. 26. Still need to look at this 27. MDL-30354 (awaiting integration) 28. MDL-30341 (awaiting integration) 29. Still need to look at this
          Hide
          Helen Foster added a comment -

          Just noting that a couple of IMS-LTI tests have been deleted as they are not relevant to Moodle 2.2, and the remaining tests - MDLQA-1418 and MDLQA-1419 - are being rewritten.

          Show
          Helen Foster added a comment - Just noting that a couple of IMS-LTI tests have been deleted as they are not relevant to Moodle 2.2, and the remaining tests - MDLQA-1418 and MDLQA-1419 - are being rewritten.
          Hide
          Deirdre Edwards added a comment -

          We would really like the maximum grade to be editable. There are several reasons for this. Many of our courses use simple weighted mean or sum of grades as their aggregation methods. As we adopt external tools with LTI links, we would have to reassign point values to all other assignments and quizzes or the external tools' grades will disproportionately affect student grades. We could change the aggregation method to weighted mean of grades, but there are reasons that teachers prefer a more "Points-based" system. In addition, Weighted Mean does not support extra credit, which is one of the ways that we allow student choice of assignment (Putting two equivalent assignments in a category and making one extra credit.)

          Show
          Deirdre Edwards added a comment - We would really like the maximum grade to be editable. There are several reasons for this. Many of our courses use simple weighted mean or sum of grades as their aggregation methods. As we adopt external tools with LTI links, we would have to reassign point values to all other assignments and quizzes or the external tools' grades will disproportionately affect student grades. We could change the aggregation method to weighted mean of grades, but there are reasons that teachers prefer a more "Points-based" system. In addition, Weighted Mean does not support extra credit, which is one of the ways that we allow student choice of assignment (Putting two equivalent assignments in a category and making one extra credit.)

            People

            • Votes:
              11 Vote for this issue
              Watchers:
              23 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: