Moodle
  1. Moodle
  2. MDL-30405

Automatic substitution of string "To" in chat conversations.

        Details

        • Type: Bug Bug
        • Status: Closed
        • Priority: Major Major
        • Resolution: Fixed
        • Affects Version/s: 2.1.2, 2.2.2
        • Fix Version/s: 2.2.5, 2.3.2
        • Component/s: Chat
        • Environment:
          Linux and Windows
        • Testing Instructions:
          Hide

          Test pre-requisites

          • A chat activity
          • Two users logged in simultaneously

          Test steps

          1. Open the chat with user A
          2. Open the chat in accessible mode with user B
          3. Have them talk in the chat and make sure it works
          4. Make sure sending messages from one to another works
          5. Make sure the command `/me fixes bug` works as expected
          6. Make sure the command `beep all` works as expected
          7. Review the minor docs edits to http://docs.moodle.org/23/en/Chat_sessions with mention of these special commands
          Show
          Test pre-requisites A chat activity Two users logged in simultaneously Test steps Open the chat with user A Open the chat in accessible mode with user B Have them talk in the chat and make sure it works Make sure sending messages from one to another works Make sure the command `/me fixes bug` works as expected Make sure the command `beep all` works as expected Review the minor docs edits to http://docs.moodle.org/23/en/Chat_sessions with mention of these special commands
        • Workaround:
          Hide

          The file in question is moodle/mod/chat/lib.php.
          The relevant code is duplicated in lines 844 and 1013 (this duplication should be avoided too).
          I think it would be better to test fot the same pattern that is used inside the elseif statement.

          Instead of 

              } elseif (substr($text, 0, 2) == 'To') {
        $special = true;
        $result->type = 'dialogue';
        $pattern = '#To[[:space:]](.*):(.*)#';
        preg_match($pattern, trim($text), $matches);
        $special = true;
        $outmain = $sender->firstname.' <b>'.get_string('saidto', 'chat').'</b> <i>'.
			$matches[1].'</i>: '.$matches[2];
    }

          Replace for something like

          //variable $patternTo='#To[[:space:]](.*):(.*)#' must be defined earlier
    } elseif (preg_match($patternTo, $text)) {
        $special = true;
        $result->type = 'dialogue';
        preg_match($patternTo, trim($text), $matches);
        $special = true;
        $outmain = $sender->firstname.' <b>'.get_string('saidto', 'chat').'</b> <i>'.
			$matches[1].'</i>: '.$matches[2];
    }
          Show
          The file in question is moodle/mod/chat/lib.php. The relevant code is duplicated in lines 844 and 1013 (this duplication should be avoided too). I think it would be better to test fot the same pattern that is used inside the elseif statement. Instead of } elseif (substr($text, 0, 2) == 'To') { $special = true ; $result->type = 'dialogue'; $pattern = '#To[[:space:]](.*):(.*)#'; preg_match($pattern, trim($text), $matches); $special = true ; $outmain = $sender->firstname.' <b>'.get_string('saidto', 'chat').'</b> <i>'. $matches[1].'</i>: '.$matches[2]; } Replace for something like //variable $patternTo='#To[[:space:]](.*):(.*)#' must be defined earlier } elseif (preg_match($patternTo, $text)) { $special = true ; $result->type = 'dialogue'; preg_match($patternTo, trim($text), $matches); $special = true ; $outmain = $sender->firstname.' <b>'.get_string('saidto', 'chat').'</b> <i>'. $matches[1].'</i>: '.$matches[2]; }
        • Affected Branches:
          MOODLE_21_STABLE, MOODLE_22_STABLE
        • Fixed Branches:
          MOODLE_22_STABLE, MOODLE_23_STABLE
        • Pull from Repository:
        • Pull Master Branch:
          MDL-30405-master
        • Pull Master Diff URL:
        • Rank:
          33029

          Description

          The string "To" is automatically replaced by a block of code to send identified messages.
          However this is not a natural behaviour and brings problemas when someone begin a sentence with words like Tomorrow, Today, ...

            Issue Links

            is duplicated by

            Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-26094 When starting a message with (To) it sends the message to a specific user

            • Minor - Minor loss of function where workaround is possible
            • Closed

            Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-32025 In Chat module the string 'To' is processed to send personal messages and is not detecting correctly the pattern 'To: '

            • Minor - Minor loss of function where workaround is possible
            • Closed

              Activity

              Ascending order - Click to sort in descending order
              Hide
              Permalink
              Michael de Raadt added a comment -

              Thanks for reporting this and providing a solution.

              I'm not sure why $special needs to be set to true twice. I assume it's used later after the if construct.

              Show
              Michael de Raadt added a comment - Thanks for reporting this and providing a solution. I'm not sure why $special needs to be set to true twice. I assume it's used later after the if construct.
              Hide
              Permalink
              Frédéric Massart added a comment -

              Thanks for the patch Emanuel.

              I have now created the branches. I slightly altered the regex and added a few checks to prevent unwanted errors/notices. I also agree that this could should not be duplicated but as I created a new function to handle it, I realised that they were slightly different in some places. Merging them would be part of a small refactoring which should be part of a new issue.

              Show
              Frédéric Massart added a comment - Thanks for the patch Emanuel. I have now created the branches. I slightly altered the regex and added a few checks to prevent unwanted errors/notices. I also agree that this could should not be duplicated but as I created a new function to handle it, I realised that they were slightly different in some places. Merging them would be part of a small refactoring which should be part of a new issue.
              Hide
              Permalink
              Rajesh Taneja added a comment -

              Thanks Fred and Emanuel,

              Patch looks good, pushing for integration review.

              Show
              Rajesh Taneja added a comment - Thanks Fred and Emanuel, Patch looks good, pushing for integration review.
              Hide
              Permalink
              Dan Poltawski added a comment -

              Hi Fred,

              I've added a link to MDL-26094.

              Seems this functionality is missing documentation, so whilst you are here, please could add a brief note about the special commands on: http://docs.moodle.org/23/en/Chat_sessions (so at least non-developers can find out it exists).

              Show
              Dan Poltawski added a comment - Hi Fred, I've added a link to MDL-26094 . Seems this functionality is missing documentation, so whilst you are here, please could add a brief note about the special commands on: http://docs.moodle.org/23/en/Chat_sessions (so at least non-developers can find out it exists).
              Hide
              Permalink
              Frédéric Massart added a comment -

              A few words on the patch provided. The 'To' issue came from a weak comparison `if (substr($text, 0, 2) == 'To')`. As soon as the sentence started with 'To' is was considered as a special message to a user, leading to errors. I have transformed that into a regex so that now 'To' has to be followed by a space and the user the message is sent to, ending with colon. I have also added some fallbacks so that the chat does not crash if something goes wrong.

              Show
              Frédéric Massart added a comment - A few words on the patch provided. The 'To' issue came from a weak comparison `if (substr($text, 0, 2) == 'To')`. As soon as the sentence started with 'To' is was considered as a special message to a user, leading to errors. I have transformed that into a regex so that now 'To' has to be followed by a space and the user the message is sent to, ending with colon. I have also added some fallbacks so that the chat does not crash if something goes wrong.
              Hide
              Permalink
              Dan Poltawski added a comment -

              Thanks Fred/Emanuel, i've integrated this now.

              Note, as requested I think we need a brief mention of the special commands in the docs, so i've added this as a testing step to ensure its there.

              Show
              Dan Poltawski added a comment - Thanks Fred/Emanuel, i've integrated this now. Note, as requested I think we need a brief mention of the special commands in the docs, so i've added this as a testing step to ensure its there.
              Hide
              Permalink
              Frédéric Massart added a comment -

              Adding Helen as watcher to have her opinion.

              About the docs, I noticed that `/me` and `beep` are already mentioned. Regarding the `To <nickname>: <message>`, IMO that should not be mentioned. It's better having users clicking on the 'Send message' link which adds the correct command to the input field. The `To` command is weak and accepts anything as <nickname>.

              The docs also mentioned `:` as an alias of `/me`, I removed that as it's not a feature, and I don't think it was ever working.

              Show
              Frédéric Massart added a comment - Adding Helen as watcher to have her opinion. About the docs, I noticed that `/me` and `beep` are already mentioned. Regarding the `To <nickname>: <message>`, IMO that should not be mentioned. It's better having users clicking on the 'Send message' link which adds the correct command to the input field. The `To` command is weak and accepts anything as <nickname>. The docs also mentioned `:` as an alias of `/me`, I removed that as it's not a feature, and I don't think it was ever working.
              Hide
              Permalink
              Helen Foster added a comment -

              Hi Fred,

              Thanks for improving the docs by removing the reference to ':'.

              Regarding 'To <nickname>: <message>', I didn't know of the possibility to send a chat message to only one participant. I don't see a 'Send message' link anywhere in the chat window either, so I'm a bit confused about what you mean!

              Show
              Helen Foster added a comment - Hi Fred, Thanks for improving the docs by removing the reference to ':'. Regarding 'To <nickname>: <message>', I didn't know of the possibility to send a chat message to only one participant. I don't see a 'Send message' link anywhere in the chat window either, so I'm a bit confused about what you mean!
              Hide
              Permalink
              Mary Cooch added a comment -

              Me too!

              Show
              Mary Cooch added a comment - Me too!
              Hide
              Permalink
              Frédéric Massart added a comment -

              Helen, Mary, sorry I meant 'Talk' right next to 'Beep'. And you're right it does not send a private message to only one participant but highlights a message like this one on the chat: "Officer said to Timmy Valmer: Hi Timmy".

              Show
              Frédéric Massart added a comment - Helen, Mary, sorry I meant 'Talk' right next to 'Beep'. And you're right it does not send a private message to only one participant but highlights a message like this one on the chat: "Officer said to Timmy Valmer: Hi Timmy".
              Hide
              Permalink
              Andrew Davis added a comment -

              The chat activity seems to be working as described. Passing.

              Show
              Andrew Davis added a comment - The chat activity seems to be working as described. Passing.
              Hide
              Permalink
              Helen Foster added a comment -

              Fred, thanks for your comment, however I'm still confused as I can't see a 'Talk' link either! I'm looking at http://qa.moodle.net/mod/chat/view.php?id=10 and have entered the chat logged in as a teacher in one browser and as a student in another browser.

              Show
              Helen Foster added a comment - Fred, thanks for your comment, however I'm still confused as I can't see a 'Talk' link either! I'm looking at http://qa.moodle.net/mod/chat/view.php?id=10 and have entered the chat logged in as a teacher in one browser and as a student in another browser.
              Hide
              Permalink
              Frédéric Massart added a comment -

              Strange, it's apparently not available on QA. But it is on my Moodle master (and previous) and I can't find any way of disabling or enabling this.

              See screen shot attached.

              Show
              Frédéric Massart added a comment - Strange, it's apparently not available on QA. But it is on my Moodle master (and previous) and I can't find any way of disabling or enabling this. See screen shot attached.
              Hide
              Permalink
              Helen Foster added a comment -

              Fred, thanks for your screenshot. Now I believe you! (Only joking.)

              It is very strange, as I also tried http://demo.moodle.net (using Moodle 2.2) and didn't see a talk link, but found it on http://school.demo.moodle.net. I wondered whether it could possibly be a themes thing, as when there is a talk link, there is also a themes link (for changing the chat window between bubbles and compact, as mentioned in http://docs.moodle.org/en/Using_Chat ) however when I switched school.demo to use the binarius theme, the same as the QA site, the talk and themes links remained. Thus, it's a mystery!

              Show
              Helen Foster added a comment - Fred, thanks for your screenshot. Now I believe you! (Only joking.) It is very strange, as I also tried http://demo.moodle.net (using Moodle 2.2) and didn't see a talk link, but found it on http://school.demo.moodle.net . I wondered whether it could possibly be a themes thing, as when there is a talk link, there is also a themes link (for changing the chat window between bubbles and compact, as mentioned in http://docs.moodle.org/en/Using_Chat ) however when I switched school.demo to use the binarius theme, the same as the QA site, the talk and themes links remained. Thus, it's a mystery!
              Hide
              Permalink
              Eloy Lafuente (stronk7) added a comment -

              YEAR!*

              CAF*, TOT!*

              • Your effort amazingly resulted. (unbelievable :-P)
              • Closing as fixed.
              • Tons of thanks.
              Show
              Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved: