Skip to content

feat(alert-manager): route alerts to Slack via newspack_log#4723

Merged
miguelpeixe merged 3 commits into
trunkfrom
feat/nppd-1443-alert-log-forward
May 15, 2026
Merged

feat(alert-manager): route alerts to Slack via newspack_log#4723
miguelpeixe merged 3 commits into
trunkfrom
feat/nppd-1443-alert-log-forward

Conversation

@miguelpeixe
Copy link
Copy Markdown
Member

@miguelpeixe miguelpeixe commented May 13, 2026

All Submissions:

Changes proposed in this Pull Request:

Adds a forward_alert_to_log listener on newspack_alert in Alert_Manager. The listener emits do_action( 'newspack_log', $code, $message, [ 'log_level' => 3, ... ] ), which Newspack Manager's Logger already routes through Jetpack to the Newspack Slack alerts channel.

Existing precedent for log_level => 3 → Slack alert is in newspack-manager's class-data-report.php:352.

Closes NPPD-1443.

How to test the changes in this Pull Request:

  1. Check out this branch on a site with newspack-manager active and connected to a hub (so newspack_log actually round-trips through Jetpack).

  2. From wp shell (or a quick snippet), fire a synthetic alert:

    do_action( 'newspack_alert', [
        'type'      => 'sync_retry_exhausted',
        'severity'  => 'error',
        'message'   => 'Synthetic test from alert-log-forward PR',
        'context'   => [ 'integration_id' => 'mailchimp' ],
        'timestamp' => time(),
    ] );
  3. Confirm a level 3 log is fired with the alert message

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@miguelpeixe miguelpeixe self-assigned this May 13, 2026
@miguelpeixe miguelpeixe added the [Status] Needs Review The issue or pull request needs to be reviewed label May 13, 2026
@miguelpeixe miguelpeixe requested a review from Copilot May 13, 2026 19:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds alert forwarding from newspack_alert to newspack_log so Newspack Manager can route alert-level logs to Slack.

Changes:

  • Registers Alert_Manager::forward_alert_to_log() on newspack_alert.
  • Emits newspack_log entries with log_level => 3.
  • Adds unit tests for forwarding and malformed alert payloads.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
includes/class-alert-manager.php Adds the alert-to-log forwarding hook and implementation.
tests/unit-tests/alert-manager.php Adds cleanup and tests for alert log forwarding behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit-tests/alert-manager.php Outdated
Comment thread includes/class-alert-manager.php Outdated
Comment thread includes/class-alert-manager.php Outdated
@miguelpeixe miguelpeixe marked this pull request as ready for review May 14, 2026 17:26
@miguelpeixe miguelpeixe requested a review from a team as a code owner May 14, 2026 17:26
@miguelpeixe miguelpeixe requested a review from chickenn00dle May 14, 2026 17:26
@chickenn00dle chickenn00dle self-assigned this May 14, 2026
Copy link
Copy Markdown
Contributor

@chickenn00dle chickenn00dle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — local checks (lint, PHPUnit for Alert_Manager: 18 tests / 72 assertions) and CI are all green. The newspack_log contract — ( $code, $message, $params ) with type ∈ error|debug and log_level ∈ 1..4 — matches Logger::log and the precedent at newspack-manager's class-data-report.php:352. Severity-to-level mapping aligns; dropping context from the forwarded params is correct (passing as data would JSON-encode reader identifiers into the log payload).

Approving — none of the inline comments are blockers, just refinements worth considering before merge.

Comment thread includes/class-alert-manager.php Outdated
Comment thread includes/class-alert-manager.php Outdated
Comment thread includes/class-alert-manager.php
Comment thread tests/unit-tests/alert-manager.php
@github-actions github-actions Bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels May 15, 2026
@miguelpeixe miguelpeixe requested a review from chickenn00dle May 15, 2026 15:25
@miguelpeixe
Copy link
Copy Markdown
Member Author

Thanks, @chickenn00dle! I've addressed the suggestions.

@miguelpeixe miguelpeixe merged commit 6e68ab6 into trunk May 15, 2026
9 checks passed
@miguelpeixe miguelpeixe deleted the feat/nppd-1443-alert-log-forward branch May 15, 2026 15:45
@github-actions
Copy link
Copy Markdown

Hey @miguelpeixe, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

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

Labels

[Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants