Просмотр исходного кода

Revert the DurationColumn extension and export execution_time verbatim

JobTable defines both render_execution_time() and value_execution_time(), so
django-tables2 never invoked DurationColumn for that column and the new
timedelta branch was unreachable and untested. Restore the column to its
minutes-only form and use a plain Column, which is what the table was
effectively getting anyway.

The export path also passed through the render path's clamping, so an
anomalous negative execution_time was normalized to zero in the one output
intended for analysis, and a running job's provisional elapsed time was
indistinguishable from a completed job's final value. Export the recorded
value verbatim and leave the still-running distinction to the UI.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Jeremy Stretch 2 недель назад
Родитель
Сommit
c9185e1eb7
3 измененных файлов с 49 добавлено и 26 удалено
  1. 6 4
      netbox/core/tables/jobs.py
  2. 34 12
      netbox/core/tests/test_tables.py
  3. 9 10
      netbox/netbox/tables/columns.py

+ 6 - 4
netbox/core/tables/jobs.py

@@ -45,7 +45,7 @@ class JobTable(NetBoxTable):
     completed = columns.DateTimeColumn(
         verbose_name=_('Completed'),
     )
-    execution_time = columns.DurationColumn(
+    execution_time = tables.Column(
         verbose_name=_('Execution Time'),
         # Render running jobs (which have no recorded execution time yet) rather than the placeholder
         empty_values=(),
@@ -87,10 +87,12 @@ class JobTable(NetBoxTable):
         return value
 
     def value_execution_time(self, record):
-        # Export the raw number of seconds rather than the humanized rendering
-        if (duration := record.elapsed_time) is None:
+        # Export the recorded execution time verbatim, as a raw number of seconds. A running job's
+        # provisional elapsed time is deliberately omitted, as is the clamping of anomalous negative
+        # values applied when rendering: an export is intended for analysis.
+        if record.execution_time is None:
             return None
-        return round(max(duration.total_seconds(), 0), 3)
+        return round(record.execution_time.total_seconds(), 3)
 
     def order_execution_time(self, queryset, is_descending):
         # Order by the value the column actually displays, so that a long-running job is not sorted

+ 34 - 12
netbox/core/tests/test_tables.py

@@ -48,6 +48,10 @@ class JobExecutionTimeColumnTestCase(TestCase):
                 name='negative', job_id=uuid.uuid4(), status=JobStatusChoices.STATUS_COMPLETED,
                 started=now, completed=now, execution_time=timedelta(seconds=-5),
             ),
+            Job(
+                name='unrecorded', job_id=uuid.uuid4(), status=JobStatusChoices.STATUS_COMPLETED,
+                started=now - timedelta(seconds=90), completed=now,
+            ),
             Job(
                 name='running', job_id=uuid.uuid4(), status=JobStatusChoices.STATUS_RUNNING,
                 started=now - timedelta(minutes=5),
@@ -87,28 +91,46 @@ class JobExecutionTimeColumnTestCase(TestCase):
         row = next(iter(table.rows))
         self.assertEqual(str(row.get_cell('execution_time')), table.default)
 
-    def test_export_value_is_raw_seconds(self):
-        table = JobTable(Job.objects.filter(name='completed-90s'))
+    def test_render_job_completed_without_execution_time(self):
+        # A completed job with no recorded execution time shows the placeholder, rather than
+        # accruing time since it started
+        table = JobTable(Job.objects.filter(name='unrecorded'))
         table.columns.show('execution_time')
-        rows = list(table.as_values())
-        index = rows[0].index('Execution Time')
-        self.assertEqual(rows[1][index], 90.0)
+        row = next(iter(table.rows))
+        self.assertEqual(str(row.get_cell('execution_time')), table.default)
 
-    def test_export_value_of_job_never_started(self):
-        table = JobTable(Job.objects.filter(name='pending'))
+    def _export_value(self, name):
+        table = JobTable(Job.objects.filter(name=name))
         table.columns.show('execution_time')
         rows = list(table.as_values())
-        index = rows[0].index('Execution Time')
-        self.assertIsNone(rows[1][index])
+        return rows[1][rows[0].index('Execution Time')]
+
+    def test_export_value_is_raw_seconds(self):
+        self.assertEqual(self._export_value('completed-90s'), 90.0)
+
+    def test_export_value_of_job_never_started(self):
+        self.assertIsNone(self._export_value('pending'))
+
+    def test_export_value_of_running_job(self):
+        # Only the recorded execution time is exported; a running job has none yet
+        self.assertIsNone(self._export_value('running'))
+
+    def test_export_value_is_not_clamped(self):
+        # An anomalous negative value is clamped when rendered, but exported verbatim so that it
+        # remains visible to analysis
+        self.assertEqual(self._export_value('negative'), -5.0)
 
     def test_ordering_matches_displayed_values(self):
         """
         Sorting must order by the value the column displays — which for a running job is its elapsed
-        time, not a null — so that a long-running job is not buried. Jobs which never started sort
-        last in both directions.
+        time, not a null — so that a long-running job is not buried. Jobs with no elapsed time to
+        display sort last in both directions, in pk order.
         """
         # 'running' has been going 5 minutes, so it sorts between the 90s and 2d3h jobs
         ascending = ['negative', 'completed-subsecond', 'completed-90s', 'running', 'completed-long']
+        # Neither a job which never started nor one which completed without recording an execution
+        # time has a value to sort by
+        no_value = ['unrecorded', 'pending']
 
         for descending, expected in (
             (False, ascending),
@@ -119,7 +141,7 @@ class JobExecutionTimeColumnTestCase(TestCase):
                 queryset, modified = table.columns['execution_time'].order(Job.objects.all(), descending)
                 self.assertTrue(modified)
                 names = list(queryset.values_list('name', flat=True))
-                self.assertEqual(names, expected + ['pending'])
+                self.assertEqual(names, expected + no_value)
 
     def test_ordering_breaks_ties_on_pk(self):
         """

+ 9 - 10
netbox/netbox/tables/columns.py

@@ -1,6 +1,5 @@
 import zoneinfo
 from dataclasses import dataclass
-from datetime import timedelta
 from urllib.parse import quote
 
 import django_tables2 as tables
@@ -23,7 +22,6 @@ from extras.choices import CustomFieldTypeChoices
 from utilities.object_types import object_type_identifier, object_type_name
 from utilities.permissions import get_permission_for_model
 from utilities.request import get_safe_request_context
-from utilities.string import humanize_duration
 from utilities.templatetags.builtins.filters import render_markdown
 from utilities.validators import url_scheme_is_allowed
 from utilities.views import get_action_url
@@ -114,16 +112,17 @@ class DateTimeColumn(tables.Column):
 
 class DurationColumn(tables.Column):
     """
-    Express a duration of time in a human-friendly format. Accepts either a timedelta or a count of
-    minutes. Example: 437 minutes becomes "7h 17m"
+    Express a duration of time (in minutes) in a human-friendly format. Example: 437 minutes becomes "7h 17m"
     """
     def render(self, value):
-        if not isinstance(value, timedelta):
-            if not value:
-                # A zero count of minutes renders as empty rather than "0s"
-                return ''
-            value = timedelta(minutes=value)
-        return humanize_duration(value)
+        ret = ''
+        if days := value // 1440:
+            ret += f'{days}d '
+        if hours := value % 1440 // 60:
+            ret += f'{hours}h '
+        if minutes := value % 60:
+            ret += f'{minutes}m'
+        return ret.strip()
 
     def value(self, value):
         return value