From 701515686579dbdb520986db84d506d076bf3c42 Mon Sep 17 00:00:00 2001 From: Struan Donald Date: Wed, 19 Jul 2023 13:31:55 +0100 Subject: [PATCH] fixes to ignore responses with no values in counts These should never happen but if they do we should not count them as responses for progress counting purposes. Also do not count two responses to the same question as two responses. --- crowdsourcer/fixtures/ror_responses.json | 6 +-- crowdsourcer/models.py | 9 +++- .../tests/test_right_of_reply_views.py | 34 +++++++++++++++ crowdsourcer/tests/test_views.py | 43 ++++++++++++++++++- crowdsourcer/views/progress.py | 12 ++++-- 5 files changed, 94 insertions(+), 10 deletions(-) diff --git a/crowdsourcer/fixtures/ror_responses.json b/crowdsourcer/fixtures/ror_responses.json index 338fc4f9..89f8f25e 100644 --- a/crowdsourcer/fixtures/ror_responses.json +++ b/crowdsourcer/fixtures/ror_responses.json @@ -4,9 +4,9 @@ "pk": 6, "fields": { "authority": 2, - "question": 273, + "question": 272, "user": 3, - "option": null, + "option": 181, "response_type": 2, "public_notes": "", "page_number": "0", @@ -27,7 +27,7 @@ "authority": 2, "question": 273, "user": 3, - "option": null, + "option": 191, "response_type": 2, "public_notes": "", "page_number": "0", diff --git a/crowdsourcer/models.py b/crowdsourcer/models.py index 5bedfc00..8133ad4b 100644 --- a/crowdsourcer/models.py +++ b/crowdsourcer/models.py @@ -134,10 +134,11 @@ def response_counts( question__in=questions, response_type=response_type, ) + .exclude(id__in=Response.null_responses()) .values("authority") - .annotate(response_count=Count("pk")) + .annotate(response_count=Count("question_id", distinct=True)) .values("response_count") - ) + ), ) if assigned is not None: @@ -227,6 +228,10 @@ def get_absolute_url(self): }, ) + @classmethod + def null_responses(cls): + return cls.objects.filter(option__isnull=True, multi_option__isnull=True) + class Assigned(models.Model): user = models.ForeignKey(User, on_delete=models.CASCADE) diff --git a/crowdsourcer/tests/test_right_of_reply_views.py b/crowdsourcer/tests/test_right_of_reply_views.py index 64f0f441..3fcba2f9 100644 --- a/crowdsourcer/tests/test_right_of_reply_views.py +++ b/crowdsourcer/tests/test_right_of_reply_views.py @@ -50,6 +50,40 @@ def test_council_homepage(self): self.assertEqual(second.total, 2) self.assertEqual(second.complete, 0) + def test_null_answers_ignored(self): + Response.objects.filter(question_id=272, user=3, response_type=2).update( + option=None + ) + url = reverse("authority_ror_sections", args=("Aberdeenshire Council",)) + response = self.client.get(url) + + context = response.context + sections = context["sections"] + + self.assertEqual(len(sections), 7) + + first = sections[0] + self.assertEqual(first.title, "Buildings & Heating") + + self.assertEqual(first.complete, 1) + + def test_duplicate_answers_ignored(self): + Response.objects.filter(question_id=272, user=3, response_type=2).update( + question_id=273 + ) + url = reverse("authority_ror_sections", args=("Aberdeenshire Council",)) + response = self.client.get(url) + + context = response.context + sections = context["sections"] + + self.assertEqual(len(sections), 7) + + first = sections[0] + self.assertEqual(first.title, "Buildings & Heating") + + self.assertEqual(first.complete, 1) + class TestTwoCouncilsAssignmentView(BaseTestCase): def setUp(self): diff --git a/crowdsourcer/tests/test_views.py b/crowdsourcer/tests/test_views.py index ad5be0c7..a1a9321a 100644 --- a/crowdsourcer/tests/test_views.py +++ b/crowdsourcer/tests/test_views.py @@ -106,6 +106,23 @@ def test_completion_stats(self): second = progress[1] self.assertEqual(second["complete"], 2) + def test_completion_stats_ignore_null_responses(self): + response = self.client.get("/") + context = response.context + progress = context["progress"] + + second = progress[1] + self.assertEqual(second["complete"], 1) + + Response.objects.filter(question_id=281, user=2).update(option=None) + + response = self.client.get("/") + context = response.context + progress = context["progress"] + + second = progress[1] + self.assertEqual(second["complete"], 0) + class TestUserSectionProgressView(BaseTestCase): def test_view(self): @@ -120,6 +137,16 @@ def test_view(self): self.assertEqual(context["authorities"][1].num_responses, None) self.assertEqual(context["authorities"][1].num_questions, 2) + def test_null_responses_ignored(self): + Response.objects.filter(question_id=281, user=2).update(option=None) + + url = reverse("section_authorities", args=("Transport",)) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + context = response.context + self.assertEqual(context["authorities"][0].num_responses, 1) + class TestSaveView(BaseTestCase): fixtures = [ @@ -443,7 +470,7 @@ def test_view(self): self.assertEquals(context["councils"]["total"], 4) -class TestSectionProgressView(BaseTestCase): +class TestAllSectionProgressView(BaseTestCase): def test_non_admin_denied(self): response = self.client.get(reverse("all_section_progress")) self.assertEquals(response.status_code, 403) @@ -465,6 +492,20 @@ def test_view(self): self.assertEquals(context["Buildings & Heating"]["assigned"], 1) self.assertEquals(context["Buildings & Heating"]["total"], 4) + def test_null_responses_ignored(self): + Response.objects.filter(question_id=281, user=2).update(option=None) + + u = User.objects.get(username="admin") + self.client.force_login(u) + response = self.client.get(reverse("all_section_progress")) + self.assertEqual(response.status_code, 200) + + context = response.context["progress"] + self.assertEquals(context["Transport"]["complete"], 0) + self.assertEquals(context["Transport"]["started"], 1) + self.assertEquals(context["Transport"]["assigned"], 2) + self.assertEquals(context["Transport"]["total"], 4) + class TestAuthorityLoginView(BaseTestCase): def test_view(self): diff --git a/crowdsourcer/views/progress.py b/crowdsourcer/views/progress.py index 07f90355..2ddce377 100644 --- a/crowdsourcer/views/progress.py +++ b/crowdsourcer/views/progress.py @@ -84,8 +84,9 @@ def get_queryset(self): authority=OuterRef("pk"), response_type=response_type, ) + .exclude(id__in=Response.null_responses()) .values("authority") - .annotate(response_count=Count("pk")) + .annotate(response_count=Count("question_id", distinct=True)) .values("response_count") ) ) @@ -150,6 +151,7 @@ def get_context_data(self, **kwargs): stage = ResponseType.objects.get(type=self.stage) sections = context["sections"] + for section in sections: questions = Question.objects.filter( section=section, how_marked__in=self.types @@ -176,8 +178,9 @@ def get_context_data(self, **kwargs): question__in=question_list, response_type=stage, ) + .exclude(id__in=Response.null_responses()) .values("authority") - .annotate(response_count=Count("pk")) + .annotate(response_count=Count("question_id", distict=True)) .values("response_count") ) ) @@ -260,8 +263,9 @@ def get_context_data(self, **kwargs): authority=OuterRef("pk"), response_type=self.current_stage, ) + .exclude(id__in=Response.null_responses()) .values("authority") - .annotate(response_count=Count("pk")) + .annotate(response_count=Count("question_id", distinct=True)) .values("response_count") ) ) @@ -428,7 +432,7 @@ def get_context_data(self, **kwargs): agree_with_response=False, ) .values("authority") - .annotate(response_count=Count("pk")) + .annotate(response_count=Count("question_id", distict=True)) .values("response_count") ) )