-
Notifications
You must be signed in to change notification settings - Fork 30
release review #1199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: review-base
Are you sure you want to change the base?
release review #1199
Changes from all commits
a506458
60999b6
fa1a437
59bf19d
42339ba
9843753
6731f28
adb6766
00cefee
562a3b4
631291b
d3fe9c3
7848f5a
88ddc7f
5f83436
46fa16e
1e6fc6f
f6c2187
b893ad5
a85bf55
87ada42
a0fb687
04e1017
1ac0481
6821d01
7c1270f
e665e38
12defc4
38522a2
dd16ce9
0d3753f
f560775
9fdd1cc
7ae1cca
37a594e
8ad5d57
cfb09ba
ce84c0a
9fb74ad
623b935
64da51b
99eb8fc
83864e7
e8f1e3a
01349c9
3b42da5
1974378
40f44ad
09e29c4
a84219c
4f3b42d
d5eb376
a51c2c1
3cd0109
2a4b8b8
0f2eef0
271aa1a
0891dd3
826b545
bae8455
b47ddc6
94f5809
003ffd5
f249529
b6aa784
3777922
db03a91
805dbf8
2e7df1c
17b201f
c8ba27d
62751e6
70cede3
988b7e8
1ba470a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,12 @@ | ||
| from django import forms | ||
| from django.core.exceptions import ValidationError | ||
| from django.db import transaction | ||
|
|
||
| from thunderstore.account.models import ServiceAccount | ||
| from thunderstore.api.cyberstorm.services.team import ( | ||
| create_service_account, | ||
| delete_service_account, | ||
| ) | ||
| from thunderstore.core.types import UserType | ||
| from thunderstore.repository.models import Team | ||
|
|
||
|
|
@@ -16,19 +21,28 @@ def __init__(self, user: UserType, *args, **kwargs) -> None: | |
| queryset=Team.objects.filter(members__user=user, is_active=True), | ||
| ) | ||
|
|
||
| def clean_team(self) -> Team: | ||
| team = self.cleaned_data["team"] | ||
| team.ensure_can_create_service_account(self.user) | ||
| return team | ||
|
|
||
| @transaction.atomic | ||
| def save(self) -> ServiceAccount: | ||
| if self.errors: | ||
| raise ValueError("Cannot save form with errors") | ||
|
|
||
| self.api_token = "" | ||
|
|
||
| owner = self.cleaned_data["team"] | ||
| nickname = self.cleaned_data["nickname"] | ||
| (service_account, token) = ServiceAccount.create( | ||
| owner=owner, nickname=nickname, creator=self.user | ||
| ) | ||
| self.api_token = token | ||
| service_account = None | ||
|
|
||
| try: | ||
| service_account, token = create_service_account( | ||
| agent=self.user, | ||
| team=owner, | ||
| nickname=nickname, | ||
| ) | ||
| self.api_token = token | ||
| except ValidationError as e: | ||
| self.add_error(None, e) | ||
| raise ValidationError(self.errors) | ||
|
|
||
| return service_account | ||
|
|
||
|
|
||
|
|
@@ -40,13 +54,17 @@ def __init__(self, user: UserType, *args, **kwargs) -> None: | |
| queryset=ServiceAccount.objects.filter(owner__members__user=user), | ||
| ) | ||
|
|
||
| def clean_service_account(self) -> ServiceAccount: | ||
| def save(self) -> None: | ||
| if self.errors: | ||
| raise ValueError("Cannot save form with errors") | ||
|
Comment on lines
+58
to
+59
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this actually necessary/useful to guard against? Asking because I'd expect Django to have this built in if yes |
||
|
|
||
| service_account = self.cleaned_data["service_account"] | ||
| service_account.owner.ensure_can_delete_service_account(self.user) | ||
| return service_account | ||
|
|
||
| def save(self) -> None: | ||
| self.cleaned_data["service_account"].delete() | ||
| try: | ||
| delete_service_account(self.user, service_account) | ||
| except ValidationError as e: | ||
| self.add_error(None, e) | ||
| raise ValidationError(self.errors) | ||
|
|
||
|
|
||
| class EditServiceAccountForm(forms.Form): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import pytest | ||
| from django.core.exceptions import ValidationError | ||
| from rest_framework.authtoken.models import Token | ||
|
|
||
| from thunderstore.account.forms import ( | ||
|
|
@@ -46,6 +47,24 @@ def test_service_account_create(user, team): | |
| ) | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_service_account_create_error_on_save(user, team): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be a good idea to explain why the test should fail |
||
| TeamMember.objects.create( | ||
| user=user, | ||
| team=team, | ||
| role=TeamMemberRole.owner, | ||
| ) | ||
|
|
||
| form = CreateServiceAccountForm( | ||
| user, | ||
| data={"team": team, "nickname": "x" * 1000}, | ||
| ) | ||
|
|
||
| assert form.is_valid() is False | ||
| with pytest.raises(ValueError): | ||
| form.save() | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_service_account_create_nickname_too_long(user, team): | ||
| TeamMember.objects.create( | ||
|
|
@@ -91,9 +110,11 @@ def test_service_account_create_not_owner(user, team): | |
| user, | ||
| data={"team": team, "nickname": "Nickname"}, | ||
| ) | ||
| with pytest.raises(ValidationError): | ||
| form.save() | ||
| assert form.is_valid() is False | ||
| assert len(form.errors["team"]) == 1 | ||
| assert form.errors["team"][0] == "Must be an owner to create a service account" | ||
| assert len(form.errors["__all__"]) == 1 | ||
| assert form.errors["__all__"][0] == "Must be an owner to create a service account" | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
|
|
@@ -129,6 +150,20 @@ def test_service_account_delete_not_member(service_account): | |
| ) | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_service_account_delete_error_on_save(service_account): | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be a good idea to explain why the test should fail |
||
| user = UserFactory.create() | ||
|
|
||
| form = DeleteServiceAccountForm( | ||
| user, | ||
| data={"service_account": service_account}, | ||
| ) | ||
|
|
||
| assert form.is_valid() is False | ||
| with pytest.raises(ValueError): | ||
| form.save() | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_service_account_delete_not_owner(service_account): | ||
| user = UserFactory.create() | ||
|
|
@@ -141,12 +176,11 @@ def test_service_account_delete_not_owner(service_account): | |
| user, | ||
| data={"service_account": service_account}, | ||
| ) | ||
| with pytest.raises(ValidationError): | ||
| form.save() | ||
| assert form.is_valid() is False | ||
| assert len(form.errors["service_account"]) == 1 | ||
| assert ( | ||
| form.errors["service_account"][0] | ||
| == "Must be an owner to delete a service account" | ||
| ) | ||
| assert len(form.errors["__all__"]) == 1 | ||
| assert form.errors["__all__"][0] == "Must be an owner to delete a service account" | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| from typing import Optional | ||
|
|
||
| from rest_framework import serializers | ||
|
|
||
| from thunderstore.api.cyberstorm.serializers.community import ( | ||
| CyberstormPackageCategorySerializer, | ||
| ) | ||
| from thunderstore.api.cyberstorm.serializers.team import CyberstormTeamMemberSerializer | ||
| from thunderstore.community.models import Community | ||
| from thunderstore.repository.models import Namespace, Package, PackageVersion | ||
|
|
||
|
|
@@ -55,3 +58,40 @@ class CyberstormPackagePreviewSerializer(serializers.Serializer): | |
| rating_count = serializers.IntegerField(min_value=0) | ||
| size = serializers.IntegerField(min_value=0) | ||
| datetime_created = serializers.DateTimeField() | ||
|
|
||
|
|
||
| class CyberstormPackageDependencySerializer(serializers.Serializer): | ||
| description = serializers.SerializerMethodField() | ||
| icon_url = serializers.SerializerMethodField() | ||
| is_active = serializers.BooleanField(source="is_effectively_active") | ||
| name = serializers.CharField() | ||
| namespace = serializers.CharField(source="package.namespace.name") | ||
| version_number = serializers.CharField() | ||
| is_removed = serializers.SerializerMethodField() | ||
|
|
||
| def get_is_removed(self, obj: PackageVersion) -> bool: | ||
| package_is_removed = not ( | ||
| obj.package.is_active and obj.package_has_active_versions | ||
| ) | ||
| if package_is_removed: | ||
| return True | ||
| return not obj.is_active | ||
|
|
||
| def get_description(self, obj: PackageVersion) -> str: | ||
| return ( | ||
| obj.description | ||
| if obj.is_effectively_active | ||
| else "This package has been removed." | ||
| ) | ||
|
|
||
| def get_icon_url(self, obj: PackageVersion) -> Optional[str]: | ||
| return obj.icon.url if obj.is_effectively_active else None | ||
|
Comment on lines
+72
to
+88
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like it could generate lots of queries if this serializer is used in a list context, but I don't know if that's the case (and perhaps we had query count tests already protecting against it?) |
||
|
|
||
|
|
||
| class CyberstormPackageTeamSerializer(serializers.Serializer): | ||
| """ | ||
| Minimal information to present the team on package detail view. | ||
| """ | ||
|
|
||
| name = serializers.CharField() | ||
| members = CyberstormTeamMemberSerializer(many=True, source="public_members") | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| from rest_framework import serializers | ||
|
|
||
| from thunderstore.api.cyberstorm.serializers.package import ( | ||
| CyberstormPackageTeamSerializer, | ||
| ) | ||
| from thunderstore.api.cyberstorm.serializers.utils import EmptyStringAsNoneField | ||
|
|
||
|
|
||
| class PackageVersionResponseSerializer(serializers.Serializer): | ||
| """ | ||
| Data shown on package version detail view. | ||
|
|
||
| Expects an annotated and customized CustomListing object. | ||
| """ | ||
|
|
||
| datetime_created = serializers.DateTimeField(source="date_created") | ||
| dependency_count = serializers.IntegerField(min_value=0) | ||
| description = serializers.CharField() | ||
| download_count = serializers.IntegerField(source="downloads", min_value=0) | ||
| download_url = serializers.CharField(source="full_download_url") | ||
| full_version_name = serializers.CharField() | ||
| icon_url = serializers.CharField(source="icon.url") | ||
| install_url = serializers.CharField() | ||
| name = serializers.CharField() | ||
| version_number = serializers.CharField() | ||
| namespace = serializers.CharField(source="package.namespace.name") | ||
| size = serializers.IntegerField(min_value=0, source="file_size") | ||
| team = CyberstormPackageTeamSerializer(source="owner") | ||
| website_url = EmptyStringAsNoneField() |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| from rest_framework import serializers | ||
|
|
||
|
|
||
| class EmptyStringAsNoneField(serializers.Field): | ||
| """ | ||
| Serialize empty string to None and deserialize vice versa. | ||
| """ | ||
|
|
||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.allow_null = True | ||
| self.allow_blank = True | ||
|
|
||
| def to_representation(self, value): | ||
| return None if value == "" else value | ||
|
|
||
| def to_internal_value(self, data): | ||
| return "" if data is None else data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually necessary/useful to guard against? Asking because I'd expect Django to have this built in if yes