Moodle
  1. Moodle
  2. MDL-32442

tool_qeupgradehelper_get_quiz_for_upgrade SQL incompatible with Postgres

    Details

    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      Ideally we should test this on all DBS.

      Create a Moodle 1.9 or 2.0 site with quiz attempts.

      Place 2.1, 2.2 or master code in place - but do not run upgrade yet!
      Create admin/tool/qeupgradehelper/partialupgrade.php (2.1 path is local/qeupgradehelper/partialupgrade.php), with the code:

      <?php
      function tool_qeupgradehelper_get_quizzes_to_upgrade() {
          return array();
      }

      This will cause the question attempt upgrade to not execute.

      Run the Moodle system upgrade.

      Once system upgraded, go to Question engine upgrade helper in Site Administration.
      Set cron to Yes and save.

      Execute cron.

      Before patch, on Postgres, would get a DB Execution error.

      Correct behavior (after patch), should get a series of 'upgrading quiz x' messages.

      Show
      Ideally we should test this on all DBS. Create a Moodle 1.9 or 2.0 site with quiz attempts. Place 2.1, 2.2 or master code in place - but do not run upgrade yet! Create admin/tool/qeupgradehelper/partialupgrade.php (2.1 path is local/qeupgradehelper/partialupgrade.php), with the code: <?php function tool_qeupgradehelper_get_quizzes_to_upgrade() { return array(); } This will cause the question attempt upgrade to not execute. Run the Moodle system upgrade. Once system upgraded, go to Question engine upgrade helper in Site Administration. Set cron to Yes and save. Execute cron. Before patch, on Postgres, would get a DB Execution error. Correct behavior (after patch), should get a series of 'upgrading quiz x' messages.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      in admin/tool/qeupgradehelper/locallib.php, function:
      tool_qeupgradehelper_get_quiz_for_upgrade

      Contains:
      SELECT quiz.id
      FROM

      {quiz_attempts}

      quiza
      JOIN

      {quiz}

      quiz ON quiz.id = quiza.quiz
      JOIN

      {course}

      c ON c.id = quiz.course
      WHERE quiza.preview = 0 AND quiza.needsupgradetonewqe = 1
      GROUP BY quiz.id, quiz.name, c.shortname, c.id
      ORDER BY quiza.timemodified DESC

      The ORDER BY clause is invalid because quiza.timemodified is not in GROUP BY or otherwise aggregated. I recommend changing it to
      ORDER BY MAX(quiza.timemodified) DESC

      but I have not verified this on other DBs yet.

        Gliffy Diagrams

          Activity

          Hide
          Eric Merrill added a comment -

          I added some pull branches, and assigned this to Tim - since IIRC, this is his baby.

          Show
          Eric Merrill added a comment - I added some pull branches, and assigned this to Tim - since IIRC, this is his baby.
          Hide
          Tim Hunt added a comment -

          That is very odd. I do all my development on Postgres.

          I wonder why quiza.timemodified is the right sort-order?

          Show
          Tim Hunt added a comment - That is very odd. I do all my development on Postgres. I wonder why quiza.timemodified is the right sort-order?
          Hide
          Eric Merrill added a comment -

          I assume the intent was to get the quiz with the most recent attempt on it - so upgrade the most recently used first.

          Show
          Eric Merrill added a comment - I assume the intent was to get the quiz with the most recent attempt on it - so upgrade the most recently used first.
          Hide
          Tim Hunt added a comment -

          Yes, so MAX is good logic. Submitting for integration.

          We really ought to back-port to 2.1.x, which is a pain because the code is in local there. However, I would be grateful if you could make a branch for that version too.

          Please could you also write some testing instructions between now and next Monday? Thanks.

          Show
          Tim Hunt added a comment - Yes, so MAX is good logic. Submitting for integration. We really ought to back-port to 2.1.x, which is a pain because the code is in local there. However, I would be grateful if you could make a branch for that version too. Please could you also write some testing instructions between now and next Monday? Thanks.
          Hide
          Eric Merrill added a comment -

          Yup, Ill get the testing instructions up.

          I just added a 2.1 branch. Luckily it was just a move of the tool, so the code was still in place and I could even cherry-pick.

          Show
          Eric Merrill added a comment - Yup, Ill get the testing instructions up. I just added a 2.1 branch. Luckily it was just a move of the tool, so the code was still in place and I could even cherry-pick.
          Hide
          Tim Hunt added a comment -

          Brilliant. Thanks Eric.

          Show
          Tim Hunt added a comment - Brilliant. Thanks Eric.
          Hide
          Eric Merrill added a comment -

          Note to testers - the function where this error occurs is also used by the MDL-32443 tool, so it may be easiest to test them together (use that tool instead of setting up and firing cron).

          Show
          Eric Merrill added a comment - Note to testers - the function where this error occurs is also used by the MDL-32443 tool, so it may be easiest to test them together (use that tool instead of setting up and firing cron).
          Hide
          Dan Poltawski added a comment -

          Thanks Eric!

          This has been integrated now

          Show
          Dan Poltawski added a comment - Thanks Eric! This has been integrated now
          Hide
          Adrian Greeve added a comment -

          Tested in postgres.
          Upgraded from 2.0 to 2.2
          Works as described.
          Thanks.

          Show
          Adrian Greeve added a comment - Tested in postgres. Upgraded from 2.0 to 2.2 Works as described. Thanks.
          Hide
          Dan Poltawski added a comment -

          Bonza mate!

          Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby.

          Hooroo

          Show
          Dan Poltawski added a comment - Bonza mate! Your changes have made it into the Moodle release! Its time to celebrate! Put a shrimp on the barbie and grab a stubby. Hooroo

            People

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

              Dates

              • Created:
                Updated:
                Resolved: