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 dialog import to only use auto_refresh if new triggers are blank #17363

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Apr 27, 2018

The old field associations still exist and are used to create the dialog associations on import. Because you can't touch the auto_refresh in the UI anymore but they still live in db and exist when we export dialogs. So if we have new associations we should use them and only them, not both the old associations and the new ones.

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 27, 2018

@eclarizio can you 👀 for me please?

@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 27, 2018

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Apr 27, 2018
Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

Code changes look good overall, can you adjust the specs so that there is one for the path when the new_associations are blank and we do still need to build the old associations and one for the path where the new_associations are sufficient?

association_list = (associations_to_be_created + old_associations).reject(&:blank?)
build_associations(new_or_existing_dialog, association_list)
association_list = new_associations.reject(&:blank?).present? ? new_associations : build_old_association_list(new_or_existing_dialog.dialog_fields).flatten
binding.pry
Copy link
Member

Choose a reason for hiding this comment

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

You might not want this here anymore :)

@d-m-u d-m-u force-pushed the change_import_field_responders_to_not_use_old_if_new_exist_on_dialog branch 2 times, most recently from 72a8755 to 9e1af4d Compare April 27, 2018 18:17
…e nonexistent.

If using a new dialog, the DialogFieldAssociations should be what we're using
@d-m-u d-m-u force-pushed the change_import_field_responders_to_not_use_old_if_new_exist_on_dialog branch from 9e1af4d to b1650e5 Compare April 27, 2018 18:29
@miq-bot
Copy link
Member

miq-bot commented Apr 27, 2018

Checked commit d-m-u@b1650e5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@d-m-u
Copy link
Contributor Author

d-m-u commented Apr 27, 2018

@gmcculloug

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

👍 LGTM, thanks!

@gmcculloug gmcculloug merged commit f8e4a47 into ManageIQ:master Apr 27, 2018
@gmcculloug gmcculloug added this to the Sprint 85 Ending May 7, 2018 milestone Apr 27, 2018
@d-m-u d-m-u deleted the change_import_field_responders_to_not_use_old_if_new_exist_on_dialog branch April 27, 2018 20:10
simaishi pushed a commit that referenced this pull request Apr 30, 2018
…o_not_use_old_if_new_exist_on_dialog

Change dialog import to only use auto_refresh if new triggers are blank
(cherry picked from commit f8e4a47)

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

Gaprindashvili backport details:

$ git log -1
commit 0e25cfde04a3974b3e0eb05a4e2ce90725ae45cf
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Fri Apr 27 16:07:21 2018 -0400

    Merge pull request #17363 from d-m-u/change_import_field_responders_to_not_use_old_if_new_exist_on_dialog
    
    Change dialog import to only use auto_refresh if new triggers are blank
    (cherry picked from commit f8e4a47560df6af862e0b12a97f6caf364dfd09b)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1573254

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