Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(memberships): remove managed flag on cancel or expire #192

Merged
merged 5 commits into from
Jan 21, 2025

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Jan 17, 2025

Closes 1200550061930446-as-1209019125448157/f

This PR removes the network managed meta flag when a network membership is cancelled or expired, and resets network membership meta otherwise. This is so readers can resume a cancelled or expired subscription on another network site.

Right now, if a reader has an inactive membership on one site, then tries to rejoin the membership on another site, the managed meta flag and other meta data causes a plan mixup between nodes as in the asana task above.

Steps to test

  1. As a reader fulfill the requirements to join a membership on a site on the network
  2. Either wait for the membership to propogate, or force via the wp newspack-network sync-all tool.
  3. Verify the membership on another site on the network
  4. On the site where the membership is managed, cancel the membership fully, either as reader or admin and wait for event to propogate
  5. On the second network site, verify the cancelled status of the membership
  6. On the second site again, log in as the same reader, and fulfill the requirements to join the same membership on this site this time, then wait for the event to propogate
  7. On the first site, verify the membership is now managed by the second site

@chickenn00dle chickenn00dle force-pushed the fix/remove-manage-meta-on-cancel branch from 7c576da to 8f70165 Compare January 17, 2025 22:44
@chickenn00dle chickenn00dle marked this pull request as ready for review January 17, 2025 22:45
@chickenn00dle chickenn00dle requested a review from a team as a code owner January 17, 2025 22:45
@chickenn00dle chickenn00dle force-pushed the fix/remove-manage-meta-on-cancel branch 2 times, most recently from 4c33ae1 to 101ce15 Compare January 17, 2025 22:56
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Interesting! So you're saying that if the membership is being permanently cancelled (not on hold) we should unlink them from the network, allowing it to be reactivated by a subscription purchase in another site. Makes sense!

Looking at the code, looks like the 3 update_post_meta starting on line 87 can be deleted after your changes (and it makes sense, because we don't check if $user_membership is a good object at that point)

I think it's worth adding a slightly bigger comment block explaining this reasoning there, so we can remember why we do this.

Not sure if it's also worth to add a note to the Membership saying it was unlinked from the other site?

@chickenn00dle chickenn00dle force-pushed the fix/remove-manage-meta-on-cancel branch from 101ce15 to 81f6156 Compare January 20, 2025 20:56
@chickenn00dle chickenn00dle force-pushed the fix/remove-manage-meta-on-cancel branch from 81f6156 to 8e8a92e Compare January 20, 2025 20:58
@chickenn00dle
Copy link
Contributor Author

Thanks for the review @leogermani!

I've updated the PR with some more thorough comments, added a membership note, and am deleting all managed meta in 8e8a92e

Let me know if these comments need to be more clear!

$user_membership = wc_memberships_create_user_membership(
[
'plan_id' => $local_plan_id,
'user_id' => $user->ID,
]
);

update_post_meta( $user_membership->get_id(), Memberships_Admin::NETWORK_MANAGED_META_KEY, true );
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean you can delete these 3 lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah whoops! Misread that. Updated in b0678eb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. Also snuck in a change to one of the comments relevant here: 299a9ca

Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Tests well!

@chickenn00dle chickenn00dle merged commit 67a5cf0 into trunk Jan 21, 2025
4 checks passed
@chickenn00dle chickenn00dle deleted the fix/remove-manage-meta-on-cancel branch January 21, 2025 16:04
Copy link

Hey @chickenn00dle, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

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

Successfully merging this pull request may close these issues.

2 participants