Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
AO3-3848 Added Feature test for a locked user account #3135
Conversation
hatal175
added some commits
Nov 1, 2017
| @@ -158,6 +158,15 @@ | ||
| click_button "Create" | ||
| end | ||
| +When /^I login username "([^"]*)" and password "([^"]*)" for ([0-9]*) times$/ do |login, password, times| | ||
| + for i in 1..times.to_i do |
sarken
added
the
Awaiting review
label
Nov 2, 2017
hatal175
changed the title from
AO3-3848
to
AO3-3848 Added Feature test for a locked user account
Nov 2, 2017
| @@ -158,6 +158,15 @@ | ||
| click_button "Create" | ||
| end | ||
| +When /^I login username "([^"]*)" and password "([^"]*)" for ([0-9]*) times$/ do |login, password, n| | ||
| + n.to_i.times { |
sarken
approved these changes
Nov 12, 2017
Verified that this is working on production, so the test looks good!
sarken
added
Reviewed: Ready to Merge
and removed
Awaiting review
labels
Nov 12, 2017
|
It could be better if I could reduce the number of attempts for the test,
but I'm not sure how to do that.
…
|
|
That's a good point. It would be easier to do via Cucumber if this were a setting in our config file, which it really should be, and I'd normally suggest making that change. However, we're switching our user authentication to Devise soon and won't have time to merge a code change to Authlogic before then, so it would be somewhat pointless to ask you to do that. Instead, it might be better to rewrite this as an Rspec test and set the user's |
sarken
added
Reviewed: Needs Coder Action
and removed
Reviewed: Ready to Merge
labels
Nov 12, 2017
|
Then I'll leave this for after the auth change.
…
|
|
If you could change to RSpec now rather than waiting to make the config change, that would actually be really helpful -- it would help us make sure we don't forget to implement account-locking when making the switch. |
|
Ok then
…
|
| +require 'spec_helper' | ||
| + | ||
| +describe UserSessionsController do | ||
| + |
| + | ||
| +describe UserSessionsController do | ||
| + | ||
| + let(:user) { create(:user, activated_at: Time.new(2007,1,1)) } |
| @@ -158,6 +158,15 @@ | ||
| click_button "Create" | ||
| end | ||
| +When /^I login username "([^"]*)" and password "([^"]*)" for ([0-9]*) times$/ do |login, password, n| |
redsummernight
Nov 15, 2017
Contributor
Can you undo changes to this file since the cucumber step is no longer needed?
| @@ -0,0 +1,28 @@ | ||
| +# frozen_string_literal: true | ||
| + | ||
| +require 'spec_helper' |
| + context "after failed to login more than consecutive login limit" do | ||
| + before do | ||
| + allow(UserSession).to receive(:consecutive_failed_logins_limit).and_return(2) | ||
| + 3.times do |
redsummernight
Nov 15, 2017
Contributor
Since you mocked consecutive_failed_logins_limit as 2, 2.times here is enough for the next post to fail. Can you make "2" a local variable?
| + before do | ||
| + allow(UserSession).to receive(:consecutive_failed_logins_limit).and_return(2) | ||
| + 3.times do | ||
| + post :create, params: { user_session: { login: user.login, password: "aaa", remember_me: true } } |
redsummernight
Nov 15, 2017
Contributor
You can leave out remember_me: true from all the posts, it's not needed for the test.
| + end | ||
| + end | ||
| + | ||
| + context "when trying to login" do |
redsummernight
Nov 15, 2017
Contributor
You don't need this nested context since there's only one test, I'd move the last post into the it block.
| + end | ||
| + | ||
| + it "should not succeed because user is locked" do | ||
| + expect(flash.now[:error]).to match("Your account has been locked") |
redsummernight
Nov 15, 2017
Contributor
You should use flash[:error] to be consistent with the other tests.
| + before do | ||
| + max_consecutive_attempts = 2 | ||
| + allow(UserSession).to receive(:consecutive_failed_logins_limit).and_return(max_consecutive_attempts) | ||
| + (max_consecutive_attempts + 1).times do |
redsummernight
Nov 16, 2017
Contributor
You don't need to add 1, this test works if you use max_consecutive_attempts.times.
| + before do | ||
| + max_consecutive_attempts = 2 | ||
| + allow(UserSession).to receive(:consecutive_failed_logins_limit).and_return(max_consecutive_attempts) | ||
| + (max_consecutive_attempts).times do |
|
Looks good to me! |
hatal175 commentedNov 1, 2017
Issue
https://otwarchive.atlassian.net/browse/AO3-3848
Purpose
As I could not reproduce the issue, I added a feature test for it.
Testing
It is a feature test.
Credit
Tal Hayon
Please use he.