-
Notifications
You must be signed in to change notification settings - Fork 141
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
Editing a user doesn't have constraints for the admin user #974
Comments
Can you link to specific code...I agree it should move to the model, actually, but it depends on the specifics |
I'd be ok with this in the model. I don't think it will affect specs, but if it does we could, worst case, always wrap the validation in an |
For this case, putting in the model probably make sense. But we're hitting more and more use cases where we have business logic in the controllers and no good place to duplicate this logic in the api controllers. Not duplicating it would be even better. I'm ok making a slew of changes across all tests to avoid the one off manner of It would be nice if we could keep in mind that we need a more permanent recipe for where we want to put this controller logic in a more sustainable place. |
These are more like constrains that should be in the model, at least that's where I'd put them in my own hypothetical Rails app 🤔 |
@skateman I agree. Generally, constraints that apply to all users should be in the model (or even the database)...things like specific value ranges, enums, certain validations. Some things we can't put in the model, particular if they only apply to a subset of users, or if they prevent standard backend usage like deleting records. In this particular case, while this is a user-specific change, that user in question is well-defined, and I think the model is an appropriate place. |
This issue has been automatically marked as stale because it has not been updated for at least 3 months. If you can still reproduce this issue on the current release or on Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation. |
In the UI the user editing form (and controller) doesn't allow the user to change the name or the assigned groups of the
admin
user. However, this is not the case in the API and it is possible to rename theadmin
user or assign it to certain groups which I think might break RBAC stuff.Ideally these constraints should be in the DB model, but we don't really do that because that might hurt db seeding so I guess we should mimic the behavior of the UI in the API.
@miq-bot assign @gtanzillo
@miq-bot add_label bug
cc @Fryguy
The text was updated successfully, but these errors were encountered: