Automattic / Automattic/newspack-rolling-coverage

An entry in two coverages cancels its own first notification

Open
#33 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
1
Forks
1
Avg merge
2d 18h
Merged PRs (30d)
3

Description

## Summary

An entry can belong to more than one coverage, and `maybe_notify()` sends once per coverage term. `onesignal_create_notification()` opens by cancelling whatever notification id is stored on the post and then overwrites it, so **the second coverage's send cancels the first coverage's notification** moments after it was created.

From the [#18](https://github.com/Automattic/newspack-rolling-coverage/pull/18) review.

## Symptoms

An entry filed under two coverages notifies only one set of followers reliably. Followers of the first coverage can receive nothing, while the editor sees a success notice for the send. Because only the last id is stored, a later cancel-on-trash can undo one of the two sends at most.

Both notifications also carry the same `web_push_topic` collapse key, derived from the entry id, so a reader following both coverages sees one notification rather than two.

## Root cause

The loop sends per term:

```php
// includes/class-push-notifications.php:255
foreach ( $term_ids as $term_id ) {
if ( self::notify_coverage_subscribers( (int) $term_id, $post ) ) {
$sent = true;
}
}
```

OneSignal cancels and overwrites on each call:

```php
// onesignal-free-web-push-notifications/v3/onesignal-notification.php:32-35
$existing_notification_id = onesignal_get_notification_id($post->ID);
if (!empty($existing_notification_id)) {
onesignal_cancel_notification($existing_notification_id);
}
```

```php
// onesignal-free-web-push-notifications/v3/onesignal-notification.php:74
'web_push_topic' => sanitize_title('post-' . $post->ID),
```

The `os_notification_id` guard added in #18 sits at the top of `maybe_notify()`, before the loop, so it does not affect iterations within a single publish.

## Evidence

Publishing one entry filed under two coverages, with the OneSignal API intercepted, produced this call sequence:

```
1. POST filters: tag = coverage_15
2. DELETE <- cancels the notification from step 1
3. POST filters: tag = coverage_16
```

OneSignal's cancel endpoint stops in-flight fan-out, so this is not only a bookkeeping problem.

## Options

**A — Send for one coverage only.** Pick the entry's primary coverage and notify its followers. Simplest, and matches how the canonical URL already works, since every coverage on the entry resolves to its own landing page.

**B — Clear the stored id between iterations.** `delete_post_meta( $post->ID, 'os_notification_id' )` before each call, so OneSignal has nothing to cancel. Keeps multi-coverage sends working, at the cost of depending on a third-party meta key and leaving only the last id recoverable.

The collapse key is OneSignal's own and cannot be overridden through `onesignal_send_notification`, so option B still delivers one visible notification to a reader who follows both.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in includes/class-push-notifications.php at maybe_notify() and then inspect onesignal-free-web-push-notifications/v3/onesignal-notification.php, especially the cancellation, stored ID, and web_push_topic handling. Reproduce the two-coverage publish with the OneSignal API intercepted, select one of the stated options, and verify the resulting sends and later cancellation behavior match that choice.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.