Merge pull request #71 from joeyjurjens/fix-popularity-annotate

Fix popularity annotation as it causes weird results due to nested join in count

oscar_elasticsearch/search/api/product.py

@@ -1,6 +1,6 @@
 from odin.codecs import dict_codec
 
-from django.db.models import QuerySet, Count, Q
+from django.db.models import QuerySet, Count, Subquery, OuterRef, IntegerField
 from django.utils import timezone
 
 from dateutil.relativedelta import relativedelta
@@ -64,12 +64,16 @@ def make_documents(self, objects):
 
         # Annotate the queryset with popularity to avoid the need of n+1 queries
         objects = objects.annotate(
-            popularity=Count(
-                "line",
-                filter=Q(
-                    line__order__date_placed__gte=timezone.now()
-                    - relativedelta(months=settings.MONTHS_TO_RUN_ANALYTICS)
-                ),
+            popularity=Subquery(
+                Line.objects.filter(
+                    product=OuterRef("pk"),
+                    order__date_placed__gte=timezone.now()
+                    - relativedelta(months=settings.MONTHS_TO_RUN_ANALYTICS),
+                )
+                .values("product")
+                .annotate(count=Count("id"))
+                .values("count"),
+                output_field=IntegerField(),
             )
         )
 

oscar_elasticsearch/search/api/search.py

@@ -300,6 +300,7 @@ def search(
         search_type=es_settings.SEARCH_QUERY_TYPE,
         search_operator=es_settings.SEARCH_QUERY_OPERATOR,
         scoring_functions=None,
+        raw_results=False,
     ):
         search_results = search(
             self.get_index_name(),
@@ -317,6 +318,8 @@ def search(
 
         total_hits = search_results["hits"]["total"]["value"]
 
+        if raw_results:
+            return search_results, total_hits
         return self.make_queryset(search_results), total_hits
 
     def facet_search(

oscar_elasticsearch/search/tests.py

@@ -7,7 +7,11 @@
 from django.urls import reverse
 
 from oscar.core.loading import get_class, get_model
-from oscar.test.factories import ProductFactory
+from oscar.test.factories import (
+    ProductFactory,
+    OrderFactory,
+    OrderLineFactory,
+)
 
 import oscar_elasticsearch.search.format
 import oscar_elasticsearch.search.utils
@@ -227,6 +231,47 @@ def create_parent_child_products():
         with self.assertNumQueries(23):
             call_command("update_index_products")
 
+    def test_popularity_based_on_order_lines(self):
+        products = []
+        for i in range(5):
+            product = ProductFactory(
+                title=f"VERY UNIQUE TITLE - {i + 1}", categories=[]
+            )
+            products.append(product)
+
+        call_command("update_index_products")
+
+        results, total_hits = ProductElasticsearchIndex().search(
+            query_string="VERY UNIQUE TITLE", raw_results=True
+        )
+        self.assertEqual(total_hits, 5)
+        for hit in results["hits"]["hits"]:
+            self.assertEqual(
+                hit["_source"]["popularity"],
+                None,
+                "no orders place yet, so popularity should be None",
+            )
+
+        order = OrderFactory()
+        for i, product in enumerate(products):
+            quantity = int(product.title.split("-")[-1].strip())
+            for _ in range(quantity):
+                OrderLineFactory(order=order, product=product)
+
+        call_command("update_index_products")
+
+        results, total_hits = ProductElasticsearchIndex().search(
+            query_string="VERY UNIQUE TITLE", raw_results=True
+        )
+        self.assertEqual(total_hits, 5)
+        for hit in results["hits"]["hits"]:
+            title = hit["_source"]["title"]
+            quantity = int(title.split("-")[-1].strip())
+            self.assertEqual(
+                hit["_source"]["popularity"],
+                quantity,
+            )
+
 
 class TestBrowsableItems(TestCase):