search

solidusio / solidus_starter_frontend

🎨 Rails-based starter kit for your Solidus storefront.
BSD 3-Clause "New" or "Revised" License
55 stars 37 forks source link

Backport payment step cleanup #325

Closed gsmendoza closed 1 year ago

gsmendoza commented 1 year ago

Goal

Closes #324.

We want to backport the recent SolidusStarterFrontend changes to v3.3 branch so that the planned SolidusBraintree 2.0.0 would be compatible with Solidus and SolidusStarterFrontend v3.3.

Background

We discovered in https://github.com/solidusio/solidus_braintree/issues/104 that the planned SolidusBraintree 2.0.0 gem is currently not compatible with Solidus 3.3, and thus would require Solidus 3.4 (or Solidus 3.4.0.dev) to work.

By backporting the recent SolidusStarterFrontend changes to v3.3 branch, SolidusBraintree 2.0.0 would hopefully be compatible with Solidus and SolidusStarterFrontend v3.3, and thus we won't have to wait for Solidus 3.4 to be released before releasing SolidusBraintree 2.0.0.

Implementation

These PRs are needed by SolidusBraintree in v3.3:

Testing

Manually tested on https://github.com/gsmendoza/solidus_store/tree/rails-7-0-4--solidus-3-3-1--no-payment-method--324-payment-step-cleanup--solidus-braintree. I was able to complete the checkout for both new and saved payment sources.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

kennyadsl commented 1 year ago

I am a bit afraid that backporting things (that are not bug fixes) is not a good practice in the starter frontend context. People may want to refer to the original code of the version they used and if we change it, it will be hard.

Can you help me understand what's the issue with waiting and having SolidusBraintree 2.0 compatible with 3.4 instead?

elia commented 1 year ago

People may want to refer to the original code of the version they used and if we change it, it will be hard.

@kennyadsl should we worry about this, every project will also have its git history to refer to, the great advantage of this setup is not needing to worry about backward compatibility. Rather we can revert as easily if we notice some other extension (e.g. PayPal commerce) are broken after this.

kennyadsl commented 1 year ago

should we worry about this, every project will also have its git history to refer to, the great advantage of this setup is not needing to worry about backward compatibility.

True 😓

Rather we can revert as easily if we notice some other extension (e.g. PayPal commerce) are broken after this.

Still, I don't understand why we should risk this. What's the benefit of the backport other than not having to wait? I feel like I'm missing some pieces.

elia commented 1 year ago

Still, I don't understand why we should risk this. What's the benefit of the backport other than not having to wait? I feel like I'm missing some pieces.

I think the risk assessment is different, so it's not perceived as a high risk scenario, and would have the benefit of enabling braintree + starter on the current solidus release.

Conversely I think also the benefit is not super strong, unless we'll need to wait a long time before 3.4 is released, so I'm personally fine with both.

kennyadsl commented 1 year ago

I think the risk assessment is different, so it's not perceived as a high risk scenario, and would have the benefit of enabling braintree + starter on the current solidus release.

If they already installed the current template, they will still not benefit from this + they need to understand that although we declare Braintree to be compatible with Soldius 3.3, it's not the case for them, because they started the project before merging this PR.

Just to be clear, I also think it's not the end of the world if we merge this, but I'd like to take this opportunity to understand how to behave in the future regarding this kind of backporting issues.

elia commented 1 year ago

@kennyadsl there is also https://github.com/solidusio/solidus_starter_frontend/issues/324, in which the release of braintree is blocked until 3.4 is out, do we have any expectation on when this will happen? That could help in weighting this decision.

/cc @waiting-for-dev

kennyadsl commented 1 year ago

@elia I think it's the same thing. This PR closes that issue, isn't it?

elia commented 1 year ago

@kennyadsl exactly

kennyadsl commented 1 year ago

Ah, got what you mean. But I think it's not a real blocker: we can release Solidus Braintree 2.0, and when 3.4 will be released it will be compatible with it. Updating the Solidus Braintree gemspec dependency to >= 3.4 will be unusable until that happens.

gsmendoza commented 1 year ago

@kennyadsl Okay, I'll close this PR and create a separate PR to the Solidus dependency of Solidus Braintree gemspec to ~> 3.4.