API: Upload new version of an extension #138
89
extensions/tests/test_api.py
Normal file
@ -0,0 +1,89 @@
|
|||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
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
|
||||||
|
|
||||||
|
from extensions.models import Version
|
||||||
|
from tokens.models import UserToken
|
||||||
|
|
||||||
|
|
||||||
|
TEST_FILES_DIR = Path(__file__).resolve().parent / 'files'
|
||||||
|
|
||||||
|
|
||||||
|
class VersionUploadAPITest(APITestCase):
|
||||||
dfelinto marked this conversation as resolved
Outdated
|
|||||||
|
def setUp(self):
|
||||||
|
self.user = UserFactory()
|
||||||
|
self.token = UserToken.objects.create(user=self.user)
|
||||||
|
|
||||||
|
self.client = APIClient()
|
||||||
|
self.version = create_approved_version(
|
||||||
|
extension__extension_id="amaranth",
|
||||||
|
version="1.0.7",
|
||||||
|
file__user=self.user,
|
||||||
|
)
|
||||||
|
self.extension = self.version.extension
|
||||||
|
self.upload_url = reverse('extensions:upload-extension-version')
|
||||||
|
self.file_path = TEST_FILES_DIR / "amaranth-1.0.8.zip"
|
||||||
|
|
||||||
|
def test_version_upload_extension_not_maintained_by_user(self):
|
||||||
|
other_user = UserFactory()
|
||||||
|
other_extension = create_approved_version(
|
||||||
|
extension__extension_id='other_extension', file__user=other_user
|
||||||
|
).extension
|
||||||
|
|
||||||
|
with open(self.file_path, 'rb') as version_file:
|
||||||
|
response = self.client.post(
|
||||||
|
self.upload_url,
|
||||||
|
{
|
||||||
|
'version_file': version_file,
|
||||||
|
'extension_id': other_extension.extension_id,
|
||||||
|
'release_notes': 'These are the release notes',
|
||||||
|
},
|
||||||
|
format='multipart',
|
||||||
|
HTTP_AUTHORIZATION=f'Bearer {self.token.token}',
|
||||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
how does this part work now? we are not keeping a how does this part work now? we are not keeping a `token` field on the object anymore as far as i can tell
|
|||||||
|
)
|
||||||
|
|
||||||
|
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.full_name}"',
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_version_upload_extension_does_not_exist(self):
|
||||||
|
extension_name = 'extension_do_not_exist'
|
||||||
|
with open(self.file_path, 'rb') as version_file:
|
||||||
|
response = self.client.post(
|
||||||
|
self.upload_url,
|
||||||
|
{
|
||||||
|
'version_file': version_file,
|
||||||
|
'extension_id': extension_name,
|
||||||
|
'release_notes': 'These are the release notes',
|
||||||
|
},
|
||||||
|
format='multipart',
|
||||||
|
HTTP_AUTHORIZATION=f'Bearer {self.token.token}',
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
|
||||||
|
self.assertEqual(response.data['message'], f'Extension "{extension_name}" not found')
|
||||||
|
|
||||||
|
def test_version_upload_success(self):
|
||||||
|
self.assertEqual(Version.objects.filter(extension=self.extension).count(), 1)
|
||||||
|
with open(self.file_path, 'rb') as version_file:
|
||||||
|
response = self.client.post(
|
||||||
|
self.upload_url,
|
||||||
|
{
|
||||||
|
'version_file': version_file,
|
||||||
|
'extension_id': self.extension.extension_id,
|
||||||
|
'release_notes': 'These are the release notes',
|
||||||
|
},
|
||||||
|
format='multipart',
|
||||||
|
HTTP_AUTHORIZATION=f'Bearer {self.token.token}',
|
||||||
|
)
|
||||||
|
|
||||||
|
self.assertEqual(response.status_code, status.HTTP_201_CREATED)
|
||||||
|
self.assertIn('version_id', response.data)
|
||||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
this assert must be failing now after the changes to the response this assert must be failing now after the changes to the response
|
|||||||
|
self.assertEqual(Version.objects.filter(extension=self.extension).count(), 2)
|
@ -16,6 +16,11 @@ urlpatterns = [
|
|||||||
),
|
),
|
||||||
# API
|
# API
|
||||||
path('api/v1/extensions/', api.ExtensionsAPIView.as_view(), name='api'),
|
path('api/v1/extensions/', api.ExtensionsAPIView.as_view(), name='api'),
|
||||||
|
path(
|
||||||
|
'api/v1/version-upload/',
|
||||||
Oleg-Komarov marked this conversation as resolved
Outdated
Oleg-Komarov
commented
sorry, haven't thought of this before, let's restructure the parameters: make them more in line with REST conventions, and get a nice bonus for logs transparency the url becomes sorry, haven't thought of this before, let's restructure the parameters: make them more in line with REST conventions, and get a nice bonus for logs transparency
the url becomes `api/v1/extensions/<extension_id>/versions/new/`, and the `extension_id` in the body is not needed
|
|||||||
|
api.UploadExtensionVersionView.as_view(),
|
||||||
|
name='upload-extension-version',
|
||||||
|
),
|
||||||
# Public pages
|
# Public pages
|
||||||
path('', public.HomeView.as_view(), name='home'),
|
path('', public.HomeView.as_view(), name='home'),
|
||||||
path('search/', public.SearchView.as_view(), name='search'),
|
path('search/', public.SearchView.as_view(), name='search'),
|
||||||
|
@ -2,14 +2,17 @@ import logging
|
|||||||
|
|
||||||
from rest_framework.permissions import AllowAny
|
from rest_framework.permissions import AllowAny
|
||||||
from rest_framework.response import Response
|
from rest_framework.response import Response
|
||||||
from rest_framework import serializers
|
from rest_framework import serializers, status
|
||||||
from rest_framework.views import APIView
|
from rest_framework.views import APIView
|
||||||
|
from rest_framework.permissions import IsAuthenticated
|
||||||
from drf_spectacular.utils import OpenApiParameter, extend_schema
|
from drf_spectacular.utils import OpenApiParameter, extend_schema
|
||||||
from django.core.exceptions import ValidationError
|
from django.core.exceptions import ValidationError
|
||||||
|
|
||||||
from common.compare import is_in_version_range, version
|
from common.compare import is_in_version_range, version
|
||||||
from extensions.models import Extension, Platform
|
from extensions.models import Extension, Platform, Version
|
||||||
from extensions.utils import clean_json_dictionary_from_optional_fields
|
from extensions.utils import clean_json_dictionary_from_optional_fields
|
||||||
|
from extensions.views.manage import NewVersionView
|
||||||
|
from files.forms import FileFormSkipAgreed
|
||||||
|
|
||||||
|
|
||||||
from constants.base import (
|
from constants.base import (
|
||||||
@ -151,3 +154,86 @@ class ExtensionsAPIView(APIView):
|
|||||||
'version': 'v1',
|
'version': 'v1',
|
||||||
}
|
}
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class ExtensionVersionSerializer(serializers.Serializer):
|
||||||
|
extension_id = serializers.CharField(max_length=255)
|
||||||
|
version_file = serializers.FileField()
|
||||||
|
release_notes = serializers.CharField(max_length=1024, required=False)
|
||||||
|
|
||||||
|
|
||||||
|
class UploadExtensionVersionView(APIView):
|
||||||
|
permission_classes = [IsAuthenticated]
|
||||||
|
|
||||||
|
@extend_schema(
|
||||||
|
request=ExtensionVersionSerializer,
|
||||||
|
responses={201: 'Extension version uploaded successfully!'},
|
||||||
|
)
|
||||||
|
def post(self, request, *args, **kwargs):
|
||||||
|
serializer = ExtensionVersionSerializer(data=request.data)
|
||||||
|
if not serializer.is_valid():
|
||||||
|
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
|
||||||
|
|
||||||
|
user = request.user
|
||||||
|
extension_id = serializer.validated_data['extension_id']
|
||||||
|
version_file = serializer.validated_data['version_file']
|
||||||
|
release_notes = serializer.validated_data.get('release_notes', '')
|
||||||
|
|
||||||
|
extension = Extension.objects.filter(extension_id=extension_id).first()
|
||||||
|
if not extension:
|
||||||
|
return Response(
|
||||||
|
{
|
||||||
|
'message': f'Extension "{extension_id}" not found',
|
||||||
|
},
|
||||||
|
status=status.HTTP_404_NOT_FOUND,
|
||||||
|
)
|
||||||
|
|
||||||
|
if not extension.has_maintainer(user):
|
||||||
|
return Response(
|
||||||
|
{
|
||||||
|
'message': f'Extension "{extension_id}" not maintained by user "{user}"',
|
||||||
|
},
|
||||||
|
status=status.HTTP_403_FORBIDDEN,
|
||||||
|
)
|
||||||
|
|
||||||
|
try:
|
||||||
Oleg-Komarov marked this conversation as resolved
Outdated
Oleg-Komarov
commented
could we avoid this big try-except? what is the default drf error behavior? could we avoid this big try-except? what is the default drf error behavior?
we need to make sure that we are not leaking any error details in production - we will have the exception logged via sentry, and api users can't do anything useful with those details
|
|||||||
|
# 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.fields['source'].initial = version_file
|
||||||
|
|
||||||
|
if not form.is_valid():
|
||||||
|
return Response({'message': form.errors}, status=status.HTTP_400_BAD_REQUEST)
|
||||||
|
|
||||||
|
# Create the file instance
|
||||||
|
file_instance = form.save(commit=False)
|
||||||
|
file_instance.user = user
|
||||||
|
file_instance.save()
|
||||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
this save and the update_or_create below should ideally be together in the same transaction, we should wrap all db writes in one this save and the update_or_create below should ideally be together in the same transaction, we should wrap all db writes in one `with transaction.atomic():`
|
|||||||
|
|
||||||
|
# Create the version from the file
|
||||||
Oleg-Komarov
commented
btw, what's the use case for update here? why we are not just creating the object? btw, what's the use case for update here? why we are not just creating the object?
Dalai Felinto
commented
I guess I was just copying the same code we used on UploadFileView.form_valid. Which probably should be changed as well. Anyways, changing the PR to use create(), and will merge after testing. I guess I was just copying the same code we used on UploadFileView.form_valid. Which probably should be changed as well.
Anyways, changing the PR to use create(), and will merge after testing.
|
|||||||
|
version = Version.objects.update_or_create(
|
||||||
|
extension=extension,
|
||||||
|
file=file_instance,
|
||||||
|
release_notes=release_notes,
|
||||||
|
**file_instance.parsed_version_fields,
|
||||||
|
)[0]
|
||||||
|
|
||||||
|
return Response(
|
||||||
|
{
|
||||||
|
'message': 'Extension version uploaded successfully!',
|
||||||
|
'extension_id': extension_id,
|
||||||
|
'version_file': version_file.name,
|
||||||
|
'release_notes': version.release_notes,
|
||||||
|
'version_id': version.id,
|
||||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
in UI we don't expose our internal ids, do we need to return this one here? in UI we don't expose our internal ids, do we need to return this one here?
|
|||||||
|
},
|
||||||
|
status=status.HTTP_201_CREATED,
|
||||||
|
)
|
||||||
|
except Exception as e:
|
||||||
|
return Response(
|
||||||
|
{
|
||||||
|
'message': str(e),
|
||||||
|
},
|
||||||
|
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
|
||||||
|
)
|
||||||
|
@ -167,6 +167,16 @@ class FileForm(forms.ModelForm):
|
|||||||
return self.cleaned_data
|
return self.cleaned_data
|
||||||
|
|
||||||
|
|
||||||
|
class FileFormSkipAgreed(FileForm):
|
||||||
|
def __init__(self, *args, **kwargs):
|
||||||
|
super().__init__(*args, **kwargs)
|
||||||
|
self.fields['agreed_with_terms'].required = False
|
||||||
|
|
||||||
|
def clean(self):
|
||||||
|
self.cleaned_data['agreed_with_terms'] = True
|
||||||
|
super().clean()
|
||||||
|
|
||||||
|
|
||||||
class BaseMediaFileForm(forms.ModelForm):
|
class BaseMediaFileForm(forms.ModelForm):
|
||||||
class Meta:
|
class Meta:
|
||||||
model = files.models.File
|
model = files.models.File
|
||||||
|
should include a test for a non-authenticated request
returning 403 is ok: maybe not 100% correct from a purist perspective, but it serves the purpose