Support the new starter frontend installation #276

kennyadsl · 2023-01-05T12:05:55Z

Summary

This works out of the box only for a fresh installation of the starter frontend because we can't anticipate what changed in each storefront, but should be a good starting point.

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.

Naokimi · 2023-01-07T10:37:36Z

lib/generators/solidus_subscriptions/install/install_generator.rb

@@ -15,6 +16,30 @@ def add_javascripts
        append_file 'vendor/assets/javascripts/spree/backend/all.js', "//= require spree/backend/solidus_subscriptions\n"
      end

+      def copy_starter_frontend_files
+        return if options[:frontend] != 'starter'


imho return unless options[:frontend] == 'starter' is better for readability

Thanks for the suggestions @Naokimi. I tend to use unless only when the condition doesn't contain a negation, that's because it would be a double negation, IMO, always harder to read. I think I took this reasoning line from this blog post years ago: https://signalvnoise.com/posts/2699-making-sense-with-rubys-unless. BTW, I'm open to invert it, let me know your thoughts.

yeah, I'm also of the school that you should avoid negations as much as possible, so since return if options[:frontend] != 'starter' contains a negation (NOT equal to) I'm suggesting removing the negation (-> equal to) while using unless

but in any case in this specific case what you wrote is still readable enough, so it wouldn't be a big deal to leave it as it is

Oh, you are right. Personally, I still prefer the negation to be on the condition, so I think I'll leave it as is for now. But appreciate the conversation around this topic. Thanks again!

waiting-for-dev

Thanks, @kennyadsl. I left some comments. Interested in your thoughts.

waiting-for-dev · 2023-01-09T09:50:16Z

lib/generators/solidus_subscriptions/install/install_generator.rb

@@ -6,6 +6,7 @@ class InstallGenerator < Rails::Generators::Base
      source_root File.expand_path('templates', __dir__)

      class_option :auto_run_migrations, type: :boolean, default: false
+      class_option :frontend, type: :string, default: 'legacy'


We don't use the 'legacy' value anywhere. What about following the same pattern that @elia used in solidus_paypal_commerce_platform and, instead, have a starter_frontend boolean option?

Using a string instead of a boolean looks better to me, since more things could end up being frontends (api, graphql, etc) and being able to select the right integration seems like a better choice.

Anyways the name is classic in the solidus installer, and I'm fine with both fixing it or moving it to a boolean, we can change later if necessary 👍

I used this format to follow the main installer. For sure I should at least change legacy to classic to follow that convention. Not sure which approach is best at this point, @elia do you have any thought?

I think we posted at the same time, but I'll restate my take: while right now I see using a string as the way forward I'm fine with both.

I other words I think we'll have a better understanding on how things "should be done" in a month or two, and we'll be able to codify it somewhere (e.g. in solidus_dev_support).

Changed to classic, but open to change while I wait for @waiting-for-dev opinion.

I also thought about the potential for adding new storefronts. However, I see it as a premature optimization, plus we won't add any other new frontend in the near future. Having code that does nothing looks off. We've been discussing whether to call it classic or legacy, but we could call it potato, and it'd make no difference 😆 But I'm not super-against it, as it could help in the future to avoid breaking changes. However, I'd try to be consistent throughout the extension ecosystems, so we should do the same here and in the PayPal one.

lib/generators/solidus_subscriptions/install/install_generator.rb

README.md

waiting-for-dev · 2023-01-09T09:55:36Z

lib/generators/solidus_subscriptions/install/install_generator.rb

+          RUBY
+        end
+        insert_into_file 'app/controllers/cart_line_items_controller.rb', after: "private\n" do
+          <<-RUBY.strip_heredoc.indent(2)


Same suggestion to use Ruby's built-in heredoc syntax to remove indentation.

waiting-for-dev · 2023-01-09T10:03:47Z

lib/generators/solidus_subscriptions/install/install_generator.rb

+          <<-RUBY.strip_heredoc.indent(2)
+
+            include SolidusSubscriptions::SubscriptionLineItemBuilder
+            after_action :handle_subscription_line_items, only: :create, if: ->{ params[:subscription_line_item] }


What about moving the injected code to a module and including that module in the host application? That would make it less brittle to installations that have already been modified.

@waiting-for-dev I'm not sure I understand, looks like it's already in a module being injected in the host application, what am I missing?

Wonderful idea, working on it

@elia I'm working on a concern (that will be copied along to the host app), so that its inclusion is the only thing we inject in the controller.

Oh I see thanks 👍

Pushed a new version that uses a Rails concern, WDYT?

elia

Awesome!
I hope we'll be able to also add testing soon, but for now this is great!

pending the nits pointed out in other reviews

elia · 2023-01-09T10:17:27Z

lib/generators/solidus_subscriptions/install/install_generator.rb

@@ -6,6 +6,7 @@ class InstallGenerator < Rails::Generators::Base
      source_root File.expand_path('templates', __dir__)

      class_option :auto_run_migrations, type: :boolean, default: false
+      class_option :frontend, type: :string, default: 'legacy'


Using a string instead of a boolean looks better to me, since more things could end up being frontends (api, graphql, etc) and being able to select the right integration seems like a better choice.

Anyways the name is classic in the solidus installer, and I'm fine with both fixing it or moving it to a boolean, we can change later if necessary 👍

elia · 2023-01-09T10:26:30Z

lib/generators/solidus_subscriptions/install/install_generator.rb

+          <<-RUBY.strip_heredoc.indent(2)
+
+            include SolidusSubscriptions::SubscriptionLineItemBuilder
+            after_action :handle_subscription_line_items, only: :create, if: ->{ params[:subscription_line_item] }


@waiting-for-dev I'm not sure I understand, looks like it's already in a module being injected in the host application, what am I missing?

...tors/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb

This works out of the box only for fresh install of the starter frontend because we can't anticipate what changed in each storefront, but should be a good starting point.

This way they will be loaded only when solidus_frontend is in use.

Auth devise is already installed with the new storefront, so we can remove a couple of options from solidus:install and the inclusion of the gem in the sandbox Gemfile. Also, we can remove enforce_available_locales, which is deprecated.

kennyadsl self-assigned this Jan 5, 2023

kennyadsl force-pushed the kennyadsl/support-starter-frontend branch from c65773f to 485b404 Compare January 5, 2023 14:30

Naokimi reviewed Jan 7, 2023

View reviewed changes

waiting-for-dev reviewed Jan 9, 2023

View reviewed changes

elia approved these changes Jan 9, 2023

View reviewed changes

kennyadsl force-pushed the kennyadsl/support-starter-frontend branch from 0436be2 to 6a2ba92 Compare January 9, 2023 10:54

waiting-for-dev approved these changes Jan 9, 2023

View reviewed changes

...tors/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb Show resolved Hide resolved

kennyadsl added 3 commits January 9, 2023 14:28

Support the new starter frontend installation

b652529

This works out of the box only for fresh install of the starter frontend because we can't anticipate what changed in each storefront, but should be a good starting point.

Move frontend partials to lib

7449ffb

This way they will be loaded only when solidus_frontend is in use.

Streamline sandbox creation

a1a2c0c

Auth devise is already installed with the new storefront, so we can remove a couple of options from solidus:install and the inclusion of the gem in the sandbox Gemfile. Also, we can remove enforce_available_locales, which is deprecated.

kennyadsl force-pushed the kennyadsl/support-starter-frontend branch from 6a2ba92 to a1a2c0c Compare January 9, 2023 13:28

kennyadsl mentioned this pull request Jan 10, 2023

Uniform installer with Solidus Starter Frontend solidusio/solidus_paypal_commerce_platform#184

Merged

7 tasks

kennyadsl merged commit 9d8eaca into master Jan 11, 2023

kennyadsl deleted the kennyadsl/support-starter-frontend branch January 11, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the new starter frontend installation #276

Support the new starter frontend installation #276

kennyadsl commented Jan 5, 2023 •

edited

Loading

Naokimi Jan 7, 2023

kennyadsl Jan 9, 2023

Naokimi Jan 9, 2023 •

edited

Loading

kennyadsl Jan 9, 2023

waiting-for-dev left a comment

waiting-for-dev Jan 9, 2023

elia Jan 9, 2023

kennyadsl Jan 9, 2023

elia Jan 9, 2023

kennyadsl Jan 9, 2023

waiting-for-dev Jan 9, 2023

waiting-for-dev Jan 9, 2023

waiting-for-dev Jan 9, 2023

elia Jan 9, 2023

kennyadsl Jan 9, 2023

kennyadsl Jan 9, 2023

elia Jan 9, 2023

kennyadsl Jan 9, 2023

waiting-for-dev Jan 9, 2023

elia left a comment

elia Jan 9, 2023

elia Jan 9, 2023

Support the new starter frontend installation #276

Support the new starter frontend installation #276

Conversation

kennyadsl commented Jan 5, 2023 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naokimi Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Jan 5, 2023 •

edited

Loading

Naokimi Jan 9, 2023 •

edited

Loading