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

feat: handle user deletion event on the network #91

Merged
merged 4 commits into from
Apr 25, 2024
Merged

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Apr 10, 2024

All Submissions:

Changes proposed in this Pull Request:

Adds user deletion handling.

(Note: this is a part of integrating the changes from the data-integrity-improvements branch (#89))

How to test the changes in this Pull Request:

  1. Register a user on one of the sites, wait for the data to sync*
  2. Delete the user on one of the sites, wait for the data to sync
  3. Observe the user was deleted from all sites
  4. Observe there is only one network_user_deleted event in the event log

* can be sped up by running wp newspack-network process-webhooks on the node and then wp newspack-network sync-all on the other node 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?

@adekbadek adekbadek requested a review from a team as a code owner April 10, 2024 09:13
Copy link
Contributor

@chickenn00dle chickenn00dle left a comment

Choose a reason for hiding this comment

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

Hey @adekbadek,

I'm running into issues testing this one. A couple things:

  1. When I delete the propagated user from a node, I see the delete event, but the user is never deleted from the hub site. Here is what I see on the node:
    Screenshot 2024-04-24 at 10 54 36

And once the event propagates, this is what I see on the hub:
Screenshot 2024-04-24 at 10 56 57

But the user is never deleted on the hub:
Screenshot 2024-04-24 at 10 57 09

  1. Might be related to the above, but when a user is deleted, I am also seeing the user updated event trigger (pictured in the screenshots above). I would expect this event not to be needed once a user is deleted. I wonder if this also has something to do with why the user persists on the hub even after the deletion event is processed?

@adekbadek
Copy link
Member Author

  1. The code was handling only deletion of Network Readers, so if a reader deleted their account on a non-origin site, the account would not be deleted elsewhere. This was erroneous, fixed in 998b4fd
  2. Fixed the superfluous network_user_updated event triggering by deletion in 1d6ff1c

@adekbadek adekbadek requested a review from chickenn00dle April 25, 2024 08:12
Copy link
Contributor

@chickenn00dle chickenn00dle left a comment

Choose a reason for hiding this comment

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

Nice! This is working as expected now. I noticed some strangeness with the logs, but considering the primary functionality is working and deletions are ultimately being logged, I'm gonna go ahead and approve this.

require_once ABSPATH . 'wp-admin/includes/user.php';

// Don't broadcast this deletion on the network.
add_filter( 'newspack_network_process_user_deleted', '__return_false' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting this results in conflicting messaging in the logs where the account is logged as skipped, but then is logged as deleted:

[25-Apr-2024 14:07:47 UTC] [3110] /Users/raz/Sites/newspack/network/includes/class-data-listeners.php:103 User deletion with email: [email protected] was skipped due to filter use.
[25-Apr-2024 14:07:47 UTC] [3110] /Users/raz/Sites/newspack/network/includes/incoming-events/class-user-deleted.php:70 User [email protected] deleted.

Since the final log indicates the deletion was successful, I think this is not a blocker.

@adekbadek adekbadek merged commit cf3df3b into trunk Apr 25, 2024
3 checks passed
@adekbadek adekbadek deleted the feat/user-deletion branch April 25, 2024 14:22
matticbot pushed a commit that referenced this pull request Apr 25, 2024
# [1.7.0-alpha.1](v1.6.0...v1.7.0-alpha.1) (2024-04-25)

### Features

* handle user deletion event on the network ([#91](#91)) ([cf3df3b](cf3df3b))
@matticbot
Copy link
Contributor

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request May 15, 2024
# [1.7.0](v1.6.0...v1.7.0) (2024-05-15)

### Features

* handle user deletion event on the network ([#91](#91)) ([cf3df3b](cf3df3b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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