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

mod_lti does not track page views for completion

    Details

    • Testing Instructions:
      Hide

      Enable Completion tracking for a course
      Turn Course Editing on
      Add a new Activity to your Course
      Choose External Tool (LTI)

      • Enter an Activity Name
      • Set Launch URL to an LTI tool
      • Set Launch Container to New Window (so you don't have to use BACK button)
        NOTE: You can use any URL here since we only care about activity (http://www.google.com works)
      • Set completion tracking to: Show activity as complete when conditions are met
      • Check the box: Student must view this activity to complete it
      • Click Save and Display

      Close the LTI window and go back to the course view.

      EXPECTED RESULT: Activity is now marked completed

      ACTUAL RESULT: Activity not marked as completed

      Show
      Enable Completion tracking for a course Turn Course Editing on Add a new Activity to your Course Choose External Tool (LTI) Enter an Activity Name Set Launch URL to an LTI tool Set Launch Container to New Window (so you don't have to use BACK button) NOTE: You can use any URL here since we only care about activity ( http://www.google.com works) Set completion tracking to: Show activity as complete when conditions are met Check the box: Student must view this activity to complete it Click Save and Display Close the LTI window and go back to the course view. EXPECTED RESULT: Activity is now marked completed ACTUAL RESULT: Activity not marked as completed
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
      git@github.com:Trii/moodle.git
    • Pull Master Branch:
      MDL-41520-master-mod_lti-completion-view

      Description

      mod/lti/view.php is missing the following code:

      // Mark viewed by user (if required)
      $completion = new completion_info($course);
      $completion->set_module_viewed($cm);

      You can enable completion on View in the lti settings but moodle never actually tracks that you viewed it

        Gliffy Diagrams

          Activity

          Hide
          joshjohnston Josh Johnston added a comment -

          This affects versions prior to 2.5.1 as well as it seems like this code was never added.

          Show
          joshjohnston Josh Johnston added a comment - This affects versions prior to 2.5.1 as well as it seems like this code was never added.
          Hide
          joshjohnston Josh Johnston added a comment -

          What is the SOP of this tracker? Do the component owners triage bugs right away or do they wait until they have free time to resolve before triaging? I'd like to start helping out as much as I can since we use this at my University.

          Show
          joshjohnston Josh Johnston added a comment - What is the SOP of this tracker? Do the component owners triage bugs right away or do they wait until they have free time to resolve before triaging? I'd like to start helping out as much as I can since we use this at my University.
          Hide
          quen Sam Marshall added a comment -

          Josh: It depends. This should really be reviewed by the component owner for the LTI module (assuming there is one...) and not me (I'm component owner for conditional activity API; this isn't an API problem).

          If you want to speed up the process then you should use Git to create a patch that includes the change, check it meets the coding standards etc, and give links here to your GitHub repository with a branch for the change. Also write testing instructions. That might encourage somebody to actually review/apply the change.

          See this page: http://docs.moodle.org/dev/Process

          For an example of what an issue submitted for peer review looks like (you can see github information/links and test instructions), here's one of mine that's currently awaiting review, obviously it is a bit more complicated a change but the same process is used:

          https://tracker.moodle.org/browse/MDL-38196

          Show
          quen Sam Marshall added a comment - Josh: It depends. This should really be reviewed by the component owner for the LTI module (assuming there is one...) and not me (I'm component owner for conditional activity API; this isn't an API problem). If you want to speed up the process then you should use Git to create a patch that includes the change, check it meets the coding standards etc, and give links here to your GitHub repository with a branch for the change. Also write testing instructions. That might encourage somebody to actually review/apply the change. See this page: http://docs.moodle.org/dev/Process For an example of what an issue submitted for peer review looks like (you can see github information/links and test instructions), here's one of mine that's currently awaiting review, obviously it is a bit more complicated a change but the same process is used: https://tracker.moodle.org/browse/MDL-38196
          Hide
          joshjohnston Josh Johnston added a comment -

          Thanks Sam,
          I'll clone, fix, and add the diffs/branch urls today. I've already fixed it in our local copy of Moodle

          Show
          joshjohnston Josh Johnston added a comment - Thanks Sam, I'll clone, fix, and add the diffs/branch urls today. I've already fixed it in our local copy of Moodle
          Hide
          joshjohnston Josh Johnston added a comment -

          Added the standard completion view magic

          Show
          joshjohnston Josh Johnston added a comment - Added the standard completion view magic
          Hide
          quen Sam Marshall added a comment -

          This is annoying, but according to current Moodle coding standards, the comment should have a full stop at the end:

          // Mark viewed by user (if required).

          Sorry I didn't notice this earlier.

          Also regarding testing instructions, two problems:

          1. These should show the expected behaviour AFTER the fix i.e. it should probably end with something like:

          EXPECTED: Activity is now marked completed

          2. 'Any LTI activity' isn't helpful for testers (who might not be familiar with LTI) as LTI activities are difficult to find and install. Is there an easy way to use a simple public LTI activity which you can describe how to set up as part of the test, i.e. give the URL for it?

          Show
          quen Sam Marshall added a comment - This is annoying, but according to current Moodle coding standards, the comment should have a full stop at the end: // Mark viewed by user (if required). Sorry I didn't notice this earlier. Also regarding testing instructions, two problems: 1. These should show the expected behaviour AFTER the fix i.e. it should probably end with something like: EXPECTED: Activity is now marked completed 2. 'Any LTI activity' isn't helpful for testers (who might not be familiar with LTI) as LTI activities are difficult to find and install. Is there an easy way to use a simple public LTI activity which you can describe how to set up as part of the test, i.e. give the URL for it?
          Hide
          joshjohnston Josh Johnston added a comment -

          No problem. I cut-n-paste coded it from another module so I guess they didn't have the full stop either.
          I will update the testing instructions now and update the diff branch shortly.

          Show
          joshjohnston Josh Johnston added a comment - No problem. I cut-n-paste coded it from another module so I guess they didn't have the full stop either. I will update the testing instructions now and update the diff branch shortly.
          Hide
          joshjohnston Josh Johnston added a comment -

          24, 25, and master branches have been updated with the code change from peer review

          Show
          joshjohnston Josh Johnston added a comment - 24, 25, and master branches have been updated with the code change from peer review
          Hide
          quen Sam Marshall added a comment -

          Thanks! Drat - one last thing (I should really use the checklist, maybe I would've noticed it straight away):

          [Y] Syntax
          [Y] Whitespace
          [-] Output
          [-] Language
          [-] Databases
          [Y] Testing (instructions and automated tests)
          [-] Security
          [-] Documentation
          [N] Git
          [Y] Sanity check

          • Could you rebase the two commits into a single one please, since this is a single change.
          • The commit message format should be something like:

          MDL-41520 LTI: Does not track page views for completion

          i.e. it must have the MDL- number in it and should then have a word or two describing the area of code, then some text explaining change (you can also add more information after a blank line if you like, but that isn't necessary for this change)

          Show
          quen Sam Marshall added a comment - Thanks! Drat - one last thing (I should really use the checklist, maybe I would've noticed it straight away): [Y] Syntax [Y] Whitespace [-] Output [-] Language [-] Databases [Y] Testing (instructions and automated tests) [-] Security [-] Documentation [N] Git [Y] Sanity check Could you rebase the two commits into a single one please, since this is a single change. The commit message format should be something like: MDL-41520 LTI: Does not track page views for completion i.e. it must have the MDL- number in it and should then have a word or two describing the area of code, then some text explaining change (you can also add more information after a blank line if you like, but that isn't necessary for this change)
          Hide
          joshjohnston Josh Johnston added a comment -

          Rebase complete for all three pull URLs. Enjoy =D

          Show
          joshjohnston Josh Johnston added a comment - Rebase complete for all three pull URLs. Enjoy =D
          Hide
          quen Sam Marshall added a comment -

          Thanks! Submitting for integration review, hopefully this will go in.

          Note: I assigned the issue to me because it needs to be assigned to somebody and I can't pick you, probably because you don't have developer access yet. After you've had a few patches accepted into the system, you can request developer access.

          Show
          quen Sam Marshall added a comment - Thanks! Submitting for integration review, hopefully this will go in. Note: I assigned the issue to me because it needs to be assigned to somebody and I can't pick you, probably because you don't have developer access yet. After you've had a few patches accepted into the system, you can request developer access.
          Hide
          joshjohnston Josh Johnston added a comment -

          Fix order of testing instructions

          Show
          joshjohnston Josh Johnston added a comment - Fix order of testing instructions
          Hide
          damyon Damyon Wiese added a comment -

          Thanks!

          This patch looks good - integrated to 24, 25 and master.

          Show
          damyon Damyon Wiese added a comment - Thanks! This patch looks good - integrated to 24, 25 and master.
          Hide
          joshjohnston Josh Johnston added a comment -

          Thanks! Is there a process I should be following to get my other requests for mod_lti looked at?

          Show
          joshjohnston Josh Johnston added a comment - Thanks! Is there a process I should be following to get my other requests for mod_lti looked at?
          Hide
          quen Sam Marshall added a comment -

          Josh: You got me looking at this one because it was related to activity completion, which I'm the maintainer for. I'm not offering to take on LTI in general, sorry If you haven't had response, I guess nobody is really maintaining it at the moment... Not honestly sure what you should do about that.

          Show
          quen Sam Marshall added a comment - Josh: You got me looking at this one because it was related to activity completion, which I'm the maintainer for. I'm not offering to take on LTI in general, sorry If you haven't had response, I guess nobody is really maintaining it at the moment... Not honestly sure what you should do about that.
          Hide
          joshjohnston Josh Johnston added a comment -

          Thanks Sam, I'll reach out to Chris and see what I can do. We use lti extensively as a consumer and provider so I will probably be fixing a lot of bugs as time goes by

          Show
          joshjohnston Josh Johnston added a comment - Thanks Sam, I'll reach out to Chris and see what I can do. We use lti extensively as a consumer and provider so I will probably be fixing a lot of bugs as time goes by
          Hide
          phalacee Jason Fowler added a comment -

          Works as expected, thank you Sam

          Show
          phalacee Jason Fowler added a comment - Works as expected, thank you Sam
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

          Or, if you prefer, yes, you fixed that boring issue.

          Thanks anyway! Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Nov/13