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

Add circular reference association check to import for ui update #16918

Merged
merged 2 commits into from
Feb 22, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 31, 2018

Dialog field associations are checked for circular references on dialog import but this check needs to also be included in the main dialog validation on save for association creation via the classic ui.

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 31, 2018

@miq-bot assign @gmcculloug

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 31, 2018

@eclarizio @bdunne can ya'll 👀 at this please for me, por favor?

@d-m-u d-m-u force-pushed the adding_circ_ref_check_to_import branch from 6c6e8e0 to f28b5b1 Compare January 31, 2018 14:23
@d-m-u d-m-u force-pushed the adding_circ_ref_check_to_import branch from f28b5b1 to 80a4756 Compare January 31, 2018 14:39
@@ -115,7 +123,7 @@ def build_association_list(dialog)
dialog["dialog_tabs"].flat_map do |tab|
tab["dialog_groups"].flat_map do |group|
group["dialog_fields"].flat_map do |field|
associations << { field["name"] => field["dialog_field_responders"] } unless field["dialog_field_responders"].nil?
associations << { field["name"] => field["dialog_field_responders"] } if field["dialog_field_responders"].present?
Copy link
Member

Choose a reason for hiding this comment

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

if field["dialog_field_responders"] is equivalent to unless field["dialog_field_responders"].nil?. present? changes that a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we want present, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Save via the UI results in there being empty arrays if there aren't associations and we don't want them in the association list if they're empty.

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.

Spec? This would help prove/disprove the necessity of the .present? vs. just the .nil?

@d-m-u d-m-u force-pushed the adding_circ_ref_check_to_import branch from 09648f6 to 06a3498 Compare January 31, 2018 20:27
@d-m-u d-m-u force-pushed the adding_circ_ref_check_to_import branch from 06a3498 to 0aaa8e5 Compare January 31, 2018 20:29
@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

Checked commits d-m-u/manageiq@80a4756~...0aaa8e5 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug gmcculloug merged commit 4429472 into ManageIQ:master Feb 22, 2018
@gmcculloug gmcculloug added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 22, 2018
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

@d-m-u Please add BZ link.

@d-m-u
Copy link
Contributor Author

d-m-u commented Mar 22, 2018

simaishi pushed a commit that referenced this pull request Mar 22, 2018
Add circular reference association check to import for ui update
(cherry picked from commit 4429472)

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

Gaprindashvili backport details:

$ git log -1
commit 25c48cd61cf987fcbdf40607c7d1019e312b6983
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Feb 22 13:37:40 2018 -0500

    Merge pull request #16918 from d-m-u/adding_circ_ref_check_to_import
    
    Add circular reference association check to import for ui update
    (cherry picked from commit 4429472038ea4876c7ff79887cad2c94ccc3b4f9)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1559483

@d-m-u d-m-u deleted the adding_circ_ref_check_to_import branch October 26, 2018 18:12
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.

6 participants