#100 refactored filters and make sure the choices are passed to the b…

…ound fields + fixed tag facet counts + tests

cast/filters.py

@@ -19,10 +19,10 @@
 from django.utils.safestring import SafeText, mark_safe
 from django.utils.translation import gettext as _
 from django_filters.fields import ChoiceField as FilterChoiceField
-from wagtail.models import Page, PageQuerySet
+from taggit.models import Tag
+from wagtail.models import PageQuerySet
 
 from cast import appsettings
-from cast.models.pages import PostTag
 from cast.models.snippets import PostCategory
 
 
@@ -114,7 +114,6 @@ def parse_date_facets(value: str) -> datetime:
 class DateFacetChoicesMixin:
     """Just a way to pass the facet counts to the field which displays the choice."""
 
-    extra: dict[str, Any]
     facet_counts: dict[datetime, int] = {}
 
     @property
@@ -124,9 +123,10 @@ def field(self) -> Field:
             date_str = year_month.strftime("%Y-%m")
             label = f"{date_str} ({count})"
             facet_count_choices.append((date_str, label))
-        self.extra["choices"] = facet_count_choices
         super_filter = cast(django_filters.filters.ChoiceFilter, super())  # make mypy happy
-        return super_filter.field
+        field = super_filter.field
+        field.choices = facet_count_choices
+        return field
 
 
 class AllDateChoicesField(FilterChoiceField):
@@ -145,7 +145,6 @@ def valid_value(self, value: str) -> bool:
 
 class DateFacetFilter(DateFacetChoicesMixin, django_filters.filters.ChoiceFilter):
     field_class = AllDateChoicesField
-    facet_count_key = "year_month"
 
     def filter(self, qs: models.QuerySet, value: str) -> models.QuerySet:
         if len(value) == 0:
@@ -191,24 +190,21 @@ def valid_value(self, value: str) -> bool:
 class CountChoicesMixin:
     """Just a way to pass the facet counts to the field which displays the choice."""
 
-    extra: dict[str, Any]
-    facet_count_key: str = ""  # you need to override this!
     facet_counts: dict[str, tuple[str, int]] = {}
     has_facets_with_posts: bool = False
 
     @property
-    def field(self) -> Field:
+    def field(self) -> FilterChoiceField:
         facet_count_choices = []
         for slug, (name, count) in sorted(self.facet_counts.items()):
-            if count == 0:
-                continue
             label = f"{name} ({count})"
             facet_count_choices.append((slug, label))
         if len(facet_count_choices) > 0:
             self.has_facets_with_posts = True
-        self.extra["choices"] = facet_count_choices
         super_filter = cast(django_filters.filters.ChoiceFilter, super())  # make mypy happy
-        return super_filter.field
+        field = super_filter.field
+        field.choices = facet_count_choices
+        return field
 
     @property
     def hide_form_field(self) -> bool:
@@ -218,7 +214,6 @@ def hide_form_field(self) -> bool:
 
 class CategoryFacetFilter(CountChoicesMixin, django_filters.filters.ChoiceFilter):
     field_class = SlugChoicesField
-    facet_count_key = "categories"
 
     def filter(self, qs: models.QuerySet, value: str):
         # Check if value is provided (not None and not an empty list)
@@ -227,13 +222,14 @@ def filter(self, qs: models.QuerySet, value: str):
         return qs
 
     def set_facet_counts(self, queryset: models.QuerySet) -> None:
-        category_count_queryset = PostCategory.objects.annotate(
-            num_posts=models.Count("post", filter=models.Q(post__in=queryset))
-        )
-        category_counts = {}
-        for category in category_count_queryset:
-            category_counts[category.slug] = (category.name, category.num_posts)  # type: ignore
-        self.facet_counts = category_counts
+        self.facet_counts = {
+            slug: (name, num_posts)
+            for slug, name, num_posts in PostCategory.objects.annotate(
+                num_posts=models.Count("post", filter=models.Q(post__in=queryset))
+            )
+            .filter(num_posts__gt=0)
+            .values_list("slug", "name", "num_posts")
+        }  # type: ignore
 
 
 class TagFacetFilter(CountChoicesMixin, django_filters.filters.ChoiceFilter):
@@ -247,14 +243,17 @@ def filter(self, qs: models.QuerySet, value: str):
         return qs
 
     def set_facet_counts(self, queryset: models.QuerySet) -> None:
-        tag_count_queryset = PostTag.objects.annotate(
-            num_posts=models.Count("content_object", filter=models.Q(content_object__in=queryset))
-        )
-        tag_counts = {}
-        for tag in tag_count_queryset:
-            tag_counts[tag.tag.slug] = (tag.tag.name, tag.num_posts)  # type: ignore
-        print("set tag counts: ", tag_counts)
-        self.facet_counts = tag_counts
+        """Count only facets with num_posts > 0."""
+        # Cannot use PostTag.objects.annotate here because the group by clause
+        # would be wrong (GROUP BY "cast_posttag"."id" instead of "tag"."id")
+        self.facet_counts = {
+            slug: (name, num_posts)
+            for slug, name, num_posts in Tag.objects.annotate(
+                num_posts=models.Count("post", filter=models.Q(post__in=queryset))
+            )
+            .filter(num_posts__gt=0)
+            .values_list("slug", "name", "num_posts")
+        }
 
 
 class PostFilterset(django_filters.FilterSet):
@@ -306,27 +305,28 @@ def __init__(
         if data is None:
             data = QueryDict("")
         if queryset is None:
-            queryset = Page.objects.none()
+            from cast.models.pages import Post
+
+            queryset = Post.objects.none()
         super().__init__(data=data, queryset=queryset)
         # Remove filters which are not configured in the settings
         configured_filters = set(appsettings.CAST_FILTERSET_FACETS)
         for filter_name in self.filters.copy().keys():
             if filter_name not in configured_filters:
                 del self.filters[filter_name]
-        if fetch_facet_counts:
-            # avoid running into infinite recursion problems, because
-            # self.get_facet_counts will instantiate PostFilterset again
-            #  -> and again -> and again ...
-            self.set_facet_counts(data, queryset)
-            self.remove_form_fields_that_should_be_hidden()
+        self.set_facet_counts(data, self.qs)
+        self.remove_form_fields_that_should_be_hidden()
+        # delattr(self, "_form")
 
     def set_facet_counts(self, data: QueryDict, queryset: models.QuerySet) -> None:
         # copy data to avoid overwriting
-        data_copy = cast(QueryDict, {k: v for k, v in data.items()})  # cast to make mypy happy
-        facet_queryset = PostFilterset(queryset=queryset, data=data_copy, fetch_facet_counts=False).qs
-        for post_filter in self.filters.values():
+        facet_queryset = queryset
+        for filter_name, post_filter in self.filters.items():
             if hasattr(post_filter, "set_facet_counts"):
                 post_filter.set_facet_counts(facet_queryset)
+                self.form.fields[filter_name] = post_filter.field
+        # this is needed to update the bound form fields with the new choices
+        delattr(self, "_form")
 
     def remove_form_fields_that_should_be_hidden(self) -> None:
         """

tests/filter_test.py

@@ -4,12 +4,7 @@
 from django.http import QueryDict
 from django.utils.timezone import make_aware
 
-from cast.filters import (
-    CategoryFacetFilter,
-    CountFacetWidget,
-    PostFilterset,
-    SlugChoicesField,
-)
+from cast.filters import CountFacetWidget, PostFilterset, SlugChoicesField
 from cast.models import Post, PostCategory
 from tests.factories import PostFactory
 
@@ -59,18 +54,10 @@ def test_validate_category_facet_choice(value, is_valid):
     assert field.valid_value(value) == is_valid
 
 
-def test_category_choices_mixin_filters_facets_with_count_0():
-    ccm = CategoryFacetFilter()  # use Filter instead of Mixin to make super and self.extra work
-    ccm.facet_counts = {"count1": ("count one", 1), "count0": ("count 0", 0)}
-    _ = ccm.field
-    category_slugs = {slug for slug, label in ccm.extra["choices"]}
-    assert category_slugs == {"count1"}
-
-
 @pytest.mark.django_db
 class TestPostFilterset:
     def test_data_is_none(self):
-        filterset = PostFilterset(None)
+        filterset = PostFilterset(data=None)
         assert filterset.data == QueryDict("")
 
     def test_queryset_is_none(self):
@@ -199,3 +186,37 @@ def test_fields_without_facets_having_some_posts_get_removed(self, post):
         # then the category and tag facet are not in the filterset form
         assert "category_facets" not in filterset.form.fields
         assert "tag_facets" not in filterset.form.fields
+
+    def test_tag_used_for_two_posts_should_be_counted_twice(self, post, body):
+        """
+        This didn't happen because `PostTag.annotate` created the wrong group
+        by clause (post_tag_id instead of tag_id).
+        """
+        # given two posts with the same tag
+        post.tags.add("tag")
+        post.save()
+        blog = post.blog
+        another_post = PostFactory(owner=blog.owner, parent=blog, title="another post", slug="another-post", body=body)
+        another_post.tags.add("tag")
+        another_post.save()
+        # when the facet counts are fetched
+        queryset = post.blog.unfiltered_published_posts
+        filterset = PostFilterset(QueryDict(), queryset=queryset)
+        # then the tag is counted twice
+        tag_facets = filterset.filters["tag_facets"].facet_counts
+        assert tag_facets["tag"] == ("tag", 2)
+
+    def test_bound_field_choices_should_be_set_from_facet_counts(self, post):
+        """
+        If this test fails, we are in big trouble since there's a lot of magic
+        necessary to make the facet counts appear in the choices.
+        """
+        # given a queryset with a tagged post
+        post.tags.add("tag")
+        post.save()
+        queryset = post.blog.unfiltered_published_posts
+        # when we create a filterset
+        filterset = PostFilterset(QueryDict(), queryset=queryset)
+        # then the tag appears in the choices of the bound field
+        slugs = {slug for slug, display in filterset.form["tag_facets"].field.choices}
+        assert "tag" in slugs