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

Action Controller support for PUT requests #181

Merged
merged 1 commit into from Mar 29, 2021
Merged

Action Controller support for PUT requests #181

merged 1 commit into from Mar 29, 2021

Conversation

rvowles
Copy link
Contributor

@rvowles rvowles commented Mar 5, 2021

Most of the other request proxies support PUT
requests and we need this gem to also support it
for Action Controller. This addresses issue #180

It includes the modified Action Controller proxy
and fixes the tests.

@rvowles
Copy link
Contributor Author

rvowles commented Mar 5, 2021

It surprises me that DELETE is not also generically supported? We appear to not require it, but...

@rvowles
Copy link
Contributor Author

rvowles commented Mar 7, 2021

There was already test coverage for PUT that was IMHO wrong, I simply fixed it so I am not expecting the code coverage to change.

@xtagon xtagon self-requested a review March 28, 2021 20:02
@@ -72,6 +72,10 @@ def parameters_for_signature
reject { |kv| kv[0] == 'oauth_signature'}
end

def raw_post_signature
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be raw_post_signature? with a ? for convention (it returns a boolean). Minor nitpick though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed!

@@ -109,7 +109,7 @@ def test_that_proxy_simple_post_request_works_with_mixed_params
def test_that_proxy_simple_put_request_works_with_mixed_params
request_proxy = request_proxy(:put, {'key'=>'value'}, {'key2'=>'value2'})

expected_parameters = [["key", "value"]]
expected_parameters = [["key", "value"],["key2", "value2"]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I see what you mean, the original test looks wrong and this fixes it. LGTM.

Most of the other request proxies support PUT
requests and we need this gem to also support it
for Action Controller. This addresses issue #180

It includes the modified Action Controller proxy
and fixes the tests.
Copy link
Contributor

@xtagon xtagon left a comment

Choose a reason for hiding this comment

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

LGTM

@xtagon xtagon merged commit 243cf06 into oauth-xx:master Mar 29, 2021
@rvowles rvowles deleted the put-me-pull-you branch March 29, 2021 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants