Merge pull request #23546 from netbox-community/23534-job-completion-fails-when-the-owner-already-has-a

Fixes #23534: Prevent job completion failures caused by existing notifications

netbox/core/models/jobs.py

@@ -277,11 +277,18 @@ class Job(models.Model):
                 self.notifications == JobNotificationChoices.NOTIFICATION_ALWAYS or
                 status != JobStatusChoices.STATUS_COMPLETED
             ):
-                Notification(
+                # A Job may be terminated more than once, so update any earlier notification in place
+                Notification.objects.update_or_create(
+                    object_type=ObjectType.objects.get_for_model(self),
+                    object_id=self.pk,
                     user=self.user,
-                    object=self,
-                    event_type=self.get_event_type(),
-                ).save()
+                    defaults={
+                        'event_type': self.get_event_type(),
+                        'object_repr': Notification.get_object_repr(self),
+                        'created': self.completed,
+                        'read': None,
+                    },
+                )
 
         # Send signal
         job_end.send(self)

netbox/core/tests/test_models.py

@@ -16,6 +16,7 @@ from django.utils import timezone
 
 from core.choices import DataSourceStatusChoices, JobNotificationChoices, JobStatusChoices, ObjectChangeActionChoices
 from core.data_backends import LocalBackend
+from core.events import JOB_COMPLETED, JOB_FAILED
 from core.exceptions import SyncError
 from core.models import AutoSyncRecord, DataFile, DataSource, Job, ObjectType
 from core.signals import post_sync
@@ -370,7 +371,7 @@ class ObjectTypeTestCase(TestCase):
 
 class JobTestCase(TestCase):
 
-    def _make_job(self, user, notifications):
+    def _make_job(self, user, notifications, **kwargs):
         """
         Create and return a persisted Job with the given user and notifications setting.
         """
@@ -380,6 +381,7 @@ class JobTestCase(TestCase):
             user=user,
             notifications=notifications,
             status=JobStatusChoices.STATUS_RUNNING,
+            **kwargs,
         )
 
     @patch('core.models.jobs.django_rq.get_queue')
@@ -587,6 +589,26 @@ class JobTestCase(TestCase):
                     msg=f"Expected no notification for status={status} with notifications=never",
                 )
 
+    @patch('core.models.jobs.job_end')
+    def test_terminate_updates_existing_notification(self, mock_job_end):
+        """Terminating a Job re-issues its owner's existing notification as unread."""
+        user = User.objects.create_user(username='notification-existing')
+        datasource = DataSource.objects.create(name='Data Source 1', type='local')
+        job = self._make_job(user, JobNotificationChoices.NOTIFICATION_ALWAYS, object=datasource)
+        previous_time = timezone.now() - timedelta(days=1)
+        notification = Notification.objects.create(user=user, object=job, event_type=JOB_FAILED, read=previous_time)
+        Notification.objects.filter(pk=notification.pk).update(created=previous_time)
+
+        job.name = 'Renamed Job'
+        job.terminate()
+
+        notification.refresh_from_db()
+        self.assertEqual(notification.event_type, JOB_COMPLETED)
+        self.assertEqual(notification.object_repr, 'Renamed Job')
+        self.assertEqual(notification.created, job.completed)
+        self.assertIsNone(notification.read)
+        mock_job_end.send.assert_called_once_with(job)
+
     @patch('core.models.jobs.job_end')
     def test_execution_time_set_on_terminate(self, mock_job_end):
         """

netbox/netbox/tests/test_jobs.py

@@ -9,10 +9,12 @@ from django.utils import timezone
 from django_rq import get_queue
 
 from core.choices import JobStatusChoices
+from core.events import JOB_COMPLETED, JOB_FAILED
 from core.exceptions import JobFailed
 from core.models import DataSource, Job
 from dcim.models import Site
 from dcim.views import SiteBulkEditView
+from extras.models import Notification
 from users.models import User
 from utilities.request import copy_safe_request
 from utilities.testing import disable_warnings
@@ -87,6 +89,20 @@ class JobRunnerTestCase(BaseJobRunnerTestCase):
 
         self.assertEqual(job.status, JobStatusChoices.STATUS_FAILED)
 
+    def test_handle_existing_notification(self):
+        """A Job whose owner already has a notification for it completes."""
+        user = User.objects.create_user(username='job-existing-notification')
+        job = Job.objects.create(name=TestJobRunner.name, job_id=uuid.uuid4(), user=user)
+        notification = Notification.objects.create(user=user, object=job, event_type=JOB_FAILED)
+
+        TestJobRunner.handle(job)
+
+        job.refresh_from_db()
+        notification.refresh_from_db()
+        self.assertEqual(job.status, JobStatusChoices.STATUS_COMPLETED)
+        self.assertEqual(job.error, '')
+        self.assertEqual(notification.event_type, JOB_COMPLETED)
+
     def test_handle_errored(self):
         class ErroredJobRunner(TestJobRunner):
             EXP = Exception('Test error')