Explorar el Código

fix(core): Update an existing job notification

Job.terminate() inserted a new notification for the job's owner, which
violated the unique constraint when the owner already had one. A
completed job then ended as errored and job_end was never sent. It now
updates that notification with the new outcome and marks it unread.

Fixes #23534
Martin Hauser hace 5 días
padre
commit
34380bc3f0
Se han modificado 3 ficheros con 50 adiciones y 5 borrados
  1. 11 4
      netbox/core/models/jobs.py
  2. 23 1
      netbox/core/tests/test_models.py
  3. 16 0
      netbox/netbox/tests/test_jobs.py

+ 11 - 4
netbox/core/models/jobs.py

@@ -270,11 +270,18 @@ class Job(models.Model):
                 self.notifications == JobNotificationChoices.NOTIFICATION_ALWAYS or
                 self.notifications == JobNotificationChoices.NOTIFICATION_ALWAYS or
                 status != JobStatusChoices.STATUS_COMPLETED
                 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,
                     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
         # Send signal
         job_end.send(self)
         job_end.send(self)

+ 23 - 1
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.choices import DataSourceStatusChoices, JobNotificationChoices, JobStatusChoices, ObjectChangeActionChoices
 from core.data_backends import LocalBackend
 from core.data_backends import LocalBackend
+from core.events import JOB_COMPLETED, JOB_FAILED
 from core.exceptions import SyncError
 from core.exceptions import SyncError
 from core.models import AutoSyncRecord, DataFile, DataSource, Job, ObjectType
 from core.models import AutoSyncRecord, DataFile, DataSource, Job, ObjectType
 from core.signals import post_sync
 from core.signals import post_sync
@@ -355,7 +356,7 @@ class ObjectTypeTestCase(TestCase):
 
 
 class JobTestCase(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.
         Create and return a persisted Job with the given user and notifications setting.
         """
         """
@@ -365,6 +366,7 @@ class JobTestCase(TestCase):
             user=user,
             user=user,
             notifications=notifications,
             notifications=notifications,
             status=JobStatusChoices.STATUS_RUNNING,
             status=JobStatusChoices.STATUS_RUNNING,
+            **kwargs,
         )
         )
 
 
     @patch('core.models.jobs.django_rq.get_queue')
     @patch('core.models.jobs.django_rq.get_queue')
@@ -471,6 +473,26 @@ class JobTestCase(TestCase):
                     msg=f"Expected no notification for status={status} with notifications=never",
                     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')
     @patch('core.models.jobs.job_end')
     def test_execution_time_set_on_terminate(self, mock_job_end):
     def test_execution_time_set_on_terminate(self, mock_job_end):
         """
         """

+ 16 - 0
netbox/netbox/tests/test_jobs.py

@@ -8,10 +8,12 @@ from django.utils import timezone
 from django_rq import get_queue
 from django_rq import get_queue
 
 
 from core.choices import JobStatusChoices
 from core.choices import JobStatusChoices
+from core.events import JOB_COMPLETED, JOB_FAILED
 from core.exceptions import JobFailed
 from core.exceptions import JobFailed
 from core.models import DataSource, Job
 from core.models import DataSource, Job
 from dcim.models import Site
 from dcim.models import Site
 from dcim.views import SiteBulkEditView
 from dcim.views import SiteBulkEditView
+from extras.models import Notification
 from users.models import User
 from users.models import User
 from utilities.request import copy_safe_request
 from utilities.request import copy_safe_request
 from utilities.testing import disable_warnings
 from utilities.testing import disable_warnings
@@ -86,6 +88,20 @@ class JobRunnerTestCase(BaseJobRunnerTestCase):
 
 
         self.assertEqual(job.status, JobStatusChoices.STATUS_FAILED)
         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):
     def test_handle_errored(self):
         class ErroredJobRunner(TestJobRunner):
         class ErroredJobRunner(TestJobRunner):
             EXP = Exception('Test error')
             EXP = Exception('Test error')