-
Notifications
You must be signed in to change notification settings - Fork 0
A user logs out #11
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.
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
base: master
Are you sure you want to change the base?
A user logs out #11
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,5 @@ | |
|
|
||
| resources :users, only: %i[new create] | ||
| resources :sessions, only: %i[new create] | ||
|
|
||
| delete 'sessions', as: 'logout' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| module Web | ||
| module Controllers | ||
| module Sessions | ||
| class Destroy | ||
| include Web::Action | ||
|
|
||
| prepend_before :skip_authentication! | ||
|
|
||
| def call(_params) | ||
| process | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def process | ||
| warden.logout | ||
| flash[:notice] = 'Successfully logged out' | ||
| redirect_to routes.root_path | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| RSpec.describe Web.routes do | ||
| describe 'DELETE session' do | ||
| let(:generated_path) { described_class.path(:logout) } | ||
| let(:env) { Rack::MockRequest.env_for('/sessions', method: 'DELETE') } | ||
| let(:route) { described_class.recognize(env) } | ||
|
|
||
| it 'has a generated path' do | ||
| expect(generated_path).to eq('/sessions') | ||
| end | ||
|
|
||
| it 'is a recognisable route' do | ||
| expect(route).to be_routable | ||
| expect(route.path).to eq('/sessions') | ||
| expect(route.verb).to eq('DELETE') | ||
| end | ||
| end | ||
| end | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you do end up adding destroy to the resources array then presumably this spec is no longer needed? I've actually never used routing specs before but I'm guessing they're only needed for custom routes?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the fact that it's a custom route makes it any more or less deserving of tests. These are just unit tests to test a piece of functionality - the functionality here being that the expected route has been generated. Currently our other routes are being tested indirectly through feature specs, but since we don't have one of those for logging out yet I didn't want to add an untested piece of code to the codebase. I followed the Hanami guides for this: https://guides.hanamirb.org/routing/testing/
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting.. there's a part of me that just thinks this is excessive. It seems to be basically testing that the Hanami routes library is working correctly, rather than testing what we've been doing. If we write a spec for one of these routes then there's no argument really against writing one for all of them. Does that seem worthwhile to you? That's not rhetorical, genuinely interested to hear your thoughts.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm to be honest I don't see this as testing the Hanami routes library, I see it more as checking that the delete sessions route exists for our application and is working as expected. The reason I added this test is because I was adding functionality to our application which is not covered by an integration-style feature test like our other routes are (as there is no feature that uses this route yet). So I added these specs to ensure that this new unit of code is tested. If I hadn't, then it would be an untested piece of functionality, meaning that for all I know it could be broken or not behaving as expected - or if it was changed or removed then there would be no failing tests to reflect this. Having individual unit tests for routes also means that if you change or remove the feature test, then you can rest assured that this unit of code is still properly tested in isolation. We already follow this principle in other areas of the codebase such as having a test for I personally I like the belt and braces approach of having both unit tests and overall feature tests to ensure the test coverage is robust, but if you feel it's excessive in this instance then I'm happy to leave these tests out for routes. This was my thinking behind adding these tests, but obviously I'm still very new to Hanami so interested to hear your thoughts |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| RSpec.describe Web::Controllers::Sessions::Destroy, type: :action do | ||
| let(:action) { described_class.new } | ||
| let(:response) { action.call(params) } | ||
| let(:params) { Hash[] } | ||
| let(:warden) { instance_double Warden::Proxy } | ||
|
|
||
| before do | ||
| allow(action).to receive(:warden).and_return(warden) | ||
| allow(warden).to receive(:logout) | ||
| allow(warden).to receive(:user) | ||
| response | ||
| end | ||
|
|
||
| it 'calls the `logout` method on warden' do | ||
| expect(warden).to have_received(:logout) | ||
| end | ||
|
|
||
| it 'returns the correct response code' do | ||
| expect(response[0]).to eq 302 | ||
| end | ||
| end |
Uh oh!
There was an error while loading. Please reload this page.