Moodle
  1. Moodle
  2. MDL-27072

Quiz reports should use enroled users code, not get_users_by_capability

    Details

    • Rank:
      16710

      Description

      This is spun off from MDL-27071.

        Issue Links

          Activity

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

          Just bumping workflow state.

          Show
          Tim Hunt added a comment - Just bumping workflow state.
          Hide
          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
          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 added a comment -

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

          Show
          Matt Clarkson added a comment - What about a new capability, something like quiz:showinreports?
          Hide
          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
          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 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 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
          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
          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 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 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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated: