Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Mattermost adapter #192

Merged
merged 4 commits into from
Aug 7, 2019
Merged

Fix Mattermost adapter #192

merged 4 commits into from
Aug 7, 2019

Conversation

blag
Copy link
Contributor

@blag blag commented Aug 3, 2019

This is the fix for the Mattermost adapter from this forum post.

I have tested this manually with a minimal Mattermost server in a VM.

The slack-attachment event was removed from the hubot-matteruser dependency in PR 71, but our Mattermost adapter did not get updated to avoid using that.

I switched over to using the send method from hubot-matteruser.

Note: That methods accepts two parameters: envelope and a variadic parameter strings, but contrary to the name, the strings parameter also works just fine if each "string" is a message-like object, because it calls postMessage from mattermost-client, and that handles objects:

    postMessage(msg, channelID) {
        const postData = {
            message: msg,
            file_ids: [],
            create_at: 0,
            user_id: this.self.id,
            channel_id: channelID
        };

        if (typeof msg === 'string') {
          postData.message = msg;
        } else {
          postData.message = msg.message;
          if (msg.props) {
            postData.props = msg.props;
          }
          if (msg.file_ids) {
            postData.file_ids = msg.file_ids;
          }
        }

(reformatted that code slightly to improve readability)

Once #190 and #191 are merged, I will either rebase and merge this with GitHub, or I will manually rebase this branch on #191, force push, and merge it.

@blag blag requested a review from arm4b August 3, 2019 00:30
@blag blag mentioned this pull request Aug 5, 2019
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

We'll need a Changelog for this PR.
Based on #193 release, Changelog sync-up in master should eventually happen.

@blag blag force-pushed the fix-mattermost-adapter branch from 23b6b21 to 44ca041 Compare August 6, 2019 20:52
@blag
Copy link
Contributor Author

blag commented Aug 6, 2019

@armab I added a change to the changelog. Since I won't ever be merging the changes from #193 directly into master, the changelogs will differ, but this and #193 both fix the same thing. Let me know if this is acceptable to you.

@arm4b
Copy link
Member

arm4b commented Aug 7, 2019

As mentioned in #193, master should have in its Changelog record about every release that happened in the project. It's not OK to compose master Changelog like 0.9.7 release never happened.

So let's please mention in Changelog a few notes about 0.9.7 for our users to have this information visible in history. This is not any worse then split-brain with patch and I hope its first and last time we're doing this.

Additionally, are you going to release v0.10.0? Please prepare the package version and changelog for that as well in PR.

@blag
Copy link
Contributor Author

blag commented Aug 7, 2019

I closed #193. We won't do a split brain release.

I have a version bump local branch ready to go, but I'd like to merge this and then rebase that branch on top of master before I push it and open a PR. This is so I don't have to do a rebase and force push (since I know you don't like those).

@blag blag merged commit 9815ee7 into master Aug 7, 2019
@blag blag deleted the fix-mattermost-adapter branch August 7, 2019 18:34
@arm4b
Copy link
Member

arm4b commented Aug 7, 2019

Sure, works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants