2.13.0 #303
Merged
2.13.0 #303
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Hubot Hipchat adapter uses the `envelope.user.reply_to` property as the primary way for determining where a message should get sent. This does not appear to be necessary. It is an abnormal pattern that doesn't seem to be in any other popular adapters (Slack, Campfire). Perhaps this was necessary on some older versions of Hubot and was never updated. The `envelope.room` property seems to have all it needs to send the message to the right place. Removing the `reply_to` logic simplifies things quite a bit. This change also helps with hipchat#175 where messages would get sent to the wrong origins for some long running processes if the user happens to send a message in another room before the process can finish. The fix for the above problem in hipchat#207 broke the brain some. Typically, in other adapters, the `message.user` object returned will be a reference to the brain. Here, you can store data that you would want to persist. By cloning this object it breaks that ability and can affect plugins expecting that behavior. Found this issue while working with the `hubot-pager-me` plugin. This plugin assumes the `message.user` property is from the brain. When doing things like `pager me as [email protected]` it assigns it to that property where it should persist but does not. This does work in other adapters though like Shell and Campfire. Finally, one other added benefit these changes bring are a level of consistency to the `user.room` property. When `changePresence()` was called it would populate the brain and set the room to the JID version which looks something like `[email protected]`. When a message was getting handled it would filter the room through `roomNameFromJid()` which would transform the room name to a friendlier format like `foo_bar`. Now, the `user.room` property will always bet set to the JID version.
|
@rbergman Let me know if there is anything I can do to assist with getting this merged. |
This was referenced Feb 12, 2018
|
Great job @benhanzl on this PR! Only issue is that it seems to not be published on NPM yet 😢 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #272 #273 #277 #278.
Proposed Changes
I have merged a few of the outstanding pull requests and propose creating the 2.13.0 release with them. I focused on the pull requests that did NOT introduce new features or were improvements that in my opinion needed some further discussion.
This release:
node-xmpp-clientto the latest version (3.2.0). From @samcday in Bump node-xmpp-client to ^3.0.0 #272.userobject to include the correct user information. From @awiddersheim in Simplify sending and return users from the brain #277. This also resolved Use a normal object when setting up the author object. #273 from @Wilfred and Get user object from brain that matches the current channel #287 from @seanfitzgeraldsc. The new response looks like:Testing
I tested each of these code changes specifically on a production HipChat instance. I have also had this version deployed to our production instance for the past two weeks without issue.
Future
Once this is merged and published to npm, I will start the discussion with the other open pull requests to get them resolved.