From 1185b6c7fe54a6de23d39918904620b9cdf5160c Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 7 May 2024 13:22:58 +0200 Subject: [PATCH 1/3] Update extension fields from manifest metadata #111 When a new version is uploaded, extension's name, support and website fields will be updated using the values present in the manifest. --- common/tests/factories/extensions.py | 11 ++++++++++- common/tests/factories/files.py | 2 ++ extensions/models.py | 18 ++++++++++++++++++ extensions/signals.py | 19 +++++++++++++++++++ extensions/tests/test_manifest.py | 15 +++++++++++++++ extensions/tests/test_models.py | 5 +++++ extensions/tests/test_views.py | 5 +++++ 7 files changed, 74 insertions(+), 1 deletion(-) diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index b476d859..0500ba46 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -68,7 +68,16 @@ class VersionFactory(DjangoModelFactory): download_count = factory.Faker('random_int') tagline = factory.Faker('bs') - file = factory.SubFactory('common.tests.factories.files.FileFactory') + file = factory.SubFactory( + 'common.tests.factories.files.FileFactory', + metadata=factory.Dict( + { + 'name': factory.Faker('name'), + 'support': factory.Faker('url'), + 'website': factory.Faker('url'), + } + ), + ) ratings = factory.RelatedFactoryList( RatingFactory, size=lambda: random.randint(1, 50), factory_related_name='version' ) diff --git a/common/tests/factories/files.py b/common/tests/factories/files.py index 4f23bc54..866af0d7 100644 --- a/common/tests/factories/files.py +++ b/common/tests/factories/files.py @@ -25,3 +25,5 @@ class FileFactory(DjangoModelFactory): source = factory.Faker('file_name', extension='zip') user = factory.SubFactory('common.tests.factories.users.UserFactory') + + metadata = factory.Dict({}) diff --git a/extensions/models.py b/extensions/models.py index 8e94036e..09b712e7 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -203,6 +203,24 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod self.clean() return super().save(*args, **kwargs) + def update_metadata_from_version(self, version): + update_fields = set() + metadata = version.file.metadata + # check if we can also update name + # if we are uploading a new version, we have just validated and don't expect a clash, + # but if a version is being deleted, we want to rollback to a name from an older version, + # which may by clashing now, and we can't do anything about it + name = metadata.get('name') + if not self.__class__.objects.filter(name=name).exclude(pk=self.pk).exists(): + update_fields.add('name') + for field in ['support', 'website']: + # if a field is missing from manifest, don't reset the corresponding extension field + if metadata.get(field): + update_fields.add(field) + for field in update_fields: + setattr(self, field, metadata.get(field)) + self.save(update_fields=update_fields) + @transaction.atomic def approve(self, reviewer=None): """TODO: Approve an extension which is currently in review.""" diff --git a/extensions/signals.py b/extensions/signals.py index edd2a48b..cc046095 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -189,3 +189,22 @@ def _create_approval_activity_for_new_version_if_listed( extension=instance.extension, message=f'uploaded new version: {instance.version}', ).save() + + +@receiver(post_delete, sender=extensions.models.Version) +@receiver(post_save, sender=extensions.models.Version) +def _update_extension_metadata_from_latest_version( + sender: object, + instance: extensions.models.Version, + **kwargs: object, +): + # this code will also be triggered when an extension is deleted + # and it deletes all related versions + extension = instance.extension + latest_version = extension.latest_version + + # should check in case we are deleting the latest version, then no need to update anything + if not latest_version: + return + + extension.update_metadata_from_version(latest_version) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 1fd45841..54e343be 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -1,6 +1,7 @@ from django.test import TestCase from django.urls import reverse from django.core.exceptions import ValidationError +import factory from common.tests.factories.extensions import create_approved_version from common.tests.factories.files import FileFactory @@ -63,6 +64,13 @@ class CreateFileTest(TestCase): file=FileFactory( type=File.TYPES.BPY, status=File.STATUSES.APPROVED, + metadata=factory.Dict( + { + 'name': factory.Faker('name'), + 'support': factory.Faker('url'), + 'website': factory.Faker('url'), + } + ), ), ) @@ -240,6 +248,13 @@ class ValidateManifestTest(CreateFileTest): file=FileFactory( type=File.TYPES.BPY, status=File.STATUSES.APPROVED, + metadata=factory.Dict( + { + 'name': factory.Faker('name'), + 'support': factory.Faker('url'), + 'website': factory.Faker('url'), + } + ), ), ) diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index 91c573e5..6962cf01 100644 --- a/extensions/tests/test_models.py +++ b/extensions/tests/test_models.py @@ -22,6 +22,11 @@ class ExtensionTest(TestCase): extension__name='Extension name', extension__status=Extension.STATUSES.INCOMPLETE, extension__support='https://example.com/', + file__metadata={ + 'name': 'Extension name', + 'support': 'https://example.com/', + 'website': 'https://example.com/', + }, ).extension self.assertEqual(entries_for(self.extension).count(), 0) self.assertIsNone(self.extension.date_approved) diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index 797e8334..4d84d2bd 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -19,6 +19,11 @@ def _create_extension(): extension__website='https://example.com/', extension__status=Extension.STATUSES.INCOMPLETE, extension__average_score=2.5, + file__metadata={ + 'name': 'Test Add-on', + 'support': 'https://example.com/issues/', + 'website': 'https://example.com/', + }, ).extension -- 2.30.2 From ea6e5ed9780cc204a915aa5f407088a69a90357f Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 10 May 2024 14:57:39 +0200 Subject: [PATCH 2/3] add tests and remove slug check from cleanup --- common/tests/factories/extensions.py | 1 + extensions/models.py | 2 - extensions/tests/test_models.py | 73 ++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 0500ba46..47af3aae 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -19,6 +19,7 @@ class ExtensionFactory(DjangoModelFactory): name = factory.Faker('catch_phrase') extension_id = factory.Faker('slug') + slug = factory.Faker('slug') description = factory.LazyAttribute( lambda _: fake_markdown.post(size=random.choice(('medium', 'large'))) ) diff --git a/extensions/models.py b/extensions/models.py index 09b712e7..2ffcae2b 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -185,8 +185,6 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod return utils.slugify(EXTENSION_STATUS_CHOICES[self.status - 1][1]) def clean(self) -> None: - if not self.slug: - self.slug = utils.slugify(self.name) # Require at least one approved version with a file for approved extensions if self.status == self.STATUSES.APPROVED: if not self.latest_version: diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index 6962cf01..5bf92964 100644 --- a/extensions/tests/test_models.py +++ b/extensions/tests/test_models.py @@ -132,3 +132,76 @@ class VersionTest(TestCase): response = self.client.get(path) self.assertEqual(response.status_code, 200, path) + + +class UpdateMetadataTest(TestCase): + fixtures = ['dev', 'licenses'] + + def setUp(self): + super().setUp() + self.first_version = create_version( + extension__description='Extension description', + extension__name='name', + extension__status=Extension.STATUSES.INCOMPLETE, + extension__support='https://example.com/', + extension__website='https://example.com/', + file__metadata={ + 'name': 'name', + 'support': 'https://example.com/', + 'website': 'https://example.com/', + }, + ) + self.extension = self.first_version.extension + + def test_version_create_and_delete(self): + second_version = create_version( + extension=self.extension, + file__metadata={ + 'name': 'new name', + 'support': 'https://example.com/new', + 'website': 'https://example.com/new', + }, + ) + self.extension.refresh_from_db() + self.assertEqual(self.extension.name, 'new name') + self.assertEqual(self.extension.support, 'https://example.com/new') + self.assertEqual(self.extension.website, 'https://example.com/new') + + second_version.delete() + self.extension.refresh_from_db() + self.assertEqual(self.extension.name, 'name') + self.assertEqual(self.extension.support, 'https://example.com/') + self.assertEqual(self.extension.website, 'https://example.com/') + + def test_old_name_taken(self): + second_version = create_version( + extension=self.extension, + file__metadata={ + 'name': 'new name', + 'support': 'https://example.com/new', + 'website': 'https://example.com/new', + }, + ) + + # another extension uses old name + create_version( + extension__description='Extension description', + extension__extension_id='lalalala', + extension__name='name', + extension__status=Extension.STATUSES.INCOMPLETE, + extension__support='https://example.com/', + extension__website='https://example.com/', + file__metadata={ + 'name': 'name', + 'support': 'https://example.com/', + 'website': 'https://example.com/', + }, + ) + + second_version.delete() + self.extension.refresh_from_db() + # couldn't revert the name because it has been taken + self.assertEqual(self.extension.name, 'new name') + # reverted other fields + self.assertEqual(self.extension.support, 'https://example.com/') + self.assertEqual(self.extension.website, 'https://example.com/') -- 2.30.2 From d72fe5d4234a612eeb8c3661e436865845562a78 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 10 May 2024 15:09:26 +0200 Subject: [PATCH 3/3] bring back post_delete import, was removed in main --- extensions/signals.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/signals.py b/extensions/signals.py index a2dcd3b0..54034d79 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -4,7 +4,7 @@ import logging from actstream.actions import follow, unfollow from django.contrib.auth import get_user_model from django.contrib.auth.models import Group -from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete +from django.db.models.signals import m2m_changed, pre_save, post_save, pre_delete, post_delete from django.dispatch import receiver from constants.activity import Flag -- 2.30.2