Automattic / Automattic/newspack-rolling-coverage

Push sends leave no record of what was sent, and audience scoping has no fallback

Open
#35 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

The push send path reports success without checking anything, and the only thing that narrows a notification to one coverage's followers is a filter callback with no fallback. Neither is broken today, but together they mean a send that reached nobody, a send that reached everybody, and a send that worked are indistinguishable from the WordPress side.

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

## Nothing records what was sent

`onesignal_create_notification()` returns `void`, performs a synchronous `wp_remote_post()`, and returns early without sending whenever `REST_REQUEST` is defined:

```php
// onesignal-free-web-push-notifications/v3/onesignal-notification.php:131-132
if (defined('REST_REQUEST') && REST_REQUEST) return;
$response = wp_remote_post('https://onesignal.com/api/v1/notifications', $args);
```

`notify_coverage_subscribers()` returns `true` regardless (`includes/class-push-notifications.php:340`), and nothing is logged. Returning an HTTP 400 from the API and publishing an entry produced a normal editor save with no trace anywhere.

That result now feeds a decision: `maybe_notify()` deletes the stored intent when `$sent` is true, and `$sent` is true even when the request failed.

Logging each attempt with the coverage term id, entry id, resolved URL, and outcome would cost little, and is what any later question about this feature would have to be answered from.

## The call blocks the editor's save

The send runs inside the meta-box POST, so a OneSignal timeout stalls the editor's save for the full `wp_remote_post` timeout, multiplied per coverage by the loop. A one-off `wp_schedule_single_event` would take it off the editor's critical path, and would pair naturally with recording the outcome.

## Audience scoping has no fallback

OneSignal builds the payload as a send-to-everyone request before any filter runs:

```php
// onesignal-free-web-push-notifications/v3/onesignal-notification.php:44,73
$segment = $notification_options['segment'] ?? 'All';
'included_segments' => array($segment),
```

and applies the filter only when one happens to be registered (`:125`). `override_notification_fields()` unsets `included_segments` and swaps in the tag filter, and nothing between that and `wp_remote_post()` checks the narrowing took effect.

This works correctly on OneSignal 3.9.2. The captured payload scopes as intended:

```json
{ "included_segments": null,
"filters": [ { "field": "tag", "key": "coverage_15", "relation": "=", "value": "1" } ] }
```

It is worth raising only because the fallback state is "notify every push subscriber on the site" rather than "notify nobody", and the send reports success either way. Passing an explicit `'segment'` that is not a real audience name would make a missed filter fail closed instead.

Two related notes on the same callback:

- Both parameters are required and `$post_id` is typed `int`. OneSignal's v2 branch still applies this hook with four arguments and a string second argument (`v2/onesignal-admin.php:889`), which would raise a `TypeError` inside `wp_insert_post()`, where the editor sees only "Updating failed". Defaulting `$post_id = 0` and dropping the hint removes that.
- `add_filter()`, the send, and `remove_filter()` plus the static reset run unguarded (`includes/class-push-notifications.php:326-338`). A Throwable from third-party code on any hook the send touches leaves the callback registered with a stale `$pending_tag`, so a later OneSignal send in the same request would be retargeted at the wrong coverage. `Rolling_Coverage_Block::render_block()` already uses `try`/`finally` for scoped state; the same shape fits here.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by tracing onesignal_create_notification() in onesignal-free-web-push-notifications/v3/onesignal-notification.php and notify_coverage_subscribers()/maybe_notify() in includes/class-push-notifications.php. Review the v2 hook call in v2/onesignal-admin.php and the try/finally pattern in Rolling_Coverage_Block::render_block(). Done should be defined around recording outcomes, preventing editor blocking, failing closed for audience scoping, and cleaning up scoped callback state.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
api, backend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.