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

      Description

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

        Gliffy Diagrams

        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: