Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-68800

Some fixes to ltiservice_gradebookservice (MDL-65306 followup)

    XMLWordPrintable

    Details

    • Testing Instructions:
      Hide

      Pre-requisite

      1. Site is configured with at least one course
      2. ZTest tool 1.3 is installed as a site external tool:
        1. Log in as an administrator
        2. Navigate to Site Administration > Plugins > External tool > Manage tools
        3. Click on configure a tool manually
        4. Fill the form as follow:
          1. Tool name: ZTest 1.3
          2. Tool url: https://ztest.cengage.info/ztest/lti
        5. LTI Version: LTI 1.3
        6. Public key: copy the value from https://ztest.cengage.info/ztest/ LTI 1.3 Connect info tab
        7. Initiate Login URI: https://ztest.cengage.info/ztest/ws/lti/startlaunch?lti13=true&client_id=CLIENT_ID_HERE&platform=moodle
        8. Redirect URI: https://ztest.cengage.info/ztest/lti13
        9. Click on ‘Show more’
        10. Check Content-Item message
        11. Change the 'Privacy' setting ‘Accept grades from the tool’ to 'Delegate to Teacher'.
        12. In Services, IMS LTI Assignment and Grade Services, choose Use this service for grade and column mgmt
        13. Save changes.
        14. Once the tool is created, click the information icon (pie icon) and copy the client id
        15. Update the initiate login URI and replace CLIENT_ID_HERE with the client id value for that tool

      Verify upgrade preserves the resource id

      In this step we upgrade the site from 3.8 to 3.9 and verify the resource id is preserved.

      • Install a Moodle 3.8.x (current MOODLE_38_STABLE) site from scratch.
      • Prepare it following the steps in the previous "Pre-requisite" section.
      • As admin, restore the attached course backup-moodle2-course-2-ztestcourse38-20200526-1227-nu.mbz
        1. Restore as new course
        2. Keep all default options
        3. Perform restore
      • Once in the restored course
        1. Verify 2 ZTest links are present
        2. Turn editing on for the course
          1. Edit> Edit settings on each link and verify the preconfigured tool is ZTest13. If not, set it and save.
        3. Click on any of those links
        4. Verify ZTest opens without error
        5. Click AnyCall
        6. Click get lineitems button and click send button
        7. Verify payload matches the JSON data shown in the last test, just the id, resourceLinkId and ltiLinkId fields should differ
        8. Upgrade the site to 3.9 (after patch).
        9. Repeat step 3->7, the same result should display

      Verify previous release backup preserves resourceid

      In this test, we restore a course created on a 3.8 moodle instance which contains LTI links and gradebook columns created by ZTest. We verify the restored backup still contains the resourceid (since it is now stored on a new location).

      1. In the same site just upgraded (to after patch) by the previous test.
      2. As admin, restore the attached course backup-moodle2-course-2-ztestcourse38-20200526-1227-nu.mbz
        1. Restore as new course
        2. Keep all default options
        3. Perform restore
      3. Once in the restored course
        1. Verify 2 ZTest links are present
        2. Click on any of those
        3. Verify ZTest opens without error
        4. Click AnyCall
        5. Click get lineitems button and click send button
        6. Verify payload matches the following JSON, just the id, resourceLinkId and ltiLinkId fields should differ:

      [
        {
          "id": "https://moodle.zeedeeyou.com/mod/lti/services.php/4/lineitems/29/lineitem?type_id=1",
          "label": "ZTest Quiz No Resource Id",
          "scoreMaximum": 20,
          "resourceId": "",
          "tag": "",
          "resourceLinkId": "23",
          "ltiLinkId": "23"
        },
        {
          "id": "https://moodle.zeedeeyou.com/mod/lti/services.php/4/lineitems/30/lineitem?type_id=1",
          "label": "ZTest Quiz With ResourceId",
          "scoreMaximum": 20,
          "resourceId": "ztest-quiz-1",
          "tag": "",
          "resourceLinkId": "24",
          "ltiLinkId": "24"
        },
        {
          "id": "https://moodle.zeedeeyou.com/mod/lti/services.php/4/lineitems/31/lineitem?type_id=1",
          "label": "API ZTest 748",
          "scoreMaximum": 22,
          "resourceId": "748",
          "tag": "finalgrade"
        }
      ]
      

      Show
      Pre-requisite Site is configured with at least one course ZTest tool 1.3 is installed as a site external tool: Log in as an administrator Navigate to Site Administration > Plugins > External tool > Manage tools Click on configure a tool manually Fill the form as follow: Tool name: ZTest 1.3 Tool url:  https://ztest.cengage.info/ztest/lti LTI Version: LTI 1.3 Public key: copy the value from  https://ztest.cengage.info/ztest/  LTI 1.3 Connect info tab Initiate Login URI:  https://ztest.cengage.info/ztest/ws/lti/startlaunch?lti13=true&client_id=CLIENT_ID_HERE&platform=moodle Redirect URI:  https://ztest.cengage.info/ztest/lti13 Click on ‘Show more’ Check Content-Item message Change the 'Privacy' setting ‘Accept grades from the tool’ to 'Delegate to Teacher'. In Services, IMS LTI Assignment and Grade Services, choose Use this service for grade and column mgmt Save changes. Once the tool is created, click the information icon (pie icon) and copy the client id Update the initiate login URI and replace CLIENT_ID_HERE with the client id value for that tool Verify upgrade preserves the resource id In this step we upgrade the site from 3.8 to 3.9 and verify the resource id is preserved. Install a Moodle 3.8.x (current MOODLE_38_STABLE) site from scratch. Prepare it following the steps in the previous "Pre-requisite" section. As admin, restore the attached course backup-moodle2-course-2-ztestcourse38-20200526-1227-nu.mbz Restore as new course Keep all default options Perform restore Once in the restored course Verify 2 ZTest links are present Turn editing on for the course Edit> Edit settings on each link and verify the preconfigured tool is ZTest13. If not, set it and save. Click on any of those links Verify ZTest opens without error Click AnyCall Click get lineitems button and click send button Verify payload matches the JSON data shown in the last test, just the id, resourceLinkId and ltiLinkId fields should differ Upgrade the site to 3.9 (after patch). Repeat step 3->7, the same result should display Verify previous release backup preserves resourceid In this test, we restore a course created on a 3.8 moodle instance which contains LTI links and gradebook columns created by ZTest. We verify the restored backup still contains the resourceid (since it is now stored on a new location). In the same site just upgraded (to after patch) by the previous test. As admin, restore the attached course backup-moodle2-course-2-ztestcourse38-20200526-1227-nu.mbz Restore as new course Keep all default options Perform restore Once in the restored course Verify 2 ZTest links are present Click on any of those Verify ZTest opens without error Click AnyCall Click get lineitems button and click send button Verify payload matches the following JSON, just the id, resourceLinkId and ltiLinkId fields should differ: [ { "id" : "https://moodle.zeedeeyou.com/mod/lti/services.php/4/lineitems/29/lineitem?type_id=1" , "label" : "ZTest Quiz No Resource Id" , "scoreMaximum" : 20 , "resourceId" : "" , "tag" : "" , "resourceLinkId" : "23" , "ltiLinkId" : "23" }, { "id" : "https://moodle.zeedeeyou.com/mod/lti/services.php/4/lineitems/30/lineitem?type_id=1" , "label" : "ZTest Quiz With ResourceId" , "scoreMaximum" : 20 , "resourceId" : "ztest-quiz-1" , "tag" : "" , "resourceLinkId" : "24" , "ltiLinkId" : "24" }, { "id" : "https://moodle.zeedeeyou.com/mod/lti/services.php/4/lineitems/31/lineitem?type_id=1" , "label" : "API ZTest 748" , "scoreMaximum" : 22 , "resourceId" : "748" , "tag" : "finalgrade" } ]
    • Affected Branches:
      MOODLE_39_STABLE
    • Fixed Branches:
      MOODLE_39_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-68800-lti-gbs-patch-fixes

      Description

      This is a followup of MDL-65306 where some points where detected in the final implementation, but was agreed to allow it to land (to save 1 iteration), as far as the 3 following points were not breaking anything. From the original issue:

      1) One little detail that I had here annotated is that, here and there the code does checks like this:

      if (isset($something) and empty($something) { ...
      

      In those cases, the isset() is redundant, empty() works perfectly with "undefined". From the manual:

      No warning is generated if the variable does not exist. That means empty() is essentially the concise equivalent to !isset($var) || $var == false.

      (tiny example).

      2) There are various alignments / code wrapping along the whole patch (all the code in fact) that, while they aren't controlled by our code checker, clearly are in violation of the coding style. Code like:

      • function declarations: link.
      • multiple conditions: link.
      • others...

      must follow always the wrapping guidelines, that, very basically are: "Always use 4-space indenting, with 4-extra allowed when the very next line in code body may conflict / look confusing". In the docs you'll find some good examples.

      3) In restore... the after_restore_lti() method... I just realized that we are executing it for all the records in a given course, not in a given activity. Problem is that the method is executed once for every mod_lti (each one is a task) restored, so we are really running and running over the same stuff N times (the number of lti activities restored). Both the existing loop (that adjusts gradeitemid and resourceid), and the new loop inserting missing records from old backups.

      We really should be able to restrict the code to be applied over current activity stuff instead of whole course.

      I've been reviewing the problem and I think that it's "safe" (meaning, nothing will become broken because of the repeated executions), just it's not "correct", the repetition itself.

      4) Finally, in MDL-65306, some problems when testing backup and restore were detected. So we need to ensure here that:

      4a) backup and restore is working ok, both from old backups and current ones.
      4b) tangentially related with the above, also upgrade (that applies a similar logic to backup & restore) needs to be checked.

      5) Testing instructions of this issue will "replay" all the testing in the original issue, PLUS the points needed to tests 4a and 4b.

        Attachments

        1. backup-moodle2-course-2-ztestcourse38-20200526-1227-nu.mbz
          6 kB
        2. Screenshot_1.png
          Screenshot_1.png
          208 kB
        3. Screenshot_2.png
          Screenshot_2.png
          98 kB
        4. screenshot-1.png
          screenshot-1.png
          84 kB
        5. screenshot-2.png
          screenshot-2.png
          62 kB

          Issue Links

            Activity

              People

              Assignee:
              claudevervoort Claude Vervoort
              Reporter:
              stronk7 Eloy Lafuente (stronk7)
              Peer reviewer:
              David Mudrák (@mudrd8mz)
              Integrator:
              Jake Dallimore
              Tester:
              Janelle Barcega
              Participants:
              Component watchers:
              Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Fix Release Date:
                15/Jun/20

                  Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 1 day, 2 hours, 35 minutes
                  1d 2h 35m