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

Dialog field loading/refresh refactor to fix automate delays #17329

Merged

Conversation

eclarizio
Copy link
Member

@eclarizio eclarizio commented Apr 23, 2018

This PR is intended to fix an issue where the automate methods for dialog fields were running multiple times for seemingly no reason. Upon loading of a dialog (via custom button click or service catalog order, for example), sometimes dialog fields would also run in a seemingly random order. We figured out that due to the way the #value method was written for some of the dialog field types, those would call the automate method first, before the method #initialize_with_values was called.

However, #initialize_with_values was kind of misnamed, as it was really more about just establishing values for the context in the ResourceActionWorkflow. So, I've renamed that method and adjusted the logic around when it gets called.

Another large change is that the DialogFieldSerializer was making a call to the automate methods because if a field was dynamic it would try to get the values for that field. However, I don't think this should be the job of the serializer, it should simply serialize what it's given. So if something needs to update, it now does so before calling into the serializer.

For a refresh, #initialize_with_values was closer to the correct name of describing what needed to be done, but I also added #load_values_into_fields for when the dialog needs to refresh. Note that the logic to go into this method depends upon an option that gets passed in from this API PR.

This PR touches all of the DialogField types and the Dialog model itself, as well as the ResourceActionWorkflow, which are (obviously) how every single dialog do their thing. Therefore, this PR should undergo a much more rigorous test before merging. QE is involved in helping test this PR as well.

Needs ManageIQ/manageiq-api#365 to be merged first.

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

@eclarizio eclarizio force-pushed the dialog_field_dynamic_loading_sev2 branch from 0bc3b9d to 9c7dadf Compare April 23, 2018 07:15
@eclarizio
Copy link
Member Author

@miq-bot add_label wip

@gmcculloug
Copy link
Member

@d-m-u Please review

@eclarizio eclarizio force-pushed the dialog_field_dynamic_loading_sev2 branch 2 times, most recently from 691582b to df6c73e Compare April 25, 2018 19:21
@eclarizio eclarizio force-pushed the dialog_field_dynamic_loading_sev2 branch from df6c73e to dac46a2 Compare April 25, 2018 20:13
@miq-bot
Copy link
Member

miq-bot commented Apr 25, 2018

Checked commits eclarizio/manageiq@956d9b5~...dac46a2 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
18 files checked, 0 offenses detected
Everything looks fine. 🍪

@eclarizio
Copy link
Member Author

@miq-bot remove_label wip

Un-WIP-ing and updated the top comment with more context. Would be nice to have multiple reviews on this as well!

@miq-bot miq-bot changed the title [WIP] Dialog field loading/refresh refactor to fix automate delays Dialog field loading/refresh refactor to fix automate delays Apr 26, 2018
@miq-bot miq-bot removed the wip label Apr 26, 2018
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 Looks good.

@gmcculloug
Copy link
Member

We have been testing heavily on this PR and at this point not seeing issues with the automate methods running extra runs. Nice performance improvements overall. 👍

@gmcculloug gmcculloug merged commit c719d7f into ManageIQ:master Apr 26, 2018
@gmcculloug gmcculloug added this to the Sprint 85 Ending May 7, 2018 milestone Apr 26, 2018
simaishi pushed a commit that referenced this pull request Apr 27, 2018
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 8545d9a074ed9b93d2ac726fd20a66428fd934fc
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Apr 26 19:19:54 2018 -0400

    Merge pull request #17329 from eclarizio/dialog_field_dynamic_loading_sev2
    
    Dialog field loading/refresh refactor to fix automate delays
    (cherry picked from commit c719d7f3867ec1c70a7d6dda92149b3e45eab093)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1572716
    https://bugzilla.redhat.com/show_bug.cgi?id=1572711

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.

5 participants