fix: store and recover lists on membership reactivation #1377

leogermani · 2023-12-06T19:08:48Z

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:

Currently, every time a user is granted a membership, they get add to all lists behind that membership.

If a membership is paused and then restored, the user will be re-added to all lists, even if they opt out of some of the lists at some point.

We recently found out that every membership that is tied to a Subscription, will be put on pause and active again when the subscription is being renewed. This could amplify this problem.

What this PR does is:

When a membership is deactivated, we store which of the lists behind the membership the user is actually subscribed to
Whenever a membership is reactivated, we will only resubscribe the user to the lists they were before (which could be none)

How to test the changes in this Pull Request:

1.Create a membership plan
2. Configure Newsletters with 2 or more lists
3. In the membership plan, configure Restricted Content, and select 2 "Subscription Lists"
4. Set the membership to be connected to a Subscription product
5. As a reader, buy the subscription
6. Confirm the reader is granted the membership
7. Confirm in the ESP that the reader was added to both lists
8. As the reader, go to My Account and unsubscribe from one of the lists
9. Confirm it worked
10. Now, process the subscription renewal (one good way of doing this, that simulates the real automatic renewal, is to find the scheduled action under Tools > Scheduled Actions and Run it - search by the subscription ID)
11. After the renewal is complete, confirm in the membership activity that it was set to paused and active again
12. Confirm that the user is still subscribed only to the one list they were subscribed before
13. Ideally, repeat the test leaving all lists selected and no lists selected

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/1206093655507511

miguelpeixe · 2023-12-21T19:06:26Z

includes/plugins/class-woocommerce-memberships.php

+		 */
+		$pre_existing_lists = self::get_user_lists_on_deactivation( $user_id, $user_membership->get_id() );
+		if ( ! empty( $pre_existing_lists ) ) {
+			$lists_to_add = array_intersect( $lists_to_add, $pre_existing_lists );


Non-blocking suggestion, as there's no need to keep that after the subscription has been restored:

Suggested change

$lists_to_add = array_intersect( $lists_to_add, $pre_existing_lists );

$lists_to_add = array_intersect( $lists_to_add, $pre_existing_lists );

delete_user_meta( $user_id, self::SUBSCRIBED_ON_DEACTIVATION_META_KEY );

Any time this happens again the meta should have the most recent subscription information, so it's also safer to not keep it in the database.

If a membership switch status from a status considered active to another status considered active, we would end up subscribing them to all lists. For example, if you switch from free_trial to active. Or even pending is considered active. See WC_Memberships_User_Memberships::get_active_access_membership_statuses()

But we need to make sure to update it when the membership is set to inactive. Looking at the code now, it seems tere could be some situations where we would keep outdated values there, let me have a closer look

So if they unsubscribe and switch to another "active" status, they'll get re-subscribed to the list they've previously opted out?

No... that's what we are trying to avoid

An additional user meta can store the last known membership status so you can check against that.

status transitions run before save_post which is where the other hook is triggered. This seems to work on my tests:

3b3d96c

Additional testing instructions

add user to membership

as the user, unsubscribe from a list

change the membership from an active state to another active state

see that nothing is done

I have not tested it yet, but how does the $previous_statuses is populated when switching from "active" to "active" if it's happening inside the remove_user_from_list() method, which is not called in that context?

Nevermind, it is called but bails here

Yes, maybe that method deserved a different name now... ?

When there are no current lists, we need to store it to make sure we override previously added info

miguelpeixe · 2023-12-21T20:37:00Z

includes/plugins/class-woocommerce-memberships.php

@@ -117,6 +133,10 @@ function ( $list_id ) use ( $list_ids ) {
 	 */
 	public static function remove_user_from_list( $user_membership, $old_status, $new_status ) {


Given that this method doesn't strictly remove a user from a list, I think it'd be good to change its name to something like handle_membership_status_change.

Looking good: 1082a5a

miguelpeixe

Working flawlessly! Thank you for the revisions!

leogermani · 2023-12-21T21:21:14Z

Working flawlessly! Thank you for the revisions!

Thank you for triggering this discussion

# [2.9.0-alpha.1](v2.8.0...v2.9.0-alpha.1) (2024-01-11) ### Bug Fixes * memberships & "other" ESP ([6e0402c](6e0402c)) * prevent the use of `add_contact` for data sync purposes ([#1386](#1386)) ([2d68c1f](2d68c1f)) * **renderer:** ensure group blocks use conditional tags ([#1380](#1380)) ([a1a9722](a1a9722)) * store and recover lists on membership reactivation ([#1377](#1377)) ([aa8a8f4](aa8a8f4)) ### Features * display warning if custom fields meta box is visible ([d595ed4](d595ed4)) * **tracking:** support UTM coming from the ESP ([#1388](#1388)) ([77d3a1e](77d3a1e))

matticbot · 2024-01-11T19:23:47Z

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [2.9.0](v2.8.3...v2.9.0) (2024-01-25) ### Bug Fixes * add lodash as dependency for editor.js ([c01aebb](c01aebb)) * memberships & "other" ESP ([6e0402c](6e0402c)) * prevent the use of `add_contact` for data sync purposes ([#1386](#1386)) ([2d68c1f](2d68c1f)) * **renderer:** ensure group blocks use conditional tags ([#1380](#1380)) ([a1a9722](a1a9722)) * store and recover lists on membership reactivation ([#1377](#1377)) ([aa8a8f4](aa8a8f4)) ### Features * display warning if custom fields meta box is visible ([d595ed4](d595ed4)) * **tracking:** support UTM coming from the ESP ([#1388](#1388)) ([77d3a1e](77d3a1e))

matticbot · 2024-01-25T19:15:07Z

🎉 This PR is included in version 2.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

fix: store and recover lists on membership deact

2a7ea7d

leogermani added the [Status] Needs Review label Dec 6, 2023

leogermani self-assigned this Dec 6, 2023

leogermani requested a review from a team as a code owner December 6, 2023 19:08

leogermani changed the title ~~fix: store and recover lists on membership deact~~ Dec 6, 2023

Merge branch 'master' into fix/premium-lists-resubscription

20f5eae

miguelpeixe approved these changes Dec 21, 2023

View reviewed changes

github-actions bot added [Status] Approved Ready to merge and removed [Status] Needs Review labels Dec 21, 2023

leogermani added 3 commits December 21, 2023 16:36

fix: store up to date info about lists

bae7c85

When there are no current lists, we need to store it to make sure we override previously added info

fix: always update the lists_on_deactivation

e149a74

fix: do not act if updating from active to active

3b3d96c

miguelpeixe requested changes Dec 21, 2023

View reviewed changes

miguelpeixe self-requested a review December 21, 2023 20:37

fix: reorg and rename methos for readability

1082a5a

miguelpeixe approved these changes Dec 21, 2023

View reviewed changes

leogermani merged commit aa8a8f4 into master Dec 21, 2023
7 checks passed

leogermani mentioned this pull request Jan 8, 2024

fix: do not auto subscribe to premium lists #1359

Closed

6 tasks

dkoo deleted the fix/premium-lists-resubscription branch January 9, 2024 16:47

matticbot added the released on @alpha label Jan 11, 2024

matticbot added the released label Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: store and recover lists on membership reactivation #1377

fix: store and recover lists on membership reactivation #1377

leogermani commented Dec 6, 2023 •

edited

Loading

miguelpeixe Dec 21, 2023

leogermani Dec 21, 2023

leogermani Dec 21, 2023

miguelpeixe Dec 21, 2023

leogermani Dec 21, 2023

miguelpeixe Dec 21, 2023

leogermani Dec 21, 2023

miguelpeixe Dec 21, 2023

miguelpeixe Dec 21, 2023

leogermani Dec 21, 2023

miguelpeixe Dec 21, 2023

leogermani Dec 21, 2023

miguelpeixe left a comment

leogermani commented Dec 21, 2023

matticbot commented Jan 11, 2024

matticbot commented Jan 25, 2024

	$lists_to_add = array_intersect( $lists_to_add, $pre_existing_lists );
	$lists_to_add = array_intersect( $lists_to_add, $pre_existing_lists );
	delete_user_meta( $user_id, self::SUBSCRIBED_ON_DEACTIVATION_META_KEY );

		@@ -117,6 +133,10 @@ function ( $list_id ) use ( $list_ids ) {
		*/
		public static function remove_user_from_list( $user_membership, $old_status, $new_status ) {

fix: store and recover lists on membership reactivation #1377

fix: store and recover lists on membership reactivation #1377

Conversation

leogermani commented Dec 6, 2023 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelpeixe left a comment

Choose a reason for hiding this comment

leogermani commented Dec 21, 2023

matticbot commented Jan 11, 2024

matticbot commented Jan 25, 2024

leogermani commented Dec 6, 2023 •

edited

Loading