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

Incorrect sorting on date for full report on recent activity

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.3, 2.4.3, 2.5
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Reports
    • Testing Instructions:
      Hide
      1. Goto a course with multiple activites, multiple forum discussions and posts, multiple assignment submissions.
      2. Add recent activity block
      3. Click on "full report" link on the block
      4. Sort by date both asc and desc and verify everything is sorted properly.
      5. Check the above holds true for different time window.
      Show
      Goto a course with multiple activites, multiple forum discussions and posts, multiple assignment submissions. Add recent activity block Click on "full report" link on the block Sort by date both asc and desc and verify everything is sorted properly. Check the above holds true for different time window.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36872-master

      Description

      As illustrated on moodle.org: http://moodle.org/course/recent.php?id=5

      See https://moodle.org/mod/forum/discuss.php?d=210492 for a screenshot illustrating the problem.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            dougiamas Martin Dougiamas added a comment -

            Increasing priority because I think it's so easy to fix.

            Show
            dougiamas Martin Dougiamas added a comment - Increasing priority because I think it's so easy to fix.
            Hide
            poltawski Dan Poltawski added a comment -

            Spent a bit of time trying to work out whats going on.. i'm not sure.

            Show
            poltawski Dan Poltawski added a comment - Spent a bit of time trying to work out whats going on.. i'm not sure.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            It seems to me that the logic in the callback functions are flawed. Consider the following piece of code:-

                if ((!array_key_exists('timestamp', $a)) or (!array_key_exists('timestamp', $b))) {
                  return 0;
                }

            And consider 3 activity x, y ,z with timestamp 1, null,3 respectively. Using y as the pivot, we find
            y = x
            y = z

            Which basically implies x, y, z can be in any order.

            From Usort manual
            "If two members compare as equal, their relative order in the sorted array is undefined."

            Thus the result of sorting is quite random.

            Show
            ankit_frenz Ankit Agarwal added a comment - It seems to me that the logic in the callback functions are flawed. Consider the following piece of code:- if ((!array_key_exists('timestamp', $a)) or (!array_key_exists('timestamp', $b))) { return 0; } And consider 3 activity x, y ,z with timestamp 1, null,3 respectively. Using y as the pivot, we find y = x y = z Which basically implies x, y, z can be in any order. From Usort manual "If two members compare as equal, their relative order in the sorted array is undefined." Thus the result of sorting is quite random.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Treating entries without timestamp as if they have a timestamp of 0 should fix this issue. Proposing a patch for the same.

            Show
            ankit_frenz Ankit Agarwal added a comment - Treating entries without timestamp as if they have a timestamp of 0 should fix this issue. Proposing a patch for the same.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Ankit,

            The patch looks great.

            [y] Syntax
            [y] Output
            [y] Whitespace
            [-] Language
            [-] Databases
            [y] Testing
            [-] Security
            [-] Documentation
            [y] Git
            [y] Sanity check

            Make sure to backport this patch before submitting for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Ankit, The patch looks great. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check Make sure to backport this patch before submitting for integration review.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Thanks for the review Rosie.
            Sending for integration.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Thanks for the review Rosie. Sending for integration. Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi Ankit,

            the patch looks perfect... just guessing if you could, alternatively, one of:

            1) Provide some manual testing steps in order to force the faulty situation (some timestamp is null), by ccessing to the DB and updating it or picking some activity with nulls.
            2) Or add some phpunit tests with the faulty situation and verifying now results are constant.

            (surely #2 is better, although I'm not sure if that function is testable as far as it's a mix of global scope + functions, grrr)

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi Ankit, the patch looks perfect... just guessing if you could, alternatively, one of: 1) Provide some manual testing steps in order to force the faulty situation (some timestamp is null), by ccessing to the DB and updating it or picking some activity with nulls. 2) Or add some phpunit tests with the faulty situation and verifying now results are constant. (surely #2 is better, although I'm not sure if that function is testable as far as it's a mix of global scope + functions, grrr) Ciao
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Hi Eloy,
            Afaik, the calls also includes section entries which always have a null timestamp, so it should automatically be testing the situation mentioned above.
            For unit testing as you said cant be done atm. I will create a issue to move the function to course/lib.php and unit tests.

            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Hi Eloy, Afaik, the calls also includes section entries which always have a null timestamp, so it should automatically be testing the situation mentioned above. For unit testing as you said cant be done atm. I will create a issue to move the function to course/lib.php and unit tests. Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated (23, 24 & master), thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks!
            Hide
            abgreeve Adrian Greeve added a comment -

            Tested on the 2.3, 2.4 and master integration branches.
            Words as described.
            Passing.

            Show
            abgreeve Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. Words as described. Passing.
            Hide
            damyon Damyon Wiese added a comment -

            This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

            Thanks for your contributions!

            Show
            damyon Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/May/13