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

          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