From 78c4003dd0ea64c4205e3f6de1f6aaab9c710424 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Tue, 5 Dec 2023 12:25:11 +0100 Subject: [PATCH] Send required metadata for all reportable types (#21548) * Send required metadata for all reportable types --- src/olympia/abuse/cinder.py | 92 +++++++++++++++++----- src/olympia/abuse/tests/test_cinder.py | 103 ++++++++++++++++++++++--- src/olympia/abuse/tests/test_tasks.py | 12 ++- 3 files changed, 173 insertions(+), 34 deletions(-) diff --git a/src/olympia/abuse/cinder.py b/src/olympia/abuse/cinder.py index 44a811d94c..f101a1eb57 100644 --- a/src/olympia/abuse/cinder.py +++ b/src/olympia/abuse/cinder.py @@ -26,7 +26,10 @@ class CinderEntity: @property def id(self): # Ideally override this in subclasses to be more efficient - return str(self.get_attributes().get('id', '')) + return self.get_str(self.get_attributes().get('id', '')) + + def get_str(self, field_content): + return str(field_content or '') def get_attributes(self): raise NotImplementedError @@ -46,10 +49,11 @@ class CinderEntity: 'relationship_type': relationship_type, } - def get_media_attributes(self): - # media attributes are typically only returned with reports (not as - # part of relationships or reporter data for instance) as they require - # us to make a copy of the media + def get_extended_attributes(self): + # Extended attributes are only returned with reports (not as part of + # relationships or reporter data for instance) as they require as they + # are more expensive to compute. Media that we need to make a copy of + # are typically returned there instead of in get_attributes(). return {} def build_report_payload(self, *, report_text, category, reporter): @@ -61,7 +65,7 @@ class CinderEntity: context['relationships'] += [ reporter.get_relationship_data(self, 'amo_reporter_of') ] - entity_attributes = {**self.get_attributes(), **self.get_media_attributes()} + entity_attributes = {**self.get_attributes(), **self.get_extended_attributes()} return { 'queue_slug': self.queue, 'entity_type': self.type, @@ -123,17 +127,18 @@ class CinderUser(CinderEntity): @property def id(self): - return str(self.user.id) + return self.get_str(self.user.id) def get_attributes(self): return { 'id': self.id, - 'name': self.user.display_name, + 'created': self.get_str(self.user.created), 'email': self.user.email, 'fxa_id': self.user.fxa_id, + 'name': self.user.display_name, } - def get_media_attributes(self): + def get_extended_attributes(self): data = {} if ( self.user.picture_type @@ -147,10 +152,25 @@ class CinderUser(CinderEntity): 'value': create_signed_url_for_file_backup(filename), 'mime_type': self.user.picture_type, } + data.update( + { + 'average_rating': int(self.user.averagerating or 0), + 'num_addons_listed': self.user.num_addons_listed, + 'biography': self.get_str(self.user.biography), + 'homepage': self.get_str(self.user.homepage), + 'location': self.get_str(self.user.location), + 'occupation': self.get_str(self.user.occupation), + } + ) return data def get_context(self): - cinder_addons = [CinderAddon(addon) for addon in self.user.addons.all()] + cinder_addons = [ + CinderAddon(addon) + for addon in self.user.addons.all() + .only_translations() + .select_related('promotedaddon') + ] return { 'entities': [ cinder_addon.get_entity_data() for cinder_addon in cinder_addons @@ -197,17 +217,31 @@ class CinderAddon(CinderEntity): @property def id(self): - return str(self.addon.id) + return self.get_str(self.addon.id) def get_attributes(self): - return { + # FIXME: translate translated fields in reporter's locale, send as + # dictionaries? + # We look at the promoted group to tell whether or not the add-on has + # a badge, but we don't care about the promotion being approved for the + # current version, it would make more queries and it's not useful for + # moderation purposes anyway. + promoted_group = self.addon.promoted_group(currently_approved=False) + data = { 'id': self.id, + 'average_daily_users': self.addon.average_daily_users, 'guid': self.addon.guid, + 'last_updated': self.get_str(self.addon.last_updated), + 'name': self.get_str(self.addon.name), 'slug': self.addon.slug, - 'name': str(self.addon.name), + 'summary': self.get_str(self.addon.summary), + 'promoted_badge': self.get_str( + promoted_group.name if promoted_group.badged else '' + ), } + return data - def get_media_attributes(self): + def get_extended_attributes(self): data = {} if backup_storage_enabled(): if self.addon.icon_type: @@ -245,6 +279,21 @@ class CinderAddon(CinderEntity): if previews: data['previews'] = previews + # Those fields are only shown on the detail page, so we only have them + # in extended attributes to avoid sending them when we send an add-on + # as a related entity to something else. + data['description'] = self.get_str(self.addon.description) + if self.addon.current_version: + data['version'] = self.get_str(self.addon.current_version.version) + data['release_notes'] = self.get_str( + self.addon.current_version.release_notes + ) + # In addition, the URL/email fields can't be sent if blank as they + # would not be considered valid by Cinder. + for field in ('homepage', 'support_email', 'support_url'): + if value := getattr(self.addon, field): + data[field] = self.get_str(value) + return data def get_context(self): @@ -266,12 +315,13 @@ class CinderRating(CinderEntity): @property def id(self): - return str(self.rating.id) + return self.get_str(self.rating.id) def get_attributes(self): return { 'id': self.id, 'body': self.rating.body, + 'score': self.rating.rating, } def get_context(self): @@ -294,16 +344,18 @@ class CinderCollection(CinderEntity): @property def id(self): - return str(self.collection.id) + return self.get_str(self.collection.id) def get_attributes(self): + # FIXME: translate translated fields in reporter's locale, send as + # dictionaries? return { 'id': self.id, - 'slug': self.collection.slug, - # FIXME: locales! - 'name': str(self.collection.name), - 'description': str(self.collection.description), 'comments': self.collection.get_all_comments(), + 'description': self.get_str(self.collection.description), + 'modified': self.get_str(self.collection.modified), + 'name': self.get_str(self.collection.name), + 'slug': self.collection.slug, } def get_context(self): diff --git a/src/olympia/abuse/tests/test_cinder.py b/src/olympia/abuse/tests/test_cinder.py index 10cefda7d6..6cbeba3b4b 100644 --- a/src/olympia/abuse/tests/test_cinder.py +++ b/src/olympia/abuse/tests/test_cinder.py @@ -1,4 +1,5 @@ import os.path +import random from unittest import mock from django.conf import settings @@ -136,7 +137,12 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): return addon_factory(**kwargs) def test_build_report_payload(self): - addon = self._create_dummy_target() + addon = self._create_dummy_target( + homepage='https://home.example.com', + support_email='support@example.com', + support_url='https://support.example.com/', + description='Sôme description', + ) reason = 'bad addon!' cinder_addon = self.cinder_class(addon) @@ -148,9 +154,19 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_addon', 'entity': { 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, + 'description': str(addon.description), 'guid': addon.guid, - 'slug': addon.slug, + 'homepage': str(addon.homepage), + 'last_updated': str(addon.last_updated), 'name': str(addon.name), + 'promoted_badge': '', + 'release_notes': '', + 'slug': addon.slug, + 'summary': str(addon.summary), + 'support_email': str(addon.support_email), + 'support_url': str(addon.support_url), + 'version': addon.current_version.version, }, 'reasoning': reason, 'context': {'entities': [], 'relationships': []}, @@ -197,6 +213,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(reporter_user.id), + 'created': str(reporter_user.created), 'name': reporter_user.display_name, 'email': reporter_user.email, 'fxa_id': reporter_user.fxa_id, @@ -229,6 +246,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(author.id), + 'created': str(author.created), 'name': author.display_name, 'email': author.email, 'fxa_id': author.fxa_id, @@ -257,6 +275,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(author.id), + 'created': str(author.created), 'name': author.display_name, 'email': author.email, 'fxa_id': author.fxa_id, @@ -266,6 +285,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(reporter_user.id), + 'created': str(reporter_user.created), 'name': reporter_user.display_name, 'email': reporter_user.email, 'fxa_id': reporter_user.fxa_id, @@ -303,6 +323,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(user.id), + 'created': str(user.created), 'name': user.display_name, 'email': user.email, 'fxa_id': user.fxa_id, @@ -365,12 +386,14 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_addon', 'entity': { 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, + 'description': '', + 'guid': addon.guid, 'icon': { 'mime_type': 'image/png', 'value': f'https://cloud.example.com/{addon.pk}-128.png?some=thing', }, - 'guid': addon.guid, - 'slug': addon.slug, + 'last_updated': str(addon.last_updated), 'name': str(addon.name), 'previews': [ { @@ -382,6 +405,11 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'value': f'https://cloud.example.com/{p1.pk}.jpg?some=thing', }, ], + 'promoted_badge': '', + 'release_notes': '', + 'slug': addon.slug, + 'summary': str(addon.summary), + 'version': str(addon.current_version.version), }, 'reasoning': reason, 'context': {'entities': [], 'relationships': []}, @@ -399,7 +427,7 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): lambda fpath, type_: os.path.basename(fpath) ) create_signed_url_for_file_backup_mock.side_effect = ( - lambda rpath: f'https://cloud.example.com/{rpath}?some=thing' + lambda rpath: f'https://cloud.example.com/{rpath}?what=ever' ) addon = self._create_dummy_target() addon.update(type=amo.ADDON_STATICTHEME) @@ -428,15 +456,22 @@ class TestCinderAddon(BaseTestCinderCase, TestCase): 'entity_type': 'amo_addon', 'entity': { 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, + 'description': '', 'guid': addon.guid, - 'slug': addon.slug, + 'last_updated': str(addon.last_updated), 'name': str(addon.name), 'previews': [ { 'mime_type': 'image/png', - 'value': f'https://cloud.example.com/{p0.pk}.png?some=thing', + 'value': f'https://cloud.example.com/{p0.pk}.png?what=ever', }, ], + 'promoted_badge': '', + 'release_notes': '', + 'slug': addon.slug, + 'summary': str(addon.summary), + 'version': str(addon.current_version.version), }, 'reasoning': reason, 'context': {'entities': [], 'relationships': []}, @@ -505,7 +540,12 @@ class TestCinderUser(BaseTestCinderCase, TestCase): return user_factory(**kwargs) def test_build_report_payload(self): - user = self._create_dummy_target() + user = self._create_dummy_target( + biography='Bîo', + location='Deep space', + occupation='Blah', + homepage='http://home.example.com', + ) reason = 'bad person!' cinder_user = self.cinder_class(user) @@ -520,6 +560,13 @@ class TestCinderUser(BaseTestCinderCase, TestCase): 'name': user.display_name, 'email': user.email, 'fxa_id': user.fxa_id, + 'homepage': user.homepage, + 'average_rating': 0, + 'num_addons_listed': 0, + 'created': str(user.created), + 'occupation': user.occupation, + 'location': user.location, + 'biography': user.biography, }, 'reasoning': reason, 'context': {'entities': [], 'relationships': []}, @@ -566,6 +613,7 @@ class TestCinderUser(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(reporter_user.id), + 'created': str(reporter_user.created), 'name': reporter_user.display_name, 'email': reporter_user.email, 'fxa_id': reporter_user.fxa_id, @@ -596,15 +644,20 @@ class TestCinderUser(BaseTestCinderCase, TestCase): 'entity_type': 'amo_addon', 'attributes': { 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, 'guid': addon.guid, - 'slug': addon.slug, + 'last_updated': str(addon.last_updated), 'name': str(addon.name), + 'promoted_badge': '', + 'slug': addon.slug, + 'summary': str(addon.summary), }, }, { 'entity_type': 'amo_user', 'attributes': { 'id': str(user.id), + 'created': str(user.created), 'name': user.display_name, 'email': user.email, 'fxa_id': user.fxa_id, @@ -644,9 +697,13 @@ class TestCinderUser(BaseTestCinderCase, TestCase): 'entity_type': 'amo_addon', 'attributes': { 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, 'guid': addon.guid, - 'slug': addon.slug, + 'last_updated': str(addon.last_updated), 'name': str(addon.name), + 'promoted_badge': '', + 'slug': addon.slug, + 'summary': str(addon.summary), }, } ], @@ -672,15 +729,20 @@ class TestCinderUser(BaseTestCinderCase, TestCase): 'entity_type': 'amo_addon', 'attributes': { 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, 'guid': addon.guid, - 'slug': addon.slug, + 'last_updated': str(addon.last_updated), 'name': str(addon.name), + 'promoted_badge': '', + 'slug': addon.slug, + 'summary': str(addon.summary), }, }, { 'entity_type': 'amo_user', 'attributes': { 'id': str(reporter_user.id), + 'created': str(reporter_user.created), 'name': reporter_user.display_name, 'email': reporter_user.email, 'fxa_id': reporter_user.fxa_id, @@ -740,6 +802,13 @@ class TestCinderUser(BaseTestCinderCase, TestCase): 'name': user.display_name, 'email': user.email, 'fxa_id': user.fxa_id, + 'homepage': '', + 'average_rating': 0, + 'num_addons_listed': 0, + 'created': str(user.created), + 'occupation': '', + 'location': '', + 'biography': '', }, 'reasoning': reason, 'context': {'entities': [], 'relationships': []}, @@ -763,7 +832,9 @@ class TestCinderRating(BaseTestCinderCase, TestCase): self.addon = addon_factory() def _create_dummy_target(self, **kwargs): - return Rating.objects.create(addon=self.addon, user=self.user, **kwargs) + return Rating.objects.create( + addon=self.addon, user=self.user, rating=random.randint(0, 5), **kwargs + ) def test_build_report_payload(self): rating = self._create_dummy_target() @@ -779,6 +850,7 @@ class TestCinderRating(BaseTestCinderCase, TestCase): 'entity': { 'id': str(rating.id), 'body': rating.body, + 'score': rating.rating, }, 'reasoning': reason, 'context': { @@ -787,6 +859,7 @@ class TestCinderRating(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(self.user.id), + 'created': str(self.user.created), 'name': self.user.display_name, 'email': self.user.email, 'fxa_id': self.user.fxa_id, @@ -818,6 +891,7 @@ class TestCinderRating(BaseTestCinderCase, TestCase): 'entity': { 'id': str(rating.id), 'body': rating.body, + 'score': rating.rating, }, 'reasoning': 'my own words!', 'context': { @@ -826,6 +900,7 @@ class TestCinderRating(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(user.id), + 'created': str(user.created), 'name': user.display_name, 'email': user.email, 'fxa_id': user.fxa_id, @@ -888,6 +963,7 @@ class TestCinderCollection(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(self.user.id), + 'created': str(self.user.created), 'name': self.user.display_name, 'email': self.user.email, 'fxa_id': self.user.fxa_id, @@ -907,6 +983,7 @@ class TestCinderCollection(BaseTestCinderCase, TestCase): 'entity': { 'comments': ['Fôo', 'Bär', 'Alice'], 'description': str(collection.description), + 'modified': str(collection.modified), 'id': str(collection.pk), 'name': str(collection.name), 'slug': collection.slug, @@ -931,6 +1008,7 @@ class TestCinderCollection(BaseTestCinderCase, TestCase): 'entity_type': 'amo_user', 'attributes': { 'id': str(user.id), + 'created': str(user.created), 'name': user.display_name, 'email': user.email, 'fxa_id': user.fxa_id, @@ -957,6 +1035,7 @@ class TestCinderCollection(BaseTestCinderCase, TestCase): 'entity': { 'comments': ['Fôo', 'Bär', 'Alice'], 'description': str(collection.description), + 'modified': str(collection.modified), 'id': str(collection.pk), 'name': str(collection.name), 'slug': collection.slug, diff --git a/src/olympia/abuse/tests/test_tasks.py b/src/olympia/abuse/tests/test_tasks.py index ace7fb59af..3bf0cfb89a 100644 --- a/src/olympia/abuse/tests/test_tasks.py +++ b/src/olympia/abuse/tests/test_tasks.py @@ -209,10 +209,17 @@ def test_addon_report_to_cinder(): assert json.loads(request.body) == { 'context': {'entities': [], 'relationships': []}, 'entity': { + 'id': str(addon.id), + 'average_daily_users': addon.average_daily_users, + 'description': '', 'guid': addon.guid, - 'id': str(addon.pk), + 'last_updated': str(addon.last_updated), 'name': str(addon.name), + 'release_notes': '', + 'promoted_badge': '', 'slug': addon.slug, + 'summary': str(addon.summary), + 'version': str(addon.current_version.version), }, 'entity_type': 'amo_addon', 'queue_slug': 'amo-dev-content-infringement', @@ -297,10 +304,11 @@ def test_addon_appeal_to_cinder_authenticated(): assert request.headers['authorization'] == 'Bearer fake-test-token' assert json.loads(request.body) == { 'appealer_entity': { + 'created': str(user.created), 'email': user.email, + 'fxa_id': user.fxa_id, 'id': str(user.pk), 'name': '', - 'fxa_id': user.fxa_id, }, 'appealer_entity_type': 'amo_user', 'decision_to_appeal_id': '4815162342-abc',