Moodle
  1. Moodle
  2. MDL-28032

DB differences found (defaultmark & maxmark) between installed & upgraded 2.1

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Database SQL/XMLDB
    • Labels:
      None
    • Environment:
      MySQL
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      • Install Moodle 2.0.x on MySQL
      • TEST: Verify question->defaultgrade is unsigned=true
      • TEST: Verify quiz_question_statistics->maxgrade is unsigned=false
      • Upgrade to 2.1beta
      • TEST: Verify question->defaultmark is unsigned=true
      • TEST: Verify quiz_question_statistics->maxmark is unsigned=false
      • TEST: Run DB functional tests for MySQL. They all should pass (but the collation related ones in case the DB isn't properly set).
      Show
      Install Moodle 2.0.x on MySQL TEST: Verify question->defaultgrade is unsigned=true TEST: Verify quiz_question_statistics->maxgrade is unsigned=false Upgrade to 2.1beta TEST: Verify question->defaultmark is unsigned=true TEST: Verify quiz_question_statistics->maxmark is unsigned=false TEST: Run DB functional tests for MySQL. They all should pass (but the collation related ones in case the DB isn't properly set).
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      unsigned_fixes
    • Rank:
      18003

      Description

      Fix DB differences in question->defaultmark and quiz_question_statistics->maxmark between installed & upgraded Moodle 2.1

      Pasted from MDL-27929:

      About question->defaultmark, it seems to be unsigned = true, both in install.xml and in its history in upgrade.php (comes from question->defaultgrade that always was unsigned). So I think we need to fix upgrade.php making it unsigned.

      About quiz_question_statistics->maxmark, it seems to be unsigned = false in install.xml (and also another question_attempts->maxmark is unsigned=false). And in upgrade, we can find both situations, but @ 2008112102 the previous column (maxgrade) was changes explicity to unsigned = false. So I think we need to fix upgrade.php making it signed.

      So, in summary, we need to add one more step to upgrade to:

      1) modify question->defaultmark to unsigned = true:
      2) modify quiz_question_statistics->maxmark to unsigned = false;

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Submitted for integration, it includes two commits:

          • d466e23c: Causing the mysql driver to retrofit the signed/unsigned information from DB for numeric/float types.
          • a0628c68: upgrade scripts in charge of adjusting wrong columns as stated above.

          Any comment is welcome, Tim, just in case I interpreted wrongly the nature of the changes to perform.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Submitted for integration, it includes two commits: d466e23c: Causing the mysql driver to retrofit the signed/unsigned information from DB for numeric/float types. a0628c68: upgrade scripts in charge of adjusting wrong columns as stated above. Any comment is welcome, Tim, just in case I interpreted wrongly the nature of the changes to perform. Ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Eloy - integrated now

          Show
          Sam Hemelryk added a comment - Thanks Eloy - integrated now
          Hide
          Sam Hemelryk added a comment -

          Thanks Eloy - passed as well (tested during integration)

          Show
          Sam Hemelryk added a comment - Thanks Eloy - passed as well (tested during integration)
          Hide
          Tim Hunt added a comment -

          As component owner - who should have peer-reviewed this before two crazy integrators took it upon themselves to integrate incorrect code - can I change the tested state to 'Failed' please.

          Show
          Tim Hunt added a comment - As component owner - who should have peer-reviewed this before two crazy integrators took it upon themselves to integrate incorrect code - can I change the tested state to 'Failed' please.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          My fault for sending this to integration instead of perr-reviewing, do we have a button for that, lol, didn't know! Sorry.

          About the nature of the code... can you point to the correct direction (final status?) Or what? Note current decision was taken based 100% on values from 2.0 (where those values are different, both @ install.xml and upgrade.xml).

          So, please clarify it and will be re-cooked. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - My fault for sending this to integration instead of perr-reviewing, do we have a button for that, lol, didn't know! Sorry. About the nature of the code... can you point to the correct direction (final status?) Or what? Note current decision was taken based 100% on values from 2.0 (where those values are different, both @ install.xml and upgrade.xml). So, please clarify it and will be re-cooked. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening this... awaiting for final direction to take...

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening this... awaiting for final direction to take...
          Hide
          Tim Hunt added a comment -

          In practice I don't think it makes any practical difference. I think these numbers will probably be >= 0 always, so can be stored just fine in the NUMBER column.

          However, why use unsigned at all? These are float numbers (once they get into PHP) and PHP (nor any other computer language I know of) has no concept of unsigned float.

          Show
          Tim Hunt added a comment - In practice I don't think it makes any practical difference. I think these numbers will probably be >= 0 always, so can be stored just fine in the NUMBER column. However, why use unsigned at all? These are float numbers (once they get into PHP) and PHP (nor any other computer language I know of) has no concept of unsigned float.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          so both must end being signed, correct? Oki:

          • quiz_question_statistics->maxmark is signed already. No changes needed.
          • question->defaultmark is unsigned. I'll provide patch to fix it (reusing last added upgrade step, incremented by -.01 just for early adopters).

          Sounds ok?

          Show
          Eloy Lafuente (stronk7) added a comment - - edited so both must end being signed, correct? Oki: quiz_question_statistics->maxmark is signed already. No changes needed. question->defaultmark is unsigned. I'll provide patch to fix it (reusing last added upgrade step, incremented by -.01 just for early adopters). Sounds ok?
          Hide
          Tim Hunt added a comment -

          Perfect, thank you very much Eloy.

          Show
          Tim Hunt added a comment - Perfect, thank you very much Eloy.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Done, now the dev branch includes a 3rd commit, redefining the final status of question->defaultmark to signed as requested. I've incremented the .01 to 02 so early adopters of integration.git get the change applied too.

          Commit: f4fd96a8 ( https://github.com/stronk7/moodle/commit/f4fd96a848fd8fd4c32a47a29f0136149976b9cc )

          It should apply clean over current integration.git. Sending to his super-duper peer-reviewer (btw that button is really hidden here, grrr, cause I don't use the "start development" transition usually).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Done, now the dev branch includes a 3rd commit, redefining the final status of question->defaultmark to signed as requested. I've incremented the .01 to 02 so early adopters of integration.git get the change applied too. Commit: f4fd96a8 ( https://github.com/stronk7/moodle/commit/f4fd96a848fd8fd4c32a47a29f0136149976b9cc ) It should apply clean over current integration.git. Sending to his super-duper peer-reviewer (btw that button is really hidden here, grrr, cause I don't use the "start development" transition usually). Ciao
          Hide
          Tim Hunt added a comment -

          Well, this crappy peer-reviewer thinks it is right now.

          By the way, I would also like it if the Request peer review button was available directly from the Open state.

          Show
          Tim Hunt added a comment - Well, this crappy peer-reviewer thinks it is right now. By the way, I would also like it if the Request peer review button was available directly from the Open state.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho, thanks Tim! (btw, I've created MDLSITE-1320)

          SamH I'm getting this for immediate integration as far as is tested and want to add it together with MDL-28021 that also involved DB upgrade.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, thanks Tim! (btw, I've created MDLSITE-1320 ) SamH I'm getting this for immediate integration as far as is tested and want to add it together with MDL-28021 that also involved DB upgrade. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          re-integrated.

          Show
          Eloy Lafuente (stronk7) added a comment - re-integrated.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          and passing tests, has worked here ok and now I don't get installed vs upgraded differences anymore, yay!

          Show
          Eloy Lafuente (stronk7) added a comment - and passing tests, has worked here ok and now I don't get installed vs upgraded differences anymore, yay!
          Hide
          Tim Hunt added a comment -

          Thank you for fixing this Eloy.

          Show
          Tim Hunt added a comment - Thank you for fixing this Eloy.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Moode is now, for sure, a bit better thanks to you! This fix is now upstream.

          Show
          Eloy Lafuente (stronk7) added a comment - Moode is now, for sure, a bit better thanks to you! This fix is now upstream.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: