Skip to content

feat(rest): add DELETE /notifications/{id} - #639

Open
faisalahammad wants to merge 1 commit into
WordPress:developfrom
faisalahammad:feature/363-delete-notification
Open

faisalahammad wants to merge 1 commit into
WordPress:developfrom
faisalahammad:feature/363-delete-notification

Conversation

@faisalahammad

Copy link
Copy Markdown

What?

Adds DELETE /wp-notifications/v1/notifications/{id}. It removes the current user's copy of a notification.

Fixes #363

Why?

#363 asks for an endpoint to delete a single notification by ID. As discussed in #612, the {id} is a message ID and a notification is a message for one user. So delete only removes that user's row in notifications_queue. Other users who got the same message keep theirs.

How?

  • Notification_Repository gets a new remove( int $message_id, int $user_id ): bool method. This is the same signature as in fix: make notification repository user-scoped #626, so the two should merge easily.
  • Wpdb_Notification_Repository::remove() runs $wpdb->delete() on notifications_queue with both message_id and user_id in the where clause.
  • Notification_Controller now takes an optional repository in the constructor. It falls back to Wpdb_Notification_Repository, so load.php does not change. This also makes it easy to swap in Instantiate the notification repository and inject it into REST controllers #617 later.
  • New route notifications/(?P<id>[\d]+) with WP_REST_Server::DELETABLE:
    • logged out: 401 rest_notifications_login_required
    • no row removed (wrong ID or another user's notification): 404 rest_notification_invalid_id. Same error for both, so it does not tell if another user has that message.
    • success: 200 { "deleted": true, "id": 42 }

Orphaned messages (no queue rows left) are not removed here. That is for purge_orphans() later.

Testing Instructions

There is no UI yet to send a notification, so add queue rows by hand. Change the wp_ prefix if needed.

  1. Run npx wp-env run cli wp db query "INSERT INTO wp_notifications_queue (message_id, user_id) VALUES (101, 1), (101, 2), (102, 2);"
  2. Log in as user 1, open any wp-admin page and the browser console.
  3. Run await wp.apiFetch( { path: '/wp-notifications/v1/notifications/101', method: 'DELETE' } );
    You get { deleted: true, id: 101 }. Row (101, 1) is gone, (101, 2) is still there.
  4. Run the same call again. You get rest_notification_invalid_id with status 404.
  5. Run it with /notifications/102 (owned by user 2). You get 404 and row (102, 2) is still there.
  6. Log out and run curl -i -X DELETE http://localhost:8888/wp-json/wp-notifications/v1/notifications/101. You get 401 and no rows are removed.

Automated tests: 4 new tests in test-notification-controller.php cover these cases. PHPUnit passes on single site and multisite, and composer lint is clean.

npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications vendor/bin/phpunit

Screenshots or screencast

REST only, no UI changes.

Remove the current user's copy of a notification. The message
itself is kept for other users. Returns 401 when logged out and
404 when the user has no such notification.

Fixes WordPress#363

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MVP: Notification Controller: DELETE /notifications/{id}

1 participant