CRM-19151: Add the ability to merge memberships without data loss #11298
Conversation
|
Can one of the admins verify this patch? |
otetard
force-pushed
the
feature/merge_memberships
branch
from
6f7c576 to
ab0162f
Compare
|
I’ve just updated the pull request:
Olivier; |
|
Hey @otetard , Can you give me any use case to test this ? Because when I tested this against 4.7.27 and on merging two contacts their contributions from old contact were transferred to new contact without apply this patch. Which case are we handling here ?
|
|
Hi @agilewarealok, This patch tries to solve CRM-19151. This issue was also discussed on StackExchange:
With the current code, if you merge the two contact (without checking the “add new” checkbox), you will loose some information on the final membership: Let’s consider two contacts:
Each membership have an associated contribution (through If you merge “Contact A” in “Contact B”, the final membership will be “Contact A”’s one (start date will be 2005-01-01; end date will be 2006-12-31; status will be expired). All membership information from Contact B will be lost. You will also loss the relation ( |
|
@otetard Thanks for the information and use case, We will try to review this today. |
|
jenkins, ok to test |
otetard
force-pushed
the
feature/merge_memberships
branch
4 times, most recently
from
bace79a to
755c1fd
Compare
|
I’ve just fixed the PHP coding style. I’ll have a look at the failing test ( |
There was a problem hiding this comment.
@otetard We've tested the mentioned use case and it's working as expected.
Expired membership of Contact A is not merged into Contact B and the link between contribution and membership in Contact B is intact.
otetard
force-pushed
the
feature/merge_memberships
branch
5 times, most recently
from
092f2ab to
17a6ec9
Compare
otetard
changed the title
|
I’ve just updated my patches. This PR was successfully checked by Jenkins and I’ve fixed some issues in edge cases. This PR is now in a correct state so it can be merged. Olivier; |
otetard
force-pushed
the
feature/merge_memberships
branch
from
17a6ec9 to
d6fd830
Compare
|
Hi @agilewarealok, I’ve just rebased this PR on current I would be glad if someone could take a look at it and merge it (or tell me what should I change). Thanks! |
There was a problem hiding this comment.
Hi @otetard,
I've tested the PR and it seems it's not working for a case. I've tested it in following way,
- Create Contact A
- Add membership with Start Date 01/01/2005 and end date 31/12/2006
- Create Contact B
- Add membership with start date 01/01/2018 and end date 31/12/2019
- Merge Contact A into Contact B
Results:
- Membership and Contributions are deleted from Contact A.
- Membership in Contact B overrides with Member since 01/01/2005 Start date 01/01/2018 and end date as 31/12/2006 and status is expired.
- Contribution from previous membership is linked in above Membership.
I think end date should be 31/12/2019
Let me know if I'm missing something.
Thanks!
When calling for some custom processing function in the merge process, add tables that needs to be merged and related options.
Add a new custom processing function `mergeMemberships()` in `CRM_Member_BAO_Membership` that allows to merge memberships. General idea is to merge memberships in regards to their type. We move the other contact’s contributions to the main contact’s membership which has the same type (if any) and then we update membership to avoid loosing `join_date`, `end_date`, etc. Related issue: https://issues.civicrm.org/jira/browse/CRM-19151
otetard
force-pushed
the
feature/merge_memberships
branch
from
d6fd830 to
c31e68a
Compare
|
Hi @agilewarealok, Thanks for having tested this code. I’m sorry for not having tested enough my code… I’ve updated this PR, everything should work fine:
Olivier; |
There was a problem hiding this comment.
Hi @otetard
Thanks for the update. I've tested the PR again and it seems to be working as expected. Performed following steps to test
- Create Contact A
- Add membership with Start Date 01/01/2005 and end date 31/12/2006
- Create Contact B
- Add membership with start date 01/01/2018 and end date 31/12/2019
- Merge Contact A into Contact B
Results:
- Membership and Contributions are deleted from Contact A.
- Membership in Contact B overrides with Member since 01/01/2005 Start date 01/01/2018 and end date as 31/12/2019 and status is Current.
- Contribution from previous membership is linked in above Membership.
Thanks!
|
@eileenmcnaughton would you mind checking this one? It would be a good fix to get into CiviCRM. We've done our own due diligence on it. |
|
Thanks for your work on this one @otetard and @agilewarealok too :) |
|
So the main concerns I have are
|
|
Sure, just a heads up as described in the overview. The current implementation in CiviCRM for membership merging is broken. This is a fix, not a new feature. |
|
@jusfreeman to clarify - does this mean the memberships are moved across unchanged (which would be a fix I would feel I could merge without broader input) or that it merges them by joining 2 memberships together? |
|
I tested and this works as intended. I agree that the existing behavior was quite broken -- I generally set the "add new" to default enabled for clients and advise they never trust the membership merge for the reasons previously specified. @eileenmcnaughton I think this should be merged. There could be future discussion about enhancements/options to the membership merge behavior, but for now, this does fix/improve broken functionality. |
|
Ok - I'm happy to merge based on @lcdservices @agilewarealok & @jusfreeman - I don't really have my head in the space on this PR - but I feel that is enough endorsement. I skimmed the code & it looks generally good. WARNING - this could regress at any time unless someone adds a test! (api_v3_JobTest is where all the merge tests are) |
|
Also - are any of you able to confirm the status of this one after the merge ... https://issues.civicrm.org/jira/browse/CRM-21151?filter=24614 (and how good is it that it perfectly demonstrates my point about the dangers of fixing something without adding a test - how did I lower my guard!?!) |
|
Sorry I didn't have a chance to test this until just now @eileenmcnaughton but there are big problems with merging memberships that have custom fields and/or inherited memberships. It also seems to have trouble with identical memberships. I'm not sure if this PR is the source of the problem. I'm just concerned that this addresses the most glaring problems with merging memberships without solving more fundamental ones. With this change, you now get silent data loss rather than more obvious problems. Personally, I don't think membership merging is safe to use in production. I don't know whether it's good to merge this PR or not, but I don't think it's responsible to allow merging of memberships. At the very least, I'd like to see "Add New" as the default and a big warning about losing the "other" contact's membership data if you don't "Add New". Here are the details of what I tried out. For these tests, I created an integer custom field called "membership sticker number" and for the inherited memberships, I created a one-year rolling membership type called "corporate" that inherits along "employer of" relationships. Otherwise, this is from a totally clean Test 1: identical contacts with identical memberships
This outcome was a bit of a surprise, but it doesn't result in true data loss. Test 2: different membership dates
This is a serious problem. With no warning and potentially little opportunity to notice after the fact, I've deleted membership data while thinking I'm merging it into the combined output. The problem here of course is that there's no UI for selecting which of two conflicting values should be kept, but I feel that this outcome is unacceptable. Test 3: inherited members
This is also a problem: inherited memberships from the merged-onto contact have evaporated, contacts inheriting memberships from the to-be-deleted contact now have extra, uninherited memberships, and some custom data is only available on those uninherited memberships. I had originally planned on doing another test with inherited memberships where the membership type has a limit on the number of inherited memberships, but things are so messed up that I didn't think that would be worth trying until even the basics of inherited memberships are covered. |
|
Wow thanks for that great testing @agh1 - keen to hear other's thoughts |
|
@agh1 the PR scope and code does not include any handling of merging memberships that have custom fields and/or inherited memberships. It looks like all your test cases 1 - 3 are directly related to those points. This PR fixes a known problem as documented . Incremental fixes are good. Fixing merging of custom fields and inherited memberships should be separate PR(s) and build upon this code further. |
|
I don't think we can treat custom fields as being some kind of edge case. Incidentally, as best I can tell, fields on the membership itself like Source are also overwritten without warning when the membership is "merged". To be honest, I had never done a systematic test of merging memberships before yesterday; the main use case for me and our clients has always been merging one contact with a membership with another contact that has none. I've heard sporadic reports of trouble, but people have always resolved it manually case-by-case. I just know that the premise of this PR is to "merge memberships without data loss", and there definitely is data loss. This PR isn't causing the data loss, but I'm concerned that it does two things:
For an uninitiated user, merging contacts and having both memberships on the new contact side-by-side (the "add new" outcome) may be an irritating quirk, but merging contacts and arbitrarily keeping only one membership (albeit with the combined dates and payments) is what I'd consider a nasty surprise. I imagine there could be a variety of solutions to this, but the status quo, with and without this PR, is a problem waiting to happen. |
I agree with this. The merging tool is complex, but should primarily be merging contacts, not trying to merge their associated information. Otherwise, what about all the other related entities (event registrations for the same event, etc?) I agree it's not ideal, but I have reservations about attempting to automagically merge memberships during the merge process. Admittedly we do not use the membership element, so I do not have much background on the issues here. |
|
I haven't been following this in detail before, but it sounds it would be
better if this functionality could be implemented by extension. Then it is
not forced upon every user of CiviCRM and a sysadmin can decide whether the
functionality should be available. It would also allow for different merge
strategies to be applied by enabling/disabling extensions.
I'm not sure if there are hooks in place to support this at the moment
though.
…On 23 March 2018 at 14:40, John Kingsnorth ***@***.***> wrote:
For an uninitiated user, merging contacts and having both memberships on
the new contact side-by-side (the "add new" outcome) may be an irritating
quirk, but merging contacts and arbitrarily keeping only one membership
(albeit with the combined dates and payments) is what I'd consider a nasty
surprise.
I agree with this.
The merging tool is complex, but should primarily be merging contacts, not
trying to merge their associated information. Otherwise, what about all the
other related entities (event registrations for the same event, etc?)
I agree it's not ideal, but I have reservations about attempting to
automagically merge memberships during the merge process. Admittedly we do
not use the membership element, so I do not have much background on the
issues here.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#11298 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB9QQSrlbO36ymwv78itmYtIRiIDd7MBks5thQlugaJpZM4QjFkT>
.
|
|
I got thinking about typical use cases, and I did realize that there is one somewhat common use case among our clients for merging memberships: a contact with a lapsed membership from years ago merging with a contact who has a newer membership. I decided to test that out as well as the scenario where a contact has a status override set. Unfortunately, it appears that the date and status calculations that are introduced by this PR have some significant problems. Fundamentally, I think it stems from blindly accepting accepting values from the surviving membership and discarding values from the merged-in membership. Test 4: Lapsed member
The start date should be the start of the current continuous membership period, so it should be 3/23/18 rather than the join date. I tried this same situation again but merging the opposite direction, and the start date for the "merged" membership is 3/23/18. It appears that this PR just takes the value from the surviving membership record. Test 5: Status override
There is no record of there ever being a status override. I tried this the other way around, merging onto the contact with the status override, and the status override field is set to Override Permanently but the status is set to Expired. Unlike with start date, which appears to be taken as-is from the surviving membership, the status seems to always get wiped out. This makes for a dangerous combination with the status itself being changed, since in this case, the member would never be able to renew themselves into good standing without an admin noticing that they have status override set. Test 6: Member in good standing, back from the dead
It appears that even though the status override from membership ID 49 was ignored, the status was calculated based upon the end date brought in from that membership. |
|
Great job Andrew. Test #4 is the most common use case in my experience.
Also I agree that custom data fields for membership are not an edge case. Several clients use custom data fields for: badge number, family member names, etc.
… On Mar 23, 2018, at 8:16 AM, Andrew Hunt ***@***.***> wrote:
I got thinking about typical use cases, and I did realize that there is one somewhat common use case among our clients for merging memberships: a contact with a lapsed membership from years ago merging with a contact who has a newer membership.
I decided to test that out as well as the scenario where a contact has a status override set.
Unfortunately, it appears that the date and status calculations that are introduced by this PR have some significant problems. Fundamentally, I think it stems from blindly accepting accepting values from the surviving membership and discarding values from the merged-in membership.
Test 4: Lapsed member
Created contact ID 212, added general membership ID 41, which had join/end dates 3/23/15 - 3/22/17
Created contact ID 213, added general membership ID 42, which had join/end dates 3/23/18 - 3/22/20
Merged the contacts (cid=212&oid=213)
The combined contact ID 212 had general membership ID 41, which had a join date of 3/23/15, a start date of 3/23/15, and an end date of 3/22/20
The start date should be the start of the current continuous membership period, so it should be 3/23/18 rather than the join date.
I tried this same situation again but merging the opposite direction, and the start date for the "merged" membership is 3/23/18. It appears that this PR just takes the value from the surviving membership record.
Test 5: Status override
Created contact ID 214, added general membership ID 43, which had join/end dates 3/23/15 - 3/22/17, permanent status override, and status of Current
Created contact ID 216, added general membership ID 45, which had join/end dates 3/23/12 - 3/23/14
Merged the contacts (cid=216&oid=214)
The combined contact 216 had general membership ID 45, which had a join date of 3/23/12, a start date of 3/23/12, an end date of 3/22/17, no status override, and a status of Expired.
There is no record of there ever being a status override.
I tried this the other way around, merging onto the contact with the status override, and the status override field is set to Override Permanently but the status is set to Expired.
Unlike with start date, which appears to be taken as-is from the surviving membership, the status seems to always get wiped out. This makes for a dangerous combination with the status itself being changed, since in this case, the member would never be able to renew themselves into good standing without an admin noticing that they have status override set.
Test 6: Member in good standing, back from the dead
Created contact ID 219, added general membership ID 48, which had join/end dates 3/23/15 - 3/22/17
Created contact ID 220 with deceased date 3/1/18, added general membership ID 49, which had join/end dates 3/23/17 - 3/22/19, permanent status override, and a status of Deceased
Merged the contacts (cid=219&oid=220)
The combined contact 219 had general membership ID 48, which had join/end dates 3/23/15 - 3/22/19, no status override, and a status of Current
It appears that even though the status override from membership ID 49 was ignored, the status was calculated based upon the end date brought in from that membership.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@agh1 this is a great discussion, which highlights the need for further work in this area. Should we move it to Gitlab? Since this PR has been merged and it would be a waste for these thoughts to be lost in the void. |
|
@agh1 @Stoob @JKingsnorth is anyone proposing this should be reverted? Is anyone of the opinion that this PR makes things WORSE |
|
@eileenmcnaughton I'm on the fence. The PR introduces the status override problems in tests 5 and 6 above, but those pale in comparison to the amount of mayhem involved in merging memberships that is neither introduced nor resolved by this PR. I think what I really feel is that this PR should become moot because it affects a feature that shouldn't exist in anything like its current form. I think the one thing I'm sure of is that CRM-19151 should be reopened, which I just did, because it's clear that what it calls for is not complete. |
|
I just noticed that one of the items @seamuslee001 mentions in CRM-19151 is
As @Stoob said, membership custom data is often really important. Among the use cases for us are membership card numbers, the day when a CSA subscriber picks up food, and whether a member receives the journal in print or electronically. |
|
Note discussion here https://lab.civicrm.org/dev/core/issues/631 about removing merge membership functionality from core |
Overview
Adds the ability to merge memberships without losing data.
With the current code, if you merge the two contact (without checking the “add new” checkbox), you will loose some information on the final membership:
join_date,start_dateandend_datewill be overridden and the contributions from the final contact won’t be associated to the final membership.Related issue: https://issues.civicrm.org/jira/browse/CRM-19151
Before
Currently CiviCRM can’t merge memberships and can result in data loss (contributions).
After
Really merge memberships by moving related contributions to the final membership and move memberships data to the new one (if relevant).
General idea is to merge memberships in regards to their type. We move the other contact’s contributions to the main contact’s membership which has the same type (if any) and then we update membership to avoid loosing
join_date,end_date, etc.Technical Details
Add a new custom processing function
mergeMemberships()inCRM_Member_BAO_Membershipthat allows to merge memberships.Links
This patch solve CRM-19151. This issue was also discussed on StackExchange: