Moodle

Automatic substitution of string "To" in chat conversations.

Details

  • Type: Bug Bug
  • Status: Closed 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:

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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

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

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: