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

Improve user completion data permission checking

    Details

    • Testing Instructions:
      Hide

      Enable course completion site-wide in advanced settings.
      Create a new course with completion enabled.
      Enrol multiple users in the course.
      Apply some criteria to the course view the Completion Tracking course settings page.
      Add the Course Completion Status block to the course.
      Login as a user and check they can view the report linked to in the bottom of the block (the "Details" link).
      Modify the URL to include a different user's id (append ?user=x to url), make sure a student can not view another students report.
      Create a teacher for the course and logged in as them visit the reports from previous steps. Teachers should be able to view these reports.

      Show
      Enable course completion site-wide in advanced settings. Create a new course with completion enabled. Enrol multiple users in the course. Apply some criteria to the course view the Completion Tracking course settings page. Add the Course Completion Status block to the course. Login as a user and check they can view the report linked to in the bottom of the block (the "Details" link). Modify the URL to include a different user's id (append ?user=x to url), make sure a student can not view another students report. Create a teacher for the course and logged in as them visit the reports from previous steps. Teachers should be able to view these reports.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      Improve and encapsulate the logic around checking if a user's completion information for a course is viewable by the logged in user.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that, Aaron.

            This should have a set of test instructions before it is peer reviewed.

            If you are interested in what is (or should be) checked during peer review, see http://docs.moodle.org/dev/Peer_reviewing_checklist

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that, Aaron. This should have a set of test instructions before it is peer reviewed. If you are interested in what is (or should be) checked during peer review, see http://docs.moodle.org/dev/Peer_reviewing_checklist
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Aaron,

            These changes look spot on thanks.
            The only thing I noticed was re: coding style, should be } else { rather than }*\n*else {.
            Otherwise spot on, add testing instructions and put up for integration when ready.
            If you're unable to (sorry I loose track) just comment when ready and I'll put it up for you.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Aaron, These changes look spot on thanks. The only thing I noticed was re: coding style, should be } else { rather than }*\n*else { . Otherwise spot on, add testing instructions and put up for integration when ready. If you're unable to (sorry I loose track) just comment when ready and I'll put it up for you. Cheers Sam
            Hide
            nebgor Aparup Banerjee 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
            nebgor Aparup Banerjee 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 have some concerns about removing the require_login() call from blocks/completionstatus/details.php, I think that this means that if a user gets logged out they will be presented with an error message rather than redirected to the login screen to see what they want.

            Was this change by design?

            Show
            poltawski Dan Poltawski added a comment - Hi Aaron, I have some concerns about removing the require_login() call from blocks/completionstatus/details.php, I think that this means that if a user gets logged out they will be presented with an error message rather than redirected to the login screen to see what they want. Was this change by design?
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            sry_not4sale Aaron Barnes added a comment -

            Hi Dan,

            Great spotting, an apologies for not getting you an answer last week. I have readded the require_login check (but without the course requirement).

            Thanks,
            Aaron

            Show
            sry_not4sale Aaron Barnes added a comment - Hi Dan, Great spotting, an apologies for not getting you an answer last week. I have readded the require_login check (but without the course requirement). Thanks, Aaron
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Aaron, i've integrated this now

            Show
            poltawski Dan Poltawski added a comment - Thanks Aaron, i've integrated this now
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Errors during testing

            I'm seeing a string error on the report...

            Invalid get_string() identifier: 'aggregateall' or component 'completion'. Perhaps you are missing $string['aggregateall'] = ''; in lang/en/completion.php?
            line 6727 of \lib\moodlelib.php: call to debugging()
            line 7388 of \lib\moodlelib.php: call to core_string_manager->get_string()
            line 203 of \blocks\completionstatus\details.php: call to get_string()

            Manually changing the userid as a GET parameter didn't seem to make any change. I only saw the report for the current user. I'm not sure if that is good or if I should have seen a warning/error.

            When viewing the Course report as a teacher, there was an error repeated (probably once for each student).

            Notice: Undefined variable: courseid in D:\xampp\htdocs\moodle_master_test_mysql\lib\completionlib.php on line 202

            Show
            salvetore Michael de Raadt added a comment - Test result: Errors during testing I'm seeing a string error on the report... Invalid get_string() identifier: 'aggregateall' or component 'completion'. Perhaps you are missing $string['aggregateall'] = ''; in lang/en/completion.php? line 6727 of \lib\moodlelib.php: call to debugging() line 7388 of \lib\moodlelib.php: call to core_string_manager->get_string() line 203 of \blocks\completionstatus\details.php: call to get_string() Manually changing the userid as a GET parameter didn't seem to make any change. I only saw the report for the current user. I'm not sure if that is good or if I should have seen a warning/error. When viewing the Course report as a teacher, there was an error repeated (probably once for each student). Notice: Undefined variable: courseid in D:\xampp\htdocs\moodle_master_test_mysql\lib\completionlib.php on line 202
            Hide
            poltawski Dan Poltawski added a comment -

            The get_string call I think is an existing problem.

            But the second issue is from this patch and suggests its been tested down that avenue. Combined with the fact that this is last second, i'm going to have to revert this.

            Show
            poltawski Dan Poltawski added a comment - The get_string call I think is an existing problem. But the second issue is from this patch and suggests its been tested down that avenue. Combined with the fact that this is last second, i'm going to have to revert this.
            Hide
            sry_not4sale Aaron Barnes added a comment -

            Fix misnamed variable and made testing instructions clearer.

            Show
            sry_not4sale Aaron Barnes added a comment - Fix misnamed variable and made testing instructions clearer.
            Hide
            poltawski Dan Poltawski added a comment -

            Integrated, thanks Aaron

            Show
            poltawski Dan Poltawski added a comment - Integrated, thanks Aaron
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            Tested in Master only.

            I was able to switch users as a teacher, but not as a student.

            I found the string error previously reported and I also found that the navigation is broken on the report page.

            Show
            salvetore Michael de Raadt added a comment - Test result: Success! Tested in Master only. I was able to switch users as a teacher, but not as a student. I found the string error previously reported and I also found that the navigation is broken on the report page.
            Hide
            poltawski Dan Poltawski added a comment -

            Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            Show
            poltawski Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13