Dialog field - set load_values_on_init to true where show_refresh_button was not enabled #357

himdel · 2019-03-27T13:40:10Z

Replaces code that was previously hardcoded in the model..

    def load_values_on_init?
      return true unless show_refresh_button
      load_values_on_init
    end

Needed to make it posssible to set both these fields to false,
without breaking all the old fields where the value of load_values_on_init previously didn't matter.

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

(Depended on by:

(core) Dialog fields - make load_values_on_init independent of show_refresh_button manageiq#18600
(ui-components) Dialog editor: Make "Load Values on Init" independent of "Show Refresh Button" ui-components#376
)

himdel · 2019-03-27T13:41:02Z

Cc @gmcculloug @eclarizio
(this is the last one of the three :))

eclarizio

I think my only concern about all of this is that we should probably now be defaulting load_values_on_init to be true, right? Previously, as you said, this was handled by the code, and this migration fixes any existing fields, but going forward new ones will be created with both show_refresh_button and load_values_on_init to be false. It might be weird for people used to the system that don't change those switches at all and now suddenly their newly created fields don't work.

Other than that I think this fix is straightforward

himdel · 2019-03-27T14:48:38Z

Agreed @eclarizio , will add that to the ui-components PR (assuming that's what sets the initial value), thanks :)

eclarizio · 2019-03-27T14:50:21Z

Actually, should it be added here? For people that use the API to create fields? (Is that even a thing people do?)

@gmcculloug Maybe you have some insight on this.

himdel · 2019-03-27T14:51:40Z

Well, not in schema (or I don't know how), but possibly default_value_for in the model?

eclarizio · 2019-03-27T14:54:42Z

Yeah, that's probably fine

…ton was not enabled Replaces code that was previously hardcoded in the model.. def load_values_on_init? return true unless show_refresh_button load_values_on_init end Needed to make it posssible to set both these fields to false, without breaking all the old fields where the value of `load_values_on_init` previously didn't matter. https://bugzilla.redhat.com/show_bug.cgi?id=1684567 https://bugzilla.redhat.com/show_bug.cgi?id=1684575

… set to true unless show_refresh_button was set

JPrause · 2019-03-28T15:31:34Z

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

himdel · 2019-03-29T14:31:20Z

(EDIT: wrong place for the comment, moved to ManageIQ/manageiq#18600 (comment))

carbonin · 2019-03-29T15:33:33Z

@JPrause We typically don't backport schema PRs. Is there a compelling reason for this one to be backported?

carbonin

What is the actual relationship between these columns? Should they really be just one column with some logic around it?

carbonin · 2019-03-29T15:31:04Z

spec/migrations/20190327132620_dialog_field_load_values_on_init_spec.rb

+  migration_context :up do
+    before do
+      [true, false, nil].each do |show|
+        [true, false, nil].each do |load|


Using keywords as variable names makes me nervous ... maybe load_value here?

carbonin · 2019-03-29T15:32:00Z

spec/migrations/20190327132620_dialog_field_load_values_on_init_spec.rb

+    end
+
+    it "updates load_values_on_init" do
+      expect(dialog_field_stub.pluck(:load_values_on_init)).to eq([true, false, nil, true, true, true, true, true, true])


Can you add an explicit sort here so we don't get bit by locale or OS default sorting rules?

I was hoping we can reliably get them in order they were created.

Is there any nice way of doing that?
(But I can live with sort too...)

Added a sort_by(:id) since we probably can rely on the ids being ascending, right?

And split into 3 lines to better show the relation.

himdel · 2019-03-29T15:39:57Z

@himdel if this will be able to be backported, can you add the hammer/yes label.
@JPrause We typically don't backport schema PRs. Is there a compelling reason for this one to be backported?

Explicitly leaving the backport/no backport decision on @gmcculloug & @Loicavenel.

It looks like there is a need for fields that will only refresh because other fields change in hammer, and this is the only (sane) way of achieving that.

himdel · 2019-03-29T15:41:42Z

What is the actual relationship between these columns? Should they really be just one column with some logic around it?

@carbonin it's described in this PR's description ;)

Before, load_on_init was completely ignored (and assumed to be true) when show_refresh_button was not set to true, and only read if show_refresh_button was true.

Now, load_on_init and show_refresh_button will be completely independent.

But we still need to make sure that all those "assumed to be true" load_on_init values get explicitly set to true for this to not break old fields.

…order fits, don't use keywords

carbonin · 2019-03-29T16:03:12Z

Okay, that makes sense @himdel

As far as the backport decision, it actually seems to me that this cannot be backported because we wouldn't be able to rely on this logic for people who have already upgraded to hammer (and by extension will not run this migration).

carbonin · 2019-03-29T16:05:41Z

spec/migrations/20190327132620_dialog_field_load_values_on_init_spec.rb

+      expect(dialog_field_stub.sort_by(:id).pluck(:load_values_on_init)).to eq([
+        true, false, nil, # show_button was true
+        true, true, true,
+        true, true, true,


I think I would just prefer individual specs for each case. For example:

it "doesnt update load_values_on_init when show_refresh_button is true" do ... end it "updates load_values_on_init when show_refresh_button is not true" do ... end

Then we don't have to play this ordering game at all.

Yeah, maybe better with less magic, fixing :)

himdel · 2019-03-29T16:07:19Z

As far as the backport decision, it actually seems to me that this cannot be backported because we wouldn't be able to rely on this logic for people who have already upgraded to hammer (and by extension will not run this migration).

Agreed that the whole feature can absolutely not work without the migration backported too, if that's what you mean, yes.

(But if we did, it would, right?)

carbonin · 2019-03-29T16:09:39Z

Agreed that the whole feature can absolutely not work without the migration backported too, if that's what you mean, yes.

(But if we did, it would, right?)

No, because people who have already upgraded to hammer will not run this migration. Users are only expected to run migrations on major releases. This is why we don't backport migrations.

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

Ah, well, in that case..

@miq-bot add_label hammer/no

That pretty much settles it I think, unless we want to wrap it in a product feature switch or something like that.

Thanks! :)

carbonin · 2019-03-29T17:22:30Z

spec/migrations/20190327132620_dialog_field_load_values_on_init_spec.rb

+  let(:dialog_field_stub) { migration_stub(:DialogField) }
+
+  migration_context :up do
+    let (:show_button) { nil }


I don't think this is needed.

It was, undefined local variable or method 'show_button' for #<RSpec::ExampleGroups::DialogFieldLoadValuesOnInit::Up:0x0000000009226718> otherwise.

carbonin · 2019-03-29T17:22:50Z

spec/migrations/20190327132620_dialog_field_load_values_on_init_spec.rb

+    end
+
+    context "when show_refresh_button is true" do
+      let (:show_button) { true }


Pretty sure there shouldn't be a space between the let and ( here. And that's why you're getting the rubocop issues.

Same with the other let(:show_button) calls.

… three possibilities

himdel · 2019-04-01T12:45:36Z

Thanks @carbonin , those should be resolved now..

have to move the definition up too

miq-bot · 2019-04-01T13:02:50Z

Checked commits https://github.com/himdel/manageiq-schema/compare/e8c1faa5dbbf46ae3d4c2ac2ac462da64dd988c2~...8252f75d908b68c5db84bf7cf6cb48b0d18ef731 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

db/migrate/20190327132620_dialog_field_load_values_on_init.rb

❗ - Line 8, Col 63 - Rails/SkipsModelValidations - Avoid using update_all because it skips validations.

…ersion 1 export Applies the same change as ManageIQ/manageiq-schema#357 when importing older exported service dialogs. https://bugzilla.redhat.com/show_bug.cgi?id=1684575 https://bugzilla.redhat.com/show_bug.cgi?id=1684567

This was referenced Mar 27, 2019

Dialog fields - make load_values_on_init independent of show_refresh_button ManageIQ/manageiq#18600

Merged

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

Merged

eclarizio reviewed Mar 27, 2019

View reviewed changes

eclarizio approved these changes Mar 27, 2019

View reviewed changes

himdel added 2 commits March 28, 2019 15:06

DialogFieldLoadValuesOnInit spec - test that load_values_on_init gets…

145312a

… set to true unless show_refresh_button was set

himdel closed this Mar 29, 2019

himdel reopened this Mar 29, 2019

carbonin reviewed Mar 29, 2019

View reviewed changes

20190327132620_dialog_field_load_values_on_init spec - make sure the …

2c98984

…order fits, don't use keywords

carbonin reviewed Mar 29, 2019

View reviewed changes

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

carbonin reviewed Mar 29, 2019

View reviewed changes

dialog_field_load_values_on_init spec - use explicit contexts for the…

0e6f406

… three possibilities

DialogFieldLoadValuesOnInit spec - fix undefined local show_button

8252f75

have to move the definition up too

carbonin self-assigned this Apr 1, 2019

carbonin approved these changes Apr 1, 2019

View reviewed changes

carbonin merged commit 445eac4 into ManageIQ:master Apr 1, 2019

carbonin added this to the Sprint 108 Ending Apr 1, 2019 milestone Apr 1, 2019

himdel deleted the bz1684575 branch April 1, 2019 14:10

himdel mentioned this pull request Apr 10, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dialog field - set load_values_on_init to true where show_refresh_button was not enabled #357

Dialog field - set load_values_on_init to true where show_refresh_button was not enabled #357

himdel commented Mar 27, 2019 •

edited

Loading

himdel commented Mar 27, 2019

eclarizio left a comment

himdel commented Mar 27, 2019

eclarizio commented Mar 27, 2019

himdel commented Mar 27, 2019 •

edited

Loading

eclarizio commented Mar 27, 2019

JPrause commented Mar 28, 2019

himdel commented Mar 29, 2019 •

edited

Loading

carbonin commented Mar 29, 2019

carbonin left a comment

carbonin Mar 29, 2019

himdel Mar 29, 2019

carbonin Mar 29, 2019

himdel Mar 29, 2019

himdel Mar 29, 2019

himdel commented Mar 29, 2019

himdel commented Mar 29, 2019 •

edited

Loading

carbonin commented Mar 29, 2019

carbonin Mar 29, 2019

himdel Mar 29, 2019

himdel commented Mar 29, 2019

carbonin commented Mar 29, 2019 •

edited

Loading

himdel commented Mar 29, 2019

carbonin Mar 29, 2019

himdel Apr 1, 2019 •

edited

Loading

carbonin Mar 29, 2019

carbonin Mar 29, 2019

himdel commented Apr 1, 2019

miq-bot commented Apr 1, 2019

Dialog field - set load_values_on_init to true where show_refresh_button was not enabled #357

Dialog field - set load_values_on_init to true where show_refresh_button was not enabled #357

Conversation

himdel commented Mar 27, 2019 • edited Loading

himdel commented Mar 27, 2019

eclarizio left a comment

Choose a reason for hiding this comment

himdel commented Mar 27, 2019

eclarizio commented Mar 27, 2019

himdel commented Mar 27, 2019 • edited Loading

eclarizio commented Mar 27, 2019

JPrause commented Mar 28, 2019

himdel commented Mar 29, 2019 • edited Loading

carbonin commented Mar 29, 2019

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Mar 29, 2019

himdel commented Mar 29, 2019 • edited Loading

carbonin commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Mar 29, 2019

carbonin commented Mar 29, 2019 • edited Loading

himdel commented Mar 29, 2019

Choose a reason for hiding this comment

himdel Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Apr 1, 2019

miq-bot commented Apr 1, 2019

himdel commented Mar 27, 2019 •

edited

Loading

himdel commented Mar 27, 2019 •

edited

Loading

himdel commented Mar 29, 2019 •

edited

Loading

himdel commented Mar 29, 2019 •

edited

Loading

carbonin commented Mar 29, 2019 •

edited

Loading

himdel Apr 1, 2019 •

edited

Loading