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:
    • Rank:
      39320

      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.

        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: