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

interpolated response #1682

Merged
merged 1 commit into from
Sep 15, 2016
Merged

interpolated response #1682

merged 1 commit into from
Sep 15, 2016

Conversation

thiagotalma
Copy link
Contributor

@thiagotalma thiagotalma commented Sep 12, 2016

improve custom response with liquid JSONPath

@cantino
Copy link
Member

cantino commented Sep 12, 2016

This looks like JSONPath, not Liquid. Can you not get that you need with Liquid?

@thiagotalma
Copy link
Contributor Author

@cantino Excuse me, I thought of one thing and write another.
The idea is to be JSONPath, maintaining consistency with the payload_path.

@dsander
Copy link
Collaborator

dsander commented Sep 12, 2016

@thiagotalma What is your use case here? I don't understand why JSONPath is required, as I see it Liquid should sufficient.

We generally use JSONPath whenever an Agent needs to work with a hash or an array (like for the payload in this Agent) and Liquid when the output can be a string.

@thiagotalma
Copy link
Contributor Author

In fact, I'm a problem that will end up coming to many people who try to use webhook with the Facebook API.
See how the request is made:
http://requestb.in/pf1twqpf?hub.mode=subscribe&hub.challenge=1275892619&hub.verify_token=s
The answer should be the value of hub.challenge.

The dot in the parameter name ruined my weekend! 😆
I swear I spent my weekend trying to work around the problem and found no solution.

@dsander
Copy link
Collaborator

dsander commented Sep 12, 2016

@thiagotalma I see, that is indeed a bit tricky, does {{["hub.challenge"]}} work?

@thiagotalma
Copy link
Contributor Author

@dsander But what is the context of interpolation?
It seems that the liquid does not take the parameter values.

Example:

{
  "secret": "3b9661ffaecb41",
  "expected_receive_period_in_days": 1,
  "payload_path": ".",
  "response": "{{ key }}",
  "verbs": "post,get",
  "code": "200"
}

Request: https://myuginn.herokuapp.com/users/1/web_requests/115/3b9661ffaecb41?key=val

Response: EMPTY

@cantino
Copy link
Member

cantino commented Sep 12, 2016

See if {{["hub.challenge"]}} works if you delete response_message and change [response_message, code] to:

[interpolated(params)['response'] || 'Event Created', code]

@cantino
Copy link
Member

cantino commented Sep 12, 2016

If so, I think that'd be a reasonable change with a spec.

@thiagotalma
Copy link
Contributor Author

[OFFTOPIC]
@cantino @dsander how do I run the tests using the docker-compose in multiple-proccess?
I have done tests on Travis, commiting code here 😆

I think the project needs a specific wiki to teach contributors the process of setting the environment and run the tests before opening a PR. I'm not the RoR world (i'm PHP) and take a whipping to get code something to help the project. A specific docker-compose for this would be perfect!!
[/OFFTOPIC]

@dsander
Copy link
Collaborator

dsander commented Sep 12, 2016

Right, I thought the interpolation context was already set.

@thiagotalma The docker containers are running in the production environment, it's not possible to run the specs there. The Wiki has a few links on how to set up a local development environment: https://gist.github.com/mjhea0/b6b58eefc38985380ff9 and https://github.com/cantino/huginn/wiki/Novice-setup-guide

@thiagotalma
Copy link
Contributor Author

thank you guys!
code fixed and tests ok!

(@dsander How about a docker-compose ready for development and testing? ☺️ )

@@ -88,7 +88,7 @@ def receive_web_request(params, method, format)
create_event(payload: payload)
end

[response_message, code]
[interpolated(params)['response'] || interpolated['response'] || 'Event Created', code]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need interpolated['response'] too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To maintain compatibility with the current context... or not?

If you find it is not necessary, I remove.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need it. @dsander?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, we don't need it.

If we want to default to Event Created when the interpolated result is an empty string interpolated(params)['response'].presence || 'Event Created' would be an option.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cantino done. But how @dsander indicated, we have no empty response option. Look at Travis ...

@cantino
Copy link
Member

cantino commented Sep 15, 2016

Interesting. I wonder if we really want the ability to return an empty response body? I guess you should remove presence for now to make it stay the same. Sorry :)

@thiagotalma
Copy link
Contributor Author

done!

@cantino cantino merged commit 4fdd69c into huginn:master Sep 15, 2016
@cantino
Copy link
Member

cantino commented Sep 15, 2016

Thanks @thiagotalma!

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.

3 participants