Order assumed for contacts with multiple memberships

[axaak]

Hi, thank you for the plugin which we're now live with.

Background:

Sync on login. Contacts have multiple memberships over time, e.g. a contact may have: Annual membership 2011-2012, status expired. Annual membership 2014-2015, status expired. Annual membership 2016-2017, status current.

Issue

Plugin gets all Civi memberships and assumes the last one in the array is the most recent. For us its the opposite way around, the first one in the array is the most recent, i.e. Current membership. Processing runs as follows: apply rules to 16-17 membership: current rules apply apply rules to 14-15 membership: expired rules apply apply rules to 11-12 membership: expired rules apply Outcome: expired rules apply even though contact has a current (16-17) membership.

Fix

File: civi-wp-ms-members.php Function: membership_get_by_contact_id() Line 532 add following code to API call:

'options' => array( 'sort' => "contact_id, end_date" ), Call should look like this: // get CiviCRM membership details $membership = civicrm_api( 'Membership', 'get', array( 'version' => '3', 'page' => 'CiviCRM', 'q' => 'civicrm/ajax/rest', 'sequential' => '1', 'contact_id' => $civi_contact_id, 'options' => array( 'sort' => "contact_id, end_date" ), )); Last item in the array is now the most recent and apply rules uses that. If you could update the plugin please that would be much appreciated.

Thanks, axaak

[christianwach]

@axaak Thanks for the report. Can you check 0f831a3a734ef798200ad4c78de25ca642e0a7ef to see if it fixes this for you?

[christianwach]

@axaak My previous commit doesn't sort by contact_id since we're already passing contact ID to the API call. However, I think it is relevant during the Manual Sync process, so I have added it to the API call there in d03a65685bc3e8f5995815247302a5162a5cfcdc. If you could test with your data, that'd be great.

[axaak]

Hi @christianwach, 0f831a3 does indeed fix the issue, many thanks. I had spotted the API call referenced in d03a656, because at first I didn't understand the limit of 5. However, a manual sync with the previous version didn't cause a problem with expired memberships, which is odd. I have re-tested today with the latest version and all is OK. i.e. those contacts with multiple memberships get the status from their latest membership on a manual sync.

Many thanks again, axaak

[christianwach]

a manual sync with the previous version didn't cause a problem with expired memberships, which is odd

The default sort order is likely to be by membership id, so it's possible that it accidentally processed later memberships, um, later. It's definitely an improvement to ensure that this order is followed.

As per #23, I'll wait for @kcristiano to test.

[kcristiano]

@christianwach Tested and this works fine. I did notice an other issue but cannot be sure when this was introduced or if it is intended behavior. I will open an issue up to track that separately.

[axaak]

Hi @christianwach, Another edge case has emerged and I think we need to change tack. The specific edge case is a contact with 2 memberships, an earlier expired membership and a later life membership. The life membership has no end date so sorting by end date doesn't work as we expect. The end_date field isn't even returned when you do the API call. The expired membership sorts to the end of the array and the member cannot login even though they have a life membership. (NB sorting by start_date, end_date doesn't work either because you could have a membership that starts later than the life one but is now expired)

Civi core knows the whether a contact has an active current membership. You can see this on the membership dashboard of a contact. The active memberships are listed first, followed by the inactive/expired ones. We should adopt the same approach, i.e. if Civi core thinks a member has an active membership then the WP user should get the current role.

If you look in civicrm/CRM/Member/Page/UserDashboard.php

function listMemberships()you'll see how its done. The important bit is a call to CRM_Member_BAO_Membership::getStatusANDTypeValues() which returns an is_current_member flag which is used to assign an ACTIVE entry in the membership array. Civi core then provides functions that will return a list of active or inactive memberships given a list of memberships via CRM_Member_BAO_Membership::activeMembers().

Ideally I think we should amend membership_get_by_contact_id() to assign the ACTIVE flag and then have apply_rules() call the activeMembers function to get any active memberships. I'm not so familiar with apply_rules() so here is a working, tested implementation changing just membership_get_by_contact_id() by way of example:

        // this part unchanged
                $membership = civicrm_api( 'Membership', 'get', array(
                        'version' => '3',
                        'sequential' => '1',
                        'contact_id' => $civi_contact_id,
                        'options' => array(
                                'sort' => 'end_date',
                        ),
                ));

        // New code. Assign the active membership attribute.
                $idx=0;
                foreach ($membership['values'] as $a_membership) {
                        $statusANDType = CRM_Member_BAO_Membership::getStatusANDTypeValues($a_membership['id']);
                        if (!empty($statusANDType[$a_membership['id']]['is_current_member'])) {
                                $membership['values'][$idx]['active'] = TRUE;
                        }
                        $idx++;
                }

        // this is probably better done in appy_rules() ?
                $inActiveMembers = CRM_Member_BAO_Membership::activeMembers($membership['values'], 'inactive');
                $activeMembers = CRM_Member_BAO_Membership::activeMembers($membership['values']);

        // this isn't ideal but we massage the array here so that apply_rules() gets what it expects.
                $membership['values']=$inActiveMembers;
                $membership['values']=array_merge($membership['values'], $activeMembers);

We massage the array of memberships to ensure that the active ones are at the end of the array. This isn't nice but its what the caller is expecting and does indeed fix the problem. Our edge case user (life membership) gets the current role, expired members get the expired role, simple members with just one current membership get the current role. I've tested sync via login and manual sync. There may be scope to simply return only the array of active memberships (which might empty) but I'm not sure what effect this would have on other callers of the function.

What do you think?

Regards, axaak

[christianwach]

@axaak Many thanks for this - I will investigate further when I get a moment.

Cheers, Christian

[christianwach]

@axaak This is great sleuthing. Thanks again.

So, I've had a cursory look over all the related code and have a few observations:

Firstly, CRM_Member_BAO_Membership::getStatusANDTypeValues() makes a new database query per membership to determine the contents of the is_current_member column in the civicrm_membership_status table. This, for me, raises two issues:

1. Doesn't the data in that table make much of this plugin's "Association Rule" UI obsolete? Isn't this plugin simply duplicating the settings in "Administer -> CiviMember -> Membership Status Rules"? When I wrote this plugin, I basically migrated the Drupal module to WordPress "as is" and have only now realised that Civi already provides this - and thus (given that Civi already knows the is_current_member status for each status_id) wouldn't mapping "Current" and "Expired" to their respective roles or capabilities be sufficient?

2. Couldn't all the new code be short-circuited with the following API call?

$memberships = civicrm_api( 'Membership', 'get', array(
    'version' => '3',
    'sequential' => 1,
    'contact_id' => $civi_contact_id,
    'status_id.is_current_member' => array(
        'IS NOT NULL' => 1
    ),
    'options' => array(
        'limit' => 0, 
        'sort' => 'status_id.is_current_member ASC',
    ),
    'return' => array(
        'id', 
        'contact_id', 
        'membership_type_id', 
        'join_date', 
        'start_date', 
        'end_date', 
        'source', 
        'status_id', 
        'is_test', 
        'is_pay_later', 
        'status_id.is_current_member'
    ),
));

The results of this gives the methods that call membership_get_by_contact_id() all the data they need in the required order with the minimum of queries, given that this is a single JOIN.

Am I making sense or have I missed something here?

[christianwach]

The results of this gives the methods that call membership_get_by_contact_id() all the data they need in the required order with the minimum of queries, given that this is a single JOIN.

I should note here that the data returned by membership_get_by_contact_id() is only ever passed to rule_apply(), so the return array could be reduced to only those params that rule_apply() uses.

[axaak]

This is great sleuthing

Its one of my favourite parts of the job ;-)

Doesn't the data in that table make much of this plugin's "Association Rule" UI obsolete?

Yes I think it does.

Isn't this plugin simply duplicating the settings in "Administer -> CiviMember -> Membership Status Rules"?

The checkbox, "Should this status be considered a current membership in good standing." is what controls this. Civi core knows, for example, that Grace may also to be considered current along with any custom statuses you may have defined here .

When I wrote this plugin, I basically migrated the Drupal module to WordPress "as is" and have only now realised that Civi already provides this - and thus (given that Civi already knows the is_current_member status for each status_id) wouldn't mapping "Current" and "Expired" to their respective roles or capabilities be sufficient?

Yes, you could get rid of the "Member codes" from the Assoc Rules UI. If Civi has is_current_member the WP user gets the Current WP Role otherwise it gets the Expiry WP Role.

Couldn't all the new code be short-circuited with the following API call?

Yes, this would work, I like it.

I guess there are two ways forward now: 1) Short-term, patch the membership_get_by_contact_id() function to return is_current_member as you've shown, (or a slimmed down version returning only what rules_apply() needs).

2) Consider a refactor of the plugin, removing obsolete columns from the UI. Worth mulling over before diving in.

As ever, many thanks for your prompt support with this. Cheers axaak

[christianwach]

Thanks for your feedback, @axaak

Short-term, patch the membership_get_by_contact_id() function

I have addressed this in a0acbc2311e48a3789103612a9f6d5b0400a0188. Seems to work as expected. I'll release on WP.org shortly (@kcristiano - would you like to test before I do so?)

Consider a refactor of the plugin, removing obsolete columns from the UI

Thanks for confirming. When I have time, I'll revisit this. If it ain't broke etc etc...

Cheers, Christian

[kcristiano]

@axaak I tested this with an expired 'Student' membership which was renewed to a 'general' membership. I ended up with two roles - 'General' linked to the renewed general membership type and lapsed student tied to the expired student membership type.

In your use case is a new membership being created?

[axaak]

@kcristiano the use case that prompted this is one contact with 2 memberships. The first membership is expired and a new one is created that is a life membership. I'm not sure how your test results in 2 roles. The WP user should be assigned the active role if they have an active (General) membership otherwise the user should get the expired role. Are you saying that if you view the user's WP account they have both an expired and active role?

[christianwach]

Are you saying that if you view the user's WP account they have both an expired and active role?

@axaak Yes, that is the case - but only when the expiry roles differ.

[christianwach]

@axaak @kcristiano I wonder if this issue and #24 are so inter-related that a new issue is called for? Seems like I'm endlessly referring to both whilst trying to solve each :)

[axaak]

FWIW I think this is issue #24 and they are different. My reading of #24 is that core Civi opts to extend an existing membership rather than create a new one. I think this is related to the issue Eilleen describes here https://civicrm.org/blog/eileen/renewing-expired-membership which although its about expired memberships is still the same idea. i.e. do you extend whats there already or create a new membership record? Different organisations will have different requirements here. I don't think its the job of CWMS to resolve this. Core civi should provide options via the UI and appropriate hooks for developers. Not sure that helps @kcristiano but fixing it at the CWMS level is probably not the way to go because it involves an org-dependent policy decision. Apologies if I've completely misunderstood issue 24!

[christianwach]

@axaak Thanks for your thoughts on this. I agree that this plugin cannot paper over weaknesses in CiviCRM core, although it does what it can to mitigate them. Does the current state of master work the way you expect it to? I'm pretty sure it fixes the original issue in that the memberships are now processed in a sensible order.

[christianwach]

Closing since I believe the issue to be fixed. 0.3.4 is now pushed to the WordPress plugin directory.