Fix CRM-21832 - Recurring activities don't carry over custom datas & add test provided by Agileware #14183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(Standard links)
|
eileenmcnaughton
changed the title
eileenmcnaughton
commented
May 2, 2019
| if (!empty($newData['custom'])) { | ||
| CRM_Core_BAO_CustomValueTable::store($newData['custom'], $newObject::getTableName(), $newObject->id); | ||
| } | ||
| $newObject->copyCustomFields($object->id, $newObject->id); |
There was a problem hiding this comment.
this is the actual change
|
@eileenmcnaughton sure, will look at this |
|
should we also remove https://github.com/civicrm/civicrm-core/blob/master/CRM/Event/BAO/Event.php#L989 since there is already a call to copyGeneric which in turn is calling copyCustomFields resulting in duplicate calls. |
Per civicrm#13470 custom fields are inconsistently copied where copying entities. This makes the code from BAO_Event called from the genericCopy function. I did a bit of an audit and the places where this is currently called from don't appear to call the copyGeneric function with the 'custom' param that would have activated the old code. I also consistently removed the & when it was being called so I could take it out of the signature. The original PR handled tags as well, but not in a generic way. I've left that out of scope but the test is present, commented out, so it would be easy enough to revist
|
@yashodha yes - you are right! I have removed it now. Thanks |
|
@eileenmcnaughton looks good and can be merged. |
|
Thanks @yashodha @agilewarealok we got the custom field part of this work by you merged |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Fixes a bug where custom fields are not copied over when copying an entity. This is in order to resolve #13470 which had a useful test. I've left the part about the tags out of scope & commented it out in the test but it would be easy now for someone to pick it up
Before
Copying most entities does not copy custom fields
After
Generic code to copy custom fields in copyGeneric
Technical Details
Per #13470 custom fields are
inconsistently copied where copying entities. This makes the code from
BAO_Event called from the genericCopy function.
I did a bit of an audit and the places where this is currently called from don't appear
to call the copyGeneric function with the 'custom' param that would have activated the old
code. I also consistently removed the & when it was being called so I could take it
out of the signature.
The original PR handled tags as well, but not in a generic way. I've left that out of scope
but the test is present, commented out, so it would be easy enough to revist
After some grepping I deprecated a function
Comments
#13470 seemed irrevocably stalled but @mattwire had left pretty clear guidance in his reviewer comment so I just did his step one & it all kinda fell into place. I didn't use the original code from that PR but I DID use the unit test which was the part of the PR I was keen not to lose
@yashodha perhaps you could review & merge this?