-
-
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
Rename bogus gateways #2000
Rename bogus gateways #2000
Conversation
Should we provide something like: Spree::Gateway::Bogus = Spree::PaymentMethod::BogusCreditCard or even class Spree::Gateway::Bogus < Spree::PaymentMethod::BogusCreditCard
def initialize(*args)
#deprecation notice
super
end
end until we're ready for a major version release? |
@gmacdougall I did this 9443aec ;) |
That'll learn me for reading PR's too fast! |
No worries, this is a large changeset |
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.
Oh the clarity. Thank you!
(as discussed IRL: Add a migration that calls the rake task to make upgrading on multiple deploys easier).
To remove confusion around payment methods, gateways and providers we rename the `Spree::Gateway::Bogus` and `Spree::Gateway::BogusSimple` payment methods into `Spree::PaymentMethod::BogusCreditCard` and `Spree::PaymentMethod::SimpleBogusCreditCard`.
Use `Spree:: PaymentMethod::BogusCreditCard` and `Spree::PaymentMethod::SimpleBogusCreditCard ` instead.
d4862fd
to
1e5070c
Compare
|
||
private | ||
|
||
def update(from: , to:) |
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.
Space found before comma.
|
||
def down | ||
GATEWAY_MAPPING.inject(0) do |count, mapping| | ||
count += update(from: mapping[1], to: mapping[0]) |
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.
Useless assignment to variable - count. Use + instead of +=.
class << self | ||
def up | ||
GATEWAY_MAPPING.inject(0) do |count, mapping| | ||
count += update(from: mapping[0], to: mapping[1]) |
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.
Useless assignment to variable - count. Use + instead of +=.
1e5070c
to
0aca193
Compare
@mamhoff made a great comment IRL about making the migration class available from the outside (eg. |
0aca193
to
df02fd9
Compare
@mamhoff done! |
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 left a couple of comments, in general everything seems fine to me!
module Migrations | ||
class RenameGateways | ||
DEFAULT_MAPPING = [ | ||
['Spree::Gateway::Bogus', 'Spree::PaymentMethod::BogusCreditCard'], |
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 not use an hash here?
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.
A good catch, as the keys are unique this will work as well. Thanks!
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.
Changed to Hash, thanks!
module Spree | ||
describe PaymentMethod::BogusCreditCard, type: :model do | ||
let(:bogus) { create(:credit_card_payment_method) } | ||
let!(:cc) { create(:credit_card, payment_method: bogus, gateway_customer_profile_id: "BGS-RERTERT") } |
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 this file is needed?
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 just renamed the file.
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.
Ah right! Now the old files just contains deprecation specs.
This file is empty since: 1fbbdcc
and this seems to be the reason this file was added for:
970eeb6#diff-4f10b6436800df7a8b39eff13961d74f
Maybe we don't need it anymore? Or can it be useful to understand if a payment method can be created with BogusCreditCard
payment method? My feeling is that this file is missing some specs, like this one, and of course we can add that with another PR, if they are needed. 😄
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 say let's create a separate PR
Running rake solidus:migrations:rename_gateways:up helps migrating your data to new bogus payment method class names. rake solidus:migrations:rename_gateways:down reverts this. Also includes a migration that invokes that task, so you don't need to care when deploying this change.
df02fd9
to
04dffbf
Compare
To remove confusion around payment methods, gateways and providers we rename the
Spree::Gateway::Bogus
andSpree::Gateway::BogusSimple
payment methods intoSpree::PaymentMethod::BogusCreditCard
andSpree::PaymentMethod::SimpleBogusCreditCard
.