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:
    • Rank:
      44269

      Description

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

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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 added a comment -

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

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          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
          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
          Dan Poltawski added a comment -

          Thanks Aaron, i've integrated this now

          Show
          Dan Poltawski added a comment - Thanks Aaron, i've integrated this now
          Hide
          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
          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
          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
          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
          Aaron Barnes added a comment -

          Fix misnamed variable and made testing instructions clearer.

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

          Integrated, thanks Aaron

          Show
          Dan Poltawski added a comment - Integrated, thanks Aaron
          Hide
          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
          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
          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
          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: