diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index e897e242..396c8076 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'))) ) @@ -68,7 +69,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 34322772..6a2bc4d3 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -201,8 +201,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: @@ -219,6 +217,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 4d773256..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 @@ -195,3 +195,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..5bf92964 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) @@ -127,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/') 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