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-sync): handle active subs from other nodes #114

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Jul 18, 2024

All Submissions:

Changes proposed in this Pull Request:

Adds handling of the following edge case: a user might have two separate subscriptions on two sites of the network, both granting the same (synchronized, that is!) membership. If one of these subscriptions is cancelled, but the other one still active, the membership should still be active.

See https://app.asana.com/0/1206478771616338/1207029554091953/f

How to test the changes in this Pull Request:

  1. Buy a synced-membership-granting subscription on two sites of the network, using the same email address
  2. As the site admin, cancel one of the subscriptions
  3. Observe that the memberships are still active on both sites

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?

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.

HI Adam, I would add a comment similar to what I did on #112 (review).

Is there a reason why we have to make all these requests on the fly and can not rely on synced data?

@adekbadek
Copy link
Member Author

adekbadek commented Jul 23, 2024

Thanks @leogermani – I completely agree that a better solution is to rely on synchronised data instead of an on-the-fly lookup in this case. However, this fix is needed pretty urgently (#112 is not). Can we ship this if it works, and I'll work on optimising the strategy in a later PR?

@leogermani
Copy link
Contributor

@adekbadek I could not confirm the fix. Both on trunk and on this branch I got the same behavior. Here are detailed steps of what I did

  • As an anonymous visitor I visited the Hub and bought the subscription
  • I confirmed the membership was granted to the user
  • I saw the user propagate to the Node and the membership being granted there as well
  • As an anonymous visitor I went to the Node and bought the subscription that would grant me access to the same membership
  • I waited for the data to propagate.
  • I went to the Hub admin panel and Cancelled the Subscription. It went to "pending cancellation" state
  • I confirmed the Membership also went to "pending cancellation"
  • I checked on the Node, and the Membership was also marked as Pending cancellation

Also, when I tried to "Cancel now" the subscription, I got

PHP Fatal error:  Uncaught TypeError: array_merge(): Argument #2 must be of type array, null given in /newspack-repos/newspack-network/includes/hub/class-network-data-endpoint.php:60

@leogermani
Copy link
Contributor

In another attempt, I tried to:

  • Deactivate the Network plugin on both ends before buying the subscription
  • Making sure I had a membership active in both sites, and they were each "native" membership, meaning they were not synced from other site
  • Then I reactivated the Network plugin and proceeded the cancellation
  • Got the same results

@adekbadek adekbadek force-pushed the feat/handle-subs-from-other-nodes-on-memb-update branch from d2ea2fb to b7d897b Compare July 24, 2024 10:11
@adekbadek
Copy link
Member Author

Thanks for the thorough testing! I was making a shortcut in my testing, not realising it influences the code behaviour. What I was doing was creating both subscriptions simultaneously, which had the effect of creating "standalone" memberships on both sites. Your testing scenario, much more real-world, ensures the second memberships is not "standalone" but synced from the Hub site.

This should be fixed in b7d897b.

@leogermani
Copy link
Contributor

ok, it mostly works, there are edge cases and observations, but I'm not sure we want to address them all. Maybe we move forward and work on dealing with them in the definitive fix.

I tested:

  • Creating the subscription on the Hub and propagating it to the Node
  • Creating the subscription on the Node and propagating it to the Hub
  • Create both subscriptions at the same time with the plugin deactivated, so they are both "standalone"

In all cases, when you cancel the membership, it's not cancelled in the other site, as expected, and the Note is added to the membership saying why it's still active. BUT, the membership is set to "Pending Cancellation" when the subscription is set to this status. And then they remain pending after the subscription is cancelled (maybe we don't want even to set them as pending)

After cancelling the subscription in the site the membership was originally created, if you go to the other site, where the membership was propagated to, and cancel the subscription, it won't cancel the membership (as it should, because now there is no longer any active membership). This happens because in that site, the Membership is not tied to any subscription or product.

When the membership is standalone in both sites, things work as expected. Only the "Pending Cancellation" behavior still happens.

lmk if you want to move forward

@adekbadek
Copy link
Member Author

Thanks again for reviewing this thoroughly! Let's move forward with this as it's now and address the edge cases in the optimised (synced data reliant) solution.

@adekbadek adekbadek merged commit 97f9a58 into trunk Jul 25, 2024
4 checks passed
@adekbadek adekbadek deleted the feat/handle-subs-from-other-nodes-on-memb-update branch July 25, 2024 17:55
matticbot pushed a commit that referenced this pull request Aug 1, 2024
## [1.10.1](v1.10.0...v1.10.1) (2024-08-01)

### Bug Fixes

* **memberships-sync:** handle active subs from other nodes ([#114](#114)) ([4255f8c](4255f8c))
leogermani added a commit that referenced this pull request Aug 1, 2024
matticbot pushed a commit that referenced this pull request Aug 1, 2024
# [1.11.0-alpha.1](v1.10.1...v1.11.0-alpha.1) (2024-08-01)

### Bug Fixes

* **memberships-sync:** handle active subs from other nodes ([#114](#114)) ([97f9a58](97f9a58))

### Features

* **distributor:** sync comment status ([#116](#116)) ([5844853](5844853))
* ensure that membership plans have unique network id ([#108](#108)) ([d2d63a0](d2d63a0))
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

leogermani added a commit that referenced this pull request Aug 7, 2024
matticbot pushed a commit that referenced this pull request Aug 13, 2024
# [2.0.0](v1.10.1...v2.0.0) (2024-08-13)

### Bug Fixes

* **memberships-sync:** handle active subs from other nodes ([#114](#114)) ([97f9a58](97f9a58))
* update dependencies to support `@wordpress/scripts` ([#104](#104)) ([e9691b8](e9691b8))

### Features

* **distributor:** sync comment status ([#116](#116)) ([5844853](5844853))
* ensure that membership plans have unique network id ([#108](#108)) ([d2d63a0](d2d63a0))

### 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 newspack-scripts dependency updates

* chore: update composer

* fix: peer dependencies

* chore: update newspack-scripts to v5.6.0-alpha.3

* chore: update newspack-scripts to v5.6.0-alpha.4

* chore: remove unnecessary prettier config file

* chore: update newspack-scripts to v5.6.0-alpha.7

* fix: update phpcs.xml

* chore: bump newspack-scripts to v5.5.2
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

leogermani added a commit that referenced this pull request Sep 4, 2024
leogermani added a commit that referenced this pull request Sep 9, 2024
* Revert "fix(memberships-sync): handle active subs from other nodes (#114)"

This reverts commit 97f9a58.

* Revert "feat(admin): subscriptions view"

This reverts commit cb5bcb7.

* Revert "feat: remove hub's subscriptions and orders"

This reverts commit e234425.

* feat: remove api requests from woo item changed

* feat: add backfillers for order and subscription_changed

* feat: add user subscriptions metadata

* fix: ensure assoc array when processing webhook data

* feat: add product id in the array keys

* feat: add membership plan updated event and backfiller

* feat: add product id to array jeys

* feat: add subscription products to woo central dashboard

* fix: remove duplicate listeners

* Revert "Revert "fix(memberships-sync): handle active subs from other nodes (#114)""

This reverts commit 4acd965.

* remove merge leftovers

* fix: fix constant name
leogermani added a commit that referenced this pull request Sep 9, 2024
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.

3 participants