PR review updates

netbox/extras/api/serializers.py

@@ -258,11 +258,7 @@ class JobResultSerializer(serializers.ModelSerializer):
 #
 
 class ReportSerializer(serializers.Serializer):
-    url = serializers.HyperlinkedIdentityField(
-        view_name='extras-api:report-detail',
-        lookup_field='full_name',
-        lookup_url_kwarg='pk'
-    )
+    id = serializers.CharField(read_only=True, source="full_name")
     module = serializers.CharField(max_length=255)
     name = serializers.CharField(max_length=255)
     description = serializers.CharField(max_length=255, required=False)
@@ -279,12 +275,8 @@ class ReportDetailSerializer(ReportSerializer):
 #
 
 class ScriptSerializer(serializers.Serializer):
-    url = serializers.HyperlinkedIdentityField(
-        view_name='extras-api:script-detail',
-        lookup_field='full_name',
-        lookup_url_kwarg='pk'
-    )
     id = serializers.CharField(read_only=True, source="full_name")
+    module = serializers.CharField(max_length=255)
     name = serializers.CharField(read_only=True)
     description = serializers.CharField(read_only=True)
     vars = serializers.SerializerMethodField(read_only=True)

netbox/extras/api/views.py

@@ -14,7 +14,7 @@ from extras.choices import JobResultStatusChoices
 from extras.models import (
     ConfigContext, CustomFieldChoice, ExportTemplate, Graph, ImageAttachment, ObjectChange, JobResult, Tag,
 )
-from extras.reports import get_report, get_reports
+from extras.reports import get_report, get_reports, run_report
 from extras.scripts import get_script, get_scripts, run_script
 from utilities.api import IsAuthenticatedOrLoginNotRequired, ModelViewSet
 from utilities.utils import copy_safe_request

netbox/extras/choices.py

@@ -129,25 +129,23 @@ class JobResultStatusChoices(ChoiceSet):
     STATUS_PENDING = 'pending'
     STATUS_RUNNING = 'running'
     STATUS_COMPLETED = 'completed'
+    STATUS_ERRORED = 'errored'
     STATUS_FAILED = 'failed'
 
     CHOICES = (
         (STATUS_PENDING, 'Pending'),
         (STATUS_RUNNING, 'Running'),
         (STATUS_COMPLETED, 'Completed'),
+        (STATUS_ERRORED, 'Errored'),
         (STATUS_FAILED, 'Failed'),
     )
 
     TERMINAL_STATE_CHOICES = (
         STATUS_COMPLETED,
+        STATUS_ERRORED,
         STATUS_FAILED,
     )
 
-    NON_TERMINAL_STATE_CHOICES = (
-        STATUS_PENDING,
-        STATUS_RUNNING,
-    )
-
 
 #
 # Webhooks

netbox/extras/migrations/0043_report_model.py

@@ -0,0 +1,22 @@
+# Generated by Django 3.0.7 on 2020-06-23 02:28
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('extras', '0042_customfield_manager'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='Report',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False)),
+            ],
+            options={
+                'managed': False,
+            },
+        )
+    ]

netbox/extras/migrations/0043_jobresult.py → netbox/extras/migrations/0044_jobresult.py

@@ -13,15 +13,11 @@ def convert_job_results(apps, schema_editor):
     """
     Convert ReportResult objects to JobResult objects
     """
-    from django.contrib.contenttypes.management import create_contenttypes
     from extras.choices import JobResultStatusChoices
 
     ReportResult = apps.get_model('extras', 'ReportResult')
     JobResult = apps.get_model('extras', 'JobResult')
     ContentType = apps.get_model('contenttypes', 'ContentType')
-    app_config = apps.get_app_config('extras')
-    app_config.models_module = app_config.models_module or True
-    create_contenttypes(app_config)
     report_content_type = ContentType.objects.get(app_label='extras', model='report')
 
     job_results = []
@@ -50,19 +46,10 @@ class Migration(migrations.Migration):
     dependencies = [
         ('contenttypes', '0002_remove_content_type_name'),
         migrations.swappable_dependency(settings.AUTH_USER_MODEL),
-        ('extras', '0042_customfield_manager'),
+        ('extras', '0043_report_model'),
     ]
 
     operations = [
-        migrations.CreateModel(
-            name='Report',
-            fields=[
-                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False)),
-            ],
-            options={
-                'managed': False,
-            },
-        ),
         migrations.CreateModel(
             name='JobResult',
             fields=[

netbox/extras/models/models.py

@@ -12,6 +12,7 @@ from django.db import models
 from django.http import HttpResponse
 from django.template import Template, Context
 from django.urls import reverse
+from django.utils import timezone
 from rest_framework.utils.encoders import JSONEncoder
 
 from utilities.querysets import RestrictedQuerySet
@@ -650,6 +651,18 @@ class JobResult(models.Model):
 
         return f"{int(minutes)} minutes, {seconds:.2f} seconds"
 
+    def set_status(self, status):
+        """
+        Helper method to change the status of the job result and save. If the target status is terminal, the
+        completion time is also set.
+        """
+        self.status = status
+        if status in JobResultStatusChoices.TERMINAL_STATE_CHOICES:
+            self.completed = timezone.now()
+
+        self.save()
+
+
     @classmethod
     def enqueue_job(cls, func, name, obj_type, user, *args, **kwargs):
         """

netbox/extras/reports.py

@@ -14,6 +14,9 @@ from .constants import *
 from .models import JobResult
 
 
+logger = logging.getLogger(__name__)
+
+
 def is_report(obj):
     """
     Returns True if the given object is a Report.
@@ -71,13 +74,18 @@ def run_report(job_result, *args, **kwargs):
     """
     module_name, report_name = job_result.name.split('.', 1)
     report = get_report(module_name, report_name)
-    report.run(job_result)
+
+    try:
+        report.run(job_result)
+    except Exception:
+        job_result.set_status(JobResultStatusChoices.STATUS_ERRORED)
+        logging.error(f"Error during execution of report {job_result.name}")
 
     # Delete any previous terminal state results
     JobResult.objects.filter(
         obj_type=job_result.obj_type,
         name=job_result.name,
-        status=JobResultStatusChoices.TERMINAL_STATE_CHOICES
+        status__in=JobResultStatusChoices.TERMINAL_STATE_CHOICES
     ).exclude(
         pk=job_result.pk
     ).delete()

netbox/extras/scripts.py

@@ -399,10 +399,6 @@ def run_script(data, request, commit=True, *args, **kwargs):
     A wrapper for calling Script.run(). This performs error handling and provides a hook for committing changes. It
     exists outside of the Script class to ensure it cannot be overridden by a script author.
     """
-    output = None
-    start_time = None
-    end_time = None
-
     job_result = kwargs.pop('job_result')
     module, script_name = job_result.name.split('.', 1)
 
@@ -463,7 +459,7 @@ def run_script(data, request, commit=True, *args, **kwargs):
     JobResult.objects.filter(
         obj_type=job_result.obj_type,
         name=job_result.name,
-        status=JobResultStatusChoices.TERMINAL_STATE_CHOICES
+        status__in=JobResultStatusChoices.TERMINAL_STATE_CHOICES
     ).exclude(
         pk=job_result.pk
     ).delete()

netbox/extras/tables.py

@@ -1,8 +1,8 @@
 import django_tables2 as tables
 from django_tables2.utils import Accessor
 
-from utilities.tables import BaseTable, BooleanColumn, ButtonsColumn, ColorColumn, ToggleColumn
-from .models import ConfigContext, ObjectChange, JobResult, Tag, TaggedItem
+from utilities.tables import BaseTable, BooleanColumn, ColorColumn, ToggleColumn
+from .models import ConfigContext, ObjectChange, Tag, TaggedItem
 
 TAGGED_ITEM = """
 {% if value.get_absolute_url %}

netbox/extras/tests/test_api.py

@@ -8,9 +8,9 @@ from rest_framework import status
 from dcim.models import Device, DeviceRole, DeviceType, Manufacturer, Rack, RackGroup, RackRole, Site
 from extras.api.views import ScriptViewSet
 from extras.models import ConfigContext, Graph, ExportTemplate, Tag
+from extras.reports import Report
 from extras.scripts import BooleanVar, IntegerVar, Script, StringVar
 from utilities.testing import APITestCase, APIViewTestCases
-from utilities.utils import copy_safe_request
 
 
 class AppTest(APITestCase):
@@ -208,6 +208,37 @@ class ConfigContextTest(APIViewTestCases.APIViewTestCase):
         self.assertEqual(rendered_context['bar'], 456)
 
 
+class ReportTest(APITestCase):
+
+    class TestReport(Report):
+        pass  # The report is not actually run, we are testing that the view enqueues the job
+
+    def get_test_report(self, *args):
+        return self.TestReport
+
+    def setUp(self):
+
+        super().setUp()
+
+        # Monkey-patch the API viewset's _get_script method to return our test script above
+        ReportViewSet._get_report = self.get_test_report
+
+    def test_get_report(self):
+
+        url = reverse('extras-api:report-detail', kwargs={'pk': None})
+        response = self.client.get(url, **self.header)
+
+        self.assertEqual(response.data['name'], self.TestReport.__name__)
+
+    def test_run_report(self):
+
+        url = reverse('extras-api:report-detail', kwargs={'pk': None})
+        response = self.client.post(url, {}, format='json', **self.header)
+        self.assertHttpStatus(response, status.HTTP_200_OK)
+
+        self.assertEqual(response.data['result']['status']['value'], 'pending')
+
+
 class ScriptTest(APITestCase):
 
     class TestScript(Script):

netbox/extras/urls.py

@@ -36,12 +36,12 @@ urlpatterns = [
 
     # Reports
     path('reports/', views.ReportListView.as_view(), name='report_list'),
-    path('reports/<str:name>/', views.ReportView.as_view(), name='report'),
-    path('reports/<str:name>/run/', views.ReportRunView.as_view(), name='report_run'),
+    path('reports/<str:module>.<str:name>/', views.ReportView.as_view(), name='report'),
+    path('reports/results/<int:job_result_pk>/', views.ReportResultView.as_view(), name='report_result'),
 
     # Scripts
     path('scripts/', views.ScriptListView.as_view(), name='script_list'),
-    path('scripts/<str:module>/<str:name>/', views.ScriptView.as_view(), name='script'),
-    path('scripts/<str:module>/<str:name>/result/<int:job_result_pk>/', views.ScriptResultView.as_view(), name='script_result'),
+    path('scripts/<str:module>.<str:name>/', views.ScriptView.as_view(), name='script'),
+    path('scripts/results/<int:job_result_pk>/', views.ScriptResultView.as_view(), name='script_result'),
 
 ]

netbox/extras/views.py

@@ -350,12 +350,10 @@ class ReportListView(ContentTypePermissionRequiredMixin, View):
 
 
 class GetReportMixin:
-    def get_report(self, name):
-        if '.' not in name:
-            raise Http404
-        # Retrieve the Report by "<module>.<report>"
-        module_name, report_name = name.split('.', 1)
-        report = get_report(module_name, report_name)
+    def _get_report(self, name, module=None):
+        if module is None:
+            module, name = name.split('.', 1)
+        report = get_report(module, name)
         if report is None:
             raise Http404
 
@@ -369,43 +367,29 @@ class ReportView(GetReportMixin, ContentTypePermissionRequiredMixin, View):
     def get_required_permission(self):
         return 'extras.view_reportresult'
 
-    def get(self, request, name):
+    def get(self, request, module, name):
 
-        report = self.get_report(name)
+        report = self._get_report(name, module)
 
         report_content_type = ContentType.objects.get(app_label='extras', model='report')
-        latest_run_result = JobResult.objects.filter(
+        report.result = JobResult.objects.filter(
             obj_type=report_content_type,
             name=report.full_name,
             status__in=JobResultStatusChoices.TERMINAL_STATE_CHOICES
         ).first()
-        pending_run_result = JobResult.objects.filter(
-            obj_type=report_content_type,
-            name=report.full_name,
-            status__in=JobResultStatusChoices.NON_TERMINAL_STATE_CHOICES
-        ).order_by(
-            'created'
-        ).first()
-
-        report.result = latest_run_result
-        report.pending_result = pending_run_result
 
         return render(request, 'extras/report.html', {
             'report': report,
             'run_form': ConfirmationForm(),
         })
 
+    def post(self, request, module, name):
 
-class ReportRunView(GetReportMixin, ContentTypePermissionRequiredMixin, View):
-    """
-    Run a Report and record a new ReportResult.
-    """
-    def get_required_permission(self):
-        return 'extras.add_reportresult'
-
-    def post(self, request, name):
+        # Permissions check
+        if not request.user.has_perm('extras.run_report'):
+            return HttpResponseForbidden()
 
-        report = self.get_report(name)
+        report = self._get_report(name, module)
 
         form = ConfirmationForm(request.POST)
         if form.is_valid():
@@ -419,15 +403,42 @@ class ReportRunView(GetReportMixin, ContentTypePermissionRequiredMixin, View):
                 request.user
             )
 
-        return redirect('extras:report', name=report.full_name)
+            return redirect('extras:report_result', job_result_pk=job_result.pk)
+
+        return render(request, 'extras/report.html', {
+            'report': report,
+            'run_form': form,
+        })
+
 
+class ReportResultView(ContentTypePermissionRequiredMixin, GetReportMixin, View):
+
+    def get_required_permission(self):
+        return 'extras.view_report'
+
+    def get(self, request, job_result_pk):
+        result = get_object_or_404(JobResult.objects.all(), pk=job_result_pk)
+        report_content_type = ContentType.objects.get(app_label='extras', model='report')
+        if result.obj_type != report_content_type:
+            raise Http404
+
+        report = self._get_report(result.name)
+
+        return render(request, 'extras/report_result.html', {
+            'report': report,
+            'result': result,
+            'class_name': report.name,
+            'run_form': ConfirmationForm(),
+        })
 
 #
 # Scripts
 #
 
 class GetScriptMixin:
-    def _get_script(self, module, name):
+    def _get_script(self, name, module=None):
+        if module is None:
+            module, name = name.split('.', 1)
         scripts = get_scripts()
         try:
             return scripts[module][name]()
@@ -453,7 +464,7 @@ class ScriptView(ContentTypePermissionRequiredMixin, GetScriptMixin, View):
         return 'extras.view_script'
 
     def get(self, request, module, name):
-        script = self._get_script(module, name)
+        script = self._get_script(name, module)
         form = script.as_form(initial=request.GET)
 
         # Look for a pending JobResult (use the latest one by creation timestamp)
@@ -461,7 +472,8 @@ class ScriptView(ContentTypePermissionRequiredMixin, GetScriptMixin, View):
         script.result = JobResult.objects.filter(
             obj_type=script_content_type,
             name=script.full_name,
-            status__in=JobResultStatusChoices.NON_TERMINAL_STATE_CHOICES
+        ).exclude(
+            status__in=JobResultStatusChoices.TERMINAL_STATE_CHOICES
         ).first()
 
         return render(request, 'extras/script.html', {
@@ -476,10 +488,8 @@ class ScriptView(ContentTypePermissionRequiredMixin, GetScriptMixin, View):
         if not request.user.has_perm('extras.run_script'):
             return HttpResponseForbidden()
 
-        script = self._get_script(module, name)
+        script = self._get_script(name, module)
         form = script.as_form(request.POST, request.FILES)
-        output = None
-        execution_time = None
 
         if form.is_valid():
             commit = form.cleaned_data.pop('_commit')
@@ -495,7 +505,13 @@ class ScriptView(ContentTypePermissionRequiredMixin, GetScriptMixin, View):
                 commit=commit
             )
 
-        return redirect('extras:script_result', module=module, name=name, job_result_pk=job_result.pk)
+            return redirect('extras:script_result', job_result_pk=job_result.pk)
+
+        return render(request, 'extras/script.html', {
+            'module': module,
+            'script': script,
+            'form': form,
+        })
 
 
 class ScriptResultView(ContentTypePermissionRequiredMixin, GetScriptMixin, View):
@@ -503,19 +519,16 @@ class ScriptResultView(ContentTypePermissionRequiredMixin, GetScriptMixin, View)
     def get_required_permission(self):
         return 'extras.view_script'
 
-    def get(self, request, module, name, job_result_pk):
-        script = self._get_script(module, name)
-        form = script.as_form(initial=request.GET)
-
-        # Look for a pending JobResult (use the latest one by creation timestamp)
-        script_content_type = ContentType.objects.get(app_label='extras', model='script')
+    def get(self, request, job_result_pk):
         result = get_object_or_404(JobResult.objects.all(), pk=job_result_pk)
+        script_content_type = ContentType.objects.get(app_label='extras', model='script')
         if result.obj_type != script_content_type:
             raise Http404
 
+        script = self._get_script(result.name)
+
         return render(request, 'extras/script_result.html', {
-            'module': module,
             'script': script,
             'result': result,
-            'class_name': name
+            'class_name': script.__class__.__name__
         })

netbox/templates/extras/report_list.html

@@ -21,7 +21,7 @@
                             {% for report in module_reports %}
                                 <tr>
                                     <td>
-                                        <a href="{% url 'extras:report' name=report.full_name %}" name="report.{{ report.name }}"><strong>{{ report.name }}</strong></a>
+                                        <a href="{% url 'extras:report_result' job_result_pk=report.result.pk %}" name="report.{{ report.name }}"><strong>{{ report.name }}</strong></a>
                                     </td>
                                     <td>
                                         {% include 'extras/inc/job_label.html' with result=report.result %}

netbox/templates/extras/report.html → netbox/templates/extras/report_result.html

@@ -16,36 +16,35 @@
     </div>
     {% if perms.extras.add_reportresult %}
         <div class="pull-right noprint">
-            <form action="{% url 'extras:report_run' name=report.full_name %}" method="post">
+            <form action="{% url 'extras:report' module=report.module name=report.name %}" method="post">
                 {% csrf_token %}
                 {{ run_form }}
                 <button type="submit" name="_run" class="btn btn-primary"><i class="fa fa-play"></i> Run Report</button>
             </form>
         </div>
     {% endif %}
-    <h1>{{ report.name }}{% include 'extras/inc/job_label.html' with result=report.result %}</h1>
+    <h1>{{ report.name }}</h1>
     <div class="row">
         <div class="col-md-12">
             {% if report.description %}
                 <p class="lead">{{ report.description }}</p>
             {% endif %}
-            {% if report.result %}
-                <p>Last run: <strong>{{ report.result.created }}</strong> {% if report.result.completed %} Duration: <strong>{{ report.result.duration }}</strong>{% endif %}</p>
-            {% endif %}
-            {% if report.pending_result %}
-                <p>
-                    Pending run: <strong>{{ report.pending_result.created }}</strong>
-                    <span id="pending-result-label">{% include 'extras/inc/job_label.html' with result=report.pending_result %}</span>
+            <p>
+                Run: <strong>{{ result.created }}</strong>
+                {% if result.completed %}
+                    Duration: <strong>{{ result.duration }}</strong>
+                {% else %}
                     <img id="pending-result-loader" src="{% static 'img/ajax-loader.gif' %}" />
-                </p>
-            {% endif %}
-            {% if report.result %}
+                {% endif %}
+                <span id="pending-result-label">{% include 'extras/inc/job_label.html' with result=result %}</span>
+            </p>
+            {% if result.completed %}
                 <div class="panel panel-default">
                     <div class="panel-heading">
                         <strong>Report Methods</strong>
                     </div>
                     <table class="table table-hover panel-body">
-                        {% for method, data in report.result.data.items %}
+                        {% for method, data in result.data.items %}
                             <tr>
                                 <td><code><a href="#{{ method }}">{{ method }}</a></code></td>
                                 <td class="text-right report-stats">
@@ -72,7 +71,7 @@
                             </tr>
                         </thead>
                         <tbody>
-                            {% for method, data in report.result.data.items %}
+                            {% for method, data in result.data.items %}
                                 <tr>
                                     <th colspan="4" style="font-family: monospace">
                                         <a name="{{ method }}"></a>{{ method }}
@@ -99,11 +98,7 @@
                     </table>
                 </div>
             {% else %}
-                <div class="well">No results are available for this report. Please run the report first.</div>
-            {% endif %}
-        </div>
-        <div class="col-md-3">
-            {% if report.result %}
+                <div class="well">Pending results</div>
             {% endif %}
         </div>
     </div>
@@ -111,19 +106,14 @@
 
 {% block javascript %}
 <script type="text/javascript">
-{% if report.pending_result %}
-var pending_result_id = {{ report.pending_result.pk }};
+{% if not result.completed %}
+var pending_result_id = {{ result.pk }};
 {% else %}
 var pending_result_id = null;
 {% endif %}
 
 function jobTerminatedAction(){
-    $('#pending-result-loader').hide();
-    var refreshButton = document.createElement('button');
-    refreshButton.className = 'btn btn-xs btn-primary';
-    refreshButton.onclick = refreshWindow;
-    refreshButton.innerHTML = '<i class="fa fa-refresh"></i> Refresh';
-    $('#pending-result-loader').parents('p').append(refreshButton)
+    refreshWindow();
 }
 
 </script>

netbox/templates/extras/script_result.html

@@ -11,7 +11,7 @@
         <div class="col-md-12">
             <ol class="breadcrumb">
                 <li><a href="{% url 'extras:script_list' %}">Scripts</a></li>
-                <li><a href="{% url 'extras:script_list' %}#module.{{ module }}">{{ module|bettertitle }}</a></li>
+                <li><a href="{% url 'extras:script_list' %}#module.{{ script.module }}">{{ script.module|bettertitle }}</a></li>
                 <li><a href="{% url 'extras:script' module=script.module name=class_name %}">{{ script }}</a></li>
                 <li>{{ result.created }}</li>
             </ol>
@@ -36,9 +36,9 @@
             {% if result.completed %}
                 Duration: <strong>{{ result.duration }}</strong>
             {% else %}
-                <span id="pending-result-label">{% include 'extras/inc/job_label.html' with result=result %}</span>
                 <img id="pending-result-loader" src="{% static 'img/ajax-loader.gif' %}" />
             {% endif %}
+            <span id="pending-result-label">{% include 'extras/inc/job_label.html' with result=result %}</span>
         </p>
         <div role="tabpanel" class="tab-pane active" id="log">
             {% if result.completed %}
@@ -76,6 +76,12 @@
                         </div>
                     </div>
                 </div>
+            {% else %}
+                <div class="row">
+                    <div class="col-md-12">
+                        <div class="well">Pending results</div>
+                    </div>
+                </div>
             {% endif %}
         </div>
         <div role="tabpanel" class="tab-pane" id="output">

netbox/templates/home.html

@@ -280,7 +280,7 @@
                 <table class="table table-hover panel-body">
                     {% for result in report_results %}
                         <tr>
-                            <td><a href="{% url 'extras:report' name=result.name %}">{{ result.name }}</a></td>
+                            <td><a href="{% url 'extras:report_result' job_result_pk=result.pk %}">{{ result.name }}</a></td>
                             <td class="text-right"><span title="{{ result.created }}">{% include 'extras/inc/job_label.html' %}</span></td>
                         </tr>
                     {% endfor %}

netbox/utilities/utils.py

@@ -270,9 +270,8 @@ class NetBoxFakeRequest:
     A fake request object which is explicitly defined at the module level so it is able to be pickled. It simply
     takes what is passed to it as kwargs on init and sets them as instance variables.
     """
-    def __init__(self, *args, **kwargs):
-        for k, v in kwargs.items():
-            setattr(self, k, v)
+    def __init__(self, _dict):
+        self.__dict__ = _dict
 
 
 def copy_safe_request(request):
@@ -285,7 +284,7 @@ def copy_safe_request(request):
         for k in HTTP_REQUEST_META_SAFE_COPY
         if k in request.META and isinstance(request.META[k], str)
     }
-    return NetBoxFakeRequest(**{
+    return NetBoxFakeRequest({
         'META': meta,
         'POST': request.POST,
         'GET': request.GET,