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
    • Pull Master Branch:
      wip-MDL-27072-MDL-31243-master

      Description

      This is spun off from MDL-27071.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              matt.clarkson Matt Clarkson added a comment -

              Attaching a patch that removes get_users_by_capability from quiz reporting.

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

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

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

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

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

              The preferred approach is MDL-31243.

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

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

              Just bumping workflow state.

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

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

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

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

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

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

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

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

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

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

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

              Please use one of

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

              as I said above.

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

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

              My argument for using a new capability is:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

                People

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

                  Dates

                  • Created:
                    Updated: