Moodle
  1. Moodle
  2. MDL-25616

Can't find messages recently sent to me on moodle.org (Search is broken + no 'recent messages' interface is present)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Messages
    • Labels:
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15226

      Description

      I just got a message from a user called George Burdo emailed to me as a copy of a message on Moodle.org. However, when I come online and login to reply, it isn't there. The messages page is blank apart from my contacts list, which George doesn't feature in and a search box.

      Searching for George Burdo shows the user record, but no sign of any message (Users: 1, Messages: 0), but clicking on the message history icon shows the message I was sent.

      The expected behaviour would be for the main messaging page to show a reverse chronological list of messages, email-inbox-style and for a search to show a message as well as a user. Without this, the system is not much good.

      My message settings are pop-up when logged in and email when not logged in.

        Activity

        Hide
        Helen Foster added a comment -

        Matt, thanks for your report. I agree that a way of seeing your most recent messages would be helpful.

        I have both pop-up AND email when logged in AND when not logged in to make doubly sure I don't miss any messages!

        Reassigning to Andrew.

        Show
        Helen Foster added a comment - Matt, thanks for your report. I agree that a way of seeing your most recent messages would be helpful. I have both pop-up AND email when logged in AND when not logged in to make doubly sure I don't miss any messages! Reassigning to Andrew.
        Hide
        Andrew Davis added a comment -

        There is a discussion of a recent messages user interface at http://moodle.org/mod/forum/discuss.php?d=164999

        Show
        Andrew Davis added a comment - There is a discussion of a recent messages user interface at http://moodle.org/mod/forum/discuss.php?d=164999
        Hide
        Martin Dougiamas added a comment -

        I would personally prefer to see something like the messaging interface on the iPhone, or Gmail for that matter, so you see a list of "conversations" with people sorted with the most recently updated first, going backwards in time.

        Each item just needs the name+icon of the other person, plus the last thing that was said by either of you ... you then click through to see the full conversation.

        Show
        Martin Dougiamas added a comment - I would personally prefer to see something like the messaging interface on the iPhone, or Gmail for that matter, so you see a list of "conversations" with people sorted with the most recently updated first, going backwards in time. Each item just needs the name+icon of the other person, plus the last thing that was said by either of you ... you then click through to see the full conversation.
        Hide
        Andrew Davis added a comment -

        Its not finished yet, need to refactor to use the html writer, but the UI is complete enough for interested parties to comment. will take some screenshots and post in the forum when I have a moment (may not be until monday).

        repo: git://github.com/andyjdavis/moodle.git
        branch: MDL-25616_recent_messages
        diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25616_recent_messages

        Show
        Andrew Davis added a comment - Its not finished yet, need to refactor to use the html writer, but the UI is complete enough for interested parties to comment. will take some screenshots and post in the forum when I have a moment (may not be until monday). repo: git://github.com/andyjdavis/moodle.git branch: MDL-25616 _recent_messages diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25616_recent_messages
        Hide
        Andrew Davis added a comment -
        Show
        Andrew Davis added a comment - screenshots posted at http://moodle.org/mod/forum/discuss.php?d=164999
        Hide
        Andrew Davis added a comment -

        Think this is ready for peer review.

        repo: git://github.com/andyjdavis/moodle.git
        branch: MDL-25616_recent_messages
        diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25616_recent_messages

        Show
        Andrew Davis added a comment - Think this is ready for peer review. repo: git://github.com/andyjdavis/moodle.git branch: MDL-25616 _recent_messages diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25616_recent_messages
        Hide
        Andrew Davis added a comment -

        I have updated the query used to get your recent conversations. The old one wasnt correct. Its a huge query but means we can retrieve all the data we need in a single query.

        Show
        Andrew Davis added a comment - I have updated the query used to get your recent conversations. The old one wasnt correct. Its a huge query but means we can retrieve all the data we need in a single query.
        Hide
        Sam Hemelryk added a comment -

        Hi Andrew, I've given the patch the first visual once over.

        I haven't actually tested the code yet but I have read through and made several notes and questions. I figure I'll give them to you now so that you can get onto it if you want while I continue testing and reviewing the SQL.

        General comments:

        theme/base/style/message.css

        • Please use hex colours rather than named colours, LightGrey = #D3D3D3

        message/lib.php

        • Why does _message_print_heading have an underscore at the front? Please fix that up.
        • Line 328: Remove unused variable $heading
        • Lines 361-363: Please tidy up code that is commented out.
        • message_print_usergroup_selector should all of the options be arrays? That seems crazy but I need to test it to see what the end outcome is.
        • Line 425: Does the key need to be a language string?
        • Line 615: That query will need SERIOUS testing and consideration, have you tested it in Postgres, or any other database engines, it would surely pay to test it in at least MySQL, and Postgres... I would perhaps even suspect it may break the limit_to_top_n function in MSSQL so perhaps test that and check the query doesn't get mangled (would represent a bug in the MSSQL driver). I will review that query separately shortly.
        • Line 672: Please remove '@global <type> $USER' (there are several of these around).
        • Is $showicontext meant to be $showincontext? and how does that name relate to the $text argument it is being supplied to in message_contact_link.
        • Is there any need to pass $messages by reference? I would think that there isn't as the foreach is going to create a copy anyway.
        • Clean up or provide explanation in the comments as to why lines 757-765 + 909-917 + 1142-1152 (this one needs to be commented out) are commented out.
        • Line 781: nobr is not a valid xhtml tag, give the span the class nobr, then add a CSS rule like '.nobr {white-space: nowrap}

          '

        • Line 1706: Remove the BR and give the above element a bottom margin in CSS.

        I also made the following notes about the code within message/lib.php while I was going through. These are things you haven't directly changed they are just things I will likely create issues for when I get time.

        • message_get_blocked_users, message_get_contacts: No need to pass $user2 by reference php5 passes all objects by reference
        • message_print_contactlist_user should really be converted to use an html_table object, or converted to return an array of data that can be put into a table to get away from manually printing the table.
        • br's should be removed all together they are the bane of the themers.
        • message_print_usergroup_selector I can't see any need to pass $courses by reference.
        • Line 434: The this.form.submit() should be converted to a component action
        • Line 1723: The static handling there seems to be a potential bug if message_format_message is called more than once with difference formats as the first format used will always be cached and used for subsequent calls.

        Yell out if you have any questions

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Andrew, I've given the patch the first visual once over. I haven't actually tested the code yet but I have read through and made several notes and questions. I figure I'll give them to you now so that you can get onto it if you want while I continue testing and reviewing the SQL. General comments: There are lots of whitespace issues still in lib.php, and index.php Please add phpdocs to all the new functions and correct any phpdocs on functions where you are changing the arguments. The indenting you have used for html output isn't in the guidelines (I double checked with Martin to be sure) and it should be changed back to conventional indenting only. e.g. https://github.com/andyjdavis/moodle/blob/dc7742e78d9977facea3d158a5b495e2f1016fa1/message/lib.php#L127 theme/base/style/message.css Please use hex colours rather than named colours, LightGrey = #D3D3D3 message/lib.php Why does _message_print_heading have an underscore at the front? Please fix that up. Line 328: Remove unused variable $heading Lines 361-363: Please tidy up code that is commented out. message_print_usergroup_selector should all of the options be arrays? That seems crazy but I need to test it to see what the end outcome is. Line 425: Does the key need to be a language string? Line 615: That query will need SERIOUS testing and consideration, have you tested it in Postgres, or any other database engines, it would surely pay to test it in at least MySQL, and Postgres... I would perhaps even suspect it may break the limit_to_top_n function in MSSQL so perhaps test that and check the query doesn't get mangled (would represent a bug in the MSSQL driver). I will review that query separately shortly. Line 672: Please remove '@global <type> $USER' (there are several of these around). Is $showicontext meant to be $showincontext? and how does that name relate to the $text argument it is being supplied to in message_contact_link. Is there any need to pass $messages by reference? I would think that there isn't as the foreach is going to create a copy anyway. Clean up or provide explanation in the comments as to why lines 757-765 + 909-917 + 1142-1152 (this one needs to be commented out) are commented out. Line 781: nobr is not a valid xhtml tag, give the span the class nobr, then add a CSS rule like '.nobr {white-space: nowrap} ' Line 1706: Remove the BR and give the above element a bottom margin in CSS. I also made the following notes about the code within message/lib.php while I was going through. These are things you haven't directly changed they are just things I will likely create issues for when I get time. message_get_blocked_users, message_get_contacts: No need to pass $user2 by reference php5 passes all objects by reference message_print_contactlist_user should really be converted to use an html_table object, or converted to return an array of data that can be put into a table to get away from manually printing the table. br's should be removed all together they are the bane of the themers. message_print_usergroup_selector I can't see any need to pass $courses by reference. Line 434: The this.form.submit() should be converted to a component action Line 1723: The static handling there seems to be a potential bug if message_format_message is called more than once with difference formats as the first format used will always be cached and used for subsequent calls. Yell out if you have any questions Cheers Sam
        Hide
        Andrew Davis added a comment -

        Thanks Sam. Im part way through fixing all that stuff. Will finish it off after Australia day

        Show
        Andrew Davis added a comment - Thanks Sam. Im part way through fixing all that stuff. Will finish it off after Australia day
        Hide
        Andrew Davis added a comment -

        Ok, I think thats all done. Ive split the get recent conversations into 2 queries. one for read and one for unread messages. Then picking out the most recent rows in php. Ive also gone through basically all of the above and made your suggested changes.

        Some responses to issues you raised:

        "Is $showicontext meant to be $showincontext?"
        no. its "show icon text".

        "message_print_usergroup_selector should all of the options be arrays?"
        theyre not all arrays however the user's courses and recent conversations/notifications are and should be. It will all make sense when you look at the UI

        "message_print_contactlist_user should really be converted to use an html_table"
        probably although i havent done that yet.

        Show
        Andrew Davis added a comment - Ok, I think thats all done. Ive split the get recent conversations into 2 queries. one for read and one for unread messages. Then picking out the most recent rows in php. Ive also gone through basically all of the above and made your suggested changes. Some responses to issues you raised: "Is $showicontext meant to be $showincontext?" no. its "show icon text". "message_print_usergroup_selector should all of the options be arrays?" theyre not all arrays however the user's courses and recent conversations/notifications are and should be. It will all make sense when you look at the UI "message_print_contactlist_user should really be converted to use an html_table" probably although i havent done that yet.
        Hide
        Andrew Davis added a comment -

        Sam has suggested an alternative query. This is the read messages version.

        SELECT m.*, mr.id, mr.smallmessage, mr.fullmessage, mr.timecreated, mc.id AS contactlistid, mc.blocked
        FROM mdl_message_read mr
        JOIN (
        SELECT u.id, u.firstname, u.lastname, u.email, u.lastaccess, MAX(mr.timecreated) AS lastmessage
        FROM mdl_message_read mr
        LEFT JOIN mdl_user u ON (u.id = mr.useridto OR u.id = mr.useridfrom) AND u.id != 112 AND u.deleted = '0'
        WHERE mr.useridto = 112 OR mr.useridfrom = 112
        GROUP BY u.id
        ) m ON m.lastmessage = mr.timecreated
        LEFT JOIN mdl_message_contacts mc ON mc.userid = 112 AND mc.contactid = m.id

        On Monday Ill test this for accuracy. Ill also populate a database with a few thousand users and a few million messages and profile the two queries.

        Show
        Andrew Davis added a comment - Sam has suggested an alternative query. This is the read messages version. SELECT m.*, mr.id, mr.smallmessage, mr.fullmessage, mr.timecreated, mc.id AS contactlistid, mc.blocked FROM mdl_message_read mr JOIN ( SELECT u.id, u.firstname, u.lastname, u.email, u.lastaccess, MAX(mr.timecreated) AS lastmessage FROM mdl_message_read mr LEFT JOIN mdl_user u ON (u.id = mr.useridto OR u.id = mr.useridfrom) AND u.id != 112 AND u.deleted = '0' WHERE mr.useridto = 112 OR mr.useridfrom = 112 GROUP BY u.id ) m ON m.lastmessage = mr.timecreated LEFT JOIN mdl_message_contacts mc ON mc.userid = 112 AND mc.contactid = m.id On Monday Ill test this for accuracy. Ill also populate a database with a few thousand users and a few million messages and profile the two queries.
        Hide
        Andrew Davis added a comment - - edited

        Ok, I've inserted 1000 users each of whom have 1000 read messages for a total of 1,000,000 messages. Using my query the recent conversations page is produced in 0.7 seconds. Sam's query seems to be producing bogus results with users who have had no contact with one another being included. It also takes page load time up to 9.8 seconds. On that basis I'm sticking with the original query.

        I think this is ready for integration review....

        Show
        Andrew Davis added a comment - - edited Ok, I've inserted 1000 users each of whom have 1000 read messages for a total of 1,000,000 messages. Using my query the recent conversations page is produced in 0.7 seconds. Sam's query seems to be producing bogus results with users who have had no contact with one another being included. It also takes page load time up to 9.8 seconds. On that basis I'm sticking with the original query. I think this is ready for integration review....
        Hide
        Andrew Davis added a comment - - edited

        This is ready for integration review. After discussion with Martin I have raised MDL-26217 and MDL-26218 for possible future implementation.

        repo: git://github.com/andyjdavis/moodle.git
        branch: MDL-25616_recent_messages
        diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25616_recent_messages

        PULL-212

        Show
        Andrew Davis added a comment - - edited This is ready for integration review. After discussion with Martin I have raised MDL-26217 and MDL-26218 for possible future implementation. repo: git://github.com/andyjdavis/moodle.git branch: MDL-25616 _recent_messages diff: https://github.com/andyjdavis/moodle/compare/master...MDL-25616_recent_messages PULL-212
        Hide
        Eloy Lafuente (stronk7) added a comment -
        Show
        Eloy Lafuente (stronk7) added a comment - Reopening this as of results of PULL-212 integration review. For comments, see http://tracker.moodle.org/browse/PULL-212?focusedCommentId=103560&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-103560 Sorry and ciao
        Hide
        Andrew Davis added a comment - - edited

        have made a number of changes. responses to your queries:

        I've squashed the lot into a single commit as requested we do for all of our pull requests. While this includes general refactoring of the messaging UI not necessarily directly related to the recent messages screens the other refactoring were not readily extractable.

        Ive switched it over to use message ID instead of message time created. Provided message IDs are always created in order this will always work and should be much faster than time created.

        notification = 0 is correct. notification = 0 just means its a personal message. Not a forum post notification, assignment submission notification etc.

        Scales fine. My dev environment has masses of users, messages and conversations. Displaying a full page of conversations doesnt seem to be an issue.

        I think Ive fixed all the whitespace issues.

        remaining issues:
        conversation sort order
        does it work with sql server?

        Show
        Andrew Davis added a comment - - edited have made a number of changes. responses to your queries: I've squashed the lot into a single commit as requested we do for all of our pull requests. While this includes general refactoring of the messaging UI not necessarily directly related to the recent messages screens the other refactoring were not readily extractable. Ive switched it over to use message ID instead of message time created. Provided message IDs are always created in order this will always work and should be much faster than time created. notification = 0 is correct. notification = 0 just means its a personal message. Not a forum post notification, assignment submission notification etc. Scales fine. My dev environment has masses of users, messages and conversations. Displaying a full page of conversations doesnt seem to be an issue. I think Ive fixed all the whitespace issues. remaining issues: conversation sort order does it work with sql server?
        Hide
        Andrew Davis added a comment -

        pushed changes to order the conversations. Installing Sql server into a VM now.

        Show
        Andrew Davis added a comment - pushed changes to order the conversations. Installing Sql server into a VM now.
        Hide
        Andrew Davis added a comment -

        I'm out of time to get this into 2.0.2. Setting up an SQL Server + git environment in a Windows VM took more time than I have. Im submitting this for consideration by the integrators with the understanding that I will perform SQL Server testing during the up coming testing period and that in future I will not modify code not directly related to the MDL I am currently working on.

        PULL-310

        Show
        Andrew Davis added a comment - I'm out of time to get this into 2.0.2. Setting up an SQL Server + git environment in a Windows VM took more time than I have. Im submitting this for consideration by the integrators with the understanding that I will perform SQL Server testing during the up coming testing period and that in future I will not modify code not directly related to the MDL I am currently working on. PULL-310
        Hide
        Helen Foster added a comment -

        Fix included in latest 2.0.1+ weekly. Many thanks Andrew and all other participants.

        Show
        Helen Foster added a comment - Fix included in latest 2.0.1+ weekly. Many thanks Andrew and all other participants.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: