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

      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.

        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: