Improve payment service providers switching errors #3837

luca-landa · 2020-11-09T21:31:02Z

Description

This PR is a solution for #3802.

Since old payment methods can't just be deleted after switching payment service provider, I provided a script to reset their type to a default Spree::PaymentMethod, to avoid breaking Single Table Inheritance while building ActiveRecord instances.
I added a type_before_removal field which will store the actual type value when the script is run.

Also, the script sets active, available_to_admin and available_to_users flags to false.

Moreover, I replaced the typical ActiveRecord::SubclassNotFound with a more meaningful error, which should save time to developers facing this issue and points them to the script.

If I got something wrong please tell me, this was my first experience with the project :)

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

core/spec/models/spree/payment_method_spec.rb

luca-landa · 2020-11-10T12:42:56Z

My bad, I didn't pay attention to failing checks yet.
I currently have no idea of what is causing the errors, I have run the full suite locally on postgresql without any trouble.
I will investigate and fix it.

spaghetticode · 2020-11-11T16:27:12Z

@luca-landa I'm wondering if we may be interested in keeping for reference the original payment method information.

For example, if we change the boolean field to a string, we can populate it with the previous type class name... what do you think?

luca-landa · 2020-11-11T18:07:41Z

@spaghetticode it seems a good idea to keep track of the previous type.

But personally I would add it in another field, instead of converting the deprecated field.
I thought that the new flag could be useful to query for all current payment methods or for the deprecated ones. For these purposes, I wouldn't find it intuitive to write .where.not(previous_type: nil), I would much rather do .where(deprecated: false) instead.

Do you think it can be a better solution?

spaghetticode · 2020-11-13T09:50:57Z

@luca-landa I don't have a strong opinion on this, I think there are pros and cons with both approaches.

I don't see where.not(previous_type: nil) as a big issue, as it would probably just end up in a scope such as
scope :deprecated, -> {where.not(deprecated_type: nil)}
Same goes for instance methods such as

def deprecated?
  deprecated_type.present?
end

which would need to be explicitly written instead of relying on AR automatic definitions. The advantage in this case would be we just need to maintain a single field on the DB instead of 2. Also, we already have the boolean field active, adding another one can potentially make things a bit messy when they're not kept in sync.

On the other hand, the burden to maintain 2 fields is rather minimal, and it would probably allow us to write simpler code in ruby.

luca-landa · 2020-11-14T10:46:59Z

@spaghetticode I agree on the potential problem of keeping active and deprecated in sync, I didn't really consider it at first and I'd like to avoid the risk.

I like your proposal with deprecated_type and the scope you defined (since active will help filter out deprecated records).

But I have another change in mind that would make the AR scope a bit more readable, and it would make the record more explicitly marked as "deprecated".
What if the new type set by the task is a new Spree::PaymentMethod::DeprecatedMethodmodel class, instead of Spree::PaymentMethod? The scope would also become scope :deprecated, -> {where(type: "Spree::PaymentMethod::DeprecatedMethod")}.

I have another (OT) question about this PR: I don't know how exactly should I fix an error that I made, I could add another commit but it wouldn't satisfy the guidelines request that the specs pass for each commit. Should I create another PR for another branch?

jarednorman

I think we should find a better name for the "deprecated" column that aligns better with how it works. Otherwise, this seems like a reasonable approach if the method we're overriding has the desired effect.

core/db/migrate/20201108142424_add_deprecated_to_payment_method.rb

kennyadsl

Thanks! I left some questions on the implementation. Looking forward to know what you think about them.

Also, can you please squash the last commit into the one that introduced the code style issue?

🙏

core/db/migrate/20201108142424_add_deprecated_to_payment_method.rb

core/lib/tasks/payment_method.rake

core/app/models/spree/payment_method.rb

guides/source/developers/payments/payment-service-providers.html.md

spaghetticode · 2020-11-16T17:49:23Z

But I have another change in mind that would make the AR scope a bit more readable, and it would make the record more explicitly marked as "deprecated".
What if the new type set by the task is a new Spree::PaymentMethod::DeprecatedMethodmodel class, instead of Spree::PaymentMethod? The scope would also become scope :deprecated, -> {where(type: "Spree::PaymentMethod::DeprecatedMethod")}.

I guess that would make sense, although it's not strictly needed... but I think it may make it easier to discriminate removed payment methods and their payments? If while doing the PR you can come up with good reasons for doing (or not doing) it, you can add some comments in the commit message, that would be very valuable.

I have another (OT) question about this PR: I don't know how exactly should I fix an error that I made, I could add another commit but it wouldn't satisfy the guidelines request that the specs pass for each commit. Should I create another PR for another branch?

No need, you can make all the fixes and changes you need in this PR, as long as they're related to it.
You can also rebase, do whatever makes sense so that, eventually, commits make sense when we'll need to navigate the git history and understand the changes.

luca-landa · 2020-11-16T18:24:23Z

Well the Spree::PaymentMethod::DeprecatedMethod class was just a proposal, I don't see any more good reasons than those I already explained, but I actually see a reason for not doing it: we would be adding a subclass which does not define any specific behavior.

I may just discard the idea, but stick with the idea of changing deprecated to previous_type if the other reviewers approve this change too.

kennyadsl

Left a second review with other thoughts. Let me know if those make sense.

Another thing: I was wondering if there's something we could do when deleting a payment method. I'm thinking of a simple PORO that handles the PaymentMethod destruction and maybe sets these attributes for us when we (soft-)delete a payment method? This way we will also prevent this error from happening, anyway, maybe it's something for another PR. 🙂

core/app/models/spree/payment_method.rb

core/db/migrate/20201108142424_add_deprecated_to_payment_method.rb

core/lib/tasks/payment_method.rake

kennyadsl

Left some other comments, we are almost there, thanks again!

core/db/migrate/20201118200630_add_previous_type_to_spree_payment_methods.rb

core/lib/tasks/payment_method.rake

guides/source/developers/payments/payment-service-providers.html.md

core/spec/models/spree/payment_method_spec.rb

core/lib/tasks/payment_method.rake

spaghetticode · 2021-09-17T12:04:02Z

@luca-landa the PR is almost ready now, thanks for the work you've done so far! There are still a couple of cosmetic issues raised by Hound, and this merge commit that should be dealt with.

luca-landa · 2022-02-21T10:13:36Z

@spaghetticode I'm sorry I completely lost track of this PR 😅 it should be fixed now

After switching payment service provider, previous payment method records still exist in spree_payment_methods table, referencing an unexisting class for STI. This generates ActiveRecord::SubclassNotFound errors while retrieving the records from the database. This commit adds a task that resets the type of unsupported payment methods to Spree::PaymentMethod, and records the old actual type in a new "type_before_removal" field. This allows to keep previous orders which still reference these unsupported payment methods.

Show a simple error after retrieving payment methods no more supported, suggesting to run a specific rake task to fix the issue and without having to delete those payment methods.

spaghetticode

LGTM, thanks Luca!

waiting-for-dev · 2022-06-07T12:12:37Z

Hey @jarednorman, I think your concern was addressed 🙂 Is there anything else we need to tackle here?

jarednorman

No, I'm happy here.

PR solidusio#3837 added a rake task to deactivate payment method records that are no longer valid because of removing a payment method class. The task iterates over all payment method records and tries to constantize from its polymorphic type column. When that process returns a `NameError`, it assumes that the class is no longer present and proceeds to deactivate the record. Before this commit, `ActiveSupport::Dependencies.constantize` was used. However, the method was removed on Rails 7 [1], so our code raised a `NoMethodError`. As `NoMethodError` is a subclass of `NameError` [2], the deactivation logic was called for every record. As Rails recommends, we update the code to use `String#constantize` instead. [1] - https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#activesupport-dependencies-private-api-has-been-deleted [2] - https://ruby-doc.org/core-3.1.0/NoMethodError.html

PR #3837 added a rake task to deactivate payment method records that are no longer valid because of removing a payment method class. The task iterates over all payment method records and tries to constantize from its polymorphic type column. When that process returns a `NameError`, it assumes that the class is no longer present and proceeds to deactivate the record. Before this commit, `ActiveSupport::Dependencies.constantize` was used. However, the method was removed on Rails 7 [1], so our code raised a `NoMethodError`. As `NoMethodError` is a subclass of `NameError` [2], the deactivation logic was called for every record. As Rails recommends, we update the code to use `String#constantize` instead. [1] - https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#activesupport-dependencies-private-api-has-been-deleted [2] - https://ruby-doc.org/core-3.1.0/NoMethodError.html

Improve payment service providers switching errors

PR #3837 added a rake task to deactivate payment method records that are no longer valid because of removing a payment method class. The task iterates over all payment method records and tries to constantize from its polymorphic type column. When that process returns a `NameError`, it assumes that the class is no longer present and proceeds to deactivate the record. Before this commit, `ActiveSupport::Dependencies.constantize` was used. However, the method was removed on Rails 7 [1], so our code raised a `NoMethodError`. As `NoMethodError` is a subclass of `NameError` [2], the deactivation logic was called for every record. As Rails recommends, we update the code to use `String#constantize` instead. [1] - https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#activesupport-dependencies-private-api-has-been-deleted [2] - https://ruby-doc.org/core-3.1.0/NoMethodError.html

In the doc printed when it's needed, we had a typo, probably due to some back and forth on the PR that introduced the change. See #3837

houndci-bot reviewed Nov 9, 2020

View reviewed changes

core/spec/models/spree/payment_method_spec.rb Outdated Show resolved Hide resolved

core/spec/models/spree/payment_method_spec.rb Outdated Show resolved Hide resolved

jarednorman requested changes Nov 16, 2020

View reviewed changes

core/db/migrate/20201108142424_add_deprecated_to_payment_method.rb Outdated Show resolved Hide resolved

kennyadsl reviewed Nov 16, 2020

View reviewed changes

luca-landa force-pushed the improve-psp-switching-errors branch from 6d8a522 to eae43dd Compare November 16, 2020 17:53

kennyadsl reviewed Nov 17, 2020

View reviewed changes

luca-landa force-pushed the improve-psp-switching-errors branch 2 times, most recently from dd1eb08 to a49d2ca Compare November 19, 2020 14:31

kennyadsl reviewed Nov 26, 2020

View reviewed changes

core/db/migrate/20201118200630_add_previous_type_to_spree_payment_methods.rb Outdated Show resolved Hide resolved

core/lib/tasks/payment_method.rake Outdated Show resolved Hide resolved

guides/source/developers/payments/payment-service-providers.html.md Show resolved Hide resolved

luca-landa force-pushed the improve-psp-switching-errors branch from a49d2ca to 5839bfe Compare November 27, 2020 22:04

luca-landa force-pushed the improve-psp-switching-errors branch from 5839bfe to b8528db Compare December 5, 2020 17:14

spaghetticode reviewed Jan 22, 2021

View reviewed changes

core/spec/models/spree/payment_method_spec.rb Outdated Show resolved Hide resolved

luca-landa force-pushed the improve-psp-switching-errors branch from b8528db to 8f9ddb8 Compare January 22, 2021 15:56

houndci-bot reviewed Jan 22, 2021

View reviewed changes

core/spec/models/spree/payment_method_spec.rb Outdated Show resolved Hide resolved

luca-landa force-pushed the improve-psp-switching-errors branch from 8f9ddb8 to 29041b6 Compare January 29, 2021 16:24

kennyadsl added this to the 3.1.0 milestone Feb 3, 2021

houndci-bot reviewed Sep 7, 2021

View reviewed changes

core/lib/tasks/payment_method.rake Outdated Show resolved Hide resolved

kennyadsl modified the milestones: 3.1.0, 3.2.0 Sep 8, 2021

luca-landa force-pushed the improve-psp-switching-errors branch 3 times, most recently from ec79d5b to ca6e770 Compare February 21, 2022 10:03

luca-landa added 3 commits June 6, 2022 14:43

Improve error on invalid payment methods retrieval

719dbf5

Show a simple error after retrieving payment methods no more supported, suggesting to run a specific rake task to fix the issue and without having to delete those payment methods.

Update docs on payment methods deprecation

a6a2d44

luca-landa force-pushed the improve-psp-switching-errors branch from ca6e770 to a6a2d44 Compare June 6, 2022 12:43

spaghetticode approved these changes Jun 6, 2022

View reviewed changes

jarednorman approved these changes Jun 7, 2022

View reviewed changes

waiting-for-dev merged commit 07c88dd into solidusio:master Jun 8, 2022

waiting-for-dev mentioned this pull request Jun 14, 2022

Fix deprecation of active payment methods #4414

Merged

waiting-for-dev added a commit that referenced this pull request Jun 20, 2022

Merge pull request #3837 from luca-landa/improve-psp-switching-errors

0155cdb

Improve payment service providers switching errors

kennyadsl added a commit that referenced this pull request Oct 25, 2023

Fix deactivate_unsupported_payment_methods name

f0bd4c6

In the doc printed when it's needed, we had a typo, probably due to some back and forth on the PR that introduced the change. See #3837

kennyadsl mentioned this pull request Oct 25, 2023

Fix deactivate_unsupported_payment_methods name in error message #5458

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve payment service providers switching errors #3837

Improve payment service providers switching errors #3837

luca-landa commented Nov 9, 2020 •

edited

Loading

luca-landa commented Nov 10, 2020 •

edited

Loading

spaghetticode commented Nov 11, 2020

luca-landa commented Nov 11, 2020

spaghetticode commented Nov 13, 2020

luca-landa commented Nov 14, 2020 •

edited

Loading

jarednorman left a comment

kennyadsl left a comment

spaghetticode commented Nov 16, 2020

luca-landa commented Nov 16, 2020

kennyadsl left a comment

kennyadsl left a comment

spaghetticode commented Sep 17, 2021

luca-landa commented Feb 21, 2022

spaghetticode left a comment

waiting-for-dev commented Jun 7, 2022

jarednorman left a comment

Improve payment service providers switching errors #3837

Improve payment service providers switching errors #3837

Conversation

luca-landa commented Nov 9, 2020 • edited Loading

luca-landa commented Nov 10, 2020 • edited Loading

spaghetticode commented Nov 11, 2020

luca-landa commented Nov 11, 2020

spaghetticode commented Nov 13, 2020

luca-landa commented Nov 14, 2020 • edited Loading

jarednorman left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

spaghetticode commented Nov 16, 2020

luca-landa commented Nov 16, 2020

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

spaghetticode commented Sep 17, 2021

luca-landa commented Feb 21, 2022

spaghetticode left a comment

Choose a reason for hiding this comment

waiting-for-dev commented Jun 7, 2022

jarednorman left a comment

Choose a reason for hiding this comment

luca-landa commented Nov 9, 2020 •

edited

Loading

luca-landa commented Nov 10, 2020 •

edited

Loading

luca-landa commented Nov 14, 2020 •

edited

Loading