Moodle
  1. Moodle
  2. MDL-29089

In grade_items_history the field 'decimals' is set as NOT NULL, but without a default value, yet the creation of a new course, category, or forum (for example) attempts to insert NULL here

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      With debug on, Set decimalpoints=NULL at site and course level, in a new course, then add a forum.

      Show
      With debug on, Set decimalpoints=NULL at site and course level, in a new course, then add a forum.
    • Workaround:
      Hide

      Set default for 'decimals' in grade_item_history to 0
      (Alternatively, but perhaps with consequences, set this field to allow NULL value

      Show
      Set default for 'decimals' in grade_item_history to 0 (Alternatively, but perhaps with consequences, set this field to allow NULL value
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      MDL-29089_grade_history_missing_columns2

      Description

      The insertion of a new category or course or gradable activity can throw an exception, visible when debug is on.
      Moodle attempts to force a NULL into the decimals field in grade_items_history as shown at http://moodle.org/mod/forum/discuss.php?d=182894
      This likely occurs only if decimalpoints value is NULL at the site and course levels in grade_settings so that the routine only has NULL available.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this.

            I've put it on our backlog and we'll try to get to it as soon as we can.

            Show
            Michael de Raadt added a comment - Thanks for reporting this. I've put it on our backlog and we'll try to get to it as soon as we can.
            Hide
            John White added a comment -

            This is actually a blocker! As with debug turned off, the user creating any gradeable item sees 'Error writing to database' and no item is written to grade_items_history. With debug switched on it is clear that a NULL is being forced into 'decimals'. And in addition my workaround of changing the default value for 'decimals' to 0 doesn't work, because the NULL is still pushed in.
            Yet this is problem is surprising since post Moodle 2.0 conversion all the 'decimals' in table grade_items were set to NULL, and all the 'decimals' on grade_items_history were set to 0.
            Regards,
            John

            Show
            John White added a comment - This is actually a blocker! As with debug turned off, the user creating any gradeable item sees 'Error writing to database' and no item is written to grade_items_history. With debug switched on it is clear that a NULL is being forced into 'decimals'. And in addition my workaround of changing the default value for 'decimals' to 0 doesn't work, because the NULL is still pushed in. Yet this is problem is surprising since post Moodle 2.0 conversion all the 'decimals' in table grade_items were set to NULL, and all the 'decimals' on grade_items_history were set to 0. Regards, John
            Hide
            John White added a comment -

            So now I have a workaround that works for all gradeable objects AFAIK, including the action of deleting a course (which had also given the same error), as follows:

            In /lib/grade/grade_object.php at lines 229, 260, and 319 (Moodle 2.0.4) immediately precede the call to:
            $DB->insert_record($this->table.'_history', $data);

            ...each time with...

            if (empty($data->decimals))

            { $data->decimals = 0; }

            I've tested this in several different scenarios, but by no means exhaustively.
            It appears to work well.

            Show
            John White added a comment - So now I have a workaround that works for all gradeable objects AFAIK, including the action of deleting a course (which had also given the same error), as follows: In /lib/grade/grade_object.php at lines 229, 260, and 319 (Moodle 2.0.4) immediately precede the call to: $DB->insert_record($this->table.'_history', $data); ...each time with... if (empty($data->decimals)) { $data->decimals = 0; } I've tested this in several different scenarios, but by no means exhaustively. It appears to work well.
            Hide
            Andrew Davis added a comment - - edited

            Looking at install.xml which lists the database structure and looking in my own development installs of Moodle grade_items_history doesn't even have a "decimals" column.

            So the first question is should there be a "decimals" column at all? If so, why don't I have one? If not, why do you have one?

            Show
            Andrew Davis added a comment - - edited Looking at install.xml which lists the database structure and looking in my own development installs of Moodle grade_items_history doesn't even have a "decimals" column. So the first question is should there be a "decimals" column at all? If so, why don't I have one? If not, why do you have one?
            Hide
            Andrew Davis added a comment -

            Ive been doing some research and I will run this past some other people at HQ however I dont think the history table should have a decimals column at all.

            It doesn't hurt to store that information I suppose however its somewhat pointless information to keep. The decimals field doesn't affect however the grade data is stored or calculated. It only affects how the data is displayed to the user.

            Show
            Andrew Davis added a comment - Ive been doing some research and I will run this past some other people at HQ however I dont think the history table should have a decimals column at all. It doesn't hurt to store that information I suppose however its somewhat pointless information to keep. The decimals field doesn't affect however the grade data is stored or calculated. It only affects how the data is displayed to the user.
            Hide
            Aparup Banerjee added a comment -

            perhaps there should be a way to check against the xmldb definitions and always report/handle these 'missed' differences (guessing from upgrade issue from version unsupported curently?)

            Show
            Aparup Banerjee added a comment - perhaps there should be a way to check against the xmldb definitions and always report/handle these 'missed' differences (guessing from upgrade issue from version unsupported curently?)
            Hide
            Andrew Davis added a comment -

            Adding links to some code to remove the decimals column from the grade_items_history table.

            Show
            Andrew Davis added a comment - Adding links to some code to remove the decimals column from the grade_items_history table.
            Hide
            Andrew Davis added a comment -

            Putting this up for peer review. Need some kind of judgment about whether this is the right solution.

            Show
            Andrew Davis added a comment - Putting this up for peer review. Need some kind of judgment about whether this is the right solution.
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            The code changes you've made look fine, and I certainly think that deleting the field is the solution.

            However before this goes up to integration I really think we need to try to track down the origin's of this field to make sure that we aren't dropping data that may have some potential value.
            I think the best thing to do it see if Martin, Petr, Eloy, or Tim have any recollection of this field, what it was used for, and/or why it may still be hanging around.

            John any chance you can give us a bit more information - it would be particularly helpful if you were able to remember the version of Moodle the site would have started at or roughly how long the site has been running and being upgraded. This would give us a time frame to hunt for the phantom field.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, The code changes you've made look fine, and I certainly think that deleting the field is the solution. However before this goes up to integration I really think we need to try to track down the origin's of this field to make sure that we aren't dropping data that may have some potential value. I think the best thing to do it see if Martin, Petr, Eloy, or Tim have any recollection of this field, what it was used for, and/or why it may still be hanging around. John any chance you can give us a bit more information - it would be particularly helpful if you were able to remember the version of Moodle the site would have started at or roughly how long the site has been running and being upgraded. This would give us a time frame to hunt for the phantom field. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            /me needs to learn to use the buttons

            Show
            Sam Hemelryk added a comment - /me needs to learn to use the buttons
            Hide
            Andrew Davis added a comment -

            I've messaged Martin, Petr and Eloy. Technical difficulties are preventing me reaching Tim right now but I will at the first opportunity.

            Show
            Andrew Davis added a comment - I've messaged Martin, Petr and Eloy. Technical difficulties are preventing me reaching Tim right now but I will at the first opportunity.
            Hide
            Martin Dougiamas added a comment -

            Sorry I don't remember anything about decimals in the grade history code!

            Show
            Martin Dougiamas added a comment - Sorry I don't remember anything about decimals in the grade history code!
            Hide
            Andrew Davis added a comment - - edited

            Looking back in the history of install.xml I found these:

            Revision 1.121 - (view) (download) (as text) (annotate) - [select for diffs]
            Tue Sep 25 16:35:17 2007 WST (3 years, 11 months ago) by nicolasconnault
            Branch: MAIN
            Changes since 1.120: +48 -47 lines
            Diff to previous 1.120
            MDL-11433 Added decimals field to grade_items table

            Revision 1.124 - (view) (download) (as text) (annotate) - [select for diffs]
            Sat Sep 29 05:00:31 2007 WST (3 years, 11 months ago) by skodak
            Branch: MAIN
            Changes since 1.123: +1 -1 lines
            Diff to previous 1.123
            MDL-11504 fixed defaults in grade_items/display; added decimals and display to grade_items_history

            Here is the link to the diff for that second commit. http://cvs.moodle.org/moodle/lib/db/install.xml?r1=1.123&r2=1.124
            Oddly you can see the modification of the grade_items.display default but not the addition of fields to grade_items_history. Actually the grade_items_history table doesnt have a decimals or display column. Maybe they were just forgotten...

            Show
            Andrew Davis added a comment - - edited Looking back in the history of install.xml I found these: Revision 1.121 - (view) (download) (as text) (annotate) - [select for diffs] Tue Sep 25 16:35:17 2007 WST (3 years, 11 months ago) by nicolasconnault Branch: MAIN Changes since 1.120: +48 -47 lines Diff to previous 1.120 MDL-11433 Added decimals field to grade_items table Revision 1.124 - (view) (download) (as text) (annotate) - [select for diffs] Sat Sep 29 05:00:31 2007 WST (3 years, 11 months ago) by skodak Branch: MAIN Changes since 1.123: +1 -1 lines Diff to previous 1.123 MDL-11504 fixed defaults in grade_items/display; added decimals and display to grade_items_history Here is the link to the diff for that second commit. http://cvs.moodle.org/moodle/lib/db/install.xml?r1=1.123&r2=1.124 Oddly you can see the modification of the grade_items.display default but not the addition of fields to grade_items_history. Actually the grade_items_history table doesnt have a decimals or display column. Maybe they were just forgotten...
            Hide
            Tim Hunt added a comment -

            git log --grep MDL-11433
            and
            git log --grep MDL-11504

            Will get you the information you need to find those changes in git.

            OK, so grade_item_history is meant to track all changes to grade_items, so the column definitions in the two tables should be the same. If not, we can probably assume that grade_items is right, and grade_items_history has got out of synch. Therefore you need to
            1. Updated install.xml to fix the definition of grade_items_history
            2. Write a DB upgrade script to fix the changes too.
            3. while you are at it, check that the other grade_history tables have not also drifted out of synch with the grade tables.

            Show
            Tim Hunt added a comment - git log --grep MDL-11433 and git log --grep MDL-11504 Will get you the information you need to find those changes in git. OK, so grade_item_history is meant to track all changes to grade_items, so the column definitions in the two tables should be the same. If not, we can probably assume that grade_items is right, and grade_items_history has got out of synch. Therefore you need to 1. Updated install.xml to fix the definition of grade_items_history 2. Write a DB upgrade script to fix the changes too. 3. while you are at it, check that the other grade_ history tables have not also drifted out of synch with the grade tables.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Sorry, I don't know/remember anything about that column.

            But I'm with Tim about all the "history" tables being 100% clones (but with own "id" and "oldid") of their "current" counterparts. No matter if it's useful for calculating grades or to display them.

            Just imagine the goal is to be able to rebuild the "current" tables with the information in the "history" ones. To achieve that we need 100% cloning.

            So IMO, I'd review all them, fix them and add one unit test comparing them to be able to detect the problem if happens again.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Sorry, I don't know/remember anything about that column. But I'm with Tim about all the "history" tables being 100% clones (but with own "id" and "oldid") of their "current" counterparts. No matter if it's useful for calculating grades or to display them. Just imagine the goal is to be able to rebuild the "current" tables with the information in the "history" ones. To achieve that we need 100% cloning. So IMO, I'd review all them, fix them and add one unit test comparing them to be able to detect the problem if happens again. Ciao
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            I've raised this to critical (causes continuous error with history tables enabled) and added versions where the problem is happening.

            Show
            Eloy Lafuente (stronk7) added a comment - - edited I've raised this to critical (causes continuous error with history tables enabled) and added versions where the problem is happening.
            Hide
            Andrew Davis added a comment -

            Ok. Having had a think about it I agree that the history tables should exactly mirror the actual tables (aside from ID).

            Show
            Andrew Davis added a comment - Ok. Having had a think about it I agree that the history tables should exactly mirror the actual tables (aside from ID).
            Hide
            Andrew Davis added a comment - - edited

            Ive gone through the grades tables and identified the following columns as being missing.

            grade_item_history
            display int(10) signed not null default 0
            decimals int(1) unsigned

            grade_grades

            grade_categories
            hidden int(1) unsigned not null default 0

            grade_outcomes
            descriptionformat int(2) unsigned not null default 0
            usermodified int(10) unsigned

            Note that the history tables dont have a timecreated column. Instead the timecreated is stored in timemodified of a history row with action == GRADE_HISTORY_INSERT

            Show
            Andrew Davis added a comment - - edited Ive gone through the grades tables and identified the following columns as being missing. grade_item_history display int(10) signed not null default 0 decimals int(1) unsigned grade_grades grade_categories hidden int(1) unsigned not null default 0 grade_outcomes descriptionformat int(2) unsigned not null default 0 usermodified int(10) unsigned Note that the history tables dont have a timecreated column. Instead the timecreated is stored in timemodified of a history row with action == GRADE_HISTORY_INSERT
            Hide
            Andrew Davis added a comment -

            Im leaving out the usermodified column of grade_outcomes as all history tables have a loggeduser column.

            Show
            Andrew Davis added a comment - Im leaving out the usermodified column of grade_outcomes as all history tables have a loggeduser column.
            Hide
            Andrew Davis added a comment -

            Updated git branch info.

            Show
            Andrew Davis added a comment - Updated git branch info.
            Hide
            Andrew Davis added a comment -

            Ive created a separate issue to create unit tests to prevent this issue reoccuring.

            Show
            Andrew Davis added a comment - Ive created a separate issue to create unit tests to prevent this issue reoccuring.
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            I've been looking at this now, but there's something that doesn't match up.

            In install.xml you add the following columns:

            • grade_items_history.display
            • grade_items_history.decimals
            • grade_categories_history.hidden
            • grade_outcomes_history.descriptionformat

            In upgrade.php you add the following:

            • grade_items_history.display
            • grade_items_history.decimals
            • grade_categories.hidden
            • grade_outcomes.descriptionformat

            Surely those grade_categories and grade_outcomes in the upgrade are meant to be their history counterparts.
            As an interesting observation there are already two sections of upgrade code that add grade_categories.hidden - this would add a third

            In regards to your code in upgrade.php I think the full field definition should be specified during its construction rather than after we know it exists through set_attributes.
            Just makes it more inline with code in upgrade.php and is the what the xmldb editor outputs.
            I also think that there should be a separate upgrade block for each table.

            In regard to the original problem it looks like there is some circumstance in which with all the muddle going on with the decimals column there is perhaps a chance users have ended up with grade_item_history.decimals table with NOTNULL. Should we include upgrade code to fix this problem should it exist or did you rule it out?

            Note I haven't compared the tables myself.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, I've been looking at this now, but there's something that doesn't match up. In install.xml you add the following columns: grade_items_history.display grade_items_history.decimals grade_categories_history.hidden grade_outcomes_history.descriptionformat In upgrade.php you add the following: grade_items_history.display grade_items_history.decimals grade_categories.hidden grade_outcomes.descriptionformat Surely those grade_categories and grade_outcomes in the upgrade are meant to be their history counterparts. As an interesting observation there are already two sections of upgrade code that add grade_categories.hidden - this would add a third In regards to your code in upgrade.php I think the full field definition should be specified during its construction rather than after we know it exists through set_attributes. Just makes it more inline with code in upgrade.php and is the what the xmldb editor outputs. I also think that there should be a separate upgrade block for each table. In regard to the original problem it looks like there is some circumstance in which with all the muddle going on with the decimals column there is perhaps a chance users have ended up with grade_item_history.decimals table with NOTNULL. Should we include upgrade code to fix this problem should it exist or did you rule it out? Note I haven't compared the tables myself. Cheers Sam
            Hide
            Andrew Davis added a comment -

            Thanks for that Sam. Not sure what was up with me. I believe Ive fixed that all including adding a check for a grade_items_history.decimals column that has somehow become "not null".

            Show
            Andrew Davis added a comment - Thanks for that Sam. Not sure what was up with me. I believe Ive fixed that all including adding a check for a grade_items_history.decimals column that has somehow become "not null".
            Hide
            Sam Hemelryk added a comment -

            Hi Andrew,

            Changes look spot on this time, just one minor thing (feel free to put it up for integration once fixed):

            • upgrade.php line 6741 empty set_attributes call.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Andrew, Changes look spot on this time, just one minor thing (feel free to put it up for integration once fixed): upgrade.php line 6741 empty set_attributes call. Cheers Sam
            Hide
            Andrew Davis added a comment -

            Fixed that and putting it up for integration.

            Show
            Andrew Davis added a comment - Fixed that and putting it up for integration.
            Hide
            Aparup Banerjee added a comment -

            Great detective work, this has been integrated into master and ready for testing.

            Show
            Aparup Banerjee added a comment - Great detective work, this has been integrated into master and ready for testing.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Uh, oh. Just got this when running the automated BD schema comparison between 2.2 installed from scratch and 2.2 upgraded from 2.1:

            • Column display of table grade_items_history difference found in unsigned: false !== true

            Looking...

            Show
            Eloy Lafuente (stronk7) added a comment - Uh, oh. Just got this when running the automated BD schema comparison between 2.2 installed from scratch and 2.2 upgraded from 2.1: Column display of table grade_items_history difference found in unsigned: false !== true Looking...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Confirmed:

            • at install.xml it is declared as UNSIGNED="false" (both the original grade_items and the grade_items_history tables).
            • at upgrade.php there is one error and its sign is incorrectly declared as XMLDB_TYPE_INTEGER, which I guess leads to UNSIGNED="true".

            Andrew, Aparup, can you please add one extra commit replacing that XMLDB_TYPE_INTEGER by null and done. I think that's enough for having the schemas back to 100% matching.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Confirmed: at install.xml it is declared as UNSIGNED="false" (both the original grade_items and the grade_items_history tables). at upgrade.php there is one error and its sign is incorrectly declared as XMLDB_TYPE_INTEGER, which I guess leads to UNSIGNED="true". Andrew, Aparup, can you please add one extra commit replacing that XMLDB_TYPE_INTEGER by null and done. I think that's enough for having the schemas back to 100% matching. TIA and ciao
            Hide
            Andrew Davis added a comment -

            Well spotted Eloy. Aparup has said he'll clean this up.

            Thankyou both for noticing and cleaning up my mess :|

            Show
            Andrew Davis added a comment - Well spotted Eloy. Aparup has said he'll clean this up. Thankyou both for noticing and cleaning up my mess :|
            Hide
            Rajesh Taneja added a comment -

            Works Great
            Thanks for fixing this Andrew.

            Show
            Rajesh Taneja added a comment - Works Great Thanks for fixing this Andrew.
            Hide
            Aparup Banerjee added a comment -

            Hi, thats just been fixed, Andrew-reviewed and pushed into integration/master. It would make sense to run test described by Eloy here too : http://tracker.moodle.org/browse/MDL-29089?focusedCommentId=125928&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-125928

            Thanks for picking that up Eloy!

            Show
            Aparup Banerjee added a comment - Hi, thats just been fixed, Andrew-reviewed and pushed into integration/master. It would make sense to run test described by Eloy here too : http://tracker.moodle.org/browse/MDL-29089?focusedCommentId=125928&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-125928 Thanks for picking that up Eloy!
            Hide
            Rajesh Taneja added a comment -

            Tested again, works as before
            Thanks Eloy for spotting it and Aparup for the patch.

            FYI:
            Not sure how to test http://tracker.moodle.org/browse/MDL-29089?focusedCommentId=125928&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-125928, so haven't done this. Please feel free to reopen this for testing if this is required.

            Show
            Rajesh Taneja added a comment - Tested again, works as before Thanks Eloy for spotting it and Aparup for the patch. FYI: Not sure how to test http://tracker.moodle.org/browse/MDL-29089?focusedCommentId=125928&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-125928 , so haven't done this. Please feel free to reopen this for testing if this is required.
            Hide
            Eloy Lafuente (stronk7) added a comment - - edited

            Just to confirm that the schema comparison test is now passing ok, thanks guys!

            Ciao

            PS: Rajesh, about the tool... I pasted it @ http://pastebin.com/BPhvVSCi It uses own Moodle DB stuff to perform the comparison, and does it for each commit landing to integration.git

            Show
            Eloy Lafuente (stronk7) added a comment - - edited Just to confirm that the schema comparison test is now passing ok, thanks guys! Ciao PS: Rajesh, about the tool... I pasted it @ http://pastebin.com/BPhvVSCi It uses own Moodle DB stuff to perform the comparison, and does it for each commit landing to integration.git
            Hide
            Rajesh Taneja added a comment -

            Thanks Eloy

            Show
            Rajesh Taneja added a comment - Thanks Eloy
            Hide
            Eloy Lafuente (stronk7) added a comment -

            git repositories have been updated with your awesome changes, thanks! Closing.

            Show
            Eloy Lafuente (stronk7) added a comment - git repositories have been updated with your awesome changes, thanks! Closing.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: