Improve bill annotations to reduce queries (#303)

* Improve bill annotations to reduce queries

* Fix tests

* Add type annotation back to manager create method

* Remove duplicate code

Author: mfosterw

democrasite/templates/webiscite/snippets/vote.html

@@ -21,7 +21,7 @@
        data-bs-toggle="tooltip" data-bs-placement="bottom" data-bs-trigger="click" title="{% trans 'Log in to vote' %}"
      {% endif %}
    {% endif %}>
-  ✓ {% trans 'Yes' %}: <span class="num-yes-votes" id="num-vote-yes-{{ bill.id }}">{{ bill.yes_votes.count }}</span>
+  ✓ {% trans 'Yes' %}: <span class="num-yes-votes" id="num-vote-yes-{{ bill.id }}">{{ bill.yes_count }}</span>
 </a>
 
 <a href="javascript:;"
@@ -35,5 +35,5 @@
        data-bs-toggle="tooltip" data-bs-placement="bottom" data-bs-trigger="click" title="{% trans 'Log in to vote' %}"
      {% endif %}
    {% endif %}>
-  X {% trans 'No' %}: <span class="num-no-votes" id="num-vote-no-{{ bill.id }}">{{ bill.no_votes.count }}</span>
+  X {% trans 'No' %}: <span class="num-no-votes" id="num-vote-no-{{ bill.id }}">{{ bill.no_count }}</span>
 </a>

democrasite/webiscite/api/views.py

@@ -53,7 +53,7 @@ def has_object_permission(self, request, view, obj: Bill) -> bool:
 )
 class BillViewSet(RetrieveModelMixin, ListModelMixin, UpdateModelMixin, GenericViewSet):
     serializer_class = BillSerializer
-    queryset = Bill.objects.select_related("author", "pull_request")
+    queryset = Bill.objects.all()
     permission_classes = [IsAuthorOrReadOnly]
 
     def get_serializer_context(self):
@@ -84,8 +84,7 @@ def vote(self, request: Request, pk):
         except ClosedBillVoteError as err:
             raise PermissionDenied(str(err)) from err
 
-        return Response(
-            {"yes_votes": bill.yes_votes.count(), "no_votes": bill.no_votes.count()}
-        )
+        bill = Bill.objects.get(pk=pk)
+        return Response({"yes_votes": bill.yes_count, "no_votes": bill.no_count})
 
     # TODO: Prefetch bill list votes

democrasite/webiscite/managers.py

@@ -47,18 +47,19 @@ def create_from_github(self, pr: dict[str, Any]) -> T:
 
 class BillManager[T](models.Manager):
     def get_queryset(self):
-        """Return a queryset with pull_request pre-fetched and vote percentages added.
+        """Return a queryset with related models pre-fetched and vote amounts added.
 
-        All Bill querysets include ``total_votes``, ``yes_percent``, and
-        ``no_percent``
-        annotations, and are ordered by creation date.
+        All Bill querysets include ``total_votes``, ``yes_count``, ``yes_percent``,
+        ``no_count`` and ``no_percent`` annotations, prefetch pull_request and author,
+        and are ordered by creation date.
         """
         return (
             super()
             .get_queryset()
-            .select_related("pull_request")
+            .select_related("pull_request", "author")
             .annotate(
                 total_votes=models.Count("vote"),
+                yes_count=models.Count("vote", filter=models.Q(vote__support=True)),
                 yes_percent=models.Case(
                     models.When(
                         models.Q(total_votes__gt=0),
@@ -69,6 +70,7 @@ def get_queryset(self):
                     default=models.Value(0),
                     output_field=models.FloatField(),
                 ),
+                no_count=models.Count("vote", filter=models.Q(vote__support=False)),
                 no_percent=models.Case(
                     models.When(
                         models.Q(total_votes__gt=0),
@@ -109,10 +111,7 @@ def annotate_user_vote(
         )
 
     def create_from_github(
-        self,
-        pull_request: "PullRequest",
-        desc: str,
-        author_uid: str,
+        self, pull_request: "PullRequest", desc: str, author_uid: str
     ) -> T | None:
         """Validate and create a :class:`~democrasite.webiscite.models.Bill` from a
         GitHub pull request

democrasite/webiscite/models.py

@@ -158,6 +158,13 @@ class Status(models.TextChoices):
 
     objects = BillManager()
 
+    # Annotations from default manager query
+    total_votes = 0
+    yes_count = 0
+    yes_percent = 0
+    no_count = 0
+    no_percent = 0
+
     class Meta:
         constraints = [
             models.UniqueConstraint(
@@ -184,6 +191,29 @@ def log(self, msg, *args, level=logging.INFO):
         )
         # f-string necessary to let string interpolation work in msg
 
+    def save(self, *args, **kwargs):
+        created = self._state.adding
+        super().save(*args, **kwargs)
+        if created and self.status != self.Status.DRAFT:
+            self._schedule_submit_task()
+
+    def _schedule_submit_task(self) -> None:
+        """Create a periodic task to submit this bill after the voting period."""
+
+        voting_ends, __ = IntervalSchedule.objects.get_or_create(
+            every=settings.WEBISCITE_VOTING_PERIOD, period=IntervalSchedule.DAYS
+        )
+        self._submit_task = PeriodicTask.objects.create(
+            interval=voting_ends,
+            name=f"bill_submit:{self.id}",
+            task="democrasite.webiscite.tasks.submit_bill",
+            args=json.dumps([self.id]),
+            one_off=True,
+            last_run_at=timezone.now(),
+        )
+        super().save()
+        self.log("Scheduled %s", self._submit_task.name)
+
     def get_absolute_url(self) -> str:
         """Returns URL to view this Bill instance"""
         return reverse("webiscite:bill-detail", kwargs={"pk": self.id})
@@ -196,20 +226,12 @@ def get_vote_url(self) -> str:
         """Returns URL for the current user to vote on this Bill instance"""
         return reverse("webiscite:bill-vote", kwargs={"pk": self.id})
 
-    @property
-    def yes_votes(self) -> models.QuerySet[User]:
-        return self.votes.filter(vote__support=True)
-
-    @property
-    def no_votes(self) -> models.QuerySet[User]:
-        return self.votes.filter(vote__support=False)
-
     def vote(self, user: User, *, support: bool) -> None:
         """Sets the given user's vote based on the support parameter
 
         If the user already voted the way the method would set, their vote is
-        removed from the bill (i.e. if ``user`` is in ``bill.yes_votes`` and support is
-        ``True``, ``user`` is removed from ``bill.yes_votes``)
+        removed from the bill (i.e. if the user previously voted yes and support is
+        ``True``, their vote is removed)
 
         Args:
             user (User): The user voting on the bill
@@ -255,34 +277,11 @@ def user_supports(self, user: User) -> bool | None:
         else:
             return vote.support
 
-    def save(self, *args, **kwargs):
-        created = self._state.adding
-        super().save(*args, **kwargs)
-        if created and self.status != self.Status.DRAFT:
-            self._schedule_submit_task()
-
-    def _schedule_submit_task(self) -> None:
-        """Create a periodic task to submit this bill after the voting period."""
-
-        voting_ends, __ = IntervalSchedule.objects.get_or_create(
-            every=settings.WEBISCITE_VOTING_PERIOD, period=IntervalSchedule.DAYS
-        )
-        self._submit_task = PeriodicTask.objects.create(
-            interval=voting_ends,
-            name=f"bill_submit:{self.id}",
-            task="democrasite.webiscite.tasks.submit_bill",
-            args=json.dumps([self.id]),
-            one_off=True,
-            last_run_at=timezone.now(),
-        )
-        super().save()
-        self.log("Scheduled %s", self._submit_task.name)
-
     def close(self, status: "Bill.Status" = Status.CLOSED) -> None:
         """Close the bill and disable its submit task"""
         self.status = status
         self.save()
-        self.log("Closed")
+        self.log(status)
 
         if self._submit_task is not None:
             self._submit_task.enabled = False
@@ -310,20 +309,18 @@ def submit(self) -> None:
         self.save()
 
     def _check_approval(self) -> "Bill.Status":
-        total_votes = self.votes.count()
-        if total_votes < settings.WEBISCITE_MINIMUM_QUORUM:
+        if self.total_votes < settings.WEBISCITE_MINIMUM_QUORUM:
             self.log("Rejected due to insufficient votes")
             return self.Status.FAILED
 
-        approval = self.yes_votes.count() / total_votes
         if self.constitutional:
-            approved = approval > settings.WEBISCITE_SUPERMAJORITY
+            approved = self.yes_percent / 100 > settings.WEBISCITE_SUPERMAJORITY
         else:
-            approved = approval > settings.WEBISCITE_NORMAL_MAJORITY
+            approved = self.yes_percent / 100 > settings.WEBISCITE_NORMAL_MAJORITY
 
         if not approved:
-            self.log("Rejected with %s%% approval", approval * 100)
+            self.log("Rejected with %s%% approval", self.yes_percent)
             return self.Status.REJECTED
 
-        self.log("Approved with %s%% approval", approval * 100)
+        self.log("Approved with %s%% approval", self.yes_percent)
         return self.Status.APPROVED

democrasite/webiscite/tests/api/test_views.py

@@ -115,7 +115,7 @@ def test_vote(self, bill: Bill, user: User, api_client: APIClient):
         assert response.status_code == HTTPStatus.OK
         assert response.data["yes_votes"] == 1
         assert response.data["no_votes"] == 0
-        assert bill.yes_votes.filter(pk=user.pk).exists()
+        assert bill.votes.filter(pk=user.pk).exists()
 
         response = api_client.post(url, {"support": False})
 

democrasite/webiscite/tests/test_models.py

@@ -240,27 +240,27 @@ def test_publish_not_draft(self, bill: Bill):
 class TestBillVote:
     def test_bill_vote_yes_toggle(self, bill: Bill, user: User):
         bill.vote(user, support=True)
-        assert bill.yes_votes.filter(pk=user.id).exists()
+        assert bill.votes.filter(pk=user.id, vote__support=True).exists()
         bill.vote(user, support=True)
-        assert not bill.yes_votes.filter(pk=user.id).exists()
+        assert not bill.votes.filter(pk=user.id, vote__support=True).exists()
 
     def test_bill_vote_no_toggle(self, bill: Bill, user: User):
         bill.vote(user, support=False)
-        assert bill.no_votes.filter(pk=user.id).exists()
+        assert bill.votes.filter(pk=user.id, vote__support=False).exists()
         bill.vote(user, support=False)
-        assert not bill.no_votes.filter(pk=user.id).exists()
+        assert not bill.votes.filter(pk=user.id, vote__support=False).exists()
 
     def test_bill_vote_yes_to_no_switch(self, bill: Bill, user: User):
         bill.vote(user, support=True)
         bill.vote(user, support=False)
-        assert not bill.yes_votes.filter(pk=user.id).exists()
-        assert bill.no_votes.filter(pk=user.id).exists()
+        assert not bill.votes.filter(pk=user.id, vote__support=True).exists()
+        assert bill.votes.filter(pk=user.id, vote__support=False).exists()
 
     def test_bill_vote_no_to_yes_switch(self, bill: Bill, user: User):
         bill.vote(user, support=False)
         bill.vote(user, support=True)
-        assert not bill.no_votes.filter(pk=user.id).exists()
-        assert bill.yes_votes.filter(pk=user.id).exists()
+        assert not bill.votes.filter(pk=user.id, vote__support=False).exists()
+        assert bill.votes.filter(pk=user.id, vote__support=True).exists()
 
     def test_bill_not_open(self, user: User):
         bill = BillFactory.create(status=Bill.Status.CLOSED)
@@ -300,6 +300,7 @@ def test_bill_rejected(self, bill: Bill):
         Vote.objects.bulk_create(
             [Vote(bill=bill, user=voter, support=False) for voter in voters]
         )
+        bill = Bill.objects.get(id=bill.id)  # set annotations
 
         bill.submit()
 
@@ -311,6 +312,7 @@ def test_constitutional_bill_rejected(self):
         Vote.objects.bulk_create(
             [Vote(bill=bill, user=voter, support=False) for voter in voters]
         )
+        bill = Bill.objects.get(id=bill.id)  # set annotations
 
         bill.submit()
 
@@ -321,6 +323,7 @@ def test_bill_passed(self, bill: Bill):
         Vote.objects.bulk_create(
             [Vote(bill=bill, user=voter, support=True) for voter in voters]
         )
+        bill = Bill.objects.get(id=bill.id)  # set annotations
 
         bill.submit()
 

democrasite/webiscite/views.py

@@ -163,9 +163,10 @@ def vote_view(request: http.HttpRequest, pk: int) -> http.HttpResponse:
     except ClosedBillVoteError as err:
         return http.HttpResponseForbidden(str(err))
 
+    bill = Bill.objects.get(pk=pk)  # refresh vote annotations
     return http.JsonResponse(
         {
-            "yes-votes": bill.yes_votes.count(),
-            "no-votes": bill.no_votes.count(),
+            "yes-votes": bill.yes_count,
+            "no-votes": bill.no_count,
         }
     )