Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Quiz
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15294

      Description

      This purpose of this task is to fix the quiz releated database discrepancies between a fresh install of Moodle 2.0 and an upgraded site.

      quiz

      • questiondecimalpoints in 1.9 defaults to 2, in 2.0 defaults to -2
      • sumgrades, grade in 1.9 are DEFAULT NULL, in 2.0 NOT NULL DEFAULT '0.0000000',

      quiz_attempts

      • sumgrades in 1.9 are DEFAULT NULL, in 2.0 NOT NULL DEFAULT '0.0000000',

      quiz_feedback

      • mingrade, maxgrade in 1.9 are DEFAULT NULL, in 2.0 NOT NULL DEFAULT '0.0000000',

      quiz_grades

      • grade in 1.9 are DEFAULT NULL, in 2.0 NOT NULL DEFAULT '0.0000000',

      quiz_question_instances

      • grade in 1.9 are DEFAULT NULL, in 2.0 NOT NULL DEFAULT '0.0000000',

        Activity

        Hide
        Sam Hemelryk added a comment -

        Hi Tim,

        I've added you as a watcher here, this issue is aiming to resolve database discrepancies between a fresh install of Moodle 2.0 and an upgraded site.
        Could you please review the changes I am purposing: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25791

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Tim, I've added you as a watcher here, this issue is aiming to resolve database discrepancies between a fresh install of Moodle 2.0 and an upgraded site. Could you please review the changes I am purposing: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25791 Cheers Sam
        Hide
        Tim Hunt added a comment -

        Almost all these things should be NOT NULL with no default, so we get error messages when there are bugs in the code that mean these fields are not set correctly. However, I seem to remember that XMLDB cannot cope with that.

        I cannot currently recall why I used a default of -2 for questoindecimalpoints. Perhaps it was to try to flush out bugs. The legal values are -1 (meaning use the decimalpoints value) and 0 to 7.

        Please do not commit PHP doc comments like * @global moodle_database $DB anywhere in code I am responsible for. I know that Netbeans creates those by default, but they are a total waste of space. Look, 5 lines below, I can see that this function uses global $DB. If fact, I would go further and say never commit PHP doc comments like that, but I don't have jurisdiction over the rest of Moodle.

        What exactly is the point of the extra conditionals you added in https://github.com/samhemelryk/moodle/commit/bc90ac05f8730e024b663163edd257e22a2a9ee2 ? These changes are idempotent.

        Show
        Tim Hunt added a comment - Almost all these things should be NOT NULL with no default, so we get error messages when there are bugs in the code that mean these fields are not set correctly. However, I seem to remember that XMLDB cannot cope with that. I cannot currently recall why I used a default of -2 for questoindecimalpoints. Perhaps it was to try to flush out bugs. The legal values are -1 (meaning use the decimalpoints value) and 0 to 7. Please do not commit PHP doc comments like * @global moodle_database $DB anywhere in code I am responsible for. I know that Netbeans creates those by default, but they are a total waste of space. Look, 5 lines below, I can see that this function uses global $DB. If fact, I would go further and say never commit PHP doc comments like that, but I don't have jurisdiction over the rest of Moodle. What exactly is the point of the extra conditionals you added in https://github.com/samhemelryk/moodle/commit/bc90ac05f8730e024b663163edd257e22a2a9ee2 ? These changes are idempotent.
        Hide
        Sam Hemelryk added a comment -

        Hi Tim,

        Thanks for looking it over.

        Given what you've told me I've made two more commits, the first is to remove the default values from the grade fields, I did this with XMLDB editor, it didn't seem to have any problem with it, perhaps a recent fix?
        The second commit was to remove the @global moodle_database $DB as requested.

        In regards to the @globals I am personally a fan of those, it allows Netbeans to auto-complete for me, without it sometimes it will sometimes it won't. As it is part of the PHPdocs I didn't think it would hurt to add it, although I completely understand that anyone who is familiar with Moodle will instantly skip over it as useless information.

        There is a small story in regards to the changes made in the commit you asked about, bc90ac.
        When I first started work on fixing the discrepancies within the core tables I chatted to Eloy about whether or not we should change old upgrade code, after a bit of discussion we decided that it would be best to both change upgrade code so that new sites or sites that have not upgraded yet are fixed as early on as possible, and at the same time add conditions around the new upgrade code so that those who's database is correct don't have the burden of running the unnecessary upgrade code.
        In the case of core tables this saves us having to drop+recreate indexes, and avoid other timetaking tasks. For the quiz it didn't look like there was any changes that would significantly hurt performance however I added the conditions anyway.
        If you prefer they can be removed, given you know that story behind it now I am quite happy to go with what ever you recommend.

        Let me know your thoughts on the above and whether you are happy with the further changes.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Tim, Thanks for looking it over. Given what you've told me I've made two more commits, the first is to remove the default values from the grade fields, I did this with XMLDB editor, it didn't seem to have any problem with it, perhaps a recent fix? The second commit was to remove the @global moodle_database $DB as requested. In regards to the @globals I am personally a fan of those, it allows Netbeans to auto-complete for me, without it sometimes it will sometimes it won't. As it is part of the PHPdocs I didn't think it would hurt to add it, although I completely understand that anyone who is familiar with Moodle will instantly skip over it as useless information. There is a small story in regards to the changes made in the commit you asked about, bc90ac. When I first started work on fixing the discrepancies within the core tables I chatted to Eloy about whether or not we should change old upgrade code, after a bit of discussion we decided that it would be best to both change upgrade code so that new sites or sites that have not upgraded yet are fixed as early on as possible, and at the same time add conditions around the new upgrade code so that those who's database is correct don't have the burden of running the unnecessary upgrade code. In the case of core tables this saves us having to drop+recreate indexes, and avoid other timetaking tasks. For the quiz it didn't look like there was any changes that would significantly hurt performance however I added the conditions anyway. If you prefer they can be removed, given you know that story behind it now I am quite happy to go with what ever you recommend. Let me know your thoughts on the above and whether you are happy with the further changes. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Tim,

        Have you had a chance to look over the changes I made after your feedback?
        I would like to get this in for review on Monday if possible.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Tim, Have you had a chance to look over the changes I made after your feedback? I would like to get this in for review on Monday if possible. Cheers Sam
        Hide
        Tim Hunt added a comment -

        I agree about the extra conditionals. Thanks for explaining.

        Also, sorry, but where I say above "these things should be NOT NULL with no default, so we get error messages when there are bugs in the code that mean these fields are not set correctly". I actually don't think we should risk changing this on the 2.0 stable branch. We don't want to find those bugs there. That is, we should not change install.xml since the goal of this bug is to fix the discrepancy between fresh install and upgrade.

        So, actually, I think you should throw away commit a8ee722f67371365a94268a0ec6f79dc80a083da for now. I will include this in my question engine rewrite.

        Sorry for wasting your time.

        Show
        Tim Hunt added a comment - I agree about the extra conditionals. Thanks for explaining. Also, sorry, but where I say above "these things should be NOT NULL with no default, so we get error messages when there are bugs in the code that mean these fields are not set correctly". I actually don't think we should risk changing this on the 2.0 stable branch. We don't want to find those bugs there. That is, we should not change install.xml since the goal of this bug is to fix the discrepancy between fresh install and upgrade. So, actually, I think you should throw away commit a8ee722f67371365a94268a0ec6f79dc80a083da for now. I will include this in my question engine rewrite. Sorry for wasting your time.
        Hide
        Sam Hemelryk added a comment -

        No probs, thanks for reviewing it again Tim.
        I've dropped the commit a8ee72 and created PULL-125 now.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - No probs, thanks for reviewing it again Tim. I've dropped the commit a8ee72 and created PULL-125 now. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Hi Tim,

        Petr rejected the pull request for this issue because he encountered an error due to having quiz_attempts.sumgrades = NULL (check out his comment on PULL-125) for more details.
        I've been trying to reproduce this bug but havn't had any luck, I'm not too sure what is going on there.
        However having had a quick look at the code it appears that quiz_attempts.sumgrades is default to 0.
        Should 0.0 be set as the default in the database or is there something else that we should try and do here do you think?

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Tim, Petr rejected the pull request for this issue because he encountered an error due to having quiz_attempts.sumgrades = NULL (check out his comment on PULL-125) for more details. I've been trying to reproduce this bug but havn't had any luck, I'm not too sure what is going on there. However having had a quick look at the code it appears that quiz_attempts.sumgrades is default to 0. Should 0.0 be set as the default in the database or is there something else that we should try and do here do you think? Cheers Sam
        Hide
        Tim Hunt added a comment -

        Can we find out from Petr where that database with quiz_attempts.sumgrades NULL in some row came from? As far as I am aware, that column has always been NOT NULL DEFAULT 0.

        However, if some crappy databases currently in a bad state are preventing you from changing the column in an upgrade to fix it, then just do a SET sumgrades = 0 WHERE sumgrades IS NULL in the upgrade code before altering the column.

        Show
        Tim Hunt added a comment - Can we find out from Petr where that database with quiz_attempts.sumgrades NULL in some row came from? As far as I am aware, that column has always been NOT NULL DEFAULT 0. However, if some crappy databases currently in a bad state are preventing you from changing the column in an upgrade to fix it, then just do a SET sumgrades = 0 WHERE sumgrades IS NULL in the upgrade code before altering the column.
        Hide
        Sam Hemelryk added a comment -

        Hi Tim,

        Thanks for the feedback.
        I went through a collection of old site backups I had including moodle.org and checked them for null values but couldn't find any.
        I added a couple of lines to set null values to 0 for sumgrades and have created PULL-208 to get it reviewed.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Tim, Thanks for the feedback. I went through a collection of old site backups I had including moodle.org and checked them for null values but couldn't find any. I added a couple of lines to set null values to 0 for sumgrades and have created PULL-208 to get it reviewed. Cheers Sam
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi,

        plz take a look to http://tracker.moodle.org/browse/PULL-208?focusedCommentId=102982&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-102982 and state here the final solution agreed (defaults in / defaults out) changing the patch as required.

        I'll review this again tomorrow morning (EU). TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi, plz take a look to http://tracker.moodle.org/browse/PULL-208?focusedCommentId=102982&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-102982 and state here the final solution agreed (defaults in / defaults out) changing the patch as required. I'll review this again tomorrow morning (EU). TIA and ciao
        Hide
        Sam Hemelryk added a comment -

        Hmmmm well this has ended up being bit of a mess.
        I've spent this morning going through this issue again.

        I've just updated the pull request now with a revised branch https://github.com/samhemelryk/moodle/compare/master...MDL-25791_20_quizdb_rev1
        This branch is only one commit, and it doesn't change install.xml or remove the defaults (Tim as you noted you can do this during you work on quiz).
        So this is only upgrade code now.

        As for what went wrong, well I've gone back through everything, git history included, I've reviewed what Petr reviewed, have reviewed previous changes and have put together this branch (which I have also tested again, and again, and again).
        The problem came from a pebkec~git problem. After the last pull request was rejected I hit a strange situation where by branch showed it was up to date with master, I thought I had done something dumb, so just went through cherry picking the commits again to form a new branch which I submitted in the most recent request.
        During my cherry-picking I picked the commit a8ee72 again by mistake so it got back in, thus it ended up in the pull request branch (sorry!).
        During my searching I found that the problem came because my original pull request was merged and then reverted, thus my changes were included and in a later commit removed. I failed to notice this, had I we could have avoided this whole dilema.
        Ohh well

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hmmmm well this has ended up being bit of a mess. I've spent this morning going through this issue again. I've just updated the pull request now with a revised branch https://github.com/samhemelryk/moodle/compare/master...MDL-25791_20_quizdb_rev1 This branch is only one commit, and it doesn't change install.xml or remove the defaults (Tim as you noted you can do this during you work on quiz). So this is only upgrade code now. As for what went wrong, well I've gone back through everything, git history included, I've reviewed what Petr reviewed, have reviewed previous changes and have put together this branch (which I have also tested again, and again, and again). The problem came from a pebkec~git problem. After the last pull request was rejected I hit a strange situation where by branch showed it was up to date with master, I thought I had done something dumb, so just went through cherry picking the commits again to form a new branch which I submitted in the most recent request. During my cherry-picking I picked the commit a8ee72 again by mistake so it got back in, thus it ended up in the pull request branch (sorry!). During my searching I found that the problem came because my original pull request was merged and then reverted, thus my changes were included and in a later commit removed. I failed to notice this, had I we could have avoided this whole dilema. Ohh well Cheers Sam
        Hide
        Petr Škoda added a comment -

        Works fine for me now, thanks!

        Show
        Petr Škoda added a comment - Works fine for me now, thanks!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: