Moodle
  1. Moodle
  2. MDL-14190 New Quiz Report Plug in Features
  3. MDL-15326

allow for per report capabilities to replace mod/quiz:viewreports

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Quiz
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      26409

      Description

      Extra capabilities are still needed for manual grading and regrading report. In that case participants must have both capabilities to view the report.

      1. permissionschanges.txt
        38 kB
        Jamie Pratt
      2. permissionschanges2.txt
        24 kB
        Jamie Pratt

        Issue Links

          Activity

          Hide
          Jamie Pratt added a comment -

          There is now a new function that allows you to list capabilities to override at the module level other than mod/

          {modulename}:{capname}.

          The new function is called {modulename}

          _get_extra_capabilities

          Currently we seem to agree that capabilities should be defined in report/

          {reportname}/db/access.php file. And that each report should have a capability name quizreport/{reportname}

          :view

          Unfortunately, in the UI to override the capabilities, the capabilities will likely appear under different headings to each other and different headings to the quiz capabilities.

          Not quite sure how we want to handle capabilities for grade and regrade now. Regrade is now not a separate report so there will be no quizreport/regrade:view capability. You said previously that you thought quizreport/grading:view should also control manual grading capabilities elsewhere across the quiz.

          Show
          Jamie Pratt added a comment - There is now a new function that allows you to list capabilities to override at the module level other than mod/ {modulename}:{capname}. The new function is called {modulename} _get_extra_capabilities Currently we seem to agree that capabilities should be defined in report/ {reportname}/db/access.php file. And that each report should have a capability name quizreport/{reportname} :view Unfortunately, in the UI to override the capabilities, the capabilities will likely appear under different headings to each other and different headings to the quiz capabilities. Not quite sure how we want to handle capabilities for grade and regrade now. Regrade is now not a separate report so there will be no quizreport/regrade:view capability. You said previously that you thought quizreport/grading:view should also control manual grading capabilities elsewhere across the quiz.
          Hide
          Jamie Pratt added a comment -

          "Unfortunately, in the UI to override the capabilities, the capabilities will likely appear under different headings to each other and different headings to the quiz capabilities. "

          We can add an exception to component_level_changed to make the capabilities appear under one heading in the module view. But they don't even appear together in the course view. The capabilities are displayed sorted by real name so mod/quiz is not near quizreport/overview

          Show
          Jamie Pratt added a comment - "Unfortunately, in the UI to override the capabilities, the capabilities will likely appear under different headings to each other and different headings to the quiz capabilities. " We can add an exception to component_level_changed to make the capabilities appear under one heading in the module view. But they don't even appear together in the course view. The capabilities are displayed sorted by real name so mod/quiz is not near quizreport/overview
          Hide
          Tim Hunt added a comment -

          I think we need to fix this so that these capabilities all appear together under the quiz heading, and after the mod/quiz capabilities - that is definitely the right interface from the user's point of view. However, we have to find a way to achieve that without doing anything too hacky in accesslib.php. I am happy to review any patch you come up with, and suggest ways to make it less hacky.

          I think regrading is a quiz feature - it just so happens that the UI is shown in the overview report. It should either be controlled by the manual grading capability (mod/quiz:grade) or perhaps it should be a now capability. Or perhaps it should be linked to mod/quiz:manage, since that is what lets you change the question weights and cause a regrade to be necessary. I just added Phil as a watcher in case he has a view.

          Show
          Tim Hunt added a comment - I think we need to fix this so that these capabilities all appear together under the quiz heading, and after the mod/quiz capabilities - that is definitely the right interface from the user's point of view. However, we have to find a way to achieve that without doing anything too hacky in accesslib.php. I am happy to review any patch you come up with, and suggest ways to make it less hacky. I think regrading is a quiz feature - it just so happens that the UI is shown in the overview report. It should either be controlled by the manual grading capability (mod/quiz:grade) or perhaps it should be a now capability. Or perhaps it should be linked to mod/quiz:manage, since that is what lets you change the question weights and cause a regrade to be necessary. I just added Phil as a watcher in case he has a view.
          Hide
          Jamie Pratt added a comment - - edited

          I have attched a patch "permissionschanges.txt " to add and use the new capabilities quizreport/*:view and quizreport/overview:regrade

          I have not fixed the UI code for grouping capabilities.

          Show
          Jamie Pratt added a comment - - edited I have attched a patch "permissionschanges.txt " to add and use the new capabilities quizreport/*:view and quizreport/overview:regrade I have not fixed the UI code for grouping capabilities.
          Hide
          Jamie Pratt added a comment -

          Tim,

          Shall I go ahead and apply this patch to HEAD?

          Jamie

          Show
          Jamie Pratt added a comment - Tim, Shall I go ahead and apply this patch to HEAD? Jamie
          Hide
          Tim Hunt added a comment -

          Sorry, I forgot about this. Looking.

          Show
          Tim Hunt added a comment - Sorry, I forgot about this. Looking.
          Hide
          Tim Hunt added a comment -

          Sorry Jamie, but I don't like this patch.

          1. It is doing two database queries for each call to quiz_report_has_view_capability, and on some pages, there will be several such calls. That is really bad for performance. (Sure, we could add caching, but ...)

          2. We have to refer to some of the capabilities, like mod/quiz:viewreports and mod/quiz:grade (to use the old names) outside of the reports. That says to me that they are nor really report capabilities. It is easier to understand if they remain quiz capabilities. (However, splitting mod/quiz:grade into two mod/quiz:grade = Able to manually grade individual questions and mod/quiz:regrade = Able to do regrades is a good idea.)

          3. I can't think of any convincing case where you would want to allow access to the overview report but not the responses report, or vice-versa, so just having one capability mod/quiz:viewreports for both (and for reviewing other people's attempts elsewhere in the quiz code) makes sense to me.

          4. For any developer who just wants to knock together a simple customised quiz report, it would be nice if they did not have to worry about defining a capability unless they really needed to.

          I know you have done what I originally specified, but I have now changed my mind about how this should work. Sorry.

          There are some use-cases that my original proposal was supposed to address, and which I still think we should do something about. They are:

          A. Someone wants a make a quiz report plugin showing some information to people who do not have the mod/quiz:viewreports capability. For example, a helpdesk report that lets help desk staff see just the times when each user submitted their last attempt, for an organisation where quiz grades and responses are considered highly confidential.

          B. Someone wants a restricted report, so that even if you have 'mod/quiz:viewreports' you may not see this restricted report. For example, a University has a lot of teachers that are confused by the statistics report and this is costing a fortune in calls to the help desk. Therefore, the managers just want to hide the statistics report from ordinary teachers.

          C. Someone wants to make a report with some sub-functionality controlled by a separate capability. A (not very good) example, in the helpdesk report from A, should help desk staff see the number of attempts of each user, in addition to the time of their most recent attempt?

          I would really like to meet these needs while introducing the minimal amount of extra complexity. Here is my proposal:

          C is easy to cope with, providing reports can define capabilities if they need to. Then the report developer can use whatever has_capability calls they like in the code. We just need the bit that makes these capabilities available for overriding.

          A. and B. need a bit more, because we need to control which tabs are visible (as efficiently as possible!). My suggestion now is that we add a requiredcapability column to the quiz_report table that defaults to mod/quiz:viewreports, and that this capability is used to control whether the report appears in the tab bar, and in the require_capability call in mod/quiz/report.php.

          Hmm, I can't see right now where in the install/upgrade code the quiz_report table is initialised. Therefore I can't see how hard it would be to let each report specify the capability it wants to require at install time.

          And, in a default install of Moodle, I would like to see just the following list of capabilities that affect the reports

          mod/quiz:viewreports
          mod/quiz:grade
          mod/quiz:regrade
          quizreport/statistics:view

          and the requiredcapability for the standard reports would be

          Grades - mod/quiz:viewreports
          Results - mod/quiz:viewreports
          Statistics - quizreport/statistics:view
          Manual grading - mod/quiz:grade

          Comments?

          Show
          Tim Hunt added a comment - Sorry Jamie, but I don't like this patch. 1. It is doing two database queries for each call to quiz_report_has_view_capability, and on some pages, there will be several such calls. That is really bad for performance. (Sure, we could add caching, but ...) 2. We have to refer to some of the capabilities, like mod/quiz:viewreports and mod/quiz:grade (to use the old names) outside of the reports. That says to me that they are nor really report capabilities. It is easier to understand if they remain quiz capabilities. (However, splitting mod/quiz:grade into two mod/quiz:grade = Able to manually grade individual questions and mod/quiz:regrade = Able to do regrades is a good idea.) 3. I can't think of any convincing case where you would want to allow access to the overview report but not the responses report, or vice-versa, so just having one capability mod/quiz:viewreports for both (and for reviewing other people's attempts elsewhere in the quiz code) makes sense to me. 4. For any developer who just wants to knock together a simple customised quiz report, it would be nice if they did not have to worry about defining a capability unless they really needed to. I know you have done what I originally specified, but I have now changed my mind about how this should work. Sorry. There are some use-cases that my original proposal was supposed to address, and which I still think we should do something about. They are: A. Someone wants a make a quiz report plugin showing some information to people who do not have the mod/quiz:viewreports capability. For example, a helpdesk report that lets help desk staff see just the times when each user submitted their last attempt, for an organisation where quiz grades and responses are considered highly confidential. B. Someone wants a restricted report, so that even if you have 'mod/quiz:viewreports' you may not see this restricted report. For example, a University has a lot of teachers that are confused by the statistics report and this is costing a fortune in calls to the help desk. Therefore, the managers just want to hide the statistics report from ordinary teachers. C. Someone wants to make a report with some sub-functionality controlled by a separate capability. A (not very good) example, in the helpdesk report from A, should help desk staff see the number of attempts of each user, in addition to the time of their most recent attempt? I would really like to meet these needs while introducing the minimal amount of extra complexity. Here is my proposal: C is easy to cope with, providing reports can define capabilities if they need to. Then the report developer can use whatever has_capability calls they like in the code. We just need the bit that makes these capabilities available for overriding. A. and B. need a bit more, because we need to control which tabs are visible (as efficiently as possible!). My suggestion now is that we add a requiredcapability column to the quiz_report table that defaults to mod/quiz:viewreports, and that this capability is used to control whether the report appears in the tab bar, and in the require_capability call in mod/quiz/report.php. Hmm, I can't see right now where in the install/upgrade code the quiz_report table is initialised. Therefore I can't see how hard it would be to let each report specify the capability it wants to require at install time. And, in a default install of Moodle, I would like to see just the following list of capabilities that affect the reports mod/quiz:viewreports mod/quiz:grade mod/quiz:regrade quizreport/statistics:view and the requiredcapability for the standard reports would be Grades - mod/quiz:viewreports Results - mod/quiz:viewreports Statistics - quizreport/statistics:view Manual grading - mod/quiz:grade Comments?
          Hide
          Jamie Pratt added a comment -

          Your suggestions sound good. Am working on a new patch.

          Show
          Jamie Pratt added a comment - Your suggestions sound good. Am working on a new patch.
          Hide
          Tim Hunt added a comment -

          Brilliant. Thanks Jamie. And once again, sorry for not getting it right the first time.

          Show
          Tim Hunt added a comment - Brilliant. Thanks Jamie. And once again, sorry for not getting it right the first time.
          Hide
          Jamie Pratt added a comment -

          I attach a new patch 'permissionschanges2.txt' that implements the changes you outlined above Tim.

          Show
          Jamie Pratt added a comment - I attach a new patch 'permissionschanges2.txt' that implements the changes you outlined above Tim.
          Hide
          Tim Hunt added a comment -

          Sorry did not get round to this today. I should have time tomorrow.

          Show
          Tim Hunt added a comment - Sorry did not get round to this today. I should have time tomorrow.
          Hide
          Jamie Pratt added a comment -

          Attaching a new version of permissionchanges2.txt patch that fixes a small issue that the quiz_report_list function was not available from review.php which also has the report tab enabled.

          Show
          Jamie Pratt added a comment - Attaching a new version of permissionchanges2.txt patch that fixes a small issue that the quiz_report_list function was not available from review.php which also has the report tab enabled.
          Hide
          Tim Hunt added a comment -

          As far as I can see, on upgrade, you don't insert the right capability for the statistics report into the quiz_report table.

          Apart from that, the code looks good. Do you want me to test it as well before you commit it?

          Thanks.

          Show
          Tim Hunt added a comment - As far as I can see, on upgrade, you don't insert the right capability for the statistics report into the quiz_report table. Apart from that, the code looks good. Do you want me to test it as well before you commit it? Thanks.
          Hide
          Tim Hunt added a comment -

          OK, am testing.

          I obviously failed to read your code carefully, because the quiz_report table was updated properly.

          However, two minor bugs.

          1. I don't see the option to override quizreport/statistics:view in the quiz context. (But I was able to override it in the course context.)

          2. After I prevent teachers from seeing having quizreport/statistics:view, I can still see the stats report!

          Ah, this is just one bug, in quiz_report_list, one of the if statements needs to be changed to

          //add any other reports on the end
          foreach ($reportdirs as $reportname) {
          if (!isset($reportcaps[$reportname]))

          { $reportcaps[$reportname] = 'mod/quiz:viewreports'; }

          }

          Show
          Tim Hunt added a comment - OK, am testing. I obviously failed to read your code carefully, because the quiz_report table was updated properly. However, two minor bugs. 1. I don't see the option to override quizreport/statistics:view in the quiz context. (But I was able to override it in the course context.) 2. After I prevent teachers from seeing having quizreport/statistics:view, I can still see the stats report! Ah, this is just one bug, in quiz_report_list, one of the if statements needs to be changed to //add any other reports on the end foreach ($reportdirs as $reportname) { if (!isset($reportcaps [$reportname] )) { $reportcaps[$reportname] = 'mod/quiz:viewreports'; } }
          Hide
          Tim Hunt added a comment -

          For 1. I think this is a suitable rewrite of quiz_get_extra_capabilities.

          /**

          • @return array all other caps used in module
            */
            function quiz_get_extra_capabilities() { global $DB; $caps = question_get_all_capabilities(); $reportcaps = $DB->get_records_select_menu('capabilities', 'name LIKE ?', array('quizreport/%'), 'id,name'); $caps = array_merge($caps, $reportcaps); $caps[] = 'moodle/site:accessallgroups'; return $caps; }
          Show
          Tim Hunt added a comment - For 1. I think this is a suitable rewrite of quiz_get_extra_capabilities. /** @return array all other caps used in module */ function quiz_get_extra_capabilities() { global $DB; $caps = question_get_all_capabilities(); $reportcaps = $DB->get_records_select_menu('capabilities', 'name LIKE ?', array('quizreport/%'), 'id,name'); $caps = array_merge($caps, $reportcaps); $caps[] = 'moodle/site:accessallgroups'; return $caps; }
          Hide
          Tim Hunt added a comment -

          Given those two changes, I think this can be committed. Thanks.

          Show
          Tim Hunt added a comment - Given those two changes, I think this can be committed. Thanks.
          Hide
          Jamie Pratt added a comment -

          I committed the patch with Tim's suggest changes.

          Show
          Jamie Pratt added a comment - I committed the patch with Tim's suggest changes.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: