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

Restore --payment-method= for solidus:install on v3.2 #4673

Merged
merged 6 commits into from
Oct 17, 2022

Conversation

elia
Copy link
Member

@elia elia commented Oct 12, 2022

Summary

Solidus PCP is now compatible with Ruby 3 and can be re-enabled, since the CLI flag has been restored we can also attach bolt to it and let user select the payment method with CLI flags.

I included the minimum required to make the installer work, since we don't have tests for it I tried with:

bin/sandbox --frontend=solidus_frontend --payment-method=bolt --with-authentication 
bin/sandbox --frontend=solidus_frontend --payment-method=none --with-authentication 

and variations, also including the same options but interactively.

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 force-pushed the elia/restore-payment-method-on-3-2 branch from b5ca42d to 84e1f91 Compare October 12, 2022 17:41
@elia elia changed the title Restore --payment-method= for `solidus:install on v3.2 Restore --payment-method= for solidus:install on v3.2 Oct 12, 2022
@elia elia marked this pull request as ready for review October 12, 2022 17:53
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Some preliminary comments. Thanks!

@@ -88,12 +79,7 @@ fi

unbundled bundle install --gemfile Gemfile
unbundled bin/rails db:drop db:create
unbundled bin/rails generate solidus:install \
--auto-accept \
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super happy removing auto-accept by default, as it slows the development feedback. What do you think about keeping it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's no big deal either way, easy to add or remove locally as needed, so I'll add it back (I had to remove it in order to test out the interactive solidus:install wizard).

@@ -248,6 +274,13 @@ def complete
private

def detect_frontend_to_install(bundler_context)
if options[:payment_method] != 'none'
say_status :warning, set_color(
"You selected a payment method that might require manual integration with `solidus_starter_frontend`",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better experience would be warning before the payment has been selected.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with that, moving it

# Solidus bolt will be handled in the installer as a payment method.
begin
skip_solidus_bolt = ENV['SKIP_SOLIDUS_BOLT']
ENV['SKIP_SOLIDUS_BOLT'] = 'true'
Copy link
Contributor

Choose a reason for hiding this comment

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

If we move it here, I think we should remove the option from solidus_frontend altogether. Otherwise we're dealing with unneeded complexity here. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

🎯 exactly, that's the idea, we can remove it as soon as v3.2 is updated

elia added 2 commits October 13, 2022 10:04
Just a generic warning, but we're trying to keep this minimal.
Since we don't have other ways to test solidus:install this allows
providing any combination, including the interactive mode.

Tangentially fix the missing newline escape before `$@`.
@elia elia force-pushed the elia/restore-payment-method-on-3-2 branch from 84e1f91 to 4a99b8b Compare October 13, 2022 08:37
elia added 3 commits October 13, 2022 10:40
Without this only --with-authentication=false had an effect and the
value of --payment-method was completely disregarded.
Forcefully skips its installation when installing solidus_frontend.
Otherwise installation checking for SolidusSupport.frontend_available?
can fail.
@elia elia force-pushed the elia/restore-payment-method-on-3-2 branch from 4a99b8b to 7c7e913 Compare October 13, 2022 08:41
@elia
Copy link
Member Author

elia commented Oct 13, 2022

@jarednorman @waiting-for-dev I had to change a couple of things to check auth options for .nil? instead of using ||= so to correctly support both true and false when provided as CLI flags.

@elia elia requested a review from waiting-for-dev October 13, 2022 08:41
@waiting-for-dev waiting-for-dev mentioned this pull request Oct 17, 2022
7 tasks
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

👍

@kennyadsl kennyadsl merged commit 89b8ae4 into solidusio:v3.2 Oct 17, 2022
@kennyadsl kennyadsl deleted the elia/restore-payment-method-on-3-2 branch October 17, 2022 14:44
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.

4 participants