fix: force active membership if the user has active subs #3268

dkoo · 2024-07-23T23:30:06Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

This is a third attempt at addressing the problem described in #3060 and #3050, where if a user has more than one subscription and not all are active, any of the user's memberships tied to the subscriptions' products could be incorrectly expired at any time.

This implements two new checks at runtime:

When checking the user membership's status
When checking whether the user has access to a restricted post

The first should run only for memberships with non-active statuses, and only if the membership's plan is tied to subscription product ownership. If it is and the user does have an active subscription with the required subscription product(s), it will update the membership to active status and relink the membership to the active subscription.

The second check is a fallback of sorts at the point where the user is trying to view restricted content. It checks if the membership requires any subscription products and allows access as long as the user has an active subscription with one of those products. Since the membership updates from the first check could take longer than it takes for the restricted content to load in the browser, this second check should avoid a race condition the first time any affected users try to view restricted content, but maybe it's not needed since the first check should permanently fix the membership status.

The major piece that was missing from #3050 was WC_Memberships_User_Membership::unschedule_expiration_events()—without calling this, updating an expired membership to active will result in an infinite loop of active/expire actions that will severely impact site performance. If we call that method before updating the status to active, the check should run exactly once for any impacted memberships, so in theory this PR should have no negative effect on site performance.

How to test the changes in this Pull Request:

Set up a membership plan that requires ownership of a subscription product(s) and set it to restrict content (e.g. of a post category).
As a reader, purchase the required subscription and confirm that you can view the restricted content.
As an admin, manually update the reader subscription to "Expired" status.
As the reader, confirm that you can no longer view the restricted content.
Still as the reader, purchase another subscription to the required product.
On trunk, observe that you still can't view restricted content despite owning an active subscription with the required product.
As an admin, observe that the reader's membership still shows "Expired" status and is tied to the reader's expired subscription, instead of the newer active one.
Check out this branch and refresh the membership either in the admin or the restricted content as the reader.
Confirm that the reader can now view restricted content, and that the membership is updated to "Active" status, relinked to the active subscription, and that it doesn't continually try to expire and reactivate itself in an infinite loop that eventually crashes your entire site.

Other information:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes, as applicable?
Have you successfully ran tests with your changes locally?

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1207858308890295

adekbadek

Works as described! There are two typos to fix in get_last_successful_order method.

The testing instructions mention comparing to the release branch. Should this PR be a hotfix?

includes/plugins/wc-memberships/class-memberships.php

includes/reader-revenue/woocommerce/class-woocommerce-connection.php

dkoo · 2024-07-31T15:48:40Z

The testing instructions mention comparing to the release branch. Should this PR be a hotfix?

It was originally a hotfix, but I walked that back since it's a pretty major change that warrants more careful QA, I think. I updated the testing instructions.

# [4.8.0-alpha.1](v4.7.0...v4.8.0-alpha.1) (2024-08-01) ### Bug Fixes * force active membership if the user has active subs ([#3268](#3268)) ([aebe581](aebe581)) * **woocommerce:** prevent /shop redirect from author archives ([46db669](46db669)) ### Features * enhancements for the Non editing contributor role ([#3277](#3277)) ([e47e422](e47e422)) * **ga4:** filter custom params ([eda5553](eda5553)) * show deprecation warning for CM in Engagement wizard ([#3264](#3264)) ([0813369](0813369))

matticbot · 2024-08-01T16:40:31Z

🎉 This PR is included in version 4.8.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

miguelpeixe · 2024-08-01T18:22:59Z

includes/reader-revenue/woocommerce/class-woocommerce-connection.php

-					return $subscription;
-				}
-			}
+		$active_subscriptions = self::get_active_subscriptions_for_user( $user_id );


This method returns an array of integers, which breaks the expected return of get_last_successful_order(). This causes the get_contact_from_customer() to throw a fatal error by trying to ->get_id() from the integer and breaks data event handlers that rely on that method.

Thanks for catching! I'll have a look and push an "alpha hotfix" for it

PR here: #3299

@wordp

# [5.0.0](v4.7.0...v5.0.0) (2024-08-13) ### Bug Fixes * **data-events:** ensure get_last_successful_order returns WC_Order ([222f58c](222f58c)) * **data-events:** ensure get_last_successful_order returns WC_Order ([#3299](#3299)) ([da21561](da21561)) * **data-events:** race condition between `order_completed` and `subscription_updated` ([#3314](#3314)) ([afbf2e8](afbf2e8)) * force active membership if the user has active subs ([#3268](#3268)) ([aebe581](aebe581)) * namespacing issue ([7b1506a](7b1506a)) * update dependencies to support `@wordpress/scripts` ([#3181](#3181)) ([0d35ac8](0d35ac8)) * **woocommerce:** prevent /shop redirect from author archives ([46db669](46db669)) ### Features * enhancements for the Non editing contributor role ([#3277](#3277)) ([e47e422](e47e422)) * **ga4:** filter custom params ([eda5553](eda5553)) * rename guest contributor role ([#3302](#3302)) ([76ac05e](76ac05e)) * rename guest contributor role ([#3302](#3302)) ([c7f6917](c7f6917)) * show deprecation warning for CM in Engagement wizard ([#3264](#3264)) ([0813369](0813369)) ### BREAKING CHANGES * Updates dependencies for compatibility with WordPress 6.6.*, but breaks JS in WordPress 6.5.* and below. If you need support for WP 6.5.*, please do not upgrade to this new major version. * chore: refactor for updating dependencies in newspack-scripts * chore: JS formatting fixes for eslint errors * chore: update NPM scripts and other dependencies * chore: fix jest unit tests * chore: update dependencies in newspack-components * fix: peer dependency conflicts with @wordp ress/* packages * chore: update newspack-components dependencies * chore: proxy stylelint from newspack-scripts * chore: remove unnecessary stylelint * refactor: use proxy script for eslint and stylelint scripts * chore: remove console.log * chore: update newspack-scripts to NPM alpha version * chore: update newspack-scripts to git branch * chore: update newspack-scripts to alpha.3 * test: remove unneeded mocks * deps: newspack-scripts@5.6.0-alpha.1 * chore: fix scripts * fix: update newspack-scripts to v5.6.0-alpha.2 * fix: add newspack-revisions.js to webpack build for linting * fix: update stylelint and format CSS for errors * chore: clear TS check errors * fix: start command * fix: remove i18n script (Blocks only) * temporarily revert stylelint autoformat to avoid merge conflicts * chore: add .stylelintrc.js * fix: reformat SCSS * chore: update newspack-scripts to v5.6.0-alpha.4 * fix: add missing Prettier config files * chore: update newspack-scripts to 5.6.0-alpha.5 * chore: update newspack-scripts to v5.6.0-alpha.7 * style: stylelint autofixes * fix: add fix:js script; temporarily remove format:js * fix: update newspack-scripts to v5.5.0-alpha.8 and restore format:js script * chore: add NPM scripts for PHP * refactor: move recaptcha JS to src directory * fix: ignore dist and node_modules inside src/components, too * fix: update Babel configs for components * fix(components): output TS and JS, and publish new major version to NPM * fix: do not delete node_modules after post-publish * chore: bump newspack-scripts to v5.5.2

matticbot · 2024-08-13T07:36:24Z

🎉 This PR is included in version 5.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

dkoo added 2 commits July 23, 2024 17:09

fix: force active membership if the user has active subs

1ed462f

fix: match memberships product ownership logic (OR, not AND)

5b865f7

dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label Jul 23, 2024

dkoo self-assigned this Jul 23, 2024

dkoo requested a review from a team as a code owner July 23, 2024 23:30

dkoo added 3 commits July 23, 2024 17:34

fix: logical error

7a9b8a6

fix: logical error

8ddecbe

fix: incorrect condition

e9312a8

adekbadek requested changes Jul 31, 2024

View reviewed changes

dkoo added 2 commits July 31, 2024 09:48

Merge branch 'trunk' into fix/expired-memberships-with-active-subs

690bb41

fix: feedback from code review

01d8e27

dkoo requested a review from adekbadek July 31, 2024 15:52

adekbadek approved these changes Aug 1, 2024

View reviewed changes

github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Aug 1, 2024

dkoo merged commit aebe581 into trunk Aug 1, 2024
11 checks passed

dkoo deleted the fix/expired-memberships-with-active-subs branch August 1, 2024 15:57

matticbot added the released on @alpha label Aug 1, 2024

miguelpeixe reviewed Aug 1, 2024

View reviewed changes

dkoo mentioned this pull request Aug 1, 2024

fix(data-events): ensure get_last_successful_order returns WC_Order #3299

Merged

6 tasks

matticbot added the released label Aug 13, 2024

adekbadek mentioned this pull request Aug 14, 2024

fix(woocommerce-memberships): membership status check #3338

Merged

6 tasks

dkoo mentioned this pull request Aug 29, 2024

fix(woocommerce-memberships): membership status check with post status instead of type #3380

Merged

6 tasks

adekbadek mentioned this pull request Sep 5, 2024

Hotfix/woo subs vs memberships expiration #3393

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: force active membership if the user has active subs #3268

fix: force active membership if the user has active subs #3268

dkoo commented Jul 23, 2024 •

edited

Loading

adekbadek left a comment

dkoo commented Jul 31, 2024

matticbot commented Aug 1, 2024

miguelpeixe Aug 1, 2024 •

edited

Loading

dkoo Aug 1, 2024

dkoo Aug 1, 2024

matticbot commented Aug 13, 2024

fix: force active membership if the user has active subs #3268

fix: force active membership if the user has active subs #3268

Conversation

dkoo commented Jul 23, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

adekbadek left a comment

Choose a reason for hiding this comment

dkoo commented Jul 31, 2024

matticbot commented Aug 1, 2024

miguelpeixe Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

dkoo Aug 1, 2024

Choose a reason for hiding this comment

dkoo Aug 1, 2024

Choose a reason for hiding this comment

matticbot commented Aug 13, 2024

dkoo commented Jul 23, 2024 •

edited

Loading

miguelpeixe Aug 1, 2024 •

edited

Loading