search

solidusio / solidus_paypal_commerce_platform

💳 Integrate Solidus with Paypal Commerce Platform
https://developer.paypal.com/docs/platforms/
BSD 3-Clause "New" or "Revised" License
17 stars 24 forks source link

Incorrect javascript_sdk_url parameter conditions #150

Open RyanofWoods opened 2 years ago

RyanofWoods commented 2 years ago

The commit and shipping_preference parameters rely on checking the Order checkout_steps. https://github.com/solidusio-contrib/solidus_paypal_commerce_platform/blob/19c00c68220fa9b7490dcc6497030809cbbfda03/app/models/solidus_paypal_commerce_platform/payment_method.rb#L67

https://github.com/solidusio-contrib/solidus_paypal_commerce_platform/blob/19c00c68220fa9b7490dcc6497030809cbbfda03/app/models/solidus_paypal_commerce_platform/payment_method.rb#L77

The checkout_steps get set here, attempting to use the order's checkout_steps, otherwise the Spree::Order model. However, order.checkout_steps returns an array of strings, and ::Spree::Order.checkout_steps.keys returns an array of symbols. As the conditionals currently rely on strings, the conditionals work incorrectly when relying on the Order model for checkout steps. This happens for example on the product page where no @order is given.

https://github.com/solidusio-contrib/solidus_paypal_commerce_platform/blob/19c00c68220fa9b7490dcc6497030809cbbfda03/app/models/solidus_paypal_commerce_platform/payment_method.rb#L65

This is a simple fix, however, the shipping_preference param might be dropped due to issue https://github.com/solidusio-contrib/solidus_paypal_commerce_platform/issues/149 and the javascript_sdk_url method changes a lot due to the https://github.com/solidusio-contrib/solidus_paypal_commerce_platform/pull/148 PR. So this issue can wait until these get resolved to avoid conflicts.

retsef commented 2 years ago

I had encored in the same issue (#133). In #152 I had moved the shipping_preference under PaypalOrder object and update the javascript hook to be aware of the missing params. We use this patch in production

RyanofWoods commented 2 years ago

Hi @retsef. I think #133 relates to the issue I made (#149 ). But this issue refers more to the step_names logic rather than the params themselves.

retsef commented 2 years ago

@RyanofWoods my bad, I had misunderstand it. I had update #152 as you suggest it ;)

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It might be closed if no further activity occurs. Thank you for your contributions.

jakemumu commented 1 year ago

Just wanted to pop in and suggest that showing shipping should be a config preference and not automatically deduced via the order state machine -- we do not have an address step, because we simply use the addresses from PayPal -- why collect address twice?