Automattic / Automattic/newspack-rolling-coverage
Push notifications repeat on every save when the OneSignal send fails
- Dominant language
- PHP
- Stars
- 1
- Forks
- 1
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 3
Description
## Summary
The duplicate-send guard added in [#18](https://github.com/Automattic/newspack-rolling-coverage/pull/18) keys on `os_notification_id`, the post meta OneSignal writes after a send. OneSignal writes that meta only on an HTTP 200 response carrying a notification id, so **every failed send leaves it unset and the guard never engages**. On a site where sends are failing, publishing an entry and then saving it again re-sends to the coverage's followers on each save.
The guard records OneSignal's success, not this plugin's attempt, so it covers the happy path and nothing else.
From the [#18](https://github.com/Automattic/newspack-rolling-coverage/pull/18) review.
## Symptoms
An editor ticks **Notify subscribers when this entry publishes**, publishes, and then keeps editing. The classic meta box sets `forceIsDirty`, so **Save** stays enabled on an untouched post, and Gutenberg re-posts the meta-box form on every save with the checkbox and its nonce intact. When the send succeeds this is harmless. When it fails, every save is another attempt.
Two ordinary states produce a failing send:
- A site with no push subscribers yet returns HTTP 200 with `{"errors":["All included players are not subscribed"]}` and no `id`.
- A missing or wrong REST API key returns HTTP 401.
Both are most likely during setup, which is also when an editor is most likely to publish, look at the result, and save again.
## Root cause
`maybe_notify()` returns early when the meta is present:
```php
// includes/class-push-notifications.php:235
// OneSignal sets this meta after a successful send.
if ( ! empty( get_post_meta( $post->ID, 'os_notification_id', true ) ) ) {
return;
}
```
OneSignal writes it in one place, behind two conditions:
```php
// onesignal-free-web-push-notifications/v3/onesignal-notification.php:139-144
if ($response_code === 200) {
$response_data = json_decode($response_body, true);
$notification_id = $response_data['id'] ?? '';
if (!empty($notification_id)) {
onesignal_save_notification_id($post->ID, $notification_id);
```
A `WP_Error`, a non-200 code, or a 200 whose body carries `errors` and no `id` all skip it.
`resolve_notify_intent()` does not close the gap either. On a re-save the meta-box POST is present, so it takes the `$_POST` branch, the nonce and capability checks pass, the checkbox is still ticked, and it returns `true`.
## Evidence
Reproduced on a local site running the branch at `3ee413d`, with the OneSignal API intercepted so the response could be controlled. Each row is one publish followed by two further saves with no edits in between:
| Response | `os_notification_id` | Sends after publish + 2 saves |
| --- | --- | --- |
| 200 with an `id` | set | 1 — guard holds |
| 200 with `errors`, no `id` | never set | **3** |
| 401 | never set | **2** (one publish, one save) |
## Options
**A — Record the attempt before making it.** Write a plugin-owned meta key immediately before calling `onesignal_create_notification()`, and bail when it is already present. The value then reflects "we tried for this entry" rather than "OneSignal accepted it", which is what the guard needs. Retrying after a genuine failure becomes an explicit action rather than a side effect of saving.
**B — Consume the stored intent before sending.** `delete_post_meta( $post->ID, self::NOTIFY_META_KEY )` before the call rather than after, and treat the intent as spent whatever the outcome. Smaller change, but it loses the record of which entries were attempted.
Either way, `NOTIFY_META_KEY` is worth registering with `register_post_meta()` now that it carries real weight. The entry post type declares `custom-fields` support (`includes/class-post-type.php:127`), so the key currently shows up as a hand-editable row in the Custom Fields box.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in includes/class-push-notifications.php at maybe_notify() and resolve_notify_intent(), then inspect onesignal-free-web-push-notifications/v3/onesignal-notification.php where the notification ID is saved. Compare the two proposed attempt-guard approaches with maintainers and reproduce the listed failed-response cases. Done means repeated saves do not resend after a failed attempt, while the intended successful-send guard still works.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php, wordpress
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100