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.

Sign up for GitHub

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

Jump to bottom

Copy routes/controllers/views/tests from auth_devise #172

Merged
merged 35 commits into from
Aug 11, 2021
Merged

Copy routes/controllers/views/tests from auth_devise #172

merged 35 commits into from
Aug 11, 2021

Conversation

gsmendoza
Copy link
Contributor

@gsmendoza gsmendoza commented Jul 29, 2021 •
edited
Loading

Goal

Allow devs to directly touch the authentication component of the storefront in their application.

Dependencies

Indirectly depends on #171. That PR adds a script for creating a sandbox with generated frontend, which is useful for testing this PR.

Walkthrough

https://www.loom.com/share/e58d10c8cc6b402e8408b1b3e70b375b

Demo

Changes

  • Copy storefront routes/controllers/views/tests from solidus_auth_devise into solidus_starter_frontend .
  • Minimize solidus_starter_frontend's dependency on solidus_auth_devise.
  • Ensure solidus_auth_devise loads frontend components only once, by ensuring that frontend_available? does not get set to true.

Alternatives considered

Chose to add the solidus_auth_devise frontend to solidus_starter_frontend instead of adding the generator to auth devise primarily because I found this to be the simpler approach. By having the checkout and the authentication code in the same project, it's easier to verify if they are working together.

The downside with this approach is that frontend code is currently duplicated here and in solidus_auth_devise. However, since we're planning to replace the old solidus_frontend with this gem, the code in frontend code in solidus_auth_devise will eventually get removed.

Previous attempts

Relevant PRs

Scope

These tasks are not covered in this PR and will be implemented in future PRs.

  • Investigate SolidusStarterFrontend::Views::OverrideGenerator and update if necessary.
  • Copy solidus_auth_devise unit tests to solidus_starter_frontend. For this PR, I found that the solidus_auth_devise feature specs were sufficient in verifying that the code migration has been set up properly.
  • Add an option to exclude solidus_auth_devise and its frontend components from the generated storefront.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [N/A] New feature (non-breaking change which adds functionality)
  • [N/A] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@gsmendoza gsmendoza changed the base branch from master to gsmendoza/eng-191-create-alternate-sandbox-script-which July 29, 2021 08:28
This was referenced Jul 29, 2021
Merged
Closed
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-115-copy-routescontrollersviewstests-from branch from 509d1ce to b0f5b72 Compare July 30, 2021 00:36
@gsmendoza gsmendoza marked this pull request as ready for review July 30, 2021 01:05
@gsmendoza gsmendoza self-assigned this Aug 1, 2021
@gsmendoza
Copy link
Contributor Author

Regarding the caching test failures at https://www.loom.com/share/3d8ab1ef74a5490eae16e4d4130a4eff, I found that this is due to config.cache_store being set to :null_store in the sandbox-generated app. Disabling config.cache_store = :null_store in config/environments/test.rb fixes the specs.

gsmendoza
gsmendoza commented Aug 5, 2021
View reviewed changes
spec/system/checkout_spec.rb
@@ -611,6 +617,7 @@

it "displays the entered state name without evaluating" do
Copy link
Contributor Author

@gsmendoza gsmendoza Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This example seems fails frequently. See for example, https://app.circleci.com/pipelines/github/nebulab/solidus_starter_frontend/879/workflows/778f9ce5-62d3-43b4-af6b-6e282b275815/jobs/2243/tests. In the test run I did in the video below, I got it to fail 40 out of 100 times.

I discovered that the example fails frequently under the following conditions:

  1. When signing in as a guest during checkout.
  2. When using the apparition JS driver.

However, under ANY of these conditions, the example succeeds most of the time:

  1. On the current master branch, where we don't have authentication yet.
  2. When signing in as a user during checkout.
  3. When using a different JS driver, like selenium_chrome_headless .

Please see the video below for more details.

https://www.loom.com/share/2a6db47101f24fcd8206361f8300d7f9

For the meantime, I'll add a commit to apply the workaround I did in the video, i.e. sign in as user instead of guest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed commit effe34d.

@gsmendoza gsmendoza force-pushed the gsmendoza/eng-191-create-alternate-sandbox-script-which branch from edd879d to 7a94c8b Compare August 6, 2021 10:06
waiting-for-dev
waiting-for-dev approved these changes Aug 6, 2021
View reviewed changes
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.

Amazing job @gsmendoza!

Base automatically changed from gsmendoza/eng-191-create-alternate-sandbox-script-which to master August 10, 2021 07:22
gsmendoza added 17 commits August 10, 2021 15:36
@gsmendoza
[BREAKS BUILD] Copy feature specs from solidus_auth_devise
969f182 
solidus_auth_devise commit version was
c03ca9a2a9eb1fa6e4421aff7b483c502f7151e0.
@gsmendoza
Remove backend solidus_auth_devise feature specs
f727619
@gsmendoza
Remove skipped spec
ff667d5
@gsmendoza
Copy solidus_auth_devise frontend routes
1b35df8 
Fixes

```
  1) Accounts editing can edit an admin user
     Failure/Error: visit spree.login_path

     NoMethodError:
       undefined method `login_path' for #<Module:0x0000563aafaae480>
     # ./spec/features/account_spec.rb:7:in `block (3 levels) in <top (required)>'
```
@gsmendoza
Move checkout solidus_auth_devise routes to checkout section
e34e9e6
@gsmendoza
Add solidus_auth_devise
6245ee4 
Fixes

```
  An error occurred while loading spec_helper.
  Failure/Error: Rails.application.initialize!

  NoMethodError:
    undefined method `devise_for' for #<ActionDispatch::Routing::Mapper:0x000055986b023a60>
  # ./config/routes.rb:6:in `block in <top (required)>'
  # ./config/routes.rb:3:in `<top (required)>'
  # ./spec/dummy/config/environment.rb:5:in `<top (required)>'
  # ./spec/spec_helper.rb:15:in `require'
  # ./spec/spec_helper.rb:15:in `<top (required)>'
```
@gsmendoza
Copy solidus_auth_devise controllers
544e8d0 
Fixes

```
    An error occurred while loading spec_helper.
    Failure/Error: Spree::UserConfirmationsController,

    NameError:
      uninitialized constant Spree::UserConfirmationsController
    # ./lib/solidus_starter_frontend/engine.rb:20:in `block in <class:Engine>'
    # ./spec/dummy/config/environment.rb:5:in `<top (required)>'
    # ./spec/spec_helper.rb:15:in `require'
    # ./spec/spec_helper.rb:15:in `<top (required)>'
```
@gsmendoza
Remove frontend_available? monkeypatch
7ba6cbf 
solidus_starter_frontend will be loading the authentication frontend
so we no longer want solidus_auth_devise to load it again.

Fixes

```
    An error occurred while loading spec_helper.
    Failure/Error: Rails.application.initialize!

    ArgumentError:
      Invalid route name, already in use: 'new_spree_user_session'
      You may have defined two routes with the same name using the `:as` option, or you may be overriding a route already defined by a resource with the same naming. For the latter, you can restrict the routes created with `resources` as explained here:
      https://guides.rubyonrails.org/routing.html#restricting-the-routes-created
    # ./spec/dummy/config/environment.rb:5:in `<top (required)>'
    # ./spec/spec_helper.rb:15:in `require'
    # ./spec/spec_helper.rb:15:in `<top (required)>'
```
@gsmendoza
Qualify Password field identifiers
479172f 
Fixes

```
  1) Accounts editing can edit a new user
     Failure/Error: fill_in 'Password', with: 'password'

     Capybara::Ambiguous:
       Ambiguous match, found 2 elements matching visible field "Password" that is not disabled
     # ./spec/features/account_spec.rb:22:in `block (3 levels) in <top (required)>'
```
@gsmendoza
Move views from lib to app
5d2db51 
Remove AuthViews concern since it's no longer needed.

Fixes

```
  1) Accounts editing can edit a new user
     Failure/Error: click_link 'Edit'

     AbstractController::ActionNotFound:
       The action 'edit' could not be found for Spree::UsersController
     # ./spec/features/account_spec.rb:28:in `block (3 levels) in <top (required)>'
```
@gsmendoza
Update Edit Account form field identifiers
9b287a7 
Fixes

```
  1) Change email work with correct password
     Failure/Error: fill_in 'user_email', with: '[email protected]'

     Capybara::ElementNotFound:
       Unable to find field "user_email" that is not disabled
     # ./spec/features/change_email_spec.rb:19:in `block (2 levels) in <top (required)>'
```
@gsmendoza
Inline solidus_auth_devise CheckoutControllerDecorator to CheckoutCon…
f86dd85 
…troller

Fixes

```
  1) Checkout leaving and returning to address step
     Failure/Error:
       within '#guest_checkout' do
         fill_in 'Email', with: '[email protected]'
       end

     Capybara::ElementNotFound:
       Unable to find css "#guest_checkout"
     # ./spec/features/checkout_spec.rb:36:in `block (2 levels) in <top (required)>'
```
@gsmendoza
Remove overridden checkout controller methods
8af71c1
@gsmendoza
Copy FillAddressFields
75f0cf4 
Fixes

```
  1) Checkout without payment being required allow a visitor to checkout as guest, without registration
     Failure/Error: fill_addresses_fields_with(address)

     NoMethodError:
       undefined method `fill_addresses_fields_with' for #<RSpec::ExampleGroups::Checkout::WithoutPaymentBeingRequired:0x00007f8714d278d0>
     # ./spec/features/checkout_spec.rb:63:in `block (3 levels) in <top (required)>'
```
@gsmendoza
Copy solidus_auth_devise UserMailer with views
30d1119 
Fixes

```
  1) Checkout without payment being required associate an incomplete guest order with user after successful password reset
     Got 0 failures and 2 other errors:

     1.1) Failure/Error: token = token_url_regex.match(reset_password_email.body.to_s)[1]

          NoMethodError:
            undefined method `body' for nil:NilClass
          # ./spec/features/checkout_spec.rb:115:in `block (3 levels) in <top (required)>'

     1.2) Failure/Error: self.resource = resource_class.send_reset_password_instructions(params[resource_name])

          ActionView::MissingTemplate:
            Missing template spree/user_mailer/reset_password_instructions with "mailer". Searched in:
              * "spree/user_mailer"
          # ./app/controllers/spree/user_passwords_controller.rb:19:in `create'
          # ------------------
          # --- Caused by: ---
          # Capybara::CapybaraError:
          #   Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
          #   /home/gsmendoza/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/capybara-3.35.3/lib/capybara/session.rb:160:in `raise_server_error!'
```
@gsmendoza
Fix spec "allow a user to register during checkout"
7a73d89 
Fixes

```
  1) Checkout without payment being required allow a user to register during checkout
     Failure/Error: expect(page).to have_text 'Registration'
       expected to find text "Registration" in "Search\nLOGIN\nCART: (1) $19.99\nLogin as Existing Customer\nEmail:\nPassword:\nRemember me\nLogin\nor Create a new account | Forgot Password?\nCheckout as a Guest\nEmail:\nContinue\nor Create a new account\nPowered by Solidus\nLanguage:\nCastellano (ES)\nCatalà\nDanish\nDeutsch (DE)\nDeutsch (Schweiz)\nEesti keel\nEnglish (Australia)\nEnglish (IN)\nEnglish (New Zealand)\nEnglish (UK)\nEnglish (US)\nEspañol\nEspañol (Chile)\nEspañol (México)\nFrançais (FR)\nIndonesian (ID)\nItaliano (IT)\nLatvijas (LV)\nNederlands (NL)\nNorsk\nPolski (PL)\nPortuguês (BR)\nPortuguês (PT)\nRomanian (RO)\nSlovenčina\nSlovenščina (SL)\nSuomi\nSvenska (SE)\nTürkçe (TR)\nUkrainian\ntiếng Việt (VN)\nČeština (CS)\nБългарски (БГ)\nРусский (RU)\nفارسی(fa)\nภาษาไทย (TH)\n中文 (繁體)\n中文(简体)\n日本語 (ja-JP)\n한국어 (KO)"
     # ./spec/features/checkout_spec.rb:136:in `block (3 levels) in <top (required)>'
```
@gsmendoza
Fix "Sign In let a user sign in successfully" spec
bc668ce 
Fixes

```
  1) Sign In let a user sign in successfully
     Failure/Error: expect(page).to have_text 'Logout'
       expected to find text "Logout" in "All departments Search\nMy Account\nCart\nLogged in successfully\nThe only eCommerce platform you’ll ever need.\nBuild, customize and scale your store with no limits or license fees. Solidus is the free, open-source eCommerce framework for digitally-native brands, fast-growing online businesses and pragmatic developers.\nNo products found\nPowered by Solidus\nLanguage: Castellano (ES) Català Danish Deutsch (DE) Deutsch (Schweiz) Eesti keel English (Australia) English (IN) English (New Zealand) English (UK) English (US) Español Español (Chile) Español (México) Français (FR) Indonesian (ID) Italiano (IT) Latvijas (LV) Nederlands (NL) Norsk Polski (PL) Português (BR) Português (PT) Romanian (RO) Slovenčina Slovenščina (SL) Suomi Svenska (SE) Türkçe (TR) Ukrainian tiếng Việt (VN) Čeština (CS) Български (БГ) Русский (RU) فارسی(fa) ภาษาไทย (TH) 中文 (繁體) 中文(简体) 日本語 (ja-JP) 한국어 (KO)"
     # ./spec/features/sign_in_spec.rb:16:in `block (2 levels) in <top (required)>'

```
gsmendoza added 17 commits August 10, 2021 15:36
@gsmendoza
Copy unauthorized_redirect from solidus_auth_devise
c873a10 
Fixes "Authorization Failure" error displayed on sign-in.

Fixes

```
  1) Sign In should store the user previous location
     Failure/Error: fill_in "Email", with: @user.email

     Capybara::ElementNotFound:
       Unable to find field "Email" that is not disabled
     # ./spec/features/sign_in_spec.rb:31:in `block (2 levels) in <top (required)>'
```
@gsmendoza
Fix 'allow a signed in user to logout' spec
f9e1b49 
Fixes

```
  1) Sign Out allow a signed in user to logout
     Failure/Error: click_link 'Logout'

     Capybara::ElementNotFound:
       Unable to find link "Logout"
     # ./spec/features/sign_out_spec.rb:21:in `block (2 levels) in <top (required)>'
```
@gsmendoza
Fix system specs that require checking out as guest
3828604
@gsmendoza
Fix expected completion routes in checkout specs
b5e0ebb
@gsmendoza
Update specs for unauthorized order requests
e00751a
@gsmendoza
Convert authentication feature specs to system specs
4ec315b
@gsmendoza
Include Devise::Test::ControllerHelpers in controller spec
4165a32 
Fixes

```
  1) Spree::ProductsController sets the default locale based off SolidusStarterFrontend::Config[:locale]
     Failure/Error: @searcher = build_searcher(params.merge(include_images: true))

     Devise::MissingWarden:
       Devise could not find the `Warden::Proxy` instance on your request environment.
       Make sure that your application is loading Devise and Warden as expected and that the `Warden::Manager` middleware is present in your middleware stack.
       If you are seeing this on one of your tests, ensure that your tests are either executing the Rails middleware stack or that your tests are using the `Devise::Test::ControllerHelpers` module to inject the `request.env['warden']` object for you.
     # ./app/controllers/spree/products_controller.rb:13:in `index'
     # ./spec/controllers/controller_helpers_spec.rb:26:in `block (2 levels) in <top (required)>'
```
@gsmendoza
[FIXES TEST SUITE] Sign in to request specs using Devise::Test::Integ…
58dc7a8 
…rationHelpers

Fixes intermittent system spec failures caused the mocks in the with_signed_in_user handler.
@gsmendoza
Inline inclusion of Taxonomies to auth_devise controllers
0940006
@gsmendoza
Ignore solidus_auth_devise controllers on solidus_compare
40cb474
@gsmendoza
Remove auth_views from solidus_compare
70c3713
@gsmendoza
Move unauthorized_redirect handler to initializer
774c6b7
@gsmendoza
Fix generator to copy solidus_auth_devise frontend files to app
22f0bf5
@gsmendoza
Indicate that solidus_auth_devise is currently installed by the gener…
4b1f2ba 
…ator
@gsmendoza
Copy controller and mailer specs from solidus_auth_devise
7387c68 
Include Devise::Test::ControllerHelpers
@gsmendoza
Fix intermittent checkout example
a88dee4 
The example fails frequently. See for example,
https://app.circleci.com/pipelines/github/nebulab/solidus_starter_frontend/879/workflows/778f9ce5-62d3-43b4-af6b-6e282b275815/jobs/2243/tests.
In the test run I did in the video below, I got it to fail 40 out of 100
times.

I discovered that the example fails frequently under the following conditions:

1. When signing in as a guest during checkout.
2. When using the apparition JS driver.

However, under ANY of these conditions, the example succeeds most of the time:

1. On the current master branch, where we don't have authentication yet.
2. When signing in as a user during checkout.
3. When using a different JS driver, like selenium_chrome_headless.

Please see the video below for more details.

https://www.loom.com/share/2a6db47101f24fcd8206361f8300d7f9
@gsmendoza
Update instructions to install Solidus with Auth Devise
bccabef
@gsmendoza gsmendoza force-pushed the gsmendoza/eng-115-copy-routescontrollersviewstests-from branch from 23a9e9e to bccabef Compare August 10, 2021 07:36
gsmendoza added a commit that referenced this pull request Aug 10, 2021
@gsmendoza
Add rspec-retry
08bbac2 
Goal
----

As a solidus_starter_frontend contributor
I want to add rspec-retry to the gem's development dependencies
So what we can retry the intermittent spec failures encountered from #172 (comment)
kennyadsl
kennyadsl approved these changes Aug 10, 2021
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Feel free to merge when ready. We can tackle the flaky spec issue in another PR, thanks!

@gsmendoza gsmendoza merged commit 602c117 into master Aug 11, 2021
@gsmendoza gsmendoza deleted the gsmendoza/eng-115-copy-routescontrollersviewstests-from branch August 11, 2021 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennyadsl kennyadsl kennyadsl approved these changes

@waiting-for-dev waiting-for-dev waiting-for-dev approved these changes

Assignees

@gsmendoza gsmendoza

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gsmendoza @waiting-for-dev @kennyadsl