diff --git a/common/admin.py b/common/admin.py index e8ea2544..0423bcd3 100644 --- a/common/admin.py +++ b/common/admin.py @@ -65,10 +65,17 @@ def link_to(field_name, title=None): @admin.display(description=title, ordering=field_name) def _raw(obj): + target_obj = getattr(obj, field_name) + if isinstance(target_obj, models.Manager): + admin_urls = [] + for _obj in target_obj.all(): + if related_field_name: + _obj = getattr(_obj, related_field_name) + admin_urls.append(get_admin_change_url(_obj)) + return format_html('
'.join(admin_urls)) + if related_field_name: - target_obj = getattr(getattr(obj, field_name), related_field_name) - else: - target_obj = getattr(obj, field_name) + target_obj = getattr(target_obj, related_field_name) admin_url = get_admin_change_url(target_obj) return admin_url diff --git a/common/tests/factories/extensions.py b/common/tests/factories/extensions.py index 6ef2ed75..f4618437 100644 --- a/common/tests/factories/extensions.py +++ b/common/tests/factories/extensions.py @@ -108,10 +108,7 @@ class VersionFactory(DjangoModelFactory): def create_version(**kwargs) -> 'Version': version = VersionFactory(**kwargs) - file = version.file - file.extension = version.extension - file.save(update_fields={'extension'}) - file.extension.authors.add(version.file.user) + version.extension.authors.add(version.file.user) return version diff --git a/extensions/forms.py b/extensions/forms.py index 6bb7dd3f..f6aed00a 100644 --- a/extensions/forms.py +++ b/extensions/forms.py @@ -64,18 +64,16 @@ class AddPreviewFileForm(files.forms.BaseMediaFileForm): """Save Preview from the cleaned form data.""" instance = super().save(*args, **kwargs) - # Create extension preview and save caption to it - extensions.models.Preview.objects.create( + # Create extension preview and save caption to it, ignore duplicate records + extensions.models.Preview.objects.get_or_create( file=instance, - caption=self.cleaned_data['caption'], extension=self.extension, + defaults={'caption': self.cleaned_data['caption']}, ) return instance class AddPreviewModelFormSet(forms.BaseModelFormSet): - msg_duplicate_file = _('Please select another file instead of the duplicate') - def __init__(self, *args, **kwargs): self.request = kwargs.pop('request') self.extension = kwargs.pop('extension') @@ -89,14 +87,6 @@ class AddPreviewModelFormSet(forms.BaseModelFormSet): form_kwargs['extension'] = self.extension return form_kwargs - def get_unique_error_message(self, unique_check): - """Replace duplicate `original_hash`/`hash` message with a more meaningful one.""" - if len(unique_check) == 1: - field = unique_check[0] - if field in ('original_hash', 'hash'): - return self.msg_duplicate_file - return super().get_unique_error_message(unique_check) - AddPreviewFormSet = forms.modelformset_factory( files.models.File, @@ -116,7 +106,6 @@ class ExtensionUpdateForm(forms.ModelForm): 'An extension can be converted to draft only while it is Awating Review' ) msg_need_previews = _('Please add at least one preview.') - msg_duplicate_file = _('Please select another file instead of the duplicate.') class Meta: model = extensions.models.Extension @@ -198,16 +187,17 @@ class ExtensionUpdateForm(forms.ModelForm): self.featured_image_form, self.icon_form, ] - seen_hashes = set() + seen_hashes = {} + # Ignore duplicate files in the formsets: point all forms sharing the hash to the same + # file instance, when forms call .save() it'll be on the same instance. for f in new_file_forms: hash = f.instance.original_hash - if hash: - if hash in seen_hashes: - f.add_error('source', self.msg_duplicate_file) - is_valid_flags.append(False) - break - seen_hashes.add(hash) - + if not hash: + continue + if hash in seen_hashes: + f.instance = seen_hashes[hash] + else: + seen_hashes[hash] = f.instance return all(is_valid_flags) def clean_team(self): @@ -352,9 +342,9 @@ class IconForm(files.forms.BaseMediaFileForm): } expected_size_px = 256 - def clean_source(self): + def clean_source(self, *args, **kwargs): """Check image resolution.""" - source = self.cleaned_data.get('source') + source = super().clean_source(*args, **kwargs) if not source: return image = getattr(source, 'image', None) diff --git a/extensions/migrations/0034_alter_extension_featured_image_alter_extension_icon_and_more.py b/extensions/migrations/0034_alter_extension_featured_image_alter_extension_icon_and_more.py new file mode 100644 index 00000000..f509c282 --- /dev/null +++ b/extensions/migrations/0034_alter_extension_featured_image_alter_extension_icon_and_more.py @@ -0,0 +1,30 @@ +# Generated by Django 4.2.11 on 2024-06-04 08:54 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0010_remove_file_extension_alter_file_hash_and_more'), + ('extensions', '0033_extensions_fts_20240603_1918'), + ] + + operations = [ + migrations.AlterField( + model_name='extension', + name='featured_image', + field=models.ForeignKey(help_text='Shown by social networks when this extension is shared (used as `og:image` metadata field).Should have resolution of at least 1920 x 1080 and aspect ratio of 16:9.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='featured_image_of', to='files.file'), + ), + migrations.AlterField( + model_name='extension', + name='icon', + field=models.ForeignKey(help_text='A 256 x 256 PNG icon representing this extension.', null=True, on_delete=django.db.models.deletion.PROTECT, related_name='icon_of', to='files.file'), + ), + migrations.AlterField( + model_name='preview', + name='file', + field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to='files.file'), + ), + ] diff --git a/extensions/models.py b/extensions/models.py index 8e2b7968..8c604325 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -178,24 +178,24 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod related_name='latest_version_of', ) - featured_image = models.OneToOneField( + featured_image = models.ForeignKey( 'files.File', related_name='featured_image_of', null=True, blank=False, - on_delete=models.SET_NULL, + on_delete=models.PROTECT, help_text=( "Shown by social networks when this extension is shared" " (used as `og:image` metadata field)." "Should have resolution of at least 1920 x 1080 and aspect ratio of 16:9." ), ) - icon = models.OneToOneField( + icon = models.ForeignKey( 'files.File', related_name='icon_of', null=True, blank=False, - on_delete=models.SET_NULL, + on_delete=models.PROTECT, help_text="A 256 x 256 PNG icon representing this extension.", ) previews = FilterableManyToManyField('files.File', through='Preview', related_name='extensions') @@ -748,12 +748,14 @@ class Maintainer(CreatedModifiedMixin, models.Model): class Preview(CreatedModifiedMixin, RecordDeletionMixin, models.Model): extension = models.ForeignKey(Extension, on_delete=models.CASCADE) - file = models.OneToOneField('files.File', on_delete=models.CASCADE) + # Files can potentially be referenced by different extensions + file = models.ForeignKey('files.File', on_delete=models.PROTECT) caption = models.CharField(max_length=255, default='', null=False, blank=True) position = models.IntegerField(default=0) class Meta: ordering = ('position', 'date_created') + # We don't want to have duplicate previews on the same extension unique_together = [['extension', 'file']] @property diff --git a/extensions/signals.py b/extensions/signals.py index a25ac85b..cb94f041 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -5,7 +5,7 @@ from actstream.actions import follow, unfollow from django.contrib.auth import get_user_model from django.contrib.auth.models import Group from django.db import transaction -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 @@ -16,22 +16,6 @@ logger = logging.getLogger(__name__) User = get_user_model() -@receiver(pre_save, sender=extensions.models.Preview) -def _set_extension( - sender: object, instance: extensions.models.Preview, raw: bool, **kwargs: object -) -> None: - if raw: - return - - file = instance.file - if not file: - return - - if not file.extension_id: - file.extension_id = instance.extension_id - file.save(update_fields={'extension_id'}) - - @receiver(pre_delete, sender=extensions.models.Extension) @receiver(pre_delete, sender=extensions.models.Preview) @receiver(pre_delete, sender=extensions.models.Version) @@ -45,6 +29,20 @@ def _log_deletion( instance.record_deletion() +@receiver(post_delete, sender=extensions.models.Version) +def _delete_version_file( + sender: object, instance: extensions.models.Version, **kwargs: object +) -> None: + # **N.B.**: this isn't part of an overloaded `Version.delete()` method because + # that method isn't called when `Extension.delete()` cascades to deleting the versions: + # + # delete() method for an object is not necessarily called ... as a result of a cascading delete + # https://docs.djangoproject.com/en/4.2/topics/db/models/#overriding-predefined-model-methods + version_file = instance.file + logger.info('Deleting file pk=%s of Version pk=%s', version_file.pk, instance.pk) + version_file.delete() + + @receiver(pre_save, sender=extensions.models.Extension) def _record_changes( sender: object, diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index f19b2e6b..d6612bcf 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -21,23 +21,38 @@ class DeleteTest(TestCase): def test_unlisted_unrated_extension_can_be_deleted_by_author(self): self.maxDiff = None + reused_image = FileFactory( + type=files.models.File.TYPES.IMAGE, + 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', - ) + ), ], ) extension = version.extension version_file = version.file + icon = extension.icon + featured_image = extension.featured_image self.assertEqual(version_file.get_status_display(), 'Awaiting Review') self.assertEqual(extension.get_status_display(), 'Draft') self.assertFalse(extension.is_listed) self.assertEqual(extension.cannot_be_deleted_reasons, []) - preview_file = extension.previews.first() + preview_file = extension.previews.last() self.assertIsNotNone(preview_file) # Create some ApprovalActivity as well moderator = create_moderator() @@ -55,11 +70,11 @@ class DeleteTest(TestCase): repr, [ version_file, - preview_file, + file_validation, extension, approval_activity, - file_validation, - preview_file.preview, + preview_file.preview_set.first(), + reused_image.preview_set.first(), version, ], ) @@ -78,12 +93,16 @@ class DeleteTest(TestCase): version.refresh_from_db() with self.assertRaises(files.models.File.DoesNotExist): version_file.refresh_from_db() - with self.assertRaises(files.models.File.DoesNotExist): - preview_file.refresh_from_db() + # Preview files aren't deleted: they might be re-uploaded shortly and should be looked up by hash + preview_file.refresh_from_db() + icon.refresh_from_db() + featured_image.refresh_from_db() self.assertIsNone(extensions.models.Extension.objects.filter(pk=extension.pk).first()) self.assertIsNone(extensions.models.Version.objects.filter(pk=version.pk).first()) self.assertIsNone(files.models.File.objects.filter(pk=version_file.pk).first()) - self.assertIsNone(files.models.File.objects.filter(pk=preview_file.pk).first()) + self.assertIsNotNone(files.models.File.objects.filter(pk=preview_file.pk).first()) + self.assertIsNotNone(files.models.File.objects.filter(pk=icon.pk).first()) + self.assertIsNotNone(files.models.File.objects.filter(pk=featured_image.pk).first()) # Check that each of the deleted records was logged deletion_log_entries_q = LogEntry.objects.filter(action_flag=DELETION) diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index f48e718d..431a1e3d 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -1,5 +1,5 @@ from django.test import TestCase -from django.urls import reverse +from django.urls import reverse_lazy from django.core.exceptions import ValidationError import factory @@ -35,6 +35,8 @@ META_DATA = { class CreateFileTest(TestCase): + submit_url = reverse_lazy('extensions:submit') + def setUp(self): super().setUp() @@ -52,10 +54,6 @@ class CreateFileTest(TestCase): super().tearDown() shutil.rmtree(self.temp_directory) - @classmethod - def _get_submit_url(cls): - return reverse('extensions:submit') - def _create_valid_extension(self, extension_id): return create_approved_version( extension__name='Blender Kitsu', @@ -120,9 +118,7 @@ class ValidateManifestTest(CreateFileTest): bad_file = self._create_file_from_data("theme.zip", file_data, self.user) with open(bad_file, 'rb') as fp: - response = self.client.post( - self._get_submit_url(), {'source': fp, 'agreed_with_terms': True} - ) + response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True}) self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source')[0] @@ -140,9 +136,7 @@ class ValidateManifestTest(CreateFileTest): bad_file = self._create_file_from_data("theme.zip", file_data, self.user) with open(bad_file, 'rb') as fp: - response = self.client.post( - self._get_submit_url(), {'source': fp, 'agreed_with_terms': True} - ) + response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True}) self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source')[0] @@ -165,9 +159,7 @@ class ValidateManifestTest(CreateFileTest): extension_file = self._create_file_from_data("theme.zip", kitsu_1_5, self.user) with open(extension_file, 'rb') as fp: - response = self.client.post( - self._get_submit_url(), {'source': fp, 'agreed_with_terms': True} - ) + response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True}) self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source')[0] @@ -191,9 +183,7 @@ class ValidateManifestTest(CreateFileTest): extension_file = self._create_file_from_data("theme.zip", kitsu_1_5, self.user) with open(extension_file, 'rb') as fp: - response = self.client.post( - self._get_submit_url(), {'source': fp, 'agreed_with_terms': True} - ) + response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True}) self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source')[0] @@ -317,9 +307,7 @@ class ValidateManifestTest(CreateFileTest): bad_file = self._create_file_from_data("theme.zip", file_data, self.user) with open(bad_file, 'rb') as fp: - response = self.client.post( - self._get_submit_url(), {'source': fp, 'agreed_with_terms': True} - ) + response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True}) self.assertEqual(response.status_code, 200) error = response.context['form'].errors.get('source') @@ -338,13 +326,11 @@ class ValidateManifestTest(CreateFileTest): extension_file = self._create_file_from_data("theme.zip", file_data, self.user) with open(extension_file, 'rb') as fp: - response = self.client.post( - self._get_submit_url(), {'source': fp, 'agreed_with_terms': True} - ) + response = self.client.post(self.submit_url, {'source': fp, 'agreed_with_terms': True}) self.assertEqual(response.status_code, 302) file = File.objects.first() - extension = file.extension + extension = file.version.extension self.assertEqual(extension.slug, 'an-id') self.assertEqual(extension.name, 'Name. - With Extra spaces and other characters Ж') diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index e081b266..3cc6cbe1 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -137,8 +137,8 @@ class SubmitFileTest(TestCase): self.assertEqual(response.status_code, 302) self.assertEqual(File.objects.count(), 1) file = File.objects.first() - self.assertEqual(response['Location'], file.get_submit_url()) - extension = file.extension + self.assertEqual(response['Location'], file.version.extension.get_draft_url()) + extension = file.version.extension self.assertEqual(extension.slug, slug) self.assertEqual(extension.name, name) self.assertEqual(file.original_name, file_name) @@ -203,8 +203,7 @@ class SubmitFileTest(TestCase): self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) self.assertEqual(File.objects.count(), 1) file = File.objects.first() - self.assertIsNotNone(file.extension_id) - self.assertEqual(response['Location'], file.get_submit_url()) + self.assertEqual(response['Location'], file.version.extension.get_draft_url()) self.assertEqual(file.user, user) self.assertEqual(file.original_name, 'theme.zip') self.assertEqual(file.size_bytes, 5895) @@ -285,18 +284,19 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): ) def test_get_finalise_addon_redirects_if_anonymous(self): - response = self.client.post(self.file.get_submit_url(), {}) + response = self.client.post(self.file.version.extension.get_draft_url(), {}) self.assertEqual(response.status_code, 302) self.assertEqual( - response['Location'], f'/oauth/login?next=/add-ons/{self.file.extension.slug}/draft/' + response['Location'], + f'/oauth/login?next=/add-ons/{self.file.version.extension.slug}/draft/', ) def test_get_finalise_addon_not_allowed_if_different_user(self): user = UserFactory() self.client.force_login(user) - response = self.client.post(self.file.get_submit_url(), {}) + response = self.client.post(self.file.version.extension.get_draft_url(), {}) # Technically this could (should) be a 403, but changing this means changing # the MaintainedExtensionMixin which is used in multiple places. @@ -305,7 +305,7 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): def test_post_finalise_addon_validation_errors(self): self.client.force_login(self.file.user) data = {**POST_DATA, 'submit_draft': ''} - response = self.client.post(self.file.get_submit_url(), data) + response = self.client.post(self.file.version.extension.get_draft_url(), data) self.assertEqual(response.status_code, 200) self.assertDictEqual( @@ -371,7 +371,9 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): 'icon-source': fp3, 'featured-image-source': fp4, } - response = self.client.post(self.file.get_submit_url(), {**data, **files}) + response = self.client.post( + self.file.version.extension.get_draft_url(), {**data, **files} + ) self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) self.assertEqual(response['Location'], '/add-ons/edit-breakdown/manage/') diff --git a/extensions/tests/test_update.py b/extensions/tests/test_update.py index b5b15fc6..55be4ead 100644 --- a/extensions/tests/test_update.py +++ b/extensions/tests/test_update.py @@ -81,7 +81,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), 1) self.assertEqual(extension.previews.count(), 1) file1 = extension.previews.all()[0] - self.assertEqual(file1.preview.caption, 'First Preview Caption Text') + self.assertEqual(file1.preview_set.first().caption, 'First Preview Caption Text') self.assertEqual( file1.original_hash, 'sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340', @@ -125,7 +125,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): extension.refresh_from_db() self.assertEqual(extension.previews.count(), 1) video_file = extension.previews.all()[0] - self.assertEqual(video_file.preview.caption, 'First Preview Caption Text') + self.assertEqual(video_file.preview_set.first().caption, 'First Preview Caption Text') self._test_file_properties( video_file, content_type='video/mp4', @@ -169,8 +169,8 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): self.assertEqual(extension.previews.count(), 2) file1 = extension.previews.all()[0] file2 = extension.previews.all()[1] - self.assertEqual(file1.preview.caption, 'First Preview Caption Text') - self.assertEqual(file2.preview.caption, 'Second Preview Caption Text') + self.assertEqual(file1.preview_set.first().caption, 'First Preview Caption Text') + self.assertEqual(file2.preview_set.first().caption, 'Second Preview Caption Text') self.assertEqual( file1.original_hash, 'sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340', @@ -222,8 +222,10 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): _get_all_form_errors(response), ) - def test_post_upload_validation_error_duplicate_images(self): + def test_post_upload_duplicate_preview_files_ignored(self): extension = create_approved_version().extension + images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() + self.assertEqual(extension.previews.count(), 0) data = { **POST_DATA, @@ -243,31 +245,24 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): } response = self.client.post(url, {**data, **files}) - self.assertEqual(response.status_code, 200) - self.maxDiff = None + self.assertEqual(response.status_code, 302) + # One image was uploaded successfully self.assertEqual( - [ - response.context['add_preview_formset'].forms[0].errors, - response.context['add_preview_formset'].forms[1].errors, - response.context['add_preview_formset'].non_form_errors(), - ], - [ - {}, - { - '__all__': ['Please correct the duplicate values below.'], - 'source': ['Please select another file instead of the duplicate.'], - }, - ['Please select another file instead of the duplicate'], - ], + File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before + 1 ) + self.assertEqual(extension.previews.count(), 1) - def test_post_upload_validation_error_image_already_exists(self): - FileFactory( + def test_post_upload_existing_image_file_linked_to_extension(self): + file = FileFactory( + type=File.TYPES.IMAGE, original_hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340', hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340', source='file/original_image_source.jpg', ) extension = create_approved_version().extension + images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() + self.assertEqual(extension.previews.count(), 0) + old_user = file.user data = { **POST_DATA, @@ -281,15 +276,56 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): files = {'form-0-source': fp1} response = self.client.post(url, {**data, **files}) - self.assertEqual(response.status_code, 200) - self.maxDiff = None - self.assertEqual( - response.context['add_preview_formset'].forms[0].errors, - {'source': ['File with this Original hash already exists.']}, - ) + self.assertEqual(response.status_code, 302) + # No new files were created: the existing one was linked to the extension instead + self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before) + self.assertEqual(extension.previews.count(), 1) + self.assertEqual(extension.previews.first().original_hash, file.original_hash) + self.assertEqual(extension.previews.first().pk, file.pk) + file.refresh_from_db() + self.assertEqual(file.user, old_user) - def test_post_upload_validation_error_duplicate_across_forms(self): + def test_post_upload_existing_preview_file_linked_to_extension(self): + file = FileFactory( + type=File.TYPES.IMAGE, + original_hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340', + hash='sha256:643e15eb6c4831173bbcf71b8c85efc70cf3437321bf2559b39aa5e9acfd5340', + source='file/original_image_source.jpg', + ) extension = create_approved_version().extension + another_extension = create_approved_version(extension__previews=[file]).extension + images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() + self.assertEqual(extension.previews.count(), 0) + self.assertEqual(another_extension.previews.count(), 1) + old_user = file.user + + data = { + **POST_DATA, + 'form-TOTAL_FORMS': ['1'], + } + file_name1 = 'test_preview_image_0001.png' + url = extension.get_manage_url() + user = extension.authors.first() + self.client.force_login(user) + with open(TEST_FILES_DIR / file_name1, 'rb') as fp1: + files = {'form-0-source': fp1} + response = self.client.post(url, {**data, **files}) + + self.assertEqual(response.status_code, 302) + # No new files were created: the existing one was linked to the extension instead + self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before) + self.assertEqual(extension.previews.count(), 1) + self.assertEqual(extension.previews.first().original_hash, file.original_hash) + self.assertEqual(extension.previews.first().pk, file.pk) + # File is referenced as a preview by both extensions + self.assertEqual(file.preview_set.count(), 2) + file.refresh_from_db() + self.assertEqual(file.user, old_user) + + def test_post_upload_duplicates_are_ignored_across_forms(self): + extension = create_approved_version().extension + images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() + self.assertEqual(extension.previews.count(), 0) data = { **POST_DATA, @@ -304,12 +340,12 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): files = {'form-0-source': fp, 'icon-source': fp1} response = self.client.post(url, {**data, **files}) - self.assertEqual(response.status_code, 200) - self.maxDiff = None + self.assertEqual(response.status_code, 302) + # One image was uploaded successfully self.assertEqual( - response.context['icon_form'].errors, - {'source': ['Please select another file instead of the duplicate.']}, + File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before + 1 ) + self.assertEqual(extension.previews.count(), 1) def test_post_upload_validation_error_unexpected_preview_format_gif(self): extension = create_approved_version().extension @@ -419,6 +455,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): original_name='old_icon.png', size_bytes=1234, ) + old_icon = extension.icon self.client.force_login(extension.authors.first()) url = extension.get_manage_url() @@ -427,7 +464,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): response = self.client.post(url, {**POST_DATA, **files}) self.assertEqual(response.status_code, 302) - extension.icon.refresh_from_db() + old_icon.refresh_from_db() + extension.refresh_from_db() + self.assertNotEqual(extension.icon_id, old_icon.pk) self._test_file_properties( extension.icon, content_type='image/png', @@ -440,6 +479,68 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): size_bytes=30177, ) + def test_update_icon_existing_file_linked(self): + file = FileFactory( + type=File.TYPES.IMAGE, + original_hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472', + hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472', + source='test_icon_0001.png', + ) + extension = create_approved_version().extension + images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() + self.assertIsNone(extension.icon) + self.assertEqual(file.icon_of.count(), 0) + old_user = file.user + + url = extension.get_manage_url() + user = extension.authors.first() + self.client.force_login(user) + with open(TEST_FILES_DIR / 'test_icon_0001.png', 'rb') as fp: + files = {'icon-source': fp} + response = self.client.post(url, {**POST_DATA, **files}) + + self.assertEqual(response.status_code, 302) + # No new files were created: the existing one was linked to the extension instead + self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before) + extension.refresh_from_db() + self.assertEqual(extension.icon, file) + self.assertEqual(file.icon_of.count(), 1) + file.refresh_from_db() + self.assertEqual(file.user, old_user) + + def test_update_icon_existing_file_linked_to_multiple_extensions(self): + file = FileFactory( + type=File.TYPES.IMAGE, + original_hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472', + hash='sha256:ee3a015c51e35a237755713ec578334efa9ed8870af65b708f591f9254ff4472', + source='test_icon_0001.png', + ) + extension = create_approved_version().extension + another_extension = create_approved_version(extension__icon=file).extension + images_count_before = File.objects.filter(type=File.TYPES.IMAGE).count() + self.assertIsNone(extension.icon) + self.assertEqual(another_extension.icon_id, file.pk) + old_user = file.user + + url = extension.get_manage_url() + user = extension.authors.first() + self.client.force_login(user) + with open(TEST_FILES_DIR / 'test_icon_0001.png', 'rb') as fp: + files = {'icon-source': fp} + response = self.client.post(url, {**POST_DATA, **files}) + + self.assertEqual(response.status_code, 302) + # No new files were created: the existing one was linked to the extension instead + self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), images_count_before) + extension.refresh_from_db() + another_extension.refresh_from_db() + self.assertEqual(extension.icon, file) + self.assertEqual(another_extension.icon, file) + # File is referenced as a preview by both extensions + self.assertEqual(file.icon_of.count(), 2) + file.refresh_from_db() + self.assertEqual(file.user, old_user) + def test_update_featured_image_changes_expected_file_fields(self): extension = create_approved_version( extension__featured_image=ImageFactory( @@ -456,6 +557,7 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): original_name='old_featured_image.png', size_bytes=1234, ) + old_featured_image = extension.featured_image self.client.force_login(extension.authors.first()) url = extension.get_manage_url() @@ -464,7 +566,9 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): response = self.client.post(url, {**POST_DATA, **files}) self.assertEqual(response.status_code, 302) - extension.featured_image.refresh_from_db() + old_featured_image.refresh_from_db() + extension.refresh_from_db() + self.assertNotEqual(extension.featured_image_id, old_featured_image.pk) self._test_file_properties( extension.featured_image, content_type='image/png', diff --git a/extensions/views/submit.py b/extensions/views/submit.py index 648c0f10..a1e22d58 100644 --- a/extensions/views/submit.py +++ b/extensions/views/submit.py @@ -64,7 +64,6 @@ class UploadFileView(LoginRequiredMixin, CreateView): # Need to save the form to be able to use the file to create the version. self.object = self.file = form.save() - self.file.extension = self.extension Version.objects.update_or_create( extension=self.extension, file=self.file, **self.file.parsed_version_fields )[0] diff --git a/files/admin.py b/files/admin.py index 8f34264f..6914ea74 100644 --- a/files/admin.py +++ b/files/admin.py @@ -41,6 +41,10 @@ class FileAdmin(admin.ModelAdmin): class Media: css = {'all': ('files/admin/file.css',)} + def get_queryset(self, *args, **kwargs): + q = super().get_queryset(*args, **kwargs) + return q.prefetch_related(*self.list_prefetch_related) + def thumbnails(self, obj): if not obj or not (obj.is_image or obj.is_video): return '' @@ -72,7 +76,6 @@ class FileAdmin(admin.ModelAdmin): 'date_created', 'date_modified', 'date_status_changed', - ('extension', admin.EmptyFieldListFilter), ('version', admin.EmptyFieldListFilter), ('icon_of', admin.EmptyFieldListFilter), ('featured_image_of', admin.EmptyFieldListFilter), @@ -80,7 +83,6 @@ class FileAdmin(admin.ModelAdmin): ) list_display = ( 'original_name', - link_to('extension'), link_to('user'), 'date_created', 'type', @@ -88,19 +90,20 @@ class FileAdmin(admin.ModelAdmin): link_to('version'), link_to('icon_of'), link_to('featured_image_of'), - link_to('preview.extension', 'preview of'), + link_to('preview_set.extension', 'preview of'), 'is_ok', ) list_select_related = ( 'version__extension', 'user', - 'extension', 'version', 'validation', + ) + list_prefetch_related = ( 'icon_of', 'featured_image_of', - 'preview__extension', + 'preview_set__extension', ) autocomplete_fields = ['user'] diff --git a/files/forms.py b/files/forms.py index ae1be39d..c49c9fdd 100644 --- a/files/forms.py +++ b/files/forms.py @@ -6,7 +6,6 @@ import tempfile from django import forms from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ -import django.core.exceptions from .validators import ( ExtensionIDManifestValidator, @@ -44,7 +43,7 @@ class FileForm(forms.ModelForm): class Meta: model = files.models.File - fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user', 'extension') + fields = ('source', 'type', 'metadata', 'agreed_with_terms', 'user') source = forms.FileField( allow_empty_file=False, @@ -123,7 +122,6 @@ class FileForm(forms.ModelForm): 'size_bytes': source.size, 'original_hash': hash_, 'hash': hash_, - 'extension': self.extension, } ) @@ -180,8 +178,7 @@ class FileFormSkipAgreed(FileForm): class BaseMediaFileForm(forms.ModelForm): class Meta: model = files.models.File - fields = ('source', 'original_hash') - widgets = {'original_hash': forms.HiddenInput()} + fields = ('source',) source = forms.ImageField(widget=forms.FileInput, allow_empty_file=False) @@ -210,22 +207,23 @@ class BaseMediaFileForm(forms.ModelForm): super().__init__(*args, **kwargs) - self.instance.user = self.request.user - - def clean_original_hash(self, *args, **kwargs): - """Calculate original hash of the uploaded file.""" + def clean_source(self, *args, **kwargs): + """Calculate original hash of the uploaded file, reuse existing record matching it.""" source = self.cleaned_data.get('source') if 'source' in self.changed_data and source: - return files.models.File.generate_hash(source) - - def add_error(self, field, error): - """Add hidden `original_hash` errors to the visible `source` field instead.""" - if isinstance(error, django.core.exceptions.ValidationError): - if getattr(error, 'error_dict', None): - hash_error = error.error_dict.pop('original_hash', None) - if hash_error: - error.error_dict['source'] = hash_error - super(forms.ModelForm, self).add_error(field, error) + original_hash = files.models.File.generate_hash(source) + instance = files.models.File.objects.filter(original_hash=original_hash).first() + if instance: + # File with this hash exists already, make sure it's reused here + if instance.pk != self.instance.pk: + self.instance = instance + else: + previous_hash = self.instance.hash + if previous_hash and original_hash != previous_hash and self.instance.pk: + # Create a new file instead of changing the existing one + self.instance.pk = None + self.instance.original_hash = original_hash + return source def save(self, *args, **kwargs): """Save as `to_field` on the parent object (Extension).""" @@ -235,7 +233,9 @@ class BaseMediaFileForm(forms.ModelForm): self.instance.original_name = source.name self.instance.size_bytes = source.size - self.instance.extension = self.extension + if not self.instance.user_id: + self.instance.user = self.request.user + instance = super().save(*args, **kwargs) if hasattr(self, 'to_field'): diff --git a/files/migrations/0010_remove_file_extension_alter_file_hash_and_more.py b/files/migrations/0010_remove_file_extension_alter_file_hash_and_more.py new file mode 100644 index 00000000..4881544f --- /dev/null +++ b/files/migrations/0010_remove_file_extension_alter_file_hash_and_more.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.11 on 2024-06-03 14:16 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('files', '0009_file_extension'), + ] + + operations = [ + migrations.RemoveField( + model_name='file', + name='extension', + ), + migrations.AlterField( + model_name='file', + name='hash', + field=models.CharField(blank=True, editable=False, max_length=255, unique=True), + ), + migrations.AlterField( + model_name='file', + name='original_hash', + field=models.CharField(blank=True, editable=False, help_text='The original hash of the file before we repackage it any way.', max_length=255, unique=True), + ), + ] diff --git a/files/models.py b/files/models.py index eaa60699..0878a353 100644 --- a/files/models.py +++ b/files/models.py @@ -49,12 +49,6 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): date_approved = models.DateTimeField(null=True, blank=True, editable=False) date_status_changed = models.DateTimeField(null=True, blank=True, editable=False) - extension = models.ForeignKey( - 'extensions.Extension', - null=True, - blank=True, - on_delete=models.CASCADE, - ) source = models.FileField(null=False, blank=False, upload_to=file_upload_to) thumbnail = models.ImageField( @@ -82,12 +76,13 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): User, related_name='files', null=False, blank=False, on_delete=models.CASCADE ) size_bytes = models.PositiveBigIntegerField(default=0, editable=False) - hash = models.CharField(max_length=255, null=False, blank=True, unique=True) + hash = models.CharField(max_length=255, null=False, blank=True, unique=True, editable=False) original_name = models.CharField(max_length=255, blank=True, null=False) original_hash = models.CharField( max_length=255, null=False, blank=True, + editable=False, unique=True, help_text='The original hash of the file before we repackage it any way.', ) @@ -196,9 +191,6 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): 'tags': data.get('tags'), } - def get_submit_url(self) -> str: - return self.extension.get_draft_url() - def get_thumbnail_of_size(self, size_key: str) -> str: """Return absolute path portion of the URL of a thumbnail of this file. diff --git a/notifications/tests/test_follow_logic.py b/notifications/tests/test_follow_logic.py index 7b2ed498..5028caf9 100644 --- a/notifications/tests/test_follow_logic.py +++ b/notifications/tests/test_follow_logic.py @@ -159,7 +159,7 @@ class TestTasks(TestCase): 'form-0-source': fp1, 'form-1-source': fp2, } - self.client.post(file.get_submit_url(), {**data, **files}) + self.client.post(file.version.extension.get_draft_url(), {**data, **files}) new_notification_nr = Notification.objects.filter(recipient=moderator).count() self.assertEqual(new_notification_nr, notification_nr + 1) diff --git a/public/media/images/8a/8a01102de8573d50bbc90033f55f232b7cacc4f1eb3e3c3d851615841d2956e1.png b/public/media/images/8a/8a01102de8573d50bbc90033f55f232b7cacc4f1eb3e3c3d851615841d2956e1.png new file mode 100644 index 00000000..4d312310 Binary files /dev/null and b/public/media/images/8a/8a01102de8573d50bbc90033f55f232b7cacc4f1eb3e3c3d851615841d2956e1.png differ