Export code cleanup: Convert relationships to pseudonames at the point of loading

[eileenmcnaughton]

Overview

Minor code cleanup

Before

Code messier

After

Code less messy

Technical Details

The convertToPseudoName function is called on each instance of the DAO loaded from the relationship query. It makes sense to do this when it is first loaded rather than to pass it deep into the parent function & then do it.

Note we removed the clone action from the main loop a while back -ie. we iterate through a DAO for main export properties & later for relationship properties. We got no issues from that, which feels it was higher risk than doing the same in the secondary loop.

Comments

Testing should be done in conjunction with #12579 which is more complicated but will require the same sorts of UI tests

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[jitendrapurohit]

Seems to break the relationship export. It works fine without this changes. To replicate -

• Search Parent of relationship from Advanced Search.
• Export the results
• Select field to export as follows

• Exported CSV has the same parent first and last name for all the rows listed.

[eileenmcnaughton]

Thanks @jitendrapurohit - once I've sorted out #12579 I'l add a test to capture that & try to fix it

[twomice]

@eileenmcnaughton Looks like this PR is part of a larger code cleanup, initiated by efforts at fixing CRM-20632, which is an issue we're facing now. Anything I could do to help this along? (I suspect the complexity is such that adding more manpower doesn't help, but just in case ...)

[eileenmcnaughton]

@twomice I stopped progressing it because I was struggling to get the parts reviewed - the current PR that is wanting review is this one #12672 & once that is reviewed & merged I'll put the next chunk up - so yes, you could definitely help progress this!

[colemanw]

@eileenmcnaughton time for the next chunk.

[twomice]

Jenkins test this please.

[twomice]

@eileenmcnaughton Having trouble following the PRs here; is there another one that's awaiting review on the way to resolving CRM-20623?

[eileenmcnaughton]

@twomice this probably needs more work on this one - but if you are ready to do some more review I'll revisit it and fix the issue above + add test, or replace this

[twomice]

@eileenmcnaughton yes I've got time for a review or two.

[eileenmcnaughton]

@twomice @jitendrapurohit I was able to capture the issue as described by @jitendrapurohit in a unit test & address it.

I've updated the PR with those fixes - it did grow it a bit but it's definitely moving in the right direction. I have some more fixes locally that build on this that I will do as the next PR

[eileenmcnaughton]

test this please

[jitendrapurohit]

The failing test is the one added in this PR and it fails on my local too.

[eileenmcnaughton]

@jitendrapurohit it seems I added a check to prove 'is_deleted' was merged in the output -but actually it never is because the relationship is skipped if the household is deleted - so I removed that & I think it should pass now

[jitendrapurohit]

Confirming that the issue mentioned in my above comment is fixed. Also checked the exports related to different components and relationships. They seem to work fine after the change 👍

[eileenmcnaughton]

thanks @jitendrapurohit

[twomice]

Thanks @jitendrapurohit !

[twomice]

@eileenmcnaughton

I have some more fixes locally that build on this that I will do as the next PR

Please ping me on subsequent PRs related to this (really, related to CRM-20632); still glad to review as needed. Thanks!

[eileenmcnaughton]

@twomice can you put those up onto github - perbhaps a WIP PR. I'm really curious as to whether it will look like the changes I was imagining next

[twomice]

@eileenmcnaughton I don't have any code to offer at this point, was just quoting you where you said "I have some more fixes locally that build on this that I will do as the next PR" in a comment above (#12586 (comment)), and offering to review the next PR(s) if/when you put them up.

[eileenmcnaughton]

@twomice oh I misread

[eileenmcnaughton]

I think I'm going to fix up the handling of phonetypes yet - I think you can see if you look at the code now we are almost at the point where we can just array merge in php & not write the household stuff to temp table at all