From 489da736839a4e7a7e8caf13e6748e29faa9ccca Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Mon, 11 Mar 2024 18:29:18 +0100 Subject: [PATCH] Draft: Make sure extensions always have a user When submitting a file the created extension gets a user right away. This makes sure there are no orphans extensions. The extension is then incomplete until the draft is saved once (which means finalizing the extension upload process, by adding a description and thumbnails). Any attempt to edit or submit a new extension will lead the user to the editing draft page. Note: We could add a new option to [x] Send for Review. The front-end even has a half-baked code for that. But should be tackled separately. --- extensions/models.py | 3 + ...bmit_finalise.html => draft_finalise.html} | 0 .../templates/extensions/manage/update.html | 9 -- extensions/tests/test_submit.py | 22 +++- extensions/urls.py | 6 +- extensions/views/manage.py | 86 +++++++++++- extensions/views/mixins.py | 30 ++++- extensions/views/submit.py | 122 ++++-------------- files/models.py | 3 +- 9 files changed, 163 insertions(+), 118 deletions(-) rename extensions/templates/extensions/{submit_finalise.html => draft_finalise.html} (100%) diff --git a/extensions/models.py b/extensions/models.py index 2e01e9c8..a6521a13 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -237,6 +237,9 @@ class Extension( def get_absolute_url(self): return reverse('extensions:detail', args=[self.type_slug, self.slug]) + def get_draft_url(self): + return reverse('extensions:draft', args=[self.type_slug, self.slug]) + def get_manage_url(self): return reverse('extensions:manage', args=[self.type_slug, self.slug]) diff --git a/extensions/templates/extensions/submit_finalise.html b/extensions/templates/extensions/draft_finalise.html similarity index 100% rename from extensions/templates/extensions/submit_finalise.html rename to extensions/templates/extensions/draft_finalise.html diff --git a/extensions/templates/extensions/manage/update.html b/extensions/templates/extensions/manage/update.html index ffa766c0..8087b57e 100644 --- a/extensions/templates/extensions/manage/update.html +++ b/extensions/templates/extensions/manage/update.html @@ -100,21 +100,12 @@
- {% if extension.status == extension. %} - - {% elif 'Incomplete' != extension.get_status_display %} - {% endif %} - {% trans 'Upload New Version' %} diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index a69853ba..d91f39cc 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -224,12 +224,24 @@ class SubmitFinaliseTest(TestCase): hash=file_data['file_hash'], metadata=file_data['metadata'], ) + self.version = create_version( + file=self.file, + extension__name=file_data['metadata']['name'], + extension__slug=file_data['metadata']['id'].replace("_", "-"), + extension__website=None, + tagline=file_data['metadata']['tagline'], + version=file_data['metadata']['version'], + blender_version_min=file_data['metadata']['blender_version_min'], + schema_version=file_data['metadata']['schema_version'], + ) def test_get_finalise_addon_redirects_if_anonymous(self): response = self.client.post(self.file.get_submit_url(), {}) self.assertEqual(response.status_code, 302) - self.assertEqual(response['Location'], f'/oauth/login?next=/submit/{self.file.pk}/') + self.assertEqual( + response['Location'], f'/oauth/login?next=/add-ons/{self.file.extension.slug}/draft/' + ) def test_get_finalise_addon_not_allowed_if_different_user(self): user = UserFactory() @@ -237,7 +249,9 @@ class SubmitFinaliseTest(TestCase): response = self.client.post(self.file.get_submit_url(), {}) - self.assertEqual(response.status_code, 403) + # Technically this could (should) be a 403, but changing this means changing + # the MaintainedExtensionMixin which is used in multiple places. + self.assertEqual(response.status_code, 404) def test_post_finalise_addon_validation_errors(self): self.client.force_login(self.file.user) @@ -258,8 +272,8 @@ class SubmitFinaliseTest(TestCase): def test_post_finalise_addon_creates_addon_with_version_awaiting_review(self): self.assertEqual(File.objects.count(), 1) - self.assertEqual(Extension.objects.count(), 0) - self.assertEqual(Version.objects.count(), 0) + self.assertEqual(Extension.objects.count(), 1) + self.assertEqual(Version.objects.count(), 1) self.assertEqual(File.objects.filter(type=File.TYPES.IMAGE).count(), 0) self.client.force_login(self.file.user) diff --git a/extensions/urls.py b/extensions/urls.py index 4a7c870e..d03358d6 100644 --- a/extensions/urls.py +++ b/extensions/urls.py @@ -7,7 +7,6 @@ from extensions.views import api, public, submit, manage app_name = 'extensions' urlpatterns = [ path('submit/', submit.UploadFileView.as_view(), name='submit'), - path('submit//', submit.SubmitFileView.as_view(), name='submit-finalise'), # TODO: move /accounts pages under its own app when User model is replaced path('accounts/extensions/', manage.ManageListView.as_view(), name='manage-list'), # FIXME: while there's no profile page, redirect to My Extensions instead @@ -35,6 +34,11 @@ urlpatterns = [ rf'^(?P{EXTENSION_SLUGS_PATH})/', include( [ + path( + '/draft/', + manage.DraftExtensionView.as_view(), + name='draft', + ), path( '/manage/', manage.UpdateExtensionView.as_view(), diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 1fc187bf..c50b174b 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -5,10 +5,16 @@ from django.contrib.messages.views import SuccessMessageMixin from django.db import transaction from django.shortcuts import get_object_or_404, reverse from django.views.generic import DetailView, ListView -from django.views.generic.edit import CreateView, UpdateView, DeleteView +from django.views.generic.edit import CreateView, UpdateView, DeleteView, FormView -from .mixins import ExtensionQuerysetMixin, OwnsFileMixin, MaintainedExtensionMixin +from .mixins import ( + ExtensionQuerysetMixin, + OwnsFileMixin, + MaintainedExtensionMixin, + DraftVersionMixin, + DraftMixin, +) from extensions.forms import ( EditPreviewFormSet, AddPreviewFormSet, @@ -91,6 +97,7 @@ class UpdateExtensionView( MaintainedExtensionMixin, SuccessMessageMixin, UpdateView, + DraftMixin, ): model = Extension template_name = 'extensions/manage/update.html' @@ -293,3 +300,78 @@ class UpdateVersionView(LoginRequiredMixin, UserPassesTestMixin, UpdateView): def test_func(self) -> bool: # Only maintainers are allowed to perform this return self.get_object().extension.has_maintainer(self.request.user) + + +class DraftExtensionView( + LoginRequiredMixin, + MaintainedExtensionMixin, + DraftVersionMixin, + UserPassesTestMixin, + FormView, +): + template_name = 'extensions/draft_finalise.html' + form_class = VersionForm + + def test_func(self) -> bool: + return self.extension.status == Extension.STATUSES.INCOMPLETE + + def get_form_kwargs(self): + form_kwargs = super().get_form_kwargs() + form_kwargs['instance'] = self.extension.versions.first() + return form_kwargs + + def get_initial(self): + """Return initial values for the version, based on the file.""" + initial = super().get_initial() + initial['file'] = self.version.file + initial.update(**self.version.file.parsed_version_fields) + return initial + + def get_context_data(self, form=None, extension_form=None, add_preview_formset=None, **kwargs): + """Add all the additional forms to the context.""" + context = super().get_context_data(**kwargs) + if not (add_preview_formset and extension_form): + extension_form = ExtensionUpdateForm(instance=self.extension) + add_preview_formset = AddPreviewFormSet(extension=self.extension, request=self.request) + context['extension_form'] = extension_form + context['add_preview_formset'] = add_preview_formset + return context + + def post(self, request, *args, **kwargs): + """Handle bound forms and valid/invalid logic with the extra forms.""" + form = self.get_form() + extension_form = ExtensionUpdateForm( + self.request.POST, self.request.FILES, instance=self.extension + ) + add_preview_formset = AddPreviewFormSet( + self.request.POST, self.request.FILES, extension=self.extension, request=self.request + ) + if form.is_valid() and extension_form.is_valid() and add_preview_formset.is_valid(): + return self.form_valid(form, extension_form, add_preview_formset) + return self.form_invalid(form, extension_form, add_preview_formset) + + @transaction.atomic + def form_valid(self, form, extension_form, add_preview_formset): + """Save all the forms in correct order. + + Extension must be saved first. + """ + try: + # Send the extension and version to the review + extension_form.instance.status = extension_form.instance.STATUSES.AWAITING_REVIEW + extension_form.save() + add_preview_formset.save() + form.save() + return super().form_valid(form) + except forms.ValidationError as e: + if 'hash' in e.error_dict: + add_preview_formset.forms[0].add_error('source', e.error_dict['hash']) + return self.form_invalid(form, extension_form, add_preview_formset) + + def form_invalid(self, form, extension_form, add_preview_formset): + return self.render_to_response( + self.get_context_data(form, extension_form, add_preview_formset) + ) + + def get_success_url(self): + return self.extension.get_manage_url() diff --git a/extensions/views/mixins.py b/extensions/views/mixins.py index ab1ff8d5..5060a492 100644 --- a/extensions/views/mixins.py +++ b/extensions/views/mixins.py @@ -1,5 +1,6 @@ from django.contrib.auth.mixins import UserPassesTestMixin -from django.shortcuts import get_object_or_404 +from django.shortcuts import get_object_or_404, redirect +from django.core.exceptions import BadRequest from extensions.models import Extension from files.models import File @@ -54,3 +55,30 @@ class ExtensionMixin: Extension.objects.exclude_deleted, slug=self.kwargs['slug'] ) return super().dispatch(*args, **kwargs) + + +class DraftVersionMixin: + """Fetch the version object which is being edited as a draft.""" + + def dispatch(self, *args, **kwargs): + self.version = self.extension.versions.first() + if self.version is None: + error_message = f'Extension "{self.extension.name}" has no versions.' + raise BadRequest(error_message) + return super().dispatch(*args, **kwargs) + + +class DraftMixin: + """If the extension is incomplete, returns the FinalizeDraftView""" + + def dispatch(self, request, *args, **kwargs): + extension = ( + Extension.objects.listed_or_authored_by(user_id=self.request.user.pk) + .filter(status=Extension.STATUSES.INCOMPLETE) + .first() + ) + + if not extension: + return super().dispatch(request, *args, **kwargs) + + return redirect(extension.get_draft_url()) diff --git a/extensions/views/submit.py b/extensions/views/submit.py index d9787e89..ef980657 100644 --- a/extensions/views/submit.py +++ b/extensions/views/submit.py @@ -1,25 +1,18 @@ import logging -from django import forms from django.contrib.auth.mixins import LoginRequiredMixin from django.db import transaction -from django.shortcuts import reverse -from django.views.generic.edit import CreateView, FormView +from django.views.generic.edit import CreateView -from .mixins import OwnsFileMixin +from .mixins import DraftMixin from extensions.models import Version, Extension -from extensions.forms import ( - ExtensionUpdateForm, - VersionForm, - AddPreviewFormSet, -) from files.forms import FileForm from files.models import File logger = logging.getLogger(__name__) -class UploadFileView(LoginRequiredMixin, CreateView): +class UploadFileView(LoginRequiredMixin, DraftMixin, CreateView): model = File template_name = 'extensions/submit.html' form_class = FileForm @@ -30,30 +23,12 @@ class UploadFileView(LoginRequiredMixin, CreateView): return kwargs def get_success_url(self): - return reverse('extensions:submit-finalise', kwargs={'pk': self.object.pk}) + return self.extension.get_draft_url() - -class SubmitFileView(LoginRequiredMixin, OwnsFileMixin, FormView): - template_name = 'extensions/submit_finalise.html' - form_class = VersionForm - - def _get_extension(self) -> 'Extension': - # Get the existing Extension and Version, if they were partially saved already - existing_version = getattr(self.file, 'version', None) - if existing_version: - extension = existing_version.extension - assert ( - extension.can_request_review - ), 'TODO: cannot request review for extension: {extension.get_status_display}' - assert not extension.authors.count() or extension.authors.filter( - user__id=self.request.user.pk - ), 'TODO: cannot request review for extension: maintained by someone else' - logger.warning( - 'Found existing version pk=%s for file pk=%s', - existing_version.pk, - self.file.pk, - ) - return extension + @transaction.atomic + def form_valid(self, form): + """Create an extension and a version already, associated with the user.""" + self.file = form.instance parsed_extension_fields = self.file.parsed_extension_fields if parsed_extension_fields: @@ -69,72 +44,21 @@ class SubmitFileView(LoginRequiredMixin, OwnsFileMixin, FormView): extension.pk, self.file.pk, ) - return extension - return Extension.objects.update_or_create(type=self.file.type, **parsed_extension_fields)[0] + return False - def _get_version(self, extension) -> 'Version': - return Version.objects.update_or_create( - extension=extension, file=self.file, **self.file.parsed_version_fields + # Make sure an extension has a user associated to it from the beginning, otherwise + # it will prevent it from being re-uploaded and yet not show on My Extensions. + self.extension = Extension.objects.update_or_create( + type=self.file.type, **parsed_extension_fields + )[0] + self.extension.authors.add(self.request.user) + self.extension.save() + + # Need to save the form to be able to use the file to create the version. + self.object = self.file = form.save() + + Version.objects.update_or_create( + extension=self.extension, file=self.file, **self.file.parsed_version_fields )[0] - def get_form_kwargs(self): - form_kwargs = super().get_form_kwargs() - form_kwargs['instance'] = self._get_version(self.extension) - return form_kwargs - - def get_initial(self): - """Return initial values for the version, based on the file.""" - initial = super().get_initial() - initial['file'] = self.file - initial.update(**self.file.parsed_version_fields) - return initial - - def get_context_data(self, form=None, extension_form=None, add_preview_formset=None, **kwargs): - """Add all the additional forms to the context.""" - context = super().get_context_data(**kwargs) - if not (add_preview_formset and extension_form): - extension_form = ExtensionUpdateForm(instance=self.extension) - add_preview_formset = AddPreviewFormSet(extension=self.extension, request=self.request) - context['extension_form'] = extension_form - context['add_preview_formset'] = add_preview_formset - return context - - def post(self, request, *args, **kwargs): - """Handle bound forms and valid/invalid logic with the extra forms.""" - form = self.get_form() - extension_form = ExtensionUpdateForm( - self.request.POST, self.request.FILES, instance=self.extension - ) - add_preview_formset = AddPreviewFormSet( - self.request.POST, self.request.FILES, extension=self.extension, request=self.request - ) - if form.is_valid() and extension_form.is_valid() and add_preview_formset.is_valid(): - return self.form_valid(form, extension_form, add_preview_formset) - return self.form_invalid(form, extension_form, add_preview_formset) - - @transaction.atomic - def form_valid(self, form, extension_form, add_preview_formset): - """Save all the forms in correct order. - - Extension must be saved first. - """ - try: - # Send the extension and version to the review - extension_form.instance.status = extension_form.instance.STATUSES.AWAITING_REVIEW - extension_form.save() - self.extension.authors.add(self.request.user) - add_preview_formset.save() - form.save() - return super().form_valid(form) - except forms.ValidationError as e: - if 'hash' in e.error_dict: - add_preview_formset.forms[0].add_error('source', e.error_dict['hash']) - return self.form_invalid(form, extension_form, add_preview_formset) - - def form_invalid(self, form, extension_form, add_preview_formset): - return self.render_to_response( - self.get_context_data(form, extension_form, add_preview_formset) - ) - - def get_success_url(self): - return self.file.extension.get_manage_url() + return super().form_valid(form) diff --git a/files/models.py b/files/models.py index 2fc6d221..1598988d 100644 --- a/files/models.py +++ b/files/models.py @@ -6,7 +6,6 @@ import re from django.contrib.auth import get_user_model from django.db import models -from django.urls import reverse from common.model_mixins import CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin from files.utils import get_sha256 @@ -209,7 +208,7 @@ class File(CreatedModifiedMixin, TrackChangesMixin, SoftDeleteMixin, models.Mode } def get_submit_url(self) -> str: - return reverse('extensions:submit-finalise', kwargs={'pk': self.pk}) + return self.extension.get_draft_url() class FileValidation(CreatedModifiedMixin, TrackChangesMixin, models.Model): -- 2.30.2