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

Fix for Default service dialog values not included in EVM when 'refresh_dialog_fields' action invoked #19005

Merged
merged 4 commits into from Jul 19, 2019

Conversation

eclarizio
Copy link
Member

@eclarizio eclarizio commented Jul 18, 2019

When calling the API directly, the refresh option gets set to true, and ends up making a call to load_values_into_fields. This is fine if the API call provides the values that the field should be refreshed with, but without the values, customer was running into an issue where they had a few fields that were static and not visible to the user (so therefore just automatically filled in and ready to go) that they did not pass in to the API call in 5.9.6.5. On updating to 5.10.6.1, I believe that #17329 made the change to attempt to eliminate automate calls that were happening when calling refresh that didn't need to happen, but also this eliminated the "call" to get the static field values. Again, this is fine if the API call provides these static values in the POST request (which is what the UI does), but direct use of the API does not necessarily inform the user that they should do that.

For @tinaafitz and @gtanzillo (and anyone else) to review: it should be easier to track the "main" change if you go commit by commit, since the first commit contains the "meat" of the change, the second commit just moves the #load_dialog method into the private namespace since it's not used elsewhere and separates out the block of if/else logic into another method.

@miq-bot add_reviewer @tinaafitz
@miq-bot add_reviewer @gtanzillo
@miq-bot add_label bug

@tinaafitz My apologies but I've been out of the cloudforms loop for a bit, so please add any other labels hammer/yes, hammer/no (?) that this might need.

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

@miq-bot
Copy link
Member

miq-bot commented Jul 18, 2019

Checked commits eclarizio/manageiq@eb51c01~...7298e4f with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

@eclarizio
Copy link
Member Author

@tinaafitz @gtanzillo I had to update the code a bit after testing on the appliance. Basically what happened was if you didn't pass in parameters, it went and used my new code that set the static values beautifully....but then overwrote them with nils because you didn't pass in parameters, ending up with exactly the same response as before. D'oh.

So instead, we pass false to the load_values_into_fields call, which used to be the overwrite parameter, but after looking at it more closely that's not really a good name, since while it does "overwrite" the value that a field potentially already has, it really only needs to be doing that if it's not nil.

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

👍 Looks good. After discussion with @tinaafitz, @abellotti, @eclarizio and @d-m-u we decided that this was the best option to address the reported issue

Copy link
Contributor

@d-m-u d-m-u left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@eclarizio Thanks so much for your work on this PR.

@gtanzillo
Copy link
Member

@eclarizio Are you or @billfitzgerald0120 still planning on testing this manually? I was planning on merging after that.

@gtanzillo
Copy link
Member

Sorry @eclarizio, @billfitzgerald0120 missed the comment above. Thanks! I'll merge 👍

@gtanzillo gtanzillo added this to the Sprint 116 Ending Jul 22, 2019 milestone Jul 19, 2019
@gtanzillo gtanzillo merged commit a8a06f8 into ManageIQ:master Jul 19, 2019
@simaishi
Copy link
Contributor

hammer/yes ?

simaishi pushed a commit that referenced this pull request Jul 22, 2019
Fix for Default service dialog values not included in EVM when 'refresh_dialog_fields' action invoked

(cherry picked from commit a8a06f8)

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

Hammer backport details:

$ git log -1
commit 6817c3365b1fb371be1b4ec5fa0d9bc232727daf
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Fri Jul 19 11:30:47 2019 -0400

    Merge pull request #19005 from eclarizio/BZ1730813-CiscoAPIFix
    
    Fix for Default service dialog values not included in EVM when 'refresh_dialog_fields' action invoked
    
    (cherry picked from commit a8a06f8826660553e00ec3ff49f0c8bad4f54681)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1731977

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.

None yet

6 participants