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

          Attachments

            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