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
    • 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

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Mary Cooch added a comment -

          Me too!

          Show
          Mary Cooch added a comment - Me too!
          Hide
          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
          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
          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
          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
          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
          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: