infrastructure/extensions-website
4
8
Fork 10
Code Issues 20 Pull Requests 2 Projects Releases Wiki Activity

API: Upload new version of an extension #138

Merged
Dalai Felinto merged 12 commits from tokens-version-api into main 2024-05-27 16:18:05 +02:00
Conversation 8 Commits 12 Files Changed 7 +28 -36
1 changed files with 28 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1baf3b9bb8 - Show all commits

64
extensions/views/api.py
View File

@ -197,44 +197,36 @@ class UploadExtensionVersionView(APIView):
status=status.HTTP_403_FORBIDDEN, status=status.HTTP_403_FORBIDDEN,
) )
Oleg-Komarov marked this conversation as resolved Outdated
Oleg-Komarov commented 2024-05-21 12:30:58 +02:00
Outdated
Review
Copy Link

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

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
try: # Create a NewVersionView instance to handle file creation
# Create a NewVersionView instance to handle file creation new_version_view = NewVersionView(request=request, extension=extension)
new_version_view = NewVersionView(request=request, extension=extension)
# Pass the version_file to the form # Pass the version_file to the form
form = new_version_view.get_form(FileFormSkipAgreed) form = new_version_view.get_form(FileFormSkipAgreed)
form.fields['source'].initial = version_file form.fields['source'].initial = version_file
if not form.is_valid(): if not form.is_valid():
return Response({'message': form.errors}, status=status.HTTP_400_BAD_REQUEST) return Response({'message': form.errors}, status=status.HTTP_400_BAD_REQUEST)
with transaction.atomic(): with transaction.atomic():
# Create the file instance # Create the file instance
file_instance = form.save(commit=False) file_instance = form.save(commit=False)
file_instance.user = user file_instance.user = user
dfelinto marked this conversation as resolved Outdated
Oleg-Komarov commented 2024-05-21 12:32:26 +02:00
Outdated
Review
Copy Link

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

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():`
file_instance.save() file_instance.save()
Oleg-Komarov commented 2024-05-27 15:59:10 +02:00
Review
Copy Link

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 2024-05-27 16:12:56 +02:00
Review
Copy Link

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.
# Create the version from the file # Create the version from the file
version = Version.objects.update_or_create( version = Version.objects.update_or_create(
extension=extension, extension=extension,
file=file_instance, file=file_instance,
release_notes=release_notes, release_notes=release_notes,
**file_instance.parsed_version_fields, **file_instance.parsed_version_fields,
)[0] )[0]
return Response( return Response(
{ {
'message': 'Extension version uploaded successfully!', 'message': 'Extension version uploaded successfully!',
'extension_id': extension_id, 'extension_id': extension_id,
'version_file': version_file.name, 'version_file': version_file.name,
'release_notes': version.release_notes, 'release_notes': version.release_notes,
dfelinto marked this conversation as resolved Outdated
Oleg-Komarov commented 2024-05-21 12:33:03 +02:00
Outdated
Review
Copy Link

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, status=status.HTTP_201_CREATED,
) )
except Exception as e:
return Response(
{
'message': str(e),
},
status=status.HTTP_500_INTERNAL_SERVER_ERROR,
)