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

      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.

        Gliffy Diagrams

        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: