Skip to content
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

Use resourceful route for user status #5437

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

Creates :user_status resource with its own controller. You can :update this resource instead of doing :set_status on User.

"Soft destroy" :destroy action is also moved to this resource because it comes together with status update. It does a similar thing (changes status), has the same permissions (admin), is linked from the same place in the UI.

@AntonKhorev AntonKhorev force-pushed the user-status-resource branch from 6c89d41 to 250b032 Compare January 8, 2025 19:41
Copy link
Contributor

@nertc nertc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and code works properly. I like this change, this kind of separation of functionalities make code more readable and easier to work with.

@gravitystorm
Copy link
Collaborator

I think you can update the status, but you can't destroy the status. You can destroy the user.

So I think refactoring the set_status is good but I think the destroy method should stay in the same place.

@AntonKhorev
Copy link
Collaborator Author

You can destroy the user.

You can't, the user is deactivated and gets some of the properties wiped. It's a status change with extras.

If you don't like :destroy on status, what if we do @user.soft_destroy! inside update too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants