From ee9cb1a9e394930ee83d3496cf92ff1d670e480c Mon Sep 17 00:00:00 2001 From: Wille Marcel Date: Tue, 30 Oct 2018 21:01:21 -0300 Subject: [PATCH 1/4] add view to list comments of a changeset --- README.rst | 1 + config/settings/common.py | 3 ++ .../changeset/tests/test_changeset_views.py | 49 +++++++++++++++++++ osmchadjango/changeset/urls.py | 2 +- osmchadjango/changeset/views.py | 17 ++++++- 5 files changed, 70 insertions(+), 2 deletions(-) diff --git a/README.rst b/README.rst index edc0f203..ca7cd1ce 100644 --- a/README.rst +++ b/README.rst @@ -60,6 +60,7 @@ DJANGO_NON_STAFF_USER_THROTTLE_RATE NON_STAFF_USER_THROTTLE_RATE 3/min OAUTH_REDIRECT_URI OAUTH_REDIRECT_URI http://localhost:8000/oauth-landing.html http://localhost:8000/oauth-landing.html OSMCHA_FRONTEND_VERSION OSMCHA_FRONTEND_VERSION oh-pages oh-pages DJANGO_ENABLE_CHANGESET_COMMENTS ENABLE_POST_CHANGESET_COMMENTS False False +DJANGO_OSM_COMMENTS_API_KEY OSM_COMMENTS_API_KEY '' '' ======================================= ================================= ========================================= =========================================== You can set each of these variables with: diff --git a/config/settings/common.py b/config/settings/common.py index f6058ea2..516d4348 100644 --- a/config/settings/common.py +++ b/config/settings/common.py @@ -324,6 +324,9 @@ # are reviewed ENABLE_POST_CHANGESET_COMMENTS = env('DJANGO_ENABLE_CHANGESET_COMMENTS', default=False) +# Authorization token to access the OSM-COMMENTS-API +OSM_COMMENTS_API_KEY = env('DJANGO_OSM_COMMENTS_API_KEY', default='') + # Your common stuff: Below this line define 3rd party library settings REST_FRAMEWORK = { 'DEFAULT_AUTHENTICATION_CLASSES': ( diff --git a/osmchadjango/changeset/tests/test_changeset_views.py b/osmchadjango/changeset/tests/test_changeset_views.py index 4969e9ca..cd117d39 100644 --- a/osmchadjango/changeset/tests/test_changeset_views.py +++ b/osmchadjango/changeset/tests/test_changeset_views.py @@ -1263,3 +1263,52 @@ def test_set_harmful_by_staff_user(self): ) self.assertEqual(response.status_code, 200) self.assertEqual(Changeset.objects.filter(checked=True).count(), 5) + +class TestChangesetCommentsView(APITestCase): + def setUp(self): + self.changeset = HarmfulChangesetFactory( + id=55736682, + ) + HarmfulChangesetFactory(id=1343) + self.user = User.objects.create_user( + username='test', + password='password', + email='a@a.com', + ) + UserSocialAuth.objects.create( + user=self.user, + provider='openstreetmap', + uid='123123', + ) + + def test_unauthenticated_changeset_detail_response(self): + response = self.client.get( + reverse('changeset:comment-list', args=[self.changeset.id]) + ) + self.assertEqual(response.status_code, 401) + + def test_authenticated_changeset_detail_response(self): + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('changeset:comment', args=[self.changeset.id]) + ) + + self.assertEqual(response.status_code, 200) + self.assertTrue(len(response.data) > 0) + + def test_changeset_does_not_exist(self): + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('changeset:comment', args=[1234]) + ) + + self.assertEqual(response.status_code, 404) + + def test_changeset_without_comments(self): + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('changeset:comment', args=[1343]) + ) + + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data, []) diff --git a/osmchadjango/changeset/urls.py b/osmchadjango/changeset/urls.py index bd4bcf03..71e0ce76 100644 --- a/osmchadjango/changeset/urls.py +++ b/osmchadjango/changeset/urls.py @@ -72,7 +72,7 @@ re_path( r'^changesets/(?P\w+)/comment/$', view=views.ChangesetCommentAPIView.as_view( - {'post': 'post_comment'} + {'post': 'post_comment', 'get': 'get_comments'} ), name='comment' ), diff --git a/osmchadjango/changeset/views.py b/osmchadjango/changeset/views.py index baca5d5c..e7b44dcf 100644 --- a/osmchadjango/changeset/views.py +++ b/osmchadjango/changeset/views.py @@ -24,6 +24,7 @@ from rest_framework_gis.pagination import GeoJsonPagination from rest_framework_csv.renderers import CSVRenderer from rest_framework.exceptions import APIException +import requests from .models import Changeset, UserWhitelist, SuspicionReasons, Tag from .filters import ChangesetFilter @@ -465,13 +466,27 @@ def get_queryset(self): class ChangesetCommentAPIView(ModelViewSet): - """Post a comment to a changeset in the OpenStreetMap website.""" queryset = Changeset.objects.all() permission_classes = (IsAuthenticated,) serializer_class = ChangesetCommentSerializer + @detail_route(methods=['get']) + def get_comments(self, request, pk): + "List comments received by a changeset on the OpenStreetMap website." + self.changeset = self.get_object() + headers = { + 'apiKey': settings.OSM_COMMENTS_API_KEY, + 'Content-Type': 'application/json' + } + data = requests.get( + 'https://osm-comments-api.mapbox.com/api/v1/changesets/{}'.format(pk), + headers=headers + ) + return data.get('properties').get('comments') + @detail_route(methods=['post']) def post_comment(self, request, pk): + "Post a comment to a changeset in the OpenStreetMap website." self.changeset = self.get_object() serializer = self.get_serializer(data=request.data) if serializer.is_valid(): From f1816e6e3d1e99e3f8d6835c9c8e6a9d6ad7f731 Mon Sep 17 00:00:00 2001 From: Wille Marcel Date: Wed, 14 Nov 2018 10:27:55 -0300 Subject: [PATCH 2/4] update test dependencies and fix error in ChangesetCommentsView --- .../changeset/tests/test_changeset_views.py | 49 ----------------- .../changeset/tests/test_comment_views.py | 52 ++++++++++++++++++- osmchadjango/changeset/tests/test_tasks.py | 2 +- osmchadjango/changeset/views.py | 11 +++- requirements/test.txt | 7 ++- 5 files changed, 65 insertions(+), 56 deletions(-) diff --git a/osmchadjango/changeset/tests/test_changeset_views.py b/osmchadjango/changeset/tests/test_changeset_views.py index cd117d39..4969e9ca 100644 --- a/osmchadjango/changeset/tests/test_changeset_views.py +++ b/osmchadjango/changeset/tests/test_changeset_views.py @@ -1263,52 +1263,3 @@ def test_set_harmful_by_staff_user(self): ) self.assertEqual(response.status_code, 200) self.assertEqual(Changeset.objects.filter(checked=True).count(), 5) - -class TestChangesetCommentsView(APITestCase): - def setUp(self): - self.changeset = HarmfulChangesetFactory( - id=55736682, - ) - HarmfulChangesetFactory(id=1343) - self.user = User.objects.create_user( - username='test', - password='password', - email='a@a.com', - ) - UserSocialAuth.objects.create( - user=self.user, - provider='openstreetmap', - uid='123123', - ) - - def test_unauthenticated_changeset_detail_response(self): - response = self.client.get( - reverse('changeset:comment-list', args=[self.changeset.id]) - ) - self.assertEqual(response.status_code, 401) - - def test_authenticated_changeset_detail_response(self): - self.client.login(username=self.user.username, password='password') - response = self.client.get( - reverse('changeset:comment', args=[self.changeset.id]) - ) - - self.assertEqual(response.status_code, 200) - self.assertTrue(len(response.data) > 0) - - def test_changeset_does_not_exist(self): - self.client.login(username=self.user.username, password='password') - response = self.client.get( - reverse('changeset:comment', args=[1234]) - ) - - self.assertEqual(response.status_code, 404) - - def test_changeset_without_comments(self): - self.client.login(username=self.user.username, password='password') - response = self.client.get( - reverse('changeset:comment', args=[1343]) - ) - - self.assertEqual(response.status_code, 200) - self.assertTrue(response.data, []) diff --git a/osmchadjango/changeset/tests/test_comment_views.py b/osmchadjango/changeset/tests/test_comment_views.py index 9e0b3ca0..aa68ec88 100644 --- a/osmchadjango/changeset/tests/test_comment_views.py +++ b/osmchadjango/changeset/tests/test_comment_views.py @@ -8,7 +8,7 @@ from social_django.models import UserSocialAuth from rest_framework.test import APITestCase from requests_oauthlib import OAuth1Session -import mock +from unittest import mock from ...users.models import User from ..models import Changeset @@ -160,3 +160,53 @@ def test_comment_changeset_doesnt_exist(self): ) self.assertEqual(response.status_code, 404) + + +class TestChangesetCommentsView(APITestCase): + def setUp(self): + self.changeset = HarmfulChangesetFactory( + id=55736682, + ) + HarmfulChangesetFactory(id=1343) + self.user = User.objects.create_user( + username='test', + password='password', + email='a@a.com', + ) + UserSocialAuth.objects.create( + user=self.user, + provider='openstreetmap', + uid='123123', + ) + + def test_unauthenticated_changeset_detail_response(self): + response = self.client.get( + reverse('changeset:comment', args=[self.changeset.id]) + ) + self.assertEqual(response.status_code, 401) + + def test_authenticated_changeset_detail_response(self): + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('changeset:comment', args=[self.changeset.id]) + ) + + self.assertEqual(response.status_code, 200) + self.assertTrue(len(response.data) > 0) + + def test_changeset_does_not_exist(self): + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('changeset:comment', args=[1234]) + ) + + self.assertEqual(response.status_code, 404) + + def test_changeset_without_comments(self): + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('changeset:comment', args=[1343]) + ) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data, []) diff --git a/osmchadjango/changeset/tests/test_tasks.py b/osmchadjango/changeset/tests/test_tasks.py index 365a683a..842bf62e 100644 --- a/osmchadjango/changeset/tests/test_tasks.py +++ b/osmchadjango/changeset/tests/test_tasks.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -from mock import patch +from unittest.mock import patch from urllib.parse import quote from django.test import TestCase diff --git a/osmchadjango/changeset/views.py b/osmchadjango/changeset/views.py index e7b44dcf..b0cd6823 100644 --- a/osmchadjango/changeset/views.py +++ b/osmchadjango/changeset/views.py @@ -482,7 +482,16 @@ def get_comments(self, request, pk): 'https://osm-comments-api.mapbox.com/api/v1/changesets/{}'.format(pk), headers=headers ) - return data.get('properties').get('comments') + if data.status_code != 200: + return Response( + data.json(), + status=data.status_code + ) + else: + return Response( + data.json().get('properties').get('comments'), + status=status.HTTP_200_OK + ) @detail_route(methods=['post']) def post_comment(self, request, pk): diff --git a/requirements/test.txt b/requirements/test.txt index 6856c20a..a2569ab1 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1,6 +1,5 @@ # Test dependencies go here. -r base.txt -flake8==3.5.0 -django-test-plus==1.0.22 -factory-boy==2.9.2 -mock==2.0.0 +flake8==3.6.0 +django-test-plus==1.1.1 +factory-boy==2.11.1 From dd77bdd87de3a6854e5f9f99888e040188915144 Mon Sep 17 00:00:00 2001 From: Wille Marcel Date: Thu, 29 Nov 2018 09:36:36 -0200 Subject: [PATCH 3/4] replace unique uid constraint for unique together with added_by --- .../migrations/0015_auto_20181129_1135.py | 24 +++++++++++++++++++ osmchadjango/supervise/models.py | 3 ++- osmchadjango/supervise/tests/test_models.py | 10 ++++++-- 3 files changed, 34 insertions(+), 3 deletions(-) create mode 100644 osmchadjango/supervise/migrations/0015_auto_20181129_1135.py diff --git a/osmchadjango/supervise/migrations/0015_auto_20181129_1135.py b/osmchadjango/supervise/migrations/0015_auto_20181129_1135.py new file mode 100644 index 00000000..628a33fc --- /dev/null +++ b/osmchadjango/supervise/migrations/0015_auto_20181129_1135.py @@ -0,0 +1,24 @@ +# Generated by Django 2.0.9 on 2018-11-29 11:35 + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('supervise', '0014_auto_20180307_1417'), + ] + + operations = [ + migrations.AlterField( + model_name='blacklisteduser', + name='uid', + field=models.CharField(max_length=255), + ), + migrations.AlterUniqueTogether( + name='blacklisteduser', + unique_together={('uid', 'added_by')}, + ), + ] diff --git a/osmchadjango/supervise/models.py b/osmchadjango/supervise/models.py index 0f1ce034..aa09d1b9 100644 --- a/osmchadjango/supervise/models.py +++ b/osmchadjango/supervise/models.py @@ -53,7 +53,7 @@ class Meta: class BlacklistedUser(models.Model): username = models.CharField(max_length=1000) - uid = models.CharField(max_length=255, unique=True) + uid = models.CharField(max_length=255) added_by = models.ForeignKey(User, on_delete=models.CASCADE) date = models.DateTimeField(auto_now_add=True) @@ -65,4 +65,5 @@ def save(self, *args, **kwargs): super(BlacklistedUser, self).save(*args, **kwargs) class Meta: + unique_together = ('uid', 'added_by') ordering = ['-date'] diff --git a/osmchadjango/supervise/tests/test_models.py b/osmchadjango/supervise/tests/test_models.py index 34744b66..c1934dd5 100644 --- a/osmchadjango/supervise/tests/test_models.py +++ b/osmchadjango/supervise/tests/test_models.py @@ -149,7 +149,8 @@ def test_other_geometry_types(self): class TestBlacklistedUserModel(TestCase): def setUp(self): - self.user = UserFactory(is_staff=True) + self.user = UserFactory() + self.other_user = UserFactory() self.blacklisted = BlacklistedUser.objects.create( username='Bad User', uid='3434', @@ -187,4 +188,9 @@ def test_validation(self): uid='5643', added_by=self.user, ) - self.assertEqual(BlacklistedUser.objects.count(), 2) + BlacklistedUser.objects.create( + username='Bad User', + uid='5643', + added_by=self.other_user, + ) + self.assertEqual(BlacklistedUser.objects.count(), 3) From 3de61ac09935d0352f7c7eb8174327a7d086b975 Mon Sep 17 00:00:00 2001 From: Wille Marcel Date: Thu, 29 Nov 2018 10:32:18 -0200 Subject: [PATCH 4/4] make blacklist endpoints accessible for any authenticated user --- osmchadjango/supervise/tests/test_views.py | 46 +++++++++++++++++----- osmchadjango/supervise/views.py | 30 +++++++++----- 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/osmchadjango/supervise/tests/test_views.py b/osmchadjango/supervise/tests/test_views.py index 7beff127..3dd184ae 100644 --- a/osmchadjango/supervise/tests/test_views.py +++ b/osmchadjango/supervise/tests/test_views.py @@ -844,6 +844,11 @@ def setUp(self): uid='3435', added_by=self.staff_user, ) + BlacklistedUser.objects.create( + username='New bad user', + uid='9888', + added_by=self.user, + ) self.url = reverse('supervise:blacklist-list-create') def test_list_view_unauthenticated(self): @@ -853,7 +858,8 @@ def test_list_view_unauthenticated(self): def test_list_view_normal_user(self): self.client.login(username=self.user.username, password='password') response = self.client.get(self.url) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 200) + self.assertEqual(len(response.data.get('results')), 1) def test_list_view_staff_user(self): self.client.login(username=self.staff_user.username, password='password') @@ -896,8 +902,8 @@ def test_create_view_unauthenticated(self): def test_create_view_normal_user(self): self.client.login(username=self.user.username, password='password') response = self.client.post(self.url, self.data) - self.assertEqual(response.status_code, 403) - self.assertEqual(BlacklistedUser.objects.count(), 0) + self.assertEqual(response.status_code, 201) + self.assertEqual(BlacklistedUser.objects.count(), 1) def test_create_view_staff_user(self): self.client.login(username=self.staff_user.username, password='password') @@ -935,6 +941,11 @@ def setUp(self): uid='3434', added_by=self.staff_user, ) + self.blacklisted_2 = BlacklistedUser.objects.create( + username='Bad User', + uid='3434', + added_by=self.user, + ) self.url = reverse( 'supervise:blacklist-detail', args=[self.blacklisted.uid] ) @@ -946,7 +957,23 @@ def test_unauthenticated_get(self): def test_normal_user_get(self): self.client.login(username=self.user.username, password='password') response = self.client.get(self.url) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data.get('username'), 'Bad User') + self.assertEqual(response.data.get('added_by'), 'test_user') + self.assertIsNotNone(response.data.get('uid')) + self.assertIn('date', response.data.keys()) + + def test_normal_user_getting_staff_user_blacklist(self): + blacklisted = BlacklistedUser.objects.create( + username='Bad User', + uid='4999', + added_by=self.staff_user, + ) + self.client.login(username=self.user.username, password='password') + response = self.client.get( + reverse('supervise:blacklist-detail', args=[4999]) + ) + self.assertEqual(response.status_code, 404) def test_staff_user_get(self): self.client.login(username=self.staff_user.username, password='password') @@ -960,19 +987,19 @@ def test_staff_user_get(self): def test_unauthenticated_delete(self): response = self.client.delete(self.url) self.assertEqual(response.status_code, 401) - self.assertEqual(BlacklistedUser.objects.count(), 1) + self.assertEqual(BlacklistedUser.objects.count(), 2) def test_normal_user_delete(self): self.client.login(username=self.user.username, password='password') response = self.client.delete(self.url) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 204) self.assertEqual(BlacklistedUser.objects.count(), 1) def test_staff_user_delete(self): self.client.login(username=self.staff_user.username, password='password') response = self.client.delete(self.url) self.assertEqual(response.status_code, 204) - self.assertEqual(BlacklistedUser.objects.count(), 0) + self.assertEqual(BlacklistedUser.objects.count(), 1) def test_unauthenticated_patch(self): response = self.client.patch(self.url, {'username': 'other_user'}) @@ -982,8 +1009,9 @@ def test_unauthenticated_patch(self): def test_normal_user_patch(self): self.client.login(username=self.user.username, password='password') response = self.client.patch(self.url, {'username': 'other_user'}) - self.assertEqual(response.status_code, 403) - self.assertEqual(self.blacklisted.username, 'Bad User') + self.assertEqual(response.status_code, 200) + self.blacklisted_2.refresh_from_db() + self.assertEqual(self.blacklisted_2.username, 'other_user') def test_staff_user_patch(self): self.client.login(username=self.staff_user.username, password='password') diff --git a/osmchadjango/supervise/views.py b/osmchadjango/supervise/views.py index d49f1a18..1034e4c1 100644 --- a/osmchadjango/supervise/views.py +++ b/osmchadjango/supervise/views.py @@ -5,7 +5,7 @@ from django.urls import reverse from rest_framework.generics import ( - ListCreateAPIView, ListAPIView, RetrieveUpdateDestroyAPIView + ListCreateAPIView, ListAPIView, RetrieveUpdateDestroyAPIView, get_object_or_404 ) from rest_framework.response import Response from rest_framework.filters import OrderingFilter @@ -222,7 +222,13 @@ class BlacklistedUserListCreateAPIView(ListCreateAPIView): """ queryset = BlacklistedUser.objects.all() serializer_class = BlacklistSerializer - permission_classes = (IsAdminUser,) + permission_classes = (IsAuthenticated,) + + def get_queryset(self): + if self.request: + return BlacklistedUser.objects.filter(added_by=self.request.user) + else: + BlacklistedUser.objects.none() def perform_create(self, serializer): serializer.save(added_by=self.request.user) @@ -234,21 +240,25 @@ class BlacklistedUserDetailAPIView(RetrieveUpdateDestroyAPIView): Get details about a BlacklistedUser. Access restricted to staff users. delete: - Delete a User from the Blacklist. - Only staff users can use this method. + Delete a User from your Blacklist. patch: Update a BlacklistedUser. - It's useful if you need to update the username of a User. Only staff users - can use this method. + It's useful if you need to update the username of a User. put: Update a BlacklistedUser. - It's useful if you need to update the username of a User. Only staff users - can use this method. + It's useful if you need to update the username of a User. """ queryset = BlacklistedUser.objects.all() serializer_class = BlacklistSerializer - permission_classes = (IsAdminUser,) - lookup_field = 'uid' + permission_classes = (IsAuthenticated, IsOwnerOrReadOnly) def perform_update(self, serializer): serializer.save(added_by=self.request.user) + + def get_object(self): + queryset = self.get_queryset() + return get_object_or_404( + queryset, + added_by=self.request.user, + uid=self.kwargs['uid'] + )