Moodle
  1. Moodle
  2. MDL-27823

Message - Messages sent with HTML editor during bulk message send displayed with HTML code

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Messages
    • Labels:
    • Database:
      Any
    • Testing Instructions:
      Hide

      1) As teacher, go to course with students
      2) Add the "people" block to a page then select "participants".
      3) Select one or more students and send them a multi-line message. Do this by ticking the student(s) then selecting to send a message from the drop down menu.
      4) Go to the messaging history between yourself and the student (via the people block, via your messaging history and searching if necessary)
      5) Review message history between you and the student and check that no html tags are included in the message history

      Show
      1) As teacher, go to course with students 2) Add the "people" block to a page then select "participants". 3) Select one or more students and send them a multi-line message. Do this by ticking the student(s) then selecting to send a message from the drop down menu. 4) Go to the messaging history between yourself and the student (via the people block, via your messaging history and searching if necessary) 5) Review message history between you and the student and check that no html tags are included in the message history
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-27823_bulk_messaging_2
    • Rank:
      17525

      Description

      Using the Participants block, I sent a bulk message to a couple students. When I look at the message history between me and the student, the message appears with the html code revealed (see attached screen shot). I would expect the HTML to be cleaned up before being displayed but stored in the database so that when it gets sent out as an email that it can take advantage of the HTML. We should probably check to ensure that the same thing is not happening when bulk messages are sent via the Site admin block.

        Issue Links

          Activity

          Hide
          Anthony Borrow added a comment -

          I confirmed that messages sent from Site admin - bulk user actions behave the same. Peace - Anthony

          Show
          Anthony Borrow added a comment - I confirmed that messages sent from Site admin - bulk user actions behave the same. Peace - Anthony
          Hide
          Anthony Borrow added a comment -

          Linking this to the issue where we began using the HTML editor for creating these messages.

          Show
          Anthony Borrow added a comment - Linking this to the issue where we began using the HTML editor for creating these messages.
          Hide
          Howard Miller added a comment -

          Bumping this to Major as it is causing a problem for one of our clients.

          Show
          Howard Miller added a comment - Bumping this to Major as it is causing a problem for one of our clients.
          Hide
          Michael Blake added a comment -

          This issue is affecting a MP client. Please give it priority.

          Show
          Michael Blake added a comment - This issue is affecting a MP client. Please give it priority.
          Hide
          Andrew Davis added a comment -

          I'm attaching a fix. I will create branches for 2.1 and 2.0 stable after peer review.

          Show
          Andrew Davis added a comment - I'm attaching a fix. I will create branches for 2.1 and 2.0 stable after peer review.
          Hide
          Andrew Davis added a comment -

          Updated the testing instructions. Let me know if I'm altered them in some way that substantially changes their meaning.

          Show
          Andrew Davis added a comment - Updated the testing instructions. Let me know if I'm altered them in some way that substantially changes their meaning.
          Hide
          Anthony Borrow added a comment -

          Andrew,

          The testing instructions look good. I had written them to demonstrate the problem, your version verifies the fix. My question is how is it that we want to handle the html tags.

          Your proposed solution strips all the tags and I'm not sure that is what we want either. I would expect the ability to see the results of basic formatting tags like em, i, u, etc. I am assuming that clean_text is run before the message is written to the database. I know that the paragraph tag brings it down into its own paragraph but I am OK with that since that is what the message is. My concern is that if someone writes a message with the editor and adds in the tags they are going to expect the person reading that message will see the emphasis added them. If are going to simply display the text, then I would say that the person creating the message should never have received the text editor in the first place indicating that only simple text is allowed. Yet even in that case, I have manually added html tags and they appear (I'm thinking of comments when I make them red). So what problem is caused if we do not use strip_tags at all or use it but take advantage of the allowable_tags parameter to allow certain tags. Do we possibly need to create a more limited list of $ALLOWED_TAGS for messages? Stripping all the tags does not seem like the right approach but that's likely because I do not understand the various problems created by leaving the tags in there.

          I played a little further with adding more input via the message box and it appears that it does not allow any html and if one tries to put it in then it just gets printed literally as it was put on (not interpreted). If that is the case, then it appears that messages are really not intended to have HTML at which point I would say that the solution to this issue is to use strip_tags as you are doing but also to use the simple text box rather than the editor when bulk sending messages. Otherwise, as I mentioned, the sender will presume the user can see the marked up text.

          Peace - Anthony

          Show
          Anthony Borrow added a comment - Andrew, The testing instructions look good. I had written them to demonstrate the problem, your version verifies the fix. My question is how is it that we want to handle the html tags. Your proposed solution strips all the tags and I'm not sure that is what we want either. I would expect the ability to see the results of basic formatting tags like em, i, u, etc. I am assuming that clean_text is run before the message is written to the database. I know that the paragraph tag brings it down into its own paragraph but I am OK with that since that is what the message is. My concern is that if someone writes a message with the editor and adds in the tags they are going to expect the person reading that message will see the emphasis added them. If are going to simply display the text, then I would say that the person creating the message should never have received the text editor in the first place indicating that only simple text is allowed. Yet even in that case, I have manually added html tags and they appear (I'm thinking of comments when I make them red). So what problem is caused if we do not use strip_tags at all or use it but take advantage of the allowable_tags parameter to allow certain tags. Do we possibly need to create a more limited list of $ALLOWED_TAGS for messages? Stripping all the tags does not seem like the right approach but that's likely because I do not understand the various problems created by leaving the tags in there. I played a little further with adding more input via the message box and it appears that it does not allow any html and if one tries to put it in then it just gets printed literally as it was put on (not interpreted). If that is the case, then it appears that messages are really not intended to have HTML at which point I would say that the solution to this issue is to use strip_tags as you are doing but also to use the simple text box rather than the editor when bulk sending messages. Otherwise, as I mentioned, the sender will presume the user can see the marked up text. Peace - Anthony
          Hide
          Howard Miller added a comment -

          Maybe I'm just being naive, but surely the edit box should be the same for typing messages everywhere - that is a plain text box?

          As messages tend to be short, I see no reason to go to all the bother of handling HTML markup at all (but others may disagree).

          Show
          Howard Miller added a comment - Maybe I'm just being naive, but surely the edit box should be the same for typing messages everywhere - that is a plain text box? As messages tend to be short, I see no reason to go to all the bother of handling HTML markup at all (but others may disagree).
          Hide
          Anthony Borrow added a comment -

          Howard - In fact, when using bulk user actions and sending a message through those interfaces the full HTML editor is currently used. Using a plain text box would probably eliminate the issue and then the stripping of tags would be more of a formality rather than a practical necessity. Peace - Anthony

          Show
          Anthony Borrow added a comment - Howard - In fact, when using bulk user actions and sending a message through those interfaces the full HTML editor is currently used. Using a plain text box would probably eliminate the issue and then the stripping of tags would be more of a formality rather than a practical necessity. Peace - Anthony
          Hide
          Andrew Davis added a comment -

          I was considering removing the editor and just using a text box. I was worried people would be unhappy with that. Perhaps I will do that after all...

          Show
          Andrew Davis added a comment - I was considering removing the editor and just using a text box. I was worried people would be unhappy with that. Perhaps I will do that after all...
          Hide
          Anthony Borrow added a comment -

          Andrew - Thanks for working on this. I agree that removing the HTML editor and using a text box is probably the way to go as it will help avoid confusion and what could be considered data loss. You certainly have my +1. Peace - Anthony

          Show
          Anthony Borrow added a comment - Andrew - Thanks for working on this. I agree that removing the HTML editor and using a text box is probably the way to go as it will help avoid confusion and what could be considered data loss. You certainly have my +1. Peace - Anthony
          Hide
          Andrew Davis added a comment -

          I have added a second commit that removes the HTML editor. I've left in the first commit that improves handling of HTML messages as its possible html messages will be send from other places like peoples custom message providers.

          Show
          Andrew Davis added a comment - I have added a second commit that removes the HTML editor. I've left in the first commit that improves handling of HTML messages as its possible html messages will be send from other places like peoples custom message providers.
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Andrew,
          The code looks good to me just 1 minor point -

          $format = FORMAT_MOODLE;

          is only symbolic now. Maybe simply use FORMAT_MOODLE instead?

          cheers,
          Aparup

          Show
          Aparup Banerjee added a comment - - edited Hi Andrew, The code looks good to me just 1 minor point - $format = FORMAT_MOODLE; is only symbolic now. Maybe simply use FORMAT_MOODLE instead? cheers, Aparup
          Hide
          Andrew Davis added a comment -

          I've removed the pointless variable that Aparup pointed out and created the various versions of this required for integration.

          Show
          Andrew Davis added a comment - I've removed the pointless variable that Aparup pointed out and created the various versions of this required for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, some Qs. leading me to fail this:

          1) I think this patch is missing the admin/users bulk messaging utility. It continues showing the editor + format. Both (participants bulk and admin's bulk should behave the same IMO).

          BUT, apart of that:

          A) I don't understand why removing the ability to send HTML is the best option. If I'm a teacher wanting to send people something richer than simple text... why I cannot? I'm sure people has used bulk messaging in that way.

          B) I don't understand why strip_tags() is the best option for messages having HTML format. We know it's HTML format, hence we should be able to handle it properly, no matter which one the provider is.

          So I'm rejecting this, not because any coding problem (apart from 1) but strongly thinking A) and B) need agreement and cannot land into stable branches suddenly.

          Please, discuss & agree it. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, some Qs. leading me to fail this: 1) I think this patch is missing the admin/users bulk messaging utility. It continues showing the editor + format. Both (participants bulk and admin's bulk should behave the same IMO). BUT, apart of that: A) I don't understand why removing the ability to send HTML is the best option. If I'm a teacher wanting to send people something richer than simple text... why I cannot? I'm sure people has used bulk messaging in that way. B) I don't understand why strip_tags() is the best option for messages having HTML format. We know it's HTML format, hence we should be able to handle it properly, no matter which one the provider is. So I'm rejecting this, not because any coding problem (apart from 1) but strongly thinking A) and B) need agreement and cannot land into stable branches suddenly. Please, discuss & agree it. Ciao
          Hide
          Tim Hunt added a comment -

          The right way to convert HTML to text is our html_to_text function. It may be appropriate to format_text the input first.

          Show
          Tim Hunt added a comment - The right way to convert HTML to text is our html_to_text function. It may be appropriate to format_text the input first.
          Hide
          Andrew Davis added a comment - - edited

          Thanks for the tip Tim. html_to_text() removes the need to strip the tags out to protect the page.

          Have left the html editor in this time around. No really compelling reason to remove it apart from consistency. The html editor was removed as an option on the main person to person messaging screen as part of 2.0.

          I guess Im after another peer review. I'll leave that until monday just in case there's anymore comments

          Show
          Andrew Davis added a comment - - edited Thanks for the tip Tim. html_to_text() removes the need to strip the tags out to protect the page. Have left the html editor in this time around. No really compelling reason to remove it apart from consistency. The html editor was removed as an option on the main person to person messaging screen as part of 2.0. I guess Im after another peer review. I'll leave that until monday just in case there's anymore comments
          Hide
          Aparup Banerjee added a comment -

          The code, its good.

          Show
          Aparup Banerjee added a comment - The code, its good.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I think the code is perfect, but this is one recall to my previous A) above.

          Why are we converting to raw text any richer html content if we have rich-enabled providers like the popup and email ones? Why trim down the contents for them?

          That's the point I'm not happy with. IMO each provider should decide how to transform the message or so. No reduce it globally to the simpler (raw text) way.

          I'l keeping this open for discussion. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I think the code is perfect, but this is one recall to my previous A) above. Why are we converting to raw text any richer html content if we have rich-enabled providers like the popup and email ones? Why trim down the contents for them? That's the point I'm not happy with. IMO each provider should decide how to transform the message or so. No reduce it globally to the simpler (raw text) way. I'l keeping this open for discussion. Ciao
          Hide
          Andrew Davis added a comment -

          Eloy, we're not. The two places where we're transforming the message are within the popup display function message_popup_window() while constructing the html for the popup and while constructing the html that is displayed in the message history in message_format_message().

          The full unmodified message is available to the processors that want it.

          Show
          Andrew Davis added a comment - Eloy, we're not. The two places where we're transforming the message are within the popup display function message_popup_window() while constructing the html for the popup and while constructing the html that is displayed in the message history in message_format_message(). The full unmodified message is available to the processors that want it.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Aha, that is better that I supposed then, thanks!

          But popup is exactly one provider able to handle HTML perfectly, so I think the question continues in the air.

          Anyway, I'm going to re-review this and integrate it. Some day we'll have to:

          • rethink the support of HTML in the popup provider.
          • create a proper popup provider taking code out from core libs and implementing everything into messaging + plugins.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Aha, that is better that I supposed then, thanks! But popup is exactly one provider able to handle HTML perfectly, so I think the question continues in the air. Anyway, I'm going to re-review this and integrate it. Some day we'll have to: rethink the support of HTML in the popup provider. create a proper popup provider taking code out from core libs and implementing everything into messaging + plugins. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, I don't really like what I see, but I think it's ok for fixing the problem reported in this issue.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, I don't really like what I see, but I think it's ok for fixing the problem reported in this issue. Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Attaching zip file with screenshots about how the sent messages are displayed now.

          Some comments:

          1) it seems we are missing some line2br, or enclose the message with some "pre" tags or CSS it to show the line breaks properly.

          2) The screenshots reveal that in "recent conversations" mode the messages continue showing in html mode

          3) But both the popup notification and the message if searched is converter to "sort of text", that I guess was the goal for this issue (don't forget the 1) above).

          So, these have been my tests. I still don't get why we decided to show the messages that way in the places specified in 3, more when the same message continues being viewable in html format in other places.

          In any case, whoever is going to officially test this, I hope the screenshots will help. Then decide if this is a pass or no. I'm sorry I don't understand the dichotomy.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Attaching zip file with screenshots about how the sent messages are displayed now. Some comments: 1) it seems we are missing some line2br, or enclose the message with some "pre" tags or CSS it to show the line breaks properly. 2) The screenshots reveal that in "recent conversations" mode the messages continue showing in html mode 3) But both the popup notification and the message if searched is converter to "sort of text", that I guess was the goal for this issue (don't forget the 1) above). So, these have been my tests. I still don't get why we decided to show the messages that way in the places specified in 3, more when the same message continues being viewable in html format in other places. In any case, whoever is going to officially test this, I hope the screenshots will help. Then decide if this is a pass or no. I'm sorry I don't understand the dichotomy. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Giving this a pass although I have the same sort of findings as Eloy, display of messages seems to be rather inconsistent depending upon where the message is being viewed, and certainly adding nl2br calls would help readability of the messages.

          Anwyay Andrew would you like to create a new issue to review the display the review of messages for consistency or shall I?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Giving this a pass although I have the same sort of findings as Eloy, display of messages seems to be rather inconsistent depending upon where the message is being viewed, and certainly adding nl2br calls would help readability of the messages. Anwyay Andrew would you like to create a new issue to review the display the review of messages for consistency or shall I? Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work, this has been sent upstream and is available in all the git and cvs repositories.
          Hide
          Andrew Davis added a comment -

          Ive created MDL-29243. Eloy, you won't hurt my feelings if you raise these kinds of bugs yourself

          Show
          Andrew Davis added a comment - Ive created MDL-29243 . Eloy, you won't hurt my feelings if you raise these kinds of bugs yourself

            People

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

              Dates

              • Created:
                Updated:
                Resolved: