Skip to content

Commit

Permalink
Refactor eula_policy action write path to be cleaner & safer (#22586)
Browse files Browse the repository at this point in the history 
* Refactor eula_policy action write path to be cleaner & safer

https://www.django-rest-framework.org/api-guide/viewsets/#routing-additional-http-methods-for-extra-actions

---------

Co-authored-by: Andrew Williamson <[email protected]>
  • Loading branch information
@diox @eviljeff @KevinMind
2 people authored and KevinMind committed Aug 22, 2024
1 parent 813b72b commit 2fd5e8d
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 25 deletions.
90 changes: 74 additions & 16 deletions src/olympia/addons/tests/test_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3015,7 +3015,7 @@ def test_developer_version_serializer_used_for_authors(self):
# not logged in
assert 'source' not in self.client.get(self.url).data

user = UserProfile.objects.create(username='user')
user = user_factory()
self.client.login_api(user)

# logged in but not an author
Expand Down Expand Up @@ -5022,7 +5022,7 @@ def test_developer_version_serializer_used_for_authors(self):
assert 'source' not in self.client.get(self.url).data['results'][0]
assert 'source' not in self.client.get(self.url).data['results'][1]

user = UserProfile.objects.create(username='user')
user = user_factory()
self.client.login_api(user)

# logged in but not an author
Expand All @@ -5045,6 +5045,7 @@ def setUp(self):
guid=generate_addon_guid(), name='My Addôn', slug='my-addon'
)
self.url = reverse_ns('addon-eula-policy', kwargs={'pk': self.addon.pk})
self.user = user_factory(read_dev_agreement=self.days_ago(1))

def test_url(self):
self.detail_url = reverse_ns('addon-detail', kwargs={'pk': self.addon.pk})
Expand Down Expand Up @@ -5076,8 +5077,7 @@ def test_eula_and_policy(self):
assert data['privacy_policy'] == {'en-US': 'My Prïvacy, My Policy'}

def test_update_non_author(self):
user = UserProfile.objects.create(username='user')
self.client.login_api(user)
self.client.login_api(self.user)
response = self.client.patch(
self.url,
{
Expand Down Expand Up @@ -5136,9 +5136,8 @@ def test_update_anonymous(self):
assert response.status_code == 401

def test_update(self):
user = UserProfile.objects.create(username='user')
AddonUser.objects.create(user=user, addon=self.addon)
self.client.login_api(user)
AddonUser.objects.create(user=self.user, addon=self.addon)
self.client.login_api(self.user)
assert not ActivityLog.objects.filter(
action=amo.LOG.EDIT_PROPERTIES.id
).exists()
Expand Down Expand Up @@ -5178,9 +5177,8 @@ def test_update(self):
].details == ['eula', 'privacy_policy']

def test_update_put(self):
user = UserProfile.objects.create(username='user')
AddonUser.objects.create(user=user, addon=self.addon)
self.client.login_api(user)
AddonUser.objects.create(user=self.user, addon=self.addon)
self.client.login_api(self.user)
response = self.client.put(
self.url,
{
Expand All @@ -5195,12 +5193,73 @@ def test_update_put(self):
)
assert response.status_code == 405

def test_update_author_not_read_dev_agreement(self):
AddonUser.objects.create(user=self.user, addon=self.addon)
self.user.update(read_dev_agreement=None)
self.client.login_api(self.user)
response = self.client.patch(
self.url,
{
'eula': {
'en-US': 'My Updated Add-on EULA in English',
'fr': 'Mes Conditions générales d’utilisation',
},
'privacy_policy': {
'en-US': 'My privacy policy',
},
},
)
assert response.status_code == 403
self.addon.reload()
assert not self.addon.eula
assert not self.addon.privacy_policy

def test_update_reviewer_not_author(self):
self.grant_permission(self.user, 'Addons:Review')
self.client.login_api(self.user)
response = self.client.patch(
self.url,
{
'eula': {
'en-US': 'My Updated Add-on EULA in English',
'fr': 'Mes Conditions générales d’utilisation',
},
'privacy_policy': {
'en-US': 'My privacy policy',
},
},
)
assert response.status_code == 403
self.addon.reload()
assert not self.addon.eula
assert not self.addon.privacy_policy

def test_update_disabled(self):
AddonUser.objects.create(user=self.user, addon=self.addon)
self.client.login_api(self.user)
self.addon.update(status=amo.STATUS_DISABLED)
response = self.client.patch(
self.url,
{
'eula': {
'en-US': 'My Updated Add-on EULA in English',
'fr': 'Mes Conditions générales d’utilisation',
},
'privacy_policy': {
'en-US': 'My privacy policy',
},
},
)
assert response.status_code == 403
self.addon.reload()
assert not self.addon.eula
assert not self.addon.privacy_policy

def test_update_something_else(self):
assert self.addon.summary
original_summary = self.addon.summary
user = UserProfile.objects.create(username='user')
AddonUser.objects.create(user=user, addon=self.addon)
self.client.login_api(user)
AddonUser.objects.create(user=self.user, addon=self.addon)
self.client.login_api(self.user)
response = self.client.patch(
self.url,
{'summary': 'attempting to change the summary via wrong endpoint'},
Expand All @@ -5212,10 +5271,9 @@ def test_update_something_else(self):
assert self.addon.summary == original_summary

def test_update_on_theme(self):
user = UserProfile.objects.create(username='user')
self.addon.update(type=amo.ADDON_STATICTHEME)
AddonUser.objects.create(user=user, addon=self.addon)
self.client.login_api(user)
AddonUser.objects.create(user=self.user, addon=self.addon)
self.client.login_api(self.user)
response = self.client.patch(
self.url,
{
Expand Down
16 changes: 7 additions & 9 deletions src/olympia/addons/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
RetrieveModelMixin,
UpdateModelMixin,
)
from rest_framework.permissions import SAFE_METHODS, IsAuthenticated
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.settings import api_settings
from rest_framework.views import APIView
Expand Down Expand Up @@ -346,20 +346,18 @@ def get_georestrictions(self):

@action(
detail=True,
methods=['get', 'patch'],
serializer_class=AddonEulaPolicySerializer,
# For this action, developers use the same serializer - it only
# contains eula/privacy policy.
serializer_class_for_developers=AddonEulaPolicySerializer,
)
def eula_policy(self, request, pk=None):
kwargs = {}
if request.method in SAFE_METHODS:
method = self.retrieve
else:
kwargs['partial'] = True
method = self.update
return method(request, **kwargs)
return self.retrieve(request)

@eula_policy.mapping.patch
def update_eula_policy(self, request, pk=None):
self.permission_classes = self.write_permission_classes
return self.update(request, partial=True)

@action(detail=True)
def delete_confirm(self, request, *args, **kwargs):
Expand Down

0 comments on commit 2fd5e8d

Please sign in to comment.