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

Fix logic for service ownership #2725

Merged
merged 1 commit into from
Nov 27, 2017

Conversation

mzazrivec
Copy link
Contributor

The broken logic would return nil in case the class in question (Service, Vm, Template ...)
doesn't support (respond to) with_ownership class method -> this would seem
like the class in question doesn't support ownership change.

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

@@ -213,7 +213,11 @@ def ownership_handle_reset_button

def filter_ownership_items(klass, ownership_ids)
records = find_records_with_rbac(klass.order(:name), ownership_ids)
records.with_ownership if klass.respond_to?(:with_ownership)
if klass.respond_to?(:with_ownership)
records.with_ownership if klass.respond_to?(:with_ownership)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec I guess it there is condition accidentally duplicated.

maybe together can be

records.try(:with_ownership) || records

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpichler fixed, thanks.

The broken logic would return nil in case the class in question
(Service, Vm, Template ...) doesn't support (respond to) with_ownership
class method -> this would seem like the class in question doesn't
support ownership change.

https://bugzilla.redhat.com/show_bug.cgi?id=1512644
@mzazrivec mzazrivec force-pushed the fix_service_ownership branch from 799411e to 814a12c Compare November 15, 2017 12:53
@lpichler
Copy link
Contributor

@miq-bot assign @martinpovolny

@miq-bot
Copy link
Member

miq-bot commented Nov 15, 2017

Checked commit mzazrivec@814a12c with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@martinpovolny
Copy link
Member

What if with_ownership exists and returns nil?

In that case records would be returned.

Is that ok?

@mzazrivec
Copy link
Contributor Author

Good point, but that's probably more a backend question. @lpichler ?

@lpichler
Copy link
Contributor

@martinpovolny martinpovolny merged commit ef4d71e into ManageIQ:master Nov 27, 2017
@martinpovolny martinpovolny added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 27, 2017
simaishi pushed a commit that referenced this pull request Nov 27, 2017
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 1d0945fe26d5a22f712c5818a7a0bafc24721aaf
Author: Martin Povolny <[email protected]>
Date:   Mon Nov 27 14:40:21 2017 +0100

    Merge pull request #2725 from mzazrivec/fix_service_ownership
    
    Fix logic for service ownership
    (cherry picked from commit ef4d71e74f9cefe13400cd3354f2ff88a613d8a2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1517950

@mzazrivec mzazrivec deleted the fix_service_ownership branch February 22, 2018 12:12
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.

5 participants