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

Add support for commenting on messages #5

Closed
wants to merge 1 commit into from

Conversation

brentc
Copy link

@brentc brentc commented Nov 21, 2013

Adds support for event:comment messages as outlined in the API Docs

@brentc
Copy link
Author

brentc commented Nov 26, 2013

Any comments? Anyone?

@brentc
Copy link
Author

brentc commented Nov 27, 2013

@lautis?

@lautis
Copy link
Contributor

lautis commented Nov 27, 2013

The API looks reasonable, but the inplementation might not work with flows with new id format (or at least the functionality is inconsistent with sending plain messages). I'll need to look of there's anything we could do on Flowdock API to fix this or does this require bigger overhaul for node-flowdock.

Ville Lautanala
Sent from my iPhone

On Wednesday 27. November 2013 at 19.43, Brent C wrote:

@lautis (https://github.com/lautis)?


Reply to this email directly or view it on GitHub (#5 (comment)).

@brentc
Copy link
Author

brentc commented Nov 27, 2013

I'm not sure what you mean with it not working with the "new id format". If you're referring to the first parameter being a combination of the org parameterized name and the flow parameterized name, I only did that to be consistent with the syntax of the session.message and session.status method parameters. The actual implementation takes the colon-separated names and uses them as the URL path as specified in the API docs for submitting a comment:

Comments can be posted to

POST /flows/:organization/:flow/messages/:message_id/comments

    orgFlowName = orgFlowName.replace(':', '/')
    @send "/flows/#{orgFlowName}/messages/#{messageId}/comments", data, callback

@Mumakil
Copy link
Contributor

Mumakil commented Nov 27, 2013

@brentc The issue here is that a while back the id's of new flows were changed from org:flow to a random string. The node-flowdock in general still makes assumptions about how to construct urls from the flow id, but it should instead use the api url of the flow provided by the api (see https://flowdock.com/api/flows), so the urls should be flow.url + "/messages/" + message_id + "/comments".

It is true that you can construct the orgFlowName by flow.organization.parameterized_name + ":" + flow.parameterized_name and give that as a parameter to the function, but the long term solution is to refactor the whole library a bit to use the correct url formats.

@brentc
Copy link
Author

brentc commented Nov 27, 2013

Ok... But right now sending a comment doesn't use the flow api url, it requires a URL that uses the org name and flow name as path parameters...

When do you...

  1. Expect the API to change to send comments via the flow api endpoint
  2. Expect to refactor this library to support that

And in the mean time.... Can this be accepted?

@lautis
Copy link
Contributor

lautis commented Dec 10, 2013

It might be reasonable to add a new endpoint to the Flowdock API, that would allow comments to be posted using flow id and message id. For example,

POST /comments
{"flow": "deadbeefdeadbeef", "message": 1234, "content": "comment content"}

@Mumakil
Copy link
Contributor

Mumakil commented Dec 16, 2013

Thanks @brentc! We added a new api endpoint to support commenting by flow id. I created a new pull request #6 that has a more unified api and up to date readme and copied the comment sending from this pull request there. We'll update the npm package asap.

@Mumakil Mumakil closed this Dec 16, 2013
@brentc
Copy link
Author

brentc commented Dec 16, 2013

@Mumakil Awesome! The new endpoint and refactored params seem much cleaner. Looking forward to #6 getting merged. 👍

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.

None yet

3 participants