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

          Attachments

            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