Moodle
  1. Moodle
  2. MDL-30022

Messages with duplicate timecreated values not displayed

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Youll need 2 users.

      Log in as user A and send user B a message.
      Log in as user B and go to messaging.
      Check that you can move back and forth between "recent message" and "all messages". The links are beneath the user's picture above the message history.
      You should not receive any errors and the messages should be ordered correctly. The newest messages should be last, immediately above the new message text box.

      Select "recent conversations" from the messaging navigation drop down and check that that doesn't give any errors.

      For bonus points you can run the attached script and repeat.

      Show
      Youll need 2 users. Log in as user A and send user B a message. Log in as user B and go to messaging. Check that you can move back and forth between "recent message" and "all messages". The links are beneath the user's picture above the message history. You should not receive any errors and the messages should be ordered correctly. The newest messages should be last, immediately above the new message text box. Select "recent conversations" from the messaging navigation drop down and check that that doesn't give any errors. For bonus points you can run the attached script and repeat.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-30022_message_duplicate
    • Rank:
      19565

      Description

      I currently have a scenario where two entries in the mdl_message_read table are from the same user, to the same user, and have an identical timecreated value.

      When viewing "Recent messages", only one of the two messages shows up.

      However, if I edit the "timecreated" database value to a different value, both messages are displayed.

      1. duplicatemessage.php
        9 kB
        Brendan Rollinson
      2. fixduplicatetimes.patch
        2 kB
        Brendan Rollinson

        Activity

        Hide
        Brendan Rollinson added a comment -

        Proposed fix

        Show
        Brendan Rollinson added a comment - Proposed fix
        Hide
        Brendan Rollinson added a comment -

        Script for reproducing problem.

        Show
        Brendan Rollinson added a comment - Script for reproducing problem.
        Hide
        Brendan Rollinson added a comment -

        I've attached a patch that seems to fix the issue.

        Also, I've attached a php script that can be run to reproduce the problem. To run it, re-define the constants at the top to refer to two users on your site, then run the script from the root of your site.
        The script sends a message from user A to user B and user B to user A at the exact same time, which causes only one of those two messages to show up in the message history. Applying the provided patch fixes that issue.

        Show
        Brendan Rollinson added a comment - I've attached a patch that seems to fix the issue. Also, I've attached a php script that can be run to reproduce the problem. To run it, re-define the constants at the top to refer to two users on your site, then run the script from the root of your site. The script sends a message from user A to user B and user B to user A at the exact same time, which causes only one of those two messages to show up in the message history. Applying the provided patch fixes that issue.
        Hide
        Andrew Davis added a comment -

        Attaching a branch that contains a slightly modified version of the patch.

        Show
        Andrew Davis added a comment - Attaching a branch that contains a slightly modified version of the patch.
        Hide
        Frédéric Massart added a comment -

        Hi Brendan, Andrew,

        the patch looks good, but I am just wondering if we should not use collatorlib::asort_objects_by_property(). Probably followed by a array_reverse(). If that's the case, maybe we could add a the reverse in the collatorlib.

        If that's not relevant, please feel free to push for integration. Thanks

        Show
        Frédéric Massart added a comment - Hi Brendan, Andrew, the patch looks good, but I am just wondering if we should not use collatorlib::asort_objects_by_property(). Probably followed by a array_reverse(). If that's the case, maybe we could add a the reverse in the collatorlib. If that's not relevant, please feel free to push for integration. Thanks
        Hide
        Andrew Davis added a comment - - edited

        Ive switched over to using collatorlib::asort_objects_by_property() for both message and conversation sorting. It makes little difference but it does allow me to delete some code.

        I haven't bothered to adding the ability to control the sort order, ASC Vs DESC, to collatorlib. I'm just doing the reversal in the messaging code.

        Peer review part 2

        Show
        Andrew Davis added a comment - - edited Ive switched over to using collatorlib::asort_objects_by_property() for both message and conversation sorting. It makes little difference but it does allow me to delete some code. I haven't bothered to adding the ability to control the sort order, ASC Vs DESC, to collatorlib. I'm just doing the reversal in the messaging code. Peer review part 2
        Hide
        Frédéric Massart added a comment -

        Looks good to me Andrew. Push forward whenever ready. Cheers \o/!

        Show
        Frédéric Massart added a comment - Looks good to me Andrew. Push forward whenever ready. Cheers \o/!
        Hide
        Andrew Davis added a comment -

        Creating the various branches and submitting for integration.

        Show
        Andrew Davis added a comment - Creating the various branches and submitting for integration.
        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 -

        Sorry, 22_STABLE collators API does not support collatorlib::SORT_NUMERIC as 3rd parameter in collatorlib::asort_objects_by_property().

        So, there are 2 options:

        1) Analyze is 22_STABLE can receive the fix with its current API. Do it if possible, else...
        2) Take 22_STABLE out from the target branches.

        Reopening...ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Sorry, 22_STABLE collators API does not support collatorlib::SORT_NUMERIC as 3rd parameter in collatorlib::asort_objects_by_property(). So, there are 2 options: 1) Analyze is 22_STABLE can receive the fix with its current API. Do it if possible, else... 2) Take 22_STABLE out from the target branches. Reopening...ciao
        Hide
        CiBoT added a comment -

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

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

        I've removed the 2.2 branch. In my opinion this bug isn't significant enough to justify investing additional time fixing just in the 2.2 branch.

        Show
        Andrew Davis added a comment - I've removed the 2.2 branch. In my opinion this bug isn't significant enough to justify investing additional time fixing just in the 2.2 branch.
        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
        Sam Hemelryk added a comment -

        Thanks Andrew, has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Andrew, has been integrated now
        Hide
        David Monllaó added a comment -

        It passes, tested in master, all works as expected

        Show
        David Monllaó added a comment - It passes, tested in master, all works as expected
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for the hard work.

        These changes have been spread upstream and are already available in the git and cvs repositories.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: