-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AdSense notifications no longer surface alerts #7559
Comments
IB ✅ |
Fix AdSense critical alerts not surfacing on SK Dashboard
QA Update:
|
Hi @wpdarren, those are valid observation. The notice does look a bit out of place and visually inconsistent with the current SK Dashboard, which is not surprising as it was designed for a different time. However, I'd say those are out of scope for this ticket as it was about getting the alerts back. We can and should definitely open a new issue to improve and polish the AdSense alert further. Cheers. |
QA Update:
|
@wpdarren you're correct, only the first alert is being shown. It looks like it was always this way, but I agree we should fix it so that each alert returned is shown. I gave this a quick test and it seems to work as expected if all the alerts are returned, we just need to ensure they have unique IDs which is what the dismissal references. Since this should be a simple change, I think we can include it as a follow-up here rather than as a separate issue. |
Moving this back to CR with a quick follow-up PR shown above. |
QA Update: ✅Verified:
Note: will be creating a ticket to highlight the UX/UI issues reported previously. alerts.mp4 |
Bug Description
The AdSense module is the only module which currently implements the
notifications
datapoint which is used for surfacing module-specific notifications on the dashboard automatically.This is currently intended to surface any "severe" alerts from AdSense via the API but does not work as expected.
Steps to reproduce
Screenshots
Example severe alert
Expected notification
Additional Context
This has not worked properly for a long time, so we may want to make some stylistic enhancements while we're at it.
Example response from the AdSense API with various alerts
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
The main problem here is that
GET:notifications
callsGET:alerts
internally which needs theaccountID
. If not provided, it falls back to theaccountID
in the module's settings which is relied upon in the case of thenotifications
endpoint.site-kit-wp/includes/Modules/AdSense.php
Lines 327 to 335 in 109791e
This fails however, even when an
accountID
is configured in the settings because$data
is aData_Request
instance, which is immutable, so the assignment is a no-op. This results in the request failing as aWP_Error
which is then silently ignored in thecore/modules
handler.site-kit-wp/includes/Core/Modules/REST_Modules_Controller.php
Lines 398 to 405 in 109791e
GET:alerts
to requireaccountID
in all cases by removing the fallback to settingsGET:notifications
to always passaccountID
from settings, returning early with an empty array if none is setformat: large
from mapping (invalid value anyways)NotificationAlertSVG
inAdSenseAlerts
to better align with other notifications (like module connect success)Test Coverage
alerts
andnotifications
datapointsQA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: