Change request_controller special privileges #447
Conversation
| @@ -44,12 +44,12 @@ def deny_resource(type, id, data) | |||
|
|
|||
| def find_automation_requests(id) | |||
| klass = collection_class(:requests) | |||
| return klass.find(id) if User.current_user.miq_user_role.request_admin_user? | |||
| return klass.find(id) if User.current_user.role_allows?(:identifier => "miq_request_approval") | |||
There was a problem hiding this comment.
Perhaps this can be put in a method like you did for the other ones. Something like MiqUserRole#request_approver_user? This way if we want to switch this logic to looks at a different feature we wouldn't have to change all these lines.
There was a problem hiding this comment.
I agree. The fact that we had the wrong feature identifier in the request_admin_user? method indicates we will probably make this mistake again so it's probably best to limit the places we hardcode the feature identifiers.
There was a problem hiding this comment.
Hmm. Not sure that was the problem in this case.
We have so many features. Not sure if we want to introduce a *_user? for every one of them.
Also, in the UI and API tend to use identifiers in most cases.
Not sure if this case in particular is worthy of a special method.
To be honest, I was looking forward to getting rid of all of them except for the super user one
There was a problem hiding this comment.
I think request admin and report admin are extremely large parts of the UI warranting methods for these types of users. Regardless, I'd prefer to have too many methods on user role than have so many hardcoded feature identifiers.
bf3b802 moved us away from user.is_admin? This is based upon the miq_request_superadmin feature. The UI used a different feature, miq_request_approval. This changes the feature identifier to make the UI and API the same. miq_request_superadmin will be deprecated https://bugzilla.redhat.com/show_bug.cgi?id=1608554
|
@gtanzillo darn - locally these specs are green for me. |
|
Checked commit kbrock@0a7a1e4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
Merges as part of #454 |
BLOCKED BY:
To make green, also look into:
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1608554
In #385 we introduced feature
miq_request_superadminno more hardcoded admin role name - YAY
The UI uses a different feature,
miq_request_approvalInconsistencies - BOO
ManageIQ/manageiq#17849 fixes main repo's identifier
Consistencies - YAY
API Tests Fail -- BOO
This PR changes feature identifiers used - YAY
For those concerned:
miq_request_superadminis deprecated