From 66af052c1a35781756dffa3ec8f3e79f85635291 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Mon, 18 Feb 2019 18:34:09 +0100 Subject: [PATCH] Refactor abuse API internals to make adding new fields easier --- src/olympia/abuse/models.py | 9 ++- src/olympia/abuse/serializers.py | 71 ++++++++++++++++++++--- src/olympia/abuse/tests/test_views.py | 12 ++-- src/olympia/abuse/views.py | 48 +-------------- src/olympia/reviewers/tests/test_views.py | 2 +- 5 files changed, 79 insertions(+), 63 deletions(-) diff --git a/src/olympia/abuse/models.py b/src/olympia/abuse/models.py index ca1ef577d2..ca8ebbe3a5 100644 --- a/src/olympia/abuse/models.py +++ b/src/olympia/abuse/models.py @@ -40,7 +40,7 @@ class AbuseReport(ModelBase): if self.reporter: user_name = '%s (%s)' % (self.reporter.name, self.reporter.email) else: - user_name = 'An anonymous coward' + user_name = 'An anonymous user' target_url = ('%s%s' % (settings.SITE_URL, self.target.get_url_path()) if self.target else 'GUID not in database') @@ -50,6 +50,12 @@ class AbuseReport(ModelBase): send_mail( six.text_type(self), msg, recipient_list=(settings.ABUSE_EMAIL,)) + def save(self, *args, **kwargs): + creation = not self.pk + super(AbuseReport, self).save(*args, **kwargs) + if creation: + self.send() + @property def target(self): return self.addon or self.user @@ -77,4 +83,3 @@ def send_abuse_report(request, obj, message): elif isinstance(obj, UserProfile): report.user = obj report.save() - report.send() diff --git a/src/olympia/abuse/serializers.py b/src/olympia/abuse/serializers.py index 87370e9ab7..8f9b1dbcc6 100644 --- a/src/olympia/abuse/serializers.py +++ b/src/olympia/abuse/serializers.py @@ -4,13 +4,55 @@ from olympia.abuse.models import AbuseReport from olympia.accounts.serializers import BaseUserSerializer -class AddonAbuseReportSerializer(serializers.ModelSerializer): - addon = serializers.SerializerMethodField() +class BaseAbuseReportSerializer(serializers.ModelSerializer): reporter = BaseUserSerializer(read_only=True) class Meta: model = AbuseReport - fields = ('reporter', 'addon', 'message') + fields = ('message', 'reporter') + + def validate_target(self, data, target_name): + if target_name not in data: + msg = serializers.Field.default_error_messages['required'] + raise serializers.ValidationError({ + target_name: [msg] + }) + + def to_internal_value(self, data): + output = super(BaseAbuseReportSerializer, self).to_internal_value(data) + request = self.context['request'] + output['ip_address'] = request.META.get('REMOTE_ADDR') + if request.user.is_authenticated: + output['reporter'] = request.user + return output + + +class AddonAbuseReportSerializer(BaseAbuseReportSerializer): + addon = serializers.SerializerMethodField() + + class Meta: + model = AbuseReport + fields = BaseAbuseReportSerializer.Meta.fields + ( + 'addon', + ) + + def to_internal_value(self, data): + self.validate_target(data, 'addon') + view = self.context.get('view') + output = { + # get_guid() needs to be called first because get_addon_object() + # would otherwise 404 on add-ons that don't match an existing + # add-on in our database. + 'guid': view.get_guid(), + 'addon': view.get_addon_object(), + } + # Pop 'addon' before passing it to super(), we already have the + # output value. + data.pop('addon') + output.update( + super(AddonAbuseReportSerializer, self).to_internal_value(data) + ) + return output def get_addon(self, obj): addon = obj.addon @@ -23,10 +65,25 @@ class AddonAbuseReportSerializer(serializers.ModelSerializer): } -class UserAbuseReportSerializer(serializers.ModelSerializer): - reporter = BaseUserSerializer(read_only=True) - user = BaseUserSerializer() +class UserAbuseReportSerializer(BaseAbuseReportSerializer): + user = BaseUserSerializer(required=False) # We validate it ourselves. class Meta: model = AbuseReport - fields = ('reporter', 'user', 'message') + fields = BaseAbuseReportSerializer.Meta.fields + ( + 'user', + ) + + def to_internal_value(self, data): + view = self.context.get('view') + self.validate_target(data, 'user') + output = { + 'user': view.get_user_object() + } + # Pop 'user' before passing it to super(), we already have the + # output value and did the validation above. + data.pop('user') + output.update( + super(UserAbuseReportSerializer, self).to_internal_value(data) + ) + return output diff --git a/src/olympia/abuse/tests/test_views.py b/src/olympia/abuse/tests/test_views.py index 51c3a6b092..3392ae8017 100644 --- a/src/olympia/abuse/tests/test_views.py +++ b/src/olympia/abuse/tests/test_views.py @@ -102,7 +102,7 @@ class AddonAbuseViewSetTestBase(object): data={'message': 'abuse!'}) assert response.status_code == 400 assert json.loads(response.content) == { - 'detail': 'Need an addon parameter'} + 'addon': ['This field is required.']} def test_message_required_empty(self): addon = addon_factory() @@ -112,7 +112,7 @@ class AddonAbuseViewSetTestBase(object): 'message': ''}) assert response.status_code == 400 assert json.loads(response.content) == { - 'detail': 'Abuse reports need a message'} + 'message': ['This field may not be blank.']} def test_message_required_missing(self): addon = addon_factory() @@ -121,7 +121,7 @@ class AddonAbuseViewSetTestBase(object): data={'addon': six.text_type(addon.id)}) assert response.status_code == 400 assert json.loads(response.content) == { - 'detail': 'Abuse reports need a message'} + 'message': ['This field is required.']} def test_throttle(self): addon = addon_factory() @@ -201,7 +201,7 @@ class UserAbuseViewSetTestBase(object): data={'message': 'abuse!'}) assert response.status_code == 400 assert json.loads(response.content) == { - 'detail': 'Need a user parameter'} + 'user': ['This field is required.']} def test_message_required_empty(self): user = user_factory() @@ -210,7 +210,7 @@ class UserAbuseViewSetTestBase(object): data={'user': six.text_type(user.username), 'message': ''}) assert response.status_code == 400 assert json.loads(response.content) == { - 'detail': 'Abuse reports need a message'} + 'message': ['This field may not be blank.']} def test_message_required_missing(self): user = user_factory() @@ -219,7 +219,7 @@ class UserAbuseViewSetTestBase(object): data={'user': six.text_type(user.username)}) assert response.status_code == 400 assert json.loads(response.content) == { - 'detail': 'Abuse reports need a message'} + 'message': ['This field is required.']} def test_throttle(self): user = user_factory() diff --git a/src/olympia/abuse/views.py b/src/olympia/abuse/views.py index 198b783461..3347bb3c50 100644 --- a/src/olympia/abuse/views.py +++ b/src/olympia/abuse/views.py @@ -59,37 +59,13 @@ class AddonAbuseViewSet(CreateModelMixin, GenericViewSet): return guid return None - def create(self, request, *args, **kwargs): - addon_id = self.request.data.get('addon') - if not addon_id: - raise ParseError('Need an addon parameter') - - message = self.request.data.get('message') - if not message: - raise ParseError('Abuse reports need a message') - - abuse_kwargs = { - 'ip_address': request.META.get('REMOTE_ADDR'), - 'message': message, - # get_guid() must be called first or addons not in our DB will 404. - 'guid': self.get_guid(), - 'addon': self.get_addon_object()} - if request.user.is_authenticated: - abuse_kwargs['reporter'] = request.user - - report = AbuseReport.objects.create(**abuse_kwargs) - report.send() - - serializer = self.get_serializer(report) - return Response(serializer.data, status=status.HTTP_201_CREATED) - class UserAbuseViewSet(CreateModelMixin, GenericViewSet): permission_classes = [] serializer_class = UserAbuseReportSerializer throttle_classes = (AbuseThrottle,) - def get_user_object(self, user_id): + def get_user_object(self): if hasattr(self, 'user_object'): return self.user_object @@ -101,25 +77,3 @@ class UserAbuseViewSet(CreateModelMixin, GenericViewSet): return AccountViewSet( request=self.request, permission_classes=[], kwargs={'pk': self.kwargs['user_pk']}).get_object() - - def create(self, request, *args, **kwargs): - user_id = self.request.data.get('user') - if not user_id: - raise ParseError('Need a user parameter') - - message = self.request.data.get('message') - if not message: - raise ParseError('Abuse reports need a message') - - abuse_kwargs = { - 'ip_address': request.META.get('REMOTE_ADDR'), - 'message': message, - 'user': self.get_user_object(user_id)} - if request.user.is_authenticated: - abuse_kwargs['reporter'] = request.user - - report = AbuseReport.objects.create(**abuse_kwargs) - report.send() - - serializer = self.get_serializer(report) - return Response(serializer.data, status=status.HTTP_201_CREATED) diff --git a/src/olympia/reviewers/tests/test_views.py b/src/olympia/reviewers/tests/test_views.py index 22c0fbc065..2cf061e4c9 100644 --- a/src/olympia/reviewers/tests/test_views.py +++ b/src/olympia/reviewers/tests/test_views.py @@ -4708,7 +4708,7 @@ class TestAbuseReports(TestCase): AbuseReport.objects.create(user=someone, message=u'hey nöw') # Make a user abuse report for one of the add-on developers: it should # show up. - AbuseReport.objects.create(user=addon_developer, message='bü!') + AbuseReport.objects.create(user=addon_developer, message=u'bü!') def test_abuse_reports_list(self): assert self.client.login(email='admin@mozilla.com')