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

Quiz reports should use enroled users code, not get_users_by_capability

    Details

    • Testing Instructions:
      Hide

      Create a course, visible with visible groups.
      Create at least 4 students (s1 to s4).
      Enrol these students on the course.
      Put the students into two groups (Group A, and Group B).
      Create quiz1, visible with visible groups.
      Add an Essay question (Write something here...)
      Add a True/False question (Choose one...)
      Login as s1, start quiz1, answer questions correctly.
      Login as s2, answer one question correctly.
      Login as s3, start quiz but do not answer any questions.
      Login as s4, do not start quiz.
      Login as admin.
      Check all pages of the quiz reports (Administration > Quiz administration > Results) and every option with each page.

      Show
      Create a course, visible with visible groups. Create at least 4 students (s1 to s4). Enrol these students on the course. Put the students into two groups (Group A, and Group B). Create quiz1, visible with visible groups. Add an Essay question (Write something here...) Add a True/False question (Choose one...) Login as s1, start quiz1, answer questions correctly. Login as s2, answer one question correctly. Login as s3, start quiz but do not answer any questions. Login as s4, do not start quiz. Login as admin. Check all pages of the quiz reports (Administration > Quiz administration > Results) and every option with each page.
    • Affected Branches:
      MOODLE_20_STABLE
    • Pull Master Branch:
      wip-MDL-27072-MDL-31243-master

      Description

      This is spun off from MDL-27071.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              matt.clarkson Matt Clarkson added a comment -

              Attaching a patch that removes get_users_by_capability from quiz reporting.

              It's has not yet received through testing, but seems to work on a course with 2 Million users.

              This patch depends on MDL-34771 which tweaks get_enrolled_sql to support multiple capabilities.

              Show
              matt.clarkson Matt Clarkson added a comment - Attaching a patch that removes get_users_by_capability from quiz reporting. It's has not yet received through testing, but seems to work on a course with 2 Million users. This patch depends on MDL-34771 which tweaks get_enrolled_sql to support multiple capabilities.
              Hide
              matt.clarkson Matt Clarkson added a comment -

              Since MDL-34771 isn't going to go into core, I'm keen to know your preferred approach to determining which users appear in the quiz report. My current thinking is that users having an enrolment should be the only criteria required - a user having the capability for quiz:attempt or quiz:reviewmyattempts seems unnecessary with Moodle 2's concept of enrolment.

              Show
              matt.clarkson Matt Clarkson added a comment - Since MDL-34771 isn't going to go into core, I'm keen to know your preferred approach to determining which users appear in the quiz report. My current thinking is that users having an enrolment should be the only criteria required - a user having the capability for quiz:attempt or quiz:reviewmyattempts seems unnecessary with Moodle 2's concept of enrolment.
              Hide
              timhunt Tim Hunt added a comment -

              The preferred approach is MDL-31243.

              Get enrolled_users_sql generates one chunk of SQL, which really should not mention capabilities at all, other than as a convenience. A new function get_with_capability_sql should exist to generate other chunks of SQL. The quiz report can then use those chunks to build the query it wants: (enrolled AND (cap1 OR cap2)). This approach also reduces duplicate code in accesslib.php.

              Show
              timhunt Tim Hunt added a comment - The preferred approach is MDL-31243 . Get enrolled_users_sql generates one chunk of SQL, which really should not mention capabilities at all, other than as a convenience. A new function get_with_capability_sql should exist to generate other chunks of SQL. The quiz report can then use those chunks to build the query it wants: (enrolled AND (cap1 OR cap2)). This approach also reduces duplicate code in accesslib.php.
              Hide
              timhunt Tim Hunt added a comment -

              Just bumping workflow state.

              Show
              timhunt Tim Hunt added a comment - Just bumping workflow state.
              Hide
              timhunt Tim Hunt added a comment -

              Oh, just saw your question. The reason you need the with quiz:attempt or quiz:reviewmyattempts conditions is because teachers are enroled in the course, and you don't want them showing up in the quiz reports.

              We could consider changing the logic to (NOT quiz:preview).

              Show
              timhunt Tim Hunt added a comment - Oh, just saw your question. The reason you need the with quiz:attempt or quiz:reviewmyattempts conditions is because teachers are enroled in the course, and you don't want them showing up in the quiz reports. We could consider changing the logic to (NOT quiz:preview).
              Hide
              matt.clarkson Matt Clarkson added a comment -

              What about a new capability, something like quiz:showinreports?

              Show
              matt.clarkson Matt Clarkson added a comment - What about a new capability, something like quiz:showinreports?
              Hide
              timhunt Tim Hunt added a comment -

              That is bad grammar. You would need to call it quiz:appearinreports, but should this be quiz-specific, or should it be generic, like the gradebook roles setting, which should really be a capability.

              Show
              timhunt Tim Hunt added a comment - That is bad grammar. You would need to call it quiz:appearinreports, but should this be quiz-specific, or should it be generic, like the gradebook roles setting, which should really be a capability.
              Hide
              matt.clarkson Matt Clarkson added a comment -

              The patch-set I've linked to on github adds the new capability mod/quiz:appearinreports and replaces get_users_by_capability() with get_enrolled_sql(). Once I've got MDL-31243 accepted I'll add some more patches to replace get_enrolled_sql() with get_by_capability().

              Show
              matt.clarkson Matt Clarkson added a comment - The patch-set I've linked to on github adds the new capability mod/quiz:appearinreports and replaces get_users_by_capability() with get_enrolled_sql(). Once I've got MDL-31243 accepted I'll add some more patches to replace get_enrolled_sql() with get_by_capability().
              Hide
              timhunt Tim Hunt added a comment -

              Sorry but a new capability mod/quiz:appearinreports is a bad idea.

              Please use one of

              • enroled AND (has_capability quiz:attempt OR has_capability quiz:reviewmyattempts)
              • enroled AND NOT (has_capability quiz:preview)

              as I said above.

              Show
              timhunt Tim Hunt added a comment - Sorry but a new capability mod/quiz:appearinreports is a bad idea. Please use one of enroled AND (has_capability quiz:attempt OR has_capability quiz:reviewmyattempts) enroled AND NOT (has_capability quiz:preview) as I said above.
              Hide
              matt.clarkson Matt Clarkson added a comment -

              Can you explain why this is now a bad idea? You didn't appear to oppose the idea when I proposed it on the 14th Aug.

              My argument for using a new capability is:

              1. Capabilities should not be OR'd as it breaks the concept of prohibits.
              2. It provides more control and makes it clearer which roles appear in the quiz reporting.
              3. The code is cleaner.

              Show
              matt.clarkson Matt Clarkson added a comment - Can you explain why this is now a bad idea? You didn't appear to oppose the idea when I proposed it on the 14th Aug. My argument for using a new capability is: 1. Capabilities should not be OR'd as it breaks the concept of prohibits. 2. It provides more control and makes it clearer which roles appear in the quiz reporting. 3. The code is cleaner.
              Hide
              timhunt Tim Hunt added a comment -

              I have always thought a new capability was a bad idea. When you suggested a new capability, I immediately pointed out several problems with the proposal (http://tracker.moodle.org/browse/MDL-27072?focusedCommentId=172974&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-172974) - problems that you have not responded to.

              You are the one proposing to change how the quiz has always worked. The burden of proof is on you. And, your target audience for getting agreement is not me. You need to discuss with the users of the quiz, and get there consensus that you are right - if you are.

              The real problem with the new capability is that is redundant. The quiz reports should show the users who can attempt (but not preview) the quiz, or who could have done at some time, even if they can't now. The existing capabilities are already sufficient for that.

              Show
              timhunt Tim Hunt added a comment - I have always thought a new capability was a bad idea. When you suggested a new capability, I immediately pointed out several problems with the proposal ( http://tracker.moodle.org/browse/MDL-27072?focusedCommentId=172974&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-172974 ) - problems that you have not responded to. You are the one proposing to change how the quiz has always worked. The burden of proof is on you. And, your target audience for getting agreement is not me. You need to discuss with the users of the quiz, and get there consensus that you are right - if you are. The real problem with the new capability is that is redundant. The quiz reports should show the users who can attempt (but not preview) the quiz, or who could have done at some time, even if they can't now. The existing capabilities are already sufficient for that.
              Hide
              timhunt Tim Hunt added a comment -

              This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

              For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

              Show
              timhunt Tim Hunt added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
              Hide
              timhunt Tim Hunt added a comment -

              Dan, I don't know why you added the patch label here. I mean, yes, if you only bother to look for 0.5 seconds, you will see that there is a file called .patch attached. If you actually read the bug, you will see that I commented months ago why the patch did not solve the problem, and nothing has happened since then. So, there is no patch that currently needs to be reviewed.

              Show
              timhunt Tim Hunt added a comment - Dan, I don't know why you added the patch label here. I mean, yes, if you only bother to look for 0.5 seconds, you will see that there is a file called .patch attached. If you actually read the bug, you will see that I commented months ago why the patch did not solve the problem, and nothing has happened since then. So, there is no patch that currently needs to be reviewed.
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks Tim - we're doing a review of Catalyst contributed bugs in the tracker - hoping to spend some more time looking at them ourselves - feel free to ignore the noise

              Show
              danmarsden Dan Marsden added a comment - Thanks Tim - we're doing a review of Catalyst contributed bugs in the tracker - hoping to spend some more time looking at them ourselves - feel free to ignore the noise
              Hide
              timhunt Tim Hunt added a comment -

              Well, I am hoping that rather than ignoring noise, I will be able to peer-review a small and perfectly formed patch in due course.

              Show
              timhunt Tim Hunt added a comment - Well, I am hoping that rather than ignoring noise, I will be able to peer-review a small and perfectly formed patch in due course.
              Hide
              danmarsden Dan Marsden added a comment -

              unassigning myself as I'm not working on this at the moment - keeping it in our CatalystIT queue as we might pick it up again at some point.

              Show
              danmarsden Dan Marsden added a comment - unassigning myself as I'm not working on this at the moment - keeping it in our CatalystIT queue as we might pick it up again at some point.
              Hide
              jb23347 John Beedell added a comment -

              Assigning myself to this issue as it is important to the OU to have this fix. Peer review requested.

              Show
              jb23347 John Beedell added a comment - Assigning myself to this issue as it is important to the OU to have this fix. Peer review requested.
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (48 errors / 53 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (38/52) , js (0/0) , css (0/0) , phpdoc (4/0) , commit (6/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/1) , More information about this report
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (11 errors / 5 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (9/2) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (1/2) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/1) , More information about this report

                People

                • Votes:
                  5 Vote for this issue
                  Watchers:
                  8 Start watching this issue

                  Dates

                  • Created:
                    Updated: