Mattermost-server: Allow plugins to dismiss posts through the MessageWillBePosted hook

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Allow plugins to dismiss posts through the MessageWillBePosted hook. The mattermost-server/plugin package should define and export a new, const error to be used when a post should be dismissed.

For additional context, see https://community.mattermost.com/core/pl/mwdht9e3a3b59x8q3dhtb1xajo

[jasonblais]

@carmo-evan here's the help wanted ticket for the changes we discussed couple of days ago

[lieut-data]

Posting this here for extra context:

I like the semantics of wanting to dismiss a post, but I wonder if we could just reserve and export an error code to mean dismiss in this case instead of having to create an "invalid" post as such.

[carmo-evan]

@jasonblais thanks! I'll work on it.

[carmo-evan]

After talking to @lieut-data , it was decided we'd signal the dismissal by passing in a custom error message that could be added to this if statement:
https://github.com/mattermost/mattermost-redux/blob/master/src/actions/posts.js#L237-L238

However, in testing I noticed that this action is not quite working:
https://github.com/mattermost/mattermost-redux/blob/master/src/actions/posts.js#L240

My theory is that it doesn't work because it returns a function, and not an action:
https://github.com/mattermost/mattermost-redux/blob/master/src/actions/posts.js#L907

If I substitute it for this function, it works just fine:
https://github.com/mattermost/mattermost-redux/blob/master/src/actions/posts.js#L122

I'd like to know from some redux-savvy people if this makes sense @jwilander @hmhealey

[jwilander]

@carmo-evan we use redux-thunk as a middleware to allow returning functions as actions. That should work fine. Do you have a link to your changes? I can better help explain what might be happening if I can see those

[carmo-evan]

My change is pretty minor. It'd be just adding a new error id to that if statement, as follows:

if (error.server_error_id === 'api.post.create_post.root_id.app_error' || error.server_error_id === 'plugin.message_will_be_posted.dismiss_post' || error.server_error_id === 'api.post.create_post.town_square_read_only' ) { actions.push(removePost(data)); } else { actions.push(receivedPost(data)); }

[carmo-evan]

This is the behavior I'm experiencing with removePost(), which is the current version of the code. The pending post with the correct error message never gets removed.

And this is what happens with postRemoved() instead. It gets removed as expected.

[carmo-evan]

@hanzei @lieut-data can you review my comments above please? We'll need to merge a redux change together with the server changes in https://github.com/mattermost/mattermost-server/pull/10921

[lieut-data]

@lieut-data, can you setup the https://github.com/reduxjs/redux-devtools and watch the sequence of actions (noting that some of them might be batched)? Time-travel can come in handy here to narrow down just what is happening.

[carmo-evan]

Will do it tonight.

[carmo-evan]

@lieut-data it looks like an issue of incompatibility of redux-batch-actions and thunks
https://github.com/tshelburne/redux-batched-actions/issues/3

[carmo-evan]

Implementing this polyfill suggestion fixes the issue.

[lieut-data]

Great investigation -- that makes a lot of sense. The polyfill seems reasonable, though I find some of this state flow unnecessarily complex and wonder "if there's a better way." Suggest pinging @hmhealey for feedback when you have a PR for review.

[carmo-evan]

Here's a draft https://github.com/mattermost/mattermost-redux/pull/840/files