Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43306

Backup introduces duplicate gradeitem sortorders when restoring

    Details

    • Testing Instructions:
      Hide

      Setup:

      1. Create COURSE1 with default settings
      2. Add an assignment 'Assign 1' with default settings
      3. Create COURSE2 with default settings
      4. Add an assignment 'Assign 2' with default settings
      5. VERIFY: Examine the database and ensure the grade items exist, and both have the same sort order in different courses. e.g:

        SELECT id, itemname, courseid, sortorder 
        FROM mdl_grade_items 
        WHERE itemname LIKE 'Assign%'
        ORDER BY sortorder, courseid;
         
         id | itemname | courseid | sortorder
        ----+----------+----------+-----------
          4 | Assign 1 |        2 |         2
          2 | Assign 2 |        3 |         2
        

      Attempt to create duplicates through import:

      1. Go to COURSE1
      2. Select import, choose COURSE2
      3. Choose ONLY to import ASSIGNCOURSE2
      4. VERIFY: Examine the database and ensure the grade items that are imported don't have duplicate sortorder and courseids:

        SELECT id, itemname, courseid, sortorder 
        FROM mdl_grade_items 
        WHERE itemname like 'Assign%' 
        ORDER BY sortorder, courseid
        ;
         id | itemname | courseid | sortorder
        ----+----------+----------+-----------
          4 | Assign 1 |        2 |         2
          2 | Assign 2 |        3 |         2
          7 | Assign 2 |        2 |         4
        (3 rows)
        

      5. Go to Grades > Categories and items > Simple view and ensure that the grade items for the two assignments appear

      Backup/Restore test:

      1. Backup a course with numerous activities and grade items
      2. VERIFY: it restores without problem.

      phpunit:
      run phpunit lib/grade/tests/grade_item_test.php

      Show
      Setup: Create COURSE1 with default settings Add an assignment 'Assign 1' with default settings Create COURSE2 with default settings Add an assignment 'Assign 2' with default settings VERIFY: Examine the database and ensure the grade items exist, and both have the same sort order in different courses. e.g: SELECT id, itemname, courseid, sortorder FROM mdl_grade_items WHERE itemname LIKE 'Assign%' ORDER BY sortorder, courseid;   id | itemname | courseid | sortorder ----+----------+----------+----------- 4 | Assign 1 | 2 | 2 2 | Assign 2 | 3 | 2 Attempt to create duplicates through import: Go to COURSE1 Select import, choose COURSE2 Choose ONLY to import ASSIGNCOURSE2 VERIFY: Examine the database and ensure the grade items that are imported don't have duplicate sortorder and courseids: SELECT id, itemname, courseid, sortorder FROM mdl_grade_items WHERE itemname like 'Assign%' ORDER BY sortorder, courseid ; id | itemname | courseid | sortorder ----+----------+----------+----------- 4 | Assign 1 | 2 | 2 2 | Assign 2 | 3 | 2 7 | Assign 2 | 2 | 4 (3 rows) Go to Grades > Categories and items > Simple view and ensure that the grade items for the two assignments appear Backup/Restore test: Backup a course with numerous activities and grade items VERIFY: it restores without problem. phpunit: run phpunit lib/grade/tests/grade_item_test.php
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      wip-mdl-43306-m
    • Sprint:
      BACKEND Sprint 9
    • Story Points (Obsolete):
      13
    • Sprint:
      BACKEND Sprint 9

      Description

      As discovered in MDL-41062, restore creates duplciate sort orders when restoring grade items. This can result in inconsistent sorting of items.

      Steps to reproduce:

      Setup:

      1. Create COURSE1 with default settings
      2. Add an assignment ASSIGNCOURSE1 with default settings
      3. Create COURSE2 with default settings
      4. Add an assignment ASSIGNCOURSE2 with default settings

      Create duplicates:

      1. Go to COURSE1
      2. Select import, choose COURSE2 and choose to import ASSIGNCOURSE2

      Examine the database and look at the sort order of the grade items. Duplicates should not be seen in the same course.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

            Requesting peer review. I chose to solve this problem using the existing API function, which deals with duplicate sort orders as I think this is the simplest solution which should result in the order being preserved properly.

            Show
            poltawski Dan Poltawski added a comment - Requesting peer review. I chose to solve this problem using the existing API function, which deals with duplicate sort orders as I think this is the simplest solution which should result in the order being preserved properly.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            NOTE: In MDL-41062 i'm going to look at tidying up the duplicate items, and handling sorting when we've got problems there.

            Show
            poltawski Dan Poltawski added a comment - - edited NOTE: In MDL-41062 i'm going to look at tidying up the duplicate items, and handling sorting when we've got problems there.
            Hide
            timhunt Tim Hunt added a comment -

            This basically looks good, but ...

            Suppose you start with an already screwed-up course where the grade-items are like:

            Assignment 1 | 2
            Assignment 2 | 2
            Assignment 3 | 2
            Assignment 4 | 3

            and then back it up and restore it. You are going to end up with something like

            Assignment 1 | 2
            Assignment 2 | 3
            Assignment 4 | 4
            Assignment 3 | 5

            (In fact, I don't think you will get exactly that. I don't understand why, in the testing instructions with just two activities, the sort-orders end up as 2 and 4, so move_after_sortorder must be doing something weird, but I hope my example is good enough for you to get the point of what I mean.)

            Do we care about this? We could decide that we don't care, and your simple fix is sufficient, or we could decide we have to try harder. What do you think?

            Show
            timhunt Tim Hunt added a comment - This basically looks good, but ... Suppose you start with an already screwed-up course where the grade-items are like: Assignment 1 | 2 Assignment 2 | 2 Assignment 3 | 2 Assignment 4 | 3 and then back it up and restore it. You are going to end up with something like Assignment 1 | 2 Assignment 2 | 3 Assignment 4 | 4 Assignment 3 | 5 (In fact, I don't think you will get exactly that. I don't understand why, in the testing instructions with just two activities, the sort-orders end up as 2 and 4, so move_after_sortorder must be doing something weird, but I hope my example is good enough for you to get the point of what I mean.) Do we care about this? We could decide that we don't care, and your simple fix is sufficient, or we could decide we have to try harder. What do you think?
            Hide
            poltawski Dan Poltawski added a comment -

            so move_after_sortorder must be doing something weird,

            Yeah, I chose to ignore that for the purposes of this issue (was going to look further in MDL-41062).

            And to your scenario:
            1/ I plan to clean up the sort orders across all the courses in the site in MDL-41062. Which will at least help a little.
            2/ I just don't think there is a good solution in the case that we've got multiple broken sort orders anyway. So, in the interests of simplicity I think it is best to ignore it.

            Show
            poltawski Dan Poltawski added a comment - so move_after_sortorder must be doing something weird, Yeah, I chose to ignore that for the purposes of this issue (was going to look further in MDL-41062 ). And to your scenario: 1/ I plan to clean up the sort orders across all the courses in the site in MDL-41062 . Which will at least help a little. 2/ I just don't think there is a good solution in the case that we've got multiple broken sort orders anyway. So, in the interests of simplicity I think it is best to ignore it.
            Hide
            timhunt Tim Hunt added a comment -

            OK. Fine.

            So, that just leaves one final point, what about a unit test

            Show
            timhunt Tim Hunt added a comment - OK. Fine. So, that just leaves one final point, what about a unit test
            Hide
            poltawski Dan Poltawski added a comment -

            Good question. Looking immediately there isn't a good place to insert a test for this.

            Show
            poltawski Dan Poltawski added a comment - Good question. Looking immediately there isn't a good place to insert a test for this.
            Hide
            timhunt Tim Hunt added a comment -

            Indeed. These parts of backup and restore are pretty un-testable, which is a pity. Submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Indeed. These parts of backup and restore are pretty un-testable, which is a pity. Submitting for integration.
            Hide
            poltawski Dan Poltawski added a comment -

            Petr has commented that he thinks that we should not backup the sort order field and instead store these things by selecting by sort order. Then in restore code always add the sort order in the order of the xml in the backup.

            That makes sense to me as a general rule, however its much more involved than the current solution and it also doesn't solve current backup files, so again i'm intentionally choosing not to do that. But I agree this is how we should design backup code.

            Show
            poltawski Dan Poltawski added a comment - Petr has commented that he thinks that we should not backup the sort order field and instead store these things by selecting by sort order. Then in restore code always add the sort order in the order of the xml in the backup. That makes sense to me as a general rule, however its much more involved than the current solution and it also doesn't solve current backup files, so again i'm intentionally choosing not to do that. But I agree this is how we should design backup code.
            Hide
            timhunt Tim Hunt added a comment -

            Yes. Though you have to consider the case where you duplicate one activity in a course. In that case, adding the duplicate next to the existing grade-item is probably natural. Other 'import into existing course' scenarios are more complex, but we should still give some though to doing something sensible by default. Anyway, as you say, that is something for another day.

            Show
            timhunt Tim Hunt added a comment - Yes. Though you have to consider the case where you duplicate one activity in a course. In that case, adding the duplicate next to the existing grade-item is probably natural. Other 'import into existing course' scenarios are more complex, but we should still give some though to doing something sensible by default. Anyway, as you say, that is something for another day.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (24, 25, 26 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (24, 25, 26 & master), thanks!
            Hide
            skodak Petr Skoda added a comment -

            Hmm, I just tried this and the order of grade items after full backup/restore is different. No idea why exactly but after discussing with Dan I think there might be a bit different solution to this problem:

            1/ when backing up the grade_items we should use ORDER BY sortorder, id - to make sure the items are created in correct order (not sure how much that helps though, but we do it elsewhere)
            2/ when restoring ignore any duplicates when adding any grade_items (=== keep previous behaviour)
            3/ at the end of restore do a full rebuild of grade_item sortorders in affected course

            Show
            skodak Petr Skoda added a comment - Hmm, I just tried this and the order of grade items after full backup/restore is different. No idea why exactly but after discussing with Dan I think there might be a bit different solution to this problem: 1/ when backing up the grade_items we should use ORDER BY sortorder, id - to make sure the items are created in correct order (not sure how much that helps though, but we do it elsewhere) 2/ when restoring ignore any duplicates when adding any grade_items (=== keep previous behaviour) 3/ at the end of restore do a full rebuild of grade_item sortorders in affected course
            Hide
            poltawski Dan Poltawski added a comment -

            I don't think i'm going to have a chance to work on a patch for this today, so should be reverted if i'm blocking.

            Show
            poltawski Dan Poltawski added a comment - I don't think i'm going to have a chance to work on a patch for this today, so should be reverted if i'm blocking.
            Hide
            skodak Petr Skoda added a comment -

            Confirming that after reverting the commit my 2.6 backup file was restored into new course with previous (correct) order of grade items.

            Show
            skodak Petr Skoda added a comment - Confirming that after reverting the commit my 2.6 backup file was restored into new course with previous (correct) order of grade items.
            Hide
            poltawski Dan Poltawski added a comment -

            Eloy: what do you think of Petr's proposed approach?

            Show
            poltawski Dan Poltawski added a comment - Eloy: what do you think of Petr's proposed approach?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            1) Surely exporting in order has sense, so restore happens also in order.

            2) What I'm not sure is why move_after_sortorder() does not its job, it should maintain the original order between the restored grade_items (specially if we guarantee point 1), no matter if there are other activities in the course or no, the relative order between restored should work ok. Or perhaps no, because we are comparing original sortorders with new ones assigned on grade_item creation/move.

            Uhm.. uhm... I think that's the key, how do we compare sortorders. Because, or I'm wrong or Petr's approach will expose the same problem (2 collections of sortorder mixed). Does "full rebuild of grade_item sortorders" guarantee that relative orders for the restored grade_items are going to be observed always.

            So, summary:

            1) +1 to always export grade items in order.
            2) +1 to do any reorder on restore, as far as it guarantees the relative ordering of all the restored grade items is kept. Perhaps we don't need to re-order everything at all, and 1) will provide us enough sorting by "adding at the end". Really I don't think there is a sorting strategy making happy to everybody, so I'm happy with any guaranteeing "the relative ordering of all the restored grade items is kept". For your consideration if grade_item->insert() is enough, or we need to resort at the end (although again, that's mixing 2 different lists of sortorders).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - 1) Surely exporting in order has sense, so restore happens also in order. 2) What I'm not sure is why move_after_sortorder() does not its job, it should maintain the original order between the restored grade_items (specially if we guarantee point 1), no matter if there are other activities in the course or no, the relative order between restored should work ok. Or perhaps no, because we are comparing original sortorders with new ones assigned on grade_item creation/move. Uhm.. uhm... I think that's the key, how do we compare sortorders. Because, or I'm wrong or Petr's approach will expose the same problem (2 collections of sortorder mixed). Does "full rebuild of grade_item sortorders" guarantee that relative orders for the restored grade_items are going to be observed always. So, summary: 1) +1 to always export grade items in order. 2) +1 to do any reorder on restore, as far as it guarantees the relative ordering of all the restored grade items is kept. Perhaps we don't need to re-order everything at all, and 1) will provide us enough sorting by "adding at the end". Really I don't think there is a sorting strategy making happy to everybody, so I'm happy with any guaranteeing "the relative ordering of all the restored grade items is kept". For your consideration if grade_item->insert() is enough, or we need to resort at the end (although again, that's mixing 2 different lists of sortorders). Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I'm moving this out from current integration by reverting it. Let's continue next week. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I'm moving this out from current integration by reverting it. Let's continue next week. Ciao
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            poltawski Dan Poltawski added a comment -

            As this is in the sprint and i'm on integration, i'm sending this back to unassigned so someone else can take it - as requested.

            Show
            poltawski Dan Poltawski added a comment - As this is in the sprint and i'm on integration, i'm sending this back to unassigned so someone else can take it - as requested.
            Hide
            salvetore Michael de Raadt added a comment -

            Carrying over to next sprint.

            Show
            salvetore Michael de Raadt added a comment - Carrying over to next sprint.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Taking this issue, as Dan is busy with integration this week.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Taking this issue, as Dan is busy with integration this week.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            There are multiple ways to fix this issue,

            1. As done by Dan to place grade_item to incremented sortorder.
            2. As proposed by Petr, to sort duplicated grade_items after the restore.

            I have fixed Dan's patch to increment sortorder, only if there exists a grade_item at original sortorder else use original sortorder. This approach seems better as code is contained in the restore step and no hacking/fixing logic is applied outside restore step scope.

            NOTE: Proposal by Petr seems nice as well, but if we plan to go that way then MDL-41062 should create a general api and not upgrade specific api to fix sortorder.

            Will create backports after review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - There are multiple ways to fix this issue, As done by Dan to place grade_item to incremented sortorder. As proposed by Petr, to sort duplicated grade_items after the restore. I have fixed Dan's patch to increment sortorder, only if there exists a grade_item at original sortorder else use original sortorder. This approach seems better as code is contained in the restore step and no hacking/fixing logic is applied outside restore step scope. NOTE: Proposal by Petr seems nice as well, but if we plan to go that way then MDL-41062 should create a general api and not upgrade specific api to fix sortorder. Will create backports after review.
            Hide
            marina Marina Glancy added a comment -

            Raj, sorry but this solution does not look good.
            Imagine if we already have sortorders 1,2,3,4 in the db and we insert items with sortorders 1,2,3,4,5. After each individual insert the existing sortorders will be incremented. And as a result we will insert all 5 new items after the first item in the original list.

            We should either:

            • restore all new items in the end
              or
            • restore them as we do now and fix sortorder duplicates afterwards.
            Show
            marina Marina Glancy added a comment - Raj, sorry but this solution does not look good. Imagine if we already have sortorders 1,2,3,4 in the db and we insert items with sortorders 1,2,3,4,5. After each individual insert the existing sortorders will be incremented. And as a result we will insert all 5 new items after the first item in the original list. We should either: restore all new items in the end or restore them as we do now and fix sortorder duplicates afterwards.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for looking at this Marina,

            I should have thought about that.

            I am going with second option and not the first one (restore all new items in the end) as this might not be expected/obvious result after duplicating the activity.

            I have used patch from MDL-41062 to create grade_item::fix_duplicate_sortorder() and calling this after restore is finished.

            Can you please have a look at this again.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for looking at this Marina, I should have thought about that. I am going with second option and not the first one (restore all new items in the end) as this might not be expected/obvious result after duplicating the activity. I have used patch from MDL-41062 to create grade_item::fix_duplicate_sortorder() and calling this after restore is finished. Can you please have a look at this again.
            Hide
            marina Marina Glancy added a comment -

            looks good Raj, thanks. Submitting for integration

            Show
            marina Marina Glancy added a comment - looks good Raj, thanks. Submitting for integration
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Raj - integrated to master, 26 and 25

            Show
            poltawski Dan Poltawski added a comment - Thanks Raj - integrated to master, 26 and 25
            Hide
            skodak Petr Skoda added a comment -

            seems to work fine for me, thanks

            Show
            skodak Petr Skoda added a comment - seems to work fine for me, thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I won't be saying "Thanks!" this week. I'm tired of it.

            For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely.

            Just closing this as fixed, ciao

            PS: Just a bit of black/cruel humor, sorry, LOL!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I won't be saying "Thanks!" this week. I'm tired of it. For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely. Just closing this as fixed, ciao PS: Just a bit of black/cruel humor, sorry, LOL!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Mar/14

                  Agile