Skip to content

SSF + Payment Element reboot#146

Merged
elia merged 5 commits into
masterfrom
elia/ssf-payment-element-reboot
Jan 9, 2023
Merged

SSF + Payment Element reboot#146
elia merged 5 commits into
masterfrom
elia/ssf-payment-element-reboot

Conversation

@elia

@elia elia commented Dec 19, 2022

Copy link
Copy Markdown
Member

Summary

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • I have used clear, explanatory commit messages.

The following are not always needed (cross them out if they are not):

  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.
  • I have opened a PR to update the guides.
  • I have updated the README to account for my changes.

@elia elia self-assigned this Dec 19, 2022
@elia elia force-pushed the elia/ssf-payment-element-reboot branch 4 times, most recently from a157e11 to 8f043da Compare December 20, 2022 10:52
@elia elia marked this pull request as ready for review December 20, 2022 13:07

@gsmendoza gsmendoza left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@elia Looking good! I just have a few questions :)

Comment thread README.md Outdated
@@ -1,257 +1,30 @@
🚧 Your looking at the source for `solidus_stripe` v5, which will only support the **starter frontend** and not the classic frontend 🚧

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
🚧 Your looking at the source for `solidus_stripe` v5, which will only support the **starter frontend** and not the classic frontend 🚧
🚧 You're looking at the source for `solidus_stripe` v5, which will only support the **starter frontend** and not the classic frontend 🚧


# rubocop:disable Lint/UnusedMethodArgument, Style/MethodCallWithoutArgsParentheses, Lint/UselessAssignment
module SolidusStripe
class Gateway

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Regarding 189dd87, can you provide more details on how Gateway, PaymentMethod, and PaymentSource were built? They seem to follow the API of SolidusPaypalCommercePlatform but their code are not exactly the same.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's just meant to be a placeholder for the implementation. Given the Gateway API is mandated by Solidus I felt it was ok to stub it out letting the implementation to be added in following PRs.

Comment thread Gemfile
# how Dependabot parses projects: https://github.com/dependabot/dependabot-core/issues/1658.
send(:eval_gemfile, 'Gemfile-local') if File.exist? 'Gemfile-local'
gem 'stringio'
gem 'timeout'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Regarding 8f043da, the commit message mentions "libvips was missing from the CI", but the commit doesn't seem to do anything about libvips. Maybe the commit message is not up-to-date?

@elia elia force-pushed the elia/ssf-payment-element-reboot branch from 8f043da to 70dbf47 Compare December 23, 2022 15:39
@elia elia requested a review from gsmendoza December 23, 2022 15:41
@elia elia force-pushed the elia/ssf-payment-element-reboot branch 4 times, most recently from 458d51b to 60ffb93 Compare December 28, 2022 17:23
@elia elia requested a review from kennyadsl December 28, 2022 17:24

@kennyadsl kennyadsl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Left a suggestions, non blocking.

Comment thread README.md Outdated
@@ -1,257 +1,30 @@
🚧 You're looking at the source for `solidus_stripe` v5, which will only support the **starter frontend** and not the classic frontend 🚧
🚧 You can find the source for [v4 on the corresponding branch](https://github.com/solidusio/solidus_stripe/tree/v4) 🚧

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd also add some accurate instructions about what to do to stick to the old version, suggesting to use the Rubygems version, or if someone wants to stick to github, they will need to change the branch.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Added and updated the layout:

image

I also added a post install message that should better cover the case of people already using it from source.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lovely, thanks!

@elia elia force-pushed the elia/ssf-payment-element-reboot branch 3 times, most recently from 90d05b0 to ad7b428 Compare December 30, 2022 15:44
elia added 5 commits December 30, 2022 16:49
- Run `solidus extension stripe` from scratch
- Keep the changelog
- Added a notice at the top of the readme
- Use the sqlite executor from the official ORB
`timeout` and `stringio` are needed on newer rubies in combo with older Rails.
Solidus will always provide a source if the payment method supports
payment profiles.
@elia elia force-pushed the elia/ssf-payment-element-reboot branch from ad7b428 to d7c1482 Compare December 30, 2022 15:49
@elia elia merged commit 96a2c2e into master Jan 9, 2023
@elia elia deleted the elia/ssf-payment-element-reboot branch January 9, 2023 12:27
@elia elia added this to the v5 milestone Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-create the extension with solidus extension solidus_stripe

3 participants