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

Option to remove non permanent headers on redirects #857

Merged
merged 1 commit into from Mar 16, 2017

Conversation

Projects
None yet
5 participants
@hoffi
Copy link
Contributor

hoffi commented Feb 16, 2017

New option "for_followed_redirect" (Default is "true") on Driver#add_header to remove temporary headers on a redirect.

@twalpole

This comment has been minimized.

Copy link
Contributor

twalpole commented Feb 17, 2017

What's the use case for this?

@der-flo

This comment has been minimized.

Copy link

der-flo commented Feb 17, 2017

Glad you asked, I waited for the question.

In one of our Rails apps we have a splitted HTTP/HTTPS routing, the pages are only available via one protocol, not both. Poltergeist/Capybara provides no distinct HTTPS server, so every visit has to go to the standard HTTP server. An HTTP header X-Forwarded-SSL helps Rails with the routing decision.

So we provide two helper methods for our feature specs to annotate which clicks will result in HTTPS requests. Code says more than thousand words:

module SslAnnotation
  def next_page_has_ssl_url
    page.driver.add_header('X-Forwarded-SSL', 'on', permanent: false)
  end

  def next_page_has_ssl_url_but_redirects_to_nonssl_url
    page.driver.add_header('X-Forwarded-SSL', 'on',
                           permanent: false, for_followed_redirect: false)
  end
RSpec.configuration.include(SslAnnotation)

If you request the target URL (HTTPS) and it redirects to a HTTP URL, Poltergeist requests this URL also with the configured header. This needs to get disabled with the for_followed_redirect: false option.

@twalpole

This comment has been minimized.

Copy link
Contributor

twalpole commented Feb 20, 2017

This seems like a pretty specialized use case, I'm not sure it belongs in Poltergeist. You could probably implement your desired behavior through middleware added to your app in Capybara. @route - thoughts?

@route

This comment has been minimized.

Copy link
Member

route commented Mar 1, 2017

@hoffi Does permanent: false send headers to a redirected url and then removes them afterwards?

@hoffi

This comment has been minimized.

Copy link
Contributor Author

hoffi commented Mar 1, 2017

Headers added with permanent: false are still there after a redirect.

The headers will only removed when i make a new request e.g. after i click on a link.

@route

This comment has been minimized.

Copy link
Member

route commented Mar 1, 2017

@twalpole While it looks like an edge case for me too they kinda complement permanent: false. I think I'm for it, maybe we should just remove a confusion with method options permanent, for_followed_redirect

@twalpole

This comment has been minimized.

Copy link
Contributor

twalpole commented Mar 1, 2017

Ok -- so if we're going to go with this, the options are a problem since a user could specify- permanent: true, for_followed_redirect: false - which is meaningless. So I think we either need to error on invalid combinations or move it all under the permanent option -- maybe with allowed values of true, false, :no_redirect or something?

@hoffi

This comment has been minimized.

Copy link
Contributor Author

hoffi commented Mar 2, 2017

I think enhancing the permanent option would be easier to understand.

If this is ok for you we can change it.

@route

This comment has been minimized.

Copy link
Member

route commented Mar 8, 2017

@twalpole yea let's use yours combinations for permanent, I can't come up with anything else

@route

This comment has been minimized.

Copy link
Member

route commented Mar 8, 2017

@hoffi let's do it

@twalpole

This comment has been minimized.

Copy link
Contributor

twalpole commented Mar 14, 2017

@hoffi Any change at an updated PR for this? We need to release 1.14 soon to support the new Capybara being released today or tomorrow and it could go into the release.

@svenwin

This comment has been minimized.

Copy link

svenwin commented Mar 15, 2017

👍

@hoffi

This comment has been minimized.

Copy link
Contributor Author

hoffi commented Mar 15, 2017

@twalpole @route I have updated the code

@twalpole

This comment has been minimized.

Copy link
Contributor

twalpole commented Mar 15, 2017

@hoffi Thanks -- Please squash the commits

@hoffi hoffi force-pushed the Nix-wie-weg:remove_temp_header_on_redirect branch from b744b0b to ca5d0c4 Mar 15, 2017

@hoffi

This comment has been minimized.

Copy link
Contributor Author

hoffi commented Mar 16, 2017

@twalpole the commits are squashed, can it be merged?

@route

This comment has been minimized.

Copy link
Member

route commented Mar 16, 2017

LGTM

@route route merged commit 34f2f0c into teampoltergeist:master Mar 16, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.