Moodle
  1. Moodle
  2. MDL-36872

Incorrect sorting on date for full report on recent activity

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical 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
    • Rank:
      46401

      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.

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

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

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

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

          Show
          Dan Poltawski added a comment - Spent a bit of time trying to work out whats going on.. i'm not sure.
          Hide
          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 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 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 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
          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
          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 Agarwal added a comment -

          Thanks for the review Rosie.
          Sending for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks for the review Rosie. Sending for integration. Thanks
          Hide
          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
          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
          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
          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 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 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
          Eloy Lafuente (stronk7) added a comment -

          Integrated (23, 24 & master), thanks!

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

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

          Show
          Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. Words as described. Passing.
          Hide
          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 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: