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

Show VM transform button only if permitted #2959

Merged

Conversation

smelamud
Copy link
Contributor

@smelamud smelamud commented Dec 5, 2017

Show VM transform button for mass transforms only if vm_transform_mass permission is set in the permission store. By default, it is visible in ManageIQ, that uses Null permission store, and hidden in CFME, if vm_transform_mass is not set explicitly in YAML permission store.

@smelamud
Copy link
Contributor Author

smelamud commented Dec 5, 2017

@miq-bot assign martinpovolny

@smelamud
Copy link
Contributor Author

smelamud commented Dec 5, 2017

@miq-bot add_labels gaprindashvili/yes

@martinpovolny
Copy link
Member

@smelamud: looks good but there's "but" ;-)

Can you, please, extract the common behavior from the 2 button classes? (a common parent class or a module?)

Why do the 2 implementations of destination_exists? differ? That does not look right.

Thank you!

@smelamud
Copy link
Contributor Author

smelamud commented Dec 5, 2017

@martinpovolny Because TransformVmButton was written by Martin Betak and MassTransformVmButton was written by me and I changed the implementation according to reviewers' comments.

@smelamud
Copy link
Contributor Author

smelamud commented Dec 6, 2017

@martinpovolny I can change the implementation in TransformVmButton or even create a common ancestor for both buttons, but this is out of scope of this PR. I can create a separate PR for this.

@oourfali
Copy link

@martinpovolny can we merge this?
Other changes can be done in subsequent patches.

Show VM transform button for mass transforms only if vm_transform_mass
permission is set in the permission store. By default, it is visible in
ManageIQ, that uses Null permission store, and hidden in CFME, if
vm_transform_mass is not set explicitly in YAML permission store.
@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2017

Checked commit smelamud@3f07554 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. 🍰

@smelamud
Copy link
Contributor Author

@jelkosz Enabled the button for single VM transforms, as we discussed.

@jelkosz
Copy link
Contributor

jelkosz commented Dec 12, 2017

@smelamud great, thank you!

@martinpovolny
Copy link
Member

@jelkosz Enabled the button for single VM transforms, as we discussed.

Now I am lost. Why did you do that? Is that discussion somewhere I can read?

@smelamud
Copy link
Contributor Author

@martinpovolny It was just my misunderstanding. I thought I should hide transform VM button everywhere, but Tomas said I need to hide only mass transforms button, but single VM transforms should be present.

@martinpovolny martinpovolny merged commit aa625cd into ManageIQ:master Dec 14, 2017
@martinpovolny martinpovolny added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 14, 2017
simaishi pushed a commit that referenced this pull request Dec 14, 2017
Show VM transform button only if permitted
(cherry picked from commit aa625cd)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 865e73b1efb68c7a212a754c364d058fcc666a4c
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Thu Dec 14 14:14:38 2017 +0100

    Merge pull request #2959 from smelamud/disable-vm-transform-in-cfme
    
    Show VM transform button only if permitted
    (cherry picked from commit aa625cd92253c41ab87e649f47c775f98c7eba08)

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.

6 participants