solidusio / solidus_paypal_braintree

⛔️ [Archived] Use solidus_braintree instead!
https://github.com/solidusio/solidus_braintree
BSD 3-Clause "New" or "Revised" License
37 stars 78 forks source link

Fix exception when other payment methods active #318

Closed embold-tyler closed 2 years ago

embold-tyler commented 2 years ago

When other payment methods are active, specifically solidus_affirm, these methods are not available and cause an exception.

kennyadsl commented 2 years ago

@embold-tyler thanks for the PR. Can you help me better understand this issue, please? That partial should be rendered for payment associated with this PaypalBraintree payment method only. In your case it seems like it's used for an affirm payment method, right?

embold-tyler commented 2 years ago

No problem. I noticed that the deface is going onto the payments view, so it hits each payment method and it seems the methods in those conditionals its using are not available on the payments sources generated by affirm for whatever reason ("display_payment_type", "paypal?", and "venmo?") so was causing the app to throw a no method error. Using .try allowed the conditional a graceful fail while still rendering those portions of the defaced partial only when needed on the PaypalBraintree method.

kennyadsl commented 2 years ago

Oh, which deface exactly? I can't see any deface that could interact with other payment partials in this extension. Maybe it's coming from somewhere else?

embold-tyler commented 2 years ago

This one: https://github.com/solidusio/solidus_paypal_braintree/blob/master/app/overrides/spree/payments/payment/add_paypal_funding_source_to_payment.rb

kennyadsl commented 2 years ago

But it is explicitly applied to the Paypal partial only, see line 8.

embold-tyler commented 2 years ago

I do see that but it seems like it's loading more generously than intended. Looks like this issue reports the same "no method" behavior with the store credit payment method from that same partial applied using the deface.

https://github.com/solidusio/solidus_paypal_braintree/issues/315

kennyadsl commented 2 years ago

I think you are right, I was reading that deface override wrongly. The partial I linked is what is being injected but it is going in the generic payment partial coming from Solidus core. I think your fix addresses the issue but I'd like to be sure to render those things only for payments coming from this extension.

RyanofWoods commented 2 years ago

I tried this locally, and this change looks good to me @kennyadsl.

Thank you for the fix @embold-tyler

RyanofWoods commented 2 years ago

Can we release this patch @kennyadsl and what about yanking 1.1.0 and 1.1.1 because 1.1.0 was a breaking change.

kennyadsl commented 2 years ago

@RyanofWoods I'll release it now. I'm against yanking those versions though. Yanking in my experience causes more problems than it solves. Having 1.1.2 would be enough to prevent people from updating to 1.1.0 or 1.1.1. If they are locked to those versions, they are probably not experiencing this bug.

kennyadsl commented 2 years ago

@RyanofWoods @embold-tyler 1.1.2 released, let me know how it goes. Thanks again!

RyanofWoods commented 2 years ago

Understood, thanks @kennyadsl 😊