Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Fix exception when other payment methods active #318

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

embold-tyler
Copy link
Contributor

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

When other payment methods are active, specifically solidus_affirm, these methods are not available and cause an exception.
@kennyadsl
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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?

@kennyadsl
Copy link
Member

kennyadsl commented May 9, 2022

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

@embold-tyler
Copy link
Contributor Author

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.

#315

@kennyadsl
Copy link
Member

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
Copy link
Contributor

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

Thank you for the fix @embold-tyler

@kennyadsl kennyadsl merged commit 1e25014 into solidusio:master Aug 22, 2022
@RyanofWoods
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Member

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

@RyanofWoods
Copy link
Contributor

Understood, thanks @kennyadsl 😊

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants