Forcing default_value to an array, if the dynamic dropdown is multiselect #17272

romanblanco · 2018-04-10T14:18:22Z

Opening this PR instead of #17259

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

The value of the is set to be an id of provider (ExtManagementSystem)

Steps for Testing/QA

setup automate expression method:

automation -> automate -> explorer -> datastore -> configuration -> add a new domain
select the new domain -> configuration -> add a new instance
select the new instance -> configuration -> add a new class
select the new class -> switch to the methods tab -> configuration -> add a new method
- select expression -> Expression Object set to ExtManagementSystem
- at the bottom select field -> Provider: Total Vms >= 1 -> commit -> save
select the class in explorer -> switch to the schema tab -> configuration -> edit selected schema
- click the green plus -> set name to execute -> type set to method -> data type set to String -> save
select the class in explorer -> switch to the instance tab -> configuration -> add a new instance
- use the name of created method as the value

the rest is from the BZ ticket:

Create a service dialog containing a single drop-down list element. Make this element dynamic, and create an expression method to populate it (I used a simple expression method that lists all VMs). Leave the 'Multiselect' option as 'No'. Save the dialog.
Now edit the dialog, and change the 'Multiselect' option to 'Yes'. Save the dialog

Thanks for the help with setting up the expression method @pkomanek!

…lect Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559030

romanblanco · 2018-04-10T14:39:54Z

@miq-bot add_label bug
@miq-bot add_label blocker
@miq-bot assign @eclarizio

eclarizio

See my comment about rearranging/adding a spec. Otherwise, looking good 👍

eclarizio · 2018-04-10T15:38:46Z

spec/models/dialog_field_sorted_item_spec.rb

+  before(:each) do
+    allow(dialog_field).to receive(:force_multi_value).and_return(false)
+  end
+


I don't think this needs to be at the top level. It should be moved into a deeper context within the #values describe method, and there needs to be a spec relevant to when it's set to false and when it's set to true.

describe "#values" do # setup that exists context "when the field is dynamic" do # setup that exists context "when force_multi_value is set to false" do # this is where you would do your allow context "when the default_value is included in the list of returned values" do # this context already exists and you would simply reuse the tests here end end context "when force_multi_value is set to true" do # this is where you would do your allow, but return true context "when the default_value is included in the list of returned values" do # this is the same existing context but now force_multi_value is true and there # would be different 'let(:default_value)' values, and different 'it' blocks end end end end

Actually, though, the relevant spec needs to be in the dialog_field_drop_down_list_spec.rb because we need to make sure that the private method default_value_included? isn't crashing like it was in the bug. So, similar to the contexts I provided above, but there's a few more in the drop down list spec that you will need to integrate.

Regardless, we definitely need a spec that proves the bug is fixed 😄

romanblanco · 2018-04-10T17:16:06Z

@miq-bot add_label gaprindashvili/yes

JPrause · 2018-04-12T15:05:21Z

@eclarizio is there anything else needed, if no, can this blocker be merged.

romanblanco · 2018-04-12T15:36:44Z

@JPrause the specs are still missing.

JPrause · 2018-04-13T13:51:21Z

@romanblanco thanks for the update. We have dev complete on April 16. Will this be completed and merged by then? cc @eclarizio

romanblanco · 2018-04-13T14:07:20Z

@JPrause Yes

Testing that default_value is wrapped into an array, if the dynamic dialog field is multiselect

miq-bot · 2018-04-16T12:46:43Z

Checked commits romanblanco/manageiq@aa5511d~...2cde07d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

spec/models/dialog_field_sorted_item_spec.rb

❗ - Line 173, Col 13 - Style/CommentAnnotation - Annotation keywords like FIXME should be all upper case, followed by a colon, and a space, then a note describing the problem.

himdel · 2018-04-16T13:12:30Z

app/models/dialog_field_sorted_item.rb

@@ -96,6 +96,9 @@ def sort_data(data_to_sort)
  end

  def determine_selected_default_value
+    if dynamic? && force_multi_value && !default_value.kind_of?(Array)


@romanblanco doesn't it make more sense to put this after the use_first_value_as_default unless default_value_included?(@raw_values) line?

If you Array.wrap first, default_value_included? will never return true - doesn't this break it for some cases? (Like, always overwriting anything wrapped with the first value?)

Also, if use_first_value_as_default does get called, it won't wrap the default value in the array (until you re-run this method twice).

Or is this intended?

@himdel It does make more sense. You're right. I'll check again, but this seems more reasonable. Thanks!

@himdel I've tried, and there's another problem while ordering a dialog containing dialog containing dynamic dropdown list after the changes you've suggested.

The workflow fails on 500 error because of calling collect operation on nil.

If this doesn't block the PR from merging, I'd say it'll be better to fix this in a following PR, as the situation is a bit confusing to me, and we want to merge this soon.

Aaah, because #16978 overrides it to do some JSON magic, without making sure default_value is a JSON... this is really broken :(

Looks like default_value can be anything from an array to a JSON-encoded array string :(

I don't know. The fix looks buggy without it, but the bug looks larger than the fix :).

Cc @eclarizio

(Somebody needs to really decide when we do the conversion to json and back and make that consistent and get rid of all those "maybe it is an array, maybe it's a json or maybe it's a string" cases. Or do we need an actual_value method? :) )

Either way, I guess you need to know why you get that error @romanblanco.

Either use_first_value_as_default is broken and should never be called in your situation.
Or it's supposed to be called and needs to be fixed too.

EDIT: IMO this may need a bigger rewrite ... looks like we're fixing random bugs again and again in this place .. and all of them could be fixed by actually fixing the field type in the DB.

Does it look like this will be able to be merged for today's dev complete milestone?

@himdel I think the line:

use_first_value_as_default unless default_value_included?(@raw_values)

is fine to stay as it is. The values list will always end up being an array of arrays, so there's no problem with use_first_value_as_default. The issue is in default_value_included?, because the default_value can be either a single value, or multiple values, but string encoded (this is where the JSON.parse comes in). To fix this bug, I think that just needs to be modified to handle when the default value is not set, when it's multiple values, and when it's a single value.

OK, no complaints for me then :)

@eclarizio You may want to adjust your review, still shows up as ⚔️

@eclarizio if this can be merged in the next few hours, we probably can get this into today's blocker-only build. If not, can you let me know so I can pass the info along.

himdel

👍 I've seen it work :)

eclarizio

Lets get this into the blocker build and then we can re-evaluate.

Forcing default_value to an array, if the dynamic dropdown is multiselect (cherry picked from commit 41f514d) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1568473

simaishi · 2018-04-17T15:04:29Z

Gaprindashvili backport details:

$ git log -1
commit 821e7822b97f9778fcf550e2c236f2d6767e3e69
Author: Greg McCullough <[email protected]>
Date:   Tue Apr 17 10:36:41 2018 -0400

    Merge pull request #17272 from romanblanco/bz1559030-v2
    
    Forcing default_value to an array, if the dynamic dropdown is multiselect
    (cherry picked from commit 41f514dbfb949c4921969514d8824c7dd6c6344c)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1568473

Forcing default_value to an array, if the dynamic dropdown is multise…

aa5511d

…lect Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1559030

romanblanco force-pushed the bz1559030-v2 branch from ed2df30 to 70f16ac Compare April 10, 2018 14:35

miq-bot added bug blocker labels Apr 10, 2018

miq-bot assigned eclarizio Apr 10, 2018

eclarizio suggested changes Apr 10, 2018

View reviewed changes

miq-bot added the gaprindashvili/yes label Apr 10, 2018

Adding specs for modified method

2cde07d

Testing that default_value is wrapped into an array, if the dynamic dialog field is multiselect

romanblanco force-pushed the bz1559030-v2 branch from 70f16ac to 2cde07d Compare April 16, 2018 12:42

himdel reviewed Apr 16, 2018

View reviewed changes

himdel approved these changes Apr 17, 2018

View reviewed changes

eclarizio approved these changes Apr 17, 2018

View reviewed changes

gmcculloug merged commit 41f514d into ManageIQ:master Apr 17, 2018

gmcculloug added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 17, 2018

romanblanco deleted the bz1559030-v2 branch April 17, 2018 14:48

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 17, 2018

eclarizio mentioned this pull request May 21, 2018

Adjust how default_value is calculated for multi-select drop downs #17449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forcing default_value to an array, if the dynamic dropdown is multiselect #17272

Forcing default_value to an array, if the dynamic dropdown is multiselect #17272

romanblanco commented Apr 10, 2018

romanblanco commented Apr 10, 2018

eclarizio left a comment

eclarizio Apr 10, 2018

romanblanco commented Apr 10, 2018

JPrause commented Apr 12, 2018

romanblanco commented Apr 12, 2018

JPrause commented Apr 13, 2018

romanblanco commented Apr 13, 2018

miq-bot commented Apr 16, 2018

himdel Apr 16, 2018 •

edited

Loading

romanblanco Apr 16, 2018

romanblanco Apr 16, 2018

himdel Apr 16, 2018 •

edited

Loading

himdel Apr 16, 2018 •

edited

Loading

JPrause Apr 16, 2018

eclarizio Apr 16, 2018

himdel Apr 17, 2018

himdel Apr 17, 2018 •

edited

Loading

JPrause Apr 17, 2018

himdel left a comment

eclarizio left a comment

simaishi commented Apr 17, 2018

Forcing default_value to an array, if the dynamic dropdown is multiselect #17272

Forcing default_value to an array, if the dynamic dropdown is multiselect #17272

Conversation

romanblanco commented Apr 10, 2018

Steps for Testing/QA

romanblanco commented Apr 10, 2018

eclarizio left a comment

Choose a reason for hiding this comment

eclarizio Apr 10, 2018

Choose a reason for hiding this comment

romanblanco commented Apr 10, 2018

JPrause commented Apr 12, 2018

romanblanco commented Apr 12, 2018

JPrause commented Apr 13, 2018

romanblanco commented Apr 13, 2018

miq-bot commented Apr 16, 2018

himdel Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

romanblanco Apr 16, 2018

Choose a reason for hiding this comment

romanblanco Apr 16, 2018

Choose a reason for hiding this comment

himdel Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

himdel Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

JPrause Apr 16, 2018

Choose a reason for hiding this comment

eclarizio Apr 16, 2018

Choose a reason for hiding this comment

himdel Apr 17, 2018

Choose a reason for hiding this comment

himdel Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

JPrause Apr 17, 2018

Choose a reason for hiding this comment

himdel left a comment

Choose a reason for hiding this comment

eclarizio left a comment

Choose a reason for hiding this comment

simaishi commented Apr 17, 2018

himdel Apr 16, 2018 •

edited

Loading

himdel Apr 16, 2018 •

edited

Loading

himdel Apr 16, 2018 •

edited

Loading

himdel Apr 17, 2018 •

edited

Loading