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
      1. Create a course, make visible and add 3 groups.
      2. Create at least 4 students (s1 to s4).
      3. Enrol these students on the course.
      4. Put the students into two groups (leave one group empty).
      5. Create quiz1, with visible groups.
      6. Add an Essay question (Write something here...) - for manual marking
      7. Add a True/False question (Choose one...) - for auto marking
      8. Login as s1, start quiz1, answer questions correctly.
      9. Login as s2, answer one question correctly.
      10. Login as s3, start quiz but do not answer any questions.
      11. Login as s4, do not start quiz.
      12. Login as admin.
      13. Check all pages of the quiz reports (Administration > Quiz administration > Results) and every option with each page.
      14. Be sure to test features on the Quiz Grades and Responses reports, such as selecting attempts and deleting them, or re-grading attempts.
      15. The above will prove that nothing is broken.
      16. Now to prove that this change actually works it is necessary to start with a course created by admin/tool/generator/maketestcourse.php with 50,000 enrolled users (look at the code to work out the setting required), and then do all the steps above on that course. Without these code changes many of the quiz reports do not work.
      Show
      Create a course, make visible and add 3 groups. Create at least 4 students (s1 to s4). Enrol these students on the course. Put the students into two groups (leave one group empty). Create quiz1, with visible groups. Add an Essay question (Write something here...) - for manual marking Add a True/False question (Choose one...) - for auto marking 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. Be sure to test features on the Quiz Grades and Responses reports, such as selecting attempts and deleting them, or re-grading attempts. The above will prove that nothing is broken. Now to prove that this change actually works it is necessary to start with a course created by admin/tool/generator/maketestcourse.php with 50,000 enrolled users (look at the code to work out the setting required), and then do all the steps above on that course. Without these code changes many of the quiz reports do not work.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_32_STABLE
    • Fixed Branches:
      MOODLE_32_STABLE
    • Pull Master Branch:
      wip-MDL-27072-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
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (9 errors / 2 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (7/2) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              jb23347 John Beedell added a comment -

              Notes on the files changed in MDL-27072. Most changes are just switching to using sql snippets generated by the new functions in MDL-31243.

              lib/accesslib.php
              Adjust get_with_capability_sql and get_with_capability_join to accept an array of capability names.

              mod/quiz/report/
              attemptsreport.php now loads sql snippets rather than arrays of userids, also uses new get_allowed_users_joins.
              attemptsreport_table.php uses sql snippets as joins rather than sql IN (userids)
              reportlib.php

              mod/quiz/report/grading/
              report.php switch out get_users_by_capability replace with get_enrolled_sql
              mod/quiz/report/grading/tests/behat/grading.feature - new (basic)

              mod/quiz/report/overview/
              overview_table.php
              report.php
              mod/quiz/report/overview/tests/behat/basic.feature - new (basic)
              mod/quiz/report/overview/tests/report_test.php adjusted to use get_enrolled_sql rather than an array of student ids

              mod/quiz/report/responses/
              last_responses_table.php
              report.php

              mod/quiz/report/statistics/
              mod/quiz/report/statistics/classes/calculator.php
              report.php switch out get_users_by_capability replace with get_enrolled_sql
              statisticslib.php removed function quiz_statistics_renumber_placeholders (no longer required)
              mod/quiz/report/statistics/tests/statisticslib_test.php removed as was only a test for function above
              mod/quiz/report/statistics/tests/stats_from_steps_walkthrough_test.php

              Show
              jb23347 John Beedell added a comment - Notes on the files changed in MDL-27072 . Most changes are just switching to using sql snippets generated by the new functions in MDL-31243 . lib/accesslib.php Adjust get_with_capability_sql and get_with_capability_join to accept an array of capability names. mod/quiz/report/ attemptsreport.php now loads sql snippets rather than arrays of userids, also uses new get_allowed_users_joins. attemptsreport_table.php uses sql snippets as joins rather than sql IN (userids) reportlib.php mod/quiz/report/grading/ report.php switch out get_users_by_capability replace with get_enrolled_sql mod/quiz/report/grading/tests/behat/grading.feature - new (basic) mod/quiz/report/overview/ overview_table.php report.php mod/quiz/report/overview/tests/behat/basic.feature - new (basic) mod/quiz/report/overview/tests/report_test.php adjusted to use get_enrolled_sql rather than an array of student ids mod/quiz/report/responses/ last_responses_table.php report.php mod/quiz/report/statistics/ mod/quiz/report/statistics/classes/calculator.php report.php switch out get_users_by_capability replace with get_enrolled_sql statisticslib.php removed function quiz_statistics_renumber_placeholders (no longer required) mod/quiz/report/statistics/tests/statisticslib_test.php removed as was only a test for function above mod/quiz/report/statistics/tests/stats_from_steps_walkthrough_test.php
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (2 errors / 0 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (1/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              timhunt Tim Hunt added a comment -

              Thanks for doing this John. The fact that this works, as proved by the phpunit tests and the new behat tests () is really impressive. However, I think there are some things that need to be improved before this is ready to go into core:

              A) https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9b2de23daa1efd203cac9cc1fd5f972dR7304 - the PHPDoc comment needs to be updated, as CiBoT points out.

              B) load_relevant_students no longer does what it says on the tin. It would be good to rename it to get_students_sql, or something like that.

              C) Making $reportstudentssql an array with elements called 0, 1, 2, ... is not the most self-explanatory thing ever. I say this after working to understand the changes in base_sql. Would it not be worth making a new class to hold $joins, $wheres, $params? (I guess this comment really belongs MDL-31243, not this issue.) (Suggested name core_extra_query_sql?)

              D) if (!empty($reportstudentssql)) { - when will this every be false? https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9775c0c9fb495fdec1aa2c0d5e9c89f8R449 and similar places

              E) https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9775c0c9fb495fdec1aa2c0d5e9c89f8R453 - break inside if looks really dangerous.

              F) https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9775c0c9fb495fdec1aa2c0d5e9c89f8R453 - why do we have to query the database here, instead of passing $groupstudents to process_actions? Or, is there some reason we can be confident this list of users won't be too long? (This comment applies in some other reports too.)

              G) The attemptsreport classes are meant to be reusable by any quiz reports that might want ot use them, not just the ones in Moodle core. Therefore, any changes to the API there needs to be documented in mod/quiz/report/upgrade.txt, so that people can be warned about the changes in the Moodle 3.2 release notes.

              I hope it does not take too long to address these points. I am happy to discuss any of them. Thanks.

              Show
              timhunt Tim Hunt added a comment - Thanks for doing this John. The fact that this works, as proved by the phpunit tests and the new behat tests ( ) is really impressive. However, I think there are some things that need to be improved before this is ready to go into core: A) https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9b2de23daa1efd203cac9cc1fd5f972dR7304 - the PHPDoc comment needs to be updated, as CiBoT points out. B) load_relevant_students no longer does what it says on the tin. It would be good to rename it to get_students_sql, or something like that. C) Making $reportstudentssql an array with elements called 0, 1, 2, ... is not the most self-explanatory thing ever. I say this after working to understand the changes in base_sql. Would it not be worth making a new class to hold $joins, $wheres, $params? (I guess this comment really belongs MDL-31243 , not this issue.) (Suggested name core_extra_query_sql?) D) if (!empty($reportstudentssql)) { - when will this every be false? https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9775c0c9fb495fdec1aa2c0d5e9c89f8R449 and similar places E) https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9775c0c9fb495fdec1aa2c0d5e9c89f8R453 - break inside if looks really dangerous. F) https://github.com/Beedell/moodle/compare/wip-MDL-31243-master...Beedell:wip-MDL-27072-MDL-31243-master#diff-9775c0c9fb495fdec1aa2c0d5e9c89f8R453 - why do we have to query the database here, instead of passing $groupstudents to process_actions? Or, is there some reason we can be confident this list of users won't be too long? (This comment applies in some other reports too.) G) The attemptsreport classes are meant to be reusable by any quiz reports that might want ot use them, not just the ones in Moodle core. Therefore, any changes to the API there needs to be documented in mod/quiz/report/upgrade.txt, so that people can be warned about the changes in the Moodle 3.2 release notes. I hope it does not take too long to address these points. I am happy to discuss any of them. Thanks.
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (3 errors / 0 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (2 errors / 0 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              timhunt Tim Hunt added a comment -

              This now looks very good.

              There is one point I would still debate:

              C) Making $reportstudentssql an array with elements called 0, 1, 2, ... is not the most self-explanatory thing ever. I say this after working to understand the changes in base_sql. Would it not be worth making a new class to hold $joins, $wheres, $params? (I guess this comment really belongs MDL-31243, not this issue.) (Suggested name core_extra_query_sql?)

              • Perhaps, at the very least, we need some named contsants SQL_JOIN = 0, SQL_WHERE = 1 and SQL_PARAM = 2, to make it more self-explanator where we access the various bits.
              • Or, alternatively, return arrays with string array keys.
              • Or, just always unpack into meaningful variable names like

              list($groupjoins, $groupwhere, $groupparams) = $groupstudentsjoins;
              

              Part of me wants to say: well getting this far is bloody amazing, and we can always improve the code later, so let's move on. However, if we do want to change, e.g. to using string array-keys rather than meaninless ints, then that is an API change, so it is better to get it right first time.


              One other think that is easy to fix before this is sent to integration:

              • The commits put up for review at MDL-31243 are not the identical commits in this branch. The commit for this fix needs to be rebased onto the exact commit submitted for integration in MDL-31243. (Let me know if you need a hand with that.)

              And, I only notice one code-style thing (great work!)

              Show
              timhunt Tim Hunt added a comment - This now looks very good. There is one point I would still debate: C) Making $reportstudentssql an array with elements called 0, 1, 2, ... is not the most self-explanatory thing ever. I say this after working to understand the changes in base_sql. Would it not be worth making a new class to hold $joins, $wheres, $params? (I guess this comment really belongs MDL-31243 , not this issue.) (Suggested name core_extra_query_sql?) Perhaps, at the very least, we need some named contsants SQL_JOIN = 0, SQL_WHERE = 1 and SQL_PARAM = 2, to make it more self-explanator where we access the various bits. Or, alternatively, return arrays with string array keys. Or, just always unpack into meaningful variable names like list( $groupjoins , $groupwhere , $groupparams ) = $groupstudentsjoins ; Part of me wants to say: well getting this far is bloody amazing, and we can always improve the code later, so let's move on. However, if we do want to change, e.g. to using string array-keys rather than meaninless ints, then that is an API change, so it is better to get it right first time. One other think that is easy to fix before this is sent to integration: The commits put up for review at MDL-31243 are not the identical commits in this branch. The commit for this fix needs to be rebased onto the exact commit submitted for integration in MDL-31243 . (Let me know if you need a hand with that.) And, I only notice one code-style thing (great work!) https://github.com/Beedell/moodle/commit/afbefd1c9f3f28d81bdc5e039e62b6aac15b753d#diff-5a9b85369c738e6206a87cf49aef82edR94 - should not add an extra blank line here.
              Hide
              timhunt Tim Hunt added a comment -

              Further discussion about API in MDL-31243 https://tracker.moodle.org/browse/MDL-31243?focusedCommentId=427557&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-427557 lead to a conclusion:

              The methods do return new core\dml\sql_join($joins, $where, $params).

              The calling code then accesses the results like $enrolsql->joins, $enrolsql->where and $enrolsql->params.

              Show
              timhunt Tim Hunt added a comment - Further discussion about API in MDL-31243 https://tracker.moodle.org/browse/MDL-31243?focusedCommentId=427557&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-427557 lead to a conclusion: The methods do return new core\dml\sql_join($joins, $where, $params). The calling code then accesses the results like $enrolsql->joins, $enrolsql->where and $enrolsql->params.
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (2 errors / 1 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (0/1) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (2/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              timhunt Tim Hunt added a comment -

              This looks really close, but I think you have missed a bit: https://github.com/Beedell/moodle/commit/03067edbf9e8c787d2130072ef8e4e01c6c6318a#diff-4a09585e4c6ed380ce464954c44dbdb3R315. (Clearly a bit that was always lacking automated tests.)

              Apart from that, I think this is ready to go for integration.

              Show
              timhunt Tim Hunt added a comment - This looks really close, but I think you have missed a bit: https://github.com/Beedell/moodle/commit/03067edbf9e8c787d2130072ef8e4e01c6c6318a#diff-4a09585e4c6ed380ce464954c44dbdb3R315 . (Clearly a bit that was always lacking automated tests.) Apart from that, I think this is ready to go for integration.
              Hide
              jb23347 John Beedell added a comment -

              Sorry Tim, too much rushing yesterday.
              I have corrected the code, made a correction for the php code styling (statisticslib.php), and extended the behat test to include a basic check for re-grading and deletion.

              Show
              jb23347 John Beedell added a comment - Sorry Tim, too much rushing yesterday. I have corrected the code, made a correction for the php code styling (statisticslib.php), and extended the behat test to include a basic check for re-grading and deletion.
              Hide
              timhunt Tim Hunt added a comment -

              Thanks John. Looks good now. Submitting for integration.

              Would be good to rebase at the end of this week, once MDL-31243 has been integrated.

              Show
              timhunt Tim Hunt added a comment - Thanks John. Looks good now. Submitting for integration. Would be good to rebase at the end of this week, once MDL-31243 has been integrated.
              Hide
              poltawski Dan Poltawski 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
              poltawski Dan Poltawski 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
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (4 errors / 0 warnings) [branch: wip-MDL-27072-MDL-31243-master | CI Job ] phplint (0/0) , phpcs (0/0) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (3/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , Should these errors be fixed?
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi,

              Changes look perfect, just wanted to comment here some details, for clarification and fixing:

              1) With the change of get_with_capability_join() we are adding a new "any" (logical OR) behavior to the function that is not documented anywhere. IMO it's a must to clearly document (phpdoc + upgrade.txt) that change in param and behavior.

              2) Same applies to functions calling it. In fact, within the patch are uses that have switched from string to array without any advise. Both get_with_capability_sql() and get_enrolled_with_capabilities_join() now support arrays (in fact the patch is using them). So it must be documented there too (both the param and the logical OR behavior). Side comment: Hope we won't end needing "all" (logical AND) alternatives for those functions, time will tell.

              3) Unless I'm getting it wrong, the changes in the issue are, effectively, changing public api. Both quiz_attempts_report_table, some global functions like quiz_report_grade_bands() and also some "internal" (protected), but potentially callable by extended subclasses, now require different parameters. How will that affect 3rd part quiz reports out there? If there are implications, they also need to be documented. NP if not.

              4) This comment in the upgrade.txt notes worries me a little bit:

              +For sites upgrading please note that the quiz statistics report will fail
              +on fist viewing unless you carry out a regrading. This is because the hash
              +changes with this code, and the regrading process creates a fresh hash.
              

              Which is the exact impact people will face? Which are the results, in practice, of such a "fail on first viewing" ? I've tried here with 2-3 quizzes and have not noticed anything with/without regrading, but maybe that's because I'm not fulfilling some requirement. My point is that, if we know in advance that something is going to fail, instead of allowing that we should, somehow "reset" or "flag" or "upgrade" or whatever is needed to avoid any of those "failures" to happen (again, without knowing what the real meaning/impact of those "fails on first viewing"). Please clarify.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi, Changes look perfect, just wanted to comment here some details, for clarification and fixing: 1) With the change of get_with_capability_join() we are adding a new "any" (logical OR) behavior to the function that is not documented anywhere. IMO it's a must to clearly document (phpdoc + upgrade.txt) that change in param and behavior. 2) Same applies to functions calling it. In fact, within the patch are uses that have switched from string to array without any advise. Both get_with_capability_sql() and get_enrolled_with_capabilities_join() now support arrays (in fact the patch is using them). So it must be documented there too (both the param and the logical OR behavior). Side comment: Hope we won't end needing "all" (logical AND) alternatives for those functions, time will tell. 3) Unless I'm getting it wrong, the changes in the issue are, effectively, changing public api. Both quiz_attempts_report_table , some global functions like quiz_report_grade_bands() and also some "internal" (protected), but potentially callable by extended subclasses, now require different parameters. How will that affect 3rd part quiz reports out there? If there are implications, they also need to be documented. NP if not. 4) This comment in the upgrade.txt notes worries me a little bit: +For sites upgrading please note that the quiz statistics report will fail +on fist viewing unless you carry out a regrading. This is because the hash +changes with this code, and the regrading process creates a fresh hash. Which is the exact impact people will face? Which are the results, in practice, of such a "fail on first viewing" ? I've tried here with 2-3 quizzes and have not noticed anything with/without regrading, but maybe that's because I'm not fulfilling some requirement. My point is that, if we know in advance that something is going to fail, instead of allowing that we should, somehow "reset" or "flag" or "upgrade" or whatever is needed to avoid any of those "failures" to happen (again, without knowing what the real meaning/impact of those "fails on first viewing"). Please clarify. Ciao
              Hide
              jb23347 John Beedell added a comment -

              Many thanks for these comments Eloy. I agree and will make adjustments as soon as I can today.

              Show
              jb23347 John Beedell added a comment - Many thanks for these comments Eloy. I agree and will make adjustments as soon as I can today.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Hi John,

              I'm reopening this because it's already Thursday and we should start aiming to roll current weeklies. Just, whenever the patch is adjusted, send it again to integration (or ask Tim) and, hopefully, it will be integrated next week without major problems.

              Thanks for the hard work, ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Hi John, I'm reopening this because it's already Thursday and we should start aiming to roll current weeklies. Just, whenever the patch is adjusted, send it again to integration (or ask Tim) and, hopefully, it will be integrated next week without major problems. Thanks for the hard work, ciao
              Hide
              cibot CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              jb23347 John Beedell added a comment -

              Firstly, I am grateful for the code review above Eloy. It made me re-check all my changes and to my horror I found one place I had not made an appropriate change. Sorry - but at least the code change is better now.
              Some comments on the comments above:
              1) I have made a change in get_with_capability_join(), but that function was only introduced in MDL-31243 (integrated last week). I have improved the phpdocs for this function, but I am not convinced that it should appear in upgrade.txt as it has no impact on any previous functionality.
              2) Phpdocs are updated now.
              3) Yes there has been a change in the public API. I have checked with Tim and he has agreed that as we maintain the majority of quiz reports, and as this change is essential for the quiz reports to work for us at the OU at all, then these changes should proceed. I have adjusted the quiz/report/upgrade.txt to explain that these changed came in with MDL-31243/MDL-27072, and pointed to some examples in the code that would help others make suitable changes in their own custom reports if required.
              4) The worrying comment is removed now as I have added an upgrade script to avoid this problem.
              I have added the latest changes to https://github.com/Beedell/moodle/tree/wip-MDL-27072-MDL-31243-master as a separate commit, and to https://github.com/moodle/moodle/compare/master...Beedell:wip-MDL-27072-master as a single commit.
              Many thanks again.

              Show
              jb23347 John Beedell added a comment - Firstly, I am grateful for the code review above Eloy. It made me re-check all my changes and to my horror I found one place I had not made an appropriate change. Sorry - but at least the code change is better now. Some comments on the comments above: 1) I have made a change in get_with_capability_join(), but that function was only introduced in MDL-31243 (integrated last week). I have improved the phpdocs for this function, but I am not convinced that it should appear in upgrade.txt as it has no impact on any previous functionality. 2) Phpdocs are updated now. 3) Yes there has been a change in the public API. I have checked with Tim and he has agreed that as we maintain the majority of quiz reports, and as this change is essential for the quiz reports to work for us at the OU at all, then these changes should proceed. I have adjusted the quiz/report/upgrade.txt to explain that these changed came in with MDL-31243 / MDL-27072 , and pointed to some examples in the code that would help others make suitable changes in their own custom reports if required. 4) The worrying comment is removed now as I have added an upgrade script to avoid this problem. I have added the latest changes to https://github.com/Beedell/moodle/tree/wip-MDL-27072-MDL-31243-master as a separate commit, and to https://github.com/moodle/moodle/compare/master...Beedell:wip-MDL-27072-master as a single commit. Many thanks again.
              Hide
              timhunt Tim Hunt added a comment -

              Sorry, John. There is a PHPUnit failure

              Hopefully not too hard to fix next week.

              Show
              timhunt Tim Hunt added a comment - Sorry, John. There is a PHPUnit failure Hopefully not too hard to fix next week.
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master [branch: wip-MDL-31243-master | CI Job ] Error: Unable to fetch information from wip- MDL-31243 -master branch at https://github.com/Beedell/moodle.git . Should these errors be fixed?
              Hide
              cibot CiBoT added a comment -

              Code verified against automated checks.

              Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-27072 using repository: https://github.com/Beedell/moodle.git master (0 errors / 0 warnings) [branch: wip-MDL-27072-master | CI Job ] More information about this report
              Hide
              timhunt Tim Hunt added a comment -

              Thanks John. Re-submitting for integration.

              Show
              timhunt Tim Hunt added a comment - Thanks John. Re-submitting for integration.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Thanks,

              think the changes look ok now, with stats cleaned in order to force recalculation, and both api changes and phpdocs in place. FYI, I'm going to amend your commit to get rid of this comment:

              +    // Moodle v3.2.0 release upgrade line.
              

              Because it's something we add automatically once a version has been released, to separate upgrade changes happening AFTER then. As far as 3.2.0 has not been released yet, it's incorrect to put it there in advance.

              Other than that, perfect.

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Thanks, think the changes look ok now, with stats cleaned in order to force recalculation, and both api changes and phpdocs in place. FYI, I'm going to amend your commit to get rid of this comment: + // Moodle v3.2.0 release upgrade line. Because it's something we add automatically once a version has been released, to separate upgrade changes happening AFTER then. As far as 3.2.0 has not been released yet, it's incorrect to put it there in advance. Other than that, perfect.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              One more little comment, you don't need to continue feeding MDL-31243-master with changes at all, as far as it was integrated last week and now closed. It's ok for seeing diffs clearly, but it's also ok to add it separately here to MDL-27072-master (no need to, current 1 commit is ok, just sharing)

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - One more little comment, you don't need to continue feeding MDL-31243 -master with changes at all, as far as it was integrated last week and now closed. It's ok for seeing diffs clearly, but it's also ok to add it separately here to MDL-27072 -master (no need to, current 1 commit is ok, just sharing) Ciao
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (master only), thanks!

              Let's see how testing goes...

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks! Let's see how testing goes...
              Hide
              damyon Damyon Wiese added a comment -

              I tested as many options as I could find and they seem to be working correctly and the performance is good even with tens of thousands of users (note I did not have tens of thousands of attempts).

              Passing.

              Show
              damyon Damyon Wiese added a comment - I tested as many options as I could find and they seem to be working correctly and the performance is good even with tens of thousands of users (note I did not have tens of thousands of attempts). Passing.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for working on this John,

              Unfortunately this is causing behat failure

              [Behat\Gherkin\Exception\ParserException]                                                                                                                                                                          
                Expected Step, but got text: "      | student1 | S1        | Student1 | student1@example.com | S1000    !" in file: /store/moodle/behat_whole_suite_m_parallel/mod/quiz/report/overview/tests/behat/basic.feature  
              [Behat\Gherkin\Exception\ParserException]                                                                                                                                                                           
                Expected Step, but got text: "      | student1 | S1        | Student1 | student1@example.com | S1000    !" in file: /store/moodle/behat_whole_suite_m_parallel/mod/quiz/report/grading/tests/behat/grading.feature  
              

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for working on this John, Unfortunately this is causing behat failure [Behat\Gherkin\Exception\ParserException] Expected Step, but got text: " | student1 | S1 | Student1 | student1@example.com | S1000 !" in file: /store/moodle/behat_whole_suite_m_parallel/mod/quiz/report/overview/tests/behat/basic.feature [Behat\Gherkin\Exception\ParserException] Expected Step, but got text: " | student1 | S1 | Student1 | student1@example.com | S1000 !" in file: /store/moodle/behat_whole_suite_m_parallel/mod/quiz/report/grading/tests/behat/grading.feature
              Show
              rajeshtaneja Rajesh Taneja added a comment - Patch fixing the problem: https://github.com/moodle/moodle/compare/2660c96...rajeshtaneja:wip-mdl-27072 git pull https://github.com/rajeshtaneja/moodle.git wip-mdl-27072
              Hide
              dmonllao David Monllaó added a comment -

              Thanks for the patch Rajesh, was this test ever passing with those exclamation marks? I thought only "|" would work as a separator. By the way, shouldn't we remove @mod_quiz_grading_basic tag?

              Back to testing.

              Show
              dmonllao David Monllaó added a comment - Thanks for the patch Rajesh, was this test ever passing with those exclamation marks? I thought only "|" would work as a separator. By the way, shouldn't we remove @mod_quiz_grading_basic tag? Back to testing.
              Hide
              rajeshtaneja Rajesh Taneja added a comment - - edited

              Not sure if that was passing previously, as in Behat 3.1 the table parser was not strict. AFA @mod_quiz_grading_basic tag, it should also be removed.

              https://github.com/moodle/moodle/compare/0115782...rajeshtaneja:wip-mdl-27072-fix
              git pull https://github.com/rajeshtaneja/moodle.git wip-mdl-27072-fix

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited Not sure if that was passing previously, as in Behat 3.1 the table parser was not strict. AFA @mod_quiz_grading_basic tag, it should also be removed. https://github.com/moodle/moodle/compare/0115782...rajeshtaneja:wip-mdl-27072-fix git pull https://github.com/rajeshtaneja/moodle.git wip-mdl-27072-fix
              Hide
              dmonllao David Monllaó added a comment -

              Thanks Rajesh. Pushed.

              Show
              dmonllao David Monllaó added a comment - Thanks Rajesh. Pushed.
              Hide
              timhunt Tim Hunt added a comment -

              Thanks for trying to provide the fix-up patches, but:

              1. quiz reports are a sub-plugin of mod_quiz. Therefore, there should be tags @quiz @quiz_grading (etc.). (See https://github.com/moodle/moodle/blob/master/mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature, for example, for how it should be)
              2. I know that, in theory, one behat scenario should test only one thing, and hence one scenario should only have one each of Give/When and Then. However, often people break that rule. When that rule is broken, there are two options:
                • Focus on human-readability, and allow multiple When and Then, to make it clear which bits of the long scenario are doing stuff and which are verifying the outcome. This is clearly the more helpful option, but unfortunately what Moodle seems to prefer is
                • Pay lip-service to the 'only one each of Give/When and Then'. I don't see what you gain by trying to pretend that the test is only testing one thing - other that confusion to anyone trying to read the test later.

              Even if you are going to do the stupid thing, of replacing useful cue words all with And, please do it in the best way. Clearly everything is set-up (Given) up to https://github.com/rajeshtaneja/moodle/blob/c76e57aace31977068ad64520ca1d602591a94e5/mod/quiz/report/grading/tests/behat/grading.feature#L62. So, 'When' should come on https://github.com/rajeshtaneja/moodle/blob/c76e57aace31977068ad64520ca1d602591a94e5/mod/quiz/report/grading/tests/behat/grading.feature#L63 and the key 'Then' is probably https://github.com/rajeshtaneja/moodle/blob/c76e57aace31977068ad64520ca1d602591a94e5/mod/quiz/report/grading/tests/behat/grading.feature#L75

              But, I still think it would be better to leave it as John had it, since that is much easier to read.

              Show
              timhunt Tim Hunt added a comment - Thanks for trying to provide the fix-up patches, but: quiz reports are a sub-plugin of mod_quiz. Therefore, there should be tags @quiz @quiz_grading (etc.). (See https://github.com/moodle/moodle/blob/master/mod/assign/feedback/editpdf/tests/behat/annotate_pdf.feature , for example, for how it should be) I know that, in theory, one behat scenario should test only one thing, and hence one scenario should only have one each of Give/When and Then. However, often people break that rule. When that rule is broken, there are two options: Focus on human-readability, and allow multiple When and Then, to make it clear which bits of the long scenario are doing stuff and which are verifying the outcome. This is clearly the more helpful option, but unfortunately what Moodle seems to prefer is Pay lip-service to the 'only one each of Give/When and Then'. I don't see what you gain by trying to pretend that the test is only testing one thing - other that confusion to anyone trying to read the test later. Even if you are going to do the stupid thing, of replacing useful cue words all with And, please do it in the best way. Clearly everything is set-up (Given) up to https://github.com/rajeshtaneja/moodle/blob/c76e57aace31977068ad64520ca1d602591a94e5/mod/quiz/report/grading/tests/behat/grading.feature#L62 . So, 'When' should come on https://github.com/rajeshtaneja/moodle/blob/c76e57aace31977068ad64520ca1d602591a94e5/mod/quiz/report/grading/tests/behat/grading.feature#L63 and the key 'Then' is probably https://github.com/rajeshtaneja/moodle/blob/c76e57aace31977068ad64520ca1d602591a94e5/mod/quiz/report/grading/tests/behat/grading.feature#L75 But, I still think it would be better to leave it as John had it, since that is much easier to read.
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks Tim,

              1. The tag (@mod_quiz_grading_basic ) removed doesn't comply with any of mentioned standards. Which tags should be added are still subjective, but every feature should have atleast frankenstyle tags. Feel free to provide a patch with more tags which have some reference to code area.
              2. We have discussed this quite a number of times in past and finally it was decided to adhere to single Given/When/Then. Feel free to create an issue to discuss this once more. To make things readable use of comments are encouraged and they were left as they are.
              3. As per modifications to when and then, I left the first when/then in code as it is and replaced rest of them to use AND. https://github.com/moodle/moodle/compare/2660c96...rajeshtaneja:wip-mdl-27072#diff-cf44bc7d35b979ce55d60491beb67268L36 .
              4. The main reason for putting up the patch was none of the above but following:
              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Tim, The tag (@mod_quiz_grading_basic ) removed doesn't comply with any of mentioned standards. Which tags should be added are still subjective, but every feature should have atleast frankenstyle tags. Feel free to provide a patch with more tags which have some reference to code area. We have discussed this quite a number of times in past and finally it was decided to adhere to single Given/When/Then. Feel free to create an issue to discuss this once more. To make things readable use of comments are encouraged and they were left as they are. As per modifications to when and then, I left the first when/then in code as it is and replaced rest of them to use AND. https://github.com/moodle/moodle/compare/2660c96...rajeshtaneja:wip-mdl-27072#diff-cf44bc7d35b979ce55d60491beb67268L36 . The main reason for putting up the patch was none of the above but following: Use of exclamation as table separator ( https://github.com/moodle/moodle/compare/2660c96...rajeshtaneja:wip-mdl-27072#diff-cf44bc7d35b979ce55d60491beb67268L12 ) Use of selectors which rely on id and can fail. ( https://github.com/moodle/moodle/compare/2660c96...rajeshtaneja:wip-mdl-27072#diff-8b1751e39658f3a76f5fb6535bd0c1feL69 )
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Behat is now passing,

              Passing this on behalf of Damyon as he has already done manual testing...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Behat is now passing, Passing this on behalf of Damyon as he has already done manual testing...
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Changes have been rolled upstream and are available in main moodle.git repo and official clones. Many thanks for your hard work and awesome solutions. Closing this as fixed!

              I have some ideas on how to fix that.
              They're not very good ideas, but
              at least they're ideas!
              — Adam Savage

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Changes have been rolled upstream and are available in main moodle.git repo and official clones. Many thanks for your hard work and awesome solutions. Closing this as fixed! I have some ideas on how to fix that. They're not very good ideas, but at least they're ideas! — Adam Savage
              Hide
              marina Marina Glancy added a comment -

              Hello, this issue was marked with "release_notes" label. Can you please explain what exactly should be mentioned in release notes for moodle users? From the current summary it looks like a code change without performance or UI changes

              Show
              marina Marina Glancy added a comment - Hello, this issue was marked with "release_notes" label. Can you please explain what exactly should be mentioned in release notes for moodle users? From the current summary it looks like a code change without performance or UI changes
              Hide
              timhunt Tim Hunt added a comment -

              It needs to be mentioned in the 'for developers' section.

              Show
              timhunt Tim Hunt added a comment - It needs to be mentioned in the 'for developers' section.
              Hide
              damyon Damyon Wiese added a comment -

              It was a performance improvement.

              Show
              damyon Damyon Wiese added a comment - It was a performance improvement.
              Hide
              marina Marina Glancy added a comment - - edited

              Tim, what exactly should be mentioned to developers? (api_change is the label for "for developers" seciton of release notes).
              This issue already has note in upgrade.txt for the plugin developers - https://github.com/moodle/moodle/commit/10c4fce520cd81bd926063f35c0275d877ee52d9#diff-e8b4c59bfc8a7f1a4c3baa8956f5299c

              Show
              marina Marina Glancy added a comment - - edited Tim, what exactly should be mentioned to developers? (api_change is the label for "for developers" seciton of release notes). This issue already has note in upgrade.txt for the plugin developers - https://github.com/moodle/moodle/commit/10c4fce520cd81bd926063f35c0275d877ee52d9#diff-e8b4c59bfc8a7f1a4c3baa8956f5299c
              Hide
              timhunt Tim Hunt added a comment -

              Well, https://docs.moodle.org/dev/Quiz_reports might have needed an update. I just checked. I did not. I'll remove the label now. I believe that is the correct process, right?

              Show
              timhunt Tim Hunt added a comment - Well, https://docs.moodle.org/dev/Quiz_reports might have needed an update. I just checked. I did not. I'll remove the label now. I believe that is the correct process, right?
              Hide
              marina Marina Glancy added a comment -

              Thanks Tim

              Show
              marina Marina Glancy added a comment - Thanks Tim
              Hide
              poltawski Dan Poltawski added a comment -

              This issue caused a regression: MDL-57234

              Show
              poltawski Dan Poltawski added a comment - This issue caused a regression: MDL-57234

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    5/Dec/16