[MM-11442] Strip markdown on desktop notification and "commented on" #1471

saturninoabril · 2018-07-23T16:08:18Z

Summary

Stripped markdown on push notification and "commented on".

On "commented on", it's not specified on the ticket but I let it render with emoji and profile pop-over (can easily remove if not expected)

Ticket Link

Jira ticket: MM-11442

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)
Has UI changes

saturninoabril · 2018-07-23T16:17:45Z

@esethna May I request first for PM review just to make sure that I'm not missing anything? Thanks!

jasonblais · 2018-07-23T17:28:22Z

@saturninoabril I was trying it out for a customer but wasn't able to get desktop notifications to show a popup.. I hear the sound though

saturninoabril · 2018-07-23T17:40:53Z

@jasonblais That's weird... in this test server, it happened to me on Chrome but not on FF... but on local, it works for me. I'll check this out. Thanks!

saturninoabril · 2018-07-24T10:03:57Z

@jasonblais I've tested this again and it's happening even on master. I've verified this using the test PR #1476.

The issue is only in Chrome connected to spinmint test server.
Chrome on local dev machine has desktop notification working fine.
Other browsers like Firefox and Safari is working fine.

I filed a separate ticket to look further into this https://mattermost.atlassian.net/browse/MM-11453.

To test this PR, I suggest to use browser other than Chrome for the meantime.

hmhealey · 2018-07-24T14:04:07Z

components/post_view/post_body/post_body.jsx

@@ -194,7 +200,10 @@ export default class PostBody extends React.PureComponent {
 className='theme'
 onClick={this.props.handleCommentClick}
 >
- {message}
+ <Markdown
+ message={formatWithRenderer(message, new RemoveMarkdown())}


This is actually running the message through the Markdown parser/renderer twice since it's doing it once here and once inside the Markdown component. Maybe we could instead pass the renderer as a prop to the Markdown component and then have it pass it into TextFormatting.formatText?

Sure, I'll address that after QA review. thanks

esethna

Looks great @saturninoabril, thanks! Queuing for QA review as there are lots of corner cases and different markdown cases to test

jwilander · 2018-07-26T12:38:30Z

actions/notification_actions.jsx

@@ -98,8 +101,10 @@ export function sendDesktopNotification(post, msgProps) {
 notifyText = notifyText.substring(0, 49) + '...';
 }

+ const strippedMarkdownNotifyText = formatWithRenderer(notifyText, new RemoveMarkdown());


Since the RemoveMarkdown instance is invariant based on the message, can we create a constant for it and not create a new instance every time?

DHaussermann · 2018-07-26T17:51:16Z

Hey @saturninoabril and @jasonblais , I've tested on the SpinMint and found a few cases where the markdown does not get stripped out of notifications. Please take a look over these and let me know if these cases should also have the markdown stripped out.

1. URLS for links and in-line images do not get stripped out. Such as "[Check out Mattermost!](https://about.mattermost.com/)"  or "[![Mattermost](../../images/icon-76x76.png)](https://github.com/mattermost/mattermost-server) "

2. using "-" underline to make headings such as 
Large Heading
-------------  
(making headings w\ '#' does get removed)

3.  Code block w\ syntax set. (code blocks without syntax get stripped out)
``` go
package main
import "fmt"
func main() {
    fmt.Println("Hello, 世界")
}
```
4.  LaTex formatting is not stripped out.
```latex
X_k = \sum_{n=0}^{2N-1} x_n \cos \left[\frac{\pi}{N} \left(n+\frac{1}{2}+\frac{N}{2}\right) \left(k+\frac{1}{2}\right) \right]
```

5. Table structure characters will not be removed.
| Left-Aligned  | Center Aligned  | Right Aligned |
| :------------ |:---------------:| -----:|
| Left column 1 | this text       |  $100 |
| Left column 2 | is              |   $10 |
| Left column 3 | centered        |    $1 |

Also, @saturninoabril would you be able to change the mobile push notification for me? Currently, it's set so that notification for mobile do not display any of the text from the post - only the generic message. So, I was unable to check mobile. I seem to recall this is a server setting somewhere but don't know where.

saturninoabril · 2018-07-26T17:58:46Z

Thanks @DHaussermann.
Now change to "Send full message snippet" at System Console > Notifications > Mobile Push.

However, markdown strip for push notification on RN is not included here. This is for desktop only + "commented on"

DHaussermann · 2018-07-27T14:30:25Z

Hi @saturninoabril, Thanks, I forgot that RN notifications were not in scope :).

Regarding the list above, number 1 with the link text may not be an issue but, it seems to me the rest of the markdown types should be changed to work consistently with the others in notifications. Is this possible?

Also looked over the "Commented On" and it looks good. Numbered lists will show numbers and task lists with square brackets will show the formatting but it looks reasonably clean.

mattermod · 2018-07-30T18:37:28Z

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

mattermod · 2018-07-30T18:39:41Z

Spinmint test server created at: http:https://i-077cc181c323993fc.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-077cc181c323993fc

saturninoabril · 2018-07-30T18:50:47Z

@DHaussermann Thank you for testing. I've updated the push notification which should be fixed now. Please test again, thanks

hmhealey · 2018-07-31T20:11:09Z

components/post_view/post_body/post_body.jsx

@@ -190,12 +199,15 @@ export default class PostBody extends React.PureComponent {
 apostrophe,
 }}
 />
- <a
- className='theme'
+ <span


Can you add role='button' and tabindex='0' to this span? That's needed to make the span properly accessible from the keyboard or when using accessibility devices since it's no longer an anchor tag

Oh, I never thought of that. I'll keep that in mind.
It's now updated, thanks!

DHaussermann · 2018-07-31T20:19:22Z

@saturninoabril
I have re-tested this and issues 1-5 listed above have been resolved. It seems everything is now working as expected. Thanks!

mattermod · 2018-08-01T16:55:58Z

Spinmint test server destroyed

…attermost#1471) * strip markdown on push notification and "commented on" * address dev review comment * strip markdown on push notification * update the span to include role and tabIndex for accessibilty

saturninoabril added the 1: PM Review Requires review by a product manager label Jul 23, 2018

saturninoabril requested review from jwilander and hmhealey July 23, 2018 16:08

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Jul 23, 2018

jasonblais assigned esethna Jul 23, 2018

hmhealey reviewed Jul 24, 2018

View reviewed changes

amyblais added this to the v5.2.0 milestone Jul 25, 2018

esethna changed the title ~~[MM-11442] Strip markdown on push notification and "commented on"~~ [MM-11442] Strip markdown on desktop notification and "commented on" Jul 25, 2018

esethna added 3: QA Review Requires review by a QA tester 2: Dev Review Requires review by a core commiter and removed 1: PM Review Requires review by a product manager labels Jul 25, 2018

esethna approved these changes Jul 25, 2018

View reviewed changes

jwilander reviewed Jul 26, 2018

View reviewed changes

mattermost deleted a comment from mattermod Jul 30, 2018

saturninoabril removed the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

mattermost deleted a comment from mattermod Jul 30, 2018

saturninoabril added the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

mattermost deleted a comment from mattermod Jul 30, 2018

saturninoabril removed the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

mattermost deleted a comment from mattermod Jul 30, 2018

saturninoabril added the Setup Old Test Server Triggers the creation of a test server label Jul 30, 2018

jwilander approved these changes Jul 31, 2018

View reviewed changes

hmhealey suggested changes Jul 31, 2018

View reviewed changes

saturninoabril added 4 commits August 1, 2018 07:57

strip markdown on push notification and "commented on"

a812116

address dev review comment

b642ddb

strip markdown on push notification

433907f

update the span to include role and tabIndex for accessibilty

f0172e1

DHaussermann removed the 3: QA Review Requires review by a QA tester label Aug 1, 2018

hmhealey approved these changes Aug 1, 2018

View reviewed changes

hmhealey removed 2: Dev Review Requires review by a core commiter Setup Old Test Server Triggers the creation of a test server labels Aug 1, 2018

hmhealey merged commit 7ec1395 into mattermost:master Aug 1, 2018

saturninoabril deleted the MM-11442 branch August 1, 2018 16:56

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Aug 2, 2018

lindalumitchell added the Tests/Done Release tests have been written label Sep 4, 2018

giuesposbit mentioned this pull request Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities giuesposbit/mattermost-webapp#10

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-11442] Strip markdown on desktop notification and "commented on" #1471

[MM-11442] Strip markdown on desktop notification and "commented on" #1471

saturninoabril commented Jul 23, 2018 •

edited

Loading

saturninoabril commented Jul 23, 2018

jasonblais commented Jul 23, 2018

saturninoabril commented Jul 23, 2018

saturninoabril commented Jul 24, 2018

hmhealey Jul 24, 2018

saturninoabril Jul 26, 2018

esethna left a comment

jwilander Jul 26, 2018

DHaussermann commented Jul 26, 2018 •

edited

Loading

saturninoabril commented Jul 26, 2018

DHaussermann commented Jul 27, 2018

mattermod commented Jul 30, 2018

mattermod commented Jul 30, 2018

saturninoabril commented Jul 30, 2018

hmhealey Jul 31, 2018

saturninoabril Aug 1, 2018

DHaussermann commented Jul 31, 2018

mattermod commented Aug 1, 2018

[MM-11442] Strip markdown on desktop notification and "commented on" #1471

[MM-11442] Strip markdown on desktop notification and "commented on" #1471

Conversation

saturninoabril commented Jul 23, 2018 • edited Loading

Summary

Ticket Link

Checklist

saturninoabril commented Jul 23, 2018

jasonblais commented Jul 23, 2018

saturninoabril commented Jul 23, 2018

saturninoabril commented Jul 24, 2018

hmhealey Jul 24, 2018

Choose a reason for hiding this comment

saturninoabril Jul 26, 2018

Choose a reason for hiding this comment

esethna left a comment

Choose a reason for hiding this comment

jwilander Jul 26, 2018

Choose a reason for hiding this comment

DHaussermann commented Jul 26, 2018 • edited Loading

saturninoabril commented Jul 26, 2018

DHaussermann commented Jul 27, 2018

mattermod commented Jul 30, 2018

mattermod commented Jul 30, 2018

saturninoabril commented Jul 30, 2018

hmhealey Jul 31, 2018

Choose a reason for hiding this comment

saturninoabril Aug 1, 2018

Choose a reason for hiding this comment

DHaussermann commented Jul 31, 2018

mattermod commented Aug 1, 2018

saturninoabril commented Jul 23, 2018 •

edited

Loading

DHaussermann commented Jul 26, 2018 •

edited

Loading