Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change reconfigure setup to include values configured with originally #17647

Merged
merged 2 commits into from
Jun 29, 2018

Conversation

eclarizio
Copy link
Member

@eclarizio eclarizio commented Jun 28, 2018

This is simply the backend counterpart to this UI PR. The problem is that for most dialogs, we don't need to inject values on the initial load, because they are expecting to be presented with the defaults. This was causing an issue since the users wanted to see what they had previously selected from a first configuration, but when clicking on the reconfigure button, they were shown the defaults.

This problem is furthered by the fact that some fields are dynamic and depend on the values of others, so if the defaults are simply shown, some fields like drop downs may not even contain the right options to pick from!

The solution is ultimately that we set up the dialog such that all of its fields have a given value coming from what the service knows about how it was configured initially, and then we call the automate method to potentially do some calculations. In the case of a text box, whatever comes back will get thrown away, since we want to use the value that was known from initial configuration, but for a drop-down, for example, the options need to be recalculated and the correct one selected. I know that sounds extremely confusing, and unfortunately, it is 😢

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

This should go in before the UI PR.

@miq-bot add_label gaprindashvili/yes, blocker, bug
/cc @gmcculloug @tinaafitz

@d-m-u I realize this is a rather large undertaking but can you test and review for me?
@syncrou Can you review please?

@d-m-u
Copy link
Contributor

d-m-u commented Jun 28, 2018

Other than the tests, looks great.

@gmcculloug
Copy link
Member

@d-m-u Good find, but I believe that was the previous behavior in the Fine branch. Can you verify this?

@eclarizio eclarizio force-pushed the BZ1591484-Reconfigure branch from 98732b1 to 5e6a689 Compare June 28, 2018 14:44
@d-m-u
Copy link
Contributor

d-m-u commented Jun 28, 2018

With my test dialog, the tag control didn't load with the value I'd sent. Can we possibly address later?

@gmcculloug
Copy link
Member

@eclarizio Would the tag control be an issue since we do not support setting a default value for that today? @d-m-u Based on that I suspect that multi-select drop-downs may not work either. Please test that as well.

We may need to address those in a separate PR.

@d-m-u
Copy link
Contributor

d-m-u commented Jun 28, 2018

@gmcculloug yeah, the multiselect gets an api error:
On second pass, DrewB can't replicate and I can't either. The multiselect only picks the default but it doesn't seem to be blowing up.

@simaishi
Copy link
Contributor

@eclarizio If you're going to push another commit, please update BZ link to the 'master' BZ (https://bugzilla.redhat.com/show_bug.cgi?id=1580987), so the correct BZ will be commented.

@@ -134,63 +134,6 @@
end
end

describe "#initialize_with_values" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eclarizio - Only thing I see, (and it's something I'm not sure I don't see) are all of these 'red' test scenarios fully null and void? e.g. because of the changes in this PR we don't need them?

Or are your new tests sufficiently covering your new version of the scenarios these tests originally covered?

It's hard to tell from the PR as none of the test names coincide.

If I was more familiar with the process I probably wouldn't have to ask the question at all. ☹️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#initialize_with_values is no longer in use, so I've removed it and basically replaced it with #initialize_with_given_value, so those specs needed to be removed.

@d-m-u
Copy link
Contributor

d-m-u commented Jun 28, 2018

The timepicker also gets set back to something appearing to be the default, but I think that's expected behavior because I think eclarizio was getting to that before this PR became a prio.

@eclarizio eclarizio force-pushed the BZ1591484-Reconfigure branch from c3c4ebd to 71d857f Compare June 28, 2018 21:53
@miq-bot
Copy link
Member

miq-bot commented Jun 28, 2018

Checked commits eclarizio/manageiq@fc78488~...71d857f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
13 files checked, 0 offenses detected
Everything looks fine. 🍰

@d-m-u
Copy link
Contributor

d-m-u commented Jun 28, 2018

Travis failure looks unrelated?

@JPrause
Copy link
Member

JPrause commented Jun 29, 2018

Can I have an update on where this PR stands. ETA on merge.

@gmcculloug gmcculloug merged commit e13f77d into ManageIQ:master Jun 29, 2018
@gmcculloug gmcculloug added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 29, 2018
simaishi pushed a commit that referenced this pull request Jun 29, 2018
Change reconfigure setup to include values configured with originally
(cherry picked from commit e13f77d)

https://bugzilla.redhat.com/show_bug.cgi?id=1591484
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit e33d35b833a01523280cdb17d049561753aadbc5
Author: Greg McCullough <[email protected]>
Date:   Fri Jun 29 08:49:55 2018 -0400

    Merge pull request #17647 from eclarizio/BZ1591484-Reconfigure
    
    Change reconfigure setup to include values configured with originally
    (cherry picked from commit e13f77de072210a43d1ca0a1d7b7e8885f5936ed)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1591484

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants