Skip to content

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Jan 5, 2017

Summary

Regression introduced by ae29142 / 8363b87.

Previously, cookies were only updated on GET requests. Now we will
update the helper for all requests, as part of process. Added
regression tests for all available HTTP method helpers in
ActionController::TestCase.

Other Information

Regression introduced by ae29142 / 8363b87.

Previously, cookies were only updated on `GET` requests. Now we will
update the helper for all requests, as part of `process`. Added
regression tests for all available HTTP method helpers in
`ActionController::TestCase`.
@rafaelfranca
Copy link
Member

Backported in cf51e74

rafaelfranca added a commit that referenced this pull request Jan 6, 2017
Update `cookies` helper on all HTTP requests
@maclover7 maclover7 deleted the jm-fix-27584 branch January 6, 2017 13:57
@rafaelfranca
Copy link
Member

rafaelfranca commented Jan 12, 2017

This caused a regression:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem "rails", github: "rails/rails", branch: "5-0-stable"
  gem "pry-byebug"
end

require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = File.dirname(__FILE__)
  config.session_store :cookie_store, key: "cookie_store_key"
  secrets.secret_token    = "secret_token"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  routes.draw do
    resources :users, only: [:index]
  end
end

class UsersController < ActionController::Base
  def index
    unless cookies[:name].present?
      cookies[:name] = "Alice"
    end
    render plain: "Home"
  end
end

require "minitest/autorun"
require "rack/test"

class UsersControllerTest < ActionController::TestCase
  setup do
    @controller = UsersController.new
    @routes = Rails.application.routes
  end

  test '#index sets a cookie if it is not already present' do
    get :index
    assert_response :ok
    assert_equal "Alice", cookies[:name]

    cookies[:name] = "Bob"

    get :index
    assert_response :ok
    assert_equal "Bob", cookies[:name]
  end
end

@davidcornu was taking a look but I'll revert it for now.

rafaelfranca added a commit that referenced this pull request Jan 12, 2017
This reverts commit cf51e74.

Reason: It caused regression and the test case is in the issue.
rafaelfranca added a commit that referenced this pull request Jan 12, 2017
This reverts commit 5eff7a9, reversing
changes made to 5f03172.

Reason: It caused a regression. The test case is on the PR.
@mrageh
Copy link
Contributor

mrageh commented Feb 1, 2017

@rafaelfranca I ran the regression script above on the latest master and it still fails, so I'm not sure if this patch by @maclover7 caused the regression.

@devohh
Copy link

devohh commented Feb 19, 2017

@rafaelfranca @maclover7 @mrageh Any news ? I still can't access to a signed cookie from my tests.

@devohh
Copy link

devohh commented Feb 28, 2017

@rafaelfranca Hello Rafael, I just realized how useless my precedent message was. It's my first participation on Github. Sorry for my inexperience. I just updated to 5.0.2.rc1.
When I run my IntegrationTest, I get this error :

Error:
CartsControllerTest#test_should_get_cart:
NoMethodError: undefined method `signed' for #<Rack::Test::CookieJar:0x00555a9c6ad320>
    test/controllers/carts_controller_test.rb:18:in `block in <class:CartsControllerTest>'

test/controllers/carts_controller_test.rb:18

assert_not_nil cookies.signed['cart_id']

At first I thought it was not the appropriate way to test the presence of a signed cookie. And then, I read this message.

Note : I have successfully tested the presence and/or the value of a simple cookie.
My configuration : ruby 2.4.0p0 && Rails 5.0.2.rc1

Hope it helps. Otherwise, could you show me the way.

skipkayhil added a commit to skipkayhil/rails that referenced this pull request Jul 17, 2024
This reverts commit 6902ca5.

This was originally reverted due to a reported regression, however the
regression appeared to be present without this patch as well (and was
fixed in a later [commit][1].

[1]: ca937c5
skipkayhil added a commit to skipkayhil/rails that referenced this pull request Sep 24, 2024
This reverts commit 6902ca5.

This was originally reverted due to a reported regression, however the
regression appeared to be present without this patch as well (and was
fixed in a later [commit][1].

[1]: ca937c5
rafaelfranca added a commit that referenced this pull request Sep 26, 2024
Reapply "Merge pull request #27586 from maclover7/jm-fix-27584"
rafaelfranca added a commit that referenced this pull request Sep 26, 2024
Reapply "Merge pull request #27586 from maclover7/jm-fix-27584"
DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024
This reverts commit 6902ca5.

This was originally reverted due to a reported regression, however the
regression appeared to be present without this patch as well (and was
fixed in a later [commit][1].

[1]: ca937c5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants