From f092c107b541838b550b85777cf4781f82cb16fb Mon Sep 17 00:00:00 2001 From: John Anderson Date: Fri, 3 Jul 2020 11:55:04 -0400 Subject: [PATCH] PR review updates --- netbox/extras/api/serializers.py | 12 +-- netbox/extras/api/views.py | 2 +- netbox/extras/choices.py | 8 +- netbox/extras/migrations/0043_report_model.py | 22 ++++ .../{0043_jobresult.py => 0044_jobresult.py} | 15 +-- netbox/extras/models/models.py | 13 +++ netbox/extras/reports.py | 12 ++- netbox/extras/scripts.py | 6 +- netbox/extras/tables.py | 4 +- netbox/extras/tests/test_api.py | 33 +++++- netbox/extras/urls.py | 8 +- netbox/extras/views.py | 101 ++++++++++-------- netbox/templates/extras/report_list.html | 2 +- .../{report.html => report_result.html} | 44 +++----- netbox/templates/extras/script_result.html | 10 +- netbox/templates/home.html | 2 +- netbox/utilities/utils.py | 7 +- 17 files changed, 178 insertions(+), 123 deletions(-) create mode 100644 netbox/extras/migrations/0043_report_model.py rename netbox/extras/migrations/{0043_jobresult.py => 0044_jobresult.py} (84%) rename netbox/templates/extras/{report.html => report_result.html} (74%) diff --git a/netbox/extras/api/serializers.py b/netbox/extras/api/serializers.py index 636e813bd..1befd082f 100644 --- a/netbox/extras/api/serializers.py +++ b/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) diff --git a/netbox/extras/api/views.py b/netbox/extras/api/views.py index 5700efe55..679c4e96d 100644 --- a/netbox/extras/api/views.py +++ b/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 diff --git a/netbox/extras/choices.py b/netbox/extras/choices.py index 8650b1e22..7ff8aab95 100644 --- a/netbox/extras/choices.py +++ b/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 diff --git a/netbox/extras/migrations/0043_report_model.py b/netbox/extras/migrations/0043_report_model.py new file mode 100644 index 000000000..d0d55c3f7 --- /dev/null +++ b/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, + }, + ) + ] diff --git a/netbox/extras/migrations/0043_jobresult.py b/netbox/extras/migrations/0044_jobresult.py similarity index 84% rename from netbox/extras/migrations/0043_jobresult.py rename to netbox/extras/migrations/0044_jobresult.py index f5ad3b1f5..f52451b8e 100644 --- a/netbox/extras/migrations/0043_jobresult.py +++ b/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=[ diff --git a/netbox/extras/models/models.py b/netbox/extras/models/models.py index 285563b9f..316f3b247 100644 --- a/netbox/extras/models/models.py +++ b/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): """ diff --git a/netbox/extras/reports.py b/netbox/extras/reports.py index 10a560771..db6e8d373 100644 --- a/netbox/extras/reports.py +++ b/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() diff --git a/netbox/extras/scripts.py b/netbox/extras/scripts.py index 895475a09..ccb4fdc82 100644 --- a/netbox/extras/scripts.py +++ b/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() diff --git a/netbox/extras/tables.py b/netbox/extras/tables.py index 2754bf24b..578975ed3 100644 --- a/netbox/extras/tables.py +++ b/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 %} diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index de9d58446..cf615d914 100644 --- a/netbox/extras/tests/test_api.py +++ b/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): diff --git a/netbox/extras/urls.py b/netbox/extras/urls.py index f199abfc9..6f8832487 100644 --- a/netbox/extras/urls.py +++ b/netbox/extras/urls.py @@ -36,12 +36,12 @@ urlpatterns = [ # Reports path('reports/', views.ReportListView.as_view(), name='report_list'), - path('reports//', views.ReportView.as_view(), name='report'), - path('reports//run/', views.ReportRunView.as_view(), name='report_run'), + path('reports/./', views.ReportView.as_view(), name='report'), + path('reports/results//', views.ReportResultView.as_view(), name='report_result'), # Scripts path('scripts/', views.ScriptListView.as_view(), name='script_list'), - path('scripts///', views.ScriptView.as_view(), name='script'), - path('scripts///result//', views.ScriptResultView.as_view(), name='script_result'), + path('scripts/./', views.ScriptView.as_view(), name='script'), + path('scripts/results//', views.ScriptResultView.as_view(), name='script_result'), ] diff --git a/netbox/extras/views.py b/netbox/extras/views.py index 92453b090..dfaeba806 100644 --- a/netbox/extras/views.py +++ b/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_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' + # Permissions check + if not request.user.has_perm('extras.run_report'): + return HttpResponseForbidden() - def post(self, request, name): - - 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__ }) diff --git a/netbox/templates/extras/report_list.html b/netbox/templates/extras/report_list.html index 4cf52243a..f507cf2b1 100644 --- a/netbox/templates/extras/report_list.html +++ b/netbox/templates/extras/report_list.html @@ -21,7 +21,7 @@ {% for report in module_reports %} - {{ report.name }} + {{ report.name }} {% include 'extras/inc/job_label.html' with result=report.result %} diff --git a/netbox/templates/extras/report.html b/netbox/templates/extras/report_result.html similarity index 74% rename from netbox/templates/extras/report.html rename to netbox/templates/extras/report_result.html index 18ea21b99..2053c86df 100644 --- a/netbox/templates/extras/report.html +++ b/netbox/templates/extras/report_result.html @@ -16,36 +16,35 @@ {% if perms.extras.add_reportresult %}
-
+ {% csrf_token %} {{ run_form }}
{% endif %} -

{{ report.name }}{% include 'extras/inc/job_label.html' with result=report.result %}

+

{{ report.name }}

{% if report.description %}

{{ report.description }}

{% endif %} - {% if report.result %} -

Last run: {{ report.result.created }} {% if report.result.completed %} Duration: {{ report.result.duration }}{% endif %}

- {% endif %} - {% if report.pending_result %} -

- Pending run: {{ report.pending_result.created }} - {% include 'extras/inc/job_label.html' with result=report.pending_result %} +

+ Run: {{ result.created }} + {% if result.completed %} + Duration: {{ result.duration }} + {% else %} -

- {% endif %} - {% if report.result %} + {% endif %} + {% include 'extras/inc/job_label.html' with result=result %} +

+ {% if result.completed %}
Report Methods
- {% for method, data in report.result.data.items %} + {% for method, data in result.data.items %} - {% for method, data in report.result.data.items %} + {% for method, data in result.data.items %}
{{ method }} @@ -72,7 +71,7 @@
{{ method }} @@ -99,11 +98,7 @@
{% else %} -
No results are available for this report. Please run the report first.
- {% endif %} -
-
- {% if report.result %} +
Pending results
{% endif %}
@@ -111,19 +106,14 @@ {% block javascript %} diff --git a/netbox/templates/extras/script_result.html b/netbox/templates/extras/script_result.html index 64e242a56..e34c5a18d 100644 --- a/netbox/templates/extras/script_result.html +++ b/netbox/templates/extras/script_result.html @@ -11,7 +11,7 @@
  1. Scripts
    2. -
  2. {{ module|bettertitle }}
    3. +
  3. {{ script.module|bettertitle }}
  4. {{ script }}
  5. {{ result.created }}
@@ -36,9 +36,9 @@ {% if result.completed %} Duration: {{ result.duration }} {% else %} - {% include 'extras/inc/job_label.html' with result=result %} {% endif %} + {% include 'extras/inc/job_label.html' with result=result %}

{% if result.completed %} @@ -76,6 +76,12 @@
+ {% else %} +
+
+
Pending results
+
+
{% endif %}
diff --git a/netbox/templates/home.html b/netbox/templates/home.html index f4acda3a3..85332ae1b 100644 --- a/netbox/templates/home.html +++ b/netbox/templates/home.html @@ -280,7 +280,7 @@ {% for result in report_results %} - + {% endfor %} diff --git a/netbox/utilities/utils.py b/netbox/utilities/utils.py index 1c8f9f12c..cb3db2465 100644 --- a/netbox/utilities/utils.py +++ b/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,
{{ result.name }}{{ result.name }} {% include 'extras/inc/job_label.html' %}