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

CRM-21773 Fix merging multi-value custom fields #11691

Merged
merged 3 commits into from
Mar 5, 2018

Conversation

JKingsnorth
Copy link
Contributor

@JKingsnorth JKingsnorth commented Feb 19, 2018

Overview

Multi-value custom fields are ignored during the merge process, when they should be merged provided we tick the box to move them during the process.

Before

Multi-value fields not migrated.

After

Multi-value fields are migrated if required.



@JKingsnorth
Copy link
Contributor Author

The test should FAIL - proving the issue.

@JKingsnorth
Copy link
Contributor Author

It failed, good: not ok 988 - Error: CRM_Dedupe_MergerTest::testMigrationOfSomeCustomDataOnEmptyCustomRecord

@JKingsnorth
Copy link
Contributor Author

Now, with the fix, it should pass.

@JKingsnorth
Copy link
Contributor Author

JKingsnorth commented Feb 19, 2018

@eileenmcnaughton I think this is failing because \CRM_Dedupe_Merger::relTables is being cached in a static variable (https://github.com/JKingsnorth/civicrm-core/blob/06f4969c149039c74940604b75d5f44e44de7bfc/CRM/Dedupe/Merger.php#L42), so it's not loading up the new group I'm creating during the test. Can you advise me on how to clear that static variable cache? I'm not sure it's even possible since the scope of the static variable is limited to the function itself? How do other unit tests get around these issues?

@eileenmcnaughton
Copy link
Contributor

@JKingsnorth search for Civi::statics - that is a static var that is cleared between test runs & can be cleared from outside the function

@JKingsnorth
Copy link
Contributor Author

Thanks @eileenmcnaughton - I'll try that out.

@JKingsnorth JKingsnorth changed the title CRM-21773 Test for merging multi-value custom fields [WIP] CRM-21773 Test for merging multi-value custom fields Feb 22, 2018
@JKingsnorth JKingsnorth changed the title [WIP] CRM-21773 Test for merging multi-value custom fields CRM-21773 Test for merging multi-value custom fields Feb 23, 2018
@JKingsnorth
Copy link
Contributor Author

JKingsnorth commented Feb 23, 2018

@eileenmcnaughton @systopia This is ready for review now! I had to refactor the static variable caching to get the tests to work. Thanks for the tip Eileen.

I confirm that I have reviewed @systopia 's patch, and it resolves the issue. If someone could check my unit test then this should be good to merge.

@JKingsnorth JKingsnorth changed the title CRM-21773 Test for merging multi-value custom fields CRM-21773 Fix merging multi-value custom fields Feb 23, 2018
@eileenmcnaughton
Copy link
Contributor

(CiviCRM Review Template WORD-1.1)

  • (r-explain) Pass
  • (r-test) Pass - added by John
  • (r-code) Pass, actual change is simple. Caching change is positive
  • (r-doc) Pass - not required
  • (r-maint) Pass - mild code improvement
  • (r-run)Pass - John tested:
  • (r-user) Pass:
  • (r-tech) Pass - trivial change:

@eileenmcnaughton eileenmcnaughton merged commit 7fba879 into civicrm:master Mar 5, 2018
@eileenmcnaughton
Copy link
Contributor

thanks @JKingsnorth & @systopia

@JKingsnorth
Copy link
Contributor Author

Thanks all.

@bjendres
Copy link
Contributor

bjendres commented Mar 5, 2018

Thanks @eileenmcnaughton and @JKingsnorth !

@mlutfy mlutfy added this to the 4.7.32 milestone Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants