Automattic / Automattic/newspack-rolling-coverage

No test coverage for the push-notification publish path

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

`tests/` holds `bootstrap.php` and `test-bootstrap.php` and nothing else. The push-notification publish path has no coverage, and it is the part of this feature where a mistake reaches readers' devices and cannot be recalled.

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

## Why here first

Every defect found reviewing [#18](https://github.com/Automattic/newspack-rolling-coverage/pull/18) sat in publish-transition plumbing — fires when it should not, silently does not fire, fires twice. That behaviour is awkward to hold in your head and cheap to assert, which is the combination unit tests are good at. It is also the area most likely to shift again: the send is now gated on a mix of `$_POST`, stored intent, and a third-party meta key, and each of those moved during review.

## Cases worth pinning

Send gating, in `Push_Notifications`:

1. Publishing with the checkbox ticked sends once.
2. A second `wp_update_post()` carrying the same `$_POST` does not send again — including when the previous send failed and wrote no `os_notification_id`.
3. A scheduled entry published by cron, with intent stored, sends once.
4. A programmatic publish with no stored intent does not send. `Entry_Ingestion_Service::ingest()` inserts entries directly at `publish` when auto-publish is on, and `Breakout::on_breakout_post_status_change()` calls `wp_update_post()` on an already-published entry; neither should notify.
5. A coverage with no canonical URL does not send.
6. `is_onesignal_configured() === false` no-ops cleanly.

Payload shape, in `override_notification_fields()`:

7. Running the callback over OneSignal's default payload removes `included_segments` and adds the tag filter. This pins the third-party contract, so the next OneSignal upgrade fails a test rather than a publisher's send.

Sanitization, in `Taxonomy::sanitize_canonical_url()`:

8. Same-host URLs pass; off-site hosts, `javascript:` and malformed input are rejected.

## Notes

Cases 1 to 6 need the OneSignal API stubbed rather than called. Filtering `pre_http_request` on `onesignal.com` is enough, and lets a test assert how many requests were made and what they contained — which is how the duplicate-send behaviour was confirmed during review.

A global `WC()`-style mock is not needed here; the only third-party surface is `onesignal_create_notification()` and the `OneSignalWPSetting` option.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with tests/bootstrap.php and test-bootstrap.php, then trace the named Push_Notifications, Entry_Ingestion_Service, Breakout, and Taxonomy methods. Stub OneSignal through pre_http_request and add tests for the six publish cases, payload filtering in override_notification_fields(), and canonical URL sanitization. Done means the specified sends, no-ops, payload shape, and URL acceptance or rejection are asserted without calling OneSignal.

Written by the indexing model from the issue text.

Assessment

Tech stack
php, wordpress
Domain
backend, testing
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.