fix find_checked_items

[matheuscmelo]

if :miq_grid_checks is not present in params, this method only returns 1 or no ids.
this PR fixes it.

[miq-bot]

Checked commit matheuscmelo@d22aeba with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[martinpovolny]

ping @romanblanco , @PanSpagetka

[PanSpagetka]

With @romanblanco we are trying to verify this change, but we are having some troubles with reproducing it. This else branch is related to legacy code so I wonder where you have encountered issue which lead you to this change. Could you please describe situation when this fix does help you?

[romanblanco]

With @romanblanco we are trying to verify this change, but we are having some troubles with reproducing it. This else branch is related to legacy code so I wonder where you have encountered issue which lead you to this change. Could you please describe situation when this fix does help you?

@matheuscmelo I've tried the temporary fix you've mentioned in a chat:

manageiq-ui-classic/app/helpers/application_helper/toolbar/physical_servers_center.rb

Line 246 in e776256

:klass => ApplicationHelper::Button::PhysicalServerProvision

changing this class to ApplicationHelper::Button::ConfiguredSystemProvision should fix this for a while

but this takes me just to a routing error

[himdel]

Testing:

[11] pry(#<EmsCloudController>)> @_params[:check_1234] = "1"                             
=> "1"                                                                                   
[12] pry(#<EmsCloudController>)> @_params[:check_10r25] = "1"                            
=> "1"                                                                                   
[13] pry(#<EmsCloudController>)> @_params[:check_11_12_13] = "1"                         
=> "1"                                                                                   
[14] pry(#<EmsCloudController>)> params                                                  
=> <ActionController::Parameters {"controller"=>"ems_cloud", "action"=>"show", "id"=>"38", "check_11_12_13"=>"1", "check_1234"=>"1", "check_10r25"=>"1"} permitted: false>

> find_checked_items                                      
=> ["11_12_13", "1234", "10r25"]

Verified that the method works as intended.

(Accepting a value different than "1" would mean all legacy checkboxes are considered checked.)

[matheuscmelo]

The issue I'm trying to fix is this:

It appears in the Physical Servers page. To access the provision screen I'm changing the class of the button from this page because of another bug we're trying to fix, it looks like this:
(In manageiq-ui-classic/app/helpers/application_helper/toolbar/physical_servers_center.rb, from line 232)

select(
        :physical_server_lifecycle_choice,
        'fa fa-recycle fa-lg',
        t = N_('Lifecycle'),
        t,
        :enabled => true,
        :items   => [
          button(
            :physical_server_provision,
            'pficon pficon-add-circle-o fa-lg',
            t = N_('Provision Physical Server'),
            t,
            :url       => "provision",
            :url_parms => "main_div",
            :onwhen    => "1+",
            :klass     => ApplicationHelper::Button::ConfiguredSystemProvision
          )
        ]
      )

[himdel]

@matheuscmelo and what's in params when that happens?

[matheuscmelo]

@himdel {"check_1"=>"1", "check_2"=>"2", "controller"=>"physical_server", "action"=>"provision"}

[matheuscmelo]

@romanblanco @PanSpagetka @himdel any news?

[himdel]

@matheuscmelo Ah, sorry about that...

So.. I don't think we can change the behaviour of find_checked_items, this may still break some older checkboxes. (Or we need to finish the GTL cleanup to be sure there are no older checkboxes remaining.) ((GTL = grid/table/list .. the component to display multiple items))

The reason you're getting those check_123 => 123 values is that the new GTL really uses those values, so we could make sure GTL sends the right value - that would be https://github.com/ManageIQ/ui-components/blob/master/src/gtl/components/data-table/data-table.html#L45-L46 .
(Fixing this there should not break anything as far as I'm aware.)

But... however much I'm trying, I can't reproduce your problem...
Any chance I'm missing a step?:

Go to Compute > Physical Infrastructure > Providers (url = /ems_physical_infra/show_list)
Select 2 items in the list
Click a toolbar button

... what I'm seeing in the request:

miq_grid_checks: 25,54
check_25: 25
check_54: 54

So.. while those check_* are indeed wrong, you should never be hitting that code path in the first place, as miq_grid_checks is available.

TLDR: check_* is obsolete, make sure miq_grid_checks is available instead.

[matheuscmelo]

@himdel actually it only occurs in Lifecycle > Provision button. If you select the first and others servers, it only indicates the first. If you don't select the first and select others, it blows a error.

[douglasgabriel]

I've just analyze a little bit the problem and I think that we can solve this issue by adding :send_checked => true property on button definition. Like this:

button(
    :physical_server_provision,
    'pficon pficon-add-circle-o fa-lg',
    t = N_('Provision Physical Server'),
    t,
    :url       => "provision",
    :send_checked => true,
    :url_parms => "main_div",
    :klass     => ApplicationHelper::Button::PhysicalServerProvision
)

This way the miq_grid_checks is filled.

[martinpovolny]

👍

I've just analyze a little bit the problem and I think that we can solve this issue by adding :send_checked => true property on button definition. Like this:

yes, that should be set

[matheuscmelo]

Thanks @douglasgabriel for the answer and @himdel @PanSpagetka @martinpovolny @romanblanco for the help. I'll open another PR and close this one.