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 2.4 Branch:
      MDL-41520-MOODLE_24_STABLE-mod_lti-completion-view
    • Pull 2.5 Branch:
      MDL-41520-MOODLE_25_STABLE-mod_lti-completion-view
    • Pull Master Branch:
      MDL-41520-master-mod_lti-completion-view
    • Rank:
      52583

      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

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        Josh Johnston added a comment -

        Added the standard completion view magic

        Show
        Josh Johnston added a comment - Added the standard completion view magic
        Hide
        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
        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
        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
        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
        Josh Johnston added a comment -

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

        Show
        Josh Johnston added a comment - 24, 25, and master branches have been updated with the code change from peer review
        Hide
        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
        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
        Josh Johnston added a comment -

        Rebase complete for all three pull URLs. Enjoy =D

        Show
        Josh Johnston added a comment - Rebase complete for all three pull URLs. Enjoy =D
        Hide
        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
        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
        Josh Johnston added a comment -

        Fix order of testing instructions

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

        Thanks!

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

        Show
        Damyon Wiese added a comment - Thanks! This patch looks good - integrated to 24, 25 and master.
        Hide
        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
        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
        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
        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
        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
        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
        Jason Fowler added a comment -

        Works as expected, thank you Sam

        Show
        Jason Fowler added a comment - Works as expected, thank you Sam
        Hide
        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
        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: