Repository navigation
Conversation
… DB group
When a user switches their active group between request creation and provision
time, vm.miq_group should reflect the group active at request time -- not
whatever group is current in the database when the task runs.
The broken path (new failing tests):
- owner_email is set in options so get_owner returns the user
- owner_group and requester_group are both absent from options
(e.g. a legacy or API request that bypassed set_request_values)
- the user switched current_group in the DB after submitting the request
get_owner finds the user via email but skips current_group_by_description
because owner_group is blank. get_user is never called because get_owner
returned non-nil. set_ownership then uses user.current_group -- the live DB
value -- and stamps the wrong group on the VM.
Scenario matrix (owner = provisioning user, R = request-time, P = provision-time):
owner_email owner_group requester_group R group P group (DB) Expected Result
present present any U U U pass
present present any U S U pass (get_owner applies owner_group)
present absent absent U S U FAIL (new test; DB group S stamped)
present absent present U S U pass (get_user cache applies requester_group)
absent absent present U S U pass (get_user applies requester_group)
Two new examples in the 'sets ownership' context reproduce the FAIL row and
are expected to fail until the fix is applied.
When owner_email is set in options but owner_group is absent, get_owner finds the user via email but never calls current_group_by_description. set_ownership then uses user.current_group -- the live DB value -- which will be wrong if the user switched active groups between request creation and provision time. set_request_values always stamps requester_group on new requests, so requester_group is the authoritative fallback for the request-time group when owner_group is not available. Fall back to requester_group in get_owner when owner_group is blank: group = get_option(:owner_group).presence || get_option(:requester_group) owner.current_group_by_description = group if owner && group This makes the group resolution explicit in get_owner rather than relying on the side-effect of passing get_user as the email-lookup cache.
Member
|
Checked commits jrafanie/manageiq@8f8ffd5~...789e196 with ruby 3.3.12, rubocop 1.91.0, haml-lint 0.78.0, and yamllint 1.37.1 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the following bug:
When you provision a VM, the VM has its group ownership set to the group you were in when you submitted the request. But if you switch your active group between submitting and approving — which is common when an admin approves their own requests — the VM ends up with the wrong group.
For example: User X submits a provision request while in Group A, then switches to Group B to approve it. The VM should be owned by Group A, but instead gets owned by Group B.