Target for mass VM transformation dialog #3073

smelamud · 2017-12-14T21:47:37Z

Created a separate vm_transform_mass action to handle VM Transform
button clicks in on Providers page and on Compute > Infrastructure >
Virtual Machines page. This action interprets the ID passed to it as
provider ID, while regular vm_transform action interprets it as VM ID.
If provider ID is not passed, the Redhat provider EMS object is used as
target.

Depends on ManageIQ/manageiq#16686

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1514939
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1516497

(See also @mkanoor comments here: ManageIQ/manageiq#16584 (review))

smelamud · 2017-12-14T21:49:22Z

@miq-bot assign himdel

smelamud · 2017-12-14T21:49:58Z

@miq-bot add_labels gaprindashvili/yes

oourfali · 2017-12-15T13:32:01Z

@himdel @martinpovolny we'll appreciate a fast review, as this is a blocker.
Thanks!

mkanoor · 2017-12-15T17:15:16Z

app/controllers/mixins/actions/vm_actions/transform.rb

@@ -17,7 +17,8 @@ def vm_transform
            @right_cell_text = _("Transform VMs to RHV")
            simple_dialog_initialize(
              dialog.resource_actions.first,
-              :header => @right_cell_text
+              :header => @right_cell_text,


@smelamud This doesn't look right.
First of this is a Provider Action not a VM action. When the UI calls the controller with no vms selected the URL looks like
http://localhost:3000/ems_infra/10000000000001?display=vms

We get the params as <ActionController::Parameters {"pressed"=>"vm_transform", "controller"=>"ems_infra", "action"=>"button", "id"=>"10000000000001"} permitted: false>

So without the context vm or provider we can't be calling vm_transform and expecting that the ID is the ID of a VM when in reality it is the ID of the provider.

I would think we should have 2 separate actions
vm_transform which works with a single/multiple vm's, the target would be the vm object

mass_transform which applies to all tagged VMs in the provider, the target object here is the provider which the controller is passing it in.

So the target needs to be set appropriately based on the context.

Currently this line

manageiq-ui-classic/app/controllers/mixins/actions/vm_actions/transform.rb

Line 7 in d4e9e12

if params.key?(:id)

is wrong without the context. It gives arbitrary vm object for which there is a different BZ open.

@mkanoor If mass_transform is a provider action, what module/class I need to put it into?

mkanoor · 2017-12-15T20:26:30Z

app/controllers/mixins/actions/vm_actions/transform.rb

@@ -17,7 +17,8 @@ def vm_transform
            @right_cell_text = _("Transform VMs to RHV")
            simple_dialog_initialize(
              dialog.resource_actions.first,
-              :header => @right_cell_text
+              :header => @right_cell_text,
+              :target => ExtManagementSystem.find_by(:type => 'ManageIQ::Providers::Redhat::InfraManager')


@smelamud
If you use the different URL's you should be able to ExtManagementSystem.find_by(:id => ....)

Since you get passed the id just like when you do a single VM.

@mkanoor What about Compute > Infrastructure > Virtual Machines? If the mass transform button is clicked there, what it receives? There is neither provider nor VM selected.

smelamud · 2017-12-19T13:51:09Z

@mkanoor Addressed all you comments, can you please to look on it again?

mkanoor · 2017-12-19T14:45:31Z

Looks good. @himdel @martinpovolny please review

martinpovolny · 2017-12-19T16:34:42Z

Looks good to me. But I cannot test it right now.

martinpovolny · 2017-12-19T16:35:48Z

@smelamud, @oourfali : can someone from your team confirm that it works correctly in the UI?

If not, I can try to test this tomorrow.

bzwei · 2017-12-19T22:12:01Z

app/controllers/mixins/actions/provider_actions/mass_transform.rb

+              :target_kls => Provider.name
+            )
+          else
+            provider = ExtManagementSystem.find_by(:type => 'ManageIQ::Providers::Redhat::InfraManager')


Multiple Redhat providers can be added. The best is to expect provider id always passed in. You can hide the provider selection field (but with an id assigned) in the dialog if there is only one provider available.

@bzwei Hiding/showing of provider field is possible, but it is out of scope of this patch. In this patch I only add a proper target for the dialog to prevent the error.

When the mass transform button is clicked in 'Compute > Infrastructure > Virtual Machines' page, there is no id passed. So I cannot expect that provider id is always passed in.

It doesn't matter if there are many Redhat providers added. The provider object here is needed only as a placeholder to be used as dialog target. find_by will return one of the providers and it is enough for this purpose.

smelamud · 2017-12-19T22:43:03Z

@martinpovolny When testing, please note that ManageIQ/manageiq#16686 must be applied as well.

mkanoor · 2017-12-19T22:59:27Z

app/controllers/mixins/actions/provider_actions/mass_transform.rb

+    module ProviderActions
+      module MassTransform
+        def vm_transform_mass
+          dialog = Dialog.find_by(:label => 'Transform VM')


@smelamud Should this be find_by! so that it raises an error if the Dialog is not found.

mkanoor · 2017-12-19T22:59:45Z

app/controllers/mixins/actions/provider_actions/mass_transform.rb

+          dialog = Dialog.find_by(:label => 'Transform VM')
+          @right_cell_text = _("Transform VMs to RHV")
+          if params.key?(:id)
+            provider = ExtManagementSystem.find_by(:id => params[:id].to_i)


@smelamud Should this be find_by!

mkanoor · 2017-12-19T23:00:16Z

app/controllers/mixins/actions/provider_actions/mass_transform.rb

+              :target_kls => Provider.name
+            )
+          else
+            provider = ExtManagementSystem.find_by(:type => 'ManageIQ::Providers::Redhat::InfraManager')


@smelamud This should also be find_by!

Created a separate vm_transform_mass action to handle VM Transform button clicks in on Providers page and on Compute > Infrastructure > Virtual Machines page. This action interprets the ID passed to it as provider ID, while regular vm_transform action interprets it as VM ID. If provider ID is not passed, the Redhat provider EMS object is used as target. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1514939 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1516497

smelamud · 2017-12-19T23:38:55Z

@mkanoor Done.

miq-bot · 2017-12-19T23:44:25Z

Checked commit smelamud@dbfc5ba with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🍪

mkanoor · 2017-12-20T15:02:44Z

@smelamud 👍

himdel · 2017-12-20T15:58:37Z

app/controllers/mixins/actions/provider_actions/mass_transform.rb

+              :target_kls => Provider.name
+            )
+          else
+            provider = ExtManagementSystem.find_by!(:type => 'ManageIQ::Providers::Redhat::InfraManager')


This bit surprises me, this is equivalent to provider = ManageIQ::Providers::Redhat::InfraManager.first, right? Meaning "I don't know which RHV this is, so just pick a random one".

Shouldn't we always know the provider?
Or do we really not care about the instance, only the type?

.. never mind, already answered in #3073 (comment)

himdel · 2017-12-20T16:04:41Z

app/controllers/mixins/actions/provider_actions/mass_transform.rb

+              dialog.resource_actions.first,
+              :header     => @right_cell_text,
+              :target_id  => provider.id,
+              :target_kls => Provider.name


should this have been provider.class.name? If not, maybe use :target_kls => "Provider", instead.

@himdel I see that Vm.name is used here

manageiq-ui-classic/app/controllers/mixins/actions/vm_actions/transform.rb

Line 14 in dd0eadb

:target_kls => Vm.name

for the same parameter.

OK, if this was intentional, no problem there .. just making sure :)

(On the one hand, Provider.name sounds like a relatively magical way of saying "Provider" .. but then on the other, with the string, we wouldn't get a nice exception if Provider ever gets renamed, so.. (shrug) :).)

smelamud · 2017-12-20T18:22:56Z

@martinpovolny Did you have a chance to test?

Target for mass VM transformation dialog (cherry picked from commit f8417f4) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530772 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530773

simaishi · 2018-01-03T18:56:40Z

Gaprindashvili backport details:

$ git log -1
commit 9363d336270329000a95cfcf6973d89897ed437f
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Thu Dec 21 12:01:54 2017 +0100

    Merge pull request #3073 from smelamud/mass-transform-target
    
    Target for mass VM transformation dialog
    (cherry picked from commit f8417f4e2ea01eca8d6b9e2935f023a810b21c56)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530772
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1530773

miq-bot assigned himdel Dec 14, 2017

miq-bot added the gaprindashvili/yes label Dec 14, 2017

mkanoor reviewed Dec 15, 2017

View reviewed changes

smelamud force-pushed the mass-transform-target branch 3 times, most recently from 780376a to 88af980 Compare December 18, 2017 22:49

martinpovolny added the compute/infrastructure label Dec 19, 2017

bzwei reviewed Dec 19, 2017

View reviewed changes

mkanoor reviewed Dec 19, 2017

View reviewed changes

smelamud force-pushed the mass-transform-target branch from 88af980 to dbfc5ba Compare December 19, 2017 23:38

himdel added the pending core label Dec 20, 2017

himdel reviewed Dec 20, 2017

View reviewed changes

martinpovolny merged commit f8417f4 into ManageIQ:master Dec 21, 2017

martinpovolny assigned martinpovolny and unassigned himdel Dec 21, 2017

martinpovolny added button actions and removed pending core labels Dec 21, 2017

martinpovolny added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 21, 2017

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target for mass VM transformation dialog #3073

Target for mass VM transformation dialog #3073

smelamud commented Dec 14, 2017 •

edited

smelamud commented Dec 14, 2017

smelamud commented Dec 14, 2017

oourfali commented Dec 15, 2017

mkanoor Dec 15, 2017

smelamud Dec 17, 2017 •

edited

mkanoor Dec 15, 2017

smelamud Dec 17, 2017

smelamud commented Dec 19, 2017

mkanoor commented Dec 19, 2017

martinpovolny commented Dec 19, 2017

martinpovolny commented Dec 19, 2017

bzwei Dec 19, 2017

smelamud Dec 19, 2017

smelamud commented Dec 19, 2017

mkanoor Dec 19, 2017

mkanoor Dec 19, 2017

mkanoor Dec 19, 2017

smelamud commented Dec 19, 2017

miq-bot commented Dec 19, 2017

mkanoor commented Dec 20, 2017

himdel Dec 20, 2017

himdel Dec 20, 2017

himdel Dec 20, 2017 •

edited

smelamud Dec 20, 2017

himdel Dec 20, 2017 •

edited

smelamud commented Dec 20, 2017

simaishi commented Jan 3, 2018

Target for mass VM transformation dialog #3073

Target for mass VM transformation dialog #3073

Conversation

smelamud commented Dec 14, 2017 • edited

smelamud commented Dec 14, 2017

smelamud commented Dec 14, 2017

oourfali commented Dec 15, 2017

Choose a reason for hiding this comment

smelamud Dec 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelamud commented Dec 19, 2017

mkanoor commented Dec 19, 2017

martinpovolny commented Dec 19, 2017

martinpovolny commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelamud commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smelamud commented Dec 19, 2017

miq-bot commented Dec 19, 2017

mkanoor commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Dec 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Dec 20, 2017 • edited

Choose a reason for hiding this comment

smelamud commented Dec 20, 2017

simaishi commented Jan 3, 2018

smelamud commented Dec 14, 2017 •

edited

smelamud Dec 17, 2017 •

edited

himdel Dec 20, 2017 •

edited

himdel Dec 20, 2017 •

edited