Installer UI improvements #4675

elia · 2022-10-14T09:33:42Z

Summary

A few installer cleanups and removals of legacy options
Created some helpers for handling interactive options
Shortened the names of options to be provided to flags (e.g. solidus_starter_frontend → starter)
Improved the layout of interactive questions (see screenshots)

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.

This change allows to run samples within the app without breaking anything.

This is an ancient workaround that we no longer need to keep around. From https://github.com/rails/rails/blob/7-0-stable/guides/source/upgrading_ruby_on_rails.md?plain=1#L1876-L1878 Rails 4.1 now defaults the I18n option `enforce_available_locales` to `true`. This means that it will make sure that all locales passed to it must be declared in the `available_locales` list.

It didn't have an effect on anything for a long time now, was introduced in 3248380 when some assets were generated for extensions.

These paths are tightly coupled with routes defined in frontend extensions and we shouldn't assume anything about them in here.

There was a lingering single quote at the end of the string.

- Clearly describe available options - Align questions and answers - Give clear feedback when using defaults or for errors - Fix deprecation on --with-authentication (`options` can't be updated) - Ask for interactive choices upfront, having all the selected values at hand is helpful for checks

We already have :frontend and, :authentication, but we're also about to bring back :payment_method, and possibly others.

Not doing this results in stale Gemfile.lock information being provided by `Bundler.locked_gems`.

Makes it easier to understand and explain what they mean, in addition to being easier to type out.

waiting-for-dev

Thanks, Elia! I left some non-blocking comments. I haven't tested it, though, but I'm sure you have 🙂

sample/db/samples/assets.rb

core/lib/generators/solidus/install/install_generator.rb

waiting-for-dev · 2022-10-17T04:36:47Z

core/lib/generators/solidus/install/install_generator.rb


      # Silence verbose output (e.g. Rails migrations will rely on this environment variable)
      ENV['VERBOSE'] = 'false'

      # No reason to check for their presence if we're about to install them
      ENV['SOLIDUS_SKIP_MIGRATIONS_CHECK'] = 'true'

-      if options[:with_authentication] != nil
-        warn \


If we're removing the warning, can't we remove the option altogether now?

The warning is still there, just moved it to detect_authentication_to_install.

waiting-for-dev · 2022-10-17T04:41:14Z

core/lib/generators/solidus/install/install_generator.rb

-    LEGACY_FRONTEND = 'solidus_frontend'
-    DEFAULT_FRONTEND = 'solidus_starter_frontend'


Up to you, but do you think it is ok to remove the constants (same for DEFAULT_AUTHENTICATION below)? It helps to check for string values that can be repeated multiple times (e.g., avoiding a typo that would create a mysterious bug).

That's a good point, I'll try with a local variable first and fall back to a constant if that doesn't work

Context:
Those were removed because the role was a bit confusing, and with checks on the gemfile the default is actually dynamic (e.g. if you have the legacy one in the gemfile and auto_accept it will "default" to it, etc.).

Cool. That's non-blocking, though 🙂

elia

@waiting-for-dev added answers to all your comments, even if optional I'd like to have your take in an additional review if you have time 🙏

core/lib/generators/solidus/install/install_generator.rb

elia · 2022-10-17T11:05:31Z

core/lib/generators/solidus/install/install_generator.rb

-    LEGACY_FRONTEND = 'solidus_frontend'
-    DEFAULT_FRONTEND = 'solidus_starter_frontend'


That's a good point, I'll try with a local variable first and fall back to a constant if that doesn't work

Context:
Those were removed because the role was a bit confusing, and with checks on the gemfile the default is actually dynamic (e.g. if you have the legacy one in the gemfile and auto_accept it will "default" to it, etc.).

elia · 2022-10-17T11:08:31Z

core/lib/generators/solidus/install/install_generator.rb


      # Silence verbose output (e.g. Rails migrations will rely on this environment variable)
      ENV['VERBOSE'] = 'false'

      # No reason to check for their presence if we're about to install them
      ENV['SOLIDUS_SKIP_MIGRATIONS_CHECK'] = 'true'

-      if options[:with_authentication] != nil
-        warn \


The warning is still there, just moved it to detect_authentication_to_install.

core/lib/generators/solidus/install/install_generator.rb

kennyadsl

Thanks Eli, the installer looks way better now!

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 14, 2022

elia added 12 commits October 14, 2022 16:17

Don't pollute Object with a local helper method

a76e7f7

This change allows to run samples within the app without breaking anything.

Remove the legacy plugin support from solidus:install

5673630

Add a spec for the install generator options

ab07f28

Deprecate the --lib-name option of the solidus installer

09e8ba4

It didn't have an effect on anything for a long time now, was introduced in 3248380 when some assets were generated for extensions.

Handle robots.txt from frontend extensions

aec8fc4

These paths are tightly coupled with routes defined in frontend extensions and we shouldn't assume anything about them in here.

Remove typo from the migrations check message

e51e33b

There was a lingering single quote at the end of the string.

Extract running a template for a topic in solidus:install

531e718

We already have :frontend and, :authentication, but we're also about to bring back :payment_method, and possibly others.

Install frontend and authentication after running migrations

6f8d56d

Reset gemfile info after every bundler command

13b8148

Not doing this results in stale Gemfile.lock information being provided by `Bundler.locked_gems`.

Shorten the option names that need to be typed during installation

f52a746

Makes it easier to understand and explain what they mean, in addition to being easier to type out.

elia force-pushed the elia/installer-ui-improvements branch from bc54f59 to f52a746 Compare October 14, 2022 14:17

elia marked this pull request as ready for review October 14, 2022 15:08

waiting-for-dev approved these changes Oct 17, 2022

View reviewed changes

elia commented Oct 17, 2022

View reviewed changes

elia requested a review from waiting-for-dev October 17, 2022 11:14

kennyadsl approved these changes Oct 17, 2022

View reviewed changes

kennyadsl merged commit bb53603 into solidusio:master Oct 17, 2022

kennyadsl deleted the elia/installer-ui-improvements branch October 17, 2022 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Installer UI improvements #4675

Installer UI improvements #4675

elia commented Oct 14, 2022 •

edited

Loading

waiting-for-dev left a comment

waiting-for-dev Oct 17, 2022

elia Oct 17, 2022

waiting-for-dev Oct 17, 2022

elia Oct 17, 2022

waiting-for-dev Oct 17, 2022

elia left a comment

elia Oct 17, 2022

elia Oct 17, 2022

kennyadsl left a comment

		LEGACY_FRONTEND = 'solidus_frontend'
		DEFAULT_FRONTEND = 'solidus_starter_frontend'

Installer UI improvements #4675

Installer UI improvements #4675

Conversation

elia commented Oct 14, 2022 • edited Loading

Summary

Checklist

waiting-for-dev left a comment

Choose a reason for hiding this comment

waiting-for-dev Oct 17, 2022

Choose a reason for hiding this comment

elia Oct 17, 2022

Choose a reason for hiding this comment

waiting-for-dev Oct 17, 2022

Choose a reason for hiding this comment

elia Oct 17, 2022

Choose a reason for hiding this comment

waiting-for-dev Oct 17, 2022

Choose a reason for hiding this comment

elia left a comment

Choose a reason for hiding this comment

elia Oct 17, 2022

Choose a reason for hiding this comment

elia Oct 17, 2022

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

elia commented Oct 14, 2022 •

edited

Loading