From 2c3b05682fcfafe2de3530d161ed433a2a7c79f8 Mon Sep 17 00:00:00 2001 From: Chris Rose Date: Sun, 9 Aug 2026 15:52:09 -0700 Subject: [PATCH] Convert report views to functional views - test reports more completely - drop the report views that don't matter --- src/nomnom/canonicalize/admin.py | 31 +-- src/nomnom/canonicalize/tests/test_reports.py | 116 +++++++++++ src/nomnom/canonicalize/urls.py | 2 +- src/nomnom/nominate/decorators.py | 10 + src/nomnom/nominate/reports.py | 113 +++++------ src/nomnom/nominate/tests/test_reports.py | 181 ++++++++++++++++-- src/nomnom/nominate/urls.py | 6 +- src/nomnom/reporting.py | 104 ++++------ 8 files changed, 398 insertions(+), 165 deletions(-) create mode 100644 src/nomnom/canonicalize/tests/test_reports.py diff --git a/src/nomnom/canonicalize/admin.py b/src/nomnom/canonicalize/admin.py index 512087d9..eb983b7e 100644 --- a/src/nomnom/canonicalize/admin.py +++ b/src/nomnom/canonicalize/admin.py @@ -1,6 +1,5 @@ import contextvars import csv -import functools import io from collections import Counter from collections.abc import Iterable @@ -19,7 +18,6 @@ from django.http import HttpRequest, HttpResponse, JsonResponse from django.shortcuts import get_object_or_404, redirect, render from django.urls import reverse -from django.utils.decorators import method_decorator from django.utils.html import format_html from django.utils.translation import gettext_lazy as _ from django_admin_action_forms import ( @@ -36,7 +34,12 @@ ) from nomnom.canonicalize.sankey import transform_eph_to_sankey from nomnom.nominate import models as nominate -from nomnom.reporting import Report, ReportView +from nomnom.nominate.decorators import compose_decorators +from nomnom.reporting import ( + Report, + get_raw_report_response, + render_report_in_page, +) from nomnom.wsfs.rules import eph from nomnom.wsfs.rules.constitution_2023 import CountData @@ -473,10 +476,11 @@ def get_admin_url_with_filters(self): admin.site.register(models.Work, WorkAdmin) admin.site.register(models.CanonicalizedNomination, NominationGroupingView) -report_decorators = [ +report_decorators_list = [ user_passes_test(lambda u: u.is_staff, login_url="/admin/login/"), permission_required("nominate.report"), ] +report_decorators = compose_decorators(*report_decorators_list) class BallotReport(Report): @@ -529,17 +533,18 @@ def get_report_row(self, field_names: list[str], row: Any) -> list[Any]: # custom views for EPH and Finalists -@method_decorator(report_decorators, name="get") -class BallotReportView(ReportView): - report_class = BallotReport - html_template_name = "canonicalize/ballots.html" +@report_decorators +def ballot_report_view(request: HttpRequest, category_id: int) -> HttpResponse: + if request.method != "GET": + return HttpResponse(status=405) - @functools.lru_cache - def category(self) -> nominate.Category: - return get_object_or_404(nominate.Category, pk=self.kwargs.get("category_id")) + category = get_object_or_404(nominate.Category, pk=category_id) + report = BallotReport(category) - def prepare_report(self): - return self.get_report_class()(self.category()) + if request.GET.get("html") == "true": + return render_report_in_page(request, report, "canonicalize/ballots.html") + else: + return get_raw_report_response(request, report, is_attachment=True) def nominating_ballots_from_category(category: nominate.Category) -> list[Counter[str]]: diff --git a/src/nomnom/canonicalize/tests/test_reports.py b/src/nomnom/canonicalize/tests/test_reports.py new file mode 100644 index 00000000..b4556aa6 --- /dev/null +++ b/src/nomnom/canonicalize/tests/test_reports.py @@ -0,0 +1,116 @@ +import pytest +from django.contrib.auth import get_user_model +from django.contrib.auth.models import Permission +from django.urls import reverse + +from nomnom.canonicalize import models +from nomnom.canonicalize.admin import BallotReport +from nomnom.nominate import factories +from nomnom.nominate import models as nominate + + +@pytest.fixture(name="user") +def make_user(db): + UserModel = get_user_model() + user = UserModel.objects.create_user( + username="testuser", password="password123", email="reports@example.com" + ) + user.is_staff = True + user.user_permissions.add(Permission.objects.get(codename="report")) + user.user_permissions.add(Permission.objects.get(codename="view_raw_results")) + user.save() + return user + + +@pytest.fixture(name="nomination") +def make_nomination(db, user, category): + nominator = nominate.NominatingMemberProfile.objects.create( + user=user, preferred_name="Test User", member_number="123" + ) + return factories.NominationFactory(nominator=nominator, category=category) + + +def test_report_is_empty_when_no_nominations(client, user, category): + report = BallotReport(category) + (header, *lines) = report.build_report() + assert header.strip() == "nominator,work 1,work 2,work 3,work 4,work 5" + assert lines == [] + + +def test_report_is_empty_when_no_canonicalizations(client, user, category, nomination): + report = BallotReport(category) + (header, *lines) = report.build_report() + assert header.strip() == "nominator,work 1,work 2,work 3,work 4,work 5" + assert lines == [] + + +def test_report_includes_canonicalized_nominations(client, user, category, nomination): + # Canonicalize the nomination + canonical_work = models.Work.objects.create( + name="Canonical Work Name", + category=nomination.category, + ) + canonical_work.nominations.add(nomination) + canonical_work.save() + + ic(models.CanonicalizedNomination.objects.all()) + + report = BallotReport(category) + (header, *lines) = report.build_report() + assert header.strip() == "nominator,work 1,work 2,work 3,work 4,work 5" + assert len(lines) == 1 + assert lines[0].startswith( + f"{nomination.nominator.preferred_name},{canonical_work.name}" + ) + + +@pytest.fixture(name="canonicalized_ballot_report_url") +def make_canonicalized_ballot_report_url(category): + return reverse("canonicalize:ballots", kwargs={"category_id": category.id}) + + +def test_report_view_get_request_unauthenticated( + client, canonicalized_ballot_report_url +): + response = client.get(canonicalized_ballot_report_url) + assert response.status_code == 302 + assert response.url.startswith("/admin/login/") # Redirect to login + + +def test_report_view_get_request_without_permission_redirects_to_login( + client, user, canonicalized_ballot_report_url +): + client.force_login(user) + user.user_permissions.clear() # Remove all permissions + response = client.get(canonicalized_ballot_report_url) + assert response.status_code == 302 + assert response.url.startswith("/accounts/login/") # Redirect to login + + +def test_report_view_get_request_with_permission( + client, user, canonicalized_ballot_report_url +): + client.force_login(user) + response = client.get(canonicalized_ballot_report_url) + assert response.status_code == 200 # OK + assert "text/csv" in response["Content-Type"] + assert "attachment" in response["Content-Disposition"] + + +def test_report_view_get_request_with_permission_contents( + client, user, canonicalized_ballot_report_url, nomination +): + # Canonicalize the nomination + canonical_work = models.Work.objects.create( + name="Canonical Work Name", + category=nomination.category, + ) + canonical_work.nominations.add(nomination) + canonical_work.save() + + client.force_login(user) + response = client.get(canonicalized_ballot_report_url) + assert response.status_code == 200 # OK + content = response.content.decode("utf-8") + assert "nominator,work 1,work 2,work 3,work 4,work 5" in content + assert f"{nomination.nominator.preferred_name},{canonical_work.name}" in content diff --git a/src/nomnom/canonicalize/urls.py b/src/nomnom/canonicalize/urls.py index d0527775..e2fe3cc4 100644 --- a/src/nomnom/canonicalize/urls.py +++ b/src/nomnom/canonicalize/urls.py @@ -7,7 +7,7 @@ urlpatterns = [ path( "/ballots/", - admin.BallotReportView.as_view(), + admin.ballot_report_view, name="ballots", ), path( diff --git a/src/nomnom/nominate/decorators.py b/src/nomnom/nominate/decorators.py index d5d07a34..4083c544 100644 --- a/src/nomnom/nominate/decorators.py +++ b/src/nomnom/nominate/decorators.py @@ -1,4 +1,5 @@ from functools import wraps +import functools from django.core.exceptions import PermissionDenied @@ -35,3 +36,12 @@ def _wrapper_view(request, *args, **kwargs): return _wrapper_view return decorator + + +def compose_decorators(*decorators): + """Compose multiple decorators into a single decorator.""" + return lambda view: functools.reduce( + lambda wrapped, decorator: decorator(wrapped), + reversed(decorators), + view, + ) diff --git a/src/nomnom/nominate/reports.py b/src/nomnom/nominate/reports.py index 72f6aacb..b63a3216 100644 --- a/src/nomnom/nominate/reports.py +++ b/src/nomnom/nominate/reports.py @@ -1,5 +1,4 @@ import csv -import functools from collections.abc import Generator, Iterable from io import StringIO from itertools import groupby @@ -18,20 +17,21 @@ HttpResponse, ) from django.shortcuts import get_object_or_404 -from django.utils.decorators import method_decorator from markdown import markdown from nomnom.nominate import models, tasks -from nomnom.nominate.decorators import user_passes_test_or_forbidden +from nomnom.nominate.decorators import compose_decorators, user_passes_test_or_forbidden from nomnom.nominate.templatetags.nomnom_filters import html_text -from nomnom.reporting import Report, ReportView +from nomnom.reporting import Report -report_decorators = [ +report_decorators_list = [ user_passes_test(lambda u: u.is_staff, login_url="/admin/login/"), permission_required("nominate.report"), ] -raw_report_decorators = [ +report_decorators = compose_decorators(*report_decorators_list) + +raw_report_decorators_list = [ login_required, user_passes_test_or_forbidden(lambda u: u.is_staff), permission_required( @@ -39,6 +39,8 @@ ), ] +raw_report_decorators = compose_decorators(*raw_report_decorators_list) + class NominationsReportBase(Report): extra_fields = ["email", "member_number", "canonical_work", "canonical_category"] @@ -152,29 +154,36 @@ def build_report(self) -> Generator[str, None, None]: yield out.getvalue() -@method_decorator(report_decorators, name="get") -class ElectionReportView(ReportView): - is_attachment: bool = True - content_type: str = "text/csv" - html_template_name: str | None = None +@report_decorators +def nominations_report_view(request: HttpRequest, election_id: str) -> HttpResponse: + election = get_object_or_404(models.Election, slug=election_id) + report = NominationsReport(election) + + response = HttpResponse(report.build_report(), content_type="text/csv") - def get_report_class(self): - return getattr(self, "report_class", NominationsReport) + response["Content-Disposition"] = f'attachment; filename="{report.get_filename()}"' + return response - @functools.lru_cache - def election(self) -> models.Election: - return get_object_or_404(models.Election, slug=self.kwargs.get("election_id")) - def prepare_report(self): - return self.get_report_class()(self.election()) +@report_decorators +def invalidated_nominations_report_view( + request: HttpRequest, election_id: str +) -> HttpResponse: + election = get_object_or_404(models.Election, slug=election_id) + report = InvalidatedNominationsReport(election) + response = HttpResponse(report.build_report(), content_type="text/csv") -class Nominations(ElectionReportView): - report_class = NominationsReport + response["Content-Disposition"] = f'attachment; filename="{report.get_filename()}"' + return response -class InvalidatedNominations(ElectionReportView): - report_class = InvalidatedNominationsReport +class BoomReport(Report): + def __init__(self, *args, **kwargs): + super().__init__() + + def build_report(self, header=True) -> Generator[str, None, None]: + raise Exception("Boom! This is a test exception for error handling.") class RanksReport(Report): @@ -252,53 +261,31 @@ def build_report(self) -> Generator[str, None, None]: yield out.getvalue() -@method_decorator(raw_report_decorators, name="dispatch") -class AllVotes(ElectionReportView): - content_type = "text/plain" - is_attachment = True - report_class = RanksReport - html_template_name = "nominate/reports/voting_report.html" - - def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: - # get the user's email address - recipient = request.user.email - if not recipient: - messages.error( - request, - "You do not have an email address configured; we cannot send you the report", - ) - return HttpResponse("") +@raw_report_decorators +def send_all_votes_report(request: HttpRequest, election_id: str) -> HttpResponse: + if request.method != "POST": + return HttpResponse(status=405) - tasks.send_rank_report.delay( - election_id=self.election().slug, - recipients=recipient, - exclude_configured_recipients=True, - ) + recipient = request.user.email - messages.success( + if not recipient: + messages.error( request, - f"The full election report will be sent to {recipient}; this may take up to a half hour.", + "You do not have an email address configured; we cannot send you the report", ) - return HttpResponse("") + election = get_object_or_404(models.Election, slug=election_id) -@method_decorator(raw_report_decorators, name="get") -class CategoryVotes(ElectionReportView): - content_type = "text/csv" - is_attachment = False - - report_class = CategoryVotingReport - - @functools.lru_cache - def category(self) -> models.Category: - return get_object_or_404(models.Category, id=self.kwargs.get("category_id")) - - @functools.lru_cache - def report(self) -> Report: - return CategoryVotingReport(category=self.category()) + tasks.send_rank_report.delay( + election_id=election.slug, + recipients=recipient, + exclude_configured_recipients=True, + ) + messages.success( + request, + f"The full election report will be sent to {recipient}; this may take up to a half hour.", + ) -@method_decorator(raw_report_decorators, name="get") -class ElectionResults(ElectionReportView): - report_class = InvalidatedNominationsReport + return HttpResponse("") diff --git a/src/nomnom/nominate/tests/test_reports.py b/src/nomnom/nominate/tests/test_reports.py index 09022a97..9704426d 100644 --- a/src/nomnom/nominate/tests/test_reports.py +++ b/src/nomnom/nominate/tests/test_reports.py @@ -1,12 +1,15 @@ import csv from io import StringIO +from unittest import mock from urllib.parse import parse_qs, urlparse import pytest from django.contrib.auth import get_user_model from django.contrib.auth.models import Permission +from django.contrib.messages.api import get_messages from django.urls import reverse from freezegun import freeze_time + from nomnom.nominate import factories, models, reports pytestmark = pytest.mark.usefixtures("db") @@ -15,10 +18,12 @@ @pytest.fixture(name="user") def make_user(db): UserModel = get_user_model() - user = UserModel.objects.create_user(username="testuser", password="password123") + user = UserModel.objects.create_user( + username="testuser", password="password123", email="reports@example.com" + ) user.is_staff = True - perm = Permission.objects.get(codename="report") - user.user_permissions.add(perm) + user.user_permissions.add(Permission.objects.get(codename="report")) + user.user_permissions.add(Permission.objects.get(codename="view_raw_results")) user.save() return user @@ -99,16 +104,36 @@ def make_nomination(db, user, category): return factories.NominationFactory(nominator=nominator, category=category) -@pytest.fixture(name="report_url") -def make_report_url(): +@pytest.fixture(name="invalidated_nomination") +def make_nomination_with_invalidated(db, user, category): + nominator = factories.NominatingMemberProfileFactory() + nomination = factories.NominationFactory(nominator=nominator, category=category) + nomination.save() + models.NominationAdminData.objects.create( + nomination=nomination, + valid_nomination=False, + ).save() + return nomination + + +@pytest.fixture(name="nomination_report_url") +def make_nomination_report_url(): return reverse( "election:nomination-report", kwargs={"election_id": "election_slug"} ) -def test_nomination_view_dispatch(client, report_url, user, nomination): +@pytest.fixture(name="invalid_nominations_report_url") +def make_invalid_nominations_report_url(): + return reverse( + "election:invalidated-nomination-report", + kwargs={"election_id": "election_slug"}, + ) + + +def test_nomination_view_dispatch(client, nomination_report_url, user, nomination): client.force_login(user) - response = client.get(report_url, HTTP_ACCEPT="text/csv") + response = client.get(nomination_report_url, HTTP_ACCEPT="text/csv") assert response.status_code == 200 assert response["Content-Type"] == "text/csv" @@ -127,25 +152,90 @@ def test_nomination_report_query_set(nominations_report, nomination): assert queryset == [nomination] -def test_nomination_view_dispatch_unauthenticated(client, report_url): - response = client.get(report_url, HTTP_ACCEPT="text/csv") +def test_nomination_view_dispatch_unauthenticated(client, nomination_report_url): + response = client.get(nomination_report_url, HTTP_ACCEPT="text/csv") assert response.status_code == 302 parts = urlparse(response.url) query = parse_qs(parts.query) - assert query["next"][0] == report_url + assert query["next"][0] == nomination_report_url -def test_nomination_view_get_unauthenticated(client, report_url): - response = client.get(report_url, HTTP_ACCEPT="text/csv") +def test_nomination_view_get_unauthenticated(client, nomination_report_url): + response = client.get(nomination_report_url, HTTP_ACCEPT="text/csv") assert response.status_code == 302 parts = urlparse(response.url) query = parse_qs(parts.query) - assert query["next"][0] == report_url + assert query["next"][0] == nomination_report_url + + +@pytest.mark.usefixtures("nomination") +@pytest.mark.parametrize("required_permission", ("report",)) +def test_nomination_view_permission_required( + client, nomination_report_url, user, required_permission +): + client.force_login(user) + user.user_permissions.remove(Permission.objects.get(codename=required_permission)) + + response = client.get(nomination_report_url, HTTP_ACCEPT="text/csv") + + assert response.status_code == 302 # this view sends the user to a login + + +@pytest.mark.usefixtures("nomination") +@pytest.mark.parametrize("required_permission", ("report",)) +def test_invalidated_nomination_view_permission_required( + client, invalid_nominations_report_url, user, required_permission +): + client.force_login(user) + user.user_permissions.remove(Permission.objects.get(codename=required_permission)) + + response = client.get(invalid_nominations_report_url, HTTP_ACCEPT="text/csv") + + assert response.status_code == 302 # this view sends the user to a login + + +@pytest.mark.usefixtures("invalidated_nomination") +def test_nomination_view_get( + client, + nomination_report_url, + user, + nomination, +): + client.force_login(user) + response = client.get(nomination_report_url, HTTP_ACCEPT="text/csv") + + assert response.status_code == 200 + + content = StringIO(response.content.decode()) + reader = csv.reader(content) + + header = next(reader) + expected_header = [ + field.name for field in models.Nomination._meta.fields + ] + reports.NominationsReport.extra_fields + assert header == expected_header + + content = StringIO(response.content.decode()) + reader = csv.DictReader(content) + + data_row = next(reader) + + assert sorted(data_row.keys()) == sorted(header) + + assert data_row["field_1"] == nomination.field_1 + assert data_row["field_2"] == nomination.field_2 + assert data_row["field_3"] == nomination.field_3 + + with pytest.raises(StopIteration): # No more data + next(reader) -def test_nomination_view_get(client, report_url, user, nomination): +@pytest.mark.usefixtures("nomination") +def test_invalidated_nomination_view_get( + client, invalid_nominations_report_url, user, invalidated_nomination +): client.force_login(user) - response = client.get(report_url, HTTP_ACCEPT="text/csv") + response = client.get(invalid_nominations_report_url, HTTP_ACCEPT="text/csv") assert response.status_code == 200 @@ -165,5 +255,66 @@ def test_nomination_view_get(client, report_url, user, nomination): assert sorted(data_row.keys()) == sorted(header) + assert data_row["field_1"] == invalidated_nomination.field_1 + assert data_row["field_2"] == invalidated_nomination.field_2 + assert data_row["field_3"] == invalidated_nomination.field_3 + with pytest.raises(StopIteration): # No more data next(reader) + + +@pytest.fixture(name="valid_ranks_report_url") +def make_valid_ranks_report_url(): + return reverse("election:vote-report", kwargs={"election_id": "election_slug"}) + + +def test_valid_ranks_report_disallows_get(client, valid_ranks_report_url, user): + client.force_login(user) + + response = client.get(valid_ranks_report_url) + + assert response.status_code == 405 + + +def test_valid_ranks_report_404_when_no_election(client, valid_ranks_report_url, user): + client.force_login(user) + + response = client.post(valid_ranks_report_url) + + assert response.status_code == 404 + + +@pytest.mark.parametrize("required_permission", ("view_raw_results", "report")) +def test_valid_ranks_report_permission_required( + client, valid_ranks_report_url, user, required_permission +): + client.force_login(user) + user.user_permissions.remove(Permission.objects.get(codename=required_permission)) + + response = client.post(valid_ranks_report_url) + + assert response.status_code == 403 + + +@pytest.mark.usefixtures("election") +def test_valid_ranks_report_post_submits_request(client, valid_ranks_report_url, user): + + client.force_login(user) + + with mock.patch("nomnom.nominate.tasks.send_rank_report.delay") as delay: + response = client.post( + valid_ranks_report_url, + ) + + assert "text/html" in response["Content-Type"] + + # an email task should have been queued to send the report to the user + delay.assert_called_once_with( + election_id="election_slug", + recipients=user.email, + exclude_configured_recipients=True, + ) + + # a flash message should be in the response + messages = [str(m) for m in get_messages(response.wsgi_request)] + assert any("The full election report will be sent" for m in messages) diff --git a/src/nomnom/nominate/urls.py b/src/nomnom/nominate/urls.py index 71e0c89b..fde23314 100644 --- a/src/nomnom/nominate/urls.py +++ b/src/nomnom/nominate/urls.py @@ -34,18 +34,18 @@ # Report Views path( "/nominations/", - reports.Nominations.as_view(), + reports.nominations_report_view, name="nomination-report", ), path( "/invalidated-nominations/", - reports.InvalidatedNominations.as_view(), + reports.invalidated_nominations_report_view, name="invalidated-nomination-report", ), # All Hugo Votes path( "/admin/votes/", - reports.AllVotes.as_view(), + reports.send_all_votes_report, name="vote-report", ), # The result of the Hugo Award elections, as of the present. diff --git a/src/nomnom/reporting.py b/src/nomnom/reporting.py index 3af37d67..2ef5c5b1 100644 --- a/src/nomnom/reporting.py +++ b/src/nomnom/reporting.py @@ -1,5 +1,4 @@ import csv -import functools from abc import abstractmethod from collections.abc import Generator, Iterable from datetime import UTC, datetime @@ -8,9 +7,8 @@ from typing import Any from django.db.models import QuerySet -from django.http import HttpRequest, HttpResponse, HttpResponseBase +from django.http import HttpRequest, HttpResponse from django.shortcuts import render -from django.views.generic import View class Report: @@ -74,71 +72,37 @@ def process(self, query_set: QuerySet) -> Iterable[Any]: yield from query_set -class ReportView(View): - is_attachment: bool = True - content_type: str = "text/csv" - html_template_name: str | None = None - - def get_report_class(self): - report_class = getattr(self, "report_class") - if report_class is None: - raise Exception(f"No report_class defined for {self.__class__}") - - return report_class - - @functools.lru_cache - def report(self) -> Report: - return self.prepare_report() - - def prepare_report(self): - return self.get_report_class()() - - def get_writer(self, response) -> Any: - return csv.writer(response) - - def dispatch( - self, request: HttpRequest, *args: Any, **kwargs: Any - ) -> HttpResponseBase: - return super().dispatch(request, *args, **kwargs) - - def get(self, request, *args, **kwargs): - report = self.report() - if ( - self.html_template_name is not None - and self.request.GET.get("html") == "true" - ): - return self.render_report_in_page( - request, self.html_template_name, report, *args, **kwargs - ) - else: - return self.get_raw_report_response(request, report, *args, **kwargs) - - def get_raw_report_response( - self, request: HttpRequest, report: Report, *args, **kwargs - ) -> HttpResponse: - response = HttpResponse(report.build_report(), content_type=self.content_type) - if self.is_attachment: - response["Content-Disposition"] = ( - f'attachment; filename="{report.get_filename()}"' - ) - return response - - def render_report_in_page( - self, - request: HttpRequest, - html_template_name: str, - report: Report, - *args, - **kwargs, - ): - # report_content = StringIO() - report_content = report.get_report_content() - - return render( - request, - html_template_name, - { - "report": report, - "report_content": report_content, - }, +# View utilities for reporting +def get_raw_report_response( + request: HttpRequest, + report: Report, + content_type: str = "text/csv", + is_attachment: bool = True, +) -> HttpResponse: + response = HttpResponse( + report.build_report(), content_type=report.get_content_type() + ) + if is_attachment: + response["Content-Disposition"] = ( + f'attachment; filename="{report.get_filename()}"' ) + return response + + +def render_report_in_page( + request: HttpRequest, + report: Report, + template_name: str, + content_type: str = "text/html", +) -> HttpResponse: + report_content = report.get_report_content() + + return render( + request, + template_name, + { + "report": report, + "report_content": report_content, + }, + content_type=content_type, + )