Add reviewer name alias for reviewers to hide their actual name

It's used when exposing reviewer name to developers for reviewer
actions - regular actions still use the normal name, even when
made by a reviewer.
This commit is contained in:
Mathieu Pillard 2019-03-28 12:31:36 +01:00
Родитель e67eeccaf8
Коммит 90c8201eb9
16 изменённых файлов: 161 добавлений и 61 удалений

Просмотреть файл

@ -39,6 +39,11 @@ Review Notes Detail
This endpoint allows you to fetch a single review note for a specific version of an add-on.
.. note::
To allow reviewers to stay anonymous if they wish, the ``user`` object ``name`` can point to
their "reviewer" name depending on the action. In addition all other fields in that object,
despite being present for backwards-compatibility, are set to ``null``.
.. http:get:: /api/v4/addons/addon/(int:addon_id|string:addon_slug|string:addon_guid)/versions/(int:id)/reviewnotes/(int:id)/
.. _review-notes-version-detail-object:
@ -46,10 +51,10 @@ This endpoint allows you to fetch a single review note for a specific version of
:>json int id: The id for a review note.
:>json string action: The :ref:`type of review note<review-note-action>`.
:>json string action_label: The text label of the action.
:>json int user.id: The id of the reviewer or author who left the review note.
:>json int|null user.id: The id of the reviewer or author who left the review note.
:>json string user.name: The name of the reviewer or author.
:>json string user.url: The link to the profile page for of the reviewer or author.
:>json string user.username: The username of the reviewer or author.
:>json string|null user.url: The link to the profile page for of the reviewer or author.
:>json string|null user.username: The username of the reviewer or author.
:>json string comments: The text content of the review note.
:>json string date: The date the review note was created.

Просмотреть файл

@ -78,19 +78,28 @@ class UserProfileSerializer(PublicUserProfileSerializer):
picture_upload = serializers.ImageField(use_url=True, write_only=True)
permissions = serializers.SerializerMethodField()
fxa_edit_email_url = serializers.SerializerMethodField()
reviewer_name = serializers.CharField(
min_length=2, max_length=50,
validators=[OneOrMorePrintableCharacterAPIValidator()])
class Meta(PublicUserProfileSerializer.Meta):
fields = PublicUserProfileSerializer.Meta.fields + (
'display_name', 'email', 'deleted', 'last_login', 'picture_upload',
'last_login_ip', 'read_dev_agreement', 'permissions',
'fxa_edit_email_url', 'username',
'deleted', 'display_name', 'email', 'fxa_edit_email_url',
'last_login', 'last_login_ip', 'permissions', 'picture_upload',
'read_dev_agreement', 'reviewer_name', 'username'
)
writeable_fields = (
'biography', 'display_name', 'homepage', 'location', 'occupation',
'picture_upload',
'picture_upload', 'reviewer_name',
)
read_only_fields = tuple(set(fields) - set(writeable_fields))
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
if (not self.instance or
not acl.is_user_any_kind_of_reviewer(self.instance)):
self.fields.pop('reviewer_name', None)
def get_fxa_edit_email_url(self, user):
base_url = '{}/settings'.format(
settings.FXA_CONFIG['default']['content_host']
@ -111,6 +120,12 @@ class UserProfileSerializer(PublicUserProfileSerializer):
ugettext(u'This display name cannot be used.'))
return value
def validate_reviewer_name(self, value):
if DeniedName.blocked(value):
raise serializers.ValidationError(
ugettext(u'This reviewer name cannot be used.'))
return value
def validate_homepage(self, value):
if settings.DOMAIN.lower() in value.lower():
raise serializers.ValidationError(

Просмотреть файл

@ -28,7 +28,8 @@ class TestBaseUserSerializer(TestCase):
def serialize(self):
# Manually reload the user first to clear any cached properties.
self.user = UserProfile.objects.get(pk=self.user.pk)
serializer = self.serializer_class(context={'request': self.request})
serializer = self.serializer_class(
self.user, context={'request': self.request})
return serializer.to_representation(self.user)
def test_basic(self):
@ -67,14 +68,19 @@ class TestPublicUserProfileSerializer(TestCase):
user_kwargs = {
'username': 'amo',
'biography': 'stuff', 'homepage': 'http://mozilla.org/',
'location': 'everywhere', 'occupation': 'job'}
'location': 'everywhere', 'occupation': 'job',
}
user_private_kwargs = {
'reviewer_name': 'batman',
}
def setUp(self):
self.request = APIRequestFactory().get('/')
self.user = user_factory(**self.user_kwargs)
self.user = user_factory(
**self.user_kwargs, **self.user_private_kwargs)
def serialize(self):
return (self.serializer(context={'request': self.request})
return (self.serializer(self.user, context={'request': self.request})
.to_representation(self.user))
def test_picture(self):
@ -92,6 +98,8 @@ class TestPublicUserProfileSerializer(TestCase):
data = self.serialize()
for prop, val in self.user_kwargs.items():
assert data[prop] == six.text_type(val), prop
for prop, val in self.user_private_kwargs.items():
assert prop not in data
return data
def test_addons(self):
@ -142,6 +150,11 @@ class TestPublicUserProfileSerializer(TestCase):
assert data['has_anonymous_username'] is False
assert data['has_anonymous_display_name'] is False
def test_is_reviewer(self):
self.grant_permission(self.user, 'Addons:PostReview')
# private data should still be absent, this is a public serializer
self.test_basic()
class PermissionsTestMixin(object):
def test_permissions(self):
@ -194,6 +207,15 @@ class TestUserProfileSerializer(TestPublicUserProfileSerializer,
self.now.replace(microsecond=0).isoformat() + 'Z')
assert data['read_dev_agreement'] == data['last_login']
def test_is_reviewer(self):
self.grant_permission(self.user, 'Addons:PostReview')
data = self.serialize()
for prop, val in self.user_kwargs.items():
assert data[prop] == six.text_type(val), prop
# We can also see private stuff, it's the same user.
for prop, val in self.user_private_kwargs.items():
assert data[prop] == six.text_type(val), prop
def test_expose_fxa_edit_email_url(self):
fxa_host = 'http://example.com'
fxa_config = {

Просмотреть файл

@ -479,6 +479,17 @@ class ActivityLog(ModelBase):
def __html__(self):
return self
@property
def author_name(self):
"""Name of the user that triggered the activity.
If it's a reviewer action that will be shown to developers, the
`reviewer_name` property is used if present, otherwise `name` is
used."""
if self.action in constants.activity.LOG_REVIEW_QUEUE_DEVELOPER:
return self.user.reviewer_name or self.user.name
return self.user.name
@classmethod
def create(cls, action, *args, **kw):
"""

Просмотреть файл

@ -2,7 +2,6 @@ from django.utils.translation import ugettext
from rest_framework import serializers
from olympia.accounts.serializers import BaseUserSerializer
from olympia.activity.models import ActivityLog
@ -11,7 +10,7 @@ class ActivityLogSerializer(serializers.ModelSerializer):
action_label = serializers.SerializerMethodField()
comments = serializers.SerializerMethodField()
date = serializers.DateTimeField(source='created')
user = BaseUserSerializer()
user = serializers.SerializerMethodField()
highlight = serializers.SerializerMethodField()
class Meta:
@ -37,3 +36,16 @@ class ActivityLogSerializer(serializers.ModelSerializer):
def get_highlight(self, obj):
return obj in self.to_highlight
def get_user(self, obj):
"""Return minimal user information using ActivityLog.author_name to
avoid revealing actual name of reviewers for their review actions if
they have set an alias.
id, username and url are present for backwards-compatibility only."""
return {
'id': None,
'username': None,
'url': None,
'name': obj.author_name,
}

Просмотреть файл

@ -4,7 +4,6 @@ from rest_framework.test import APIRequestFactory
from olympia import amo
from olympia.activity.models import ActivityLog
from olympia.activity.serializers import ActivityLogSerializer
from olympia.amo.templatetags.jinja_helpers import absolutify
from olympia.amo.tests import TestCase, addon_factory, user_factory
@ -24,16 +23,16 @@ class LogMixin(object):
class TestReviewNotesSerializerOutput(TestCase, LogMixin):
def setUp(self):
self.request = APIRequestFactory().get('/')
self.user = user_factory()
self.user = user_factory(reviewer_name='fôo')
self.addon = addon_factory()
self.now = self.days_ago(0)
self.entry = self.log(u'Oh nôes!', amo.LOG.REJECT_VERSION, self.now)
self.entry = self.log(u'Oh nøes!', amo.LOG.REJECT_VERSION, self.now)
def serialize(self, context=None):
if context is None:
context = {}
context['request'] = self.request
serializer = ActivityLogSerializer(context=context)
serializer = ActivityLogSerializer(self.entry, context=context)
return serializer.to_representation(self.entry)
def test_basic(self):
@ -43,33 +42,32 @@ class TestReviewNotesSerializerOutput(TestCase, LogMixin):
assert result['date'] == self.now.isoformat() + 'Z'
assert result['action'] == 'rejected'
assert result['action_label'] == 'Rejected'
assert result['comments'] == u'Oh nôes!'
assert result['comments'] == u'Oh nøes!'
# To allow reviewers to stay anonymous the user object only contains
# the "activity name", which uses the reviewer name alias if present.
assert result['user'] == {
'id': self.user.pk,
'name': self.user.name,
'id': None,
'name': 'fôo',
'url': None,
'username': self.user.username,
'username': None,
}
def test_url_for_yourself(self):
# should include account profile url for your own requests
self.request.user = self.user
result = self.serialize()
assert result['user']['url'] == absolutify(self.user.get_url_path())
assert result['user']['url'] is None
def test_url_for_developers(self):
# should include account profile url for a developer
addon_factory(users=[self.user])
result = self.serialize()
assert result['user']['url'] == absolutify(self.user.get_url_path())
assert result['user']['url'] is None
def test_url_for_admins(self):
# should include account profile url for admins
admin = user_factory()
self.grant_permission(admin, 'Users:Edit')
self.request.user = admin
result = self.serialize()
assert result['user']['url'] == absolutify(self.user.get_url_path())
assert result['user']['url'] is None
def test_should_highlight(self):
result = self.serialize(context={'to_highlight': [self.entry]})

Просмотреть файл

@ -4,6 +4,7 @@ import json
import os
from datetime import datetime, timedelta
from email.utils import formataddr
from django.conf import settings
from django.core import mail
@ -21,7 +22,7 @@ from olympia.activity.utils import (
ACTIVITY_MAIL_GROUP, ActivityEmailEncodingError, ActivityEmailParser,
ActivityEmailTokenError, ActivityEmailUUIDError, add_email_to_activity_log,
add_email_to_activity_log_wrapper, log_and_notify,
notify_about_activity_log, send_activity_mail)
notify_about_activity_log, NOTIFICATIONS_FROM_EMAIL, send_activity_mail)
from olympia.addons.models import Addon, AddonReviewerFlags
from olympia.amo.templatetags.jinja_helpers import absolutify
from olympia.amo.tests import TestCase, addon_factory, user_factory
@ -238,7 +239,7 @@ class TestLogAndNotify(TestCase):
def setUp(self):
self.developer = user_factory()
self.developer2 = user_factory()
self.reviewer = user_factory()
self.reviewer = user_factory(reviewer_name='Revîewer')
self.grant_permission(self.reviewer, 'Addons:Review',
'Addon Reviewers')
@ -280,6 +281,8 @@ class TestLogAndNotify(TestCase):
assert days_text in body
assert 'reviewing version %s of the add-on %s' % (
self.version.version, self.addon.name) in body
assert self.reviewer.name not in body
assert self.reviewer.reviewer_name in body
def _check_email(self, call, url, reason_text):
subject = call[0][0]
@ -289,6 +292,7 @@ class TestLogAndNotify(TestCase):
assert ('visit %s' % url) in body
assert ('receiving this email because %s' % reason_text) in body
assert 'If we do not hear from you within' not in body
assert self.reviewer.name not in body
@mock.patch('olympia.activity.utils.send_mail')
def test_reviewer_request_for_information(self, send_mail_mock):
@ -300,8 +304,8 @@ class TestLogAndNotify(TestCase):
amo.LOG.REQUEST_INFORMATION, 'blah', self.reviewer, self.version)
assert send_mail_mock.call_count == 2 # Both authors.
sender = '%s <notifications@%s>' % (
self.reviewer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.reviewer.reviewer_name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2
@ -331,8 +335,8 @@ class TestLogAndNotify(TestCase):
amo.LOG.REQUEST_INFORMATION, 'blah', self.reviewer, self.version)
assert send_mail_mock.call_count == 2 # Both authors.
sender = '%s <notifications@%s>' % (
self.reviewer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.reviewer.reviewer_name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2
@ -362,8 +366,8 @@ class TestLogAndNotify(TestCase):
amo.LOG.REQUEST_INFORMATION, 'blah', self.reviewer, self.version)
assert send_mail_mock.call_count == 2 # Both authors.
sender = '%s <notifications@%s>' % (
self.reviewer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.reviewer.reviewer_name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2
@ -415,8 +419,8 @@ class TestLogAndNotify(TestCase):
assert logs[0].details['comments'] == u'Thïs is á reply'
assert send_mail_mock.call_count == 2 # One author, one reviewer.
sender = '%s <notifications@%s>' % (
self.developer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.developer.name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2
@ -456,8 +460,8 @@ class TestLogAndNotify(TestCase):
assert logs[0].details['comments'] == u'Thîs ïs a revïewer replyîng'
assert send_mail_mock.call_count == 2 # Both authors.
sender = '%s <notifications@%s>' % (
self.reviewer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.reviewer.reviewer_name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2
@ -489,8 +493,8 @@ class TestLogAndNotify(TestCase):
assert not logs[0].details # No details json because no comment.
assert send_mail_mock.call_count == 2 # One author, one reviewer.
sender = '%s <notifications@%s>' % (
self.developer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.developer.name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2
@ -518,8 +522,8 @@ class TestLogAndNotify(TestCase):
assert len(logs) == 1
recipients = self._recipients(send_mail_mock)
sender = '%s <notifications@%s>' % (
self.developer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.developer.name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
assert len(recipients) == 2
# self.reviewers wasn't on the thread, but gets an email anyway.
@ -545,8 +549,8 @@ class TestLogAndNotify(TestCase):
assert len(logs) == 1
recipients = self._recipients(send_mail_mock)
sender = '%s <notifications@%s>' % (
self.developer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr(
(self.developer.name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
developer_subject = send_mail_mock.call_args_list[0][0][0]
assert developer_subject == (
@ -675,7 +679,7 @@ class TestLogAndNotify(TestCase):
@mock.patch('olympia.activity.utils.send_mail')
def test_from_name_escape(self, send_mail_mock):
self.reviewer.update(display_name='mr "quote" escape')
self.reviewer.update(reviewer_name='mr "quote" escape')
# One from the reviewer.
self._create(amo.LOG.REJECT_VERSION, self.reviewer)
@ -707,8 +711,8 @@ class TestLogAndNotify(TestCase):
assert ActivityLog.objects.count() == 1 # No new activity created.
assert send_mail_mock.call_count == 2 # Both authors.
sender = '%s <notifications@%s>' % (
self.reviewer.name, settings.INBOUND_EMAIL_DOMAIN)
sender = formataddr((
self.reviewer.reviewer_name, NOTIFICATIONS_FROM_EMAIL))
assert sender == send_mail_mock.call_args_list[0][1]['from_email']
recipients = self._recipients(send_mail_mock)
assert len(recipients) == 2

Просмотреть файл

@ -242,7 +242,7 @@ def notify_about_activity_log(addon, version, note, perm_setting=None,
author_context_dict = {
'name': addon.name,
'number': version.version,
'author': note.user.name,
'author': note.author_name,
'comments': comments,
'url': absolutify(addon.get_dev_url('versions')),
'SITE_URL': settings.SITE_URL,
@ -279,7 +279,7 @@ def notify_about_activity_log(addon, version, note, perm_setting=None,
addon.name, version.version)
# Build and send the mail for authors.
template = template_from_user(note.user, version)
from_email = formataddr((note.user.name, NOTIFICATIONS_FROM_EMAIL))
from_email = formataddr((note.author_name, NOTIFICATIONS_FROM_EMAIL))
send_activity_mail(
subject, template.render(author_context_dict),
version, addon_authors, from_email, note.id, perm_setting)

Просмотреть файл

@ -30,7 +30,8 @@
{{ item }}
</p>
<p class="timestamp">
{% trans user=item.user|user_link, ago=item.created|timesince,
{% trans user=item.author_name,
ago=item.created|timesince,
iso=item.created|isotime,
pretty=item.created|datetime %}
<time datetime="{{ iso }}" title="{{ pretty }}">{{ ago }}</time>

Просмотреть файл

@ -121,8 +121,8 @@
<p><a href="#" class="review-history-loadmore" data-div="#{{ version.id }}-review-history">{{ _('Load older...') }}</a></p>
</div>
<div class="review-entry-empty hidden">
<p><span class="action">$action_label</span> {{ _('by') }}
<a href="$user_profile">$user_name</a> <time class="timeago" datetime=$date>$date</time></p>
<p><strong class="action">$action_label</strong> {{ _('by') }}
<em class="user_name">$user_name</em> <time class="timeago" datetime=$date>$date</time></p>
<pre>$comments</pre>
</div>
</div>

Просмотреть файл

@ -647,6 +647,8 @@ class TestActivityFeed(TestCase):
assert self.client.login(email='del@icio.us')
self.addon = Addon.objects.get(id=3615)
self.version = self.addon.versions.first()
self.action_user = UserProfile.objects.get(
email='reviewer@mozilla.com')
def test_feed_for_all(self):
response = self.client.get(reverse('devhub.feed_all'))
@ -675,7 +677,7 @@ class TestActivityFeed(TestCase):
assert response.status_code == 302
def add_log(self, action=amo.LOG.ADD_RATING):
core.set_user(UserProfile.objects.get(email='del@icio.us'))
core.set_user(self.action_user)
ActivityLog.create(action, self.addon, self.version)
def add_hidden_log(self, action=amo.LOG.COMMENT_VERSION):
@ -728,6 +730,32 @@ class TestActivityFeed(TestCase):
doc = pq(res.content)
assert len(doc('#recent-activity .item')) == 1
def test_reviewer_name_is_used_for_reviewer_actions(self):
self.action_user.update(display_name='HîdeMe', reviewer_name='ShöwMe')
self.add_log(action=amo.LOG.APPROVE_VERSION)
response = self.client.get(
reverse('devhub.feed', args=[self.addon.slug]))
doc = pq(response.content)
assert len(doc('#recent-activity .item')) == 1
content = force_text(response.content)
assert self.action_user.reviewer_name in content
assert self.action_user.name not in content
def test_regular_name_is_used_for_non_reviewer_actions(self):
# Fields are inverted compared to the test above.
self.action_user.update(reviewer_name='HîdeMe', display_name='ShöwMe')
self.add_log(action=amo.LOG.ADD_RATING) # not a reviewer action.
response = self.client.get(
reverse('devhub.feed', args=[self.addon.slug]))
doc = pq(response.content)
assert len(doc('#recent-activity .item')) == 1
content = force_text(response.content)
# Assertions are inverted compared to the test above.
assert self.action_user.reviewer_name not in content
assert self.action_user.name in content
class TestAPIAgreement(TestCase):
fixtures = ['base/addon_3615', 'base/addon_5579', 'base/users']

Просмотреть файл

@ -0,0 +1 @@
ALTER TABLE `users` ADD COLUMN `reviewer_name` varchar(50);

Просмотреть файл

@ -570,7 +570,7 @@ class ReviewBase(object):
dev_ver_url = self.addon.get_dev_url('versions')
return {'name': addon.name,
'number': self.version.version if self.version else '',
'reviewer': self.user.name,
'reviewer': self.user.reviewer_name or self.user.name,
'addon_url': absolutify(addon_url),
'dev_versions_url': absolutify(dev_ver_url),
'review_url': absolutify(reverse('reviewers.review',

Просмотреть файл

@ -48,8 +48,8 @@ class UserAdmin(admin.ModelAdmin):
fieldsets = (
(None, {
'fields': ('id', 'email', 'fxa_id', 'username', 'display_name',
'biography', 'homepage', 'location', 'occupation',
'picture_img'),
'reviewer_name', 'biography', 'homepage', 'location',
'occupation', 'picture_img'),
}),
('Flags', {
'fields': ('display_collections', 'deleted', 'is_public'),

Просмотреть файл

@ -166,6 +166,10 @@ class UserProfile(OnChangeMixin, ModelBase, AbstractBaseUser):
# newsletter
basket_token = models.CharField(blank=True, default='', max_length=128)
reviewer_name = models.CharField(
max_length=50, default='', null=True, blank=True,
validators=[validators.MinLengthValidator(2)])
class Meta:
db_table = 'users'

Просмотреть файл

@ -563,11 +563,10 @@ function initVersions() {
if (note["highlight"] == true) {
clone.addClass("new");
}
clone.find('span.action')[0].textContent = note["action_label"];
var user = clone.find('a:contains("$user_name")');
clone.find('.action')[0].textContent = note["action_label"];
var user = clone.find('.user_name');
user[0].textContent = note["user"]["name"];
user.attr('href', note["user"]["url"]);
var date = clone.find('time.timeago');
var date = clone.find('.timeago');
date[0].textContent = note["date"];
date.attr('datetime', note["date"]);
date.attr('title', note["date"]);