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

Lock account #5564 #5643

Merged
merged 1 commit into from Feb 19, 2015

Conversation

Projects
None yet
5 participants
@aka001
Copy link
Contributor

aka001 commented Feb 9, 2015

No description provided.

def lock_account
u = User.find(close_account_params)
u.lock_account!
redirect_to user_search_path, notice: t('admins.user_search.account_locking_scheduled', name: u.username)

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Line is too long. [109/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

u = User.find(close_account_params)
u.lock_account!
redirect_to user_search_path, notice: t('admins.user_search.account_locking_scheduled', name: u.username)
end

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Trailing whitespace detected.

@@ -456,6 +456,11 @@ def close_account!
AccountDeletion.create(:person => self.person)
end

def lock_account!
self.locked_at=Time.now
self.save

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Redundant self detected.

@@ -456,6 +456,11 @@ def close_account!
AccountDeletion.create(:person => self.person)
end

def lock_account!
self.locked_at=Time.now

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Surrounding space missing for operator '='.

@aka001 aka001 referenced this pull request Feb 9, 2015

Closed

Lock account #5564

@@ -7,6 +7,12 @@ def close_account
redirect_to user_search_path, notice: t('admins.user_search.account_closing_scheduled', name: u.username)

This comment has been minimized.

@aka001

aka001 Feb 9, 2015

Contributor

@houndci Shall I change the single-quotes here into double-quotes ?

@@ -4,7 +4,13 @@ class Admin::UsersController < Admin::AdminController
def close_account
u = User.find(close_account_params)
u.close_account!
redirect_to user_search_path, notice: t('admins.user_search.account_closing_scheduled', name: u.username)
redirect_to user_search_path, notice: t("admins.user_search.account_closing_scheduled", name: u.username)

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Line is too long. [109/80]

def lock_account
u = User.find(close_account_params)
u.lock_account!
redirect_to user_search_path, notice: t("admins.user_search.account_locking_scheduled", name: u.username)

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Line is too long. [109/80]

@@ -456,6 +456,10 @@ def close_account!
AccountDeletion.create(:person => self.person)
end

def lock_account!
self.save(:locked_at => Time.now)

This comment has been minimized.

@houndci-bot

houndci-bot Feb 9, 2015

Use the new Ruby 1.9 hash syntax.
Redundant self detected.

@svbergerem

This comment has been minimized.

Copy link
Member

svbergerem commented Feb 10, 2015

@aka001 We use houndci to check our javascript code. Apparently it also checks the ruby code. We were trying to disable checks for ruby but weren't successful so far. As long as those comments don't inform you about obvious errors you can just ignore them. We aren't enforcing a ruby code style right now.

@@ -456,6 +456,10 @@ def close_account!
AccountDeletion.create(:person => self.person)
end

def lock_account!
self = { :locked_at => Time.now }

This comment has been minimized.

@houndci-bot

houndci-bot Feb 10, 2015

cannot assign to a keyword

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from b3a696e to 4afd3e1 Feb 10, 2015

@aka001

This comment has been minimized.

Copy link
Contributor

aka001 commented Feb 10, 2015

@svbergerem Okay. I have updated the pr and squashed the commits. Thanks.

@@ -24,6 +24,9 @@
- unless user.person.closed_account
%li= link_to t('admins.user_search.close_account'), admin_close_account_path(user), method: :post, data: { confirm: t('admins.user_search.are_you_sure') }, class: 'btn btn-danger btn-mini'

- unless user.locked_at
%li= link_to t('admins.user_search.lock_account'), admin_close_account_path(user), method: :post, data: { confirm: t('admins.user_search.are_you_sure_lock_account') }, class: 'btn btn-danger btn-mini'

This comment has been minimized.

@aka001

aka001 Feb 10, 2015

Contributor

I didn't get the use of "admin_close_account_path" which messed up the "lock account" option.
I used the command in main repository to figured out the use of the same:

grep -r "admin_close_account_path" .

but didn't get any result which details the same.
Please help me in figuring it out.

This comment has been minimized.

@jhass

jhass Feb 10, 2015

Member

You might want to give http://guides.rubyonrails.org/routing.html a read then.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@jhass Thank you for the link. It helped a lot. I have taken the liberty to add the unlock feature.
When admin closes an user account it's "locked_at" field doesn't change, which results in "lock account"/"unlock account" option to be visible in admin's page.
Should we change the design to: "lock account/ unlock account" shouldn't be visible if the user account is locked?
I have attached the screenshot which reflects the same. The users "aka" and "aka_002" 's accounts are closed still the "Unlock Account" option is visible.
screenshot from 2015-02-18 17 17 04

This comment has been minimized.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from f307ee0 to ea83d91 Feb 18, 2015

def unlock_account
u = User.find(close_account_params)
u.unlock_account!
redirect_to user_search_path, notice: t("admins.user_search.account_unlocking_scheduled", name: u.username)

This comment has been minimized.

@houndci-bot

houndci-bot Feb 18, 2015

Line is too long. [111/80]


def unlock_account!
self.locked_at = nil
self.save

This comment has been minimized.

@houndci-bot

houndci-bot Feb 18, 2015

Redundant self detected.

@@ -139,6 +139,8 @@

namespace :admin do
post 'users/:id/close_account' => 'users#close_account', :as => 'close_account'
post 'users/:id/lock_account' => 'users#lock_account', :as => 'lock_account'
post 'users/:id/unlock_account' => 'users#unlock_account', :as => 'unlock_account'

This comment has been minimized.

@houndci-bot

houndci-bot Feb 18, 2015

Line is too long. [86/80]
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -139,6 +139,8 @@

namespace :admin do
post 'users/:id/close_account' => 'users#close_account', :as => 'close_account'
post 'users/:id/lock_account' => 'users#lock_account', :as => 'lock_account'

This comment has been minimized.

@houndci-bot

houndci-bot Feb 18, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 18, 2015

@aka001 Looks like the rebase went a bit wrong. Let's try this:

git remote add upstream git:://github.com/diaspora/diaspora.git
git fetch upstream
git checkout 5564_lock_account
git rebase -i upstream/develop
# Drop the line with my commit
# Choose squash for the now second line
# Save & quit
# You'll be in some conflicts, git status shows them, once you cleaned them out use git add to mark them resolved, once all are resolved, use git rebase --continue, that might be needed more than one time
git push -f origin 5564_lock_account

# Later, you can edit the last commit with
git commit --amend
git push -f origin 5564_lock_account

Please also add at least one controller test per new route.

@@ -24,6 +24,11 @@
- unless user.person.closed_account
%li= link_to t('admins.user_search.close_account'), admin_close_account_path(user), method: :post, data: { confirm: t('admins.user_search.are_you_sure') }, class: 'btn btn-danger btn-mini'

- unless user.locked_at
%li= link_to t('admins.user_search.lock_account'), admin_lock_account_path(user), method: :post, data: { confirm: t('admins.user_search.are_you_sure_lock_account') }, class: 'btn btn-danger btn-mini'
- if user.locked_at

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

I think using else here would be clearer

def lock_account!
self.locked_at = Time.now
self.save
end

This comment has been minimized.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@jhass Devise is a separate module. Don't we need to write the code in core repository? Kindly clarify it.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Devise is an authentication framework, a library that handles user authentication. And we use it, you can see that in this very file, notice the devise at the top.

This comment has been minimized.

@jhass

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

okay. Thank you for the details. I will make the required changes and update the pr.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@jhass
The code is:

unless user.access_locked?
#Lock_account code
else
#Unlock_account code

So when user account is closed then:
user.access_locked? will return true, hence else part will get executed and "Unlock Account" button will appear. I will have to write the code for user.access_unlocked? in "user.rb".
Please correct me if I am wrong.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

See this line: https://github.com/diaspora/diaspora/blob/develop/app/views/admins/_user_entry.haml#L43

I'd say make that a bit more pretty by adding closed_account? to the User model, you can just delegate that call to the associated Person model, as you can see in the current usage, then replace the call I linked with the it and wrap the entire locking code into an unless user.closed_account?

I don't see why you would need an access_unlocked? method.

def unlock_account!
self.locked_at = nil
self.save
end
def unlock_account!
self.locked_at = nil
self.failed_attempts = 0 if respond_to?(:failed_attempts=)
self.unlock_token = nil if respond_to?(:unlock_token=)

This comment has been minimized.

@houndci-bot

houndci-bot Feb 18, 2015

Unnecessary spacing detected.

@aka001

This comment has been minimized.

Copy link
Contributor

aka001 commented Feb 18, 2015

Updated

number: "head" # Do not touch unless doing a release, do not backport the version number that's in master but keep develop to always say "head"
>>>>>>> develop

This comment has been minimized.

@svbergerem

svbergerem Feb 18, 2015

Member

here is an unresolved conflict

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@svbergerem Resolved. Thanks.

u.close_account!
redirect_to user_search_path, notice: t('admins.user_search.account_closing_scheduled', name: u.username)
end
def close_account

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Please properly indent the code, it's a method in a class in a module, so four spaces of indentation here.

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 18, 2015

There's still a bogus commit in here and this is really fine in one commit, no need for 12. See #5643 (comment)

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from e1eab1b to 1d4c0d1 Feb 18, 2015

@aka001

This comment has been minimized.

Copy link
Contributor

aka001 commented Feb 18, 2015

I have squashed the commits.

@@ -19,4 +19,24 @@
end
end

describe '#lock_account' do
it 'queues a job to lock the given account' do

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Uh, I should have payed closer attention to this earlier, sorry. This doesn't reflect what the method does and neither what the spec tests for. It does not queue a job, it executes that action directly (which is fine, since it's a single UPDATE query). So please update the wording here, something like "it locks the given account" should be fine.

Remember that you can edit your commit with git commit --amend.

end

def lock_account
u = User.find(close_account_params)

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Another detail I missed, let's actually refactor this and get rid of the rather unnecessary close_account_params here, passing params[:id] to .find directly is just fine. Please also update close_account for this and drop the method below.


post :lock_account, id: other_user.id
end
end

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Did you actually run your tests? They're failing: https://travis-ci.org/diaspora/diaspora/jobs/51284331#L440

See https://wiki.diasporafoundation.org/Testing_workflow

I'd say don't mock the call out, put an expectation like expect(other_user.reload.access_locked?).to be_truthy after the post.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

I understood the use of the same.
I tried doing it the way it's done in "close_account" and it works fine.
https://github.com/diaspora/diaspora/pull/5643/files#diff-3212b7cc0ec103914d1d59733b84f107R38
Should I use expect method instead?

@aka001 aka001 force-pushed the aka001:5564_lock_account branch 2 times, most recently from 96e5527 to 0faaa4a Feb 18, 2015

expect(other_user).to receive(:lock_access!)
allow(User).to receive(:find).and_return(other_user)

post other_user.expect(other_user.reload.access_locked?).to be_truthy

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@jhass
This line:

other_user.expect(other_user.reload.access_locked?).to be_truthy

returns the error "undefined method `expect' "
Is this the expected behavior? Kindly explain.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Yes it is. expect is not monkey patched onto all objects, it's only available inside the it block. Just drop the other_user. before it, I'm not following what that should express anyway ;)

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from 19daaf5 to 87fd9dc Feb 18, 2015

expect(other_user).to receive(:lock_access!)
allow(User).to receive(:find).and_return(other_user)

post :lock_account, id: other_user.id

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

It's not about replacing this call.

it 'it locks the given account' do
other_user = FactoryGirl.create :user
expect(other_user).to receive(:lock_access!)
allow(User).to receive(:find).and_return(other_user)

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

It's about replacing those two calls.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

I guess I'll just spoil it. What I mean is something like:

user = FactoryGirl.create :user

post :lock_account, id: user.id

expect(user.reload.access_locked?).to be_truthy

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@jhass I didn't get the use of:
post :lock_account, id: user.id initially so I did:
https://github.com/diaspora/diaspora/pull/5643/files#diff-3212b7cc0ec103914d1d59733b84f107R25
(made it a bit complicated)
I got the whole concept now. I will update the pr.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch 2 times, most recently from 2fd4d40 to 2e18818 Feb 18, 2015

@@ -19,4 +19,20 @@
end
end

describe '#lock_account' do
it 'it locks the given account' do
other_user = FactoryGirl.create :user

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

@jhass Shall I change the "other_user" to "user" ?

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

I used user over other_user in my example because I found the other_ rather redundant, I didn't get what it would relate to. But I don't care too much either way.

it 'it unlocks the given account' do
other_user = FactoryGirl.create :user
post :unlock_account, id: other_user.id
expect(not(other_user.reload.access_locked?)).to be_truthy

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

As said before use !, not not for negation. But here again there's a better way than negation in the first place and that is to use the be_falsey matcher.

Besides this spec is not testing much. You start with a user account that's not locked, run some code and then check whether the user account is not locked. So while you ensure that the code you run does not lock the account, you do not ensure that it unlocks the account.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

When I use:
expect(other_user.reload.access_locked?).to be_falsify instead of
expect(!(other_user.reload.access_locked?)).to be_truthy
I get the error:
http://pastebin.com/aSv3nMv9
If I use the later "expect(!(other_user.reload.access_locked?)).to be_truthy" I don't get any error.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

be_falsey not be_falsify

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

I have updated the pr. I guess it's correct now.
In which segment of code can I see the implementation of expect method and all it's flags?
I would like to know more about it.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

The main part is http://www.rubydoc.info/gems/rspec-expectations/RSpec/Matchers but things like rspec-rails add more matchers.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from 31f9bf3 to 53126fb Feb 18, 2015

post :lock_account, id: other_user.id
expect(other_user.reload.access_locked?).to be_truthy
post :unlock_account, id: other_user.id
expect(!(other_user.reload.access_locked?)).to be_truthy

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

I have first locked the account -> tested it -> unlocked it -> tested it. I think this will cover the test case.

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

The

post :lock_account, id: other_user.id
expect(other_user.reload.access_locked?).to be_truthy

is kind of redundant with the other test, I think a simple other_user.lock_access! is enough.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

I have updated the pr.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch 2 times, most recently from 72f21bf to a8267ba Feb 18, 2015

it 'it unlocks the given account' do
other_user = FactoryGirl.create :user
other_user.lock_access!
expect(other_user.reload.access_locked?).to be_truthy

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Just drop this line, that expectation is not part of the scope of this test.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

True that. I have updated the pr.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from f306723 to 2c47ecd Feb 18, 2015

def unlock_account
u = User.find(params[:id])
u.unlock_access!
redirect_to user_search_path, notice: t("admins.user_search.account_unlocking_scheduled", name: u.username)
end

private

This comment has been minimized.

@jhass

jhass Feb 18, 2015

Member

Please drop the method below, we made it obsolete earlier.

This comment has been minimized.

@aka001

aka001 Feb 18, 2015

Contributor

Done.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch from 617188f to e968dd6 Feb 18, 2015

end

private

This comment has been minimized.

@jhass

jhass Feb 19, 2015

Member

No need for the stray private either.

This comment has been minimized.

@aka001

aka001 Feb 19, 2015

Contributor

Updated.

@aka001 aka001 force-pushed the aka001:5564_lock_account branch to 3bb5e78 Feb 19, 2015

@jhass jhass added this to the next-major milestone Feb 19, 2015

@jhass

This comment has been minimized.

Copy link
Member

jhass commented Feb 19, 2015

Thank you!

@jhass jhass merged commit 3bb5e78 into diaspora:develop Feb 19, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound Hound has reviewed the changes.

jhass added a commit that referenced this pull request Feb 19, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Feb 19, 2015

Nice work @aka001 thank you!

@aka001

This comment has been minimized.

Copy link
Contributor

aka001 commented Feb 19, 2015

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment