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

Make sure inherited memberships are not themselves inherited #15114

Merged
merged 1 commit into from
Aug 29, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fixes weird scenario (encountered in tests, maybe not in the wild) whereby an inherited membership can be itself inherited

This scenario was created in the test setup (probably by accident) but it seems highly problematic - as shown in #15062 - the code is already pretty problematic.

Before

An inherited membership can itself be inherited

After

Inherited memberships are not inherited

Technical Details

In trying to make sense of the code / fix / test for #14410 (& isn't current spin off #15062)

I discovered the tests wouldn't pass due to a weird edge case where an individual inherited a membership and that membership was inherited in turn via a relationship the individual had (with the same organization)

Comments

@agh1 thoughts ? @agileware-pengyi fyi

Will rebase once extraction is merged

@civibot
Copy link

civibot bot commented Aug 23, 2019

(Standard links)

@civibot civibot bot added the master label Aug 23, 2019
@agh1
Copy link
Contributor

agh1 commented Aug 23, 2019

I'm not sure I follow what this PR is attempting to do, but we need to make sure the following scenario continues to work:

  • "Family Membership" is a membership type inherited by "Household Member of" and "Household Member is"
  • Two individuals, "Alice" and "Bob", each have household member relationships to "Alice and Bob Household"
  • Alice signs up for a Family Membership
  • The membership should be inherited by Alice and Bob Household and Bob

I was skeptical when this "feature" started happening years ago in conjunction with allowing multiple relationship types on a membership type, but lots of organizations rely on it, especially with both directions of a relationship type to an organization or household.

Here's another, slightly less common scenario using single directions of multiple relationship types:

  • "Association Membership" is a membership type inherited by "Primary Contact for" (a relationship from an individual to an organization) and "Employer of"
  • "Consolidated Crap Co." has "Employer of" relationships to lots of individuals and a "Primary Contact is" relationship to one individual "Don"
  • Don signs up for an Association Membership
  • The membership should be inherited by CCC and all of the employees

Anyway, it's true that this is ripe for misconfigurations, and you can get some funny things happening if you use both directions of a relationship type and someone has that relationship with multiple contacts. However, it's a core thing people rely on.

Further, you should expect that the membership should be chained in the same way the relationships are chained:

  • Alice and Bob's daughter Evelyn should inherit the Family Membership as long as she's related to the household, and then when she grows up and moves away, her membership goes away when she's no longer related to the household.
  • If Alice and Bob get a divorce, and Bob has custody of Evelyn such that the Alice and Bob Household stays related to Bob and Evelyn, but not Alice, Alice keeps her Family Membership, the Alice and Bob Household loses its membership, and consequently Bob and Evelyn lose theirs, too.

In trying to make sense of the code / fix / test for civicrm#15062
I discovered the tests wouldn't pass due to a weird edge case where an individual inherited a membership
and that membership was inherited in turn via a relationship the individual had (with the same organization)

I don't believe that inheriting inherited memberships is an intentional feature & it DOEs cause issues
- this removes that possibility
@eileenmcnaughton
Copy link
Contributor Author

@agh1 thanks for that - I've updated this so that you CAN inherit an inherited membership but you cannot inherit a membership that was inherited from you.

ie. we have a membership for a household that was inherited from an individual & the relationship has 2 way inheritence. With this change the individual will not get a second copy of the relationship inheritted from the household that inherited the membership from them.

@agileware-pengyi
Copy link
Contributor

Thanks @eileenmcnaughton
I did a quick test and it looks good.

@eileenmcnaughton
Copy link
Contributor Author

thanks @agileware-pengyi - do you agree this cut is narrow enough to not affect other scenarios?

@agileware-pengyi
Copy link
Contributor

thanks @agileware-pengyi - do you agree this cut is narrow enough to not affect other scenarios?

I don't think this will do something wrong, but it may not cover all the cases. For example

  • Three contacts which have two-way relationship to each other (like a triangle)
  • One of the contacts has the membership and others will get inherited membership by the relationship

In the end, everyone will have duplicated membership.

I am not sure if this is in the scope of this PR.

@eileenmcnaughton
Copy link
Contributor Author

Oh wow - yeah the 3 way is def out of scope. We'd have to follow the chain back up to find the original membership. Yuck.

OK I'll merge this as addressing the NARROW scope of a 2 person loop

@eileenmcnaughton eileenmcnaughton merged commit 875ce6c into civicrm:master Aug 29, 2019
@eileenmcnaughton eileenmcnaughton deleted the weird2 branch August 29, 2019 04:34
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.

3 participants