Skip to content

Option to CONPD when running the Cancel Membership script or CLI command#72

Open
kimcoleman wants to merge 1 commit into
strangerstudios:devfrom
kimcoleman:dev
Open

Option to CONPD when running the Cancel Membership script or CLI command#72
kimcoleman wants to merge 1 commit into
strangerstudios:devfrom
kimcoleman:dev

Conversation

@kimcoleman

Copy link
Copy Markdown
Member

All Submissions:

Changes proposed in this Pull Request:

Adds the option to support cancel on next payment date for members with active subscriptions as part of the "Cancel Membership" Toolkit script and WP CLI command.

Screenshot:
pmpro-toolkit-cancel-membership-next-payment-date

or via WP CLI command:
wp pmpro-toolkit cancel_level --level=1 --when=next-payment-date

@flintfromthebasement flintfromthebasement left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: #72 — Option to CONPD when running the Cancel Membership script or CLI command
kimcoleman → dev | 2 files, +105 -4
#72


Summary
Adds a "cancel on next payment date" mode to the Cancel Membership admin script and WP-CLI command. The overall approach is solid — mark the subscription cancelled in the DB first (to suppress the webhook from stripping access), then cancel at the gateway, then set the enddate so PMPro's expiration cron handles cleanup. One data integrity gap needs addressing before merge.


Issues

  • Major adminpages/scripts.php (the $wpdb->update() block in pmprodev_cancel_level_on_next_payment_date) — the return value of $wpdb->update() is ignored. If the update affects 0 rows — e.g. the status = 'active' condition in the WHERE clause doesn't match because the row was already modified — the subscription is already cancelled at the gateway and marked cancelled in the DB, but the enddate was never written. The member's level has no expiration; PMPro's expiration cron will never fire for them. They get free access until a manual correction. At minimum, log a warning on false/0 return:

    $updated = $wpdb->update( $wpdb->pmpro_memberships_users, ... );
    if ( ! $updated ) {
        // Fallback or log: subscription cancelled but enddate not set.
        pmpro_cancelMembershipLevel( $level_id, $user_id );
        return;
    }
  • Minor adminpages/scripts.php ~L749 — current_time( 'timestamp' ) has been deprecated since WP 5.3. Replace with time() for UTC comparison, which is correct here since strtotime( $next_payment_date ) returns UTC.

  • Minor adminpages/scripts.php$subscriptions is assigned inside if ( class_exists( 'PMPro_Subscription' ) ) but used in the foreach outside that block. PHP scoping makes this safe (you can only reach that foreach when $next_payment_date was set inside the block), but static analysis tools will flag it. Initialize $subscriptions = [] before the if block.


Looks Good

The two-phase sequence (mark cancelled in DB → cancel at gateway → set enddate) correctly prevents the incoming webhook from stripping the membership. I verified cancel_at_gateway() in core (class-pmpro-subscription.php:1169) — it marks status = 'cancelled' and saves before hitting the gateway API, and already handles gateway failures by emailing the admin. This PR's code doesn't need to re-check the gateway return value; the DB+enddate path is correct regardless.

The CLI $_REQUEST injection pattern matches the existing code, the --when WP-CLI docblock with options fences is correct format, and the next-payment-datenext_payment_date translation is handled cleanly.


Questions

  1. The $wpdb->update() targets rows where status = 'active'. At this point in the function, cancel_at_gateway() has already called $this->save() which sets the subscription's DB status to cancelled. Does that subscription status change also flip pmpro_memberships_users.status? If so, the WHERE clause would miss and the update would silently no-op — which is exactly the failure path the Major issue above describes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants