Moodle
  1. Moodle
  2. MDL-37473

Completion blocks do not respect new isincompletionreports capability

    Details

    • Testing Instructions:
      Hide

      1. Enable completion site-wide under "Advanced Features"
      2. Create a new course and make sure completion is enabled
      3. Create some completion criteria for the course (make one of which "Self completion" - enable it), and add the Course Completion Status and Self Completion blocks
      4. Enrol two user's in the course - a student and an editing teacher.
      5. Login as the student and ensure the Course Completion Status block shows the criteria and the user's state in them. Ensure the Self Completion block shows a link for the user to complete the course.
      6. Login as the teacher, ensure the blocks do not display the information displayed for students.
      7. Add the "isincompletionreports" capability to the editing teacher role. Ensure the block now displays data for the teacher as if they were a student (as in step 5).

      Show
      1. Enable completion site-wide under "Advanced Features" 2. Create a new course and make sure completion is enabled 3. Create some completion criteria for the course (make one of which "Self completion" - enable it), and add the Course Completion Status and Self Completion blocks 4. Enrol two user's in the course - a student and an editing teacher. 5. Login as the student and ensure the Course Completion Status block shows the criteria and the user's state in them. Ensure the Self Completion block shows a link for the user to complete the course. 6. Login as the teacher, ensure the blocks do not display the information displayed for students. 7. Add the "isincompletionreports" capability to the editing teacher role. Ensure the block now displays data for the teacher as if they were a student (as in step 5).
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37473b-24
    • Pull Master Branch:
    • Rank:
      47111

      Description

      When MDL-32107 was fixed completion changed to track all users enrolled in a course, rather than just user's who had a tracked role in the course (normally just learner).

      This now causes the Course Completion Status and Self Completion blocks to display tracking details when teachers are logged in.

      MDL-34228 introduced a new capability isincompletionreports for enabling/disabling this for roles/users, and the blocks need to be modified to respect this e.g. only dislaying tracking information if the logged in user does has this capability.

        Issue Links

          Activity

          Hide
          Aaron Barnes added a comment -

          Investigating further it seems like MDL-34228 only added the capability check to the function get_tracked_users(), but get_num_tracked_users() and is_tracked_user() were both missed.

          Was this intentional Sam, or should I update both those functions with the same check?

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - Investigating further it seems like MDL-34228 only added the capability check to the function get_tracked_users(), but get_num_tracked_users() and is_tracked_user() were both missed. Was this intentional Sam, or should I update both those functions with the same check? Cheers, Aaron
          Hide
          Aaron Barnes added a comment -

          Checked the completion report and with a teacher present the participant count and the number of participants shown is mismatched, so unless Sam has a reason to not add the check I'll put this up for peer review.

          Show
          Aaron Barnes added a comment - Checked the completion report and with a teacher present the participant count and the number of participants shown is mismatched, so unless Sam has a reason to not add the check I'll put this up for peer review.
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this, Aaron.

          Show
          Michael de Raadt added a comment - Thanks for working on this, Aaron.
          Hide
          Sam Marshall added a comment -

          Sorry Aaron - you are correct, I missed those two functions!

          The change looks correct to me.

          Show
          Sam Marshall added a comment - Sorry Aaron - you are correct, I missed those two functions! The change looks correct to me.
          Hide
          Sam Marshall added a comment -

          +1 from me

          Show
          Sam Marshall added a comment - +1 from me
          Hide
          Aaron Barnes added a comment -

          Awesome, thanks for the prompt attention Sam!

          Show
          Aaron Barnes added a comment - Awesome, thanks for the prompt attention Sam!
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi Aaron,

          I think this should be backported to 23/24?

          Show
          Dan Poltawski added a comment - Hi Aaron, I think this should be backported to 23/24?
          Hide
          Aaron Barnes added a comment -

          Hi Dan,

          I totally agree! I'll create the required branches.

          Aaron

          Show
          Aaron Barnes added a comment - Hi Dan, I totally agree! I'll create the required branches. Aaron
          Hide
          Dan Poltawski added a comment -

          Thanks, integrated to master, 24 and 23.

          Show
          Dan Poltawski added a comment - Thanks, integrated to master, 24 and 23.
          Hide
          Dan Poltawski added a comment -

          argh, just noticed no testing instructions. Please can you provide some, Aaron.

          Show
          Dan Poltawski added a comment - argh, just noticed no testing instructions. Please can you provide some, Aaron.
          Hide
          Ankit Agarwal added a comment -

          Hi Aaron,
          This works as described(So far tested in master). I cannot find any docs relating to this cap, so it might be worth adding the docs required tag. Am not sure how this cap is supposed to behave exactly, but here are a few things I noted:-

          1. If teacher doesn't have this cap he is shown a msg "you are not enrolled in this course" which is confusing as the teacher is enrolled.
          2. course/togglecompletion.php?course=9 is accessible irrespective of the cap.

          Please confirm these are expected/known behaviours before I go ahead and test on rest of the branches.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Aaron, This works as described(So far tested in master). I cannot find any docs relating to this cap, so it might be worth adding the docs required tag. Am not sure how this cap is supposed to behave exactly, but here are a few things I noted:- If teacher doesn't have this cap he is shown a msg "you are not enrolled in this course" which is confusing as the teacher is enrolled. course/togglecompletion.php?course=9 is accessible irrespective of the cap. Please confirm these are expected/known behaviours before I go ahead and test on rest of the branches. Thanks
          Hide
          Aaron Barnes added a comment -

          Nope, that's a fail. ABORT ABORT!

          Thanks, will fix!

          Show
          Aaron Barnes added a comment - Nope, that's a fail. ABORT ABORT! Thanks, will fix!
          Hide
          Ankit Agarwal added a comment -

          Thanks for the confirmation, failing the test.

          Show
          Ankit Agarwal added a comment - Thanks for the confirmation, failing the test.
          Hide
          Dan Poltawski added a comment -

          ping?

          Show
          Dan Poltawski added a comment - ping?
          Hide
          Aaron Barnes added a comment -

          Won't have a chance to fix this until late today at the earliest I'm afraid.

          Show
          Aaron Barnes added a comment - Won't have a chance to fix this until late today at the earliest I'm afraid.
          Hide
          Aaron Barnes added a comment -

          How does "You are currently not being tracked by Course Completion in this course" sound?

          togglecompletion.php is a bit of a mess, lot's of old style code - I'll just add a is_tracked_user() check if that's enough? I don't want to diverge to far from the bug's original intent.

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - How does "You are currently not being tracked by Course Completion in this course" sound? togglecompletion.php is a bit of a mess, lot's of old style code - I'll just add a is_tracked_user() check if that's enough? I don't want to diverge to far from the bug's original intent. Cheers, Aaron
          Hide
          Dan Poltawski added a comment -

          Sounds OK to me.

          Sorry to 'push', I know you can't work around our schedule! But we'll need a solution for this in the next few hours else we'll need to revert the change.

          Show
          Dan Poltawski added a comment - Sounds OK to me. Sorry to 'push', I know you can't work around our schedule! But we'll need a solution for this in the next few hours else we'll need to revert the change.
          Hide
          Aaron Barnes added a comment -

          There ya go Dan

          Show
          Aaron Barnes added a comment - There ya go Dan
          Hide
          Dan Poltawski added a comment -

          Thanks a lot Aaron.

          Ankit, please can you re-test.

          Show
          Dan Poltawski added a comment - Thanks a lot Aaron. Ankit, please can you re-test.
          Hide
          Ankit Agarwal added a comment -

          Looks good to me, can be passed.
          Noticed a minor issue.
          completion is enabled and self completion is not added. if you visit togglecompletion.php?course=9 when you have the cap, you can access it which is wrong since self completion is not enabled.

          Please suggest to pass/fail
          Thanks

          Show
          Ankit Agarwal added a comment - Looks good to me, can be passed. Noticed a minor issue. completion is enabled and self completion is not added. if you visit togglecompletion.php?course=9 when you have the cap, you can access it which is wrong since self completion is not enabled. Please suggest to pass/fail Thanks
          Hide
          Aparup Banerjee added a comment -

          if its not a regression i'd suggest passing.

          Show
          Aparup Banerjee added a comment - if its not a regression i'd suggest passing.
          Hide
          Aaron Barnes added a comment - - edited

          I suggest "pass", as that is a separate issue.

          I have created the bug MDL-37685 to track fixing the issue you mention (and also noted the block needs a general tidy-up).

          Cheers,
          Aaron

          Show
          Aaron Barnes added a comment - - edited I suggest "pass", as that is a separate issue. I have created the bug MDL-37685 to track fixing the issue you mention (and also noted the block needs a general tidy-up). Cheers, Aaron
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao
          Hide
          Mary Cooch added a comment - - edited

          Removing docs_required as this capability is documented in 2.3 and 2.4 docs: http://docs.moodle.org/en/Capabilities/moodle/course:isincompletionreports

          Show
          Mary Cooch added a comment - - edited Removing docs_required as this capability is documented in 2.3 and 2.4 docs: http://docs.moodle.org/en/Capabilities/moodle/course:isincompletionreports
          Hide
          Amanda Doughty added a comment -

          This causes another regression. If a teacher does not have capability isincompletionreports then they are not able to mark approval for a student, in the course completion report.

          Show
          Amanda Doughty added a comment - This causes another regression. If a teacher does not have capability isincompletionreports then they are not able to mark approval for a student, in the course completion report.
          Hide
          Dan Poltawski added a comment -

          Thanks Amanda! Please can you create a new issue for it, and link to this issue?

          Show
          Dan Poltawski added a comment - Thanks Amanda! Please can you create a new issue for it, and link to this issue?

            People

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

              Dates

              • Created:
                Updated:
                Resolved: