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

clarify that a room token might be the expected parameter #1012

Merged
merged 2 commits into from
Oct 9, 2015

Conversation

alininja
Copy link

Hi,

Thanks for such an amazing project. :)

I was testing out a hubot installation that uses the let's chat adapter. So I'm not sure if this change only applies to the let's chat adapter or if it is applicable to all adapters. That also means, I'm not sure if this is the correct place to update the docs.

Anyways, I couldn't get hubot to "hear" my curl request using just the chat room name. Instead I needed to pass the chat room token to it.

Sorry, if this document change is way off.

Thanks again!

@@ -380,7 +380,7 @@ The most common use of this is for providing HTTP end points for services with w

```coffeescript
module.exports = (robot) ->
robot.router.post '/hubot/chatsecrets/:room', (req, res) ->
robot.router.post '/hubot/chatsecrets/:room_name_or_token', (req, res) ->
room = req.params.room
Copy link
Collaborator

Choose a reason for hiding this comment

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

req.params.room_name_or_token

Copy link
Member

Choose a reason for hiding this comment

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

What @michaelansel is pointing out is the route uses room_name_or_token, but then accesses req.params.room. It should be updated req.params.room_name_or_token.

@michaelansel
Copy link
Collaborator

Hmm... I see the confusion for chat services that have a unique ID for rooms that isn't the room name. I'm not sure whether this makes that much clearer though.

@bkeepers Do you have any good ideas on a better way of expressing that concept? IIRC Campfire does the same thing.

@bkeepers
Copy link
Contributor

Hmm, I'm not sure I understand what this is clarifying. Currently, req.params.room can be either a name or a token, depending on the chat service, correct?

@alininja
Copy link
Author

Yes, room can be a name or a token. The only thing I was trying to prevent was for new users to assume that room only means name.

Maybe instead of changing it from room to room_name_or_token, just a note letting users know that room can be a token?

Not sure, but I'm just pointing this out.

Thanks again! 😄

@technicalpickles
Copy link
Member

Maybe just include a comment like "# the expected value of room is going to vary by adapter, it might be a numeric id, name, token, or some other value`.

@technicalpickles
Copy link
Member

@alininja one last thing... Your commits aren't attributed to your user. Can you work through https://help.github.com/articles/why-are-my-commits-linked-to-the-wrong-user/ to make sure your commits are attributed to your user? Once you are done, you should be able to work through https://help.github.com/articles/changing-author-info/ to update this branch's history.

@alininja
Copy link
Author

alininja commented Sep 9, 2015

@technicalpickles it's okay if the commits aren't attributed to my user. 😄

technicalpickles added a commit that referenced this pull request Oct 9, 2015
clarify that a room token might be the expected parameter
@technicalpickles technicalpickles merged commit e6102b2 into hubotio:master Oct 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants