Moodle
  1. Moodle
  2. MDL-42957

Harcoded LIMIT clause in recent statistics report

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.6.1
    • Component/s: Quiz
    • Labels:

      Description

      Looking for another failure it was detected that the statistics quiz report is using some harcoded "LIMIT" clauses (questions/calculator.php, esponses/analyser.php at least). That leads to crash under mssql/sqlsrv/oracle.

      Please, confirm, fix and add some way to have it covered.

      Ciao

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Tim Hunt added a comment -

            Assigning to Jamie so he can fix this regression.

            Show
            Tim Hunt added a comment - Assigning to Jamie so he can fix this regression.
            Hide
            Jamie Pratt added a comment -

            Thanks, will get on to this today.

            Show
            Jamie Pratt added a comment - Thanks, will get on to this today.
            Hide
            Jamie Pratt added a comment - - edited

            I removed the LIMIT as under normal circumstances there is no way the query should hit more than one record. The LIMIT helped during development when some extra cache records had gotten through.

            Show
            Jamie Pratt added a comment - - edited I removed the LIMIT as under normal circumstances there is no way the query should hit more than one record. The LIMIT helped during development when some extra cache records had gotten through.
            Hide
            Tim Hunt added a comment -

            Jamies master branch should not be used. It was done as a merge, not a cherry-pick.

            With this patch, if we do ever get multiple caches records, as was the case during development, there will just be a debugging('Error: mdb->get_record() found more than one record!'); message output, which is a perfectly good way to handle this.

            So, +1 from me. Submitting for integration.

            Show
            Tim Hunt added a comment - Jamies master branch should not be used. It was done as a merge, not a cherry-pick. With this patch, if we do ever get multiple caches records, as was the case during development, there will just be a debugging('Error: mdb->get_record() found more than one record!'); message output, which is a perfectly good way to handle this. So, +1 from me. Submitting for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sorry guys:

            grep -r '\bLIMIT\b' question
             
            question/classes/statistics/questions/calculator.php
            question/classes/statistics/responses/analyser.php
            

            Show
            Eloy Lafuente (stronk7) added a comment - Sorry guys: grep -r '\bLIMIT\b' question   question/classes/statistics/questions/calculator.php question/classes/statistics/responses/analyser.php
            Hide
            Jamie Pratt added a comment -

            Thanks for catching that extra LIMIT Eloy. For the response analysis cache we expect more than one record might be returned since there is a record for each answer that one or more student response(s) have been matched to, so here I have changed the strictness to expect multiple.

            Show
            Jamie Pratt added a comment - Thanks for catching that extra LIMIT Eloy. For the response analysis cache we expect more than one record might be returned since there is a record for each answer that one or more student response(s) have been matched to, so here I have changed the strictness to expect multiple.
            Hide
            Tim Hunt added a comment -

            OK, re-submitting for integration.

            Show
            Tim Hunt added a comment - OK, re-submitting for integration.
            Hide
            Dan Poltawski added a comment -

            Integrated to master and 26, thanks.

            As Eloy implied, it would've been good if this could be covered by some sort of automated tests

            Show
            Dan Poltawski added a comment - Integrated to master and 26, thanks. As Eloy implied, it would've been good if this could be covered by some sort of automated tests
            Hide
            Frédéric Massart added a comment -

            Failing this test for now.

            1/ Log outputs this and the graph is not displayed.

            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Warning:  Division by zero in /home/fred/www/repositories/im/moodle/lib/graphlib.php on line 1021, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   1. {main}() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:0, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   2. graph->draw() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:158, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   3. graph->init() /home/fred/www/repositories/im/moodle/lib/graphlib.php:275, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   4. graph->init_y_axis() /home/fred/www/repositories/im/moodle/lib/graphlib.php:199, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Warning:  Division by zero in /home/fred/www/repositories/im/moodle/lib/graphlib.php on line 1023, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   1. {main}() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:0, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   2. graph->draw() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:158, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   3. graph->init() /home/fred/www/repositories/im/moodle/lib/graphlib.php:275, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   4. graph->init_y_axis() /home/fred/www/repositories/im/moodle/lib/graphlib.php:199, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Warning:  Division by zero in /home/fred/www/repositories/im/moodle/lib/graphlib.php on line 707, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   1. {main}() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:0, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   2. graph->draw() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:158, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   3. graph->init() /home/fred/www/repositories/im/moodle/lib/graphlib.php:275, referer: http://fred.moodle.local/im/mod/quiz/report.php
            [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP   4. graph->init_data() /home/fred/www/repositories/im/moodle/lib/graphlib.php:226, referer: http://fred.moodle.local/im/mod/quiz/report.php
            

            • I have attempted the quiz with 2 students.

            2/ Looking at the diff, it seems that an argument has been dropped in the query of get_last_analysed_time(), which is not easily testable as there are no Unit Tests and no core code calling it.

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Failing this test for now. 1/ Log outputs this and the graph is not displayed. [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Warning: Division by zero in /home/fred/www/repositories/im/moodle/lib/graphlib.php on line 1021, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 1. {main}() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:0, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 2. graph->draw() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:158, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 3. graph->init() /home/fred/www/repositories/im/moodle/lib/graphlib.php:275, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 4. graph->init_y_axis() /home/fred/www/repositories/im/moodle/lib/graphlib.php:199, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Warning: Division by zero in /home/fred/www/repositories/im/moodle/lib/graphlib.php on line 1023, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 1. {main}() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:0, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 2. graph->draw() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:158, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 3. graph->init() /home/fred/www/repositories/im/moodle/lib/graphlib.php:275, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 4. graph->init_y_axis() /home/fred/www/repositories/im/moodle/lib/graphlib.php:199, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Warning: Division by zero in /home/fred/www/repositories/im/moodle/lib/graphlib.php on line 707, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 1. {main}() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:0, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 2. graph->draw() /home/fred/www/repositories/im/moodle/mod/quiz/report/statistics/statistics_graph.php:158, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 3. graph->init() /home/fred/www/repositories/im/moodle/lib/graphlib.php:275, referer: http://fred.moodle.local/im/mod/quiz/report.php [Wed Nov 27 10:44:55 2013] [error] [client 192.168.100.54] PHP 4. graph->init_data() /home/fred/www/repositories/im/moodle/lib/graphlib.php:226, referer: http://fred.moodle.local/im/mod/quiz/report.php I have attempted the quiz with 2 students. 2/ Looking at the diff, it seems that an argument has been dropped in the query of get_last_analysed_time() , which is not easily testable as there are no Unit Tests and no core code calling it. Cheers, Fred
            Hide
            Jamie Pratt added a comment -

            Hi Fred,

            It looks like you have found an unrelated bug in the statistics report graph, can you attach a backup of the quiz you have created with the student attempt data included, in a new tracker issue?

            In the mean time I will write some tests to cover this new code.

            Thanks!

            Show
            Jamie Pratt added a comment - Hi Fred, It looks like you have found an unrelated bug in the statistics report graph, can you attach a backup of the quiz you have created with the student attempt data included, in a new tracker issue? In the mean time I will write some tests to cover this new code. Thanks!
            Hide
            Jamie Pratt added a comment -

            Hi Fred,

            I have reproduced the bug you found. See MDL-43079

            Will file a fix for that and get back to this bug.

            Jamie

            Show
            Jamie Pratt added a comment - Hi Fred, I have reproduced the bug you found. See MDL-43079 Will file a fix for that and get back to this bug. Jamie
            Hide
            Dan Poltawski added a comment -

            Ping about the fixes for this.

            Show
            Dan Poltawski added a comment - Ping about the fixes for this.
            Hide
            Tim Hunt added a comment -

            You had a response yesterday: MDL-43079 has a fix, that was peer reviewed, and is waiting for integration since about 20 hours ago.

            Show
            Tim Hunt added a comment - You had a response yesterday: MDL-43079 has a fix, that was peer reviewed, and is waiting for integration since about 20 hours ago.
            Hide
            Dan Poltawski added a comment -

            That is a fix to a separate issue i'm not worried about. But in this change we've changed the definition of the get_field_select, passing a select statement into a return value of get_field_select. Which i'm sure will cause a fatal a fatal error if any code ever uses it..

            Show
            Dan Poltawski added a comment - That is a fix to a separate issue i'm not worried about. But in this change we've changed the definition of the get_field_select, passing a select statement into a return value of get_field_select. Which i'm sure will cause a fatal a fatal error if any code ever uses it..
            Hide
            Frédéric Massart added a comment -

            I suspect you have fixed the issue Jamie. Though, if you have amended your commit then it will be a problem for integrators to pull your patch in, as you rewrote the history ignoring the previous commit. The best approach to fix an issue that is already integrated is not to rebase but to add a new commit on top of your branch. Could you please restore your branch to its initial state and add the extra commit?

            Thanks!
            Fred

            Show
            Frédéric Massart added a comment - I suspect you have fixed the issue Jamie. Though, if you have amended your commit then it will be a problem for integrators to pull your patch in, as you rewrote the history ignoring the previous commit. The best approach to fix an issue that is already integrated is not to rebase but to add a new commit on top of your branch. Could you please restore your branch to its initial state and add the extra commit? Thanks! Fred
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sorry Dan Poltawski, can you explain your last comment? I've looked to code here 3-4 times and cannot find anything strange there.

            My only point (and not very strong) is that I don't like this type of assertions:

            assertEquals(time(), .....)
            

            and we should be using, instead

            assertTimeCurrent(....)
            

            to avoid random time failures.

            Show
            Eloy Lafuente (stronk7) added a comment - Sorry Dan Poltawski , can you explain your last comment? I've looked to code here 3-4 times and cannot find anything strange there. My only point (and not very strong) is that I don't like this type of assertions: assertEquals(time(), .....) and we should be using, instead assertTimeCurrent(....) to avoid random time failures.
            Hide
            Jamie Pratt added a comment - - edited

            Eloy Lafuente (stronk7) Dan Poltawski Eloy I suspect you are not seeing the errors because you were looking at the branch I had force committed after I fixed the problems that Dan pointed out. I have them fixed now.

            Show
            Jamie Pratt added a comment - - edited Eloy Lafuente (stronk7) Dan Poltawski Eloy I suspect you are not seeing the errors because you were looking at the branch I had force committed after I fixed the problems that Dan pointed out. I have them fixed now.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            oh Jamie, so you forced-pushed changes, overwriting already integrated commits here, in this issue?

            I thought that Fred's comment above was referring to the other, linked issue. Verifying...

            Aha I see, yes, you forced it, be warned it's no-no for the future. Once an issue has been integrated, any modification should come in the form of new commits, never overwritten (amended) ones.

            In any case, the conflicts do not seem complex to solve, this is what I'm getting and what I'll pick:

            question/classes/statistics/responses/analyser.php

            <<<<<<< HEAD
                    return $DB->get_field_select('question_response_analysis', 'hashcode = ? AND questionid = ? AND timemodified > ?',
            =======
                    return $DB->get_field_select('question_response_analysis', 'timemodified',
                                                 'hashcode = ? AND questionid = ? AND timemodified > ?',
            >>>>>>> FETCH_HEAD
            

            (will pick your new code, FETCH_HEAD option, where you fix the get_field_select() call 2nd param)

            question/classes/statistics/questions/calculator.php

            <<<<<<< HEAD
                                                 array($qubaids->get_hash_code(), $timemodified));
            ======= 
                                                 array($qubaids->get_hash_code(), $timemodified), IGNORE_MULTIPLE);
            >>>>>>> FETCH_HEAD
            

            Again, will pick the second (newer) FETCH_HEAD option, that includes the IGNORE_MULTIPLE param.

            If my proposed picks are ok, I'm happy doing that. Can you, please, confirm them?

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - oh Jamie, so you forced-pushed changes, overwriting already integrated commits here, in this issue? I thought that Fred's comment above was referring to the other, linked issue. Verifying... Aha I see, yes, you forced it, be warned it's no-no for the future. Once an issue has been integrated, any modification should come in the form of new commits, never overwritten (amended) ones. In any case, the conflicts do not seem complex to solve, this is what I'm getting and what I'll pick: question/classes/statistics/responses/analyser.php <<<<<<< HEAD return $DB->get_field_select('question_response_analysis', 'hashcode = ? AND questionid = ? AND timemodified > ?', ======= return $DB->get_field_select('question_response_analysis', 'timemodified', 'hashcode = ? AND questionid = ? AND timemodified > ?', >>>>>>> FETCH_HEAD (will pick your new code, FETCH_HEAD option, where you fix the get_field_select() call 2nd param) question/classes/statistics/questions/calculator.php <<<<<<< HEAD array($qubaids->get_hash_code(), $timemodified)); ======= array($qubaids->get_hash_code(), $timemodified), IGNORE_MULTIPLE); >>>>>>> FETCH_HEAD Again, will pick the second (newer) FETCH_HEAD option, that includes the IGNORE_MULTIPLE param. If my proposed picks are ok, I'm happy doing that. Can you, please, confirm them? TIA and ciao
            Hide
            Jamie Pratt added a comment -

            Hi,

            I have now found the original commit and force pushed that up to github, plus the new changes for both the master and 26 branches. So hopefully you can just merge these into the integration.

            Jamie

            Show
            Jamie Pratt added a comment - Hi, I have now found the original commit and force pushed that up to github, plus the new changes for both the master and 26 branches. So hopefully you can just merge these into the integration. Jamie
            Hide
            Jamie Pratt added a comment -

            I have replaced assertEquals(time(), ..) with assertTimeCurrent(...). Thanks for pointing out that function Eloy. I had hoped that the assertEquals(time(), ..) would not fail as I had put in a one second delta but assertTimeCurrent is definitely a superior solution.

            Thanks.

            Show
            Jamie Pratt added a comment - I have replaced assertEquals(time(), ..) with assertTimeCurrent(...). Thanks for pointing out that function Eloy. I had hoped that the assertEquals(time(), ..) would not fail as I had put in a one second delta but assertTimeCurrent is definitely a superior solution. Thanks.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            hehe, Jamie... but you have cherry picked the original commit, hence changing it, so your new collection of commits continues conflicting exactly the same than the old ones.

            for example, for master the original was a9e2f21125843b7d7def8accb61ca80fc52e9cfc and your "new" original commit is 487e7e66022360ffee34f19478e131024510b51c.

            So I end getting the same 2 conflicts above. If the proposed resolution is ok, I'll be cherry picking your 2nd and 3rd new commits and done. Sounds ok?

            Show
            Eloy Lafuente (stronk7) added a comment - hehe, Jamie... but you have cherry picked the original commit, hence changing it, so your new collection of commits continues conflicting exactly the same than the old ones. for example, for master the original was a9e2f21125843b7d7def8accb61ca80fc52e9cfc and your "new" original commit is 487e7e66022360ffee34f19478e131024510b51c. So I end getting the same 2 conflicts above. If the proposed resolution is ok, I'll be cherry picking your 2nd and 3rd new commits and done. Sounds ok?
            Hide
            Jamie Pratt added a comment -

            Sorry Eloy, I thought I had managed to reset the branch to that commit. Sorry seem to have got my reflogs rather in a twist.

            Your proposed fix of those conflicts would be great. Thanks.

            Show
            Jamie Pratt added a comment - Sorry Eloy, I thought I had managed to reset the branch to that commit. Sorry seem to have got my reflogs rather in a twist. Your proposed fix of those conflicts would be great. Thanks.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            it seems you did it ok for 26_STABLE, but not master.... np... applying conflict fixing into master.

            Show
            Eloy Lafuente (stronk7) added a comment - it seems you did it ok for 26_STABLE, but not master.... np... applying conflict fixing into master.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Done, changes pushed to integration.git (26 and master), thanks!

            Sending this back to testing to get final approval from Fred... ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Done, changes pushed to integration.git (26 and master), thanks! Sending this back to testing to get final approval from Fred... ciao
            Hide
            Eloy Lafuente (stronk7) added a comment -

            For the records, CI servers passed the new added unit tests.

            Show
            Eloy Lafuente (stronk7) added a comment - For the records, CI servers passed the new added unit tests.
            Hide
            Dan Poltawski added a comment -

            (Thanks Eloy & Jamie)

            Show
            Dan Poltawski added a comment - (Thanks Eloy & Jamie)
            Hide
            Dan Poltawski added a comment -

            Unit tests and manual test passed on mssql.

            Show
            Dan Poltawski added a comment - Unit tests and manual test passed on mssql.
            Hide
            Frédéric Massart added a comment -

            Passing the test (tested master only):

            • Manual tests (PGSQL, MySQL, Oracle and MSSQL) ran by Dan and I.
            • Unit Tests (MSSQL, PGSQL, Oracle) ran by Dan and I.
            • Unit Tests MySQL ran by CI .

            Cheers,
            Fred

            Show
            Frédéric Massart added a comment - Passing the test (tested master only): Manual tests (PGSQL, MySQL, Oracle and MSSQL) ran by Dan and I. Unit Tests (MSSQL, PGSQL, Oracle) ran by Dan and I. Unit Tests MySQL ran by CI . Cheers, Fred
            Hide
            Dan Poltawski added a comment -

            Congratulations, this change has now made its way upstream. Thanks for your contribution!

            “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne

            Show
            Dan Poltawski added a comment - Congratulations, this change has now made its way upstream. Thanks for your contribution! “ Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live. ” - Rick Osborne

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: