Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect javascript_sdk_url parameter conditions #150

Open
RyanofWoods opened this issue Dec 16, 2021 · 5 comments
Open

Incorrect javascript_sdk_url parameter conditions #150

RyanofWoods opened this issue Dec 16, 2021 · 5 comments
Labels

Comments

@RyanofWoods
Copy link
Contributor

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 #149 and the javascript_sdk_url method changes a lot due to the #148 PR. So this issue can wait until these get resolved to avoid conflicts.

@retsef
Copy link
Contributor

retsef commented Dec 20, 2021

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

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

retsef commented Dec 23, 2021

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

@stale
Copy link

stale bot commented Mar 2, 2022

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.

@stale stale bot added the stale label Mar 2, 2022
@jakemumu
Copy link

jakemumu commented Mar 2, 2023

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?

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

No branches or pull requests

3 participants