From 57580f46f6c7879afc0c2c852c630c839babccb3 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 20 Dec 2024 15:16:08 +0100 Subject: [PATCH 1/4] :sparkles: [#4930] Add custom admin view for submission export view --- src/openforms/forms/admin/form_statistics.py | 14 ++++ src/openforms/forms/admin/views.py | 39 ++++++++++- src/openforms/forms/forms/__init__.py | 5 +- src/openforms/forms/forms/form_statistics.py | 57 ++++++++++++++++ .../change_list_object_tools.html | 11 ++++ .../forms/formstatistics/export_form.html | 66 +++++++++++++++++++ .../forms/tests/admin/test_form_statistics.py | 59 +++++++++++++++++ 7 files changed, 249 insertions(+), 2 deletions(-) create mode 100644 src/openforms/forms/forms/form_statistics.py create mode 100644 src/openforms/forms/templates/admin/forms/formstatistics/change_list_object_tools.html create mode 100644 src/openforms/forms/templates/admin/forms/formstatistics/export_form.html create mode 100644 src/openforms/forms/tests/admin/test_form_statistics.py diff --git a/src/openforms/forms/admin/form_statistics.py b/src/openforms/forms/admin/form_statistics.py index f0d5759dba..b330b7adf0 100644 --- a/src/openforms/forms/admin/form_statistics.py +++ b/src/openforms/forms/admin/form_statistics.py @@ -1,6 +1,8 @@ from django.contrib import admin +from django.urls import path from ..models import FormStatistics +from .views import ExportSubmissionStatisticsView @admin.register(FormStatistics) @@ -31,3 +33,15 @@ def has_delete_permission(self, request, obj=None): def has_change_permission(self, request, obj=None): return False + + def get_urls(self): + urls = super().get_urls() + export_view = self.admin_site.admin_view( + ExportSubmissionStatisticsView.as_view( + media=self.media, + ) # pyright: ignore[reportArgumentType] + ) + custom_urls = [ + path("export/", export_view, name="formstatistics_export"), + ] + return custom_urls + urls diff --git a/src/openforms/forms/admin/views.py b/src/openforms/forms/admin/views.py index 2edab625c3..ae9476df63 100644 --- a/src/openforms/forms/admin/views.py +++ b/src/openforms/forms/admin/views.py @@ -3,12 +3,15 @@ from django import forms from django.contrib import messages +from django.contrib.admin.helpers import AdminField +from django.contrib.admin.views.decorators import staff_member_required from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.contrib.postgres.forms import SimpleArrayField from django.http import FileResponse from django.shortcuts import get_object_or_404 from django.urls import reverse_lazy +from django.utils.decorators import method_decorator from django.utils.translation import gettext_lazy as _ from django.views import View from django.views.generic.edit import FormView @@ -18,8 +21,9 @@ from openforms.logging import logevent +from ..forms import ExportStatisticsForm from ..forms.form import FormImportForm -from ..models.form import Form, FormsExport +from ..models import Form, FormsExport, FormStatistics from ..utils import import_form from .tasks import process_forms_export, process_forms_import @@ -109,3 +113,36 @@ def _bulk_import_forms(self, import_file): filename = private_media_storage.save(name, import_file) process_forms_import.delay(filename, self.request.user.id) + + +@method_decorator(staff_member_required, name="dispatch") +class ExportSubmissionStatisticsView( + LoginRequiredMixin, PermissionRequiredMixin, FormView +): + permission_required = "forms.view_formstatistics" + template_name = "admin/forms/formstatistics/export_form.html" + form_class = ExportStatisticsForm + + # must be set by the ModelAdmin + media: forms.Media | None = None + + def get_context_data(self, **kwargs): + assert ( + self.media is not None + ), "You must pass media=self.media in the model admin" + context = super().get_context_data(**kwargs) + + form = context["form"] + + def form_fields(): + for name in form.fields: + yield AdminField(form, name, is_first=False) + + context.update( + { + "opts": FormStatistics._meta, + "media": self.media + form.media, + "form_fields": form_fields, + } + ) + return context diff --git a/src/openforms/forms/forms/__init__.py b/src/openforms/forms/forms/__init__.py index ac9f05610b..93449ca0b8 100644 --- a/src/openforms/forms/forms/__init__.py +++ b/src/openforms/forms/forms/__init__.py @@ -1 +1,4 @@ -from .form_definition import FormDefinitionForm # noqa +from .form_definition import FormDefinitionForm +from .form_statistics import ExportStatisticsForm + +__all__ = ["FormDefinitionForm", "ExportStatisticsForm"] diff --git a/src/openforms/forms/forms/form_statistics.py b/src/openforms/forms/forms/form_statistics.py new file mode 100644 index 0000000000..6b1c2de071 --- /dev/null +++ b/src/openforms/forms/forms/form_statistics.py @@ -0,0 +1,57 @@ +from __future__ import annotations + +from datetime import date + +from django import forms +from django.contrib.admin.widgets import AdminDateWidget +from django.utils import timezone +from django.utils.translation import gettext_lazy as _ + +from dateutil.relativedelta import relativedelta + +from ..models import Form + + +def get_first_of_previous_month() -> date: + now = timezone.localtime() + one_month_ago = now - relativedelta(months=1) + first_of_previous_month = one_month_ago.replace(day=1) + return first_of_previous_month.date() + + +def get_last_of_previous_month() -> date: + now = timezone.localtime() + first_of_current_month = now.replace(day=1) + get_last_of_previous_month = first_of_current_month - relativedelta(days=1) + return get_last_of_previous_month.date() + + +class ExportStatisticsForm(forms.Form): + start_date = forms.DateField( + label=_("From"), + required=True, + initial=get_first_of_previous_month, + help_text=_( + "Export form submission that were submitted on or after this date." + ), + widget=AdminDateWidget, + ) + end_date = forms.DateField( + label=_("Until"), + required=True, + initial=get_last_of_previous_month, + help_text=_( + "Export form submission that were submitted before or on this date." + ), + widget=AdminDateWidget, + ) + limit_to_forms = forms.ModelMultipleChoiceField( + label=_("Forms"), + required=False, + queryset=Form.objects.filter(_is_deleted=False), + help_text=_( + "Limit the export to the selected forms, if specified. Leave the field " + "empty to export all forms. Hold CTRL (or COMMAND on Mac) to select " + "multiple options." + ), + ) diff --git a/src/openforms/forms/templates/admin/forms/formstatistics/change_list_object_tools.html b/src/openforms/forms/templates/admin/forms/formstatistics/change_list_object_tools.html new file mode 100644 index 0000000000..36bdaaf87c --- /dev/null +++ b/src/openforms/forms/templates/admin/forms/formstatistics/change_list_object_tools.html @@ -0,0 +1,11 @@ +{% extends "admin/change_list_object_tools.html" %} +{% load i18n %} + +{% block object-tools-items %} +
  • + + {% trans "Export submission statistics" %} + +
  • + {{ block.super }} +{% endblock %} diff --git a/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html b/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html new file mode 100644 index 0000000000..b3cec95956 --- /dev/null +++ b/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html @@ -0,0 +1,66 @@ +{% extends "admin/base_site.html" %} +{% load static i18n django_admin_index %} + +{% block extrahead %}{{ block.super }} + +{{ media }} +{% endblock %} + +{% block extrastyle %}{{ block.super }} + +{% endblock %} + +{% block nav-global %}{% include "django_admin_index/includes/app_list.html" %}{% endblock nav-global %} + +{% block title %} {% trans "Export submission statistics" %} {{ block.super }} {% endblock %} + +{% block breadcrumbs %} + +{% endblock %} + +{% block content %} +

    {% trans 'Export submission statistics' %}

    + +
    +
    + {% csrf_token %} + +
    +
    {% blocktrans trimmed %} +

    Here you can create an export of successfully registered form submissions. The + export file contains the following columns: public reference, form name, + form internal name, the submission datetime and the timestamp of registration.

    + +

    You can use the filters below to limit the result set in the export.

    + {% endblocktrans %}
    + + {# TODO: doesn't handle checkboxes, see admin/includes/fieldset.html for when this is necessary #} + {% for field in form_fields %} +
    + {{ field.errors }} +
    +
    + {{ field.label_tag }} + {{ field.field }} +
    +
    + + {% if field.field.help_text %} +
    +
    {{ field.field.help_text|safe }}
    +
    + {% endif %} +
    + {% endfor %} +
    + +
    + +
    +
    +
    +{% endblock %} diff --git a/src/openforms/forms/tests/admin/test_form_statistics.py b/src/openforms/forms/tests/admin/test_form_statistics.py new file mode 100644 index 0000000000..5570a2885a --- /dev/null +++ b/src/openforms/forms/tests/admin/test_form_statistics.py @@ -0,0 +1,59 @@ +from django.urls import reverse, reverse_lazy +from django.utils.translation import gettext as _ + +from django_webtest import WebTest +from maykin_2fa.test import disable_admin_mfa + +from openforms.accounts.tests.factories import UserFactory + + +@disable_admin_mfa() +class FormStatisticsExportAdminTests(WebTest): + + admin_url = reverse_lazy("admin:formstatistics_export") + + def test_access_control_no_access(self): + # various flavours of users do not have access, only if the right permissions + # are set are you allowed in + invalid_users = ( + ( + "plain user", + UserFactory.create(), + 302, + ), + ( + "staff user without perms", + UserFactory.create(is_staff=True), + 403, + ), + ( + "user with perms no staff", + UserFactory.create( + is_staff=False, user_permissions=["forms.view_formstatistics"] + ), + 302, + ), + ) + + for label, user, expected_status in invalid_users: + with self.subTest(label, expected_status=expected_status): + response = self.app.get( + self.admin_url, + user=user, + auto_follow=False, + status=expected_status, + ) + + self.assertEqual(response.status_code, expected_status) + + def test_navigate_from_changelist(self): + user = UserFactory.create( + is_staff=True, user_permissions=["forms.view_formstatistics"] + ) + changelist = self.app.get( + reverse("admin:forms_formstatistics_changelist"), user=user + ) + + export_page = changelist.click(_("Export submission statistics")) + + self.assertEqual(export_page.request.path, self.admin_url) From a53096df48c70d058650cabf35cfc4ad287de443 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 20 Dec 2024 16:31:18 +0100 Subject: [PATCH 2/4] :sparkles: [#4930] Implement the actual export Export submission statistics based on the timeline logs. Note: if the timeline logs are pruned, this affects the exports. It's up to the users to periodically create these exports and save them somewhere if they periodically prune log records. Note 2: filtering on forms only works on new log records, as existing log records don't have the form ID stored in the structured data. Submissions that were deleted for which existing log records are present will display 'unknown' for some columns because the relevant information has been deleted. Only from 3.0 onwards are we snapshotting the data required for the exports. --- src/openforms/forms/admin/views.py | 22 +++- src/openforms/forms/forms/form_statistics.py | 11 ++ src/openforms/forms/statistics.py | 102 +++++++++++++++++++ src/openforms/logging/logevent.py | 14 ++- 4 files changed, 147 insertions(+), 2 deletions(-) create mode 100644 src/openforms/forms/statistics.py diff --git a/src/openforms/forms/admin/views.py b/src/openforms/forms/admin/views.py index ae9476df63..fdfbdd5c3e 100644 --- a/src/openforms/forms/admin/views.py +++ b/src/openforms/forms/admin/views.py @@ -1,4 +1,5 @@ import zipfile +from datetime import date from uuid import uuid4 from django import forms @@ -8,14 +9,16 @@ from django.contrib.auth.mixins import LoginRequiredMixin, PermissionRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.contrib.postgres.forms import SimpleArrayField -from django.http import FileResponse +from django.http import FileResponse, HttpResponse from django.shortcuts import get_object_or_404 from django.urls import reverse_lazy from django.utils.decorators import method_decorator +from django.utils.http import content_disposition_header from django.utils.translation import gettext_lazy as _ from django.views import View from django.views.generic.edit import FormView +from import_export.formats.base_formats import XLSX from privates.storages import private_media_storage from rest_framework.exceptions import ValidationError @@ -126,6 +129,23 @@ class ExportSubmissionStatisticsView( # must be set by the ModelAdmin media: forms.Media | None = None + def form_valid(self, form: ExportStatisticsForm) -> HttpResponse: + start_date: date = form.cleaned_data["start_date"] + end_date: date = form.cleaned_data["end_date"] + dataset = form.export() + format = XLSX() + filename = f"submissions_{start_date.isoformat()}_{end_date.isoformat()}.xlsx" + return HttpResponse( + format.export_data(dataset), + content_type=format.get_content_type(), + headers={ + "Content-Disposition": content_disposition_header( + as_attachment=True, + filename=filename, + ), + }, + ) + def get_context_data(self, **kwargs): assert ( self.media is not None diff --git a/src/openforms/forms/forms/form_statistics.py b/src/openforms/forms/forms/form_statistics.py index 6b1c2de071..0abfbbf087 100644 --- a/src/openforms/forms/forms/form_statistics.py +++ b/src/openforms/forms/forms/form_statistics.py @@ -8,8 +8,10 @@ from django.utils.translation import gettext_lazy as _ from dateutil.relativedelta import relativedelta +from tablib import Dataset from ..models import Form +from ..statistics import export_registration_statistics def get_first_of_previous_month() -> date: @@ -55,3 +57,12 @@ class ExportStatisticsForm(forms.Form): "multiple options." ), ) + + def export(self) -> Dataset: + start_date: date = self.cleaned_data["start_date"] + end_date: date = self.cleaned_data["end_date"] + return export_registration_statistics( + start_date, + end_date, + self.cleaned_data["limit_to_forms"], + ) diff --git a/src/openforms/forms/statistics.py b/src/openforms/forms/statistics.py new file mode 100644 index 0000000000..5ce87c5683 --- /dev/null +++ b/src/openforms/forms/statistics.py @@ -0,0 +1,102 @@ +from datetime import date, datetime, time + +from django.contrib.contenttypes.models import ContentType +from django.db import models +from django.utils.timezone import make_aware +from django.utils.translation import gettext_lazy as _ + +from tablib import Dataset + +from openforms.logging import logevent +from openforms.logging.models import TimelineLogProxy +from openforms.submissions.models import Submission + +from .models import Form + + +def export_registration_statistics( + start_date: date, + end_date: date, + limit_to_forms: models.QuerySet[Form] | None = None, +) -> Dataset: + """ + Export the form registration statistics to a tablib Dataset. + + The export retrieves log records within the specified date range (closed interval, + [start_date, end_date]), optionally filtering them down to a set of forms. Only log + records for successful registration are considered. + + :arg start_date: include log records starting from this date (midnight, in the local + timezone). + :arg end_date: include log records until (and including) this date. Log record up to + midnight (in the local timezone) the next day are included, i.e. + ``$date, 23:59:59 999999us``. + :arg limit_to_forms: A queryset of forms to limit the export to. If not provided or + ``None`` is given, all forms are included. + """ + dataset = Dataset( + headers=( + _("Public reference"), + _("Form name (public)"), + _("Form name (internal)"), + _("Submitted on"), + _("Registered on"), + ), + title=_("Successfully registered submissions between {start} and {end}").format( + start=start_date.isoformat(), + end=end_date.isoformat(), + ), + ) + + _start_date = make_aware(datetime.combine(start_date, time.min)) + _end_date = make_aware(datetime.combine(end_date, time.max)) + + log_records = TimelineLogProxy.objects.filter( + content_type=ContentType.objects.get_for_model(Submission), + timestamp__gte=_start_date, + timestamp__lt=_end_date, + # see openforms.logging.logevent for the data structure of the extra_data + # JSONField + extra_data__log_event=logevent.REGISTRATION_SUCCESS_EVENT, + ).order_by("timestamp") + + if limit_to_forms: + form_ids = list(limit_to_forms.values_list("pk", flat=True)) + log_records = log_records.filter(extra_data__form_id__in=form_ids) + + for record in log_records.iterator(): + extra_data = record.extra_data + # GFKs will be broken when the submissions are pruned, so prefer extracting + # information from the extra_data snapshot + submission: Submission | None = record.content_object + dataset.append( + ( + # public reference + extra_data.get( + "public_reference", + ( + submission.public_registration_reference + if submission + else "-unknown-" + ), + ), + # public form name + extra_data.get( + "form_name", submission.form.name if submission else "-unknown-" + ), + # internal form name + extra_data.get( + "internal_form_name", + submission.form.internal_name if submission else "-unknown-", + ), + # when the user submitted the form + extra_data.get( + "submitted_on", + submission.completed_on.isoformat() if submission else None, + ), + # when the registration succeeeded - this must be close to when it was logged + record.timestamp.isoformat(), + ) + ) + + return dataset diff --git a/src/openforms/logging/logevent.py b/src/openforms/logging/logevent.py index c50634f453..e2a75cc609 100644 --- a/src/openforms/logging/logevent.py +++ b/src/openforms/logging/logevent.py @@ -216,10 +216,22 @@ def registration_start(submission: Submission): ) +REGISTRATION_SUCCESS_EVENT = "registration_success" + + def registration_success(submission: Submission, plugin): + extra_data = { + # note: these keys are used in form statistics exports! + "public_reference": submission.public_registration_reference, + "form_id": submission.form.pk, + "form_name": submission.form.name, + "internal_form_name": submission.form.internal_name, + "submitted_on": submission.completed_on, + } _create_log( submission, - "registration_success", + REGISTRATION_SUCCESS_EVENT, + extra_data=extra_data, plugin=plugin, ) From 33554a1ee5608ffbe9962194ea39ead0f7899e49 Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 20 Dec 2024 16:47:15 +0100 Subject: [PATCH 3/4] :white_check_mark: [#4930] Add test for export view --- .../forms/formstatistics/export_form.html | 2 +- .../forms/tests/admin/test_form_statistics.py | 40 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html b/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html index b3cec95956..f849c90c1e 100644 --- a/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html +++ b/src/openforms/forms/templates/admin/forms/formstatistics/export_form.html @@ -26,7 +26,7 @@

    {% trans 'Export submission statistics' %}

    -
    + {% csrf_token %}
    diff --git a/src/openforms/forms/tests/admin/test_form_statistics.py b/src/openforms/forms/tests/admin/test_form_statistics.py index 5570a2885a..f8710fc1dc 100644 --- a/src/openforms/forms/tests/admin/test_form_statistics.py +++ b/src/openforms/forms/tests/admin/test_form_statistics.py @@ -2,9 +2,15 @@ from django.utils.translation import gettext as _ from django_webtest import WebTest +from freezegun import freeze_time from maykin_2fa.test import disable_admin_mfa from openforms.accounts.tests.factories import UserFactory +from openforms.logging import logevent +from openforms.registrations.contrib.demo.plugin import DemoRegistration +from openforms.submissions.tests.factories import SubmissionFactory + +from ...forms import ExportStatisticsForm @disable_admin_mfa() @@ -57,3 +63,37 @@ def test_navigate_from_changelist(self): export_page = changelist.click(_("Export submission statistics")) self.assertEqual(export_page.request.path, self.admin_url) + + def test_successful_export_downloads_file(self): + user = UserFactory.create( + is_staff=True, + user_permissions=["forms.view_formstatistics"], + ) + # create some log records for submissions + with freeze_time("2024-12-20T16:44:00+01:00"): + sub1, sub2, sub3 = SubmissionFactory.create_batch( + 3, registration_success=True + ) + plugin = DemoRegistration("demo") + logevent.registration_success(sub1, plugin=plugin) + logevent.registration_success(sub2, plugin=plugin) + logevent.registration_success(sub3, plugin=plugin) + + export_page = self.app.get(self.admin_url, user=user) + form = export_page.forms["export-statistics"] + + # fill out the form and submit it + form["start_date"] = "2024-12-01" + form["end_date"] = "2025-01-01" + response = form.submit() + + self.assertEqual(response.status_code, 200) + self.assertEqual( + response.content_type, + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertGreater(int(response["Content-Length"]), 0) + self.assertEqual( + response["Content-Disposition"], + 'attachment; filename="submissions_2024-12-01_2025-01-01.xlsx"', + ) From d8b516ee017bcf66937d17d513d8d7195bec1e7c Mon Sep 17 00:00:00 2001 From: Sergei Maertens Date: Fri, 20 Dec 2024 17:00:03 +0100 Subject: [PATCH 4/4] :white_check_mark: [#4930] Test the export form implementation Ensures that the filters are correctly implemented. --- .../forms/tests/admin/test_form_statistics.py | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/openforms/forms/tests/admin/test_form_statistics.py b/src/openforms/forms/tests/admin/test_form_statistics.py index f8710fc1dc..b20094ce61 100644 --- a/src/openforms/forms/tests/admin/test_form_statistics.py +++ b/src/openforms/forms/tests/admin/test_form_statistics.py @@ -11,6 +11,7 @@ from openforms.submissions.tests.factories import SubmissionFactory from ...forms import ExportStatisticsForm +from ..factories import FormFactory @disable_admin_mfa() @@ -97,3 +98,92 @@ def test_successful_export_downloads_file(self): response["Content-Disposition"], 'attachment; filename="submissions_2024-12-01_2025-01-01.xlsx"', ) + + def test_export_form_filters(self): + """ + Test that the form filters correctly filter down the matching log records. + """ + plugin = DemoRegistration("demo") + form1, form2, form3 = FormFactory.create_batch(3) + with freeze_time("2024-12-20T16:44:00+01:00"): + registered_submission_1 = SubmissionFactory.create( + form=form1, + registration_success=True, + public_registration_reference="SUB-01", + ) + logevent.registration_success(registered_submission_1, plugin=plugin) + + failed_submission = SubmissionFactory.create( + form=form1, + registration_failed=True, + public_registration_reference="FAIL-01", + ) + logevent.registration_failure(failed_submission, error=Exception("nope")) + with freeze_time("2024-11-20T12:00:00+01:00"): + registered_submission_2 = SubmissionFactory.create( + form=form2, + registration_success=True, + public_registration_reference="SUB-02", + ) + logevent.registration_success(registered_submission_2, plugin=plugin) + + with freeze_time("2024-12-05T12:00:00+01:00"): + registered_submission_3 = SubmissionFactory.create( + form=form3, + registration_success=True, + public_registration_reference="SUB-03", + ) + logevent.registration_success(registered_submission_3, plugin=plugin) + + with freeze_time("2024-12-06T10:00:00+01:00"): + registered_submission_4 = SubmissionFactory.create( + form=form3, + registration_success=True, + public_registration_reference="SUB-04", + ) + logevent.registration_success(registered_submission_4, plugin=plugin) + + with self.subTest("filter on start date"): + export_form1 = ExportStatisticsForm( + data={ + "start_date": "2024-12-14", + "end_date": "2024-12-31", + } + ) + assert export_form1.is_valid() + + dataset1 = export_form1.export() + + self.assertEqual(len(dataset1), 1) + self.assertEqual(dataset1[0][0], "SUB-01") + + with self.subTest("filter on end date"): + export_form2 = ExportStatisticsForm( + data={ + "start_date": "2024-01-01", + "end_date": "2024-12-05", + } + ) + assert export_form2.is_valid() + + dataset2 = export_form2.export() + + self.assertEqual(len(dataset2), 2) + self.assertEqual(dataset2[0][0], "SUB-02") + self.assertEqual(dataset2[1][0], "SUB-03") + + with self.subTest("filter on subset of forms"): + export_form3 = ExportStatisticsForm( + data={ + "start_date": "2024-01-01", + "end_date": "2024-12-31", + "limit_to_forms": [form2.pk, form3.pk], + } + ) + assert export_form3.is_valid() + + dataset3 = export_form3.export() + self.assertEqual(len(dataset3), 3) + self.assertEqual(dataset3[0][0], "SUB-02") + self.assertEqual(dataset3[1][0], "SUB-03") + self.assertEqual(dataset3[2][0], "SUB-04")