Uploaded image for project: '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 Master Branch:

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              sry_not4sale 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
              sry_not4sale 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
              sry_not4sale 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
              sry_not4sale 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
              salvetore Michael de Raadt added a comment -

              Thanks for working on this, Aaron.

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

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

              The change looks correct to me.

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

              +1 from me

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

              Awesome, thanks for the prompt attention Sam!

              Show
              sry_not4sale Aaron Barnes added a comment - Awesome, thanks for the prompt attention Sam!
              Hide
              poltawski 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
              poltawski 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
              poltawski Dan Poltawski added a comment -

              Hi Aaron,

              I think this should be backported to 23/24?

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

              Hi Dan,

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

              Aaron

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

              Thanks, integrated to master, 24 and 23.

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

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

              Show
              poltawski Dan Poltawski added a comment - argh, just noticed no testing instructions. Please can you provide some, Aaron.
              Hide
              ankit_frenz 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_frenz 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
              sry_not4sale Aaron Barnes added a comment -

              Nope, that's a fail. ABORT ABORT!

              Thanks, will fix!

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

              Thanks for the confirmation, failing the test.

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

              ping?

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

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

              Show
              sry_not4sale Aaron Barnes added a comment - Won't have a chance to fix this until late today at the earliest I'm afraid.
              Hide
              sry_not4sale 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
              sry_not4sale 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
              poltawski 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
              poltawski 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
              sry_not4sale Aaron Barnes added a comment -

              There ya go Dan

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

              Thanks a lot Aaron.

              Ankit, please can you re-test.

              Show
              poltawski Dan Poltawski added a comment - Thanks a lot Aaron. Ankit, please can you re-test.
              Hide
              ankit_frenz 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_frenz 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
              nebgor Aparup Banerjee added a comment -

              if its not a regression i'd suggest passing.

              Show
              nebgor Aparup Banerjee added a comment - if its not a regression i'd suggest passing.
              Hide
              sry_not4sale 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
              sry_not4sale 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
              stronk7 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
              stronk7 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
              marycooch 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
              marycooch 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 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 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
              poltawski Dan Poltawski added a comment -

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

              Show
              poltawski 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:
                    Fix Release Date:
                    14/Jan/13