Skip to content

feat: notify on change - #696

Open
blaipr wants to merge 5 commits into
ctrliq:mainfrom
blaipr:feat/notification-on-change
Open

feat: notify on change#696
blaipr wants to merge 5 commits into
ctrliq:mainfrom
blaipr:feat/notification-on-change

Conversation

@blaipr

@blaipr blaipr commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Closes #206.

Problem

Notifications can be attached to a job template for start, success and failure. A hardening playbook that runs nightly in check mode succeeds whether or not it found drift, so there is nothing to notify on: the interesting outcome is that something reported as changed.

Change

Job templates and organizations gain a fourth trigger, Changed.

A run that reported a change on any host notifies the templates attached to that trigger, next to the ones for how the run ended. A job that changed something and succeeded notifies both Success and Changed; one that changed something and failed notifies Failure and Changed. Check mode counts, which is what makes the trigger useful for a compliance playbook. Start never fires it, since nothing has run yet.

  • notification_templates_changed on the job template and on the organization, with the endpoints and the related links that the other triggers have.
  • Job.has_changes() is true when any of the job's host summaries recorded a change. Ad hoc commands answer the same way, so an organization's changed templates cover them as they cover its jobs. Project updates, inventory updates, system jobs and workflow jobs record no per host results and never fire it.
  • The trigger only costs a query when changed templates are actually attached: the dispatch checks for them before asking whether the run changed anything.
  • The message is customizable like the other triggers, and defaults to {{ job_friendly_name }} #{{ job.id }} '{{ job.name }}' reported changes: {{ url }}.

In the UI the Notifications tab of a job template and of an organization gains a Changed toggle, and the notification template form gains the Changed message fields.

Workflow job templates are left out: a workflow does not run against hosts itself, and the job templates inside it each notify on their own.

Testing

  • New model tests cover: a job whose host summaries recorded no change, one that recorded a change, an ad hoc command that recorded one, the job types that never report changes, a changed job notifying both the outcome and the changed templates, a job that changed nothing notifying only the outcome, a failed job that changed something notifying both, start not firing the trigger, and an unknown status still being refused.
  • New API tests cover the two endpoints, plus that the job template aggregates its own changed templates and the ones on its organization.
  • New UI tests cover the Changed toggle rendering, its two click paths, and the list fetching and disassociating through it.
  • flake8 awx and black --check awx pass, and prettier passes on every changed file under awx/ui.

The suites themselves were not run locally, since that needs the development image; CI runs them on this pull request.

Notifications could be attached for start, success and failure, so a playbook
that runs nightly in check mode to report drift had nothing to notify on: it
succeeds whether or not it found anything.

Job templates and organizations gain a changed trigger. A run that reported a
change on any host notifies those templates next to the ones for how it ended,
so a job that changed something and succeeded notifies both. Ad hoc commands
are covered by the organization trigger the same way jobs are, and the job
types that do not record per host results never fire it.

The message for the trigger is customizable like the others, and defaults to
one that says the run reported changes.
@blaipr
blaipr force-pushed the feat/notification-on-change branch from 9a803ad to 08009df Compare August 23, 2026 21:19
@cigamit
cigamit requested a lite review from Copilot August 24, 2026 03:20
@cigamit cigamit self-assigned this Aug 24, 2026
@cigamit cigamit added the enhancement New feature or request label Aug 24, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a new Changed notification trigger for job templates and organizations, enabling notifications when a run reports changes (including check mode), alongside the existing start/success/failure triggers.

Changes:

  • Backend: introduce notification_templates_changed, a has_changes() contract, and dispatch logic to emit Changed in addition to the terminal outcome trigger.
  • API/UI: add new association endpoints and UI toggles + message customization fields for the Changed trigger.
  • Tests/docs: add functional tests for the new trigger and update user documentation.

Reviewed changes

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

Show a summary per file
File Description
docs/docsite/rst/userguide/notifications.rst Documents the new Changed notification trigger and endpoints.
awx/ui/src/screens/Template/Template.js Enables Changed toggle for job template notifications UI.
awx/ui/src/screens/Organization/Organization.js Enables Changed toggle for organization notifications UI.
awx/ui/src/screens/NotificationTemplate/shared/NotificationTemplateForm.js Adds Changed message fields to the notification template form normalization/defaulting.
awx/ui/src/screens/NotificationTemplate/shared/notification-template-default-messages.json Defines default Changed messages/bodies per notification backend type.
awx/ui/src/screens/NotificationTemplate/shared/hasCustomMessages.js Treats Changed message customizations as “custom messages”.
awx/ui/src/screens/NotificationTemplate/shared/CustomMessagesSubForm.js Adds Changed message/body editors to the custom messages subform.
awx/ui/src/components/NotificationList/NotificationListItem.test.js Adds tests for rendering/clicking the Changed toggle.
awx/ui/src/components/NotificationList/NotificationListItem.js Renders a Changed toggle in notification list rows when enabled.
awx/ui/src/components/NotificationList/NotificationList.test.js Adds tests for fetching/disassociating Changed templates in lists.
awx/ui/src/components/NotificationList/NotificationList.js Fetches Changed-associated templates and wires toggle state.
awx/ui/src/api/mixins/Notifications.mixin.js Adds client methods for Changed notification template association endpoints.
awx/main/tests/functional/models/test_notifications.py Adds model-level tests for has_changes() and changed-trigger dispatch behavior.
awx/main/tests/functional/api/test_notifications.py Adds API endpoint tests for job template/org Changed notification associations.
awx/main/notifications/webhook_backend.py Adds Changed defaults for webhook notifications.
awx/main/notifications/pagerduty_backend.py Adds Changed defaults for PagerDuty notifications.
awx/main/notifications/grafana_backend.py Adds Changed defaults for Grafana notifications.
awx/main/notifications/email_backend.py Adds Changed defaults for email notifications.
awx/main/notifications/custom_notification_base.py Introduces default Changed message/body templates.
awx/main/models/organization.py Adds notification_templates_changed M2M on Organization.
awx/main/models/notifications.py Adds has_changes() hook and dispatch logic for Changed notifications.
awx/main/models/jobs.py Adds notification_templates_changed on JobTemplate and aggregates org/jobtemplate changed templates.
awx/main/models/ad_hoc_commands.py Adds org-level Changed templates aggregation and has_changes() for ad hoc commands.
awx/main/migrations/0209_notification_templates_changed.py Migration adding Changed notification template relations.
awx/api/views/organization.py Adds Organization changed-notification templates sublist view.
awx/api/views/init.py Adds JobTemplate changed-notification templates sublist view.
awx/api/urls/organization.py Adds organization /notification_templates_changed/ route.
awx/api/urls/job_template.py Adds job template /notification_templates_changed/ route.
awx/api/serializers.py Adds related links and message validation support for the Changed event.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread awx/main/tests/functional/models/test_notifications.py Outdated
Comment thread awx/ui/src/api/mixins/Notifications.mixin.js
Comment thread awx/ui/src/api/mixins/Notifications.mixin.js
@blaipr

blaipr commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, this review earned its keep. Both findings were right, and chasing the first one turned up a real bug in the code rather than in the test. Fixed in 0e90343.

has_changes() on AdHocCommand was broken. The parametrized test flagged above was failing, and once I ran the class properly a third failure came out with it:

FAILED test_job_types_without_host_results_never_report_changes[InventoryUpdate]
FAILED test_job_types_without_host_results_never_report_changes[ProjectUpdate]
FAILED test_ad_hoc_command_with_changes
AttributeError: 'AdHocCommand' object has no attribute 'job_host_summaries'

That last one is not a test problem. I had written AdHocCommand.has_changes() as a copy of Job.has_changes(), and an ad hoc command has no JobHostSummary rows: its per host results are AdHocCommandEvent records, related as ad_hoc_command_events, and the changed flag there is a boolean rather than a count. So the method raised AttributeError for every ad hoc command, which send_notification_templates would have hit in production the moment one finished.

-        return self.job_host_summaries.filter(changed__gt=0).exists()
+        return self.ad_hoc_command_events.filter(changed=True).exists()

I also added the negative case, test_ad_hoc_command_without_changes, since only the positive one existed and neither was really running.

The parametrized test. Correct on both counts: InventoryUpdate and ProjectUpdate cannot be created from a name alone. The first thing that actually stops it is NOT NULL constraint failed: main_inventoryupdate.source, before the inventory_source foreign key even comes up. Rather than invent an inventory source and a project to reach a method that never queries anything, I dropped the database from the test:

-    @pytest.mark.django_db
     @pytest.mark.parametrize('JobClass', [InventoryUpdate, ProjectUpdate, SystemJob, WorkflowJob])
     def test_job_types_without_host_results_never_report_changes(self, JobClass):
-        assert JobClass.objects.create(name='fake-job').has_changes() is False
+        assert JobClass().has_changes() is False

These four inherit JobNotificationMixin.has_changes(), which returns False without touching the database, so an unsaved instance tests the contract exactly and the test stops depending on relations that have nothing to do with what it asserts. If one of them ever gains a real implementation, this fails loudly, which is what you want from it.

The two docstrings. Updated both to "approvals", "started", "success", "error" and "changed". Worth noting they were already stale before this pull request: approvals and started have been handled by both helpers for a while and were never documented. Now the docstring matches the branches in the function.

Tests after the change:

py.test awx/main/tests/functional/models/test_notifications.py \
        awx/main/tests/functional/api/test_notifications.py
49 passed in 39.01s

black --check and flake8 are clean on the touched Python files.

The whole suite, on a freshly created database, in a checkout linked with make awx-link:

py.test -p no:cacheprovider --create-db -n auto --dist=loadfile \
  awx/main/tests/unit awx/main/tests/functional awx/conf/tests awx/sso/tests

1 failed, 3835 passed, 10 skipped in 403.72s (0:06:43)
FAILED awx/main/tests/functional/api/test_generic.py::test_proxy_ip_allowed

That failure is unrelated: test_proxy_ip_allowed patches REMOTE_HOST_HEADERS and PROXY_IP_ALLOWED_LIST on the settings singleton and intermittently sees another worker's state under -n auto. It is the flake instrumented in #694 and it passes solo. main at e197d7cc0f reports 3817 passed on the same command, so this branch adds 18 tests and no failures.

@cigamit

cigamit commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

After this PR, it was wanting to create a new 210 migration (and ui tests are failing), so I ran this through Fable internally, and it came up with these.

Issues found

  1. Migration state mismatch — the models declare related_name='%(class)s_notification_templates_for_changed', but 0209_notification_templates_changed.py hard-coded the resolved names (jobtemplate..., organization_..._). Django's autodetector compares these literally, so makemigrations --check failed and wanted a corrective 0210 AlterField migration. I changed 0209 to use the %(class)s form. Since related_name is ORM-only metadata, this doesn't touch the DB schema — the migration you already ran is still valid, no re-run needed. makemigrations --check is now clean.

  2. 10 pre-existing UI tests brokenNotificationTemplateForm.js:161 now unconditionally reads defs.changed, but the fixture in NotificationTemplateForm.test.js didn't include a changed key, crashing the form render. I added changed: messageDef to the fixture (the real prop comes from server OPTIONS, which now always includes it — the form already hard-depends on the other keys the same way, so hardening the code instead would have been off-convention).

  3. Model default left behind — NotificationTemplate.default_messages() at notifications.py still returned only started/success/error/workflow_approval, so templates created without explicit messages exposed a messages dict missing the changed key. Harmless functionally (the send path falls back via .get('changed', {})), but inconsistent in API responses. Added 'changed': None plus the matching assertion in test_basic_parameterization. No migration impact (callable default).

diff --git a/awx/main/migrations/0209_notification_templates_changed.py b/awx/main/migrations/0209_notification_templates_changed.py
index 21bf8505..838a1301 100644
--- a/awx/main/migrations/0209_notification_templates_changed.py
+++ b/awx/main/migrations/0209_notification_templates_changed.py
@@ -10,11 +10,11 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name='jobtemplate',
             name='notification_templates_changed',
-            field=models.ManyToManyField(blank=True, related_name='jobtemplate_notification_templates_for_changed', to='main.notificationtemplate'),
+            field=models.ManyToManyField(blank=True, related_name='%(class)s_notification_templates_for_changed', to='main.notificationtemplate'),
         ),
         migrations.AddField(
             model_name='organization',
             name='notification_templates_changed',
-            field=models.ManyToManyField(blank=True, related_name='organization_notification_templates_for_changed', to='main.notificationtemplate'),
+            field=models.ManyToManyField(blank=True, related_name='%(class)s_notification_templates_for_changed', to='main.notificationtemplate'),
         ),
     ]
diff --git a/awx/main/models/notifications.py b/awx/main/models/notifications.py
index 70eaf7c3..aa5cfce3 100644
--- a/awx/main/models/notifications.py
+++ b/awx/main/models/notifications.py
@@ -73,7 +73,7 @@ class NotificationTemplate(CommonModelNameNotUnique):
     notification_configuration = prevent_search(models.JSONField(default=dict))
 
     def default_messages():
-        return {'started': None, 'success': None, 'error': None, 'workflow_approval': None}
+        return {'started': None, 'success': None, 'error': None, 'changed': None, 'workflow_approval': None}
 
     messages = models.JSONField(null=True, blank=True, default=default_messages, help_text=_('Optional custom messages for notification template.'))
 
diff --git a/awx/main/tests/functional/test_notifications.py b/awx/main/tests/functional/test_notifications.py
index cf93030a..c635bc57 100644
--- a/awx/main/tests/functional/test_notifications.py
+++ b/awx/main/tests/functional/test_notifications.py
@@ -47,7 +47,7 @@ def test_basic_parameterization(get, post, user, organization):
     assert 'url' in response.data['notification_configuration']
     assert 'headers' in response.data['notification_configuration']
     assert 'messages' in response.data
-    assert response.data['messages'] == {'started': None, 'success': None, 'error': None, 'workflow_approval': None}
+    assert response.data['messages'] == {'started': None, 'success': None, 'error': None, 'changed': None, 'workflow_approval': None}
 
 
 @pytest.mark.django_db
diff --git a/awx/ui/src/screens/NotificationTemplate/shared/NotificationTemplateForm.test.js b/awx/ui/src/screens/NotificationTemplate/shared/NotificationTemplateForm.test.js
index d293d335..46ced336 100644
--- a/awx/ui/src/screens/NotificationTemplate/shared/NotificationTemplateForm.test.js
+++ b/awx/ui/src/screens/NotificationTemplate/shared/NotificationTemplateForm.test.js
@@ -66,6 +66,7 @@ const defaults = {
   started: messageDef,
   success: messageDef,
   error: messageDef,
+  changed: messageDef,
   workflow_approval: {
     approved: messageDef,
     denied: messageDef,

@cigamit cigamit added the Needs triage When a Issue needs to be researched or a PR has an issue that needs fixing before merging label Aug 26, 2026
blaipr added 2 commits August 26, 2026 13:48
Three things came out of the review:

- 0209 hard-coded the resolved related_name values, jobtemplate_... and
  organization_..., while the models declare the %(class)s form. Django's
  autodetector compares them literally, so makemigrations wanted a corrective
  0210 AlterField. related_name is ORM-only metadata, so the migration as
  already applied stays valid and the schema does not change.
- NotificationTemplateForm reads defs.changed the way it reads the other
  message keys, and the fixture in NotificationTemplateForm.test.js had no
  changed key, so the form crashed on render and took 10 pre-existing tests
  with it. The real prop comes from server OPTIONS, which now always includes
  it, so the fixture is what was stale.
- NotificationTemplate.default_messages() still returned only started, success,
  error and workflow_approval, so a template created without explicit messages
  exposed a messages dict with no changed key. The send path falls back through
  .get('changed', {}), so nothing broke, but the API response was inconsistent.
@blaipr

blaipr commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, all three were right. Applied in 5dbfcda, and the branch now carries current main.

Migration state mismatch. Confirmed and fixed. I reproduced it before and after: with the hard-coded names, check_migrations wants a corrective 0210 and exits 1.

Migrations for 'main':
  awx/main/migrations/0210_missing_migration_file.py
    ~ Alter field notification_templates_changed on jobtemplate
    ~ Alter field notification_templates_changed on organization

With the %(class)s form it is No changes detected. That is also the convention the rest of the tree already uses: 0184 writes %(class)s_notification_templates_for_errors and %(class)s_notification_templates_for_success for the same relations. Worth noting api-migrations stayed green through this, since test_migrations never runs the autodetector check, so it only surfaces via make test.

The 10 UI tests. Same call, and for the same reason: NotificationTemplateForm reads defs.changed exactly the way it reads defs.started, so the fixture is what was stale, not the code. NotificationTemplateForm.test.js is 10 passed now, and the wider notification UI is 8 suites, 55 passed.

default_messages(). Added 'changed': None and the matching assertion. Agreed it is cosmetic given the .get('changed', {}) fallback, but the OPTIONS response and the stored default should not disagree.

Verification after the merge:

awx-manage check_migrations --dry-run --check -n missing_migration_file
No changes detected

py.test awx/main/tests/functional/test_notifications.py \
        awx/main/tests/functional/models/test_notifications.py \
        awx/main/tests/functional/api/test_notifications.py
60 passed in 38.66s

npx jest src/components/NotificationList src/screens/NotificationTemplate
Test Suites: 8 passed, Tests: 55 passed

black --check and flake8 are clean on the touched Python files, and prettier is clean on the touched JS.

@cigamit

cigamit commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Main has a new migration file, so will need to bump this one up to 210 now, then it should be good to merge.

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

Labels

enhancement New feature or request Needs triage When a Issue needs to be researched or a PR has an issue that needs fixing before merging

Development

Successfully merging this pull request may close these issues.

Notification on Change

3 participants