diff --git a/common/static/common/scripts/app.js b/common/static/common/scripts/app.js index e54772e0..67d8865c 100644 --- a/common/static/common/scripts/app.js +++ b/common/static/common/scripts/app.js @@ -95,20 +95,19 @@ // Create function copyInstallUrl function copyInstallUrl() { function init() { - // Create variables + // Create variables single const btnInstall = document.querySelector('.js-btn-install'); const btnInstallAction = document.querySelector('.js-btn-install-action'); const btnInstallGroup = document.querySelector('.js-btn-install-group'); - const btnInstallDrag = document.querySelector('.js-btn-install-drag'); - const btnInstallDragGroup = document.querySelector('.js-btn-install-drag-group'); + + // Create variables multiple + const btnInstallActionItem = document.querySelectorAll('.js-btn-install-action-item'); if (btnInstall == null) { return; } - // Get data install URL - const btnInstallUrl = btnInstall.getAttribute('data-install-url'); - + // Show btnInstallAction btnInstall.addEventListener('click', function() { // Hide btnInstallGroup btnInstallGroup.classList.add('d-none'); @@ -117,19 +116,28 @@ btnInstallAction.classList.add('show'); }); - // Drag btnInstallUrl - btnInstallDrag.addEventListener('dragstart', function(e) { - // Set data install URL to be transferred during drag - e.dataTransfer.setData('text/plain', btnInstallUrl); + btnInstallActionItem.forEach(function(item) { + // Create variables in function scope + const btnInstallDrag = item.querySelector('.js-btn-install-drag'); + const btnInstallDragGroup = item.querySelectorAll('.js-btn-install-drag-group'); - // Set drag area active - btnInstallDragGroup.classList.add('opacity-50'); - }); + // Get data install URL + const btnInstallUrl = item.getAttribute('data-install-url'); - // Undrag btnInstallUrl - btnInstallDrag.addEventListener('dragend', function() { - // Set drag area inactive - btnInstallDragGroup.classList.remove('opacity-50'); + // Drag btnInstallUrl + btnInstallDrag.addEventListener('dragstart', function(e) { + // Set data install URL to be transferred during drag + e.dataTransfer.setData('text/plain', btnInstallUrl); + + // Set drag area active + btnInstallDragGroup.classList.add('opacity-50'); + }); + + // Undrag btnInstallUrl + btnInstallDrag.addEventListener('dragend', function() { + // Set drag area inactive + btnInstallDragGroup.classList.remove('opacity-50'); + }); }); } diff --git a/common/static/common/styles/_extension.sass b/common/static/common/styles/_extension.sass index 33127a35..07826cca 100644 --- a/common/static/common/styles/_extension.sass +++ b/common/static/common/styles/_extension.sass @@ -164,6 +164,12 @@ .btn-accent box-shadow: .2rem .8rem .8rem rgba(0,82,255,.102), 1.6rem 1.0rem 1.8rem rgba(0,82,255,.102), 3.2rem 1.0rem 3.2rem rgba(0,82,255,.349) + .btn-install-action-item + +margin(3, bottom) + + &:last-child + +margin(0, bottom) + .btn-install-drag, .btn-install-drag:active transform: initial diff --git a/extensions/models.py b/extensions/models.py index 0c02b5c8..b91390cd 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -7,7 +7,6 @@ from django.core.exceptions import ObjectDoesNotExist, BadRequest from django.db import models, transaction from django.db.models import Q, Count from django.urls import reverse -from django.utils.functional import cached_property from common.fields import FilterableManyToManyField from common.model_mixins import CreatedModifiedMixin, RecordDeletionMixin, TrackChangesMixin @@ -253,7 +252,9 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model): def update_metadata_from_version(self, version): update_fields = set() - metadata = version.file.metadata + files = list(version.files.all()) + # picking the last uploaded file + metadata = files[-1].metadata # check if we can also update name # if we are uploading a new version, we have just validated and don't expect a clash, # but if a version is being deleted, we want to rollback to a name from an older version, @@ -276,9 +277,9 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model): # TODO: check that latest version is currently in review (whatever that means in data?) latest_version = self.latest_version - file = latest_version.file - file.status = FILE_STATUS_CHOICES.APPROVED - file.save() + for file in latest_version.files.all(): + file.status = FILE_STATUS_CHOICES.APPROVED + file.save() # TODO: check that this extension is currently in review (whatever that means in data?) self.previews.update(status=FILE_STATUS_CHOICES.APPROVED) @@ -422,8 +423,12 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model): ) def suspicious_files(self): - versions = self.versions.all() - return [v.file for v in versions if not v.file.validation.is_ok] + files = [] + for version in self.versions.all(): + for file in version.files.all(): + if not file.validation.is_ok: + files.append(file) + return files @classmethod def get_lookup_field(cls, identifier): @@ -439,7 +444,8 @@ class Extension(CreatedModifiedMixin, TrackChangesMixin, models.Model): for version in versions: if skip_version and version == skip_version: continue - if version.file.status not in self.valid_file_statuses: + files = version.files.all() + if not any([file.status in self.valid_file_statuses for file in files]): continue latest_version = version break @@ -667,25 +673,71 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): def __str__(self) -> str: return f'{self.extension} v{self.version}' - # TODO get rid of this once all places are aware of multi-file reality - # - # the fact that it is cached is important, this allows to use version.file as if it still was a - # model field, i.e. write things like - # version.file.status = ... - # version.file.save() - # if it isn't cached, then the save call is applied to a different, newly fetched File instance - @cached_property - def file(self): - files = list(self.files.all()) - if files: - return files[0] - else: - return None + @property + def has_single_file(self): + return len(self.files.all()) == 1 + + def add_file(self, file: File): + current_platforms = set([p.slug for p in self.platforms.all()]) + if not current_platforms: + raise ValueError( + f'add_file failed: Version pk={self.pk} is not platform-specific, ' + f'no more files allowed' + ) + file_platforms = set(file.get_platforms() or []) + if overlap := current_platforms & file_platforms: + raise ValueError( + f'add_file failed: File pk={file.pk} and Version pk={self.pk} have ' + f'overlapping platforms: {overlap}' + ) + self.files.add(file) + self.update_platforms() + + @transaction.atomic + def update_platforms(self): + current_platforms = set([p.slug for p in self.platforms.all()]) + files_platforms = set() + for file in self.files.all(): + if file_platforms := file.get_platforms(): + files_platforms.update(file_platforms) + else: + # we have a cross-platform file - version must not specify platforms + # other files should not exist (validation takes care of that), + # but here we won't double-check it (?) + if len(current_platforms): + self.platforms.delete() + return + if to_create := files_platforms - current_platforms: + for p in to_create: + self.platforms.add(Platform.objects.get(slug=p)) + if to_delete := current_platforms - files_platforms: + for p in to_delete: + self.platforms.remove(Platform.objects.get(slug=p)) + + def get_remaining_platforms(self): + all_platforms = set(p.slug for p in Platform.objects.all()) + for file in self.files.all(): + platforms = file.get_platforms() + if not platforms: + # no platforms means any platform + return set() + all_platforms -= set(platforms) + return all_platforms + + def get_file_for_platform(self, platform): + for file in self.files.all(): + platforms = file.get_platforms() + # not platform-specific, matches all platforms + if not platforms: + return file + if platform is None or platform in platforms: + return file + return None @property def is_listed(self): - # To be public, version file must have a public status. - return self.file.status == self.file.STATUSES.APPROVED + # To be public, at least one version file must have a public status. + return any([file.status == File.STATUSES.APPROVED for file in self.files.all()]) @property def cannot_be_deleted_reasons(self) -> List[str]: @@ -699,35 +751,41 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): @property def permissions_with_reasons(self) -> List[dict]: - """Returns permissions with reasons, slugs, and names to be shown in templates.""" - if 'permissions' not in self.file.metadata: - return [] - permissions = [] - all_permission_names = {p.slug: p.name for p in VersionPermission.objects.all()} - for slug, reason in self.file.metadata['permissions'].items(): - permissions.append({'slug': slug, 'reason': reason, 'name': all_permission_names[slug]}) + """Returns permissions with reasons, slugs, and names to be shown in templates. + It might make sense to not aggregate at the Version level, and query directly from File. + If we ever restructure multi-platform UI, this method should be revisited. + + Normally we can expect that all files have the same permissions, but checking all files, + just in case. + """ + slug2reason = {} + for file in self.files.all(): + if 'permissions' in file.metadata: + slug2reason.update(file.metadata['permissions']) + + if not slug2reason: + return [] + + all_permission_names = {p.slug: p.name for p in VersionPermission.objects.all()} + permissions = [] + for slug, reason in slug2reason.items(): + permissions.append({'slug': slug, 'reason': reason, 'name': all_permission_names[slug]}) return permissions - @property - def download_name(self) -> str: + def _get_download_name(self, file) -> str: """Return a file name for downloads.""" - replace_char = f'{self}'.replace('.', '-') - return f'{utils.slugify(replace_char)}{self.file.suffix}' + parts = [self.extension.type_slug_singular, self.extension.slug, f'v{self.version}'] + if platforms := file.get_platforms(): + parts.extend(platforms) + return f'{"-".join(parts)}.zip' - @property - def downloadable_signed_url(self) -> str: - # TODO: actual signed URLs? - return self.file.source.url - - def download_url(self, append_repository_and_compatibility=True) -> str: - filename = f'{self.extension.type_slug_singular}-{self.extension.slug}-v{self.version}.zip' + def get_download_url(self, file, append_repository_and_compatibility=True) -> str: + filename = self._get_download_name(file) download_url = reverse( - 'extensions:version-download', + 'extensions:download', kwargs={ - 'type_slug': self.extension.type_slug, - 'slug': self.extension.slug, - 'version': self.version, + 'filehash': file.hash, 'filename': filename, }, ) @@ -738,12 +796,57 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model): } if self.blender_version_max: params['blender_version_max'] = self.blender_version_max - if platforms := self.platforms.all(): - params['platforms'] = ','.join([p.slug for p in platforms]) + if platforms := file.get_platforms(): + params['platforms'] = ','.join(platforms) query_string = urlencode(params) download_url += f'?{query_string}' return download_url + def get_download_list(self) -> List[dict]: + files = list(self.files.all()) + if len(files) == 1: + file = files[0] + return [ + { + 'name': self._get_download_name(file), + 'size': file.size_bytes, + 'url': self.get_download_url(file), + } + ] + platform2file = {} + for file in files: + platforms = file.get_platforms() + if not platforms: + log.warning( + f'data error: Version pk={self.pk} has multiple files, but File pk={file.pk} ' + f'is not platform-specific' + ) + for platform in platforms: + platform2file[platform] = file + return [ + { + 'name': self._get_download_name(file), + 'platform': p, + 'size': file.size_bytes, + 'url': self.get_download_url(file), + } + for p, file in platform2file.items() + ] + + def get_build_list(self) -> List[dict]: + build_list = [] + for file in self.files.all(): + platforms = file.get_platforms() or [] + build_list.append( + { + 'name': self._get_download_name(file), + 'platforms': platforms, + 'size': file.size_bytes, + 'url': self.get_download_url(file), + } + ) + return build_list + def get_delete_url(self) -> str: return reverse( 'extensions:version-delete', diff --git a/extensions/signals.py b/extensions/signals.py index f088cfbd..4840604d 100644 --- a/extensions/signals.py +++ b/extensions/signals.py @@ -43,10 +43,19 @@ def _delete_versionfiles_file( logger.info('Deleting File pk=%s of VersionFile pk=%s', file.pk, instance.pk) file.delete() - if instance.version.files.count() == 0: - # this was the last file, clean up the version - logger.info('Deleting Version pk=%s because its last file was deleted', instance.version.pk) - instance.version.delete() + # this code is already quite convoluted :double-facepalm: + # TODO? maybe find some way to have a predictable order of deletion + try: + instance.version.update_platforms() + if instance.version.files.count() == 0: + # this was the last file, clean up the version + logger.info( + 'Deleting Version pk=%s because its last file was deleted', + instance.version.pk, + ) + instance.version.delete() + except extensions.models.Version.DoesNotExist: + pass @receiver(pre_save, sender=extensions.models.Extension) diff --git a/extensions/templates/extensions/components/extension_edit_detail_card.html b/extensions/templates/extensions/components/extension_edit_detail_card.html index 89b3b009..928b8a24 100644 --- a/extensions/templates/extensions/components/extension_edit_detail_card.html +++ b/extensions/templates/extensions/components/extension_edit_detail_card.html @@ -69,7 +69,13 @@
{% trans 'Size' %}
-
{{ version.file.size_bytes|filesizeformat }}
+ {% if version.has_single_file %} +
{{ version.files.first.size_bytes|filesizeformat }}
+ {% else %} + {% for file in version.files.all %} +
{{ file.size_bytes|filesizeformat }} ({{ file.get_platforms|join:", " }})
+ {% endfor %} + {% endif %}
diff --git a/extensions/templates/extensions/components/platforms.html b/extensions/templates/extensions/components/platforms.html index 037a84d1..ef77985f 100644 --- a/extensions/templates/extensions/components/platforms.html +++ b/extensions/templates/extensions/components/platforms.html @@ -1,10 +1,16 @@ +{% load i18n %} + {% if version.platforms.all %} -
- Supported platforms: - -
+
+
+
{% trans "Supported Platforms" %}
+
+
    + {% for p in version.platforms.all %} +
  • {{p.name}}
  • + {% endfor %} +
+
+
+
{% endif %} diff --git a/extensions/templates/extensions/detail.html b/extensions/templates/extensions/detail.html index 2c0e9936..6d939420 100644 --- a/extensions/templates/extensions/detail.html +++ b/extensions/templates/extensions/detail.html @@ -187,7 +187,20 @@
{% trans 'Size' %}
-
{{ latest.file.size_bytes|filesizeformat }}
+ {% if latest.has_single_file %} +
{{ latest.files.first.size_bytes|filesizeformat }}
+ {% else %} +
+ +
+ {% endif %}
@@ -196,11 +209,12 @@
{% trans 'Compatibility' %}
{% include "extensions/components/blender_version.html" with version=latest %} - {% include "extensions/components/platforms.html" with version=latest %}
+ {% include "extensions/components/platforms.html" with version=latest %} + {% if extension.website %}
@@ -250,37 +264,45 @@ {% block extension_download %}
{% if extension.is_approved %} + {% with download_list=latest.get_download_list %}
-
-
- -
- - {# TODO @front-end: Replace URL of the manual /dev/ with /latest/. #} - ...or download - and Install from Disk - + {% for download_item in download_list %} +
+
+ +
+ + {# TODO @front-end: Replace URL of the manual /dev/ with /latest/. #} + ...or download + and Install from Disk + +
+ {% endfor %}
{# If JavaScript is disabled. #} - {% else %} + {% endwith %} + {% else %}

This {{ extension.get_type_display|lower }} is currently under review.

@@ -290,7 +312,7 @@ {% trans 'Review Extension' %}

- {% endif %} + {% endif %}
{% endblock extension_download %} diff --git a/extensions/templates/extensions/manage/update.html b/extensions/templates/extensions/manage/update.html index 640ff645..69dbf2b4 100644 --- a/extensions/templates/extensions/manage/update.html +++ b/extensions/templates/extensions/manage/update.html @@ -3,7 +3,7 @@ {% block page_title %}{{ extension.name }}{% endblock page_title %} {% block content %} -{% with author=extension.latest_version.file.user form=form|add_form_classes %} +{% with form=form|add_form_classes %}

{{ extension.get_type_display }} {% trans 'details' %}

diff --git a/extensions/templates/extensions/new_version_finalise.html b/extensions/templates/extensions/new_version_finalise.html index 13881e18..7681164e 100644 --- a/extensions/templates/extensions/new_version_finalise.html +++ b/extensions/templates/extensions/new_version_finalise.html @@ -16,11 +16,7 @@ {% csrf_token %} {% with form=form|add_form_classes %} -

- {% blocktranslate with version=form.instance.version %} - v{{ version }} - {% endblocktranslate %} -

+

v{{ form.instance.version }}

@@ -39,6 +35,21 @@
{% endif %}
+
+ {% trans 'Builds' %}: + {% with build_list=form.instance.get_build_list %} +
+ {% for build in build_list %} + + + + {% trans 'Download' %} v{{ form.instance.version }} {{ build.platforms|join:", " }} + + + {% endfor %} + {% endwith %} +
+
diff --git a/extensions/templates/extensions/submit.html b/extensions/templates/extensions/submit.html index e896a7bd..50d07611 100644 --- a/extensions/templates/extensions/submit.html +++ b/extensions/templates/extensions/submit.html @@ -23,9 +23,11 @@ {% trans 'Upload Extension' %} {% endif %} + {% if extension %}

{{ extension.name }}

+ {% endif %}
{% if not extension and drafts %}
diff --git a/extensions/templates/extensions/version_list.html b/extensions/templates/extensions/version_list.html index 2182aba1..1cf6a52d 100644 --- a/extensions/templates/extensions/version_list.html +++ b/extensions/templates/extensions/version_list.html @@ -21,7 +21,7 @@
- {% for version in extension.versions.all %} + {% for version in object_list %} {% if version.is_listed or is_maintainer %}
@@ -30,7 +30,13 @@ {% include "extensions/components/blender_version.html" with version=version %}
+ + {% include "extensions/components/platforms.html" with version=version %} +
Downloads
@@ -69,7 +77,13 @@
Size
-
{{ version.file.size_bytes|filesizeformat }}
+ {% if version.has_single_file %} +
{{ version.files.first.size_bytes|filesizeformat }}
+ {% else %} + {% for file in version.files.all %} +
{{ file.size_bytes|filesizeformat }} ({{ file.get_platforms|join:", " }})
+ {% endfor %} + {% endif %}
@@ -91,17 +105,22 @@
Status
-
{% include "common/components/status.html" with object=version.file %}
+ {# all files of a version get approved together #} +
{% include "common/components/status.html" with object=version.files.first %}
{% endif %}
- + {% with download_list=version.get_download_list %} + {% for download_item in download_list %} + - {% trans 'Download' %} v{{ version.version }} + {% trans 'Download' %} v{{ version.version }} {{ download_item.platform }} + {% endfor %} + {% endwith %} {% if is_maintainer %} Edit diff --git a/extensions/tests/files/addon-with-split-platforms.zip b/extensions/tests/files/addon-with-split-platforms-linux.zip similarity index 100% rename from extensions/tests/files/addon-with-split-platforms.zip rename to extensions/tests/files/addon-with-split-platforms-linux.zip diff --git a/extensions/tests/files/addon-with-split-platforms-windows.zip b/extensions/tests/files/addon-with-split-platforms-windows.zip new file mode 100644 index 00000000..37a5a828 Binary files /dev/null and b/extensions/tests/files/addon-with-split-platforms-windows.zip differ diff --git a/extensions/tests/files/addon-without-platforms-for-split-test.zip b/extensions/tests/files/addon-without-platforms-for-split-test.zip new file mode 100644 index 00000000..f3df336a Binary files /dev/null and b/extensions/tests/files/addon-without-platforms-for-split-test.zip differ diff --git a/extensions/tests/test_api.py b/extensions/tests/test_api.py index da38b708..0ec1fc07 100644 --- a/extensions/tests/test_api.py +++ b/extensions/tests/test_api.py @@ -5,8 +5,9 @@ from django.urls import reverse from rest_framework import status from rest_framework.test import APITestCase, APIClient -from common.tests.factories.users import UserFactory from common.tests.factories.extensions import create_approved_version, create_version +from common.tests.factories.files import FileFactory +from common.tests.factories.users import UserFactory from common.tests.utils import create_user_token from extensions.models import Extension, Version @@ -48,8 +49,9 @@ class ListedExtensionsTest(APITestCase): self.assertEqual(self._listed_extensions_count(), 0) def test_moderate_only_version(self): - self.version.file.status = File.STATUSES.DISABLED - self.version.file.save() + for file in self.version.files.all(): + file.status = File.STATUSES.DISABLED + file.save() self.assertEqual(self._listed_extensions_count(), 0) @@ -150,6 +152,66 @@ class FiltersTest(APITestCase): ).json() self.assertEqual(len(json['data']), 1) + def test_platform_filter_same_extension(self): + version = create_approved_version( + metadata__platforms=['linux-x64'], + metadata__version='1.0.0', + ) + extension = version.extension + version = create_approved_version( + extension=extension, + metadata__platforms=['windows-x64'], + metadata__version='1.0.1', + ) + + url = reverse('extensions:api') + json = self.client.get( + url + '?platform=linux-x64', + HTTP_ACCEPT='application/json', + ).json() + self.assertEqual(len(json['data']), 1) + + json = self.client.get( + url + '?platform=windows-x64', + HTTP_ACCEPT='application/json', + ).json() + self.assertEqual(len(json['data']), 1) + + def test_platform_filter_same_version(self): + version = create_approved_version( + metadata__id='filter_test', + metadata__platforms=['linux-x64'], + metadata__version='1.0.0', + ) + version = version.add_file( + FileFactory( + metadata__id='filter_test', + metadata__platforms=['windows-x64'], + metadata__version='1.0.0', + ) + ) + + url = reverse('extensions:api') + json = self.client.get( + url + '?platform=linux-x64', + HTTP_ACCEPT='application/json', + ).json() + self.assertEqual(len(json['data']), 1) + + json = self.client.get( + url + '?platform=windows-x64', + HTTP_ACCEPT='application/json', + ).json() + self.assertEqual(len(json['data']), 1) + + json = self.client.get( + url, + HTTP_ACCEPT='application/json', + ).json() + self.assertEqual(len(json['data']), 2) + # all archive_url values should be unique + self.assertEqual(len(json['data']), len(set(item['archive_url'] for item in json['data']))) + def test_blender_version_filter_latest_not_max_version(self): version = create_approved_version(metadata__blender_version_min='4.0.1') date_created = version.date_created @@ -238,7 +300,8 @@ class VersionUploadAPITest(APITestCase): self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) self.assertEqual( response.data['message'], - f'Extension "{other_extension.extension_id}" not maintained by user "{self.user.username}"', + f'Extension "{other_extension.extension_id}" not maintained by user ' + f'"{self.user.username}"', ) def test_version_upload_extension_does_not_exist(self): @@ -288,3 +351,59 @@ class VersionUploadAPITest(APITestCase): self.assertEqual(response.status_code, status.HTTP_201_CREATED) self.token.refresh_from_db() self.assertIsNotNone(self.token.date_last_access) + + def test_multiplatform_upload(self): + extension = create_version( + metadata__id="some_addon", + metadata__version="0.0.1", + user=self.user, + ).extension + file_linux = TEST_FILES_DIR / 'addon-with-split-platforms-linux.zip' + file_windows = TEST_FILES_DIR / 'addon-with-split-platforms-windows.zip' + + with open(file_linux, 'rb') as version_file: + response = self.client.post( + self._get_upload_url('some_addon'), + { + 'version_file': version_file, + 'release_notes': 'only linux', + }, + format='multipart', + HTTP_AUTHORIZATION=f'Bearer {self.token_key}', + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED, response.json()) + extension.refresh_from_db() + self.assertEqual(extension.latest_version.files.count(), 1) + self.assertEqual(extension.latest_version.platforms.count(), 1) + + with open(file_windows, 'rb') as version_file: + response = self.client.post( + self._get_upload_url('some_addon'), + { + 'version_file': version_file, + 'release_notes': 'linux and windows', + }, + format='multipart', + HTTP_AUTHORIZATION=f'Bearer {self.token_key}', + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED, response.json()) + extension.refresh_from_db() + self.assertEqual(extension.latest_version.release_notes, 'linux and windows') + self.assertEqual(extension.latest_version.files.count(), 2) + self.assertEqual(extension.latest_version.platforms.count(), 2) + + with open(file_windows, 'rb') as version_file: + response = self.client.post( + self._get_upload_url('some_addon'), + { + 'version_file': version_file, + 'release_notes': 'linux and windows', + }, + format='multipart', + HTTP_AUTHORIZATION=f'Bearer {self.token_key}', + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST, response.json()) + self.assertEqual( + response.data['message']['version_file'], + [f'{extension.latest_version} already has files for windows-x64'], + ) diff --git a/extensions/tests/test_approve.py b/extensions/tests/test_approve.py index f438f316..2bd00de9 100644 --- a/extensions/tests/test_approve.py +++ b/extensions/tests/test_approve.py @@ -26,7 +26,7 @@ class ApproveExtensionTest(TestCase): # latest_version of approved extension must be listed # check that we rollback latest_version when file is not approved - file = new_version.file + file = new_version.files.first() file.status = File.STATUSES.AWAITING_REVIEW file.save(update_fields={'status'}) new_version.refresh_from_db() @@ -36,8 +36,9 @@ class ApproveExtensionTest(TestCase): self.assertTrue(extension.is_listed) # break the first_version, check that nothing is listed anymore - first_version.file.status = File.STATUSES.AWAITING_REVIEW - first_version.file.save() + file = first_version.files.first() + file.status = File.STATUSES.AWAITING_REVIEW + file.save() self.assertFalse(first_version.is_listed) extension.refresh_from_db() self.assertFalse(extension.is_listed) diff --git a/extensions/tests/test_delete.py b/extensions/tests/test_delete.py index fda39d5e..815dd6dd 100644 --- a/extensions/tests/test_delete.py +++ b/extensions/tests/test_delete.py @@ -43,7 +43,7 @@ class DeleteTest(TestCase): source='images/b0/b03fa981527593fbe15b28cf37c020220c3d83021999eab036b87f3bca9c9168.png', ) ) - version_file = version.file + version_file = version.files.first() icon = extension.icon featured_image = extension.featured_image self.assertEqual(version_file.get_status_display(), 'Awaiting Review') @@ -216,3 +216,14 @@ class DeleteTest(TestCase): self.assertEqual(response.status_code, 403) extension.refresh_from_db() + + def test_multi_file_version_delete(self): + version = create_version(metadata__platforms=['linux-x64']) + version.add_file( + FileFactory( + metadata__id=version.extension.extension_id, + metadata__version=version.version, + metadata__platforms=['windows-x64'], + ) + ) + version.extension.delete() diff --git a/extensions/tests/test_manifest.py b/extensions/tests/test_manifest.py index 4691d900..5458492c 100644 --- a/extensions/tests/test_manifest.py +++ b/extensions/tests/test_manifest.py @@ -186,7 +186,7 @@ class ValidateManifestTest(CreateFileTest): self.assertEqual(Extension.objects.count(), 1) # The same author is to send a new version to thte same extension - self.client.force_login(version.file.user) + self.client.force_login(version.files.first().user) non_kitsu_1_6 = { "name": "Blender Kitsu with a different ID", @@ -213,7 +213,7 @@ class ValidateManifestTest(CreateFileTest): self.assertEqual(Extension.objects.count(), 1) # The same author is to send a new version to thte same extension - self.client.force_login(version.file.user) + self.client.force_login(version.files.first().user) kitsu_version_clash = { "name": "Change the name for lols", @@ -254,7 +254,7 @@ class ValidateManifestTest(CreateFileTest): ) # The same author is to send a new version to thte same extension - self.client.force_login(version.file.user) + self.client.force_login(version.files.first().user) updated_kitsu = { "name": version.extension.name, @@ -692,7 +692,7 @@ class VersionPermissionsTest(CreateFileTest): self.assertEqual(version_original, extension.latest_version) # The same author is to send a new version to the same extension but with a new permission - self.client.force_login(version_original.file.user) + self.client.force_login(version_original.files.first().user) new_kitsu = { "id": "blender_kitsu", diff --git a/extensions/tests/test_models.py b/extensions/tests/test_models.py index c2f17e5e..84c4e541 100644 --- a/extensions/tests/test_models.py +++ b/extensions/tests/test_models.py @@ -5,7 +5,9 @@ from django.test import TestCase 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.files import FileFactory from common.tests.factories.users import UserFactory +from extensions.models import Platform class ExtensionTest(TestCase): @@ -70,19 +72,16 @@ class VersionTest(TestCase): maxDiff = None fixtures = ['dev', 'licenses'] - def setUp(self): - super().setUp() - self.version = create_version( + def test_admin_change_view(self): + version = create_version( 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) - - def test_admin_change_view(self): - path = get_admin_change_path(obj=self.version) + self.assertEqual(entries_for(version).count(), 0) + path = get_admin_change_path(obj=version) self.assertEqual(path, '/admin/extensions/version/1/change/') admin_user = UserFactory(is_staff=True, is_superuser=True) @@ -91,6 +90,77 @@ class VersionTest(TestCase): self.assertEqual(response.status_code, 200, path) + def test_get_remaining_platforms(self): + version_platforms_none = create_version(metadata__platforms=None) + self.assertEqual(version_platforms_none.get_remaining_platforms(), set()) + + version_platforms_empty = create_version(metadata__platforms=[]) + self.assertEqual(version_platforms_empty.get_remaining_platforms(), set()) + + version_platforms_some = create_version(metadata__platforms=['linux-x64', 'windows-x64']) + self.assertTrue(version_platforms_some.get_remaining_platforms()) + + version_two_files = create_version(metadata__platforms=['linux-x64']) + file = FileFactory(metadata__platforms=['windows-x64']) + version_two_files.files.add(file) + self.assertTrue(version_two_files.get_remaining_platforms()) + + version_platforms_all = create_version( + metadata__platforms=[p.slug for p in Platform.objects.all()] + ) + self.assertEqual(version_platforms_all.get_remaining_platforms(), set()) + + skip_platforms = set(['linux-x64', 'windows-x64']) + version_platforms_without_some = create_version( + metadata__platforms=[ + p.slug for p in Platform.objects.all() if p.slug not in skip_platforms + ] + ) + self.assertEqual(version_platforms_without_some.get_remaining_platforms(), skip_platforms) + + def test_collect_platforms_across_files(self): + version = create_version(metadata__platforms=['linux-x64']) + file = FileFactory(metadata__platforms=['windows-x64']) + version.add_file(file) + self.assertQuerysetEqual( + version.platforms.order_by('slug'), + Platform.objects.filter(slug__in=['linux-x64', 'windows-x64']).order_by('slug'), + ) + file.delete() + version.refresh_from_db() + self.assertQuerysetEqual( + version.platforms.order_by('slug'), + Platform.objects.filter(slug__in=['linux-x64']).order_by('slug'), + ) + + def test_add_file(self): + version = create_version(metadata__platforms=['linux-x64']) + file = FileFactory(metadata__platforms=['linux-x64']) + with self.assertRaises(ValueError): + version.add_file(file) + + all_platforms_version = create_version(metadata__platforms=[]) + file = FileFactory(metadata__platforms=['linux-x64']) + with self.assertRaises(ValueError): + all_platforms_version.add_file(file) + + def test_get_file_for_platform(self): + version = create_version(metadata__platforms=['linux-x64']) + file = FileFactory(metadata__platforms=['windows-x64']) + version.add_file(file) + self.assertIsNotNone(version.get_file_for_platform(None)) + self.assertIsNotNone(version.get_file_for_platform('linux-x64')) + self.assertIsNotNone(version.get_file_for_platform('windows-x64')) + self.assertIsNone(version.get_file_for_platform('windows-arm64')) + + file2 = FileFactory(metadata__platforms=['macos-x64', 'macos-arm64']) + version.add_file(file2) + self.assertIsNotNone(version.get_file_for_platform('macos-x64')) + + version2 = create_version(metadata__platforms=[]) + self.assertIsNotNone(version2.get_file_for_platform(None)) + self.assertIsNotNone(version2.get_file_for_platform('macos-x64')) + class UpdateMetadataTest(TestCase): fixtures = ['dev', 'licenses'] diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index 687726bd..b3b8cbb4 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -80,7 +80,7 @@ EXPECTED_EXTENSION_DATA = { 'version_str': '0.1.0', 'slug': 'some-addon', }, - 'addon-with-split-platforms.zip': { + 'addon-with-split-platforms-linux.zip': { 'metadata': { 'tagline': 'Some add-on tag line', 'name': 'Some Add-on', @@ -118,12 +118,14 @@ EXPECTED_VALIDATION_ERRORS = { 'source': ['The manifest file is missing.'], }, 'invalid-manifest-toml.zip': {'source': ['Manifest file contains invalid code.']}, - 'invalid-theme-multiple-xmls.zip': {'source': ['Themes can only contain one XML file.']}, - 'invalid-theme-multiple-xmls-macosx.zip': {'source': ['Archive contains forbidden files or directories: __MACOSX/']}, + 'invalid-theme-multiple-xmls.zip': { + 'source': ['Themes can only contain one XML file.'] + }, + 'invalid-theme-multiple-xmls-macosx.zip': { + 'source': ['Archive contains forbidden files or directories: __MACOSX/'] + }, 'invalid-missing-wheels.zip': { - 'source': [ - 'Python wheel missing: addon/wheels/test-wheel-whatever.whl' - ] + 'source': ['Python wheel missing: addon/wheels/test-wheel-whatever.whl'] }, } POST_DATA = { @@ -435,11 +437,11 @@ class SubmitFinaliseTest(CheckFilePropertiesMixin, TestCase): self.assertEqual(version.release_notes, data['release_notes']) # Check version file properties self._test_file_properties( - version.file, + version.files.first(), content_type='application/zip', get_status_display='Awaiting Review', get_type_display='Add-on', - hash=version.file.original_hash, + hash=version.files.first().original_hash, original_hash='sha256:2831385', ) # We cannot check for the ManyToMany yet (tags, licences, permissions) @@ -515,7 +517,7 @@ class NewVersionTest(TestCase): self.assertTrue(response['Location'].startswith('/oauth/login')) def test_validation_errors_agreed_with_terms_required(self): - self.client.force_login(self.version.file.user) + self.client.force_login(self.version.files.first().user) with open(TEST_FILES_DIR / 'amaranth-1.0.8.zip', 'rb') as fp: response = self.client.post(self.url, {'source': fp}) @@ -527,7 +529,7 @@ class NewVersionTest(TestCase): ) def test_validation_errors_invalid_extension(self): - self.client.force_login(self.version.file.user) + self.client.force_login(self.version.files.first().user) with open(TEST_FILES_DIR / 'empty.txt', 'rb') as fp: response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) @@ -539,7 +541,7 @@ class NewVersionTest(TestCase): ) def test_validation_errors_empty_file(self): - self.client.force_login(self.version.file.user) + self.client.force_login(self.version.files.first().user) with open(TEST_FILES_DIR / 'empty.zip', 'rb') as fp: response = self.client.post(self.url, {'source': fp, 'agreed_with_terms': True}) @@ -551,7 +553,7 @@ class NewVersionTest(TestCase): ) def test_upload_new_file_and_finalise_new_version(self): - self.client.force_login(self.version.file.user) + self.client.force_login(self.version.files.first().user) self.extension.approve() # Check step 1: upload a new file @@ -570,7 +572,7 @@ class NewVersionTest(TestCase): 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.files.first().get_status_display(), 'Approved') self.assertEqual(new_version.release_notes, '') self.assertEqual( ApprovalActivity.objects.filter( @@ -595,6 +597,60 @@ class NewVersionTest(TestCase): self.assertEqual(new_version.release_notes, 'new version') +class MultiPlatformUploadTest(TestCase): + def test_upload_more_files(self): + extension = create_version( + metadata__id="some_addon", + metadata__version="0.0.1", + ).extension + file_linux = TEST_FILES_DIR / 'addon-with-split-platforms-linux.zip' + file_windows = TEST_FILES_DIR / 'addon-with-split-platforms-windows.zip' + file_no_platforms = TEST_FILES_DIR / 'addon-without-platforms-for-split-test.zip' + self.client.force_login(extension.authors.all()[0]) + + with open(file_linux, 'rb') as fp: + response = self.client.post( + extension.get_new_version_url(), + {'source': fp, 'agreed_with_terms': True}, + ) + self.assertEqual(response.status_code, 302) + extension.refresh_from_db() + self.assertEqual(extension.latest_version.files.count(), 1) + self.assertEqual(extension.latest_version.platforms.count(), 1) + + with open(file_windows, 'rb') as fp: + response = self.client.post( + extension.get_new_version_url(), + {'source': fp, 'agreed_with_terms': True}, + ) + self.assertEqual(response.status_code, 302) + extension.refresh_from_db() + self.assertEqual(extension.latest_version.files.count(), 2) + self.assertEqual(extension.latest_version.platforms.count(), 2) + + # can't upload the same file a second time + with open(file_windows, 'rb') as fp: + response = self.client.post( + extension.get_new_version_url(), + {'source': fp, 'agreed_with_terms': True}, + ) + self.assertEqual(response.status_code, 200) + extension.refresh_from_db() + self.assertEqual(extension.latest_version.files.count(), 2) + self.assertEqual(extension.latest_version.platforms.count(), 2) + + # can't upload a platform-agnostic file + with open(file_no_platforms, 'rb') as fp: + response = self.client.post( + extension.get_new_version_url(), + {'source': fp, 'agreed_with_terms': True}, + ) + self.assertEqual(response.status_code, 200) + extension.refresh_from_db() + self.assertEqual(extension.latest_version.files.count(), 2) + self.assertEqual(extension.latest_version.platforms.count(), 2) + + class DraftsWarningTest(TestCase): fixtures = ['licenses'] diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index 9c531034..2a560b8b 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -216,7 +216,7 @@ class ExtensionManageViewTest(_BaseTestCase): class UpdateVersionViewTest(_BaseTestCase): def test_update_view_access(self): extension = _create_extension() - extension_owner = extension.latest_version.file.user + extension_owner = extension.latest_version.files.first().user extension.authors.add(extension_owner) random_user = UserFactory() @@ -246,7 +246,7 @@ class UpdateVersionViewTest(_BaseTestCase): def test_blender_max_version(self): extension = _create_extension() - extension_owner = extension.latest_version.file.user + extension_owner = extension.latest_version.files.first().user extension.authors.add(extension_owner) self.client.force_login(extension_owner) url = reverse( diff --git a/extensions/urls.py b/extensions/urls.py index a2c63b8e..576a3dd5 100644 --- a/extensions/urls.py +++ b/extensions/urls.py @@ -17,7 +17,7 @@ urlpatterns = [ # API path('api/v1/extensions/', api.ExtensionsAPIView.as_view(), name='api'), path( - 'api/v1/extensions//versions/new/', + 'api/v1/extensions//versions/upload/', api.UploadExtensionVersionView.as_view(), name='upload-extension-version', ), @@ -28,6 +28,7 @@ urlpatterns = [ path('search/', public.SearchView.as_view(), name='search'), path('tag//', public.SearchView.as_view(), name='by-tag'), path('team//', public.SearchView.as_view(), name='by-team'), + path('download//', public.file_download, name='download'), re_path( rf'^(?P{EXTENSION_SLUGS_PATH})/', include( diff --git a/extensions/views/api.py b/extensions/views/api.py index 1d18f21b..7e254d05 100644 --- a/extensions/views/api.py +++ b/extensions/views/api.py @@ -12,7 +12,6 @@ from django.db import transaction from common.compare import is_in_version_range, 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 @@ -55,17 +54,16 @@ class ListedExtensionsSerializer(serializers.ModelSerializer): except Platform.DoesNotExist: self.platform = self.UNKNOWN_PLATFORM - def to_representation(self, instance): - matching_version = None + def find_matching_files_and_version(self, instance): # avoid triggering additional db queries, reuse the prefetched queryset - versions = [ - v - for v in instance.versions.all() - if v.file and v.file.status in instance.valid_file_statuses - ] + versions = sorted( + [v for v in instance.versions.all() if v.is_listed], + key=lambda v: v.date_created, + reverse=True, + ) if not versions: - return None - versions = sorted(versions, key=lambda v: v.date_created, reverse=True) + return ([], None) + for v in versions: if self.blender_version and not is_in_version_range( self.blender_version, @@ -73,42 +71,48 @@ class ListedExtensionsSerializer(serializers.ModelSerializer): v.blender_version_max, ): continue - platform_slugs = set(p.slug for p in v.platforms.all()) - # empty platforms field matches any platform filter - # UNKNOWN_PLATFORM matches only empty platforms field - if self.platform and (platform_slugs and self.platform not in platform_slugs): - continue - matching_version = v - break + if self.platform: + if file := v.get_file_for_platform(self.platform): + return ([file], v) + else: + return (v.files.all(), v) - if not matching_version: - return None + return ([], None) - data = { - 'id': instance.extension_id, - 'schema_version': matching_version.schema_version, - 'name': instance.name, - 'version': matching_version.version, - 'tagline': matching_version.tagline, - 'archive_hash': matching_version.file.original_hash, - 'archive_size': matching_version.file.size_bytes, - 'archive_url': self.request.build_absolute_uri( - matching_version.download_url(append_repository_and_compatibility=False) - ), - 'type': EXTENSION_TYPE_SLUGS_SINGULAR.get(instance.type), - 'blender_version_min': matching_version.blender_version_min, - 'blender_version_max': matching_version.blender_version_max, - 'website': self.request.build_absolute_uri(instance.get_absolute_url()), - # avoid triggering additional db queries, reuse the prefetched queryset - 'maintainer': instance.team and instance.team.name or str(instance.authors.all()[0]), - 'license': [license_iter.slug for license_iter in matching_version.licenses.all()], - 'permissions': matching_version.file.metadata.get('permissions'), - 'platforms': [platform.slug for platform in matching_version.platforms.all()], - # TODO: handle copyright - 'tags': [str(tag) for tag in matching_version.tags.all()], - } - - return clean_json_dictionary_from_optional_fields(data) + def to_representation(self, instance): + matching_files, matching_version = self.find_matching_files_and_version(instance) + result = [] + for file in matching_files: + data = { + 'id': instance.extension_id, + 'schema_version': matching_version.schema_version, + 'name': instance.name, + 'version': matching_version.version, + 'tagline': matching_version.tagline, + 'archive_hash': file.original_hash, + 'archive_size': file.size_bytes, + 'archive_url': self.request.build_absolute_uri( + matching_version.get_download_url( + file, + append_repository_and_compatibility=False, + ) + ), + 'type': EXTENSION_TYPE_SLUGS_SINGULAR.get(instance.type), + 'blender_version_min': matching_version.blender_version_min, + 'blender_version_max': matching_version.blender_version_max, + 'website': self.request.build_absolute_uri(instance.get_absolute_url()), + # avoid triggering additional db queries, reuse the prefetched queryset + 'maintainer': ( + instance.team and instance.team.name or str(instance.authors.all()[0]) + ), + 'license': [license_iter.slug for license_iter in matching_version.licenses.all()], + 'permissions': file.metadata.get('permissions'), + 'platforms': file.get_platforms(), + # TODO: handle copyright + 'tags': [str(tag) for tag in matching_version.tags.all()], + } + result.append(clean_json_dictionary_from_optional_fields(data)) + return result class ExtensionsAPIView(APIView): @@ -153,7 +157,10 @@ class ExtensionsAPIView(APIView): request=request, many=True, ) - data = [e for e in serializer.data if e is not None] + data = [] + for entry in serializer.data: + data.extend(entry) + return Response( { 'blocklist': Extension.objects.blocklisted.values_list('extension_id', flat=True), @@ -201,14 +208,16 @@ class UploadExtensionVersionView(APIView): status=status.HTTP_403_FORBIDDEN, ) - # Create a NewVersionView instance to handle file creation - new_version_view = NewVersionView(request=request, extension=extension) - - # Pass the version_file to the form - form = new_version_view.get_form(FileFormSkipAgreed) + form = FileFormSkipAgreed( + data={}, + extension=extension, + request=request, + ) form.fields['source'].initial = version_file if not form.is_valid(): + if 'source' in form.errors: + form.errors['version_file'] = form.errors.pop('source') return Response({'message': form.errors}, status=status.HTTP_400_BAD_REQUEST) with transaction.atomic(): @@ -217,10 +226,16 @@ class UploadExtensionVersionView(APIView): file_instance.user = user file_instance.save() - version = extension.create_version_from_file( - file=file_instance, - release_notes=release_notes, - ) + manifest_version = file_instance.metadata['version'] + if version := extension.versions.filter(version=manifest_version).first(): + version.add_file(file_instance) + version.release_notes = release_notes + version.save(update_fields={'release_notes'}) + else: + version = extension.create_version_from_file( + file=file_instance, + release_notes=release_notes, + ) return Response( { diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 6b62031f..21980358 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -30,17 +30,9 @@ class VersionsView(ExtensionQuerysetMixin, ListView): paginate_by = 15 def get_queryset(self): - """Allow logged in users to view unlisted versions in certain conditions. - - * maintainers should be able to preview their yet unlisted versions; - * staff should be able to preview yet unlisted versions; - """ self.extension_queryset = self.get_extension_queryset() self.extension = get_object_or_404(self.extension_queryset, slug=self.kwargs['slug']) - queryset = self.extension.versions - if self.request.user.is_staff or self.extension.has_maintainer(self.request.user): - return queryset.all() - return queryset.listed + return self.extension.versions.prefetch_related('files', 'platforms', 'permissions') def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) @@ -222,7 +214,11 @@ class NewVersionView( @transaction.atomic def form_valid(self, form): response = super().form_valid(form) - self.extension.create_version_from_file(self.object) + manifest_version = self.object.metadata['version'] + if version := self.extension.versions.filter(version=manifest_version).first(): + version.add_file(self.object) + else: + version = self.extension.create_version_from_file(self.object) return response def get_success_url(self): diff --git a/extensions/views/public.py b/extensions/views/public.py index ef92cdba..2209d24d 100644 --- a/extensions/views/public.py +++ b/extensions/views/public.py @@ -8,16 +8,15 @@ from django.shortcuts import get_object_or_404, redirect from django.utils.translation import gettext_lazy as _ from django.views.generic import DetailView, ListView -from extensions.models import Extension, Version, Tag from constants.base import ( EXTENSION_TYPE_SLUGS, EXTENSION_TYPE_PLURAL, EXTENSION_TYPE_CHOICES, ) -from stats.models import ExtensionView +from extensions.models import Extension, Version, Tag +from files.models import File +from stats.models import ExtensionDownload, ExtensionView, VersionDownload import ratings.models - -from stats.models import ExtensionDownload, VersionDownload import teams.models User = get_user_model() @@ -52,16 +51,32 @@ class HomeView(ListedExtensionsView): def extension_version_download(request, type_slug, slug, version, filename): + """A backward-compatible url for downloads. + + Assuming only a single file for a given version. + """ + extension_version = get_object_or_404(Version, extension__slug=slug, version=version) + file = extension_version.files.first() + return file_download(request, file.hash, filename) + + +def file_download(request, filehash, filename): """Download an extension version and count downloads. + This method processes urls constructed by Version.get_download_list. + The `filename` parameter is used to pass a file name ending with `.zip`. This is a convention Blender uses to initiate an extension installation on an HTML anchor drag&drop. + Also see $arg_filename usage in playbooks/templates/nginx/http.conf """ - extension_version = get_object_or_404(Version, extension__slug=slug, version=version) + # TODO check file status + file = get_object_or_404(File, hash=filehash, version__isnull=False) + extension_version = file.version.first() ExtensionDownload.create_from_request(request, object_id=extension_version.extension_id) VersionDownload.create_from_request(request, object_id=extension_version.pk) - return redirect(extension_version.downloadable_signed_url + f'?filename={filename}') + url = file.source.url + return redirect(f'{url}?filename={filename}') class SearchView(ListedExtensionsView): @@ -227,6 +242,9 @@ class ExtensionDetailView(DetailView): queryset = Extension.objects.listed.prefetch_related( 'authors', 'latest_version__files', + 'latest_version__files__validation', + 'latest_version__permissions', + 'latest_version__platforms', 'latest_version__tags', 'preview_set', 'preview_set__file', @@ -235,11 +253,6 @@ class ExtensionDetailView(DetailView): 'ratings__ratingreply__user', 'ratings__user', 'team', - 'versions', - 'versions__files', - 'versions__files__validation', - 'versions__permissions', - 'versions__platforms', ).distinct() context_object_name = 'extension' template_name = 'extensions/detail.html' diff --git a/files/forms.py b/files/forms.py index f8e2e254..44464771 100644 --- a/files/forms.py +++ b/files/forms.py @@ -10,6 +10,7 @@ from django.utils.translation import gettext_lazy as _ from .validators import ( ExtensionIDManifestValidator, ExtensionNameManifestValidator, + ExtensionVersionManifestValidator, FileMIMETypeValidator, ManifestValidator, ) @@ -36,11 +37,15 @@ class FileForm(forms.ModelForm): # TODO: surface TOML parsing errors? 'invalid_manifest_toml': _('Manifest file contains invalid code.'), 'invalid_missing_init': mark_safe(_('Add-on file missing: __init__.py.')), - 'missing_or_multiple_theme_xml': mark_safe(_('Themes can only contain one XML file.')), + 'missing_or_multiple_theme_xml': mark_safe( + _('Themes can only contain one XML file.') + ), 'invalid_zip_archive': msg_only_zip_files, 'missing_manifest_toml': _('The manifest file is missing.'), - 'missing_wheel': _('Python wheel missing: %(path)s'), # TODO %(path)s - 'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), #TODO %(paths)s + 'missing_wheel': _('Python wheel missing: %(path)s'), # TODO %(path)s + 'forbidden_filepaths': _( + 'Archive contains forbidden files or directories: %(paths)s' + ), # TODO %(paths)s } class Meta: @@ -69,8 +74,8 @@ class FileForm(forms.ModelForm): ) def __init__(self, *args, **kwargs): - self.request = kwargs.pop('request') self.extension = kwargs.pop('extension', None) + self.request = kwargs.pop('request') for field in self.base_fields: if field not in {'source', 'agreed_with_terms'}: self.base_fields[field].required = False @@ -160,6 +165,10 @@ class FileForm(forms.ModelForm): ManifestValidator(manifest) ExtensionIDManifestValidator(manifest, self.extension) ExtensionNameManifestValidator(manifest, self.extension) + ExtensionVersionManifestValidator( + manifest, + self.extension, + ) self.cleaned_data['metadata'] = manifest self.cleaned_data['type'] = EXTENSION_SLUG_TYPES[manifest['type']] diff --git a/files/models.py b/files/models.py index 2c6f47c1..048b9be1 100644 --- a/files/models.py +++ b/files/models.py @@ -177,15 +177,20 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): 'website': data.get('website'), } - @property - def parsed_version_fields(self) -> Dict[str, Any]: - """Return Version-related data that was parsed from file's content.""" - # Currently, the content of the manifest file is the only - # kind of file metadata that is supported. - data = self.metadata + def get_platforms(self): + return self.parse_platforms_from_manifest(self.metadata) + + @classmethod + def parse_platforms_from_manifest(self, data): build_generated_platforms = None if 'build' in data and 'generated' in data['build']: build_generated_platforms = data['build']['generated'].get('platforms') + return build_generated_platforms or data.get('platforms') + + @property + def parsed_version_fields(self) -> Dict[str, Any]: + """Return Version-related data that was parsed from file's content.""" + data = self.metadata return { 'version': data.get('version'), 'tagline': data.get('tagline'), @@ -193,7 +198,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, models.Model): 'schema_version': data.get('schema_version'), 'licenses': data.get('license'), 'permissions': data.get('permissions'), - 'platforms': build_generated_platforms or data.get('platforms'), + 'platforms': self.get_platforms(), 'tags': data.get('tags'), } diff --git a/files/validators.py b/files/validators.py index f040e882..944d29b9 100644 --- a/files/validators.py +++ b/files/validators.py @@ -16,6 +16,7 @@ from extensions.models import ( ) from constants.base import EXTENSION_TYPE_SLUGS_SINGULAR, EXTENSION_TYPE_CHOICES from files.utils import guess_mimetype_from_ext, guess_mimetype_from_content +import files.models logger = logging.getLogger(__name__) @@ -164,6 +165,52 @@ class ExtensionNameManifestValidator: ) +class ExtensionVersionManifestValidator: + """Validates version existence in db and available platforms.""" + + def __init__(self, manifest, extension_to_be_updated): + + # If the extension wasn't created yet, any version is valid + if not extension_to_be_updated: + return + + manifest_version = manifest.get('version') + version = extension_to_be_updated.versions.filter(version=manifest_version).first() + if not version: + return + # for existing versions only submissions for remaining platfroms are valid + remaining_platforms = version.get_remaining_platforms() + if platforms := files.models.File.parse_platforms_from_manifest(manifest): + if diff := set(platforms) - remaining_platforms: + raise ValidationError( + { + 'source': [f'{version} already has files for {", ".join(diff)}'], + }, + code='invalid', + ) + else: + if remaining_platforms: + raise ValidationError( + { + 'source': [ + f'File upload for {version} is allowed only for remaining platforms: ' + f'{", ".join(remaining_platforms)}' + ], + }, + code='invalid', + ) + else: + raise ValidationError( + { + 'source': [ + f'The version {escape(manifest_version)} was already uploaded for this ' + f'extension ({extension_to_be_updated.name})' + ], + }, + code='invalid', + ) + + class ManifestFieldValidator: @classmethod def validate(cls, *, name: str, value: object, manifest: dict) -> str: @@ -511,30 +558,6 @@ class SchemaVersionValidator(VersionValidator): ) -class VersionVersionValidator(VersionValidator): - example = '1.0.0' - - @classmethod - def validate(cls, *, name: str, value: str, manifest: dict) -> str: - """Return error message if cannot validate, otherwise returns nothing""" - if err_message := super().validate(name=name, value=value, manifest=manifest): - return err_message - - extension = Extension.objects.filter(extension_id=manifest.get("id")).first() - - # If the extension wasn't created yet, any version is valid - if not extension: - return - - version = extension.versions.filter(version=value).first() - - if version: - return mark_safe( - f'The version {value} was already uploaded for this extension ' - f'({extension.name})' - ) - - class VersionMinValidator(VersionValidator): example = '4.2.0' @@ -639,7 +662,7 @@ class ManifestValidator: 'schema_version': SchemaVersionValidator, 'tagline': TaglineValidator, 'type': TypeValidator, - 'version': VersionVersionValidator, + 'version': VersionValidator, } optional_fields = { 'blender_version_max': VersionMaxValidator, diff --git a/ratings/templates/ratings/rating_list.html b/ratings/templates/ratings/rating_list.html index b2e70db7..67d3922e 100644 --- a/ratings/templates/ratings/rating_list.html +++ b/ratings/templates/ratings/rating_list.html @@ -4,7 +4,7 @@ {% block page_title %}{{ extension.name }}{% endblock page_title %} {% block content %} - {% with latest=extension.latest_version author=extension.latest_version.file.user %} + {% with latest=extension.latest_version %} {% endif %} diff --git a/reviewers/tests/test_views.py b/reviewers/tests/test_views.py index 1fb91fca..d2028a1d 100644 --- a/reviewers/tests/test_views.py +++ b/reviewers/tests/test_views.py @@ -14,7 +14,7 @@ class CommentsViewTest(TestCase): self.default_version = version ApprovalActivity( type=ApprovalActivity.ActivityType.COMMENT, - user=version.file.user, + user=version.files.first().user, extension=version.extension, message='test comment', ).save() diff --git a/reviewers/views.py b/reviewers/views.py index f9ad2ef7..4e6569e6 100644 --- a/reviewers/views.py +++ b/reviewers/views.py @@ -77,9 +77,13 @@ class ExtensionsApprovalDetailView(DetailView): def get_queryset(self): return self.model.objects.prefetch_related( 'authors', + 'latest_version__files', + 'latest_version__files__validation', + 'latest_version__permissions', + 'latest_version__platforms', + 'latest_version__tags', 'preview_set', 'preview_set__file', - 'versions', ).all() def get_context_data(self, **kwargs): diff --git a/users/tests/test_tasks.py b/users/tests/test_tasks.py index 12567b39..59028ff5 100644 --- a/users/tests/test_tasks.py +++ b/users/tests/test_tasks.py @@ -10,7 +10,6 @@ from common.tests.factories.extensions import RatingFactory, create_approved_ver from common.tests.factories.reviewers import ApprovalActivityFactory from common.tests.factories.users import OAuthUserFactory, create_moderator, UserFactory import extensions.models -import files.models import users.tasks as tasks User = get_user_model() @@ -83,11 +82,11 @@ class TestTasks(TestCase): self.assertEqual(response.status_code, 200) response = self.client.get(version.extension.get_versions_url()) self.assertEqual(response.status_code, 200) - response = self.client.get(version.download_url()) + response = self.client.get(version.get_download_url(version.files.first())) self.assertEqual(response.status_code, 302) self.assertEqual( response['Location'], - f'/media/{version.file.original_name}' + f'/media/{version.files.first().original_name}' f'?filename=add-on-{version.extension.slug}-v{version.version}.zip', ) # Check that ratings remained publicly accessible @@ -146,10 +145,9 @@ class TestTasks(TestCase): user.refresh_from_db() # Check that unlisted extension file and version were deleted - with self.assertRaises(files.models.File.DoesNotExist): - self.authored_unlisted_extension.latest_version.file.refresh_from_db() with self.assertRaises(extensions.models.Version.DoesNotExist): self.authored_unlisted_extension.latest_version.refresh_from_db() + self.assertFalse(self.authored_unlisted_extension.latest_version.files.all()) # Check that reports made by this account remained accessible self.report1.refresh_from_db()