Correctly recurse over nested field associations

[d-m-u]

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

This originally caught (and returned) only the first of multiple nested fields that are linked via the dialog associations:
[fieldname_being_triggered, associations[fieldname_being_triggered].first]
and that's wrong, see the test case which was distilled straight from the bug.

[d-m-u]

@miq-bot add_label bug
@miq-bot assign @tinaafitz
@miq-bot add_reviewer @eclarizio

[d-m-u]

@miq-bot add_label hammer/yes

[miq-bot]

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

[eclarizio]

👍

[eclarizio]

I don't know if this is useful, but maybe we extract path & associations[fieldname_being_triggered] into a local variable? & is already pretty performant as far as I know, so even if both of the arrays are large that shouldn't be a big deal, it would really just be to understand exactly what path & associations[fieldname_being_triggered] represents.

It's not necessary though, I'm going to approve regardless.

[d-m-u]

@bdunne any chance i could also con you into merging this one, please?

[d-m-u]

can i get someone to review this or whatever again please

[eclarizio]

@d-m-u Did something else change? I don't see anything that looks out of place. 👍

[d-m-u]

sorry, Erik, nothing changed, I just, no one other than you's looking at this and I'd like to get it in

[d-m-u]

@bdunne please review

[d-m-u]

@tinaafitz please review

[d-m-u]

hey @bdunne I'm getting asked to speed this one up, could you please review?

[bdunne]

👍 LGTM. This looks much simpler. Thanks for taking this on!

[bdunne]

@eclarizio can you re-review?

[eclarizio]

All of your it block names seem kind of weird, they don't really describe what the test is testing (it "false" and then the test is ensuring the return value is nil? or that it doesn't blow up? You have two it "trivial case" blocks, one of them is expecting a nil return and the other is expecting an error), but as far as the actual code goes, it looks good to me.

Is there a reason we got rid of the specs in dialog_import_validator_spec.rb around the circular reference stuff though?

[d-m-u]

Sure, I mean, since I'm not raising in the import validator any longer, the dialog_field_association_validator test cases handle the error stuff and I'm not sure what the import validator is supposed to be testing now.

[eclarizio]

Sure, I mean, since I'm not raising in the import validator any longer, the dialog_field_association_validator test cases handle the error stuff and I'm not sure what the import validator is supposed to be testing now.

True, the error handling isn't there anymore, but right now you could comment out the entire line that delegates to the circular reference checker and the tests would all still pass because nothing is ensuring we go in there, I think.

I think either a spec should be added to ensure work is being delegated to it, or maybe you feed it a circular reference and allow it to run through and bubble up expecting the error.

[d-m-u]

Could I get you all to look again at this, please? It's subject to escalation at the moment

[d-m-u]

@lfu @bdunne @jrafanie @eclarizio
I don't know who else to ask for review but this needs in pronto

[simaishi]

Hammer backport details:

$ git log -1
commit 8bffceb80bcd559bd723b127965f340b5a7e299b
Author: Brandon Dunne <[email protected]>
Date:   Fri Jun 28 16:03:23 2019 -0400

    Merge pull request #18890 from d-m-u/fixing_field_association_recursion
    
    Correctly recurse over nested field associations
    
    (cherry picked from commit b7f78ba0696d2d7f60d1175727276b8cd2c3cb27)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1725894