Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.4, 2.4.5, 2.5.4, 2.6.1
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: Gradebook
    • Environment:
      Debian 6.0.7 64 Bits, Apache, PHP 5.3.3, MySQL 5.1.66 and Nginx frontend
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      DONT UPGRADE YET!

      On previous weekly (must not contain the fix for MDL-43306):
      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
        

      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 have created duplicates, there should be results fem the following query:

        SELECT courseid, sortorder
        FROM {grade_items}
        GROUP BY courseid, sortorder
        HAVING COUNT(id) > 1
        

      UPGRADE!

      1. VERIFY: Run the same query as above, and ensure there now are no results.
      2. Go to the Gradebook of COURSE1
      3. Go to Grades > Categories and items > Simple view and ensure that the grade items for the two assignments appear
      4. VERIFY: that you can resort the grade items and they behave in the way the interface demonstrates
      Show
      DONT UPGRADE YET! On previous weekly (must not contain the fix for MDL-43306 ): 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 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 have created duplicates, there should be results fem the following query: SELECT courseid, sortorder FROM {grade_items} GROUP BY courseid, sortorder HAVING COUNT(id) > 1 UPGRADE! VERIFY: Run the same query as above, and ensure there now are no results. Go to the Gradebook of COURSE1 Go to Grades > Categories and items > Simple view and ensure that the grade items for the two assignments appear VERIFY: that you can resort the grade items and they behave in the way the interface demonstrates
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      wip-MDL-41062-master
    • Sprint:
      BACKEND Sprint 9
    • Story Points (Obsolete):
      20
    • Sprint:
      BACKEND Sprint 9

      Description

      We are suffering an erratic behaviour while trying to sort items in the gradebook using moodle 2.4.4 or 2.4.5.
      Please check the attached video.

      If we try to put the "Final Assessmet" item at the first place it works, when we try to put it at the end of the list it does not work and that item appears in a random? position.
      The same happens when we try to put the last item in the right place and after trying several other combinations.

        Gliffy Diagrams

        1. Gradebook Problem.m4v
          2.31 MB
          Generazion Consulting S.L.
        2. mdl-41062-brokengradeitems.mbz
          15 kB
          Dan Poltawski

          Issue Links

            Activity

            Hide
            whereduck David Richards added a comment -

            I am seeing a similar problem on a Moodle v. 2.3.7 site. Cannot move gradebook items around.

            Show
            whereduck David Richards added a comment - I am seeing a similar problem on a Moodle v. 2.3.7 site. Cannot move gradebook items around.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that.

            Feel free to help by working on a solution.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that. Feel free to help by working on a solution.
            Hide
            kernel_ Ignasi added a comment -

            I'm working on Moodle 2.4 and I have the same problem. There are some items that I can't reorder.

            Show
            kernel_ Ignasi added a comment - I'm working on Moodle 2.4 and I have the same problem. There are some items that I can't reorder.
            Hide
            timhunt Tim Hunt added a comment -

            What is going on here is as follows:

            Somehow, we get two items in the gradebook with the same sortorder (and courseid, categoryid). For example:

            • Quiz 2, sortorder = 1
            • Quiz 1, sortorder = 2
            • Quiz 3, sortorder = 2

            In this case, if you try to move Quiz 2 to after Quiz 1, you actually end up with

            • Quiz 1, sortorder = 2
            • Quiz 3, sortorder = 2
            • Quiz 2, sortorder = 3

            You can 'solve' the problem if instead you move Quiz 1 to the first positions, or quiz 3 to the last position. Either of those actions will cause all the sortorders to become unique, and after that things behave the way you would expect.

            Sadly, given the way the re-order code is written (grade_item::move_after_sortorder around like 1200 of lib/grade/grade_item.php) it is not easy to fix this.

            Note there is some commented-out code at the top of grade_category::_fetch_course_tree_recursion which tries to fix the sort-order, but that really should not be there. Updateing the database in a method called fetch is just wrong. Ah, that was commented out as part of MDL-24504, 'fixed' in 2.0.

            If I were going to try to fix this, I guess I would add code to grade/edit/tree, to check for duplicate sort-orders, and if it finds them, fix them. That is still pretty evil (DB update when you just view a particular page) but it is less bad than doing it deep in the gradebook classes. However I don't really have time to work on this.

            Another key question is: where do these duplicate sortorders come from? My guess is backup and restore. The code there makes no effort to verify that sort-orders are unique. (Nor is it really feasible that it tries.) Also, not that the gradebook code suffers from race conditions, but I think the odds of them causing problems in practice is tiny.

            Ah, here is another approach we could take to try to fix this. In grade_item::update, detect duplicate sortorder, and if found, fix it. This will cost 1 DB query on each grade_item::update. Not sure if that is a good trade-off. If we did it, it might look like https://github.com/timhunt/moodle/compare/master...MDL-41062 (not tested). What do people think of that?

            I suppose that if we did a fix like that, then we should probably also do a DB upgrade to find existing duplicate sort orders, and fix them once and for all. You can find those with a query somethig like

            SELECT courseid, c.shortname, categoryid, gi.sortorder, COUNT(1)
            FROM mdl_grade_items gi
            JOIN mdl_course c ON c.id = gi.courseid
            GROUP BY courseid, c.shortname, categoryid, gi.sortorder
            HAVING COUNT(1) > 1
            ORDER BY c.shortname, COUNT(1) DESC, categoryid, gi.sortorder

            If we did that, then we could decide to be brave, and add a unique index on grade_items (courseid, sortorder) to stop this breaking again in the future.

            Show
            timhunt Tim Hunt added a comment - What is going on here is as follows: Somehow, we get two items in the gradebook with the same sortorder (and courseid, categoryid). For example: Quiz 2, sortorder = 1 Quiz 1, sortorder = 2 Quiz 3, sortorder = 2 In this case, if you try to move Quiz 2 to after Quiz 1, you actually end up with Quiz 1, sortorder = 2 Quiz 3, sortorder = 2 Quiz 2, sortorder = 3 You can 'solve' the problem if instead you move Quiz 1 to the first positions, or quiz 3 to the last position. Either of those actions will cause all the sortorders to become unique, and after that things behave the way you would expect. Sadly, given the way the re-order code is written (grade_item::move_after_sortorder around like 1200 of lib/grade/grade_item.php) it is not easy to fix this. Note there is some commented-out code at the top of grade_category::_fetch_course_tree_recursion which tries to fix the sort-order, but that really should not be there. Updateing the database in a method called fetch is just wrong. Ah, that was commented out as part of MDL-24504 , 'fixed' in 2.0. If I were going to try to fix this, I guess I would add code to grade/edit/tree, to check for duplicate sort-orders, and if it finds them, fix them. That is still pretty evil (DB update when you just view a particular page) but it is less bad than doing it deep in the gradebook classes. However I don't really have time to work on this. Another key question is: where do these duplicate sortorders come from? My guess is backup and restore. The code there makes no effort to verify that sort-orders are unique. (Nor is it really feasible that it tries.) Also, not that the gradebook code suffers from race conditions, but I think the odds of them causing problems in practice is tiny. Ah, here is another approach we could take to try to fix this. In grade_item::update, detect duplicate sortorder, and if found, fix it. This will cost 1 DB query on each grade_item::update. Not sure if that is a good trade-off. If we did it, it might look like https://github.com/timhunt/moodle/compare/master...MDL-41062 (not tested). What do people think of that? I suppose that if we did a fix like that, then we should probably also do a DB upgrade to find existing duplicate sort orders, and fix them once and for all. You can find those with a query somethig like SELECT courseid, c.shortname, categoryid, gi.sortorder, COUNT (1) FROM mdl_grade_items gi JOIN mdl_course c ON c.id = gi.courseid GROUP BY courseid, c.shortname, categoryid, gi.sortorder HAVING COUNT (1) > 1 ORDER BY c.shortname, COUNT (1) DESC , categoryid, gi.sortorder If we did that, then we could decide to be brave, and add a unique index on grade_items (courseid, sortorder) to stop this breaking again in the future.
            Hide
            poltawski Dan Poltawski added a comment -

            Whenever I see sort orders I cringe, because we seem to continually hit the exact same bugs. Just look at fix_course_sortorder() to see a really complex solution to one of these.

            Petr advises against adding unique indexes to sortorders due to concurrency problems.

            Show
            poltawski Dan Poltawski added a comment - Whenever I see sort orders I cringe, because we seem to continually hit the exact same bugs. Just look at fix_course_sortorder() to see a really complex solution to one of these. Petr advises against adding unique indexes to sortorders due to concurrency problems.
            Hide
            poltawski Dan Poltawski added a comment -

            Attaching backup file with broken grade items in the way Tim described.

            Show
            poltawski Dan Poltawski added a comment - Attaching backup file with broken grade items in the way Tim described.
            Hide
            poltawski Dan Poltawski added a comment -

            Tim, your guess was right on, I can introduce duplicate sort order grade items by simply doing import. I've added that to the testing instructions for now.

            Show
            poltawski Dan Poltawski added a comment - Tim, your guess was right on, I can introduce duplicate sort order grade items by simply doing import. I've added that to the testing instructions for now.
            Hide
            poltawski Dan Poltawski added a comment -

            The sort order problem looks to me like it has come from 4d787d8 in MDL-23362.

            I wonder if it would be better to:
            a) Do this duplicate check in restore
            b) Clean up duplicates
            c) Not do any duplicate checking in normal operation.

            Unfortunately that assumes that the problem has only come from backup/restore.

            Show
            poltawski Dan Poltawski added a comment - The sort order problem looks to me like it has come from 4d787d8 in MDL-23362 . I wonder if it would be better to: a) Do this duplicate check in restore b) Clean up duplicates c) Not do any duplicate checking in normal operation. Unfortunately that assumes that the problem has only come from backup/restore.
            Hide
            poltawski Dan Poltawski added a comment - - edited

            I have created a new issue for the creation of these duplicate items in backup MDL-43306 (and have a put a patch up for peer review).

            Show
            poltawski Dan Poltawski added a comment - - edited I have created a new issue for the creation of these duplicate items in backup MDL-43306 (and have a put a patch up for peer review).
            Hide
            poltawski Dan Poltawski added a comment -

            I've been greping around the code and I can't see any obvious places where we will be introducing duplicate sort orders so at the moment, my gut feeling is to simply clean up the duplicate sort orders. I suppose i'll prepare a patch with both and then we can see.

            Show
            poltawski Dan Poltawski added a comment - I've been greping around the code and I can't see any obvious places where we will be introducing duplicate sort orders so at the moment, my gut feeling is to simply clean up the duplicate sort orders. I suppose i'll prepare a patch with both and then we can see.
            Hide
            timhunt Tim Hunt added a comment -

            The unit test currently has a chunk of commented out code.

            In upgrade_grade_item_fix_sortorder, should you make the order used unambiguous. Either 'sortorder ASC, itemname ASC', or 'sortorder ASC, id ASC'

            Apart from that, looks plausible.

            Show
            timhunt Tim Hunt added a comment - The unit test currently has a chunk of commented out code. In upgrade_grade_item_fix_sortorder, should you make the order used unambiguous. Either 'sortorder ASC, itemname ASC', or 'sortorder ASC, id ASC' Apart from that, looks plausible.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Tim. (Its just my code pushed for backup not review ) What I would like to do is do a better job of only updating the grade items which will be affected rather than forcing a whole reorder of on every single item. But my head is not in the space for it today, so just would like to start by proving what is required and writing tests for the edge cases.

            Show
            poltawski Dan Poltawski added a comment - Thanks Tim. (Its just my code pushed for backup not review ) What I would like to do is do a better job of only updating the grade items which will be affected rather than forcing a whole reorder of on every single item. But my head is not in the space for it today, so just would like to start by proving what is required and writing tests for the edge cases.
            Hide
            poltawski Dan Poltawski added a comment -

            Ok, i've updated the patch to do it more efficiently and sending it for peer review.

            Some thoughts for the peer reviewer:
            1/ What do you think about the question of whether we should add some code to check for duplicates in normal operation? (i.e. the one query trade off Tim discusses above). I've found at least one source of the duplicates (backup) and I think that is it. But maybe we should use belt and braces at the sacrifice of performance?

            2/ Can you think of some more cases we can add to the test case to prove my upgrade script?

            (I will wait for the weekly release and approval to produce backports).

            Show
            poltawski Dan Poltawski added a comment - Ok, i've updated the patch to do it more efficiently and sending it for peer review. Some thoughts for the peer reviewer: 1/ What do you think about the question of whether we should add some code to check for duplicates in normal operation? (i.e. the one query trade off Tim discusses above). I've found at least one source of the duplicates (backup) and I think that is it. But maybe we should use belt and braces at the sacrifice of performance? 2/ Can you think of some more cases we can add to the test case to prove my upgrade script? (I will wait for the weekly release and approval to produce backports).
            Hide
            fred Frédéric Massart added a comment -

            Hi Dan,

            I think your upgrade script clearly does the work, I just have some comments here:

            1. You should bump version.php, it's better for integrators to notice it
            2. I dislike the method to create fake grade items, to me it pollutes the upgrade test class. It would be more of my liking it at least it had _sortorder in its name. Though I think the proper solution is a data generator.
            3. Isn't that a coding style to wrap SQL statements in double quotes? Not that I like it...
            4. I would suggest adding more than 2 duplicates, we could imagine a common coding error where you bump the sortorder of all of them but the first one. I'm not saying your code wouldn't work, but that unit tests proving it is better.
            5. I would also add more edges cases such as:
              • Duplicates as first records, not necessarily starting with 1
              • Missing entry between sortorders, e.g. 1 - 1 - 1 - 2 - 4 - 4 - 6 - 6
            6. In the comment "// Create fake items in course1 which need no action." you meant course 2.

            Now, about the fix suggested, my opinion is that we should prevent those duplicates rather than just fixing them once. If nothing changes in core, then there will be more duplicates and we will end up running this piece of code every now and then. As we have identified one of the places where those duplicates come from, we should try to fix them... that does not seem easy though. And if so, your testing instructions should test this part.

            That's it from me.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Dan, I think your upgrade script clearly does the work, I just have some comments here: You should bump version.php, it's better for integrators to notice it I dislike the method to create fake grade items, to me it pollutes the upgrade test class. It would be more of my liking it at least it had _sortorder in its name. Though I think the proper solution is a data generator. Isn't that a coding style to wrap SQL statements in double quotes? Not that I like it... I would suggest adding more than 2 duplicates, we could imagine a common coding error where you bump the sortorder of all of them but the first one. I'm not saying your code wouldn't work, but that unit tests proving it is better. I would also add more edges cases such as: Duplicates as first records, not necessarily starting with 1 Missing entry between sortorders, e.g. 1 - 1 - 1 - 2 - 4 - 4 - 6 - 6 In the comment "// Create fake items in course1 which need no action." you meant course 2. Now, about the fix suggested, my opinion is that we should prevent those duplicates rather than just fixing them once. If nothing changes in core, then there will be more duplicates and we will end up running this piece of code every now and then. As we have identified one of the places where those duplicates come from, we should try to fix them... that does not seem easy though. And if so, your testing instructions should test this part. That's it from me. Cheers, Fred
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Fred,

            Thanks for the review

            1. Yup, thanks
            2. I agree its a little ugly, however its less ugly than a load of manual sql statements. I also don't think I agree that a data generator is a good solution, given that i'm using it to create broken, invalid data. While many of the data generators probably allow the creation of invalid data at the moment, I don't think see that will be the case forever.
            3. I'll change it. (But for the history books. it's 'Petr style' http://docs.moodle.org/dev/index.php?title=Coding_style&diff=40908&oldid=36862, it wasn't agreed on properly and I prefer single quotes because it demonstrates there isn't variable subsitution going on.)
            4. There are actually more than two duplicates (which is a related problem to 6 ).
            5. Yep, thanks - that was what I was asking for.
            6. Yep & some outdated comments - see also point 4.

            Now, about the fix suggested, my opinion is that we should prevent those duplicates rather than just fixing them once.

            Obviously I agree.

            If nothing changes in core, then there will be more duplicates and we will end up running this piece of code every now and then. As we have identified one of the places where those duplicates come from, we should try to fix them... that does not seem easy though.

            I do not understand what you are saying here. If you read up, you'll see I have made changes in core to prevent them, see MDL-43306. The question is is that the only place, they were coming from? I've looked through the code points and I think it is.

            But what we could do is put in some code which tries even harder to prevent them like Tim's POC, at the expense of a database query per grade item modificaiton. I am not too keen on this because it would make updates slightly more expensive and its sort of lazy (we can't find whats causing it, so we just put in code to do additional work to prevent it). It also wouldn't be guaranteed to be the entry point which creates the invalid data.

            Show
            poltawski Dan Poltawski added a comment - Hi Fred, Thanks for the review 1. Yup, thanks 2. I agree its a little ugly, however its less ugly than a load of manual sql statements. I also don't think I agree that a data generator is a good solution, given that i'm using it to create broken, invalid data. While many of the data generators probably allow the creation of invalid data at the moment, I don't think see that will be the case forever. 3. I'll change it. (But for the history books. it's 'Petr style' http://docs.moodle.org/dev/index.php?title=Coding_style&diff=40908&oldid=36862 , it wasn't agreed on properly and I prefer single quotes because it demonstrates there isn't variable subsitution going on.) 4. There are actually more than two duplicates (which is a related problem to 6 ). 5. Yep, thanks - that was what I was asking for. 6. Yep & some outdated comments - see also point 4. Now, about the fix suggested, my opinion is that we should prevent those duplicates rather than just fixing them once. Obviously I agree. If nothing changes in core, then there will be more duplicates and we will end up running this piece of code every now and then. As we have identified one of the places where those duplicates come from, we should try to fix them... that does not seem easy though. I do not understand what you are saying here. If you read up, you'll see I have made changes in core to prevent them, see MDL-43306 . The question is is that the only place, they were coming from? I've looked through the code points and I think it is. But what we could do is put in some code which tries even harder to prevent them like Tim's POC, at the expense of a database query per grade item modificaiton. I am not too keen on this because it would make updates slightly more expensive and its sort of lazy (we can't find whats causing it, so we just put in code to do additional work to prevent it). It also wouldn't be guaranteed to be the entry point which creates the invalid data.
            Hide
            poltawski Dan Poltawski added a comment -

            (Just an update that your suggestions lead to me discover a horrible bug in that code - and created MDL-43395)

            Show
            poltawski Dan Poltawski added a comment - (Just an update that your suggestions lead to me discover a horrible bug in that code - and created MDL-43395 )
            Hide
            fred Frédéric Massart added a comment -

            Hi Dan,

            all good then, I missed your comment about the new issue to fix those duplicates. Feel free to push this issue for integration.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Dan, all good then, I missed your comment about the new issue to fix those duplicates. Feel free to push this issue for integration. Cheers, Fred
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Fred,

            Thanks again for the review, i've pushed some fixes (will squash for integration, just left for your review purposes).

            Firstly fixing your comments:
            https://github.com/danpoltawski/moodle/commit/91b3bb362bc58d41d728d1667ae08f059e7e89ae

            1) Fixed
            2) Changed, although looking at it again, maybe a better name is needed.
            3) Fixed
            4) Was already fixed, but comments fixed.
            5) Fixed
            6) Fixed.

            But then we also discussed a different approach for the function to avoid another query, so I implemented that and added some new assertions to ensure the upgrade function doesn't rely on id.
            https://github.com/danpoltawski/moodle/commit/34178ccf9de086eba0321cd420344ffb6d40bfac

            Look forward to your thoughts, thanks.

            Show
            poltawski Dan Poltawski added a comment - Hi Fred, Thanks again for the review, i've pushed some fixes (will squash for integration, just left for your review purposes). Firstly fixing your comments: https://github.com/danpoltawski/moodle/commit/91b3bb362bc58d41d728d1667ae08f059e7e89ae 1) Fixed 2) Changed, although looking at it again, maybe a better name is needed. 3) Fixed 4) Was already fixed, but comments fixed. 5) Fixed 6) Fixed. But then we also discussed a different approach for the function to avoid another query, so I implemented that and added some new assertions to ensure the upgrade function doesn't rely on id. https://github.com/danpoltawski/moodle/commit/34178ccf9de086eba0321cd420344ffb6d40bfac Look forward to your thoughts, thanks.
            Hide
            fred Frédéric Massart added a comment -

            Hi Dan,

            Thanks for fixing all those, and for improving the SQL query. As I was looking at this for a second time, I realised that the query (improved or not) would not work properly. If I happen to add 3 grade items, but the I reorder the first one to the last position, and this one is not duplicated, then we re-create duplicates.

            Example of data to replicate:

            id initial sortorder after upgrade expected
            10 2 2 3
            20 1 1 1
            30 1 2 2

            diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php
            index 2cd498f..0764203 100644
            --- a/lib/tests/upgradelib_test.php
            +++ b/lib/tests/upgradelib_test.php
            @@ -85,15 +85,19 @@ class core_upgradelib_testcase extends advanced_testcase {
                     $course4item[0] = $this->insert_fake_grade_item_sortorder($course4->id, 3);
                     $course4item[1] = $this->insert_fake_grade_item_sortorder($course4->id, 3);
             
            +        // Pretent that this item was added early, but ordered down afterwards.
            +        $course4item[7] = $this->insert_fake_grade_item_sortorder($course4->id, 7);
            +
                     $course4item[2] = $this->insert_fake_grade_item_sortorder($course4->id, 5);
                     $course4item[3] = $this->insert_fake_grade_item_sortorder($course4->id, 6);
                     $course4item[4] = $this->insert_fake_grade_item_sortorder($course4->id, 6);
             
                     $course4item[5] = $this->insert_fake_grade_item_sortorder($course4->id, 9);
                     $course4item[6] = $this->insert_fake_grade_item_sortorder($course4->id, 10);
            +
                     // Create some items with non-sequential id and sortorder relationship.
            -        $course4item[7] = $this->insert_fake_grade_item_sortorder($course4->id, 7);
                     $course4item[8] = $this->insert_fake_grade_item_sortorder($course4->id, 8);
            +        $course4item[9] = $this->insert_fake_grade_item_sortorder($course4->id, 3);
             
                     $duplicatedetectionsql = "SELECT courseid, sortorder
                                                 FROM {grade_items}
            

            Also, I have found that the select query returns more than 1 row with the same ID if there are more than 1 duplicate, I found that by switching to get_records_sql(). I suggest adding more than 2 duplicates in the Unit Tests.

            On a side note, I found it very difficult to check what your Unit Tests were doing. Would it be possible to make some real assertion on real IDs and sortorders? Or at least on an array of expected sortorder? Maybe using get_records_menu() or something like that.

            I hope I made sense.

            Many thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Dan, Thanks for fixing all those, and for improving the SQL query. As I was looking at this for a second time, I realised that the query (improved or not) would not work properly. If I happen to add 3 grade items, but the I reorder the first one to the last position, and this one is not duplicated, then we re-create duplicates. Example of data to replicate: id initial sortorder after upgrade expected 10 2 2 3 20 1 1 1 30 1 2 2 diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php index 2cd498f..0764203 100644 --- a/lib/tests/upgradelib_test.php +++ b/lib/tests/upgradelib_test.php @@ -85,15 +85,19 @@ class core_upgradelib_testcase extends advanced_testcase { $course4item[0] = $this->insert_fake_grade_item_sortorder($course4->id, 3); $course4item[1] = $this->insert_fake_grade_item_sortorder($course4->id, 3); + // Pretent that this item was added early, but ordered down afterwards. + $course4item[7] = $this->insert_fake_grade_item_sortorder($course4->id, 7); + $course4item[2] = $this->insert_fake_grade_item_sortorder($course4->id, 5); $course4item[3] = $this->insert_fake_grade_item_sortorder($course4->id, 6); $course4item[4] = $this->insert_fake_grade_item_sortorder($course4->id, 6); $course4item[5] = $this->insert_fake_grade_item_sortorder($course4->id, 9); $course4item[6] = $this->insert_fake_grade_item_sortorder($course4->id, 10); + // Create some items with non-sequential id and sortorder relationship. - $course4item[7] = $this->insert_fake_grade_item_sortorder($course4->id, 7); $course4item[8] = $this->insert_fake_grade_item_sortorder($course4->id, 8); + $course4item[9] = $this->insert_fake_grade_item_sortorder($course4->id, 3); $duplicatedetectionsql = "SELECT courseid, sortorder FROM {grade_items} Also, I have found that the select query returns more than 1 row with the same ID if there are more than 1 duplicate, I found that by switching to get_records_sql() . I suggest adding more than 2 duplicates in the Unit Tests. On a side note, I found it very difficult to check what your Unit Tests were doing. Would it be possible to make some real assertion on real IDs and sortorders? Or at least on an array of expected sortorder? Maybe using get_records_menu() or something like that. I hope I made sense. Many thanks! Fred
            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
            poltawski Dan Poltawski added a comment -

            Unassigning this issue from me because this wee i'm on integration and the sprint continues without me.

            To whoever picks up this issue, i'm really sorry! I do not feel good at all about passing this buck on in a half finished state. Feel free to talk to me about the issue.

            Show
            poltawski Dan Poltawski added a comment - Unassigning this issue from me because this wee i'm on integration and the sprint continues without me. To whoever picks up this issue, i'm really sorry! I do not feel good at all about passing this buck on in a half finished state. Feel free to talk to me about the issue.
            Hide
            poltawski Dan Poltawski added a comment -

            Just one comment in response to Fred (apart from thanks very much for discovering a problem!).

            On a side note, I found it very difficult to check what your Unit Tests were doing. Would it be possible to make some real assertion on real IDs and sortorders? Or at least on an array of expected sortorder? Maybe using get_records_menu() or something like that.

            I agree its kind of difficult to understand - although because there are so many different pieces of information - I am not really sure there is any good way to make it particularly understandable. Although perhaps verifying real sort orders might be an idea. I think using greater than might be making the test code a bit too clever.

            Show
            poltawski Dan Poltawski added a comment - Just one comment in response to Fred (apart from thanks very much for discovering a problem!). On a side note, I found it very difficult to check what your Unit Tests were doing. Would it be possible to make some real assertion on real IDs and sortorders? Or at least on an array of expected sortorder? Maybe using get_records_menu() or something like that. I agree its kind of difficult to understand - although because there are so many different pieces of information - I am not really sure there is any good way to make it particularly understandable. Although perhaps verifying real sort orders might be an idea. I think using greater than might be making the test code a bit too clever.
            Hide
            timhunt Tim Hunt added a comment -

            Please try to avoid asserting on assumed database ids. Moodle core often does this, e.g. with context ids, and it breaks with our code. That is because we have add-ons which create things as part of their install. E.g. oublog which creates a special hidden course when it is installed, or something else that automatically creates a support user on install. So, there were merits in what Dan was doing.

            My approach would be to improving the readability of the tests would be to make a helper function with a clear name, like assert_comes_before_in_sort_order($grade1, $grade2)

            Show
            timhunt Tim Hunt added a comment - Please try to avoid asserting on assumed database ids. Moodle core often does this, e.g. with context ids, and it breaks with our code. That is because we have add-ons which create things as part of their install. E.g. oublog which creates a special hidden course when it is installed, or something else that automatically creates a support user on install. So, there were merits in what Dan was doing. My approach would be to improving the readability of the tests would be to make a helper function with a clear name, like assert_comes_before_in_sort_order($grade1, $grade2)
            Hide
            marina Marina Glancy added a comment -

            I picked the issue from Dan. Fred (or maybe Dan) can you review this solution please?

            Show
            marina Marina Glancy added a comment - I picked the issue from Dan. Fred (or maybe Dan) can you review this solution please?
            Hide
            marina Marina Glancy added a comment -

            Tim, this particular test tests just one upgrade function that does not delete or insert records (and therefore does not change ids). So we can be sure here that ids are not changed. But generally I agree with you, we should not expect database id to remain unchanged.

            Show
            marina Marina Glancy added a comment - Tim, this particular test tests just one upgrade function that does not delete or insert records (and therefore does not change ids). So we can be sure here that ids are not changed. But generally I agree with you, we should not expect database id to remain unchanged.
            Hide
            marina Marina Glancy added a comment -

            I added another commit re-writing the unittest, please tell me if it is more readable so I can replace Dan's original test.

            Show
            marina Marina Glancy added a comment - I added another commit re-writing the unittest, please tell me if it is more readable so I can replace Dan's original test.
            Hide
            fred Frédéric Massart added a comment - - edited

            Hi Marina,

            thanks for finishing that up. I only had a look at the commits you added and they seem good to me. I personally prefer your version of the test, so I'd be fine keeping it.

            On a minor note, the commit history can probably be re-written a bit as it does not make much sense. I also find that your commit messages could be more explicit too.

            Before you push for peer review you could perhaps add some testing instructions which mess up with the IDs, such as placing in first position a grade item that was added last or something (if possible).

            Cheers,
            Fred

            PS: There are 3 successive blank lines in the test method.
            Edit: Could you add Unit Tests with more than 1 duplicate? Just in case .

            Show
            fred Frédéric Massart added a comment - - edited Hi Marina, thanks for finishing that up. I only had a look at the commits you added and they seem good to me. I personally prefer your version of the test, so I'd be fine keeping it. On a minor note, the commit history can probably be re-written a bit as it does not make much sense. I also find that your commit messages could be more explicit too. Before you push for peer review you could perhaps add some testing instructions which mess up with the IDs, such as placing in first position a grade item that was added last or something (if possible). Cheers, Fred PS: There are 3 successive blank lines in the test method. Edit: Could you add Unit Tests with more than 1 duplicate? Just in case .
            Hide
            timhunt Tim Hunt added a comment -

            The new unit test starts well. The set-up is very clear. But at this point: https://github.com/marinaglancy/moodle/commit/892dacf7253c2108c31a2cfb888985e22c0ec8c3#diff-ec328f1a30e59aed57fc4f0555f82f44R111 it all becomes deeply mystifying, so I am not sure it is a win.

            Show
            timhunt Tim Hunt added a comment - The new unit test starts well. The set-up is very clear. But at this point: https://github.com/marinaglancy/moodle/commit/892dacf7253c2108c31a2cfb888985e22c0ec8c3#diff-ec328f1a30e59aed57fc4f0555f82f44R111 it all becomes deeply mystifying, so I am not sure it is a win.
            Hide
            marina Marina Glancy added a comment -

            How about adding a comment for the unittest:
            The purpose of this test is to make sure that after upgrade script there is no duplicates in the field grade_items.sortorder (for each course) and the result of query "SELECT id FROM grade_items WHERE courseid=? ORDER BY sortorder, id" does not change.

            Show
            marina Marina Glancy added a comment - How about adding a comment for the unittest: The purpose of this test is to make sure that after upgrade script there is no duplicates in the field grade_items.sortorder (for each course) and the result of query "SELECT id FROM grade_items WHERE courseid=? ORDER BY sortorder, id" does not change.
            Hide
            marina Marina Glancy added a comment -

            I added this comment and also squashed multiple commits of the same author.

            TO INTEGRATOR: Please note that there is a blocker MDL-43306

            Show
            marina Marina Glancy added a comment - I added this comment and also squashed multiple commits of the same author. TO INTEGRATOR: Please note that there is a blocker MDL-43306
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Marina,

            I really don't like adding {$CFG->gradeitemsortorderfixed}. I know we have introduced that hack elsewhere for the course fixes because those are expensive upgrade steps, but I am not sure its justified here. We only run one query per broken record when they are broken, when everything is OK its only a single query. Now that query will not be as fast as we expect on interactive pages but for upgrade I think it should be fine. Have you got some particular evidence this is an especially slow query to run?

            (Really if we re going to keep introducing all these $CFG->upgradestepx steps config values we should actually implement a proper solution to it rather than polluting $CFG).

            Show
            poltawski Dan Poltawski added a comment - Hi Marina, I really don't like adding {$CFG->gradeitemsortorderfixed}. I know we have introduced that hack elsewhere for the course fixes because those are expensive upgrade steps, but I am not sure its justified here. We only run one query per broken record when they are broken, when everything is OK its only a single query. Now that query will not be as fast as we expect on interactive pages but for upgrade I think it should be fine. Have you got some particular evidence this is an especially slow query to run? (Really if we re going to keep introducing all these $CFG->upgradestepx steps config values we should actually implement a proper solution to it rather than polluting $CFG).
            Hide
            marina Marina Glancy added a comment -

            Dan, I can remove it if you are sure that the query that looks for duplicates will run fast on big databases.
            From my point of view there is not much difference from the similar query joining two course_module tables (see MDL-38228) because grade_items has very big size - every graded module instance has an entry there. I can test the performance tonight at home, my big db that I use for testing is on another computer.

            Show
            marina Marina Glancy added a comment - Dan, I can remove it if you are sure that the query that looks for duplicates will run fast on big databases. From my point of view there is not much difference from the similar query joining two course_module tables (see MDL-38228 ) because grade_items has very big size - every graded module instance has an entry there. I can test the performance tonight at home, my big db that I use for testing is on another computer.
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Marina,

            I am not certain, I don't have a big dataset to hand myself. An postgres explain indicates its not too costly, but its always hard to tell when you don't have decent dataset.

            But from that linked issue, the first of the queries is relatively similar (though has string manipulations too) and was 711ms on the OU dataset and 2.22 seconds from Moodlerooms on mysql. I think that is more than fast enough for upgrade code.

            Even if this query took 10mins, we need to run it at least once (probably minor release for people) and the second run of it will be a major upgrade - which should be expected to be slow anyway. I think its a bit premature to add the flag.

            Show
            poltawski Dan Poltawski added a comment - Hi Marina, I am not certain , I don't have a big dataset to hand myself. An postgres explain indicates its not too costly, but its always hard to tell when you don't have decent dataset. But from that linked issue, the first of the queries is relatively similar (though has string manipulations too) and was 711ms on the OU dataset and 2.22 seconds from Moodlerooms on mysql. I think that is more than fast enough for upgrade code. Even if this query took 10mins, we need to run it at least once (probably minor release for people) and the second run of it will be a major upgrade - which should be expected to be slow anyway. I think its a bit premature to add the flag.
            Hide
            marina Marina Glancy added a comment -

            ok, agree. I removed the cfg and squashed commits

            Show
            marina Marina Glancy added a comment - ok, agree. I removed the cfg and squashed commits
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks Marina - integrated to master, 26 & 25

            Show
            poltawski Dan Poltawski added a comment - Thanks Marina - integrated to master, 26 & 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!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            WTF is this? The main query is clearly a pseudo-full-cross-join! Every record of "g1" is being returned N times (basically, its number of duplicates,). And that can be huge!

            Simple example:

            id    courseid    sortorder
            1     2           1
            2     2           1
            3     2           1
            4     2           1
            

            Expected # of returned rows: 4, ending with sortorders 1, 2, 3, 4.
            Current # of returned rows: 12 (every record is returned 3 times), ending with sortorders 1, 4, 7, 10.

            Now imagine we are talking about thousands of dupes and you'll get an idea of the number of updates performed in excess later in the loop (and they are not "cheap" updates, given the conditions it uses).

            The query needs to be reworked to only return every tuple once (COUNT+HAVING, SUBSELECT or DISTINCT). And try it against BIG dataset.

            Also, it seems not optimal to be moving all the record sortorders 1 by 1 (when surely it could be done by the number of duplicates found, by iterating by sortorder instead of doing it by id. But I've not looked to that deeply, just noting it).

            Creating a new issue...done @ MDL-43946

            Ciao

            PS: Re-reading the issue, it seems that Fred already reported the dupes happening, grrr.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited WTF is this? The main query is clearly a pseudo-full-cross-join! Every record of "g1" is being returned N times (basically, its number of duplicates,). And that can be huge! Simple example: id courseid sortorder 1 2 1 2 2 1 3 2 1 4 2 1 Expected # of returned rows: 4, ending with sortorders 1, 2, 3, 4. Current # of returned rows: 12 (every record is returned 3 times), ending with sortorders 1, 4, 7, 10. Now imagine we are talking about thousands of dupes and you'll get an idea of the number of updates performed in excess later in the loop (and they are not "cheap" updates, given the conditions it uses). The query needs to be reworked to only return every tuple once (COUNT+HAVING, SUBSELECT or DISTINCT). And try it against BIG dataset. Also, it seems not optimal to be moving all the record sortorders 1 by 1 (when surely it could be done by the number of duplicates found, by iterating by sortorder instead of doing it by id. But I've not looked to that deeply, just noting it). Creating a new issue...done @ MDL-43946 Ciao PS: Re-reading the issue, it seems that Fred already reported the dupes happening, grrr.

              People

              • Votes:
                4 Vote for this issue
                Watchers:
                14 Start watching this issue

                Dates

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

                  Agile