Multi-platform: support multiple files per version #201

Merged
Oleg-Komarov merged 43 commits from multi-os into main 2024-07-09 16:27:46 +02:00
16 changed files with 106 additions and 84 deletions
Showing only changes of commit addb5c2127 - Show all commits

View File

@ -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,7 +277,7 @@ 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
for file in latest_version.files.all():
file.status = FILE_STATUS_CHOICES.APPROVED
file.save()
@ -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,23 +673,9 @@ 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 len(files) == 1:
return files[0]
elif len(files) == 0:
return None
else:
log.warning('FIXME: multiple files accessed via .file property')
return files[0]
@property
def single_file(self):
return len(self.files.all()) == 1
@property
def can_upload_more_files(self):
@ -701,8 +693,8 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
@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]:
@ -717,26 +709,31 @@ class Version(CreatedModifiedMixin, TrackChangesMixin, models.Model):
@property
Oleg-Komarov marked this conversation as resolved Outdated

or s/get_available_platforms/get_remaining_platforms/

in the sense that these are the platforms which don't yet have files uploaded for them, if any platform-specific file were uploaded

or `s/get_available_platforms/get_remaining_platforms/` in the sense that these are the platforms which don't yet have files uploaded for them, if any platform-specific file were uploaded
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:
# FIXME? is it ok to check just the first file?
file = self.files.all()[0]
if 'permissions' not in 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():
for slug, reason in file.metadata['permissions'].items():
permissions.append({'slug': slug, 'reason': reason, 'name': all_permission_names[slug]})
return permissions
# FIXME make dependent on File or platform
@property
def download_name(self) -> str:
"""Return a file name for downloads."""
replace_char = f'{self}'.replace('.', '-')
return f'{utils.slugify(replace_char)}{self.file.suffix}'
return f'{utils.slugify(replace_char)}{self.files.first().suffix}'
# FIXME make dependent on File or platform
@property
def downloadable_signed_url(self) -> str:
# TODO: actual signed URLs?
return self.file.source.url
return self.files.first().source.url
# FIXME make dependent on File or platform
def download_url(self, append_repository_and_compatibility=True) -> str:
filename = f'{self.extension.type_slug_singular}-{self.extension.slug}-v{self.version}.zip'
download_url = reverse(

View File

@ -69,7 +69,11 @@
</div>
<div class="dl-col">
<dt>{% trans 'Size' %}</dt>
<dd>{{ version.file.size_bytes|filesizeformat }}</dd>
{% if version.single_file %}
<dd>{{ version.files.first.size_bytes|filesizeformat }}</dd>
{% else %}
<dd>TODO multiple files</dd>
{% endif %}
</div>
</div>

View File

@ -187,7 +187,11 @@
</div>
<div class="dl-col">
<dt>{% trans 'Size' %}</dt>
<dd>{{ latest.file.size_bytes|filesizeformat }}</dd>
{% if latest.single_file %}
<dd>{{ latest.files.first.size_bytes|filesizeformat }}</dd>
{% else %}
<dd>TODO multiple files</dd>
{% endif %}
</div>
</div>

View File

@ -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 %}
<div class="row">
<div class="col-md-8">
<h2>{{ extension.get_type_display }} {% trans 'details' %}</h2>

View File

@ -30,7 +30,11 @@
{% include "extensions/components/blender_version.html" with version=version %}
</span>
<ul class="ms-auto">
<li class="show-on-collapse">{{ version.file.size_bytes|filesizeformat }}</li>
{% if version.single_file %}
<li class="show-on-collapse">{{ version.files.first.size_bytes|filesizeformat }}</li>
{% else %}
<li class="show-on-collapse">TODO multiple files</li>
{% endif %}
<li class="show-on-collapse"><i class="i-download"></i> {{ version.download_count }}</li>
<li>
<a href="#v{{ version.version|slugify }}" title="{% firstof version.date_approved|date:'l jS, F Y - H:i' version.date_created|date:'l jS, F Y - H:i' %}">
@ -69,7 +73,11 @@
</div>
<div class="dl-col">
<dt>Size</dt>
<dd>{{ version.file.size_bytes|filesizeformat }}</dd>
{% if version.single_file %}
<dd>{{ version.files.first.size_bytes|filesizeformat }}</dd>
{% else %}
<dd>TODO multiple files</dd>
{% endif %}
</div>
</div>
<div class="dl-row">
@ -91,7 +99,11 @@
<div class="dl-row">
<div class="dl-col">
<dt>Status</dt>
<dd>{% include "common/components/status.html" with object=version.file %}</dd>
{% if version.single_file %}
<dd>{% include "common/components/status.html" with object=version.files.first %}</dd>
{% else %}
<dd>TODO multiple files</dd>
{% endif %}
</div>
</div>
{% endif %}

View File

@ -48,8 +48,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)

View File

@ -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)

View File

@ -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')

View File

@ -185,7 +185,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",
@ -212,7 +212,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",
@ -253,7 +253,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",

View File

@ -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 <strong>one XML file</strong>.']},
'invalid-theme-multiple-xmls-macosx.zip': {'source': ['Archive contains forbidden files or directories: __MACOSX/']},
'invalid-theme-multiple-xmls.zip': {
'source': ['Themes can only contain <strong>one XML file</strong>.']
},
'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(

View File

@ -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(

View File

@ -55,13 +55,10 @@ class ListedExtensionsSerializer(serializers.ModelSerializer):
self.platform = self.UNKNOWN_PLATFORM
def to_representation(self, instance):
matching_file = None
matching_version = None
# 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 = [v for v in instance.versions.all() if v.is_listed]
if not versions:
return None
versions = sorted(versions, key=lambda v: v.date_created, reverse=True)
@ -72,15 +69,17 @@ class ListedExtensionsSerializer(serializers.ModelSerializer):
v.blender_version_max,
):
continue
platform_slugs = set(p.slug for p in v.platforms.all())
for file in v.files.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):
platforms = file.platforms()
if self.platform and (platforms and self.platform not in platforms):
continue
matching_file = file
matching_version = v
Oleg-Komarov marked this conversation as resolved Outdated

this looks shared with the logic in public view that selects which file to pick from storage, and should probable become a utility or method somewhere.

this looks shared with the logic in `public` view that selects which file to pick from storage, and should probable become a utility or method somewhere.
break
Oleg-Komarov marked this conversation as resolved Outdated

needs another break here

needs another `break` here
if not matching_version:
if not matching_file:
return None
data = {
@ -89,9 +88,10 @@ class ListedExtensionsSerializer(serializers.ModelSerializer):
'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_hash': matching_file.original_hash,
'archive_size': matching_file.size_bytes,
'archive_url': self.request.build_absolute_uri(
# FIXME download_url is per file
matching_version.download_url(append_repository_and_compatibility=False)
),
'type': EXTENSION_TYPE_SLUGS_SINGULAR.get(instance.type),
@ -101,7 +101,8 @@ class ListedExtensionsSerializer(serializers.ModelSerializer):
# 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'),
'permissions': matching_file.metadata.get('permissions'),
# FIXME? should we instead list platforms of matching_file?
'platforms': [platform.slug for platform in matching_version.platforms.all()],
# TODO: handle copyright
'tags': [str(tag) for tag in matching_version.tags.all()],

View File

@ -350,8 +350,10 @@ class DraftExtensionView(
"""Return initial values for the version, based on the file."""
initial = super().get_initial()
if self.version:
initial['file'] = self.version.file
initial.update(**self.version.file.parsed_version_fields)
# FIXME? double-check that looking at the first file makes sense
file = self.version.files.all()[0]
initial['file'] = file
initial.update(**file.parsed_version_fields)
return initial
def get_context_data(self, form=None, extension_form=None, **kwargs):

View File

@ -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 %}
<div class="d-flex mt-4">
<h2>
{% with text_ratings_count=extension.text_ratings_count %}

View File

@ -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()

View File

@ -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()
@ -87,7 +86,7 @@ class TestTasks(TestCase):
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()