From a138cced28a43f4defa3fddd8b7b80b0553d7dcc Mon Sep 17 00:00:00 2001 From: Andrew Williamson Date: Fri, 2 Sep 2016 13:57:22 +0100 Subject: [PATCH] Add new message indicator to review history for messages since last reply. --- src/olympia/activity/serializers.py | 11 +++++- .../activity/tests/test_serializers.py | 38 +++++++++++++------ src/olympia/activity/tests/test_views.py | 19 ++++++++-- src/olympia/activity/views.py | 13 +++++++ src/olympia/amo/log.py | 10 ++++- src/olympia/devhub/helpers.py | 13 +++++++ .../templates/devhub/versions/list.html | 4 ++ src/olympia/devhub/tests/test_helpers.py | 22 ++++++++++- .../devhub/tests/test_views_versions.py | 9 +++++ src/olympia/editors/tests/test_views.py | 2 +- static/css/zamboni/developers.css | 19 +++++++++- static/js/zamboni/devhub.js | 5 ++- 12 files changed, 144 insertions(+), 21 deletions(-) diff --git a/src/olympia/activity/serializers.py b/src/olympia/activity/serializers.py index 213f9c0396..4a37fe2a41 100644 --- a/src/olympia/activity/serializers.py +++ b/src/olympia/activity/serializers.py @@ -12,10 +12,16 @@ class ActivityLogSerializer(serializers.ModelSerializer): comments = serializers.SerializerMethodField() date = serializers.DateTimeField(source='created') user = BaseUserSerializer() + highlight = serializers.SerializerMethodField() class Meta: model = ActivityLog - fields = ('id', 'action', 'action_label', 'comments', 'user', 'date') + fields = ('id', 'action', 'action_label', 'comments', 'user', 'date', + 'highlight') + + def __init__(self, *args, **kwargs): + super(ActivityLogSerializer, self).__init__(*args, **kwargs) + self.to_highlight = kwargs.get('context', []).get('to_highlight', []) def get_comments(self, obj): return obj.details['comments'] @@ -26,3 +32,6 @@ class ActivityLogSerializer(serializers.ModelSerializer): def get_action(self, obj): return self.get_action_label(obj).replace(' ', '-').lower() + + def get_highlight(self, obj): + return obj in self.to_highlight diff --git a/src/olympia/activity/tests/test_serializers.py b/src/olympia/activity/tests/test_serializers.py index 2b2115e1c4..b4256f8c83 100644 --- a/src/olympia/activity/tests/test_serializers.py +++ b/src/olympia/activity/tests/test_serializers.py @@ -9,7 +9,9 @@ from olympia.amo.tests import ( class LogMixin(object): - def log(self, comments, action, created): + def log(self, comments, action, created=None): + if not created: + created = self.days_ago(0) details = {'comments': comments} details['version'] = self.addon.current_version.version kwargs = {'user': self.user, 'created': created, 'details': details} @@ -22,22 +24,36 @@ class TestReviewNotesSerializerOutput(TestCase, LogMixin): self.request = APIRequestFactory().get('/') self.user = user_factory() self.addon = addon_factory() + self.now = self.days_ago(0) + self.entry = self.log(u'Oh nôes!', amo.LOG.REJECT_VERSION, self.now) - def serialize(self, id_): - serializer = ActivityLogSerializer(context={'request': self.request}) - return serializer.to_representation(id_) + def serialize(self, context={}): + context['request'] = self.request + serializer = ActivityLogSerializer(context=context) + return serializer.to_representation(self.entry) def test_basic(self): - now = self.days_ago(0) - entry = self.log(u'Oh nôes!', amo.LOG.REJECT_VERSION, now) + result = self.serialize() - result = self.serialize(entry) - - assert result['id'] == entry.pk - assert result['date'] == now.isoformat() + assert result['id'] == self.entry.pk + assert result['date'] == self.now.isoformat() assert result['action'] == 'rejected' assert result['action_label'] == 'Rejected' - assert result['comments'] == u'Oh nôes!' + assert result['comments'] == u'Oh nôes!' assert result['user'] == { 'name': self.user.name, 'url': absolutify(self.user.get_url_path())} + + def test_should_highlight(self): + result = self.serialize(context={'to_highlight': [self.entry]}) + + assert result['id'] == self.entry.pk + assert result['highlight'] + + def test_should_not_highlight(self): + no_highlight = self.log(u'something élse', amo.LOG.REJECT_VERSION) + + result = self.serialize(context={'to_highlight': [no_highlight]}) + + assert result['id'] == self.entry.pk + assert not result['highlight'] diff --git a/src/olympia/activity/tests/test_views.py b/src/olympia/activity/tests/test_views.py index 8ac92a9436..b3213a25ec 100644 --- a/src/olympia/activity/tests/test_views.py +++ b/src/olympia/activity/tests/test_views.py @@ -142,6 +142,7 @@ class TestReviewNotesViewSetDetail(ReviewNotesViewSetDetailMixin, TestCase): assert result['id'] == self.note.pk assert result['action_label'] == amo.LOG.APPROVE_VERSION.short assert result['comments'] == u'noôo!' + assert result['highlight'] # Its the first reply so highlight def _set_tested_url(self, pk=None, version_pk=None, addon_pk=None): self.url = reverse('version-reviewnotes-detail', kwargs={ @@ -165,8 +166,10 @@ class TestReviewNotesViewSetList(ReviewNotesViewSetDetailMixin, TestCase): guid=generate_addon_guid(), name=u'My Addôn', slug='my-addon') self.user = user_factory() self.note = self.log(u'noôo!', amo.LOG.APPROVE_VERSION, - self.days_ago(1)) - self.note2 = self.log(u'yéss!', amo.LOG.REJECT_VERSION, + self.days_ago(2)) + self.note2 = self.log(u'réply!', amo.LOG.DEVELOPER_REPLY_VERSION, + self.days_ago(1)) + self.note3 = self.log(u'yéss!', amo.LOG.REJECT_VERSION, self.days_ago(0)) self.version = self.addon.latest_version @@ -177,11 +180,19 @@ class TestReviewNotesViewSetList(ReviewNotesViewSetDetailMixin, TestCase): assert response.status_code == 200 result = json.loads(response.content) assert result['results'] - assert len(result['results']) == 2 + assert len(result['results']) == 3 + result_version = result['results'][0] - assert result_version['id'] == self.note2.pk + assert result_version['id'] == self.note3.pk + assert result_version['highlight'] # This note is after the dev reply. + result_version = result['results'][1] + assert result_version['id'] == self.note2.pk + assert not result_version['highlight'] # This note is the dev reply. + + result_version = result['results'][2] assert result_version['id'] == self.note.pk + assert not result_version['highlight'] # The dev replied so read it. def _set_tested_url(self, pk=None, version_pk=None, addon_pk=None): self.url = reverse('version-reviewnotes-list', kwargs={ diff --git a/src/olympia/activity/views.py b/src/olympia/activity/views.py index 29a49e2924..24f647f3e2 100644 --- a/src/olympia/activity/views.py +++ b/src/olympia/activity/views.py @@ -37,3 +37,16 @@ class VersionReviewNotesViewSet(AddonChildMixin, RetrieveModelMixin, # Just loading the add-on object triggers permission checks, because # the implementation in AddonChildMixin calls AddonViewSet.get_object() self.get_addon_object() + + def get_serializer_context(self): + ctx = super(VersionReviewNotesViewSet, self).get_serializer_context() + ctx['to_highlight'] = self.pending_queryset( + amo.LOG.DEVELOPER_REPLY_VERSION) + return ctx + + def pending_queryset(self, log_type): + version_qs = self.get_queryset() + latest_reply = version_qs.filter(action=log_type.id).first() + if not latest_reply: + return version_qs + return version_qs.filter(created__gt=latest_reply.created) diff --git a/src/olympia/amo/log.py b/src/olympia/amo/log.py index 9e7ce1d87a..95637934c3 100644 --- a/src/olympia/amo/log.py +++ b/src/olympia/amo/log.py @@ -650,6 +650,14 @@ class PRELIMINARY_ADDON_MIGRATED(_LOG): review_queue = True +class DEVELOPER_REPLY_VERSION(_LOG): + id = 140 + format = _(u'Reply by developer on {addon} {version}.') + short = _(u'Developer Reply') + keep = True + review_queue = True + + LOGS = [x for x in vars().values() if isclass(x) and issubclass(x, _LOG) and x != _LOG] # Make sure there's no duplicate IDs. @@ -671,7 +679,7 @@ LOG_HIDE_DEVELOPER = [l.id for l in LOGS if (getattr(l, 'hide_developer', False) or l.id in LOG_ADMINS)] # Review Queue logs to show to developer (i.e. hiding admin/private) -LOG_REVIEW_QUEUE_DEVELOPER = list(set(LOG_EDITOR_REVIEW_ACTION) - +LOG_REVIEW_QUEUE_DEVELOPER = list(set(LOG_REVIEW_QUEUE) - set(LOG_HIDE_DEVELOPER)) diff --git a/src/olympia/devhub/helpers.py b/src/olympia/devhub/helpers.py index 06bd8a7a79..6e14c9f560 100644 --- a/src/olympia/devhub/helpers.py +++ b/src/olympia/devhub/helpers.py @@ -13,6 +13,7 @@ from olympia.amo.helpers import breadcrumbs, impala_breadcrumbs, page_title from olympia.access import acl from olympia.addons.helpers import new_context from olympia.addons.models import Addon +from olympia.devhub.models import ActivityLog from olympia.compat.models import CompatReport from olympia.files.models import File @@ -225,3 +226,15 @@ def version_disabled(version): disabled = [status == amo.STATUS_DISABLED for _id, status in version.statuses] return all(disabled) + + +@register.function +def pending_activity_log_count_for_developer(version): + alog = ActivityLog.objects.for_version(version).filter( + action__in=amo.LOG_REVIEW_QUEUE_DEVELOPER) + + latest_reply = alog.filter( + action=amo.LOG.DEVELOPER_REPLY_VERSION.id).first() + if not latest_reply: + return alog.count() + return alog.filter(created__gt=latest_reply.created).count() diff --git a/src/olympia/devhub/templates/devhub/versions/list.html b/src/olympia/devhub/templates/devhub/versions/list.html index 3aef5f558e..68ddb44548 100644 --- a/src/olympia/devhub/templates/devhub/versions/list.html +++ b/src/olympia/devhub/templates/devhub/versions/list.html @@ -4,6 +4,7 @@ {% block title %}{{ dev_page_title(title, addon) }}{% endblock %} {% macro version_details(version, full_info=False) %} + {% set is_last_version = (version == version.addon.latest_or_rejected_version) %} @@ -48,6 +49,9 @@
{{ _('Review History') }} + {% if is_last_version %} + {{ pending_activity_log_count_for_developer(version) }} + {% endif %}
diff --git a/src/olympia/devhub/tests/test_helpers.py b/src/olympia/devhub/tests/test_helpers.py index ea34b24097..9f15aa3deb 100644 --- a/src/olympia/devhub/tests/test_helpers.py +++ b/src/olympia/devhub/tests/test_helpers.py @@ -8,7 +8,8 @@ from mock import Mock from pyquery import PyQuery as pq from olympia import amo -from olympia.amo.tests import TestCase +from olympia.amo import LOG +from olympia.amo.tests import addon_factory, days_ago, TestCase, user_factory from olympia.amo.urlresolvers import reverse from olympia.amo.tests.test_helpers import render from olympia.addons.models import Addon @@ -212,3 +213,22 @@ class TestDevFilesStatus(TestCase): self.addon.status = amo.STATUS_PUBLIC self.file.status = amo.STATUS_DISABLED self.expect(File.STATUS_CHOICES[amo.STATUS_DISABLED]) + + +@pytest.mark.parametrize( + 'action1,action2,action3,count', + ((LOG.REQUEST_INFORMATION, LOG.REJECT_VERSION, LOG.APPROVE_VERSION, 3), + (LOG.DEVELOPER_REPLY_VERSION, LOG.REJECT_VERSION, LOG.REJECT_VERSION, 2), + (LOG.APPROVE_VERSION, LOG.DEVELOPER_REPLY_VERSION, LOG.REJECT_VERSION, 1), + (LOG.APPROVE_VERSION, LOG.REJECT_VERSION, LOG.DEVELOPER_REPLY_VERSION, 0)) +) +def test_pending_activity_log_count_for_developer( + action1, action2, action3, count): + user = user_factory() + addon = addon_factory() + version = addon.latest_version + amo.log(action1, addon, version, user=user, created=days_ago(2)) + amo.log(action2, addon, version, user=user, created=days_ago(1)) + amo.log(action3, addon, version, user=user, created=days_ago(0)) + + assert helpers.pending_activity_log_count_for_developer(version) == count diff --git a/src/olympia/devhub/tests/test_views_versions.py b/src/olympia/devhub/tests/test_views_versions.py index dea6623f14..a724184358 100644 --- a/src/olympia/devhub/tests/test_views_versions.py +++ b/src/olympia/devhub/tests/test_views_versions.py @@ -469,6 +469,10 @@ class TestVersion(TestCase): self.client.cookies['jwt_api_auth_token'] = 'magicbeans' v1 = self.version v2, _ = self._extra_version_and_file(amo.STATUS_UNREVIEWED) + # Add some activity log messages + amo.log(amo.LOG.REJECT_VERSION, v2.addon, v2, user=self.user) + amo.log(amo.LOG.REJECT_VERSION, v2.addon, v2, user=self.user) + r = self.client.get(self.url) assert r.status_code == 200 doc = pq(r.content) @@ -482,6 +486,11 @@ class TestVersion(TestCase): 'version-reviewnotes-list', args=[self.addon.id, self.version.id]) assert doc('.review-history-hide').length == 2 + pending_activity_count = doc('.review-history-pending-count') + # Only one, for the latest/deleted version + assert pending_activity_count.length == 1 + assert pending_activity_count.text() == '2' + class TestVersionEditMixin(object): diff --git a/src/olympia/editors/tests/test_views.py b/src/olympia/editors/tests/test_views.py index a31c7f81a5..1bf9e49f9a 100644 --- a/src/olympia/editors/tests/test_views.py +++ b/src/olympia/editors/tests/test_views.py @@ -1428,7 +1428,7 @@ class TestPerformance(QueueTest): doc = pq(r.content) data = json.loads(doc('#monthly').attr('data-chart')) label = datetime.now().strftime('%Y-%m') - assert data[label]['usercount'] == 19 + assert data[label]['usercount'] == len(amo.LOG_REVIEW_QUEUE) - 1 def _test_performance_other_user_as_admin(self): userid = amo.get_user().pk diff --git a/static/css/zamboni/developers.css b/static/css/zamboni/developers.css index c2a7c9bb57..14593bddbc 100644 --- a/static/css/zamboni/developers.css +++ b/static/css/zamboni/developers.css @@ -637,8 +637,24 @@ a.extra { margin: 0; } +b.review-history-pending-count { + background-color: #000; + border-radius: 5px; + padding: 0 0.3em; + color: #FFF; +} + div.history-container { - margin: 0em 2em; + margin: 0em 1em; +} + +div.review-entry { + padding: 0.5em 1em; + margin-bottom: 0.25em; +} + +div.review-entry.new { + background-color: #FFFFD5; } div.review-entry p { @@ -649,6 +665,7 @@ div.review-entry p { div.review-entry pre { white-space: pre-wrap; word-break: break-all; + margin-bottom: 0px; } .status-lite-nom i, diff --git a/static/js/zamboni/devhub.js b/static/js/zamboni/devhub.js index 17e68be69d..15758e923a 100644 --- a/static/js/zamboni/devhub.js +++ b/static/js/zamboni/devhub.js @@ -699,7 +699,7 @@ function initVersions() { container.children('.review-entry-loading').removeClass("hidden"); container.children('.review-entry-failure').addClass("hidden"); if (!nextLoad) { - container.children('.review-entry').empty(); + container.children('.review-entry').remove(); var api_url = div.data('api-url'); } else { var api_url = div.data('next-url'); @@ -709,6 +709,9 @@ function initVersions() { json["results"].forEach(function(note) { var clone = empty_note.clone(true, true); clone.attr('class', 'review-entry'); + if (note["highlight"] == true) { + clone.addClass("new"); + } clone.find('span.action')[0].textContent = note["action_label"]; var user = clone.find('a:contains("$user_name")'); user[0].textContent = note["user"]["name"];