Ap 602 refactor roles#46
Conversation
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
Looks pretty good to me. There are a few inline comments to resolve before merging, and I'd like to review again after those are done. (I'm a bit tired today and hope to review again tomorrow with fresh eyes after these changes are made.)
| authenticate! | ||
| @user_is_admin = current_user.role?(Role.stackpass_admin) | ||
| @user_is_admin = current_user.any_role?(Role.stackpass_admin, :framework_admin) | ||
| raise Error::ForbiddenError unless @user_is_admin |
There was a problem hiding this comment.
Is there a reason we don't use authenticate_with_role! Role.stackpass_admin, :framework_admin here? Seems well-suited for the task.
There was a problem hiding this comment.
Changing this one, easy change!
| it 'returns false when the user does not have the role' do | ||
| user = User.new(uid: '12345') | ||
|
|
||
| allow(FrameworkUsers).to receive(:hardcoded_admin?).with('12345').and_return(false) |
There was a problem hiding this comment.
It doesn't look like hardcoded_admin? exists any more - should this still be in the test?
There was a problem hiding this comment.
These other two opened up a bit of a can-o-worms. Updating these and a couple of other specs...wrestling with them now.
| it 'returns false when the user has none of the requested roles' do | ||
| user = User.new(uid: '12345') | ||
|
|
||
| allow(FrameworkUsers).to receive(:hardcoded_admin?).with('12345').and_return(false) |
2 participants
No description provided.