Provide a default _navigation_footer.html.erb #1450

jrochkind · 2016-09-16T20:28:32Z

Prior to this PR, backend/app/views/spree/admin/shared/_navigation_footer.html.erb
is an empty file, save for a comment.

solidus_auth_devise overrides the partial to provide basic 'you are logged in as',
'logout' etc functionality in the admin sidebar.

However, there's nothing really devise-specific about this functionality.
We can easily implement it just in terms of ordinary spree, and the
methods that any auth solution is expected to provide, like spree_logout_path

This will make it easier on custom auth, no reason to have to figure out what to do
here on your own to get equiv func to the solidus_auth_devise standard when we
can provide a default. Custom auth could still override this partial if wanted/needed, but
this default should ordinarily suffice.

solidus_auth_devise can keep overriding it, we don't hurt it any by providing a default
implementation, instead of just a blank file, for it to replace.

The actual implementation here was straightforward -- figuring out how to test it
was a colossal pain. It's kind of kludgey in a couple ways, but I tried nearly many
approaches before arriving at what's here as least evil I could find.

houndci-bot · 2016-09-16T20:28:41Z

backend/spec/test_routes/add_spree_root_routes.rb

@@ -0,0 +1,3 @@
+Spree::Core::Engine.routes.draw do
+  root to: lambda {|env| [200, {'Content-Type'  => 'text/plain'}, ["Home page"]]}


Space between { and | missing.
Unused block argument - env. If it's necessary, use _ or _env as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.
Space inside { missing.
Operator => should be surrounded by a single space.
Space inside } missing.
Space missing inside }.

houndci-bot · 2016-09-16T20:28:41Z

backend/spec/test_routes/add_spree_root_routes.rb

@@ -0,0 +1,3 @@
+Spree::Core::Engine.routes.draw do
+  root to: lambda {|env| [200, {'Content-Type'  => 'text/plain'}, ["Home page"]]}


Space between { and | missing.
Unused block argument - env. If it's necessary, use _ or _env as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.
Space inside { missing.
Operator => should be surrounded by a single space.
Space inside } missing.
Space missing inside }.

soldius_auth_devise currently overrides with it's own. That's fine, but we can provide a default that should work fine for most custom auth solutions (rails g spree:custom_user)

mtomov · 2016-09-16T21:31:39Z

backend/spec/features/admin/auth_nav_spec.rb

+      end
+    end
+  end
+end


Why don't you try writing a view spec. Those are usually easy to write, very fast, and you can mock the hell out of it. I don't think that you need to test rails routing, and that spree_logout_path and such exist for real. It's a great overdo for such a view functionality.

okay if the consensus is a view spec is correct and sufficient, I can do that.

huh, under the new github system it looks like hound comments don't go away even if you fix em and force push? THAT's annoying.

@mtomov I'll change to a view spec, that does make sense, backend previously had no view specs at all, which is maybe what discouraged me from using it before. Hopefully that'll deal with the test failure from unreliable spec too.

mtomov · 2016-09-19T13:22:31Z

Yeah, hopefully it won't require you to go through leaps and bounds to test
this.

On Mon, Sep 19, 2016 at 2:07 PM, Jonathan Rochkind <[email protected]

wrote:

@jrochkind commented on this pull request.

In backend/spec/features/admin/auth_nav_spec.rb
#1450:
 reloader.reload!
end
after do
 @load_extra_routes = false
 Rails.application.routes_reloader.reload!
end
it "has a back to store link" do
 visit spree.admin_path
 within("#login-nav") do
   page.find_link Spree.t(:back_to_store), href: "/"
 end
end

end
+end
@mtomov https://github.com/mtomov I'll change to a view spec, that does
make sense, backend previously had no view specs at all, which is maybe
what discouraged me from using it before. Hopefully that'll deal with the
test failure from unreliable spec too.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1450, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABk0JhEsPL4X_NR7YEpz440c1zy5YHHsks5qrokMgaJpZM4J_Uad
.

jrochkind · 2016-09-19T15:06:36Z

@mtomov switched to view spec. Github's recent changes seem to mean that rubocop's warnings that no longer apply to any code in this branch stay there forever, as does our comments on source code lines no longer in this branch. Kind of annoying.

mtomov

I like the view specs very much, very easy to understand and follow 👍

mtomov · 2016-09-19T16:10:51Z

backend/spec/views/spree/admin/shared/navigation_footer_spec.rb

+require 'spec_helper'
+
+describe "spree/admin/shared/_navigation_footer", type: :view do
+  let(:user) { create(:admin_user) }


Theoretically, even Spree::User.new should do fine here

is it possible for an ordinary Spree::User to be viewing the admin backend at all? I guess it doesn't matter for a view test though.

I believe it does need to be a saved user (with an id) (or something mocked similarly), so generating a path to the admin user edit page for the user will work.

I am wary of mocking too much, because of other spree/solidus tests that mock so much they become unreliable.

I think it's prob okay the way it is, most 'realistic' test scenario, but if you require a different approach here just let me know. I suppose it can be create(:user), or a built non-saved user with it's id mocked out so a path can still be generated.... that latter one is not attractive to me for several reasons though.

Spree.user_class.new can be used. Of course, as you see fit. Just in terms of speed, not persisting data where possible I think is preferable.

And yeah .. making the start with the first view spec is indeed the toughest, so great on doing that! Might encourage others to follow suite as they are easy to write, and quick to run.

I don't have anything else, maybe the others can have their say.

Spree.user_class.new is quicker than factorygirl? I guess it can be build instead of create if it doesn't need to persist, but I feel like it oughta be an admin user?

Yes, new is faster. FactoryGirl.build is marginally slower, as it will persist associated objects. For now though I think it's fine, let's deal with this in a subsequent PR. The answer to the conundrum would probably FactoryGirl.build_stubbed.

jrochkind · 2016-09-19T16:23:26Z

I don't understand the CI failure, and think it is unrelated to this PR?

houndci-bot · 2016-09-19T16:39:33Z

backend/spec/views/spree/admin/shared/navigation_footer_spec.rb

+      expect(rendered).to have_link(Spree.t(:back_to_store), href: "/")
+    end
+  end
+


Extra empty line detected at block body end.

jrochkind · 2017-01-05T21:21:08Z

ping? @mtomov? Anything else I can do?

mtomov · 2017-03-23T23:23:02Z

Looks good!

jarednorman · 2017-05-22T09:41:46Z

backend/app/views/spree/admin/shared/_navigation_footer.html.erb

-<!-- solidus_auth_devise inserts a login_nav here.
-<ul id="login-nav" class="admin-login-nav"></ul>
+<!-- solidus_auth_devise replaces this partial with it's own login_nav.
+     But we provide a defaut implementation below that should work with


Typo "defaut".

mamhoff

I'm for this despite the typo and the DB hit in the view spec.

mamhoff · 2017-10-04T10:38:25Z

backend/spec/views/spree/admin/shared/navigation_footer_spec.rb

+require 'spec_helper'
+
+describe "spree/admin/shared/_navigation_footer", type: :view do
+  let(:user) { create(:admin_user) }


Yes, new is faster. FactoryGirl.build is marginally slower, as it will persist associated objects. For now though I think it's fine, let's deal with this in a subsequent PR. The answer to the conundrum would probably FactoryGirl.build_stubbed.

tvdeyen

Thanks

Caught in a PR review (solidusio#1450), and merged.

houndci-bot reviewed Sep 16, 2016

View reviewed changes

Provide a default _navigation_footer.html.erb

83e9a50

soldius_auth_devise currently overrides with it's own. That's fine, but we can provide a default that should work fine for most custom auth solutions (rails g spree:custom_user)

jrochkind force-pushed the default_backend_navigation_footer branch from 48dc6a7 to 83e9a50 Compare September 16, 2016 20:43

mtomov reviewed Sep 16, 2016

View reviewed changes

jrochkind added 2 commits September 19, 2016 10:27

remove accidental byebug

b03c024

test default navigation_footer with view spec not feature spec

9d11159

mtomov reviewed Sep 19, 2016

View reviewed changes

houndci-bot reviewed Sep 19, 2016

View reviewed changes

jarednorman reviewed May 22, 2017

View reviewed changes

mamhoff approved these changes Oct 4, 2017

View reviewed changes

tvdeyen approved these changes Oct 4, 2017

View reviewed changes

tvdeyen merged commit f91a0fd into solidusio:master Oct 4, 2017

gregdaynes pushed a commit to gregdaynes/solidus that referenced this pull request Oct 4, 2017

Fix typo in backend/navigation_footer

768d522

Caught in a PR review (solidusio#1450), and merged.

gregdaynes mentioned this pull request Oct 4, 2017

Default backend navigation footer #2261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a default _navigation_footer.html.erb #1450

Provide a default _navigation_footer.html.erb #1450

jrochkind commented Sep 16, 2016 •

edited

Loading

houndci-bot Sep 16, 2016

houndci-bot Sep 16, 2016

mtomov Sep 16, 2016

jrochkind Sep 16, 2016

jrochkind Sep 16, 2016

jrochkind Sep 19, 2016

mtomov commented Sep 19, 2016

@jrochkind commented on this pull request.

jrochkind commented Sep 19, 2016

mtomov left a comment

mtomov Sep 19, 2016

jrochkind Sep 19, 2016

mtomov Jan 5, 2017 •

edited

Loading

jrochkind Jan 5, 2017

mamhoff Oct 4, 2017

jrochkind commented Sep 19, 2016

houndci-bot Sep 19, 2016

jrochkind commented Jan 5, 2017

mtomov commented Mar 23, 2017

jarednorman May 22, 2017

mamhoff left a comment

mamhoff Oct 4, 2017

tvdeyen left a comment

		@@ -0,0 +1,3 @@
		Spree::Core::Engine.routes.draw do
		root to: lambda {\|env\| [200, {'Content-Type' => 'text/plain'}, ["Home page"]]}

Provide a default _navigation_footer.html.erb #1450

Provide a default _navigation_footer.html.erb #1450

Conversation

jrochkind commented Sep 16, 2016 • edited Loading

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

mtomov commented Sep 19, 2016

@jrochkind commented on this pull request.

jrochkind commented Sep 19, 2016

mtomov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomov Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrochkind commented Sep 19, 2016

Choose a reason for hiding this comment

jrochkind commented Jan 5, 2017

mtomov commented Mar 23, 2017

Choose a reason for hiding this comment

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

jrochkind commented Sep 16, 2016 •

edited

Loading

mtomov Jan 5, 2017 •

edited

Loading