Setup compatibility with the starter frontend #166

elia · 2022-10-19T14:36:57Z

Sart testing within a freshly generated, full fledged, dummy app that can fully run the app-template script from SSF.
The product and cart buttons are not working and the specs disabled as they're dependent on upstream support in SSF.
jQuery has been fully dropped in favor of vanilla JS and a few bugs (already present in master) were squashed.

gsmendoza

Hi @elia , @kennyadsl ! I still have to go through the JS code, so here's my initial feedback. No blockers :)

bin/dummy-app

bin/rspec

gsmendoza · 2022-11-24T08:26:25Z

bin/dummy-app

+
+# Stay away from the bundler env of the containing extension.
+function unbundled {
+  ruby -rbundler -e'b = proc {system *ARGV}; Bundler.respond_to?(:with_unbundled_env) ? Bundler.with_unbundled_env(&b) : Bundler.with_clean_env(&b)' -- $@


I'm getting the following errors and suggestions from ShellCheck:

Line 9: ruby -rbundler -e'b = proc {system *ARGV}; Bundler.respond_to?(:with_unbundled_env) ? Bundler.with_unbundled_env(&b) : Bundler.with_clean_env(&b)' -- $@ >> ^-- SC2068 (error): Double quote array expansions to avoid re-splitting elements. Line 18: --database=${DB:-sqlite3} \ ^-- SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: (apply this, apply all SC2086) --database="${DB:-sqlite3}" \ Line 39: unbundled bundle exec rails generate solidus:install --auto-accept --payment-method=none --no-seed --no-sample $@ >> ^-- SC2068 (error): Double quote array expansions to avoid re-splitting elements.

Nice! Didn't know about https://www.shellcheck.net/wiki/SC2068, fixing

Thanks @elia . I noticed you didn't change --database=${DB:-sqlite3} to --database="${DB:-sqlite3}". Are we keeping the unquoted version?

I don't remember if it was a conscious decision or I missed it, but I think it's fine given it's a single controlled word, in the case of $@ it was literally anything, would that be fine with you?

gsmendoza · 2022-11-24T08:51:50Z

spec/system/frontend/product_spec.rb

-    let(:paypal_payment_method) { create(:paypal_payment_method) }
-    let(:product) { create(:product, variants: [variant, variant_two]) }
-    let(:store) { create(:store) }
-    let(:variant) { create(:variant) }
-    let(:variant_two) { create(:variant) }


What's the RuboCop rule causing these let declarations to fail? I'm guessing it doesn't like that the before block just calls paypal_payment_method?

It was complaining about having too many lets.

@elia That's odd. I reset the branch to 66ac005 and ran rubocop on product_spec.rb. I didn't get any rubocop failures:

solidus_paypal_commerce_platform 09:48:33 $ bundle exec rubocop spec/system/frontend/product_spec.rb --debug For /home/gsmendoza/repos/solidusio/solidus_paypal_commerce_platform: configuration from /home/gsmendoza/repos/solidusio/solidus_paypal_commerce_platform/.rubocop.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/solidus_dev_support-2.5.5/lib/solidus_dev_support/rubocop/config.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-rspec-2.15.0/config/default.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-rspec-2.15.0/config/default.yml Default configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-1.39.0/config/default.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-rails-2.17.3/config/default.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-rails-2.17.3/config/default.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-performance-1.15.1/config/default.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/rubocop-performance-1.15.1/config/default.yml configuration from /home/gsmendoza/.rbenv/versions/2.7.6/lib/ruby/gems/2.7.0/gems/solidus_dev_support-2.5.5/lib/solidus_dev_support/rubocop/config.yml Use parallel by default. Skipping parallel inspection: only a single file needs inspection Inspecting 1 file Scanning /home/gsmendoza/repos/solidusio/solidus_paypal_commerce_platform/spec/system/frontend/product_spec.rb . 1 file inspected, no offenses detected

@elia In any case, I'm guessing it's https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers? Maybe it's a matter of taste, but I prefer the let definitions here over the updated before block with assignments. I like the declarative feeling of the let code over the procedural feeling of the before block.

A quick glance at StackOverflow indicates that not everyone is a fan of it, and in fact, it's disabled in rubocop and rubocop-ast.

What are your thoughts about disabling the rule?

Here's the discussion in rubocop-rspec about the MultipleMemoizedHelpers cop: rubocop/rubocop-rspec#862.

Last one: rubocop/rubocop#8491

...tes/app/assets/javascripts/spree/frontend/solidus_paypal_commerce_platform/button_actions.js

gsmendoza

@elia @kennyadsl I'm done with my review :) I looked into the JS files, primarily the button_actions.js. I'm not up-to-date with JavaScript, so please bear with me :)

...tes/app/assets/javascripts/spree/frontend/solidus_paypal_commerce_platform/button_actions.js

gsmendoza

Hi @elia Thanks for making the changes! With regard to the let declarations, I'm okay with the change as it currently is, but I think we could explore other ways to improve the test code while passing the RuboCop rule:

Extract setup of records to factories.
Move down let declarations to the tests where they are relevant.

waiting-for-dev

Thanks for leading it, @elia ❤️

I left some minor suggestions. I do have a more serious concern about the way we're trying to detect if SSF is being used, but I don't want to block your progress. It'd good to discuss it before releasing the new version, though.

bin/dummy-app

bin/sandbox

waiting-for-dev · 2022-11-25T13:05:08Z

bin/rails-dummy-app

+
+app_root = 'dummy-app'
+
+unless File.exist? "#{app_root}/bin/rails"


We're testing the existence of the dummy application in the current directory, but then we'll create it on the root folder regardless, right?

The current directory when calling scripts inside bin/ is (usually?) the root folder. I took the script from the sandbox one, so I didn't question it too much, but in any case we're using __dir__ just to reach the creation script.

Happy to make it more strict if you think it could be run from the wrong current dir.

I think it'd be an easy fix, so I think it's worth it to change it to:

Suggested change

unless File.exist? "#{app_root}/bin/rails"

unless File.exist? "#{__dir__}#{app_root}/bin/rails"

That way we'll be consistent and expect the dummy app to be always in the root directory. WDYT?

waiting-for-dev · 2022-11-25T14:43:18Z

Gemfile

-                                    else
-                                      %w[solidusio/solidus] * 2
-                                    end
-gem 'solidus', github: solidus_git, branch: branch


Why are we removing the option of trying the extension with different Solidus versions? 🤔

The different versions are handled at the dummy-app level, we could keep that, but since we're not running much within the extension anymore it's no longer required.

Not sure I follow. We still have a bunch of models we probably want to ensure they're working with different Solidus versions. How will we run CI on older versions if we remove the expected logic around env vars? 🤔

Although we don't have the CI harness to leverage that yet, we still have the env var support in bin/dummy-app here

solidus_paypal_commerce_platform/bin/dummy-app

Line 32 in f9b9b31

unbundled bundle add solidus --github solidusio/solidus --branch "${BRANCH:-master}" --version '> 0.a'

I think we can use that without the need to also handle it in the main Gemfile, wdyt?

I see. Probably the decision here is coupled to #166 (comment)

Yes indeed, the current setup doesn't run specs through this Gemfile anymore 👍

Gemfile

lib/generators/solidus_paypal_commerce_platform/install/install_generator.rb

waiting-for-dev · 2022-11-25T15:03:42Z

lib/generators/solidus_paypal_commerce_platform/install/install_generator.rb

-      # This should only be used by the solidus installer prior to v3.3.
-      class_option :skip_migrations, type: :boolean, default: false, hide: true
+      class_option :migrate, type: :boolean, default: false
+      class_option :specs, type: :string, enum: %w[all frontend], default: 'frontend'


Do you think it's even worth it to provide the all option?

That's used by the CI and in development (by bin/rspec and bin/dummy-app), I guess it won't be very popular among end-users, open to alternative suggestions tho.

I see. What do you think about modifying bin/rspec to do two things:

Run the frontend specs within the dummy app.

Run the rest of the regular specs from the extension root directory.

In my view, it communicates better what we want to do, as it doesn't make sense to test the code of a dependency from within the application.

@waiting-for-dev this would also be my preference, but I'm still concerned if that's the right approach because:

We will end up having two different approaches for testing things. This means having two different paths to maintain, two different spec_helpers and dependencies to maintain (we are relying on the spec_helper and test dependencies added with the SSF now), and more importantly two paths to document and explain to people that want to contribute.

I've been thinking if there are possibilities of having feature specs that involve both frontend and backend, like a full journey from changing an admin preference to how that is reflected in the storefront. In that case, it would be impossible.

@kennyadsl, I see your point. But I think that's what makes sense. In the end, if we need two different approaches is because they're two different things. I don't think it'll help us hide that complexity under the carpet. On the contrary, blurring that distinction in development will make it too easy to cross boundaries and regret if there's a moment where we find a more modular approach to the frontend. Think of it as mixing two different libraries in a single one. If we decide to extract the SSF code to another repo at one point, ideally, it should only be a matter of moving files around.

I understand that the dummy app was only used as support to test integration tests (which has now been integrated into SSF), but unit tests didn't use that.

This is true only partially, the space in which any extension is moving is that of Rails engines/railties, so the initialization process of rails is required for testing anything inside app/ and inside lib/ if it interacts in any way with models or Solidus core.

I fully get the point of being "surprised" by an unusual setup (but it will be usual once we start using it in our ecosystem), but don't fully get how the difference is essential (I see it as very blurry), can you please expand on it?

Hey, @elia. When you test a Rails model at the unit level, you only require its file, but you don't need to go through all Rails initialization processes. That should also be the case for our models inheriting Solidus ones. Does it make sense?

However, my point is that we're thinking about it as a temporary solution, so we shouldn't make the process consolidated to a point where we remove the line in our development pipeline, making it too easy to make the wrong assumptions and 1) breaking apps not using SSF & 2) making our life harder once we come with a definitive solution (like the solidus_checkout extension you proposed).

@waiting-for-dev what I meant is that even to test a model Rails needs to be initialized so you get all the paths and autoloading to work, not to mention the db configuration and all that jazz. Only true POROs will be runnable by themselves, but that's usually a tiny minority of what it's done.

With regards with this being temporary I see it as viable for every extension, and we're already forced to use env-vars and the CI to have a matrix of combinations (e.g. if you want to test with and without active-storage). That would cover also something like solidus_checkout.

@waiting-for-dev what I meant is that even to test a model Rails needs to be initialized so you get all the paths and autoloading to work, not to mention the db configuration and all that jazz. Only true POROs will be runnable by themselves, but that's usually a tiny minority of what it's done.

@elia, that's a fair point.

With regards with this being temporary I see it as viable for every extension, and we're already forced to use env-vars and the CI to have a matrix of combinations (e.g. if you want to test with and without active-storage). That would cover also something like solidus_checkout.

I'm not sure I understand here. Yeah, we're forced to use env vars no matter what. But I don't think that we should diverge from the standard way of testing engines unless there's a compelling reason. And the reason I see now is making our temporary solution easy.

We agreed in a slack chat to handle this separately since it doesn't affect the release, but only the development experience

.../install/templates/app/assets/javascripts/spree/frontend/solidus_paypal_commerce_platform.js

elia

@waiting-for-dev I tried to address/answer everything

bin/dummy-app

elia · 2022-11-25T15:57:16Z

bin/rails-dummy-app

+
+app_root = 'dummy-app'
+
+unless File.exist? "#{app_root}/bin/rails"


The current directory when calling scripts inside bin/ is (usually?) the root folder. I took the script from the sandbox one, so I didn't question it too much, but in any case we're using __dir__ just to reach the creation script.

Happy to make it more strict if you think it could be run from the wrong current dir.

bin/sandbox

elia · 2022-11-25T16:18:04Z

lib/generators/solidus_paypal_commerce_platform/install/install_generator.rb

-      # This should only be used by the solidus installer prior to v3.3.
-      class_option :skip_migrations, type: :boolean, default: false, hide: true
+      class_option :migrate, type: :boolean, default: false
+      class_option :specs, type: :string, enum: %w[all frontend], default: 'frontend'


That's used by the CI and in development (by bin/rspec and bin/dummy-app), I guess it won't be very popular among end-users, open to alternative suggestions tho.

lib/generators/solidus_paypal_commerce_platform/install/install_generator.rb

Gemfile

elia · 2022-11-25T17:28:18Z

Gemfile

-                                    else
-                                      %w[solidusio/solidus] * 2
-                                    end
-gem 'solidus', github: solidus_git, branch: branch


The different versions are handled at the dummy-app level, we could keep that, but since we're not running much within the extension anymore it's no longer required.

waiting-for-dev · 2022-11-28T05:10:44Z

bin/rails-dummy-app

+  end
+end
+
+Dir.chdir app_root


Suggested change

Dir.chdir app_root

Dir.chdir "#{__dir__}#{app_root}"

Worth it to directly prepend app_root with __dir__?

We'll use the dummy app to run specs in a full-fledged target app. The sandbox script also got a couple of updates/fixes in parallel with what was don in bin/dummy-app.

Defaults to not run migrations.

Rely on the SSF spec helper and only add to support things that are specific to this extension (e.g. factories and helpers).

That allows better compatibility with what the SSF expects.

Everything now happens in the dummy-app gemfile.

Frontend paths are no longer defined within the Spree::Core routes and are now coming directly from the main app. Removed `Spree::` namespaces where they're no longer needed. Adjusted selectors for the updated HTML. Some specs have been changed to reduce the usage of `let` and appease rubocop. Use vanilla JS instead of relying on `window.Spree` and jQuery, which are no longer available in SSF.

Checking the installed gems is not completely reliable, especially when it comes to checking their absence. This more explicit mode allows for more control and good defaults.

elia self-assigned this Oct 19, 2022

elia force-pushed the elia+kennyadsl/ssf-support branch 3 times, most recently from f318a0b to 6dee325 Compare October 20, 2022 13:06

elia force-pushed the elia+kennyadsl/ssf-support branch from beec87e to ecda961 Compare November 7, 2022 12:56

elia mentioned this pull request Nov 7, 2022

Support for product and cart checkout buttons from payment methods solidusio/solidus_starter_frontend#278

Closed

elia force-pushed the elia+kennyadsl/ssf-support branch 7 times, most recently from 93327b7 to d4f01d1 Compare November 15, 2022 12:32

elia force-pushed the elia+kennyadsl/ssf-support branch 2 times, most recently from 77cd8e2 to 2196ccf Compare November 21, 2022 17:34

elia marked this pull request as ready for review November 21, 2022 17:47

elia force-pushed the elia+kennyadsl/ssf-support branch from 2196ccf to 670aac9 Compare November 22, 2022 09:36

elia added this to the v1.0 milestone Nov 22, 2022

elia force-pushed the elia+kennyadsl/ssf-support branch 8 times, most recently from e8e9201 to 0ec4a33 Compare November 23, 2022 11:40

elia marked this pull request as draft November 23, 2022 11:51

elia marked this pull request as ready for review November 23, 2022 12:29

gsmendoza reviewed Nov 24, 2022

View reviewed changes

kennyadsl mentioned this pull request Nov 24, 2022

Release 1.0.0.beta1 compatible with the new starter frontend #170

Closed

elia force-pushed the elia+kennyadsl/ssf-support branch from d125afc to c940f60 Compare November 24, 2022 16:15

elia requested review from gsmendoza and kennyadsl November 24, 2022 16:16

gsmendoza approved these changes Nov 25, 2022

View reviewed changes

elia force-pushed the elia+kennyadsl/ssf-support branch from c940f60 to 9ce3e1b Compare November 25, 2022 11:23

waiting-for-dev approved these changes Nov 25, 2022

View reviewed changes

Rename the spec helper to avoid conflicts with the target app

8722de9

elia force-pushed the elia+kennyadsl/ssf-support branch from 9ce3e1b to f9b9b31 Compare November 25, 2022 17:11

elia commented Nov 25, 2022

View reviewed changes

elia requested a review from waiting-for-dev November 25, 2022 17:34

waiting-for-dev reviewed Nov 28, 2022

View reviewed changes

elia added 12 commits November 28, 2022 12:02

Add a dummy app generator script

65790de

We'll use the dummy app to run specs in a full-fledged target app. The sandbox script also got a couple of updates/fixes in parallel with what was don in bin/dummy-app.

Use a single option to control migrations

8a8a31c

Defaults to not run migrations.

Streamline the spec helper

b65803c

Rely on the SSF spec helper and only add to support things that are specific to this extension (e.g. factories and helpers).

Move feature specs under spec/system

d005263

That allows better compatibility with what the SSF expects.

Cleanup the main Gemfile

be460ec

Everything now happens in the dummy-app gemfile.

Stick to the capybara js driver provided by SSF

f14087e

Run specs within the dummy-app/

f9c5461

Switch from SF to SSF in the install generator

fcbfd53

Skip product and cart buttons until there's support on SSF's side

2337b77

Fix typos in the readme

8840af2

Use explicit options to control which components get installed

002dfe8

Checking the installed gems is not completely reliable, especially when it comes to checking their absence. This more explicit mode allows for more control and good defaults.

elia force-pushed the elia+kennyadsl/ssf-support branch from f9b9b31 to 002dfe8 Compare November 28, 2022 12:19

elia mentioned this pull request Nov 28, 2022

RFC: Overhauling solidus_stripe solidusio/solidus_stripe#135

Closed

3 tasks

kennyadsl mentioned this pull request Nov 29, 2022

Decide about the testing approach for extensions that involve SSF code #171

Closed

elia merged commit b2558c1 into master Dec 5, 2022

elia deleted the elia+kennyadsl/ssf-support branch December 5, 2022 13:05

elia mentioned this pull request Dec 13, 2022

Re-create the extension with solidus extension solidus_stripe solidusio/solidus_stripe#141

Closed

3 tasks


		app_root = 'dummy-app'

		unless File.exist? "#{app_root}/bin/rails"

	unless File.exist? "#{app_root}/bin/rails"
	unless File.exist? "#{__dir__}#{app_root}/bin/rails"

Setup compatibility with the starter frontend #166

Setup compatibility with the starter frontend #166

Conversation

elia commented Oct 19, 2022 • edited Loading

gsmendoza 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

gsmendoza left a comment

Choose a reason for hiding this comment

gsmendoza left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elia commented Oct 19, 2022 •

edited

Loading