Moodle
  1. Moodle
  2. MDL-21987

Problem with repeat grade imports when is_overridable_item is true

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9.8
    • Fix Version/s: 1.9.9
    • Component/s: Gradebook
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      26628

      Description

      This issue affects 'overridable' gradebook columns. Those are columns where an automatic grade is assigned, like mod, course, calculated, ... items. See grade_item::is_overridable_item().

      Let me first explain what happens with a custom (non-overridable) column, because that currently works correctly.

      1. On Administration ? Grades ? General settings, set Primary grade export methods to XML file.

      2. Create a course, and add some students with idnumbers to it.

      3. Add a custom grade item, with an idnumber.

      4. Create an XML grade import file (in1.xml) to give grades to your students in your custom column.

      5. Import test.xml.

      6. Now export the column to XML (out1.xml), choosing the 'Export new or updated grades only' option. This file will contain the grades you just imported.

      7. Repeat step 6, exporting out2.xml. This file will be empty, as no grades have changed.

      8. Repeat step 5, importing in1.xml again.

      9. Repeat step 6, exporting out3.xml. Again this file will be emtpy, as no grades have changed.

      10. Change a few grades in in1.xml to create a new file in2.xml.

      11. Import in2.xml, then export out4.xml. out4.xml will contain only the changed grades.

      As I said above, I believe that behaviour is all correct.

      The problem happens if, for example, you change step 3 above to

      3*. Add an Oflline Activity to the course, with an idnumber.

      If you do that, everything proceed the same up until step 9.

      9*. Repeat step 6, exporting out3.xml. Now this file contains all the grades, because Moodle things they changed at step 8*.

      I think this is a bug.

      1. grade_import.patch.txt
        2 kB
        Tim Hunt
      2. grade_import.patch.txt
        2 kB
        Tim Hunt
      3. grade_import3.patch.txt
        1 kB
        Tim Hunt
      4. overridingupdate1.patch
        0.9 kB
        Petr Škoda

        Activity

        Hide
        Tim Hunt added a comment -

        My analysis is that the problem lies in grade_item::update_final_grade().

        Around line 1449, for grades where is_overridable_item returns true, it sets $grade->overridden to the current time, whether or not the new grade is different to the old grade.

        Then, on line 1477, when deciding whether the grade has changed and needs to be saved in the database, one of the tests it performs is whether $grade->overridden has changed. Well, of course it has just changed, we just changed it!

        Therefore, my proposed fix (grade_import.patch.txt) is to move the place where $grade->overridden is set to after the test for whether the grade has changed.

        Please (Andy or Petr) could you review this patch. If you think it is OK, I will commit it.

        Show
        Tim Hunt added a comment - My analysis is that the problem lies in grade_item::update_final_grade(). Around line 1449, for grades where is_overridable_item returns true, it sets $grade->overridden to the current time, whether or not the new grade is different to the old grade. Then, on line 1477, when deciding whether the grade has changed and needs to be saved in the database, one of the tests it performs is whether $grade->overridden has changed. Well, of course it has just changed, we just changed it! Therefore, my proposed fix (grade_import.patch.txt) is to move the place where $grade->overridden is set to after the test for whether the grade has changed. Please (Andy or Petr) could you review this patch. If you think it is OK, I will commit it.
        Hide
        Petr Škoda added a comment -

        hmmm, this might not be the best solution because we want to flip the overridden flag even ff the grade coming from external system is the same - so this should be done only if the final grade is overridden (of course only if the overridden flag actually makes sense).

        Going to have a look at the code after I commit my latest work in progress (cca 2 days).

        thanks for the report

        Show
        Petr Škoda added a comment - hmmm, this might not be the best solution because we want to flip the overridden flag even ff the grade coming from external system is the same - so this should be done only if the final grade is overridden (of course only if the overridden flag actually makes sense). Going to have a look at the code after I commit my latest work in progress (cca 2 days). thanks for the report
        Hide
        Petr Škoda added a comment -

        Confirming, the patch is causing regression, we need to set the flag even if the value is the same, only if the value did not change we can keep the original time in overridden flag

        Show
        Petr Škoda added a comment - Confirming, the patch is causing regression, we need to set the flag even if the value is the same, only if the value did not change we can keep the original time in overridden flag
        Hide
        Tim Hunt added a comment -

        Thanks for looking at my patch, but please could you explain:

        What regressions does the patch cause?

        Why do we need to set the [overridden] flag even if the value is the same?

        I would like to understand this code better.

        Show
        Tim Hunt added a comment - Thanks for looking at my patch, but please could you explain: What regressions does the patch cause? Why do we need to set the [overridden] flag even if the value is the same? I would like to understand this code better.
        Hide
        Petr Škoda added a comment -

        1/ any modification of grade that originally came from activity or is calculated ($this->is_overridable_item() === true) must set the overridden flag, otherwise the change would not "stick" and could be immediately undone by the activity

        2/ import of grades that were originally produced in activity like quiz does not make much sense because the only thing it can do is to force overridden flag on all imported grades; actually the only meaningful import is for manual items and hopefully for assignments in future (unfortunately I have to keep repeating over and over again that the assignment grading should be done strictly in gradebook === without the overridden flag)

        3/ since 1.9.0 the import always sets the overridden flag - it would be a regression if it did not do that when the current grade is present in import because activity would be able to change it at any later time or it would be quickly recalculated

        Show
        Petr Škoda added a comment - 1/ any modification of grade that originally came from activity or is calculated ($this->is_overridable_item() === true) must set the overridden flag, otherwise the change would not "stick" and could be immediately undone by the activity 2/ import of grades that were originally produced in activity like quiz does not make much sense because the only thing it can do is to force overridden flag on all imported grades; actually the only meaningful import is for manual items and hopefully for assignments in future (unfortunately I have to keep repeating over and over again that the assignment grading should be done strictly in gradebook === without the overridden flag) 3/ since 1.9.0 the import always sets the overridden flag - it would be a regression if it did not do that when the current grade is present in import because activity would be able to change it at any later time or it would be quickly recalculated
        Hide
        Tim Hunt added a comment -

        OK, I now understand why overridden flag must be set.

        The situation where we hit this problem is with a custom activity we created to simplify importing grades from another system. The Moodle activity just makes it easier to set up a link in the course + column in the gradebook + cron grade import.

        I'll make a new patch tomorrow that sets overridden flag the first time an import is done, but does not change the overridden + timemodified flags if you import the same grade again.

        Show
        Tim Hunt added a comment - OK, I now understand why overridden flag must be set. The situation where we hit this problem is with a custom activity we created to simplify importing grades from another system. The Moodle activity just makes it easier to set up a link in the course + column in the gradebook + cron grade import. I'll make a new patch tomorrow that sets overridden flag the first time an import is done, but does not change the overridden + timemodified flags if you import the same grade again.
        Hide
        Petr Škoda added a comment -

        Aah, in 2.0 there should be some way to specify that activity is not sending grades to gradebook === does not need the overridden flag, I will definitely use it in my new "assignent" thing

        Show
        Petr Škoda added a comment - Aah, in 2.0 there should be some way to specify that activity is not sending grades to gradebook === does not need the overridden flag, I will definitely use it in my new "assignent" thing
        Hide
        Tim Hunt added a comment -

        Revised patch attached. Basically, it just adds

        or $this->is_overridable_item() and !$oldgrade->overridden)

        to the test for whether the database needs to be updated (that is compared to yesterday's patch.) However, this is also a bit more code clean-up in there.

        Show
        Tim Hunt added a comment - Revised patch attached. Basically, it just adds or $this->is_overridable_item() and !$oldgrade->overridden) to the test for whether the database needs to be updated (that is compared to yesterday's patch.) However, this is also a bit more code clean-up in there.
        Hide
        Petr Škoda added a comment -

        Another new regression, the is_overridable_item_feedback() completely disappeared from the patch - so most probably this is going to break the overriding of assingment feedback.

        After a bit more thinking my +1 to keep the update_final_grade() as is in 1.9.x and instead improve the is_overridable_item() and is_overridable_item_feedback() because in your example you actually do not want the overriding to be there at all.

        Show
        Petr Škoda added a comment - Another new regression, the is_overridable_item_feedback() completely disappeared from the patch - so most probably this is going to break the overriding of assingment feedback. After a bit more thinking my +1 to keep the update_final_grade() as is in 1.9.x and instead improve the is_overridable_item() and is_overridable_item_feedback() because in your example you actually do not want the overriding to be there at all.
        Hide
        Petr Škoda added a comment -

        hmmm, thinking more aobut this, maybe the attached patch would solve it for you without any regressions for others...

        Show
        Petr Škoda added a comment - hmmm, thinking more aobut this, maybe the attached patch would solve it for you without any regressions for others...
        Hide
        Tim Hunt added a comment -

        OK, following more chat in dev chat, I (and more importantly Petr) think grade_import3.patch.txt is the right thing.

        Show
        Tim Hunt added a comment - OK, following more chat in dev chat, I (and more importantly Petr) think grade_import3.patch.txt is the right thing.
        Hide
        Tim Hunt added a comment -

        Fix now checked in.

        Show
        Tim Hunt added a comment - Fix now checked in.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: