Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-14341 Add client-side messageWillBePosted hook #2526

Merged
merged 4 commits into from
Mar 25, 2019
Merged

MM-14341 Add client-side messageWillBePosted hook #2526

merged 4 commits into from
Mar 25, 2019

Conversation

hmhealey
Copy link
Member

This hook is needed for the NPS plugin because we wanted to show a confirmation modal for written feedback so that users know their feedback is being sent to Mattermost.

Ticket Link

https://mattermost.atlassian.net/browse/MM-14341

Checklist

  • Added or updated unit tests (required for all new features)

@hanzei
Copy link
Contributor

hanzei commented Mar 21, 2019

Hey @hmhealey,

Thanks for requesting my review, but I'm not familiar enough with react to review this. I will defer to @crspeller.

@hanzei hanzei requested review from crspeller and jespino and removed request for hanzei and levb March 21, 2019 22:16
@hanzei
Copy link
Contributor

hanzei commented Mar 25, 2019

@levb Could you please help out and review this PR? Lev is OOO atm and this should go into 5.10.

Copy link
Member

@jespino jespino left a comment

Choose a reason for hiding this comment

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

LGTM. One small comment

actions/views/create_comment.jsx Show resolved Hide resolved

if (hookResult.error) {
this.setState({
serverError: hookResult.error,
Copy link
Member

Choose a reason for hiding this comment

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

Not really a server error.

Copy link
Member Author

Choose a reason for hiding this comment

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

The name is bad, but I didn't want to go through with changing it elsewhere. Ideally, I think we'd just have one "error" state that controls the error message instead of multiple different fields depending on where the error came from, but seeing as there's already this.state.serverError and this.state.textError, I didn't want to add another one.

@amyblais
Copy link
Member

A quick reminder that tomorrow (v5.10 Feature Complete) is the last day to merge feature PRs if we want this for v5.10.

@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Mar 25, 2019
@hmhealey hmhealey merged commit 9681600 into master Mar 25, 2019
@hmhealey hmhealey deleted the mm14341 branch March 25, 2019 17:26
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 25, 2019
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Mar 29, 2019
stevepartridge pushed a commit to stevepartridge/mattermost-webapp that referenced this pull request Mar 30, 2019
* MM-14341 Add client-side messageWillBePosted hook

* Fix and add another test

* Update CreatePost unit tests

* Handle falsey results from MessageWillBePosted hooks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants