[GAPRINDASHVILI] Add requester info to raise_retirement_event #18106
Conversation
|
@miq-bot add_label bug, blocker |
35bd150 to
e6fd1f2
Compare
|
@miq-bot assign @gmcculloug |
|
but MOOOOARRRR EMAILS |
|
This is looking good - the test failures make sense. Are you still working on this? |
|
Still working on this. |
2164d1a to
13f228a
Compare
|
@miq-bot assign @gmcculloug sorry about this, people, i swear we're almost done retiring. |
13f228a to
a569caf
Compare
|
@miq-bot remove_label wip |
eed6785 to
f60eaf4
Compare
|
@tinaafitz could you please take a look at this? i have fruit snacks... |
|
@kbrock, any chance I could get you to re-review please? |
|
@bdunne sorry but could I please get you to take another look at this, as well? |
| end | ||
|
|
||
| def q_user_info(q_options, requester) | ||
| if requester.present? |
There was a problem hiding this comment.
Do you need the present? check? It should be nil or a User object, right?
|
|
||
| def q_user_info(q_options, requester) | ||
| if requester.present? | ||
| if requester.kind_of?(String) |
There was a problem hiding this comment.
Does this ever get called with a string?
There was a problem hiding this comment.
88cb976 to
9aa105e
Compare
There was a problem hiding this comment.
I found one change (the "admin" one) and 2 nits.
Since this is a backport, do we really want to be making any minor changes to this? It will just cause issues when we are backporting other code in the future.
I feel as if we should make those changes to master and only tweak the backport if there are actual bugs. So maybe ignore all my suggestions?
| event_hash[retirement_base_model_name.underscore.to_sym] = self | ||
| event_hash[:host] = host if self.respond_to?(:host) | ||
| def system_context_requester | ||
| if try(:evm_owner_id) |
There was a problem hiding this comment.
| if try(:evm_owner_id) | |
| if evm_owner_id |
| requester = User.find_by(:userid => requester) | ||
| end | ||
| q_options[:user_id] = requester.id | ||
| q_options[:group_id] = requester.current_group.id if requester.current_group |
There was a problem hiding this comment.
Heh - this makes me reference the code every time I see it - It is not necessary since the very next line is looking up the current_group and all, but I feel like this is better form. (sorry, think you probably introduced this from my suggestion before)
| q_options[:group_id] = requester.current_group.id if requester.current_group | |
| q_options[:group_id] = requester.current_group_id if requester.current_group_id |
9aa105e to
051cb5f
Compare
|
"I feel as if we should make those changes to master and only tweak the backport if there are actual bugs." I feel this way too, so anyone who's tagged on this who'd like to approve it, I'd be pretty grateful, cause I'd like to be able to close this bug sometime this century. |
|
@mkanoor Please review. |
|
Checked commit https://github.com/d-m-u/manageiq/commit/051cb5f19eab9e1659a664dbcbbb705f053fae6c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
It's the g-branch start to the horror that got merged earlier today for https://bugzilla.redhat.com/show_bug.cgi?id=1618530
It's an attempt to 1) delete the system retirer that was previously in place, 2) add the requester info to the raise_retirement_event, and 3) remove User.current_user references from all places in retirement except through the UI, as well as 4) fixing the listed bug.