Moodle
  1. Moodle
  2. MDL-13805

Reading student messages as a teacher doesn't function correctly

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.3, 2.3.2
    • Fix Version/s: 2.3.5, 2.4.2
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      For this test you'll need two browsers, ie Firefox and Chrome, and three users.

      Make sure each user has at least one message to and from each other user. Make them something easy to identify. For example, "hello user A, this is from user B."

      Make sure one of the users is your admin.

      Pick one of the other two to be your target user. They must be enrolled in a course. Make sure that user has the other non-admin user as a contact.

      As admin go into a course and click on participants in the navigation block. Click on the name of your target user.

      You'll notice that the participants part of the navigation tree now includes the user you selected. Click on "messages" under your selected user.

      The target user's contact list should be displayed. Click on one of their contacts and check that the correct users are displayed.

      Note that the add/remove contact icon etc are hidden as you are not allowed to do that stuff while viewing someone else's messages. Search is also unavailable as one user searching another user's messages is not currently supported.

      Open your other browser and log in as the target user. Go to their messages. Click on the same contact.
      The two screens should now match aside from the small differences noted above.

      In both browsers select the various options available in the messaging navigation drop down and check that the stuff displayed is the same.

      Show
      For this test you'll need two browsers, ie Firefox and Chrome, and three users. Make sure each user has at least one message to and from each other user. Make them something easy to identify. For example, "hello user A, this is from user B." Make sure one of the users is your admin. Pick one of the other two to be your target user. They must be enrolled in a course. Make sure that user has the other non-admin user as a contact. As admin go into a course and click on participants in the navigation block. Click on the name of your target user. You'll notice that the participants part of the navigation tree now includes the user you selected. Click on "messages" under your selected user. The target user's contact list should be displayed. Click on one of their contacts and check that the correct users are displayed. Note that the add/remove contact icon etc are hidden as you are not allowed to do that stuff while viewing someone else's messages. Search is also unavailable as one user searching another user's messages is not currently supported. Open your other browser and log in as the target user. Go to their messages. Click on the same contact. The two screens should now match aside from the small differences noted above. In both browsers select the various options available in the messaging navigation drop down and check that the stuff displayed is the same.
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-13805_others_messages_24
    • Pull Master Branch:
      MDL-13805_others_messages
    • Rank:
      2007

      Description

      Accessing the messages of a student while logged in as a teacher does not function quite right. You tend to be directed back to your own messages.

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment - - edited

          The ability to read other user's messages is controlled by the "moodle/site:readallmessages" capability. While that is enforced correctly something is amiss.

          If I go into a course, go to participants and select a student, they appear in the navigation block. I can select "messages" for that student. However the link seems to take me to my own messages with that student rather than the student's messages with other users. The URL is /message/index.php?id=3 and should be /message/index.php?user1=3

          If you hand craft the URL you can see the correct conversation however the breadcrumbs and the navigation block current node are incorrect. Go through the process of selecting a course participant but select "notes" instead of "messages" to see how it should behave.

          Show
          Andrew Davis added a comment - - edited The ability to read other user's messages is controlled by the "moodle/site:readallmessages" capability. While that is enforced correctly something is amiss. If I go into a course, go to participants and select a student, they appear in the navigation block. I can select "messages" for that student. However the link seems to take me to my own messages with that student rather than the student's messages with other users. The URL is /message/index.php?id=3 and should be /message/index.php?user1=3 If you hand craft the URL you can see the correct conversation however the breadcrumbs and the navigation block current node are incorrect. Go through the process of selecting a course participant but select "notes" instead of "messages" to see how it should behave.
          Hide
          Andrew Davis added a comment -

          Im changing this from a feature request to a bug report and updating the issue title as the functionality now exists, it just doesn't function correctly.

          Show
          Andrew Davis added a comment - Im changing this from a feature request to a bug report and updating the issue title as the functionality now exists, it just doesn't function correctly.
          Hide
          Andrew Davis added a comment -

          I've done some work on this however I'm parking it until MDL-36162 is resolved. It involves changes in the same area of code so I'll get that done before doing anything further here.

          Show
          Andrew Davis added a comment - I've done some work on this however I'm parking it until MDL-36162 is resolved. It involves changes in the same area of code so I'll get that done before doing anything further here.
          Hide
          Andrew Davis added a comment -

          This is a note to myself to remind me to check that the problem reported in MDL-26018 is definitely fixed.

          Show
          Andrew Davis added a comment - This is a note to myself to remind me to check that the problem reported in MDL-26018 is definitely fixed.
          Hide
          Andrew Davis added a comment -

          Adding testing instructions, a branch and putting this up for peer review.

          Show
          Andrew Davis added a comment - Adding testing instructions, a branch and putting this up for peer review.
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew,

          I had a look and this is better, but I still think the UI is confusing when viewing someone elses messages.

          Suggestions:

          The nav bar should be "Home / ► Users / ► Mr T / ► Messages" but it is just
          "Home / ► Users / ► Mr T"

          Also, IMO, when viewing someone elses messages, the Message navigation list should change too. It currently says "My contacts" - it should say "Contacts - Mr T". Similarly the headings for "Recent notifications" and "Recent conversations" would be much clearer if they were "Recent notifications (Mr T)" and "Recent conversations (Mr T)".

          Checklist:
          [N] Syntax - Found a couple of really long lines, and some badly formatted comments (/// not blocked
          [Y] Output - (Same as existing code)
          [N] Whitespace - Found a minor white space issue in message/lib.php line 1821 (commas in function arguments)
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security (There are manually created forms with no sesskey protection, but they do not seem to do anything desctructive)
          [-] Documentation
          [Y] Git
          [N] Sanity check - Just my comments above about the confusion UI when viewing someone elses messages.

          Thanks - Damyon

          Show
          Damyon Wiese added a comment - Thanks Andrew, I had a look and this is better, but I still think the UI is confusing when viewing someone elses messages. Suggestions: The nav bar should be "Home / ► Users / ► Mr T / ► Messages" but it is just "Home / ► Users / ► Mr T" Also, IMO, when viewing someone elses messages, the Message navigation list should change too. It currently says "My contacts" - it should say "Contacts - Mr T". Similarly the headings for "Recent notifications" and "Recent conversations" would be much clearer if they were "Recent notifications (Mr T)" and "Recent conversations (Mr T)". Checklist: [N] Syntax - Found a couple of really long lines, and some badly formatted comments (/// not blocked [Y] Output - (Same as existing code) [N] Whitespace - Found a minor white space issue in message/lib.php line 1821 (commas in function arguments) [Y] Language [-] Databases [Y] Testing [-] Security (There are manually created forms with no sesskey protection, but they do not seem to do anything desctructive) [-] Documentation [Y] Git [N] Sanity check - Just my comments above about the confusion UI when viewing someone elses messages. Thanks - Damyon
          Hide
          Andrew Davis added a comment -

          Hi. I started doing a big clean up of the comments but then realized thats going to cause me conflicts with MDL-36941 so Im parking that for the moment.

          Show
          Andrew Davis added a comment - Hi. I started doing a big clean up of the comments but then realized thats going to cause me conflicts with MDL-36941 so Im parking that for the moment.
          Hide
          Andrew Davis added a comment -

          Re this

          The nav bar should be "Home / ► Users / ► Mr T / ► Messages"
          but it is just "Home / ► Users / ► Mr T"

          Ill ping Sam Hemelryk about the nav bar not being quite right. Ive done some experimenting and Im not sure how to fix it. /notes/index.php works properly when viewing another user's notes but I can't for the life of me see the difference.

          Show
          Andrew Davis added a comment - Re this The nav bar should be "Home / ► Users / ► Mr T / ► Messages" but it is just "Home / ► Users / ► Mr T" Ill ping Sam Hemelryk about the nav bar not being quite right. Ive done some experimenting and Im not sure how to fix it. /notes/index.php works properly when viewing another user's notes but I can't for the life of me see the difference.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          The navigation finds the active page but checking every page added to see whether it exactly matches the URL set against the page.
          If it can't find an exact match then it looks for the first page with a matching path.
          Finally if it still can't find the

          Usually the case with issues like this is that additional params have been added to the page URL that weren't present in the navigation URL.
          There are two solutions:

          1. Add them to the navigation URL, not usually a great solution as usually the params have been missed because they control display or filter or such.
          2. Call navigation_node::override_active_url. That allows you to set the URL used to find the active page in the navigation. You can tell it precisely what URL it should look for.

          Hope that helps, let me know if you're still stuck.
          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, The navigation finds the active page but checking every page added to see whether it exactly matches the URL set against the page. If it can't find an exact match then it looks for the first page with a matching path. Finally if it still can't find the Usually the case with issues like this is that additional params have been added to the page URL that weren't present in the navigation URL. There are two solutions: Add them to the navigation URL, not usually a great solution as usually the params have been missed because they control display or filter or such. Call navigation_node::override_active_url. That allows you to set the URL used to find the active page in the navigation. You can tell it precisely what URL it should look for. Hope that helps, let me know if you're still stuck. Cheers Sam
          Hide
          Andrew Davis added a comment -

          Thanks Sam. I've figure it out

          This is ready for peer review again. There are two commits. The second switches the navigation drop down from saying "My contacts" to just "Contacts" as that will work just as well regardless of whether you are viewing your own or someone else's messages. That second commit will however only be able to go into master as it involves the removal of a string that is no longer used.

          Show
          Andrew Davis added a comment - Thanks Sam. I've figure it out This is ready for peer review again. There are two commits. The second switches the navigation drop down from saying "My contacts" to just "Contacts" as that will work just as well regardless of whether you are viewing your own or someone else's messages. That second commit will however only be able to go into master as it involves the removal of a string that is no longer used.
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew!

          Comments:

          You have an unrelated commit sitting in that branch (called squish).

          Also - there are a couple of other strings in that messaging ui that don't make sense when viewing someone elses messages:

          "Your contact list is empty"
          and
          "These messages are from people who are not in your contact list. To add them to your contacts, click the "Add contact" icon next to their name."

          Apart from these issues the patch looks good.

          Show
          Damyon Wiese added a comment - Thanks Andrew! Comments: You have an unrelated commit sitting in that branch (called squish). Also - there are a couple of other strings in that messaging ui that don't make sense when viewing someone elses messages: "Your contact list is empty" and "These messages are from people who are not in your contact list. To add them to your contacts, click the "Add contact" icon next to their name." Apart from these issues the patch looks good.
          Hide
          Andrew Davis added a comment -

          "Your contact list is empty" I've changed that string to "Contact list empty" so it works in both scenarios. The other string isn't displayed when you're viewing another user's messages.

          The "squish" commit isn't shown anymore. I did have some git trouble that I resolved in the last few days.

          Putting this up for integration review. Thanks for all of your help Damyon

          Show
          Andrew Davis added a comment - "Your contact list is empty" I've changed that string to "Contact list empty" so it works in both scenarios. The other string isn't displayed when you're viewing another user's messages. The "squish" commit isn't shown anymore. I did have some git trouble that I resolved in the last few days. Putting this up for integration review. Thanks for all of your help Damyon
          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 -

          Hi Andrew,

          Changes look good thank you.
          There are a couple of questions/points to raise before this gets integrated however:

          1. When I merge your branch in I see there are six commits on, a commit called squish, merging that branch to head X2, and then it being reverted. Finally the two commits we are interested in. There are two options here, you can fix up your branch if you like, or during integration I will cherry-pick just the two commits of interest.
          2. Changes to the API should be noted in message/upgrade.txt, just makes it easier to see where changes are happening.
          3. I'd just like to clarify what branches this should be land to?

          For the time being I'll leave this in integration review, but please reply ASAP otherwise it will be reopened.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Changes look good thank you. There are a couple of questions/points to raise before this gets integrated however: When I merge your branch in I see there are six commits on, a commit called squish, merging that branch to head X2, and then it being reverted. Finally the two commits we are interested in. There are two options here, you can fix up your branch if you like, or during integration I will cherry-pick just the two commits of interest. Changes to the API should be noted in message/upgrade.txt, just makes it easier to see where changes are happening. I'd just like to clarify what branches this should be land to? For the time being I'll leave this in integration review, but please reply ASAP otherwise it will be reopened. Many thanks Sam
          Hide
          Andrew Davis added a comment -

          Hi Sam.

          1. A few weeks ago I accidentally committed something with the commit message "squish" to master. I didnt notice it for a week or two so now its everywhere In every one of my branches etc. I'm not sure how to get rid of it short of blowing away my local and github repositories. If you can cherry-pick around my screw up that would be great.

          2. There aren't any API changes in here. The message API lives in lib/messagelib.php and isn't being modified here. The changes in message/lib.php are all to functions internal to the messaging system.

          3. Im cherry-picking in to 2.4 and 2.3 now to check that works fine. Only the first commit should go into the stable branches.

          Show
          Andrew Davis added a comment - Hi Sam. 1. A few weeks ago I accidentally committed something with the commit message "squish" to master. I didnt notice it for a week or two so now its everywhere In every one of my branches etc. I'm not sure how to get rid of it short of blowing away my local and github repositories. If you can cherry-pick around my screw up that would be great. 2. There aren't any API changes in here. The message API lives in lib/messagelib.php and isn't being modified here. The changes in message/lib.php are all to functions internal to the messaging system. 3. Im cherry-picking in to 2.4 and 2.3 now to check that works fine. Only the first commit should go into the stable branches.
          Hide
          Andrew Davis added a comment -

          It seems to go cleanly onto 2.4. 2.3, less so...

          Show
          Andrew Davis added a comment - It seems to go cleanly onto 2.4. 2.3, less so...
          Hide
          Andrew Davis added a comment -

          Adding 2.4 and 2.3 versions.

          Show
          Andrew Davis added a comment - Adding 2.4 and 2.3 versions.
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

          I was just looking to your github repository and indeed your master branch there contains the squash commit.
          If you want to give it a shot fixing it now you could run the following:

          git fetch git://git.moodle.org/moodle.git master && git push git@github.com:andyjdavis/moodle.git FETCH_HEAD:master
          

          That fetches the moodle.git master and force pushes it to your github repository.
          The run the following on all repositories where you have a master branch that is tracking your github master:

          git checkout master && git fetch git://git.moodle.org/moodle.git master && git reset --hard FETCH_HEAD; git fetch --all --prune
          

          That checks out the master branch, fetches the moodle.git master and resets your local master to moodle.git master. Then fetches all updates. Hopefully if you run `git branch -v` everything looks normal.

          Hope that helps.
          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now. I was just looking to your github repository and indeed your master branch there contains the squash commit. If you want to give it a shot fixing it now you could run the following: git fetch git: //git.moodle.org/moodle.git master && git push git@github.com:andyjdavis/moodle.git FETCH_HEAD:master That fetches the moodle.git master and force pushes it to your github repository. The run the following on all repositories where you have a master branch that is tracking your github master: git checkout master && git fetch git: //git.moodle.org/moodle.git master && git reset --hard FETCH_HEAD; git fetch --all --prune That checks out the master branch, fetches the moodle.git master and resets your local master to moodle.git master. Then fetches all updates. Hopefully if you run `git branch -v` everything looks normal. Hope that helps. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          I should add that fixes just your master branches.

          Any branches that are based off master and were created after you messed it up would still need to be fixed as well.
          The only way I can think to do that would be to go through each branch, take note of the commit hashes, hard reset it and then cherry-pick the commits.

          Show
          Sam Hemelryk added a comment - I should add that fixes just your master branches. Any branches that are based off master and were created after you messed it up would still need to be fixed as well. The only way I can think to do that would be to go through each branch, take note of the commit hashes, hard reset it and then cherry-pick the commits.
          Hide
          David Monllaó added a comment -

          I've found different notices in 23, attaching screenshot

          To reproduce the problem:

          • Login as a user
          • Go to settings block -> my profile settings -> messaging
          Show
          David Monllaó added a comment - I've found different notices in 23, attaching screenshot To reproduce the problem: Login as a user Go to settings block -> my profile settings -> messaging
          Hide
          Andrew Davis added a comment -

          I've added a second commit (https://github.com/andyjdavis/moodle/compare/MOODLE_23_STABLE...MDL-13805_others_messages_23) that fixes this. This can be reset.

          Show
          Andrew Davis added a comment - I've added a second commit ( https://github.com/andyjdavis/moodle/compare/MOODLE_23_STABLE...MDL-13805_others_messages_23 ) that fixes this. This can be reset.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, I've pulled in the fix for 2.3 and this is ready for testing once more.

          Show
          Sam Hemelryk added a comment - Thanks guys, I've pulled in the fix for 2.3 and this is ready for testing once more.
          Hide
          David Monllaó added a comment -

          It passes, all works as expected, tested in 23 and master

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

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: