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

Allow Redirect Requests #1881

Merged
merged 3 commits into from
Jan 28, 2017
Merged

Allow Redirect Requests #1881

merged 3 commits into from
Jan 28, 2017

Conversation

thiagotalma
Copy link
Contributor

Use Case: The user fills out a form that is posted to the webhook and then he is redirected to the success page.

PS: I'm not a Ruby programmer, you know, right? Please have patience with my code 😉

@dsander
Copy link
Collaborator

dsander commented Jan 22, 2017

I think that is a good addition! Can you add some specs for the redirect case to the web_request_controller_spec and for the validation of the agent?

@thiagotalma
Copy link
Contributor Author

@dsander I do not know how to write the redirect tests. Can you help me please?

@thiagotalma
Copy link
Contributor Author

@cantino
Feel free to start a new PR with an attempt at this, and I can help suggest how to do it there.

I did it! :)

@dsander
Copy link
Collaborator

dsander commented Jan 22, 2017

@thiagotalma I think that test should work:

diff --git a/spec/controllers/web_requests_controller_spec.rb b/spec/controllers/web_requests_controller_spec.rb
index addb912..0c64750 100644
--- a/spec/controllers/web_requests_controller_spec.rb
+++ b/spec/controllers/web_requests_controller_spec.rb
@@ -10,7 +10,7 @@ describe WebRequestsController do
         memory[:web_request_values] = params
         memory[:web_request_format] = format
         memory[:web_request_method] = method
-        ["success", 200, memory['content_type']]
+        ["success", (options[:status] || 200).to_i, memory['content_type']]
       else
         ["failure", 404]
       end
@@ -85,6 +85,13 @@ describe WebRequestsController do
     expect(response.headers['Content-Type']).to eq('application/json; charset=utf-8')
   end

+  it 'should redirect correctly' do
+    @agent.options['status'] = 302
+    @agent.save
+    post :handle_request, params: {:user_id => users(:bob).to_param, :agent_id => @agent.id, :secret => "my_secret"}, format: :json
+    expect(response).to redirect_to('success')
+  end
+
   it "should fail on incorrect users" do
     post :handle_request, params: {:user_id => users(:jane).to_param, :agent_id => @agent.id, :secret => "my_secret", :no => "go"}
     expect(response).to be_missing

@thiagotalma
Copy link
Contributor Author

@dsander done!

Thanks!

Copy link
Collaborator

@dsander dsander left a comment

Choose a reason for hiding this comment

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

Thanks looks good to me, looks like the WebhookAgent could use some additional specs but that does not need to happen in this PR.

@@ -103,6 +103,10 @@ def validate_options
if options['code'].present? && options['code'].to_s !~ /\A\s*(\d+|\{.*)\s*\z/
errors.add(:base, "Must specify a code for request responses")
end

if options['code'].in?(['301', '302']) && !options['response'].present?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe options['code'].to_s in case they used an int?

render plain: content, :status => status || 200, :content_type => content_type || 'text/plain'
status = status || 200

if status.in?([301, 302])
Copy link
Member

Choose a reason for hiding this comment

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

Does agent.trigger_web_request(request) always return status as an integer?

@thiagotalma
Copy link
Contributor Author

@cantino fixed!

@cantino cantino merged commit 51b9bbc into huginn:master Jan 28, 2017
@cantino
Copy link
Member

cantino commented Jan 28, 2017

Worked for me, thanks @thiagotalma, this is a handy contribution.

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