Add default

[eotb]

I'm still rooting around in the code for a few things, but the basic logic, I think, is there.

I'm pretty sure my new else code will need to do something with $flag, but I'm still working out what that does.

There are certainly other changes missing for this to be complete, the most obvious being adding a "default_wp_role" field to the settings page so it gets populated in the first place.

Also, the settings page automatically checks the states in current when they are unchecked in expired. That logic will have to change too.

[christianwach]

@eotb Look forward to seeing how this progresses 👍

[eotb]

I made some more changes to the code. Struggling to test the changes, I can’t activate the plugin. Not sure what I have changed that would prevent activation. I’m trying to get access to the debug logs.

From: Christian Wach notifications@github.com Sent: January 29, 2020 12:19 PM To: christianwach/civicrm-wp-member-sync civicrm-wp-member-sync@noreply.github.com Cc: eotb paul.dufresne@eotb.ca; Mention mention@noreply.github.com Subject: Re: [christianwach/civicrm-wp-member-sync] Add default (#30)

@eotb https://github.com/eotb Look forward to seeing how this progresses 👍

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/christianwach/civicrm-wp-member-sync/pull/30?email_source=notifications&email_token=AONDGFAWIUIYE4XREHY3MYLRAG3BPA5CNFSM4KNIDGMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKIATMI#issuecomment-579865009 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AONDGFDKQU2FHYGSPCOQBNTRAG3BPANCNFSM4KNIDGMA . https://github.com/notifications/beacon/AONDGFDT4SVZUYCTBQNKJCDRAG3BPA5CNFSM4KNIDGMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKIATMI.gif

[eotb]

The add_default branch is now fully functional, including UI changes. If you get a chance, give it a try.

[christianwach]

@eotb Thanks! I'll try it when I get a free moment.

[christianwach]

@eotb I'm sorry it has taken so long for me to look at this. Lockdown, other commitments etc etc.

I've reviewed your code and can see what you're trying to do. What I can't see at the moment is why you're trying to do it. Could you please explain the situation that this PR solves?

Thanks, Christian

[eotb]

Hey Christian, no problem. I should start by noting that I’ve been running my version of the plugin for almost a year now, and I haven’t had any issues, so it seems stable enough.

In our particular use case, we needed three membership states:

1. Current – Members that have paid their dues and have access to all club resources
2. Expired – Members that haven’t yet renewed, but still have access to the renewal form so that they can renew (which would restore them to Current)
3. Cancelled – Members who’s memberships have been cancelled by the club or because they’ve let their membership lapse for more than a year. They cannot renew.

Your standard model was binary Either they were current, or they weren’t.

With the changes I made to the plugin, there’s a default role that is applied to any user that doesn’t match an association rule. I map the “current” states to the “current” role and the “expired” state to the “expired” role, and if a user doesn’t match one of those conditions, they’re assigned the default role which basically has no privileges.

The ideal option would be to allow a one-to-one relationship between member states and roles (even if multiple states mapped to the same role), but that’s a significant UI change, even if your backend code is already setup to handle it. The default role option allowed us that third option we needed and required minimal UI changes.

From: Christian Wach notifications@github.com Sent: December 1, 2020 6:40 To: christianwach/civicrm-wp-member-sync civicrm-wp-member-sync@noreply.github.com Cc: eotb paul.dufresne@eotb.ca; Mention mention@noreply.github.com Subject: Re: [christianwach/civicrm-wp-member-sync] Add default (#30)

@eotb https://github.com/eotb I'm sorry it has taken so long for me to look at this. Lockdown, other commitments etc etc.

I've reviewed your code and can see what you're trying to do. What I can't see at the moment is why you're trying to do it. Could you please explain the situation that this PR solves?

Thanks, Christian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/christianwach/civicrm-wp-member-sync/pull/30#issuecomment-736479651 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AONDGFFE4HKLF23B5GWKWHDSSTIXPANCNFSM4KNIDGMA . https://github.com/notifications/beacon/AONDGFFKNB4WPSYYAJHWFZDSSTIXPA5CNFSM4KNIDGMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFPS4TIY.gif

[christianwach]

@eotb Thanks for explaining the situation.

I think I would have approached this a bit differently, though I can see why you approached it the way you did. The issue (as I see it) is that yours is perhaps an unusual case - I don't think many installs will want the kind of setup that you have with the custom Membership status you've put in place. Some might, for sure, but most won't - and might get confused by the UI changes. Here's how I would have approached it...

Your "cancelled" status seems to be a special kind of "expired" status with extra requirements. I would include that Membership Type among the "expired" group, then add a callback to the civi_wp_member_sync_rule_apply_roles_expired action and watch for the "cancelled" $membership_type_id being passed to it. When it is, I'd remove the "expired" role and assign the "cancelled" role to that WordPress User instead. Example code below:

add_action( 'civi_wp_member_sync_rule_apply_roles_expired', 'my_maybe_assign_cancelled_role', 10, 4 );
function my_maybe_assign_cancelled_role( $user, $membership_type_id, $status_id, $association_rule ) {

    // Bail if not the "cancelled" Membership Type ID.
    if ( $membership_type_id !== YOUR_CUSTOM_TYPE_ID ) {
        return;
    }

    // Get plugin reference.
    $plugin = civicrm_wpms();

    // Remove expired role.
    if ( $plugin->users->wp_has_role( $user, $association_rule['expired_wp_role'] ) ) {
        $plugin->users->wp_role_remove( $user, $association_rule['expired_wp_role'] );
    }                                        

    // Add cancelled role.
    if ( ! $plugin->users->wp_has_role( $user, 'YOUR_CANCELLED_ROLE' ) ) {
        $plugin->users->wp_role_add( $user, 'YOUR_CANCELLED_ROLE' );
    }                                        

}                                        

Wouldn't this code cover your situation without requiring modifications to this plugin?

Cheers, Christian

[christianwach]

I can (for completeness) see how you might also want an equivalent action in rule_undo() so you could act on events where the Contact's "cancelled" Membership is deleted. I suspect, however, that it's more likely that the Membership Type ID would be changed, but I'm happy to add an action in rule_undo() anyway.

Cheers, Christian

[eotb]

It would meet the requirements, but it would complicate the changes for the next administrator coming in. They’d not only need to understand the GUI of your application, they’d also need to understand the code below. I gave you an example using one membership type, but we have others that have similar 3-state requirements.

I would argue that my approach simplifies the use of your plugin. An organization could go in and map the current member states to their appropriate roles and allow other states to use the default automatically unless they need the default to map to a specific role other than the default.

You could also give them the option of leaving the default state blank, in which case your plugin could revert to the current mode of operation.

From: Christian Wach notifications@github.com Sent: December 2, 2020 7:41 To: christianwach/civicrm-wp-member-sync civicrm-wp-member-sync@noreply.github.com Cc: eotb paul.dufresne@eotb.ca; Mention mention@noreply.github.com Subject: Re: [christianwach/civicrm-wp-member-sync] Add default (#30)

@eotb https://github.com/eotb Thanks for explaining the situation.

I think I would have approached this a bit differently, though I can see why you approached it the way you did. The issue (as I see it) is that yours is perhaps an unusual case - I don't think many installs will want the kind of setup that you have with the custom Membership status you've put in place. Some might, for sure, but most won't - and might get confused by the UI changes. Here's how I would have approached it...

Your "cancelled" status seems to be a special kind of "expired" status with extra requirements. I would include that Membership Type among the "expired" group, then add a callback to the civi_wp_member_sync_rule_apply_roles_expired action and watch for the "cancelled" $membership_type_id being passed to it. When it is, I'd remove the "expired" role and assign the "cancelled" role to that WordPress User instead. Example code below:

add_action( 'civi_wp_member_sync_rule_apply_roles_expired', 'my_maybe_assign_cancelled_role', 10, 4 ); function my_maybe_assign_cancelled_role( $user, $membership_type_id, $status_id, $association_rule ) {

    // Bail if not the "cancelled" Membership Type ID.
    if ( $membership_type_id !== YOUR_CUSTOM_TYPE_ID ) {
           return;
    }

    // Get plugin reference.
    $plugin = civicrm_wpms();

    // Remove expired role.
    if ( $plugin->users->wp_has_role( $user, $association_rule['expired_wp_role'] ) ) {
           $plugin->users->wp_role_remove( $user, $association_rule['expired_wp_role'] );
    }                                        

    // Add cancelled role.
    if ( ! $plugin->users->wp_has_role( $user, 'YOUR_CANCELLED_ROLE' ) ) {
           $plugin->users->wp_role_add( $user, 'YOUR_CANCELLED_ROLE' );
    }                                        

}

Wouldn't this code cover your situation without requiring modifications to this plugin?

Cheers, Christian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/christianwach/civicrm-wp-member-sync/pull/30#issuecomment-737205350 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AONDGFD7H4C3YGGUG56UZYLSSYYURANCNFSM4KNIDGMA . https://github.com/notifications/beacon/AONDGFC3AOOXZ5AYE3U6QH3SSYYURA5CNFSM4KNIDGMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFPYNYZQ.gif

[christianwach]

It would meet the requirements, but it would complicate the changes for the next administrator coming in. They’d not only need to understand the GUI of your application, they’d also need to understand the code below.

Isn't this just a documentation issue? I genuinely don't think that the code I posted is difficult to grasp :-)

I gave you an example using one membership type, but we have others that have similar 3-state requirements.

You are of course free to copy the code, adapt it for your needs and use in as many situations as you want.

I would argue that my approach simplifies the use of your plugin.

With respect, I disagree. This plugin offers basic functionality that is being used on 600+ active installations via the WordPress Plugin Directory. There have been no requests (other than this one) for states other than "current" and "expired", so I assume that, for the practically everyone, the current functionality is all that's required.

Like many plugins, it is not intended to cover every eventuality - which is why it offers actions and filters that make it extensible by others (like you) with particular needs. As a result, I'm reluctant to add options that are only needed by one user. I will leave this PR open, however, and will reconsider if/when others chime in.

Cheers, Christian