Editing past memberships assigns incorrect rule set

[axaak]

Hi,
as ever, thank you for the plugin.
I spotted that the code will apply rules on a membership update. This made me think about updating an expired membership from the past.

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.

The current status rules should apply. In situations where the current status rules are in place, editing one of the expired memberships (e.g. 14-15) causes the expired rules to be applied even though the contact has a current membership.

The plugin should pull back all memberships and apply the status of the most recent on a membership update. It shouldn't apply rules based on a past, expired membership.

Hope that makes sense.

Thanks
axaak

[christianwach]

@axaak I addressed this initially in a34ec70, however I am still unsure about the logic of this issue.

What concerns me is the assumption that the membership with the latest end date is necessarily the canonical one from which the status(es) of the WordPress user can be derived. This is especially relevant when syncing to memberships to capabilities.

To this end, I changed my mind and in 58f0ed0 I apply rules based on all memberships when a membership is edited or created.

Thoughts?

[kcristiano]

@christianwach We have a number of Orgs that insert new memberships of the same type rather than renew. Therefore, I was interested in your initial patch. Before I head off to test, what is the expected behavior if we find multiple records of the same member type for a user. The latest of which has a current/active status and the others are expired?

[christianwach]

@kcristiano With the current master branch, you should find the following:

When syncing memberships-to-roles, the role should be determined by the status of the final membership - i.e. the one with the latest end_date. If multiple roles are in use, they should be toggled according to the status of each membership.

When syncing memberships-to-capabilities, each capability should be toggled according to the status of each membership.

This is because:

• When syncing via "Manual Sync", memberships are now passed (stepped, of course) to rule_apply() sorted by contact_id ASC, end_date ASC
• When creating or editing a single membership in the CiviCRM admin, all memberships are passed to rule_apply() sorted by end_date ASC
• When syncing a single user's memberships (e.g. on login/logout), all memberships are passed to rule_apply() sorted by end_date ASC
• When syncing via WordPress pseudo-cron, each WordPress user is handled in turn and all their memberships are passed to rule_apply() sorted by end_date ASC (this routine probably needs attention given that it is not stepped)
• When sync is triggered via CiviCRM cron, each membership that changes status should trigger the same procedure as creating or editing a single membership in the CiviCRM admin via the civicrm_post hook.

I'd value input on this logic - it's a bit daunting to audit all these procedures in one go - but I think @axaak is right that the previous logic was flawed.

[axaak]

Thanks for your quick response on this @christianwach.
58f0ed0 looks as I'd expected and the approach of processing all memberships to arrive at a final result is sound. I've re-tested and contacts now retain their current status even if I edit an expired membership :-)

Thanks again, really grateful for your work on this,
axaak

[christianwach]

@axaak Thanks for testing and confirming that the commit fixes things for you. I'll wait until @kcristiano has had a chance to test before releasing a new version on the WordPress plugin directory.

[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.