Feature/user ban flag #84
Conversation
Fix an error
…ure/user-ban-flag
monorkin
left a comment
There was a problem hiding this comment.
@daitangio thank you for the PR!
I left a few comments, mostly minor things and stylistic stuff.
| branches: [ main ] | ||
| branches: [ master, feature/* ] | ||
| pull_request: | ||
| branches: [ main ] |
There was a problem hiding this comment.
Could you remove this from this PR?
I'd like to keep it focused on the ban functionality
There was a problem hiding this comment.
Done. I have also removed the other files and restored the original workflows
There was a problem hiding this comment.
We probably want to keep this one :)
.vscode/extensions.json
Outdated
There was a problem hiding this comment.
This shouldn't be part of the PR
AGENTS.md
Outdated
There was a problem hiding this comment.
Neither should this be part of the PR
EXTRA_TIPS.md
Outdated
There was a problem hiding this comment.
Nor should this be in the PR
| if turbo_request? | ||
| render turbo_stream: turbo_stream.update("main-content", partial: "application/banned_notice"), status: :forbidden | ||
| elsif request.format.html? | ||
| render "application/banned", status: :forbidden | ||
| else | ||
| head :forbidden | ||
| end |
There was a problem hiding this comment.
Did you consider revoking the user's session when they get banned?
That way they'd be forced to re-login and you wouldn't have to handle all these cases.
There was a problem hiding this comment.
Hum my (humble) idea was to try to show something to the user and let him log in.
Also if the user is unbanned, (s)he get access back immediatly.
There was a problem hiding this comment.
From my perspective, you are banning someone because you don't want them on your instance. So you want to completely forbid them from using any resources on your instance. And session do use up some resources occasionally. You can still let the user know that they've been banned with a flash message in the Sessions controller.
| def banned_user_allowed_action? | ||
| controller_path == "sessions" || controller_path.start_with?("sessions/") | ||
| end |
There was a problem hiding this comment.
Maybe it would be nicer if this was an action like the session/auth actions are?
Then, at the top of the SessionsController, you could add something like allow_access_while_banned only: :destroy.
| { role: params.require(:user)[:role].presence_in(%w[ member administrator ]) || "member" } | ||
| { | ||
| role: params.require(:user)[:role].presence_in(%w[ member administrator ]) || "member", | ||
| banned: params.require(:user)[:banned] == "1" |
There was a problem hiding this comment.
You don't have to do the comparison by hand, if the column is of type boolean, ActiveRecord will do the type casting for you - e.g. 1, true, TRUE would all set the boolean to true.
I'm a bit unsure about the naming here, banning someone isn't really a role change?
There was a problem hiding this comment.
In my humble opinion banning is just an attribute of the user. The role did not change. So if in the future we will have more than 2 roles, you will be able to ban the user and keep its role unchanged, to simplify unbanning.
Also, if I simply the code into
banned: params.require(:user)[:banned]
One unit test fails wilth error
Error:
Accounts::UsersControllerTest#test_update:
ActiveRecord::NotNullViolation: SQLite3::ConstraintException: NOT NULL constraint failed: users.banned
app/controllers/accounts/users_controller.rb:9:in 'Accounts::UsersController#update'
test/controllers/accounts/users_controller_test.rb:11:in 'block in <class:UsersControllerTest>'
So if you can suggest me how to fix it...in the meantime I keep the equals
There was a problem hiding this comment.
There is a with_defaults method that you can use to set banned to fals if it's not passed.
In my humble opinion banning is just an attribute of the user. The role did not change.
I fully agree, that's why I'm bringing this up - the change to allow the banned flag was to the role_params method. And banning someone isn't a role change, so I believe it should be its own method.
| bot_token: <%= User.generate_bot_token %> | ||
| role: bot | ||
|
|
||
| bruce_banned: |
There was a problem hiding this comment.
I renamed the fixture user in bruce banned, so it should be clearer its aim
|
@monorkin I have fixed most of the review points. |
Contribution for feature Add the ability to ban people #71
It is a first implementation, banned user can login but it is unable to send chats or do whatever. It just say a message he is banned.
It is my first contribution on this project, so feel free to comment or ask for changes.