-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix issue not updating payment method type on admin #2788
Fix issue not updating payment method type on admin #2788
Conversation
Thanks! Can we also add a regression test for this please? |
@kennyadsl done, please let me know what do you think |
|
||
it 'updates the resource' do | ||
subject | ||
Spree::PaymentMethod.all.reload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why grab all the payment methods, reload the data, and then not use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reloading the payment methods because I got this error if I use the payment_method
created before:
pry(#<RSpec::ExampleGroups::SpreeAdminPaymentMethodsController::Update>)> payment_method.reload
ActiveRecord::RecordNotFound: Couldn't find Spree::PaymentMethod::StoreCredit with 'id'=1 [WHERE "spree_payment_methods"."type" IN ('Spree::PaymentMethod::StoreCredit')]
from /Users/kaifan11/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/activerecord-5.2.0/lib/active_record/relation/finder_methods.rb:346:in `raise_record_not_found_exception!'
I think it's because of the payment method reference, any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird. I don't think you're using the reference anymore though.
} | ||
end | ||
|
||
subject { put :update, params: params } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a before
. Then you wouldn't have to call subject
at the beginning of each spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@BenMorganIO because of the error I didn't use the payment_method
reference, for example, here:
describe "#update" do
# Regression test for https://github.com/solidusio/solidus/issues/2789
let(:payment_method) { create(:store_credit_payment_method) }
let(:params) do
{
id: payment_method.id,
payment_method: {
name: 'Check',
type: 'Spree::PaymentMethod::Check'
}
}
end
it 'redirects to payment method edit page' do
put :update, params: params
expect(response).to redirect_to(spree.edit_admin_payment_method_path(payment_method))
end
it 'updates the resource' do
expect { put :update, params: params }.to change { payment_method.reload.name }.from('Store Credit').to('Check')
end
end
I'm getting this:
ActiveRecord::RecordNotFound: Couldn't find Spree::PaymentMethod::StoreCredit with 'id'=1 [WHERE "spree_payment_methods"."type" IN ('Spree::PaymentMethod::StoreCredit')]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to instantiate payment_method before the specs. Since it's used by both tests, you can do:
let!(:payment_method) { ... }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing that I got another error
1) Spree::Admin::PaymentMethodsController#index respects the order of payment methods by position
Failure/Error: expect(assigns(:payment_methods).to_a).to eql([second_method, first_method])
expected: [#<Spree::GatewayWithPassword id: 3, type: "Spree::GatewayWithPassword", name: "Second", description:...=>"1235"}, preference_source: nil, position: 2, available_to_users: true, available_to_admin: true>]
got: [#<Spree::GatewayWithPassword id: 3, type: "Spree::GatewayWithPassword", name: "Second", description:...=>"1235"}, preference_source: nil, position: 3, available_to_users: true, available_to_admin: true>]
(compared using eql?)
Diff:
@@ -1,3 +1,4 @@
[#<Spree::GatewayWithPassword id: 3, type: "Spree::GatewayWithPassword", name: "Second", description: nil, active: true, deleted_at: nil, created_at: "2018-07-03 19:13:28", updated_at: "2018-07-03 19:13:28", auto_capture: nil, preferences: {:server=>"test", :test_mode=>true, :password=>"1235"}, preference_source: nil, position: 1, available_to_users: true, available_to_admin: true>,
- #<Spree::GatewayWithPassword id: 2, type: "Spree::GatewayWithPassword", name: "First", description: nil, active: true, deleted_at: nil, created_at: "2018-07-03 19:13:28", updated_at: "2018-07-03 19:13:28", auto_capture: nil, preferences: {:server=>"test", :test_mode=>true, :password=>"1235"}, preference_source: nil, position: 2, available_to_users: true, available_to_admin: true>]
+ #<Spree::GatewayWithPassword id: 1, type: "Spree::GatewayWithPassword", name: "Bogus", description: nil, active: true, deleted_at: nil, created_at: "2018-07-03 19:13:28", updated_at: "2018-07-03 19:13:28", auto_capture: nil, preferences: {:server=>"test", :test_mode=>true, :password=>"haxme"}, preference_source: nil, position: 2, available_to_users: true, available_to_admin: true>,
+ #<Spree::GatewayWithPassword id: 2, type: "Spree::GatewayWithPassword", name: "First", description: nil, active: true, deleted_at: nil, created_at: "2018-07-03 19:13:28", updated_at: "2018-07-03 19:13:28", auto_capture: nil, preferences: {:server=>"test", :test_mode=>true, :password=>"1235"}, preference_source: nil, position: 3, available_to_users: true, available_to_admin: true>]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable. Thanks for the contribution!
@BenMorganIO thanks for the comments and @gmacdougall thanks, glad to help 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
Issue: http://recordit.co/EkfGNYkyqJ
Deleting the
type
param at that point is causing that on update method it's not changing the payment method type