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

Add default of admin to requester in make retire request #18109

Closed
wants to merge 1 commit into from

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Oct 18, 2018

Since the merge of #17951, which adds a system_context for retirement, make_retire_request takes a user.

Brandon thinks make_retire_request should have a default user, and Tina thinks if we must have one, it should be admin. So. Boooya.

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=1618530

related:
ManageIQ/manageiq-api#497

@d-m-u
Copy link
Contributor Author

d-m-u commented Oct 18, 2018

@miq-bot add_reviewer @bdunne
@miq-bot add_reviewer @tinaafitz
@miq_bot add_label bug, blocker, gaprindashvili/yes, hammer/yes

@miq-bot miq-bot requested review from bdunne and tinaafitz October 18, 2018 16:04
@miq-bot
Copy link
Member

miq-bot commented Oct 18, 2018

Checked commit d-m-u@244a248 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 4 offenses detected

app/models/mixins/retirement_mixin.rb

  • 💣 💥 🔥 🚒 - Line 12, Col 3 - Lint/Syntax - module definition in method body
    (Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)
  • 💣 💥 🔥 🚒 - Line 13, Col 49 - Lint/Syntax - unexpected token tEQL
    (Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)
  • 💣 💥 🔥 🚒 - Line 13, Col 83 - Lint/Syntax - unexpected token tRPAREN
    (Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)
  • 💣 💥 🔥 🚒 - Line 1, Col 1 - Lint/Syntax - module definition in method body
    (Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)

@@ -10,7 +10,7 @@ module RetirementMixin
end

module ClassMethods
def make_retire_request(*src_ids, requester)
def make_retire_request(*src_ids, requester = User.find_by(:userid => 'admin'))
Copy link
Member

Choose a reason for hiding this comment

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

Why 'admin' here and User.current_user.userid everywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we shouldn't be doing retirement based on current_user, it should be from the evm_owner, and if not one of those, admin. Otherwise the system retirement can run with different users on a retry.

Copy link
Member

@kbrock kbrock Oct 18, 2018

Choose a reason for hiding this comment

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

agreed

    def make_retire_request(*src_ids, requester = User.current_user)

If you want the userid - please use User.current_userid

Copy link
Contributor Author

@d-m-u d-m-u Oct 18, 2018

Choose a reason for hiding this comment

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

I don't want current_user here at all. The place we use it is from the UI where it makes sense that we want the user who clicked on retirement. It should not be in make_retire_request. We're not using "User.current_user.userid everywhere else", Brandon, that was the point of the changes yesterday.

@d-m-u d-m-u closed this Oct 18, 2018
@d-m-u d-m-u deleted the adding_default_user_to_mrr branch February 1, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants