Dialog fields - make load_values_on_init independent of show_refresh_button #18600

himdel · 2019-03-27T13:23:24Z

Depends on (schema): ManageIQ/manageiq-schema#357 (merged)
Depended on by (ui-components): ManageIQ/ui-components#376

Before, show_refresh_button being falsy meant that load_values_on_init would be considered true.

Now, the two are independent, allowing for a field to only be refreshed by other fields.

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

Cc @gmcculloug @eclarizio

JPrause · 2019-03-28T15:32:13Z

@himdel if this will be able to be backported, can you add the hammer/yes label.

himdel · 2019-03-28T16:15:48Z

@JPrause this depends on a data migration, so, I'm not setting hammer/yes.

(Looks like there is still some discussion about whether to backport with the migration, backport with a product setting, or not backport at all.)

eclarizio · 2019-03-28T16:26:29Z

spec/models/dialog_field_text_box_spec.rb

@@ -43,6 +43,7 @@

      context "when show_refresh_button is false" do
        let(:show_refresh_button) { false }
+        let(:load_values_on_init) { true }


Similar to the above specs that have multiple contexts for whether or not load_values_on_init is true/false, I think this context also needs those contexts since the logic is changing, instead of simply setting it true here just to make this one spec pass as it used to.

Essentially, we're missing the case where show_refresh_button is false and load_values_on_init is also false, since this is a thing that can actually happen now.

Agreed, we need a new spec for the situation where both are false.

I will add those, the specs that are still failing are the same problem, they are a copy of the specs for load_values_on_init true, which is no longer the same.

Right, but I meant as opposed to just setting this value so that it works like it used to. Sorry, my wording was poor.

himdel · 2019-03-29T14:33:12Z

@eclarizio I'm still working on the other dialog_field_text_box specs (EDIT: done), but.. for sorted item,
when show_refresh_button is false, load_values_on_init also false,
and the default value is not in the set of values provided,
it uses the default value anyway.

Which, on the one hand makes sense, because there is no list of values to compare against,
but OTOH is a random difference from the load_values_on_init=true case, where the random defaullt value does not get used.

Do you think that's acceptable behaviour now? :)
(talking about https://github.com/ManageIQ/manageiq/pull/18600/files#diff-2e3988a1d9dd5e5c9de21dd07757fdceR110)

himdel · 2019-03-29T14:48:51Z

@miq-bot remove_label wip

(Unless @eclarizio disagrees :))

himdel · 2019-03-29T14:55:25Z

Also added default_value_for :load_values_on_init, true, which should make sure even fields cretated though the API get a sane default.

(Doing this on DialogField, just because no other dialogfield* models do anything similar, but also because it seems none of the other dialog fields classes are using that field for anything.)

(The editor has the defaults hardcoded, so defaults for the ui will happen in ManageIQ/ui-components#376.)

eclarizio

@eclarizio I'm still working on the other dialog_field_text_box specs (EDIT: done), but.. for sorted item,
when show_refresh_button is false, load_values_on_init also false,
and the default value is not in the set of values provided,
it uses the default value anyway.

Which, on the one hand makes sense, because there is no list of values to compare against,
but OTOH is a random difference from the load_values_on_init=true case, where the random defaullt value does not get used.

Do you think that's acceptable behaviour now? :)
(talking about https://github.com/ManageIQ/manageiq/pull/18600/files#diff-2e3988a1d9dd5e5c9de21dd07757fdceR110)

Hmm, I think it's kinda weird, it's like they're providing a default value that could never actually exist. I think if the automate method isn't running because show_refresh_button and load_values_on_init are both false, it should just have the "<None>" selected.

@gmcculloug Any input here?

Otherwise, I think it looks good 👍

himdel · 2019-03-29T15:32:49Z

Sorry, correction, dialog_field.default_value will be set to the default value.

I have no idea how to test what the field will be set to.

himdel · 2019-03-29T15:35:06Z

Allso, the equivalent logic for dialog_field_text_box is just dynamic && load_values_on_init ? values_from_automate : default_value.

So no idea what should be happening there.
Do any devel docs exist for this area?

eclarizio · 2019-03-29T15:34:45Z

spec/models/dialog_field_sorted_item_spec.rb

            dialog_field.initialize_value_context
-            expect(dialog_field.default_value).to eq("test1")
+            expect(dialog_field.default_value).to eq("test4")
          end

          it "sets the values to what should be returned from automate" do


This description should probably be changed since it's not actually returning values from automate anymore.

Thanks, fixed :)

eclarizio · 2019-03-29T15:35:01Z

spec/models/dialog_field_sorted_item_spec.rb

@@ -125,7 +126,7 @@

          it "sets the values to what should be returned from automate" do


Same with this one.

eclarizio · 2019-03-29T15:45:33Z

Sorry, correction, dialog_field.default_value will be set to the default value.

I have no idea how to test what the field will be set to.

Well, in the UI, the only option to select would be "<None>", so I think when the user hits "submit", it would just submit nil for that field.

Allso, the equivalent logic for dialog_field_text_box is just dynamic && load_values_on_init ? values_from_automate : default_value.

So no idea what should be happening there.
Do any devel docs exist for this area?

I think the above is the intended behavior, we should only be calling automate if both of those values are true. For sorted items, the logic is worded differently, but I think it still ends up being similar. I don't believe we have any devel docs 😢

himdel · 2019-03-29T15:46:02Z

Well, in the UI, the only option to select would be "", so I think when the user hits "submit", it would just submit nil for that field.

Yes, in the UI.

But ... how do I get that value from the model?

(It's not value, or default_value, or initial_value, or extract_dynamic_values ....)

…button Before, show_refresh_button being falsy meant that load_values_on_init would be considered true. Now, the two are independent, allowing for a field to only be refreshed by other fields. https://bugzilla.redhat.com/show_bug.cgi?id=1684575

…to explicitly set load_values_on_init true to test the same situation as before

…n_init=false spec previously, this was identical to show_refresh=false, load_on_init=true

…init=false show_refresh_button=false scenario

miq-bot · 2019-03-29T15:51:41Z

Checked commits https://github.com/himdel/manageiq/compare/1ca148ee613e1d9fde259c18eef8f588150b65bf~...33af34a371067be395189673e25576f1e6377948 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

spec/models/dialog_field_text_box_spec.rb

❗ - Line 47, Col 1 - Layout/EmptyLines - Extra blank line detected.

himdel · 2019-03-29T16:16:29Z

@miq-bot add_label hammer/no

(because ManageIQ/manageiq-schema#357 (comment))

himdel · 2019-04-01T14:12:28Z

Schema PR got merged, so this should be ready too...

@eclarizio do you have any idea where to take the actual value from please? So that we can update the surprising test..

eclarizio · 2019-04-01T14:48:23Z

@himdel What do you mean by "the actual value"?

If you're looking for the list of things that the sorted item should show, it could be .values, but the problem is that calling .values ends up running through automate because it then calls the #raw_values method which then figures out and stores them in the @raw_values instance variable, regardless of the load_values_on_init option. So, if you want to test what your values are when load_values_on_init is false without hitting automate, you should call #initialize_value_context first, and then call #extract_dynamic_values.

If you're looking for the item in the list that should be selected, it should just be .default_value, since #initialize_value_context should end up figuring out what the correct value is. When load_values_on_init is false, we're not explicitly setting the default_value to nil, but since it's not running through automate, it shouldn't have a default value anyway.

Hopefully that helped?

himdel · 2019-04-02T13:52:33Z

Yeah, I meant "the value that ends up getting used" in that case.

Which gets computed in the UI it seems, so I can't test that :(.

None of those functions seem to give me any useful values in this case,
so... maybe I don't care about that test after all.

@eclarizio are you OK with the current state, or do you have specific updates in mind?

eclarizio

Looks good to me 👍

Loicavenel · 2019-04-08T15:36:39Z

Can we merge this one, I need it for test :)

himdel · 2019-04-08T15:59:57Z

Oh yeah, one TODO here - importing old dialogs.
(But it could go as a separate PR.)

gmcculloug · 2019-04-08T19:27:47Z

@himdel I know I mentioned the import of old dialogs, but agree we can work on that in a separate PR.

Not sure we can use the same basic logic that the migration uses for imports

DialogField.where(:show_refresh_button => [false, nil]).update_all('load_values_on_init = true')

as post-migration we would want to honor the settings on load_values_on_init as any combination could be valid.

Do we need a version number or some other indicator in the exports?

I'll merge this PR so we can keep the work moving but someone needs to merge the UI PR in the descripition.

eclarizio · 2019-04-08T19:58:05Z

@himdel So I know the schema PR was hammer/no, but does that mean this and the ui-components PR can't be hammer/yes? The schema PR was just a data migration, right? So, in theory, if this and the ui-components PR get backported, then the users who would like to see this functionality would have to manually adjust their dialogs they care about and it should work, I think?

himdel · 2019-04-09T10:00:08Z

@eclarizio Well, it could be hammer/yes if we introduce a product flag, or something like that, to enable it only on demand.

Otherwise, without adjusting all their reports (or running the migration manually), most likely no dynamic field would ever load on init.

(It's not about whether the users could adjust, it's about whether we can force all of them to adjust mid-release.)

himdel · 2019-04-09T10:50:02Z

Thanks @gmcculloug , investigating :)

I'll merge this PR so we can keep the work moving but someone needs to merge the UI PR in the descripition.

No problem, ui-components PR merged, released as 1.2.3, and created the "make the UIs use the thing" PRs :) (ManageIQ/manageiq-ui-classic#5441 + ManageIQ/manageiq-ui-service#1534)

EDIT: and merged 👍

himdel · 2019-04-10T10:34:24Z

Do we need a version number or some other indicator in the exports?

Maybe we do. Currently we just export the fields, I'm not seeing any versioning metadata.

So unless we want to rely on assuming that nil => true (which won't catch cases where the button was init=true, button=true, changed to init=false, button=true and then to button=false, leaving load_values_on_init set to false in the db), or add an explicit UI option to the import screen, we will need versioning.

That said, maybe that assumption could be enough for imports?
(Dialogs after the migration & ui change should never have nil in there.)
((OTOH adding versioning to import/export looks relatively easy, so maybe better safe than sorry.))

himdel · 2019-04-10T12:14:33Z

=> created #18645 for import/export

Fryguy · 2019-04-15T21:53:21Z

@tinaafitz My suggestion for backport is to "recouple" it in the hammer branch, but differently, so as to ensure the performance benefits of this PR.

That is,

Backport this PR
Change https://github.com/ManageIQ/manageiq/pull/18600/files#diff-7ce0bf669c097595f83217f85438645bL159 (and the other load_values_on_init? as well) back, but slightly different to something like:

  def load_values_on_init?
    load_values_on_init.nil? ? true : load_values_on_init
  end

This way a "null" value (that is, a value that couldn't be set because the UI didn't allow you to because you didn't allow the refresh button), will be true (which matches the migration). With the new ui component, now that the value can be set independently, then we will ignore the nil check and use whatever is in the column.

Regardless, whatever you do with backporting, if you start to allow "real" values in load_values_on_init in hammer, then the current data migration has a problem, because the migration as written now hard-changes all records with show_refresh_button. You will have to change that to be more selective so as not to undo any changes the customer makes.

himdel · 2019-04-16T08:05:56Z

@Fryguy I was thinking opt-in only in hammer, I don't really see a way around that migration, so a product feature/setting to switch to the new version and assume the dialogs are just ok sounds like the only safe way.

(I don't think we can assume nil for existing dialogs, the value doesn't get reset after it was changed to anything else.)

Regardless, whatever you do with backporting, if you start to allow "real" values in load_values_on_init in hammer, then the current data migration has a problem

Great point, but if we hide it under a product feature, that just means using that to run the migration conditionally, would that work?

miq-bot changed the title ~~Dialog fields - make load_values_on_init independent of show_refresh_button~~ [WIP] Dialog fields - make load_values_on_init independent of show_refresh_button Mar 27, 2019

miq-bot added the wip label Mar 27, 2019

This was referenced Mar 27, 2019

Dialog editor: Make "Load Values on Init" independent of "Show Refresh Button" ManageIQ/ui-components#376

Merged

Dialog field - set load_values_on_init to true where show_refresh_button was not enabled ManageIQ/manageiq-schema#357

Merged

gmcculloug requested a review from eclarizio March 28, 2019 12:39

gmcculloug self-assigned this Mar 28, 2019

eclarizio reviewed Mar 28, 2019

View reviewed changes

miq-bot changed the title ~~[WIP] Dialog fields - make load_values_on_init independent of show_refresh_button~~ Dialog fields - make load_values_on_init independent of show_refresh_button Mar 29, 2019

miq-bot removed the wip label Mar 29, 2019

eclarizio reviewed Mar 29, 2019

View reviewed changes

himdel added 5 commits March 29, 2019 15:47

dialog_field_text_box_spec: fix spec using show_refresh_button false …

165f2f8

…to explicitly set load_values_on_init true to test the same situation as before

dialog_field_sorted_item_spec - update the show_refresh=false, load_o…

5b76a42

…n_init=false spec previously, this was identical to show_refresh=false, load_on_init=true

dialog_field_text_box_spec - test default value handling for load_on_…

4fce543

…init=false show_refresh_button=false scenario

DialogField - make load_values_on_init default to true

33af34a

miq-bot added the hammer/no label Mar 29, 2019

eclarizio approved these changes Apr 2, 2019

View reviewed changes

gmcculloug merged commit 7ef7460 into ManageIQ:master Apr 8, 2019

himdel deleted the bz1684575 branch April 9, 2019 10:01

himdel mentioned this pull request Apr 10, 2019

Dialog import/export - add versioning, migrate load_values_on_init #18645

Merged

mfeifer added this to the Sprint 109 Ending Apr 15, 2019 milestone Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dialog fields - make load_values_on_init independent of show_refresh_button #18600

Dialog fields - make load_values_on_init independent of show_refresh_button #18600

himdel commented Mar 27, 2019 •

edited

Loading

JPrause commented Mar 28, 2019

himdel commented Mar 28, 2019

eclarizio Mar 28, 2019

himdel Mar 28, 2019 •

edited

Loading

eclarizio Mar 28, 2019

himdel commented Mar 29, 2019 •

edited

Loading

himdel commented Mar 29, 2019

himdel commented Mar 29, 2019 •

edited

Loading

eclarizio left a comment •

edited

Loading

himdel commented Mar 29, 2019

himdel commented Mar 29, 2019

eclarizio Mar 29, 2019

himdel Mar 29, 2019

eclarizio Mar 29, 2019

eclarizio commented Mar 29, 2019 •

edited

Loading

himdel commented Mar 29, 2019 •

edited

Loading

miq-bot commented Mar 29, 2019

himdel commented Mar 29, 2019

himdel commented Apr 1, 2019

eclarizio commented Apr 1, 2019 •

edited

Loading

himdel commented Apr 2, 2019

eclarizio left a comment

Loicavenel commented Apr 8, 2019

himdel commented Apr 8, 2019

gmcculloug commented Apr 8, 2019

eclarizio commented Apr 8, 2019

himdel commented Apr 9, 2019 •

edited

Loading

himdel commented Apr 9, 2019 •

edited

Loading

himdel commented Apr 10, 2019 •

edited

Loading

himdel commented Apr 10, 2019 •

edited

Loading

Fryguy commented Apr 15, 2019

himdel commented Apr 16, 2019 •

edited

Loading

		@@ -125,7 +126,7 @@

		it "sets the values to what should be returned from automate" do

Dialog fields - make load_values_on_init independent of show_refresh_button #18600

Dialog fields - make load_values_on_init independent of show_refresh_button #18600

Conversation

himdel commented Mar 27, 2019 • edited Loading

JPrause commented Mar 28, 2019

himdel commented Mar 28, 2019

eclarizio Mar 28, 2019

Choose a reason for hiding this comment

himdel Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

eclarizio Mar 28, 2019

Choose a reason for hiding this comment

himdel commented Mar 29, 2019 • edited Loading

himdel commented Mar 29, 2019

himdel commented Mar 29, 2019 • edited Loading

eclarizio left a comment • edited Loading

Choose a reason for hiding this comment

himdel commented Mar 29, 2019

himdel commented Mar 29, 2019

eclarizio Mar 29, 2019

Choose a reason for hiding this comment

himdel Mar 29, 2019

Choose a reason for hiding this comment

eclarizio Mar 29, 2019

Choose a reason for hiding this comment

eclarizio commented Mar 29, 2019 • edited Loading

himdel commented Mar 29, 2019 • edited Loading

miq-bot commented Mar 29, 2019

himdel commented Mar 29, 2019

himdel commented Apr 1, 2019

eclarizio commented Apr 1, 2019 • edited Loading

himdel commented Apr 2, 2019

eclarizio left a comment

Choose a reason for hiding this comment

Loicavenel commented Apr 8, 2019

himdel commented Apr 8, 2019

gmcculloug commented Apr 8, 2019

eclarizio commented Apr 8, 2019

himdel commented Apr 9, 2019 • edited Loading

himdel commented Apr 9, 2019 • edited Loading

himdel commented Apr 10, 2019 • edited Loading

himdel commented Apr 10, 2019 • edited Loading

Fryguy commented Apr 15, 2019

himdel commented Apr 16, 2019 • edited Loading

himdel commented Mar 27, 2019 •

edited

Loading

himdel Mar 28, 2019 •

edited

Loading

himdel commented Mar 29, 2019 •

edited

Loading

himdel commented Mar 29, 2019 •

edited

Loading

eclarizio left a comment •

edited

Loading

eclarizio commented Mar 29, 2019 •

edited

Loading

himdel commented Mar 29, 2019 •

edited

Loading

eclarizio commented Apr 1, 2019 •

edited

Loading

himdel commented Apr 9, 2019 •

edited

Loading

himdel commented Apr 9, 2019 •

edited

Loading

himdel commented Apr 10, 2019 •

edited

Loading

himdel commented Apr 10, 2019 •

edited

Loading

himdel commented Apr 16, 2019 •

edited

Loading