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

Export class code readability - Reduce passing of variable, define on class #12290

Merged
merged 1 commit into from
Jul 13, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jun 9, 2018

Overview

Code readability refactor towards being able to review & merge fixes to the Export functionality

Before

$contactRelationships passed from pillar to post

After

self::$relationshipTypes used instead

also self::$memberOfHouseholdRelationshipKey & $headOfHouseholdRelationshipKey defined & a function extracted in the test (this is towards adding second test but is added as incomplete at this stage as it passes on local mysql config but not CI config - which is in fact the bug we are working towards addressing)

Technical Details

This is one of series of small refactors aimed at getting the toxic code to a point where I feel comfortable reviewing the proposed changes & bug fix

Comments

@monishdeb

@civibot
Copy link

civibot bot commented Jun 9, 2018

(Standard links)

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb this is currently blocking further work on your export fixes

@monishdeb
Copy link
Member

@eileenmcnaughton earlier when I included the cleanup changes on my earlier PR #11901 and #11870 during rebase, it breaks the workflow and thus reflected on the added test failure. I need to revert some of the changes but still with no luck :( This is a very complex code and took me days to understand and fix the DB error.
I will first try to correct the errors on those 2 PRs and will review it.

@eileenmcnaughton
Copy link
Contributor Author

@monishdeb the thing is I pretty much concluded I was not going to be able to review your 2 Prs until we had massively simplified the code - ie. I think about 10 more PRs of this size need to be done & merged before we reach the point where we can take the logic of your PRs & fix the actual thing - I wasn't necessarily trying to keep the specific code changes in your PRs as I did this but to eventually enact the logic.

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

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

I've reviewed the changes on the export screen and while skimming through the code. +1 for the cleanup 👍

@eileenmcnaughton
Copy link
Contributor Author

merging based on @jitendrapurohit - thanks!

@eileenmcnaughton eileenmcnaughton merged commit 9be1987 into civicrm:master Jul 13, 2018
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.

4 participants