From 6d8fd61b23a43ef620eebf526a2517c3f1f5c251 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 18 Jun 2024 18:09:37 +0200 Subject: [PATCH 1/5] Refactor Extension and Version: explicit constructors from File This change solves a blocker for an upcoming drop of Version.file field: calculating Extension.latest_version relied on having VersionFiles already populated, but it was triggered on Version.save(), which happens before the cross table could be populated. One functional change is that a Version object is now created in NewVersionView, immediately after the File is saved, and not in NewVersionFinalizeView, as before. Tests are rewritten to require only FileFactory, the Extension and Version objects are created as if it happens in the rest of production code. This loses a bit on the ergonomics of factories, but overall makes the state of test objects more consistent, often simplifying the test code. --- .../management/commands/generate_fake_data.py | 80 ++++++-------- .../commands/generate_more_fake_data.py | 44 ++++---- common/tests/factories/extensions.py | 14 ++- common/tests/factories/files.py | 31 +++++- extensions/admin.py | 2 + extensions/models.py | 91 +++++++++------- extensions/tests/test_api.py | 47 ++++---- extensions/tests/test_delete.py | 45 ++++---- extensions/tests/test_manifest.py | 41 ++----- extensions/tests/test_models.py | 103 ++++-------------- extensions/tests/test_submit.py | 44 +++----- extensions/tests/test_update.py | 38 +++---- extensions/tests/test_views.py | 27 ++--- extensions/views/api.py | 9 +- extensions/views/manage.py | 18 ++- extensions/views/mixins.py | 1 - extensions/views/submit.py | 40 +------ files/models.py | 4 +- files/tests/test_models.py | 1 + files/tests/test_tasks.py | 6 +- ratings/tests/test_views.py | 16 +-- reviewers/tests/test_views.py | 2 +- .../commands/tests/test_write_stats.py | 35 +++--- users/tests/test_tasks.py | 6 +- 24 files changed, 311 insertions(+), 434 deletions(-) diff --git a/common/management/commands/generate_fake_data.py b/common/management/commands/generate_fake_data.py index a2d94efb..5a0bdd34 100644 --- a/common/management/commands/generate_fake_data.py +++ b/common/management/commands/generate_fake_data.py @@ -2,18 +2,16 @@ import logging import random from django.core.management.base import BaseCommand -import faker +from django.db import transaction from common.tests.factories.extensions import create_approved_version, create_version -from common.tests.factories.files import FileFactory +from common.tests.factories.files import FileFactory, ImageFactory from common.tests.factories.teams import TeamFactory from files.models import File from constants.version_permissions import VERSION_PERMISSION_FILE, VERSION_PERMISSION_NETWORK from constants.licenses import LICENSE_GPL2, LICENSE_GPL3 from extensions.models import Extension, Tag -_faker = faker.Faker() - FILE_SOURCES = { "blender-kitsu": { "file": 'files/ed/ed656b177b01999e6fcd0e37c34ced471ef88c89db578f337e40958553dca5d2.zip', @@ -64,6 +62,7 @@ LICENSES = (LICENSE_GPL2.id, LICENSE_GPL3.id) class Command(BaseCommand): help = 'Generate fake data with extensions, users and versions using test factories.' + @transaction.atomic def handle(self, *args, **options): verbosity = int(options['verbosity']) root_logger = logging.getLogger('root') @@ -81,70 +80,55 @@ class Command(BaseCommand): # Create a fixed example example_version = create_approved_version( - file__status=File.STATUSES.APPROVED, - # extension__status=Extension.STATUSES.APPROVED, - extension__name='Blender Kitsu', - extension__extension_id='blender_kitsu', - tags=['Development'], - extension__description=EXAMPLE_DESCRIPTION, - extension__support='https://developer.blender.org/', - extension__website='https://studio.blender.org/', - blender_version_min='2.93.0', - version='0.1.5-alpha+f52258de', file=FileFactory( - type=File.TYPES.BPY, - source=FILE_SOURCES["blender-kitsu"]["file"], + metadata__blender_version_min='2.93.0', + metadata__id='blender_kitsu', + metadata__name='Blender Kitsu', + metadata__support='https://developer.blender.org/', + metadata__tags=['Development'], + metadata__version='0.1.5-alpha+f52258de', + metadata__website='https://studio.blender.org/', original_hash=FILE_SOURCES["blender-kitsu"]["hash"], size_bytes=FILE_SOURCES["blender-kitsu"]["size"], + source=FILE_SOURCES["blender-kitsu"]["file"], status=File.STATUSES.APPROVED, - metadata={'name': 'Blender Kitsu'}, + type=File.TYPES.BPY, ), - extension__previews=[ - FileFactory( - type=File.TYPES.IMAGE, - source=source, - status=File.STATUSES.APPROVED, - ) - for source in PREVIEW_SOURCES[-2:] - ], ) + for preview in [ + ImageFactory(source=source, status=File.STATUSES.APPROVED) + for source in PREVIEW_SOURCES[-2:] + ]: + example_version.extension.previews.add(preview) example_version.permissions.add(VERSION_PERMISSION_FILE.id) example_version.permissions.add(VERSION_PERMISSION_NETWORK.id) example_version.licenses.add(LICENSE_GPL2.id) # Create a few publicly listed extensions for i in range(10): - extension__type = random.choice(Extension.TYPES)[0] - name = _faker.catch_phrase() + type = random.choice(Extension.TYPES)[0] version = create_approved_version( - file__status=File.STATUSES.APPROVED, - file__metadata={'name': name}, - # extension__status=Extension.STATUSES.APPROVED, - extension__name=name, - extension__type=extension__type, - tags=random.sample(tags[extension__type], k=1), - extension__previews=[ - FileFactory( - type=File.TYPES.IMAGE, - source=source, - status=File.STATUSES.APPROVED, - ) - for source in random.sample( - PREVIEW_SOURCES, k=random.randint(1, len(PREVIEW_SOURCES) - 1) - ) - ], + status=File.STATUSES.APPROVED, + metadata__tags=random.sample(tags[type], k=1), + type=type, ) + for preview in [ + ImageFactory(source=source, status=File.STATUSES.APPROVED) + for source in random.sample( + PREVIEW_SOURCES, k=random.randint(1, len(PREVIEW_SOURCES) - 1) + ) + ]: + version.extension.previews.add(preview) for i in range(random.randint(1, len(LICENSES))): version.licenses.add(LICENSES[i]) # Create a few unlisted extension versions for i in range(5): - extension__type = random.choice(Extension.TYPES)[0] + type = random.choice(Extension.TYPES)[0] version = create_version( - file__status=random.choice( - (File.STATUSES.DISABLED, File.STATUSES.DISABLED_BY_AUTHOR) - ), - tags=random.sample(tags[extension__type], k=1), + status=random.choice((File.STATUSES.DISABLED, File.STATUSES.DISABLED_BY_AUTHOR)), + type=type, + metadata__tags=random.sample(tags[type], k=1), ) for i in range(random.randint(1, len(LICENSES))): version.licenses.add(LICENSES[i]) diff --git a/common/management/commands/generate_more_fake_data.py b/common/management/commands/generate_more_fake_data.py index de273ea7..56e86bf3 100644 --- a/common/management/commands/generate_more_fake_data.py +++ b/common/management/commands/generate_more_fake_data.py @@ -7,11 +7,11 @@ from django.db.models import signals import factory from common.tests.factories.extensions import create_version, RatingFactory -from common.tests.factories.files import FileFactory +from common.tests.factories.files import ImageFactory from files.models import File from constants.licenses import LICENSE_GPL2, LICENSE_GPL3 from extensions.models import Extension, Tag -from utils import slugify, chunked +from utils import chunked import faker _faker = faker.Faker() @@ -109,31 +109,25 @@ class Command(BaseCommand): for _ in i_chunked: name = _faker.catch_phrase() + _faker.bothify('###???') extension_id = name.replace(' ', '_') - slug = slugify(extension_id)[:50] version = create_version( - file__status=file_status, - file__metadata={'name': name, 'id': extension_id}, - tags=random.sample(tags[_type], k=1), - extension__extension_id=extension_id, - extension__is_listed=extension_status == e_sts.APPROVED, - extension__name=name, - extension__slug=slug, - extension__status=extension_status, - extension__type=_type, - extension__previews=[ - FileFactory( - type=File.TYPES.IMAGE, - source=source, - status=file_status, - ) - for source in random.sample( - PREVIEW_SOURCES, - k=random.randint(1, len(PREVIEW_SOURCES) - 1), - ) - ], - # Create these separately - ratings=[], + metadata__id=extension_id, + metadata__name=name, + metadata__tags=random.sample(tags[_type], k=1), + status=file_status, + type=_type, ) + version.extension.is_listed = extension_status == e_sts.APPROVED + version.extension.status = extension_status + version.extension.save(update_fields={'is_listed', 'status'}) + for preview in [ + ImageFactory(source=source, status=file_status) + for source in random.sample( + PREVIEW_SOURCES, + k=random.randint(1, len(PREVIEW_SOURCES) - 1), + ) + ]: + version.extension.previews.add(preview) + for i in range(random.randint(1, len(LICENSES))): version.licenses.add(LICENSES[i]) if version.is_listed: diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 7dd0203c..897bbb82 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -6,6 +6,7 @@ from mdgen import MarkdownPostProvider import factory import factory.fuzzy +from common.tests.factories.files import FileFactory from extensions.models import Extension, Version, Tag, Preview, Platform from ratings.models import Rating @@ -120,9 +121,16 @@ class VersionFactory(DjangoModelFactory): def create_version(**kwargs) -> 'Version': - version = VersionFactory(**kwargs) - version.extension.authors.add(version.file.user) - return version + extension = kwargs.pop('extension', None) + file = kwargs.pop('file', None) + + if not file: + file = FileFactory(**kwargs) + + if not extension: + extension = Extension.create_from_file(file, file.user) + + return extension.create_version_from_file(file) def create_approved_version(**kwargs) -> 'Version': diff --git a/common/tests/factories/files.py b/common/tests/factories/files.py index f3da6bfc..626e9526 100644 --- a/common/tests/factories/files.py +++ b/common/tests/factories/files.py @@ -1,3 +1,5 @@ +import random + from factory.django import DjangoModelFactory import factory import factory.fuzzy @@ -5,6 +7,23 @@ import factory.fuzzy from files.models import File +class FileMetadataFactory(factory.DictFactory): + name = factory.Faker('name') + id = factory.Faker('slug') + support = factory.Faker('url') + website = factory.Faker('url') + version = factory.LazyAttribute( + lambda _: f'{random.randint(0, 9)}.{random.randint(0, 9)}.{random.randint(0, 9)}', + ) + blender_version_min = factory.fuzzy.FuzzyChoice( + {'2.83.1', '2.93.0', '2.93.8', '3.0.0', '3.2.1'} + ) + tagline = factory.Faker('bs') + schema_version = '1.0.0' + platforms = [] + tags = [] + + class FileFactory(DjangoModelFactory): class Meta: model = File @@ -14,10 +33,11 @@ class FileFactory(DjangoModelFactory): hash = factory.Faker('lexify', text='fakehash:??????????????????', letters='deadbeef') size_bytes = factory.Faker('random_int', min=1234) source = factory.Faker('file_name', extension='zip') + type = File.TYPES.BPY user = factory.SubFactory('common.tests.factories.users.UserFactory') - metadata = factory.Dict({}) + metadata = factory.SubFactory(FileMetadataFactory) class ImageFactory(FileFactory): @@ -25,3 +45,12 @@ class ImageFactory(FileFactory): source = 'images/de/deadbeef.png' type = File.TYPES.IMAGE size_bytes = 1234 + metadata = {} + + +class VideoFactory(FileFactory): + original_name = factory.Faker('file_name', extension='mp4') + source = 'images/be/beefcafe.mp4' + type = File.TYPES.VIDEO + size_bytes = 12345678 + metadata = {} diff --git a/extensions/admin.py b/extensions/admin.py index 9dbed1d0..1330b2db 100644 --- a/extensions/admin.py +++ b/extensions/admin.py @@ -84,6 +84,7 @@ class ExtensionAdmin(admin.ModelAdmin): 'icon', 'featured_image', 'latest_version', + 'is_listed', ) autocomplete_fields = ('team',) @@ -106,6 +107,7 @@ class ExtensionAdmin(admin.ModelAdmin): ('icon', 'featured_image'), 'status', 'latest_version', + 'is_listed', ), }, ), diff --git a/extensions/models.py b/extensions/models.py index e76d036e..1c0b1d4c 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -185,6 +185,55 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model): ] ordering = ['-average_score', '-date_created', 'name'] + @classmethod + def create_from_file(cls, file: File, author: User) -> 'Extension': + extension = cls(**file.parsed_extension_fields) + with transaction.atomic(): + extension.save() + extension.authors.add(author) + return extension + + def create_version_from_file(self, file: File, release_notes='') -> 'Version': + """This should be the only method to create a new Version object. + + Side effects: + - updates file status and adds an approval queue comment when extension is listed + - updates extension.latest_version + """ + fields = file.parsed_version_fields + licenses = fields.pop('licenses', []) + permissions = fields.pop('permissions', {}) + if permissions: + permissions = list(permissions.keys()) + platforms = fields.pop('platforms', []) + tags = fields.pop('tags', []) + version = Version(**fields, extension=self, release_notes=release_notes, file=file) + with transaction.atomic(): + version.save() + version.files.add(file) + version.set_initial_licenses(licenses) + version.set_initial_permissions(permissions) + version.set_initial_platforms(platforms) + version.set_initial_tags(tags) + + # auto approving our file if extension is already listed (i.e. have been approved) + if self.is_listed: + args = {'f_id': file.pk, 'pk': version.pk, 's': file.source.name} + log.info('Auto-approving file pk=%(f_id)s of Version pk=%(pk)s source=%(s)s', args) + file.status = File.STATUSES.APPROVED + file.save(update_fields={'status', 'date_modified'}) + + ApprovalActivity( + type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, + user=file.user, + extension=self, + message=f'uploaded new version: {version}', + ).save() + + self.update_latest_version() + + return version + def __str__(self): return f'{self.get_type_display()} "{self.name}"' @@ -486,28 +535,6 @@ class VersionManager(models.Manager): def unlisted(self): return self.exclude(file__status=FILE_STATUS_CHOICES.APPROVED) - def update_or_create(self, *args, **kwargs): - # Stash the ManyToMany to be created after the Version has a valid ID already - licenses = kwargs.pop('licenses', []) - permissions = kwargs.pop('permissions', {}) - # FIXME: ideally this dict->list transformation should be a caller's responsibility - # but we plan to change the model soon, so getting a dict as an input here makes sense - if permissions: - permissions = list(permissions.keys()) - platforms = kwargs.pop('platforms', []) - tags = kwargs.pop('tags', []) - file = kwargs.get('file') # FIXME legacy_version: replace get with pop - - version, result = super().update_or_create(*args, **kwargs) - - # Add the ManyToMany to the already initialized Version - version.files.add(file) - version.set_initial_licenses(licenses) - version.set_initial_permissions(permissions) - version.set_initial_platforms(platforms) - version.set_initial_tags(tags) - return version, result - class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): track_changes_to_fields = { @@ -725,32 +752,12 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): @transaction.atomic def save(self, *args, **kwargs): - is_new = self.pk is None update_fields = kwargs.get('update_fields', None) was_changed, old_state = self.pre_save_record(update_fields=update_fields) self.record_status_change(was_changed, old_state, **kwargs) super().save(*args, **kwargs) - if not is_new: - return - - # auto approving our file if extension is already listed (i.e. have been approved) - if self.extension.is_listed: - args = {'f_id': self.file.pk, 'pk': self.pk, 's': self.file.source.name} - log.info('Auto-approving file pk=%(f_id)s of Version pk=%(pk)s source=%(s)s', args) - self.file.status = File.STATUSES.APPROVED - self.file.save(update_fields={'status', 'date_modified'}) - - ApprovalActivity( - type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, - user=self.file.user, - extension=self.extension, - message=f'uploaded new version: {self.version}', - ).save() - - self.extension.update_latest_version() - @transaction.atomic def delete(self, *args, **kwargs): if self == self.extension.latest_version: diff --git a/extensions/tests/test_api.py b/extensions/tests/test_api.py index efb15d99..da38b708 100644 --- a/extensions/tests/test_api.py +++ b/extensions/tests/test_api.py @@ -79,7 +79,7 @@ class ResponseFormatTest(APITestCase): self.assertEqual(v['archive_url'][-4:], '.zip') def test_maintaner_is_team(self): - version = create_approved_version(blender_version_min='4.0.1') + version = create_approved_version(metadata__blender_version_min='4.0.1') team = Team(name='test team', slug='test-team') team.save() version.extension.team = team @@ -107,9 +107,9 @@ class ResponseFormatTest(APITestCase): class FiltersTest(APITestCase): def test_blender_version_filter(self): - create_approved_version(blender_version_min='4.0.1') - create_approved_version(blender_version_min='4.1.1') - create_approved_version(blender_version_min='4.2.1') + create_approved_version(metadata__blender_version_min='4.0.1') + create_approved_version(metadata__blender_version_min='4.1.1') + create_approved_version(metadata__blender_version_min='4.2.1') url = reverse('extensions:api') json = self.client.get( @@ -131,8 +131,8 @@ class FiltersTest(APITestCase): self.assertEqual(len(json3['data']), 3) def test_platform_filter(self): - create_approved_version(platforms=['windows-x64']) - create_approved_version(platforms=['windows-arm64']) + create_approved_version(metadata__platforms=['windows-x64']) + create_approved_version(metadata__platforms=['windows-arm64']) create_approved_version() url = reverse('extensions:api') @@ -151,27 +151,30 @@ class FiltersTest(APITestCase): self.assertEqual(len(json['data']), 1) def test_blender_version_filter_latest_not_max_version(self): - version = create_approved_version(blender_version_min='4.0.1') - version.date_created + version = create_approved_version(metadata__blender_version_min='4.0.1') + date_created = version.date_created extension = version.extension - create_approved_version( - blender_version_min='4.2.1', + version = create_approved_version( extension=extension, - date_created=version.date_created + timedelta(days=1), - version='2.0.0', + metadata__blender_version_min='4.2.1', + metadata__version='2.0.0', ) + version.date_created = date_created + timedelta(days=1) + version.save(update_fields={'date_created'}) create_approved_version( - blender_version_min='3.0.0', extension=extension, - date_created=version.date_created + timedelta(days=2), - version='1.0.1', + metadata__blender_version_min='3.0.0', + metadata__version='1.0.1', ) + version.date_created = date_created + timedelta(days=2) + version.save(update_fields={'date_created'}) create_approved_version( - blender_version_min='4.2.1', extension=extension, - date_created=version.date_created + timedelta(days=3), - version='2.0.1', + metadata__blender_version_min='4.2.1', + metadata__version='2.0.1', ) + version.date_created = date_created + timedelta(days=3) + version.save(update_fields={'date_created'}) url = reverse('extensions:api') json = self.client.get( @@ -190,9 +193,9 @@ class VersionUploadAPITest(APITestCase): self.client = APIClient() self.version = create_approved_version( - extension__extension_id="amaranth", - version="1.0.7", - file__user=self.user, + metadata__id="amaranth", + metadata__version="1.0.7", + user=self.user, ) self.extension = self.version.extension self.file_path = TEST_FILES_DIR / "amaranth-1.0.8.zip" @@ -218,7 +221,7 @@ class VersionUploadAPITest(APITestCase): def test_version_upload_extension_not_maintained_by_user(self): other_user = UserFactory() other_extension = create_approved_version( - extension__extension_id='other_extension', file__user=other_user + metadata__id='other_extension', user=other_user ).extension with open(self.file_path, 'rb') as version_file: diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index 51e1670d..67353428 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -1,5 +1,4 @@ from pathlib import Path -import factory import json from django.contrib.admin.models import LogEntry, DELETION @@ -27,25 +26,23 @@ class DeleteTest(TestCase): original_name='extension_feature_image.png', source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png', ) - version = create_version( - file__status=files.models.File.STATUSES.AWAITING_REVIEW, - ratings=[], - extension__icon=FileFactory( - type=files.models.File.TYPES.IMAGE, - original_name='extension_icon_final.png', - source='images/8a/8a01102de8573d50bbc90033f55f232b7cacc4f1eb3e3c3d851615841d2956e1.png', - ), - extension__featured_image=reused_image, - extension__previews=[ - reused_image, - FileFactory( - type=files.models.File.TYPES.IMAGE, - original_name='extension_preview_001.png', - source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png', - ), - ], - ) + version = create_version(status=files.models.File.STATUSES.AWAITING_REVIEW) extension = version.extension + extension.featured_image = reused_image + extension.icon = FileFactory( + type=files.models.File.TYPES.IMAGE, + original_name='extension_icon_final.png', + source='images/8a/8a01102de8573d50bbc90033f55f232b7cacc4f1eb3e3c3d851615841d2956e1.png', + ) + extension.save(update_fields={'featured_image', 'icon'}) + extension.previews.add(reused_image) + extension.previews.add( + FileFactory( + type=files.models.File.TYPES.IMAGE, + original_name='extension_preview_001.png', + source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png', + ) + ) version_file = version.file icon = extension.icon featured_image = extension.featured_image @@ -152,7 +149,7 @@ class DeleteTest(TestCase): # TODO: check that files were deleted from storage (create a temp one prior to the check) def test_publicly_listed_extension_cannot_be_deleted(self): - version = create_approved_version(ratings=[]) + version = create_approved_version() self.assertTrue(version.is_listed) extension = version.extension self.assertTrue(extension.is_listed) @@ -169,12 +166,8 @@ class DeleteTest(TestCase): self.assertEqual(response.status_code, 403) def test_rated_extension_cannot_be_deleted(self): - version = create_version( - file__status=files.models.File.STATUSES.AWAITING_REVIEW, - ratings=factory.RelatedFactoryList( - RatingFactory, size=1, factory_related_name='version' - ), - ) + version = create_version(status=files.models.File.STATUSES.AWAITING_REVIEW) + RatingFactory(version=version) self.assertFalse(version.is_listed) extension = version.extension self.assertFalse(extension.is_listed) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index bb3177f0..e7a1b29d 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -1,10 +1,8 @@ from django.test import TestCase from django.urls import reverse_lazy from django.core.exceptions import ValidationError -import factory from common.tests.factories.extensions import create_approved_version -from common.tests.factories.files import FileFactory from common.tests.factories.users import UserFactory from constants.licenses import LICENSE_GPL3 @@ -56,20 +54,10 @@ class CreateFileTest(TestCase): def _create_valid_extension(self, extension_id): return create_approved_version( - extension__name='Blender Kitsu', - extension__extension_id=extension_id, - version='0.1.5-alpha+f52258de', - file=FileFactory( - type=File.TYPES.BPY, - status=File.STATUSES.APPROVED, - metadata=factory.Dict( - { - 'name': 'Blender Kitsu', - 'support': factory.Faker('url'), - 'website': factory.Faker('url'), - } - ), - ), + metadata__id=extension_id, + metadata__name='Blender Kitsu', + metadata__version='0.1.5-alpha+f52258de', + status=File.STATUSES.APPROVED, ) def _create_file_from_data(self, filename, file_data, user): @@ -258,20 +246,10 @@ class ValidateManifestTest(CreateFileTest): # Create another unrelated extension with the same target version we will use. create_approved_version( - extension__name='Another Extension', - extension__extension_id='another_extension', - version='0.1.6', - 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'), - } - ), - ), + metadata__id='another_extension', + metadata__name='Another Extension', + metadata__version='0.1.6', + status=File.STATUSES.APPROVED, ) # The same author is to send a new version to thte same extension @@ -696,9 +674,6 @@ class VersionPermissionsTest(CreateFileTest): ) self.assertEqual(response.status_code, 302) - # Although this creates the file, the version is not created until - # we click on Submit for Review. - # Check step 2: finalise new version and send to review url = response['Location'] response = self.client.post( diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index d3ab76ca..aab17672 100644 --- a/extensions/tests/test_models.py +++ b/extensions/tests/test_models.py @@ -6,7 +6,6 @@ from common.admin import get_admin_change_path from common.log_entries import entries_for from common.tests.factories.extensions import create_version from common.tests.factories.users import UserFactory -from extensions.models import Extension class ExtensionTest(TestCase): @@ -16,17 +15,9 @@ class ExtensionTest(TestCase): def setUp(self): super().setUp() self.extension = create_version( - file__size_bytes=123, - extension__description='Extension description', - extension__website='https://example.com/', - extension__name='Extension name', - extension__status=Extension.STATUSES.DRAFT, - extension__support='https://example.com/', - file__metadata={ - 'name': 'Extension name', - 'support': 'https://example.com/', - 'website': 'https://example.com/', - }, + metadata__name='Extension name', + metadata__support='https://example.com/', + metadata__website='https://example.com/', ).extension self.assertEqual(entries_for(self.extension).count(), 0) self.assertIsNone(self.extension.date_approved) @@ -47,7 +38,7 @@ class ExtensionTest(TestCase): 'new_state': {'status': 'Approved'}, 'object': '', 'old_state': { - 'description': 'Extension description', + 'description': '', 'website': 'https://example.com/', 'name': 'Extension name', 'status': 1, @@ -57,13 +48,6 @@ class ExtensionTest(TestCase): }, ) - def test_status_change_updates_date_creates_log_entry(self): - self.extension.approve() - - self.assertIsNotNone(self.extension.date_approved) - self.assertIsNotNone(self.extension.date_status_changed) - self._check_change_message() - def test_status_change_updates_date_creates_log_entry_with_update_fields(self): self.extension.approve() @@ -89,42 +73,16 @@ class VersionTest(TestCase): def setUp(self): super().setUp() self.version = create_version( - blender_version_min='2.83.1', - version='1.1.2', - extension__description='Extension description', - extension__website='https://example.com/', - extension__name='Extension name', - extension__status=Extension.STATUSES.DRAFT, - extension__support='https://example.com/', + metadata__blender_version_min='2.83.1', + metadata__name='Extension name', + metadata__support='https://example.com/', + metadata__version='1.1.2', + metadata__website='https://example.com/', ) self.assertEqual(entries_for(self.version).count(), 0) # FIXME remove when dropping Version.file field self.assertEqual(self.version.file, self.version.files.first()) - def _check_change_message(self): - entries = entries_for(self.version) - self.assertEqual(entries.count(), 1) - log_entry = entries.first() - change_message = json.loads(log_entry.change_message) - self.assertEqual(len(change_message), 1) - self.assertDictEqual( - change_message[0], - { - 'changed': { - 'fields': ['status'], - 'name': 'version', - 'new_state': {'status': 'Approved'}, - 'object': '', - 'old_state': { - 'blender_version_max': None, - 'blender_version_min': '4.2.0', - 'status': 1, - 'version': '1.1.2', - }, - } - }, - ) - def test_admin_change_view(self): path = get_admin_change_path(obj=self.version) self.assertEqual(path, '/admin/extensions/version/1/change/') @@ -142,27 +100,18 @@ class UpdateMetadataTest(TestCase): def setUp(self): super().setUp() self.first_version = create_version( - extension__description='Extension description', - extension__name='name', - extension__status=Extension.STATUSES.DRAFT, - extension__support='https://example.com/', - extension__website='https://example.com/', - file__metadata={ - 'name': 'name', - 'support': 'https://example.com/', - 'website': 'https://example.com/', - }, + metadata__name='name', + metadata__support='https://example.com/', + metadata__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', - }, + metadata__name='new name', + metadata__support='https://example.com/new', + metadata__website='https://example.com/new', ) self.extension.refresh_from_db() self.assertEqual(self.extension.name, 'new name') @@ -178,26 +127,16 @@ class UpdateMetadataTest(TestCase): 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', - }, + metadata__name='new name', + metadata__support='https://example.com/new', + metadata__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.DRAFT, - extension__support='https://example.com/', - extension__website='https://example.com/', - file__metadata={ - 'name': 'name', - 'support': 'https://example.com/', - 'website': 'https://example.com/', - }, + metadata__name='name', + metadata__support='https://example.com/', + metadata__website='https://example.com/', ) second_version.delete() diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 7ec75a6b..ad6185db 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -294,16 +294,7 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): hash=file_data['file_hash'], metadata=file_data['metadata'], ) - self.version = create_version( - file=self.file, - extension__name=file_data['metadata']['name'], - extension__slug=file_data['metadata']['id'].replace("_", "-"), - extension__website=None, - tagline=file_data['metadata']['tagline'], - version=file_data['metadata']['version'], - blender_version_min=file_data['metadata']['blender_version_min'], - schema_version=file_data['metadata']['schema_version'], - ) + self.version = create_version(file=self.file) def test_get_finalise_addon_redirects_if_anonymous(self): response = self.client.post(self.file.legacy_version.extension.get_draft_url(), {}) @@ -492,7 +483,7 @@ class NewVersionTest(TestCase): fixtures = ['licenses'] def setUp(self): - self.version = create_version(extension__extension_id='amaranth') + self.version = create_version(metadata__id='amaranth') self.extension = self.version.extension self.url = self.extension.get_new_version_url() @@ -541,6 +532,7 @@ class NewVersionTest(TestCase): def test_upload_new_file_and_finalise_new_version(self): self.client.force_login(self.version.file.user) + self.extension.approve() # Check step 1: upload a new file with open(TEST_FILES_DIR / 'amaranth-1.0.8.zip', 'rb') as fp: @@ -552,17 +544,22 @@ class NewVersionTest(TestCase): response['Location'], f'/add-ons/{self.extension.slug}/manage/versions/new/{file.pk}/', ) - self.assertEqual(self.extension.versions.count(), 1) - self.extension.approve() + # now a file upload creates a corresponding version object immediately + self.assertEqual(self.extension.versions.count(), 2) + new_version = self.extension.versions.order_by('date_created').last() + self.assertEqual(new_version.version, '1.0.8') + self.assertEqual(new_version.blender_version_min, '4.2.0') + self.assertEqual(new_version.schema_version, '1.0.0') + self.assertEqual(new_version.file.get_status_display(), 'Approved') + self.assertEqual(new_version.release_notes, '') self.assertEqual( ApprovalActivity.objects.filter( extension=self.extension, type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, ).count(), - 0, + 1, ) - # Check step 2: finalise new version and send to review url = response['Location'] response = self.client.post( url, @@ -572,28 +569,17 @@ class NewVersionTest(TestCase): ) self.assertEqual(response.status_code, 302) - self.assertEqual(self.extension.versions.count(), 2) self.assertEqual(response['Location'], f'/add-ons/{self.extension.slug}/manage/versions/') - new_version = self.extension.versions.order_by('date_created').last() - self.assertEqual(new_version.version, '1.0.8') - self.assertEqual(new_version.blender_version_min, '4.2.0') - self.assertEqual(new_version.schema_version, '1.0.0') + self.assertEqual(self.extension.versions.count(), 2) + new_version.refresh_from_db() self.assertEqual(new_version.release_notes, 'new version') - self.assertEqual(new_version.file.get_status_display(), 'Approved') - self.assertEqual( - ApprovalActivity.objects.filter( - extension=self.extension, - type=ApprovalActivity.ActivityType.UPLOADED_NEW_VERSION, - ).count(), - 1, - ) class DraftsWarningTest(TestCase): fixtures = ['licenses'] def test_page_contains_warning(self): - version = create_version(extension__extension_id='draft_warning') + version = create_version() extension = version.extension self.assertEqual(extension.status, Extension.STATUSES.DRAFT) self.client.force_login(extension.authors.all()[0]) diff --git a/extensions/tests/test_update.py b/extensions/tests/test_update.py index 55be4ead..1c01f1b5 100644 --- a/extensions/tests/test_update.py +++ b/extensions/tests/test_update.py @@ -293,7 +293,8 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): source='file/original_image_source.jpg', ) extension = create_approved_version().extension - another_extension = create_approved_version(extension__previews=[file]).extension + another_extension = create_approved_version().extension + another_extension.previews.add(file) images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() self.assertEqual(extension.previews.count(), 0) self.assertEqual(another_extension.previews.count(), 1) @@ -440,11 +441,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): ) def test_update_icon_changes_expected_file_fields(self): - extension = create_approved_version( - extension__icon=ImageFactory( - original_name='old_icon.png', - ), - ).extension + extension = create_approved_version().extension + extension.icon = ImageFactory(original_name='old_icon.png') + extension.save(update_fields={'icon'}) self._test_file_properties( extension.icon, content_type='image/png', @@ -516,7 +515,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): source='test_icon_0001.png', ) extension = create_approved_version().extension - another_extension = create_approved_version(extension__icon=file).extension + another_extension = create_approved_version().extension + another_extension.icon = file + another_extension.save(update_fields={'icon'}) images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() self.assertIsNone(extension.icon) self.assertEqual(another_extension.icon_id, file.pk) @@ -542,11 +543,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): self.assertEqual(file.user, old_user) def test_update_featured_image_changes_expected_file_fields(self): - extension = create_approved_version( - extension__featured_image=ImageFactory( - original_name='old_featured_image.png', - ), - ).extension + extension = create_approved_version().extension + extension.featured_image = ImageFactory(original_name='old_featured_image.png') + extension.save(update_fields={'featured_image'}) self._test_file_properties( extension.featured_image, content_type='image/png', @@ -582,8 +581,10 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): ) def test_convert_to_draft(self): - version = create_version(extension__status=Extension.STATUSES.AWAITING_REVIEW) + version = create_version() extension = version.extension + extension.status = Extension.STATUSES.AWAITING_REVIEW + extension.save(update_fields={'status'}) url = extension.get_manage_url() user = extension.authors.first() self.client.force_login(user) @@ -607,9 +608,8 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): self.assertEqual(response3['Location'], extension.get_draft_url()) def test_team_field_in_draft_form(self): - version = create_version( - extension__status=Extension.STATUSES.DRAFT, - ) + # default status is DRAFT + version = create_version() extension = version.extension author = extension.authors.first() self.client.force_login(author) @@ -670,10 +670,10 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): def test_team_field_in_update_form(self): """This test is a copy-paste of the one above, only status, url and form data differ.""" - version = create_version( - extension__status=Extension.STATUSES.APPROVED, - ) + version = create_version() extension = version.extension + extension.status = Extension.STATUSES.APPROVED + extension.save(update_fields={'status'}) author = extension.authors.first() self.client.force_login(author) diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index abd0609c..cd20b1d8 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -4,26 +4,20 @@ from django.urls import reverse from common.tests.factories.extensions import create_version, create_approved_version from common.tests.factories.teams import TeamFactory from common.tests.factories.users import UserFactory, create_moderator -from extensions.models import Extension from teams.models import TeamsUsers def _create_extension(): - return create_version( - version='1.3.4', - blender_version_min='2.93.1', - extension__name='Test Add-on', - extension__description='**Description in bold**', - extension__support='https://example.com/issues/', - extension__website='https://example.com/', - extension__status=Extension.STATUSES.DRAFT, - extension__average_score=2.5, - file__metadata={ - 'name': 'Test Add-on', - 'support': 'https://example.com/issues/', - 'website': 'https://example.com/', - }, + extension = create_version( + metadata__blender_version_min='2.93.1', + metadata__name='Test Add-on', + metadata__support='https://example.com/issues/', + metadata__version='1.3.4', + metadata__website='https://example.com/', ).extension + extension.description = '**Description in bold**' + extension.save(update_fields={'description'}) + return extension class _BaseTestCase(TestCase): @@ -59,7 +53,8 @@ class PublicViewsTest(_BaseTestCase): self.assertTemplateUsed(response, 'extensions/home.html') def test_no_one_can_view_extension_page_when_not_listed_404(self): - extension = create_version(extension__is_listed=False).extension + # not listed by default + extension = create_version().extension moderator = create_moderator() staff = UserFactory(is_staff=True) diff --git a/extensions/views/api.py b/extensions/views/api.py index 5dc4370f..4148e24f 100644 --- a/extensions/views/api.py +++ b/extensions/views/api.py @@ -10,7 +10,7 @@ from django.core.exceptions import ValidationError from django.db import transaction from common.compare import is_in_version_range, version -from extensions.models import Extension, Platform, Version +from extensions.models import Extension, Platform from extensions.utils import clean_json_dictionary_from_optional_fields from extensions.views.manage import NewVersionView from files.forms import FileFormSkipAgreed @@ -217,13 +217,10 @@ class UploadExtensionVersionView(APIView): file_instance.user = user file_instance.save() - # Create the version from the file - version = Version.objects.update_or_create( - extension=extension, + version = extension.create_version_from_file( file=file_instance, release_notes=release_notes, - **file_instance.parsed_version_fields, - )[0] + ) return Response( { diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 85247ce2..1d6ea40c 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -218,6 +218,12 @@ class NewVersionView( kwargs['extension'] = self.extension return kwargs + @transaction.atomic + def form_valid(self, form): + response = super().form_valid(form) + self.extension.create_version_from_file(self.object) + return response + def get_success_url(self): return reverse( 'extensions:new-version-finalise', @@ -235,17 +241,9 @@ class NewVersionFinalizeView(LoginRequiredMixin, OwnsFileMixin, CreateView): template_name = 'extensions/new_version_finalise.html' form_class = VersionForm - def _get_extension(self) -> 'Extension': - return get_object_or_404(Extension, slug=self.kwargs['slug']) - - def _get_version(self, extension) -> 'Version': - return Version.objects.update_or_create( - extension=extension, file=self.file, **self.file.parsed_version_fields - )[0] - def get_form_kwargs(self): form_kwargs = super().get_form_kwargs() - form_kwargs['instance'] = self._get_version(self.extension) + form_kwargs['instance'] = self.file.version.first() return form_kwargs def get_initial(self): @@ -256,7 +254,7 @@ class NewVersionFinalizeView(LoginRequiredMixin, OwnsFileMixin, CreateView): return initial def get_success_url(self): - return self.extension.get_manage_versions_url() + return self.object.extension.get_manage_versions_url() class UpdateVersionView(LoginRequiredMixin, UserPassesTestMixin, UpdateView): diff --git a/extensions/views/mixins.py b/extensions/views/mixins.py index 03fc6168..eeb62183 100644 --- a/extensions/views/mixins.py +++ b/extensions/views/mixins.py @@ -8,7 +8,6 @@ from files.models import File class OwnsFileMixin(UserPassesTestMixin): def dispatch(self, *args, **kwargs): self.file = get_object_or_404(File, pk=self.kwargs['pk']) - self.extension = self._get_extension() return super().dispatch(*args, **kwargs) def test_func(self) -> bool: diff --git a/extensions/views/submit.py b/extensions/views/submit.py index a1e22d58..17039a29 100644 --- a/extensions/views/submit.py +++ b/extensions/views/submit.py @@ -4,7 +4,7 @@ from django.contrib.auth.mixins import LoginRequiredMixin from django.db import transaction from django.views.generic.edit import CreateView -from extensions.models import Version, Extension +from extensions.models import Extension from files.forms import FileForm from files.models import File @@ -35,37 +35,9 @@ class UploadFileView(LoginRequiredMixin, CreateView): @transaction.atomic def form_valid(self, form): """Create an extension and a version already, associated with the user.""" - self.file = form.instance - - parsed_extension_fields = self.file.parsed_extension_fields - if parsed_extension_fields: - # Try to look up extension by the same author and file info - extension = ( - Extension.objects.authored_by(self.request.user) - .filter(type=self.file.type, **parsed_extension_fields) - .first() - ) - if extension: - logger.warning( - 'Found existing extension pk=%s for file pk=%s', - extension.pk, - self.file.pk, - ) - return False - - # Make sure an extension has a user associated to it from the beginning, otherwise - # it will prevent it from being re-uploaded and yet not show on My Extensions. - self.extension = Extension.objects.update_or_create( - type=self.file.type, **parsed_extension_fields - )[0] - self.extension.authors.add(self.request.user) - self.extension.save() - - # Need to save the form to be able to use the file to create the version. - self.object = self.file = form.save() - - Version.objects.update_or_create( - extension=self.extension, file=self.file, **self.file.parsed_version_fields - )[0] - + file = form.save() + # TODO understand if file.user can ever be different from self.request.user + # e.g. if reusing an already uploaded file? + self.extension = Extension.create_from_file(file, self.request.user) + self.extension.create_version_from_file(file) return super().form_valid(form) diff --git a/files/models.py b/files/models.py index e5f89808..e6a535e2 100644 --- a/files/models.py +++ b/files/models.py @@ -169,9 +169,11 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): extension_id = data.get('id') name = data.get('name', self.original_name) return { + 'extension_id': extension_id, 'name': name, 'slug': utils.slugify(extension_id), - 'extension_id': extension_id, + 'support': data.get('support'), + 'type': self.type, 'website': data.get('website'), } diff --git a/files/tests/test_models.py b/files/tests/test_models.py index 732f2ec9..1e3cf19d 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -22,6 +22,7 @@ class FileTest(TestCase): original_name='test.zip', hash='foobar', size_bytes=7149, + metadata={}, ) self.assertEqual(entries_for(self.file).count(), 0) self.assertIsNone(self.file.date_approved) diff --git a/files/tests/test_tasks.py b/files/tests/test_tasks.py index 0699b9eb..857dbbb6 100644 --- a/files/tests/test_tasks.py +++ b/files/tests/test_tasks.py @@ -4,7 +4,7 @@ import logging from django.test import TestCase, override_settings -from common.tests.factories.files import FileFactory, ImageFactory +from common.tests.factories.files import FileFactory, ImageFactory, VideoFactory from files.tasks import make_thumbnails import files.models @@ -96,9 +96,7 @@ class TasksTest(TestCase): @patch('files.utils.Image') @patch('files.utils.FFmpeg') def test_make_thumbnails_for_video(self, mock_ffmpeg, mock_image, mock_resize_image): - file = FileFactory( - hash='deadbeef', source='file/path.mp4', type=files.models.File.TYPES.VIDEO - ) + file = VideoFactory(hash='deadbeef') files.models.FileValidation.objects.create(file=file, is_ok=True, results={}) self.assertIsNone(file.thumbnail.name) self.assertEqual(file.metadata, {}) diff --git a/ratings/tests/test_views.py b/ratings/tests/test_views.py index 23404213..33130385 100644 --- a/ratings/tests/test_views.py +++ b/ratings/tests/test_views.py @@ -9,7 +9,7 @@ class RatingsViewTest(TestCase): fixtures = ['dev', 'licenses'] def test_get_anonymous(self): - version = create_approved_version(ratings=[]) + version = create_approved_version() [ RatingFactory( version=version, text='this rating is rejected', status=Rating.STATUSES.REJECTED @@ -40,7 +40,7 @@ class RatingsViewTest(TestCase): self.assertNotContains(response, 'this rating is deleted', html=True) def test_get_logged_in_can_see_own_unlisted_rating(self): - version = create_approved_version(ratings=[]) + version = create_approved_version() user = UserFactory() [ RatingFactory( @@ -73,7 +73,7 @@ class AddRatingViewTest(TestCase): fixtures = ['dev', 'licenses'] def test_get_anonymous_redirects_to_login(self): - version = create_approved_version(ratings=[]) + version = create_approved_version() url = version.extension.get_rate_url() response = self.client.get(url) @@ -82,7 +82,7 @@ class AddRatingViewTest(TestCase): self.assertTrue(response['Location'].startswith('/oauth/login')) def test_get_logged_in_as_maintainer_cant_rate(self): - version = create_approved_version(ratings=[]) + version = create_approved_version() url = version.extension.get_rate_url() self.client.force_login(version.extension.authors.first()) @@ -93,7 +93,7 @@ class AddRatingViewTest(TestCase): def test_get_logged_in_can_rate(self): user = UserFactory() - version = create_approved_version(ratings=[]) + version = create_approved_version() url = version.extension.get_rate_url() self.client.force_login(user) @@ -104,7 +104,7 @@ class AddRatingViewTest(TestCase): def test_post_logged_in_validation_errors(self): user = UserFactory() - version = create_approved_version(ratings=[]) + version = create_approved_version() url = version.extension.get_rate_url() self.client.force_login(user) @@ -143,7 +143,7 @@ class AddRatingViewTest(TestCase): def test_post_logged_in_adds_new_rating(self): user = UserFactory() - version = create_approved_version(ratings=[]) + version = create_approved_version() extension = version.extension self.assertEqual(Rating.objects.count(), 0) self.assertEqual(extension.ratings.count(), 0) @@ -165,7 +165,7 @@ class AddRatingViewTest(TestCase): self.assertEqual(rating.text, text) def test_reply(self): - version = create_approved_version(ratings=[]) + version = create_approved_version() rating = RatingFactory(version=version, text='some text', status=Rating.STATUSES.APPROVED) random_user = UserFactory() diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index 7edf2cd3..1fb91fca 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -10,7 +10,7 @@ class CommentsViewTest(TestCase): fixtures = ['licenses'] def setUp(self): - version = create_version(file__status=File.STATUSES.AWAITING_REVIEW) + version = create_version(status=File.STATUSES.AWAITING_REVIEW) self.default_version = version ApprovalActivity( type=ApprovalActivity.ActivityType.COMMENT, diff --git a/stats/management/commands/tests/test_write_stats.py b/stats/management/commands/tests/test_write_stats.py index 3e8ec980..0ddde1bc 100644 --- a/stats/management/commands/tests/test_write_stats.py +++ b/stats/management/commands/tests/test_write_stats.py @@ -14,9 +14,7 @@ class WriteStatsCommandTest(TestCase): def test_command_updates_extensions_view_counters(self): out = StringIO() - extension = create_approved_version( - extension__view_count=0, extension__download_count=0 - ).extension + extension = create_approved_version().extension ExtensionView.objects.bulk_create( [ ExtensionView(extension_id=extension.pk, ip_address='192.19.10.10'), @@ -44,9 +42,7 @@ class WriteStatsCommandTest(TestCase): def test_command_updates_extensions_download_counters(self): out = StringIO() - extension = create_approved_version( - extension__view_count=0, extension__download_count=0 - ).extension + extension = create_approved_version().extension ExtensionDownload.objects.bulk_create( [ ExtensionDownload(extension_id=extension.pk, ip_address='192.19.10.10'), @@ -74,9 +70,9 @@ class WriteStatsCommandTest(TestCase): def test_command_adds_extensions_view_counters(self): out = StringIO() - extension = create_approved_version( - extension__view_count=10, extension__download_count=0 - ).extension + extension = create_approved_version().extension + extension.view_count = 10 + extension.save(update_fields={'view_count'}) ExtensionView.objects.bulk_create( [ ExtensionView(extension_id=extension.pk, ip_address='192.19.10.10'), @@ -104,9 +100,9 @@ class WriteStatsCommandTest(TestCase): def test_command_adds_extensions_download_counters(self): out = StringIO() - extension = create_approved_version( - extension__view_count=0, extension__download_count=10 - ).extension + extension = create_approved_version().extension + extension.download_count = 10 + extension.save(update_fields={'download_count'}) ExtensionDownload.objects.bulk_create( [ ExtensionDownload(extension_id=extension.pk, ip_address='192.19.10.10'), @@ -134,9 +130,10 @@ class WriteStatsCommandTest(TestCase): def test_command_updates_extensions_both_download_and_view_counters(self): out = StringIO() - extension = create_approved_version( - extension__view_count=4, extension__download_count=5 - ).extension + extension = create_approved_version().extension + extension.download_count = 5 + extension.view_count = 4 + extension.save(update_fields={'download_count', 'view_count'}) ExtensionView.objects.bulk_create( [ ExtensionView(extension_id=extension.pk, ip_address='192.19.10.10'), @@ -174,10 +171,10 @@ class WriteStatsCommandTest(TestCase): def test_command_updates_extensions_both_download_and_view_counters_uses_last_seen_id(self): out = StringIO() initial_view_count, initial_download_count = 4, 5 - extension = create_approved_version( - extension__download_count=initial_download_count, - extension__view_count=initial_view_count, - ).extension + extension = create_approved_version().extension + extension.download_count = initial_download_count + extension.view_count = initial_view_count + extension.save(update_fields={'download_count', 'view_count'}) download_count_starts_here = ExtensionDownload( extension_id=extension.pk, ip_address='192.19.10.15' ) diff --git a/users/tests/test_tasks.py b/users/tests/test_tasks.py index 62e8e1d8..c923a99b 100644 --- a/users/tests/test_tasks.py +++ b/users/tests/test_tasks.py @@ -33,14 +33,12 @@ class TestTasks(TestCase): # Abuse reported by someone else ABOUT this account: self.report3 = AbuseReportFactory(user=user) self.report4 = AbuseReportFactory(user=user) - self.authored_unlisted_extension = create_version( - file__user=user, extension__is_listed=False - ).extension + self.authored_unlisted_extension = create_version(user=user).extension self.assertFalse(self.authored_unlisted_extension.is_listed) def create_account_data_that_cannot_be_deleted(self, user): """Create objects which prevent account deletion but allow anonymisation.""" - self.authored_listed_extension = create_approved_version(file__user=user).extension + self.authored_listed_extension = create_approved_version(user=user).extension self.assertTrue(self.authored_listed_extension.is_listed) def test_handle_deletion_request_anonymized(self): -- 2.30.2 From c6ba74c76da19e22356555fbc3e59cbe6a49e2f3 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 12:36:56 +0200 Subject: [PATCH 2/5] cleanup unused VersionFactory --- common/tests/factories/extensions.py | 65 +------------------ common/tests/factories/notifications.py | 1 + .../commands/tests/test_write_stats.py | 2 - 3 files changed, 2 insertions(+), 66 deletions(-) diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 897bbb82..99d06143 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -7,7 +7,7 @@ import factory import factory.fuzzy from common.tests.factories.files import FileFactory -from extensions.models import Extension, Version, Tag, Preview, Platform +from extensions.models import Extension, Version, Preview from ratings.models import Rating fake_markdown = Faker() @@ -57,69 +57,6 @@ class RatingFactory(DjangoModelFactory): extension = factory.LazyAttribute(lambda o: o.version.extension) -class VersionFactory(DjangoModelFactory): - class Meta: - model = Version - - extension = factory.SubFactory(ExtensionFactory) - version = factory.LazyAttribute( - lambda _: f'{random.randint(0, 9)}.{random.randint(0, 9)}.{random.randint(0, 9)}' - ) - blender_version_min = factory.fuzzy.FuzzyChoice( - {'2.83.1', '2.93.0', '2.93.8', '3.0.0', '3.2.1'} - ) - download_count = factory.Faker('random_int') - tagline = factory.Faker('bs') - - 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(0, 5), factory_related_name='version' - ) - - @factory.post_generation - def files(self, create, extracted, **kwargs): - if not create: - return - - if not extracted: - self.files.add(self.file) - return - - for file in extracted: - self.files.add(file) - - @factory.post_generation - def platforms(self, create, extracted, **kwargs): - if not create: - return - - if not extracted: - return - - platforms = Platform.objects.filter(slug__in=extracted) - self.platforms.add(*platforms) - - @factory.post_generation - def tags(self, create, extracted, **kwargs): - if not create: - return - - if not extracted: - return - - tags = Tag.objects.filter(name__in=extracted) - self.tags.add(*tags) - - def create_version(**kwargs) -> 'Version': extension = kwargs.pop('extension', None) file = kwargs.pop('file', None) diff --git a/common/tests/factories/notifications.py b/common/tests/factories/notifications.py index a7eb3339..f4d63ff8 100644 --- a/common/tests/factories/notifications.py +++ b/common/tests/factories/notifications.py @@ -58,6 +58,7 @@ def construct_fake_notifications() -> list['NotificationFactory']: ), Verb.DISMISSED_ABUSE_REPORT: None, Verb.RATED_EXTENSION: RatingFactory.build( + extension=fake_extension, text=fake.paragraph(nb_sentences=2), ), Verb.REPORTED_EXTENSION: None, # TODO: fake action_object diff --git a/stats/management/commands/tests/test_write_stats.py b/stats/management/commands/tests/test_write_stats.py index 0ddde1bc..a343a83f 100644 --- a/stats/management/commands/tests/test_write_stats.py +++ b/stats/management/commands/tests/test_write_stats.py @@ -6,8 +6,6 @@ from django.test import TestCase from common.tests.factories.extensions import create_approved_version from stats.models import ExtensionView, ExtensionDownload, ExtensionCountedStat -# TODO: tests for VersionFactory Version download_count - class WriteStatsCommandTest(TestCase): fixtures = ['dev', 'licenses'] -- 2.30.2 From 1df59f7a1043ed687f4bfa4c700409b5703eecbb Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 12:49:42 +0200 Subject: [PATCH 3/5] add a check that uploaded file belongs to the current user --- common/tests/factories/extensions.py | 2 +- extensions/models.py | 4 ++-- extensions/views/submit.py | 11 ++++++++--- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 99d06143..4dbe3d41 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -65,7 +65,7 @@ def create_version(**kwargs) -> 'Version': file = FileFactory(**kwargs) if not extension: - extension = Extension.create_from_file(file, file.user) + extension = Extension.create_from_file(file) return extension.create_version_from_file(file) diff --git a/extensions/models.py b/extensions/models.py index 1c0b1d4c..30cf670a 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -186,11 +186,11 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model): ordering = ['-average_score', '-date_created', 'name'] @classmethod - def create_from_file(cls, file: File, author: User) -> 'Extension': + def create_from_file(cls, file: File) -> 'Extension': extension = cls(**file.parsed_extension_fields) with transaction.atomic(): extension.save() - extension.authors.add(author) + extension.authors.add(file.user) return extension def create_version_from_file(self, file: File, release_notes='') -> 'Version': diff --git a/extensions/views/submit.py b/extensions/views/submit.py index 17039a29..e4a47dca 100644 --- a/extensions/views/submit.py +++ b/extensions/views/submit.py @@ -36,8 +36,13 @@ class UploadFileView(LoginRequiredMixin, CreateView): def form_valid(self, form): """Create an extension and a version already, associated with the user.""" file = form.save() - # TODO understand if file.user can ever be different from self.request.user - # e.g. if reusing an already uploaded file? - self.extension = Extension.create_from_file(file, self.request.user) + if file.user != self.request.user: + # this must never happen, but checking here to prevent misattributing the extension + # to a wrong user + raise Exception( + f'user {self.request.user} uploaded a file hash={file.hash} owned by {file.user}, ' + f'this must never happen' + ) + self.extension = Extension.create_from_file(file) self.extension.create_version_from_file(file) return super().form_valid(form) -- 2.30.2 From 77fa0f9b05b66920c8e4317f95b1b3f1b280d756 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 13:56:26 +0200 Subject: [PATCH 4/5] cleanups --- .../management/commands/generate_fake_data.py | 5 ++--- common/tests/factories/files.py | 18 ++++++++---------- extensions/views/manage.py | 2 ++ files/tests/test_models.py | 6 +++--- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/common/management/commands/generate_fake_data.py b/common/management/commands/generate_fake_data.py index 5a0bdd34..46235b33 100644 --- a/common/management/commands/generate_fake_data.py +++ b/common/management/commands/generate_fake_data.py @@ -92,7 +92,6 @@ class Command(BaseCommand): size_bytes=FILE_SOURCES["blender-kitsu"]["size"], source=FILE_SOURCES["blender-kitsu"]["file"], status=File.STATUSES.APPROVED, - type=File.TYPES.BPY, ), ) for preview in [ @@ -108,8 +107,8 @@ class Command(BaseCommand): for i in range(10): type = random.choice(Extension.TYPES)[0] version = create_approved_version( - status=File.STATUSES.APPROVED, metadata__tags=random.sample(tags[type], k=1), + status=File.STATUSES.APPROVED, type=type, ) for preview in [ @@ -126,9 +125,9 @@ class Command(BaseCommand): for i in range(5): type = random.choice(Extension.TYPES)[0] version = create_version( + metadata__tags=random.sample(tags[type], k=1), status=random.choice((File.STATUSES.DISABLED, File.STATUSES.DISABLED_BY_AUTHOR)), type=type, - metadata__tags=random.sample(tags[type], k=1), ) for i in range(random.randint(1, len(LICENSES))): version.licenses.add(LICENSES[i]) diff --git a/common/tests/factories/files.py b/common/tests/factories/files.py index 626e9526..7b6839d5 100644 --- a/common/tests/factories/files.py +++ b/common/tests/factories/files.py @@ -7,7 +7,7 @@ import factory.fuzzy from files.models import File -class FileMetadataFactory(factory.DictFactory): +class ManifestFactory(factory.DictFactory): name = factory.Faker('name') id = factory.Faker('slug') support = factory.Faker('url') @@ -28,29 +28,27 @@ class FileFactory(DjangoModelFactory): class Meta: model = File - original_name = factory.LazyAttribute(lambda x: x.source) - original_hash = factory.Faker('lexify', text='fakehash:??????????????????', letters='deadbeef') hash = factory.Faker('lexify', text='fakehash:??????????????????', letters='deadbeef') + metadata = factory.SubFactory(ManifestFactory) + original_hash = factory.Faker('lexify', text='fakehash:??????????????????', letters='deadbeef') + original_name = factory.LazyAttribute(lambda x: x.source) size_bytes = factory.Faker('random_int', min=1234) source = factory.Faker('file_name', extension='zip') type = File.TYPES.BPY - user = factory.SubFactory('common.tests.factories.users.UserFactory') - metadata = factory.SubFactory(FileMetadataFactory) - class ImageFactory(FileFactory): + metadata = {} original_name = factory.Faker('file_name', extension='png') + size_bytes = 1234 source = 'images/de/deadbeef.png' type = File.TYPES.IMAGE - size_bytes = 1234 - metadata = {} class VideoFactory(FileFactory): + metadata = {} original_name = factory.Faker('file_name', extension='mp4') + size_bytes = 12345678 source = 'images/be/beefcafe.mp4' type = File.TYPES.VIDEO - size_bytes = 12345678 - metadata = {} diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 408acfe1..5becacf9 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -244,6 +244,8 @@ class NewVersionFinalizeView(LoginRequiredMixin, OwnsFileMixin, CreateView): def get_form_kwargs(self): form_kwargs = super().get_form_kwargs() + # this lookup via VersionFiles ManyToManyManager returns the version that was created on + # the previous step by create_version_from_file form_kwargs['instance'] = self.file.version.first() return form_kwargs diff --git a/files/tests/test_models.py b/files/tests/test_models.py index 1e3cf19d..db33db91 100644 --- a/files/tests/test_models.py +++ b/files/tests/test_models.py @@ -18,11 +18,11 @@ class FileTest(TestCase): def setUp(self): super().setUp() self.file = FileFactory( - status=File.STATUSES.AWAITING_REVIEW, - original_name='test.zip', hash='foobar', - size_bytes=7149, metadata={}, + original_name='test.zip', + size_bytes=7149, + status=File.STATUSES.AWAITING_REVIEW, ) self.assertEqual(entries_for(self.file).count(), 0) self.assertIsNone(self.file.date_approved) -- 2.30.2 From dce7a40241569cc0e5a8e472261d3c1b9cb9755b Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 20 Jun 2024 14:33:19 +0200 Subject: [PATCH 5/5] remove the check, rely on FileForm behavior --- extensions/views/submit.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/extensions/views/submit.py b/extensions/views/submit.py index e4a47dca..954dae60 100644 --- a/extensions/views/submit.py +++ b/extensions/views/submit.py @@ -36,13 +36,6 @@ class UploadFileView(LoginRequiredMixin, CreateView): def form_valid(self, form): """Create an extension and a version already, associated with the user.""" file = form.save() - if file.user != self.request.user: - # this must never happen, but checking here to prevent misattributing the extension - # to a wrong user - raise Exception( - f'user {self.request.user} uploaded a file hash={file.hash} owned by {file.user}, ' - f'this must never happen' - ) self.extension = Extension.create_from_file(file) self.extension.create_version_from_file(file) return super().form_valid(form) -- 2.30.2