Skip to content

Fixes #171 : Remove comments before posting a notification - #172

Merged
lieut-data merged 13 commits into
mattermost:masterfrom
M-ZubairAhmed:GH-171
Jan 17, 2020
Merged

Fixes #171 : Remove comments before posting a notification#172
lieut-data merged 13 commits into
mattermost:masterfrom
M-ZubairAhmed:GH-171

Conversation

@M-ZubairAhmed

@M-ZubairAhmed M-ZubairAhmed commented Dec 31, 2019

Copy link
Copy Markdown
Member

Summary

This PR adds the ability to filter out the markdown comments from the push notifications.

Ticket Link

Fixes : #171
PR : nil

@M-ZubairAhmed M-ZubairAhmed changed the title added remove comment regex Fixes #171 : Remove comments before posting a notification Dec 31, 2019
@jasonblais jasonblais added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 1, 2020
@M-ZubairAhmed

M-ZubairAhmed commented Jan 6, 2020

Copy link
Copy Markdown
Member Author

Hi, can someone please review this

@hanzei

hanzei commented Jan 6, 2020

Copy link
Copy Markdown
Contributor

Hi @M-ZubairAhmed,

Thank you very much for your PR. All reviewers where on vacation last week, but will look into your PR in the next few days.

@M-ZubairAhmed

M-ZubairAhmed commented Jan 6, 2020

Copy link
Copy Markdown
Member Author

Thanks @hanzei for following up

mickmister
mickmister suggested changes Jan 6, 2020
View reviewed changes

@mickmister mickmister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@M-ZubairAhmed Just one comment on MustCompile usage. Otherwise LGTM!

Comment thread server/template.go Outdated
@aaronrothschild aaronrothschild removed the 1: PM Review Requires review by a product manager label Jan 6, 2020
aaronrothschild
aaronrothschild approved these changes Jan 6, 2020
View reviewed changes

@aaronrothschild aaronrothschild left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @M-ZubairAhmed for your contribution!

mickmister
mickmister approved these changes Jan 7, 2020
View reviewed changes

@mickmister mickmister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @M-ZubairAhmed!

Comment thread go.sum Outdated
@hanzei hanzei added this to the v0.13.0 milestone Jan 7, 2020
@mickmister
mickmister self-requested a review January 7, 2020 15:48
lieut-data
lieut-data requested changes Jan 8, 2020
View reviewed changes

@lieut-data lieut-data left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a request for clarity below.

Comment thread server/template.go Outdated
@M-ZubairAhmed

M-ZubairAhmed commented Jan 9, 2020

Copy link
Copy Markdown
Member Author

Added the test for multiline, please take a look

lieut-data
lieut-data approved these changes Jan 10, 2020
View reviewed changes

@lieut-data lieut-data left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @M-ZubairAhmed!

@lieut-data lieut-data removed the 2: Dev Review Requires review by a core committer label Jan 10, 2020
@M-ZubairAhmed

M-ZubairAhmed commented Jan 17, 2020

Copy link
Copy Markdown
Member Author

Hi, the checks are failing because of the coverage. What do i need to do?

@mickmister

mickmister commented Jan 17, 2020

Copy link
Copy Markdown
Contributor

@M-ZubairAhmed No action is necessary. The updated tests are sufficient for these changes, even though the code coverage tool says otherwise.

DHaussermann
DHaussermann approved these changes Jan 17, 2020
View reviewed changes

@DHaussermann DHaussermann left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and passed.
HTML comments from PR description are no longer included in the post delivered by the event
Briefly regression tested to ensured other lines in the description are still behaving as expected and support markdown.
LGTM!
Thanks @M-ZubairAhmed!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jan 17, 2020
@lieut-data
lieut-data merged commit de24b76 into mattermost:master Jan 17, 2020
@M-ZubairAhmed
M-ZubairAhmed deleted the GH-171 branch January 18, 2020 04:18
@mickmister mickmister mentioned this pull request Jul 12, 2020
Closed
@stefan-thothmind stefan-thothmind mentioned this pull request Jun 6, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lieut-data lieut-data lieut-data approved these changes
@DHaussermann DHaussermann DHaussermann approved these changes
+2 more reviewers
@aaronrothschild aaronrothschild aaronrothschild approved these changes
@mickmister mickmister mickmister approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

4: Reviews Complete All reviewers have approved the pull request

Projects

None yet

Milestone

v0.13.0

Development

Successfully merging this pull request may close these issues.

Remove comments before posting a notification

7 participants

@M-ZubairAhmed @hanzei @mickmister @aaronrothschild @lieut-data @DHaussermann @jasonblais