dev/core#631 - Enable 'add new' by default on merge screen

[jitendrapurohit]

Overview

Enable 'add new' checkbox on merge screen by default

Before

Add new is not selected by default which does not move the membership to the main contact.

After

'add new' is enabled by default if main contact does not contain any membership similar to other contact.

See complete explanation at https://lab.civicrm.org/dev/core/issues/631#note_13435

Comments

Gitlab - https://lab.civicrm.org/dev/core/issues/631

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@agh1 @agileware @Stoob I remember the last change to membership merging was pretty contentious & was not necessarily agreed as an improvement by all

[agh1]

Yeah, if I recall correctly, the problem was all if you don't "add new" there's a rudimentary attempt to merge the membership dates, but the other data on the left-hand contact's membership is lost. "Add new" is definitely the safer way to go.

[lcdservices]

strong +1 on this. I frequently set this as a default via extension, but definitely think it should be the core default. if it's not selected, there is a risk of unintended data loss (as it attempts to "merge" the memberships, but I rarely find the algorithm used for that process to be what people expect).

[seamuslee001]

I am a +1 on this. AUG have actually done this but through an extension and would be good to have it in core

[eileenmcnaughton]

Looks like the best starting point for a test would be

api_v3_JobTest::testMerge

@jitendrapurohit are you able to add a test? What actually happens in headless mode?.... Does this change headless behaviour? Or only via the form

[jusfreeman]

+1 from me on this one.

[jitendrapurohit]

As the changes here only modifies the $elements variable which is used to display the inputs on the form, this only affects the process executed via the form. I've added a unit test which asserts the addition of this attribute and also checks if membership was migrated correctly to the main contact. @eileenmcnaughton

Thanks.

[jitendrapurohit]

jenkins, retest this please

[francescbassas]

+1 to enable "add new" to default. It has always generated confusion for us this option. It would be helpful if a help pop up explains clearly what happens if you select "move related" checkbox and "add new" checkbox (when a membership can be overwriten or added).

[mattwire]

It would be helpful if a help pop up explains clearly what happens if you select "move related" checkbox and "add new" checkbox

Yes!

[eileenmcnaughton]

People are all commenting in favour - let's merge this in a couple of days if not more input.

I understood the gitlab commenters to be in support of this but @petednz read it the other way - Pete commented there to try to get some clarity

[eileenmcnaughton]

So it seems the gitlab posters are arguing that this checkbox should go all together rather than the default changing

[lcdservices]

Personally, I wouldn't be opposed to that. But that might deserve more time for community input.

[agh1]

Cross-posting from GitLab: see this comment and following on #11298 (comment) for some workflows that get really messed up when memberships are "merged". I'm with the others on GitLab--I think memberships should be copied over and the user told to merge manually.

[eileenmcnaughton]

Ok - so my take is that

1. there is a general consensus this is an improvement
2. a number of people think we should go further and entirely remove membership merging

If people agree with the above I will merge this & we can continue to consider going further.

My personal take on it is that membership merging seems to be a niche need & perhaps would be best out of code with the maintenance burden falling on those who have the need.

There are a few chunks of functionality in core like that & the question is how do we remove them - just remove them & tell people who want them to build their own? exacerbate the maintenance problem by adding a setting? put in the work ourselves to convert to an extension - my recent gitlab on 'given that calculating mean & mode slows down contribution searches by a significant multiplier and no-one seems to care about them should they be in core' is a similar type of issue.

Anyway, please give this a thumbs up if you specifically support merging this PR (then you can use the comments to talk about whether & how we would remove the membership merge option)