Moodle
  1. Moodle
  2. MDL-33125

Repeated import results in duplicate manual grade items

    Details

    • Testing Instructions:
      Hide

      */ Create a new empty course
      */ Add a manual grade_item called manualtest (in the gradebook, goto: Categories and items / ► Simple view in the settings block and click 'Add grade item' from the page that comes up)
      */ Add a resource (a label for example) in the course.
      */ Use the duplicate icon to copy the resource in the course one or more times.
      */ Check the gradebook:

      If you have more than one item called 'manualtest', this fix has failed.

      If you have just the one item, this test has passed and the patch works as intended.

      Show
      */ Create a new empty course */ Add a manual grade_item called manualtest (in the gradebook, goto: Categories and items / ► Simple view in the settings block and click 'Add grade item' from the page that comes up) */ Add a resource (a label for example) in the course. */ Use the duplicate icon to copy the resource in the course one or more times. */ Check the gradebook: If you have more than one item called 'manualtest', this fix has failed. If you have just the one item, this test has passed and the patch works as intended.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
    • Rank:
      40394

      Description

      If you import content from a course that has grade_items of the 'manual' itemtype, these grade items are imported into your destination course.
      The problem comes in if you do a subsequent import, the manual grade items are imported /again/.

      This again highlights the issues around import importing stuff without giving the user the option to not include it (like groups) and not correctly checking for duplicates on doing so.

      There should probably be some check to see if the identical grade item already exists in the course. For manual grade_items this might be difficult to guarentee its the same grade_item however...

      To reproduce:
      */ Create a new empty course
      */ Add a manual grade_item
      */ Duplicate a resource in the course one or more times.
      You'll see the manual grade item has been imported as many times as you've done imports.

      This may not seem like a big deal, but if you chain these together, it gets insane. One course with say, 10 manual items, might get imported into a course 2 or 3 times (the teacher selecting different things to import each time), giving this course 20~30 items. Then another course imports from that course a couple times, getting 20~30 each time... and so on. This doubling effect quickly adds up, to the point I've seen what started as 4 manual grade items in a course now adding up to 258k grade_items in a course!

        Activity

        Hide
        Adam Olley added a comment -

        Thinking about it, this might be as simple as preventing the import of the extra manual grade_items when importing where a grade item of the same itemname already exists. Afterall, import doesn't include user grades.

        Show
        Adam Olley added a comment - Thinking about it, this might be as simple as preventing the import of the extra manual grade_items when importing where a grade item of the same itemname already exists. Afterall, import doesn't include user grades.
        Hide
        Adam Olley added a comment -

        Fix added to github.

        Show
        Adam Olley added a comment - Fix added to github.
        Hide
        Michael de Raadt added a comment -

        Thanks for reporting that and providing a fix.

        Feel free to push this to peer review.

        Show
        Michael de Raadt added a comment - Thanks for reporting that and providing a fix. Feel free to push this to peer review.
        Hide
        Andrew Davis added a comment -

        [Y] Syntax
        [NA] Output
        [Y] Whitespace
        [NA] Language
        [NA] Databases
        [N] Testing
        [NA] Security
        [NA] Documentation
        [N] Git
        [Y] Sanity check

        Looks good.

        The git commit message isn't quite right. It should be "MDL-33125 core_backup: Prevent import/activity duplication..." You're just missing the component "core_backup".

        Can you please add some testing instructions. The more detailed the better.

        Show
        Andrew Davis added a comment - [Y] Syntax [NA] Output [Y] Whitespace [NA] Language [NA] Databases [N] Testing [NA] Security [NA] Documentation [N] Git [Y] Sanity check Looks good. The git commit message isn't quite right. It should be " MDL-33125 core_backup: Prevent import/activity duplication..." You're just missing the component "core_backup". Can you please add some testing instructions. The more detailed the better.
        Hide
        Adam Olley added a comment -

        No problem, change made

        P.S. when did that become a requirement?

        Show
        Adam Olley added a comment - No problem, change made P.S. when did that become a requirement?
        Hide
        Andrew Davis added a comment -

        I think the component in the commit message was always a requirement. It was just poorly communicated and rarely enforced.

        You just need some testing instructions and you're done

        Show
        Andrew Davis added a comment - I think the component in the commit message was always a requirement. It was just poorly communicated and rarely enforced. You just need some testing instructions and you're done
        Hide
        Andrew Davis added a comment -

        Hi Adam. You're still missing testing instructions. Edit the issue to get to the test instructions field. Try to make them as explicit as possible so that someone who isn't necessarily particularly familiar with the area can verify that it is working correctly.

        Show
        Andrew Davis added a comment - Hi Adam. You're still missing testing instructions. Edit the issue to get to the test instructions field. Try to make them as explicit as possible so that someone who isn't necessarily particularly familiar with the area can verify that it is working correctly.
        Hide
        Adam Olley added a comment -

        I must've been half asleep last time and not noticed where you wrote it needed test instructions. Sorry about that

        Show
        Adam Olley added a comment - I must've been half asleep last time and not noticed where you wrote it needed test instructions. Sorry about that
        Hide
        Andrew Davis added a comment -

        Ok, I think we're ready for integration review. Adam, let me know if you don't have the capability to submit for integration.

        Show
        Andrew Davis added a comment - Ok, I think we're ready for integration review. Adam, let me know if you don't have the capability to submit for integration.
        Hide
        Andrew Davis added a comment -

        Oh, and as this issue has been around for a while it would be worth rebasing the master branch at least. Just to make sure the integrators don't immediately run into problems when reviewing this.

        Show
        Andrew Davis added a comment - Oh, and as this issue has been around for a while it would be worth rebasing the master branch at least. Just to make sure the integrators don't immediately run into problems when reviewing this.
        Hide
        Adam Olley added a comment -

        Branches rebased.

        Show
        Adam Olley added a comment - Branches rebased.
        Hide
        Adam Olley added a comment -

        Hi Andrew,

        I can only request or start a peer review.

        Show
        Adam Olley added a comment - Hi Andrew, I can only request or start a peer review.
        Hide
        Andrew Davis added a comment -

        Submitting for integration.

        Show
        Andrew Davis added a comment - Submitting for integration.
        Hide
        Sam Hemelryk added a comment -

        Thanks Adam, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks Adam, this has been integrated now.
        Hide
        Damyon Wiese added a comment -

        Thanks Adam,

        Works as described.

        Show
        Damyon Wiese added a comment - Thanks Adam, Works as described.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: