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

dev/core#1120 remove multiple export handling #14830

Merged
merged 1 commit into from Jul 16, 2019

Conversation

@eileenmcnaughton
Copy link
Contributor

commented Jul 15, 2019

Overview

Per https://lab.civicrm.org/dev/core/issues/1120 I'm not convinced the current merge code does 'what it says on the box'

It implies that both the drop down option or the 'other' string only kick in with > 2 contacts with the same address. In fact it seems that they both kick in with any merge but there is code that attempts to only apply the 'other' string for 2 or more (I'm not sure it succeeds since I wrote a unit test before I moved that code around & it seemed to show the 'other' string being used for 2 contacts). This simplifies to doing more or less what the code seems to do.

Before

Screen Shot 2019-07-16 at 9 32 04 AM

After

Screen Shot 2019-07-16 at 9 45 13 AM

Technical Details

Comments

More discussion in https://lab.civicrm.org/dev/core/issues/1120 but I think at best it applies the 2 or more rule to the 'other' rather than the variants and 'other'

@lcdservices @Stoob @agh1 @colemanw @MegaphoneJon @petednz thoughts? I

@civibot

This comment has been minimized.

Copy link

commented Jul 15, 2019

(Standard links)

@civibot civibot bot added the master label Jul 15, 2019

@eileenmcnaughton eileenmcnaughton force-pushed the eileenmcnaughton:export branch from 4e6f2b8 to ee769c0 Jul 15, 2019

@MegaphoneJon

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

As far as I'm concerned, this is "concept approved". The behavior you're describing matches my recollection of how this works, and is preferable to the stated behavior.

@lcdservices

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

It's not clear to me how this alters existing functionality. I agree that the terminology ">2" is confusing, and I don't think it was intended to make a distinction between merging two contacts and merging more than two contacts -- it was just poorly worded, and your change is more accurate to the intent. But beyond the terminology change, it's not clear what the code changes accomplish. Are you saying that the code did make a distinction between merging 2 vs. merging > 2? And this simplifies to any merge? If so -- I'm in favor.

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@lcdservices the code definitely attempted to treat > 2 differently than just 2. I'm not sure how well that worked since it only attempted it for the 'other' string - not the list or the option value and I have some doubts even then since I'm pretty sure we merged this test

public function testMergeSameAddressGreetingOptions() {
before making any changes to the code, so I suspect the removed code didn't actually work properly anyway

@eileenmcnaughton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2019

@colemanw this seems to be getting a pretty clear cut response - are you OK to merge

@colemanw

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

I'm all for simplification, esp when it passes tests and community approval :)

@colemanw colemanw merged commit 290463a into civicrm:master Jul 16, 2019

1 check passed

default Build finished.
Details

@eileenmcnaughton eileenmcnaughton deleted the eileenmcnaughton:export branch Jul 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.