From 2075e31afcc66880dd4ef83c82cff0b48413f266 Mon Sep 17 00:00:00 2001 From: Rob Hudson Date: Wed, 25 Jul 2012 22:40:54 -0700 Subject: [PATCH] Locked down escalated review queue to Sr. Reviewers (bug 777149) --- apps/access/admin.py | 1 + migrations/440-sr-reviewer-rules.sql | 1 + mkt/reviewers/helpers.py | 17 +++++--- mkt/reviewers/tests/test_views.py | 61 ++++++++++++++++++++-------- mkt/reviewers/views.py | 4 +- 5 files changed, 59 insertions(+), 25 deletions(-) create mode 100644 migrations/440-sr-reviewer-rules.sql diff --git a/apps/access/admin.py b/apps/access/admin.py index 88bc46fa9d..c6afbe9911 100644 --- a/apps/access/admin.py +++ b/apps/access/admin.py @@ -12,6 +12,7 @@ class GroupAdmin(admin.ModelAdmin): raw_id_fields = ('users',) ordering = ('name',) list_display = ('name', 'rules', 'notes') + readonly_fields = ('rules',) inlines = (GroupUserInline,) diff --git a/migrations/440-sr-reviewer-rules.sql b/migrations/440-sr-reviewer-rules.sql new file mode 100644 index 0000000000..abc669a34a --- /dev/null +++ b/migrations/440-sr-reviewer-rules.sql @@ -0,0 +1 @@ +UPDATE `groups` SET rules=CONCAT(rules, ',Apps:ReviewEscalated') WHERE name='Senior App Reviewers'; diff --git a/mkt/reviewers/helpers.py b/mkt/reviewers/helpers.py index 722b5d252e..ba12b348fa 100644 --- a/mkt/reviewers/helpers.py +++ b/mkt/reviewers/helpers.py @@ -2,6 +2,7 @@ import jinja2 from jingo import register from tower import ugettext as _, ugettext_lazy as _lazy +from access import acl from amo.helpers import impala_breadcrumbs from amo.urlresolvers import reverse @@ -59,15 +60,21 @@ def queue_tabnav(context): Each tuple contains three elements: (tab_code, page_url, tab_text) """ counts = context['queue_counts'] - return [ + rv = [ ('pending', 'queue_pending', _('Apps ({0})', counts['pending']).format(counts['pending'])), ('rereview', 'queue_rereview', _('Re-reviews ({0})', counts['rereview']).format(counts['rereview'])), - ('escalated', 'queue_escalated', - _('Escalations ({0})', - counts['escalated']).format(counts['escalated'])), + ] + if acl.action_allowed(context['request'], 'Apps', 'ReviewEscalated'): + rv.append( + ('escalated', 'queue_escalated', + _('Escalations ({0})', + counts['escalated']).format(counts['escalated'])) + ) + rv.append( ('moderated', 'queue_moderated', _('Moderated Reviews ({0})', counts['moderated']).format(counts['moderated'])), - ] + ) + return rv diff --git a/mkt/reviewers/tests/test_views.py b/mkt/reviewers/tests/test_views.py index bf738803e1..179a46cce5 100644 --- a/mkt/reviewers/tests/test_views.py +++ b/mkt/reviewers/tests/test_views.py @@ -15,6 +15,7 @@ from pyquery import PyQuery as pq import amo import reviews from abuse.models import AbuseReport +from access.models import GroupUser from addons.models import AddonDeviceType, AddonUser, DeviceType from amo.tests import app_factory, check_links, formset, initial from amo.urlresolvers import reverse @@ -45,7 +46,7 @@ class AppReviewerTest(amo.tests.TestCase): def login_as_senior_reviewer(self): self.client.logout() user = UserProfile.objects.get(email='editor@mozilla.com') - self.grant_permission(user, 'Addons:Edit') + self.grant_permission(user, 'Addons:Edit,Apps:ReviewEscalated') self.login_as_editor() def check_actions(self, expected, elements): @@ -243,6 +244,15 @@ class TestAppQueue(AppReviewerTest, AccessMixin): eq_(r.context['pager'].number, 1) def test_queue_count(self): + r = self.client.get(self.url) + eq_(r.status_code, 200) + doc = pq(r.content) + eq_(doc('.tabnav li a:eq(0)').text(), u'Apps (2)') + eq_(doc('.tabnav li a:eq(1)').text(), u'Re-reviews (1)') + eq_(doc('.tabnav li a:eq(2)').text(), u'Moderated Reviews (0)') + + def test_queue_count_senior_reviewer(self): + self.login_as_senior_reviewer() r = self.client.get(self.url) eq_(r.status_code, 200) doc = pq(r.content) @@ -252,6 +262,7 @@ class TestAppQueue(AppReviewerTest, AccessMixin): eq_(doc('.tabnav li a:eq(3)').text(), u'Moderated Reviews (0)') def test_escalated_not_in_queue(self): + self.login_as_senior_reviewer() EscalationQueue.objects.create(addon=self.apps[0]) res = self.client.get(self.url) # self.apps[2] is not pending so doesn't show up either. @@ -357,6 +368,15 @@ class TestRereviewQueue(AppReviewerTest, AccessMixin): eq_(r.context['pager'].number, 1) def test_queue_count(self): + r = self.client.get(self.url) + eq_(r.status_code, 200) + doc = pq(r.content) + eq_(doc('.tabnav li a:eq(0)').text(), u'Apps (0)') + eq_(doc('.tabnav li a:eq(1)').text(), u'Re-reviews (3)') + eq_(doc('.tabnav li a:eq(2)').text(), u'Moderated Reviews (0)') + + def test_queue_count_senior_reviewer(self): + self.login_as_senior_reviewer() r = self.client.get(self.url) eq_(r.status_code, 200) doc = pq(r.content) @@ -366,6 +386,7 @@ class TestRereviewQueue(AppReviewerTest, AccessMixin): eq_(doc('.tabnav li a:eq(3)').text(), u'Moderated Reviews (0)') def test_escalated_not_in_queue(self): + self.login_as_senior_reviewer() EscalationQueue.objects.create(addon=self.apps[0]) res = self.client.get(self.url) eq_(res.context['addons'], self.apps[1:]) @@ -395,12 +416,20 @@ class TestEscalationQueue(AppReviewerTest, AccessMixin): eq3 = EscalationQueue.objects.create(addon=self.apps[2]) eq3.update(created=days_ago(1)) - self.login_as_editor() + self.login_as_senior_reviewer() self.url = reverse('reviewers.apps.queue_escalated') def review_url(self, app): return urlparams(reverse('reviewers.apps.review', args=[app.app_slug])) + def test_no_access_regular_reviewer(self): + # Since setUp added a new group, remove all groups and start over. + user = UserProfile.objects.get(email='editor@mozilla.com') + GroupUser.objects.filter(user=user).delete() + self.grant_permission(user, 'Apps:Review') + res = self.client.get(self.url) + eq_(res.status_code, 403) + def test_template_links(self): r = self.client.get(self.url) eq_(r.status_code, 200) @@ -414,27 +443,13 @@ class TestEscalationQueue(AppReviewerTest, AccessMixin): ] check_links(expected, links, verify=False) - def test_action_buttons_public_senior_reviewer(self): - self.login_as_senior_reviewer() - - r = self.client.get(self.review_url(self.apps[0])) - eq_(r.status_code, 200) - actions = pq(r.content)('#review-actions input') - expected = [ - (u'Reject', 'reject'), - (u'Disable app', 'disable'), - (u'Clear Escalation', 'clear_escalation'), - (u'Request more information', 'info'), - (u'Comment', 'comment'), - ] - self.check_actions(expected, actions) - def test_action_buttons_public(self): r = self.client.get(self.review_url(self.apps[0])) eq_(r.status_code, 200) actions = pq(r.content)('#review-actions input') expected = [ (u'Reject', 'reject'), + (u'Disable app', 'disable'), (u'Clear Escalation', 'clear_escalation'), (u'Request more information', 'info'), (u'Comment', 'comment'), @@ -448,6 +463,7 @@ class TestEscalationQueue(AppReviewerTest, AccessMixin): actions = pq(r.content)('#review-actions input') expected = [ (u'Push to public', 'public'), + (u'Disable app', 'disable'), (u'Clear Escalation', 'clear_escalation'), (u'Request more information', 'info'), (u'Comment', 'comment'), @@ -1141,7 +1157,7 @@ class TestAbuseReports(amo.tests.TestCase): eq_(sorted([r.message for r in reports]), [u'eff', u'yeah']) -class TestModeratedQueue(amo.tests.TestCase, AccessMixin): +class TestModeratedQueue(AppReviewerTest, AccessMixin): fixtures = ['base/users'] def setUp(self): @@ -1213,6 +1229,15 @@ class TestModeratedQueue(amo.tests.TestCase, AccessMixin): eq_(pq(res.content)('#reviews-flagged .no-results').length, 1) def test_queue_count(self): + r = self.client.get(self.url) + eq_(r.status_code, 200) + doc = pq(r.content) + eq_(doc('.tabnav li a:eq(0)').text(), u'Apps (0)') + eq_(doc('.tabnav li a:eq(1)').text(), u'Re-reviews (0)') + eq_(doc('.tabnav li a:eq(2)').text(), u'Moderated Reviews (2)') + + def test_queue_count_senior_reviewer(self): + self.login_as_senior_reviewer() r = self.client.get(self.url) eq_(r.status_code, 200) doc = pq(r.content) diff --git a/mkt/reviewers/views.py b/mkt/reviewers/views.py index caf3a58fc5..9ec9c24bbb 100644 --- a/mkt/reviewers/views.py +++ b/mkt/reviewers/views.py @@ -54,7 +54,7 @@ def home(request): return jingo.render(request, 'reviewers/home.html', data) -def queue_counts(type=None, **kw): +def queue_counts(): excluded_ids = EscalationQueue.uncached.values_list('addon', flat=True) counts = { @@ -229,7 +229,7 @@ def queue_rereview(request): lambda p: [r.addon for r in p.object_list]) -@permission_required('Apps', 'Review') +@permission_required('Apps', 'ReviewEscalated') def queue_escalated(request): qs = (EscalationQueue.uncached.filter(addon__disabled_by_user=False) .order_by('created'))