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.

Sign up for GitHub

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

Jump to bottom

Fix master, import paypal-checkout-sdk #161

Merged
merged 6 commits into from
Sep 30, 2022
Merged

Fix master, import paypal-checkout-sdk #161

merged 6 commits into from
Sep 30, 2022

Conversation

cpfergus1
Copy link
Contributor

@cpfergus1 cpfergus1 commented Sep 21, 2022 •
edited by elia
Loading

Description

Fixes failing specs on master and imports paypal-checkout-sdk which will no longer be maintained by PayPal.

Motivation and Context

We used to offer PayPal support while installing solidus until v3.2, that's because we wanted to support Ruby 3 but paypal-checkout-sdk is dependent on an older version of paypalhttp that doesn't support Ruby 3.

Upon further inspection the paypal-checkout-sdk is just a bunch of request-like objects that are to be used with paypalhttp, that makes them good candidates for taking them over and upgrading them to be compatible with both ruby 3 and the latest paypalhttp.

Now we would like to restore support by embedding the code we need from paypal-checkout-sdk into solidus_paypal_commerce_platform and directly depend on the latest version of paypalhttp.

How Has This Been Tested?

The current test suite covers the changes made in the PR

Types of changes

  • - Bug fix (non-breaking change which fixes an issue)
  • - New feature (non-breaking change which adds functionality)
  • - Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • - My code follows the code style of this project.
  • - My change requires a change to the documentation.
  • - I have updated the documentation accordingly.

@cpfergus1 cpfergus1 force-pushed the nebulab/import-paypal-sdk branch 2 times, most recently from 0218046 to 5d0ffe0 Compare September 21, 2022 15:55
@kennyadsl
Copy link
Member

@cpfergus1 thanks! Is this now compatible with Ruby 3 so?

@cpfergus1
Copy link
Contributor Author

cpfergus1 commented Sep 22, 2022 •
edited
Loading

@cpfergus1 thanks! Is this now compatible with Ruby 3 so?

@kennyadsl All 83 examples are passing when testing with Ruby 3.1.0

elia
elia requested changes Sep 26, 2022
View reviewed changes
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments, I would also suggest to move all the inherited requests under the solidus_paypal_commerce_platform namespace and throw away the ones we don't use, so we won't need to keep them up to date in the future.

app/controllers/solidus_paypal_commerce_platform/orders_controller.rb Outdated Show resolved Hide resolved
spec/features/frontend/checkout_spec.rb Outdated Show resolved Hide resolved
@cpfergus1
Sign in prior to running specs
fabe98d 
Master branch was failing because a unassociated user
was attempting to access an order assigned to the example user.
Signing in ensures that the correct user is accessing that order.
@cpfergus1 cpfergus1 requested a review from elia September 26, 2022 13:34
@cpfergus1
Copy link
Contributor Author

I attempted namespacing but ended up getting different results when testing in the sandbox. I spent some time trying to discover the issue and worked with @kennyadsl. After some time, it was proposed to just release the fix without the namespace to get the fix in the codebase. We can revisit the namespacing at a later time

@cpfergus1 cpfergus1 force-pushed the nebulab/import-paypal-sdk branch 2 times, most recently from c23cb1b to 2e44b56 Compare September 27, 2022 11:11
@xeruf xeruf mentioned this pull request Sep 29, 2022
Closed
elia
elia requested changes Sep 29, 2022
View reviewed changes
app/controllers/solidus_paypal_commerce_platform/orders_controller.rb Outdated Show resolved Hide resolved
lib/paypal/access_token.rb Outdated Show resolved Hide resolved
@elia elia mentioned this pull request Sep 29, 2022
Closed
@cpfergus1 cpfergus1 force-pushed the nebulab/import-paypal-sdk branch 2 times, most recently from 4e4f313 to b5c2110 Compare September 29, 2022 14:57
kennyadsl
kennyadsl approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cpfergus1

@cpfergus1 cpfergus1 requested a review from elia September 29, 2022 15:38
elia
elia approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a nit, mostly to avoid confusion for future readers, all good once that's fixed 👍

app/controllers/solidus_paypal_commerce_platform/orders_controller.rb Outdated Show resolved Hide resolved
@cpfergus1
Remove try_spree_current_user
01118ed 
solidusio/solidus#3923
@cpfergus1
Remove Deprecated SolidusSupport.payment_x_parent_class
07e5877
@cpfergus1
Import Paypal-Checkout-SDK
c4e7fb9 
Paypal has opted to no longer support their sdk gem, this commit
imports the current iteration of that gem within this library and will
be maintained here.
@cpfergus1
Lint Corrections
fee386c 
squash on import
@cpfergus1
Allow Ruby 3.0+
97b5c76
@kennyadsl kennyadsl merged commit 1e3a3a4 into solidusio:master Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elia elia elia approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cpfergus1 @kennyadsl @elia