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.

Sign up for GitHub

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

Jump to bottom

Remove empty array associations created via UI from association list #16919

Merged
merged 1 commit into from
Feb 13, 2018
Merged

Remove empty array associations created via UI from association list #16919

merged 1 commit into from
Feb 13, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 31, 2018 •
edited
Loading

This check needs to be a present because it needs to remove associations whose values are merely an empty array, which present does but the existing code won't.

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 31, 2018

@miq-bot add_label cleanup
@miq-bot assign @gmcculloug

@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

Checked commit d-m-u@636173f with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

gmcculloug
gmcculloug reviewed Jan 31, 2018
View reviewed changes
app/models/dialog_import_validator.rb
@@ -57,7 +57,7 @@ def check_dialog_fields_for_validity(dialog_fields)

def check_dialog_associations_for_validity(dialog_fields)
associations = {}
dialog_fields.each { |df| associations.merge!(df["name"] => df["dialog_field_responders"]) unless df["dialog_field_responders"].nil? }
dialog_fields.each { |df| associations.merge!(df["name"] => df["dialog_field_responders"]) if df["dialog_field_responders"].present? }
unless associations.blank?
Copy link
Member

Choose a reason for hiding this comment

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

Rubocop is not complaining about the line you changed, but does about line 61 that you reverted back. 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, but I still think this needs to be a stricter check, that of presence, rather than the one that's existing, because the UI associations have blank arrays that need to be dealt with here.

Copy link
Member

Choose a reason for hiding this comment

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

What confused me is the PR is labeled as cleanup but I think you are really addressing a bug. I initially thought this was just a refactoring based on rubocop warnings. Re-reading the description it sounds like it is a bug.

@d-m-u
Copy link
Contributor Author

d-m-u commented Feb 1, 2018

@miq-bot remove_label cleanup
@miq-bot add_label bug

@miq-bot miq-bot added bug and removed cleanup labels Feb 1, 2018
@d-m-u d-m-u changed the title Clean up the presence checks Remove empty array associations created via UI from association list Feb 1, 2018
@gmcculloug gmcculloug merged commit bcc70a2 into ManageIQ:master Feb 13, 2018
@gmcculloug gmcculloug added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 13, 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

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

simaishi pushed a commit that referenced this pull request Mar 22, 2018
@gmcculloug @simaishi
Merge pull request #16919 from d-m-u/cleaning_up_import_stuff
760adf1 
Remove empty array associations created via UI from association list
(cherry picked from commit bcc70a2)

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

Gaprindashvili backport details:

$ git log -1
commit 760adf117b45eb76b2eb1defa1cadbead2512de6
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Feb 13 11:25:22 2018 -0500

    Merge pull request #16919 from d-m-u/cleaning_up_import_stuff
    
    Remove empty array associations created via UI from association list
    (cherry picked from commit bcc70a2774525f9742c5e3e2f855015393412797)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1559475

@d-m-u d-m-u deleted the cleaning_up_import_stuff branch October 26, 2018 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug left review comments

Assignees

@gmcculloug gmcculloug

Labels
bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@d-m-u @miq-bot @simaishi @gmcculloug