Extract code for getting additional return properties, test #12505
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
export_fix_return_order
branch
from
12d86a5
to
0233dd8
Compare
|
@eileenmcnaughton There is a related warning about the return statement removal in Activity.php file. |
|
@jitendrapurohit ug. Actually I have a couple more commits locally. I might add them in. It will be more change so harder to review but less testing overall - you prefer I add before you review? I've extracted more of the getReturnProperties & added more tests |
|
hmm, yes, would like to review it as a whole 👍 |
|
@jitendrapurohit OK - I'm gonna be chipping away for a while - but I'll grow this PR a bit |
eileenmcnaughton
force-pushed
the
export_fix_return_order
branch
3 times, most recently
from
01335b4
to
a2cf7b1
Compare
|
@jitendrapurohit I added some more commits. It looks like a lot of code change now but each commit should be self-contained. This feels like a sensible 'set' because all the changes relate to setting $returnProperties when $fields is NULL (ie. export 'Primary fields') rather than specifying the output fields |
|
In my defence - there are 430 extra test lines - the score is not fair |
eileenmcnaughton
force-pushed
the
export_fix_return_order
branch
2 times, most recently
from
9744247
to
97bb8d1
Compare
|
@jitendrapurohit I added two more commits - but it's 600 lines of unit tests & 3 lines of code removal da6faba - to fix an enotice blocking the test - pretty sure the removal would not have a negative impact & the 'case' var would never be set as the $queryFields comes from the query object & is very standard |
|
@eileenmcnaughton Tested on all the export components, contacts, membership, activities, cases, etc and the functionality looks correct. Also tested by inserting some debug statements and checked the values of different variables($returnProperties, etc) before and after the changes and they seem fine in all cases. Can conclude that this is safe to be merged 👍 |
Fix for test fail when full suite run, make sure Case is enabled, fix params
…inconsisten results from this function during testing
Code tidy up on payent fields var
eileenmcnaughton
force-pushed
the
export_fix_return_order
branch
from
97bb8d1
to
944ed38
Compare
|
Added merge on pass based on @jitendrapurohit testing - thanks heaps! Just rebased it so will check tests before merging |
Overview
Extract code for getting additional return properties, test
Before
Code jumbled
After
Code less jumbled
Technical Details
@jitendrapurohit this should address the re-ordering issue you spotted.
Comments
Anything else you would like the reviewer to note