Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-27823

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

    Details

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

      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.

        Gliffy Diagrams

        1. message_dirty.png
          142 kB

          Issue Links

            Activity

            Hide
            aborrow Anthony Borrow added a comment -

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

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

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

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

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

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

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

            Show
            mblake Michael Blake added a comment - This issue is affecting a MP client. Please give it priority.
            Hide
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            andyjdavis 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
            aborrow 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
            aborrow 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
            howardsmiller 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
            howardsmiller 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
            aborrow 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
            aborrow 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
            andyjdavis 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
            andyjdavis 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
            aborrow 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
            aborrow 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
            andyjdavis 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
            andyjdavis 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
            nebgor 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
            nebgor 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
            andyjdavis 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
            andyjdavis 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
            stronk7 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
            stronk7 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
            timhunt 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
            timhunt 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
            andyjdavis 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
            andyjdavis 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
            nebgor Aparup Banerjee added a comment -

            The code, its good.

            Show
            nebgor Aparup Banerjee added a comment - The code, its good.
            Hide
            stronk7 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
            stronk7 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
            andyjdavis 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
            andyjdavis 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            samhemelryk 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
            samhemelryk 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
            stronk7 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
            stronk7 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
            andyjdavis 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
            andyjdavis 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:
                  Fix Release Date:
                  10/Oct/11