Hide empty reviews, apart from admins and your own (#5227)

docs/topics/api/reviews.rst

@@ -44,6 +44,10 @@ user has already posted a review for the current version of an add-on.
    can change that with the ``filter=with_deleted`` query parameter, which
    requires the Addons:Edit permission.
 
+   Only non-empty reviews (reviews with both a rating and text body) are returned,
+   unless a) the reviews were created by the user making the API request or
+   b) the user has the Addons:Edit permission.
+
 ------
 Detail
 ------

src/olympia/addons/tests/test_views.py

@@ -2,6 +2,7 @@
 from datetime import datetime, timedelta
 from decimal import Decimal
 import json
+import random
 import re
 
 from django import test
@@ -17,7 +18,7 @@ from waffle.testutils import override_switch
 from olympia import amo
 from olympia.amo.tests import APITestClient, ESTestCase, TestCase
 from olympia.amo.helpers import numberfmt, urlparams
-from olympia.amo.tests import addon_factory, version_factory
+from olympia.amo.tests import addon_factory, user_factory, version_factory
 from olympia.amo.urlresolvers import reverse
 from olympia.addons.utils import generate_addon_guid
 from olympia.abuse.models import AbuseReport
@@ -28,6 +29,7 @@ from olympia.bandwagon.models import Collection
 from olympia.constants.categories import CATEGORIES
 from olympia.files.models import WebextPermission, WebextPermissionDescription
 from olympia.paypal.tests.test import other_error
+from olympia.reviews.models import Review
 from olympia.stats.models import Contribution
 from olympia.users.helpers import users_list
 from olympia.users.models import UserProfile
@@ -1075,6 +1077,29 @@ class TestDetailPage(TestCase):
         assert pq(content)('.manage-button a').eq(2).attr('href') == (
             reverse('zadmin.addon_manage', args=[self.addon.slug]))
 
+    def test_reviews(self):
+        def create_review(body='review text'):
+            return Review.objects.create(
+                addon=self.addon, user=user_factory(),
+                rating=random.randrange(0, 6),
+                body=body)
+
+        url = reverse('addons.detail', args=['a3615'])
+
+        create_review()
+        response = self.client.get(url, follow=True)
+        assert len(response.context['reviews']) == 1
+
+        # Add a new review but with no body - shouldn't be shown on detail page
+        create_review(body=None)
+        response = self.client.get(url, follow=True)
+        assert len(response.context['reviews']) == 1
+
+        # Test one last time in case caching
+        create_review()
+        response = self.client.get(url, follow=True)
+        assert len(response.context['reviews']) == 2
+
     def get_pq(self):
         return pq(self.client.get(self.url).content)
 

src/olympia/addons/views.py

@@ -133,7 +133,7 @@ def extension_detail(request, addon):
         'grouped_ratings': GroupedRating.get(addon.id),
         'review_form': ReviewForm(),
         'reviews': Review.without_replies.all().filter(
-            addon=addon, is_latest=True),
+            addon=addon, is_latest=True).exclude(body=None),
         'get_replies': Review.get_replies,
         'collections': collections.order_by('-subscribers')[:3],
         'abuse_form': AbuseForm(request=request),

src/olympia/reviews/tests/test_views.py

@@ -105,6 +105,31 @@ class TestViews(ReviewTest):
         assert r.rating is None
         assert item.attr('data-rating') == ''
 
+    def test_empty_reviews_in_list(self):
+        def create_review(body='review text', user=None):
+            return Review.objects.create(
+                addon=self.addon, user=user or user_factory(),
+                rating=3, body=body)
+
+        url = helpers.url('addons.reviews.list', self.addon.slug)
+
+        create_review()
+        create_review(body=None)
+        create_review(
+            body=None,
+            user=UserProfile.objects.get(email='trev@adblockplus.org'))
+
+        # Don't show the reviews with no body.
+        assert len(self.client.get(url).context['reviews']) == 2
+
+        self.login_dev()
+        # Except if it's your review
+        assert len(self.client.get(url).context['reviews']) == 3
+
+        # Or you're an admin
+        self.login_admin()
+        assert len(self.client.get(url).context['reviews']) == 4
+
     def test_list_rss(self):
         r = self.client.get(helpers.url('addons.reviews.list',
                                         self.addon.slug))
@@ -926,6 +951,36 @@ class TestReviewViewSetGet(TestCase):
     def test_list_addon_slug(self):
         self.test_list_addon(addon_pk=self.addon.slug)
 
+    def test_list_with_empty_reviews(self):
+        def create_review(body='review text', user=None):
+            return Review.objects.create(
+                addon=self.addon, user=user or user_factory(),
+                rating=3, body=body)
+
+        self.user = user_factory()
+
+        create_review()
+        create_review()
+        create_review(body=None)
+        create_review(body=None, user=self.user)
+
+        # Don't show the reviews with no body.
+        response = self.client.get(self.url, {'addon': self.addon.pk})
+        data = json.loads(response.content)
+        assert data['count'] == 2 == len(data['results'])
+
+        # Except if it's your review
+        self.client.login_api(self.user)
+        response = self.client.get(self.url, {'addon': self.addon.pk})
+        data = json.loads(response.content)
+        assert data['count'] == 3 == len(data['results'])
+
+        # Or you're an admin
+        self.grant_permission(self.user, 'Addons:Edit')
+        response = self.client.get(self.url, {'addon': self.addon.pk})
+        data = json.loads(response.content)
+        assert data['count'] == 4 == len(data['results'])
+
     def test_list_user(self, **kwargs):
         self.user = user_factory()
         review1 = Review.objects.create(

src/olympia/reviews/views.py

@@ -1,6 +1,6 @@
 from django import http
 from django.core.exceptions import PermissionDenied
-from django.db.models import Prefetch
+from django.db.models import Q, Prefetch
 from django.db.transaction import non_atomic_requests
 from django.shortcuts import get_object_or_404, redirect
 from django.utils.encoding import force_text
@@ -49,6 +49,7 @@ def review_list(request, addon, review_id=None, user_id=None, template=None):
            'grouped_ratings': GroupedRating.get(addon.id)}
 
     ctx['form'] = forms.ReviewForm(None)
+    is_admin = acl.action_allowed(request, amo.permissions.ADDONS_EDIT)
 
     if review_id is not None:
         ctx['page'] = 'detail'
@@ -66,13 +67,18 @@ def review_list(request, addon, review_id=None, user_id=None, template=None):
     else:
         ctx['page'] = 'list'
         qs = qs.filter(is_latest=True)
+        # Don't filter out empty reviews for admins.
+        if not is_admin:
+            # But otherwise, filter out everyone elses empty reviews.
+            user_filter = (Q(user=request.user.pk)
+                           if request.user.is_authenticated() else Q())
+            qs = qs.filter(~Q(body=None) | user_filter)
 
     ctx['reviews'] = reviews = paginate(request, qs)
     ctx['replies'] = Review.get_replies(reviews.object_list)
     if request.user.is_authenticated():
         ctx['review_perms'] = {
-            'is_admin': acl.action_allowed(request,
-                                           amo.permissions.ADDONS_EDIT),
+            'is_admin': is_admin,
             'is_editor': acl.is_editor(request, addon),
             'is_author': acl.check_addon_ownership(request, addon, viewer=True,
                                                    dev=True, support=True),
@@ -359,6 +365,8 @@ class ReviewViewSet(AddonChildMixin, ModelViewSet):
 
     def get_queryset(self):
         requested = self.request.GET.get('filter')
+        has_addons_edit = acl.action_allowed(self.request,
+                                             amo.permissions.ADDONS_EDIT)
 
         # Add this as a property of the view, because we need to pass down the
         # information to the serializer to show/hide delete replies.
@@ -366,7 +374,7 @@ class ReviewViewSet(AddonChildMixin, ModelViewSet):
             self.should_access_deleted_reviews = (
                 (requested == 'with_deleted' or self.action != 'list') and
                 self.request.user.is_authenticated() and
-                acl.action_allowed(self.request, amo.permissions.ADDONS_EDIT))
+                has_addons_edit)
 
         should_access_only_top_level_reviews = (
             self.action == 'list' and self.get_addon_object())
@@ -387,6 +395,12 @@ class ReviewViewSet(AddonChildMixin, ModelViewSet):
             # reviews instead.
             self.queryset = Review.without_replies.all()
 
+        # Filter out (other user's) empty reviews for non-admins.
+        if self.action == 'list' and not has_addons_edit:
+            user_filter = (Q(user=self.request.user.pk)
+                           if self.request.user.is_authenticated() else Q())
+            self.queryset = self.queryset.filter(~Q(body=None) | user_filter)
+
         qs = super(ReviewViewSet, self).get_queryset()
         # The serializer needs reply, version (only the "version" field) and
         # user. We don't need much for version and user, so we can make joins