-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/spaces #3
base: master
Are you sure you want to change the base?
Changes from 6 commits
f33d93c
07a6c88
2ae2f27
30e645f
c5fef9c
0fc2e34
46cdc43
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 |
---|---|---|
@@ -0,0 +1,19 @@ | ||
from rest_framework import permissions | ||
|
||
|
||
class IsAdminOrSelf(permissions.BasePermission): | ||
def get_owner(self, obj): | ||
return obj | ||
|
||
def has_object_permission(self, request, view, obj): | ||
if request.user.is_superuser: | ||
return True | ||
if request.user == self.get_owner(obj): | ||
return True | ||
return False | ||
|
||
|
||
class IsPlaceOwnerOrStaff(IsAdminOrSelf): | ||
def get_owner(self, obj): | ||
return obj.owner | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
from django.contrib import admin | ||
|
||
from .models import Place | ||
|
||
admin.site.register(Place) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
from django.db import models | ||
from django.contrib.auth.models import User | ||
|
||
|
||
class Address(models.Model): | ||
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. Yo creo que city debería ser su propia entidad y aquí una ForeignKey, pero eso está abierto a discusión. 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. cc/ @gpul-org/xea-devs |
||
street_name = models.CharField(max_length=140) | ||
civic = models.CharField(max_length=20) | ||
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. What's 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. It stands for "civic number". Should we be more specific? 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. Does that even exist outside Canada? On the contrary, I think it's too specific. I would just say
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. I think I didn't explain it very well. That's what I was referring to: https://en.wikipedia.org/wiki/House_numbering 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. And, as that very page reads, it seems not every country uses the same numbering system, and many of them are not like ours. And no two countries call their numbers the same uses them in the same way, or are even numbers :-) |
||
floor = models.IntegerField(blank=True) | ||
door = models.CharField(max_length=2, blank=True) | ||
city = models.CharField(max_length=140) | ||
postal_code = models.IntegerField(blank=True) | ||
region = models.CharField(max_length=140) | ||
nation = models.CharField(max_length=140) | ||
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. Nation should just be a two-character field meaning its ISO-3166 code. |
||
|
||
|
||
class Place(models.Model): | ||
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. A Place needs GPS coordinates. We need to be able to put it on a map. At least, we need latitude and longitude to draw a point on a map. |
||
class Meta: | ||
app_label = 'spaces' | ||
|
||
name = models.CharField(max_length=140) | ||
owner = models.ForeignKey(User, on_delete=models.CASCADE) | ||
description = models.TextField(max_length=500) | ||
address = models.ForeignKey(Address) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
from rest_framework import serializers | ||
|
||
from .models import Address, Place | ||
|
||
|
||
class AddressSerializer(serializers.ModelSerializer): | ||
class Meta: | ||
model = Address | ||
fields = ('street_name', 'civic', 'floor', 'door', 'city', 'postal_code', 'region', 'nation') | ||
|
||
|
||
class PlaceSerializer(serializers.HyperlinkedModelSerializer): | ||
address = AddressSerializer() | ||
|
||
class Meta: | ||
model = Place | ||
fields = ('url', 'name', 'owner', 'description', 'address') | ||
|
||
def update(self, instance, validated_data): | ||
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. According to http://www.django-rest-framework.org/api-guide/serializers/#modelserializer The ModelSerializer class is the same as a regular Serializer class, except that:
You don't seem to be doing anything that the ModelSerializer would not already do for you. |
||
instance.name = validated_data.get('name', instance.name) | ||
instance.owner = validated_data.get('owner', instance.owner) | ||
instance.description = validated_data.get('nationality', instance.description) | ||
instance.address = validated_data.get('address', instance.address) | ||
instance.save() | ||
return instance | ||
|
||
def create(self, validated_data): | ||
address_data = validated_data.get('address') | ||
address = Address.objects.create(street_name=address_data.get('street_name'), | ||
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. You should never do this. You are replicating code from However, as I say in the previous note, this should already be handled by the HyperlinkedModelSerializer and this code might as well not exist. 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. It seems working without the explicit implementation of .update(). So we can immediately remove that code.
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. Yup. It seems it was me who was mistaken. 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. Then, it would be nice if both save's were performed in a single DB transaction (in case one fails so do the others). For that, we can use django.db.transaction.atomic() either as a decorator on the update/create function, or as a context inside, as with transaction.atomic():
# stuff here |
||
civic=address_data.get('civic'), | ||
floor=address_data.get('floor'), | ||
door=address_data.get('door'), | ||
city=address_data.get('city'), | ||
postal_code=address_data.get('postal_code'), | ||
region=address_data.get('region'), | ||
nation=address_data.get('nation') | ||
) | ||
address.save() | ||
place = Place.objects.create( | ||
name=validated_data.get('name'), | ||
owner=validated_data.get('owner'), | ||
description=validated_data.get('description'), | ||
address=address | ||
) | ||
|
||
place.save() | ||
return place |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
from django.contrib.auth import get_user_model | ||
from django.core.urlresolvers import reverse | ||
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.
|
||
from rest_framework import status | ||
from rest_framework.test import APITestCase | ||
from accounts.tests import create_n_users | ||
from .models import Place | ||
|
||
|
||
class PlacesTest(APITestCase): | ||
# Default place data | ||
|
||
name = 'place' | ||
description = 'A very nice place' | ||
|
||
# Default address data | ||
|
||
street = 'streetname' | ||
civic = '123' | ||
floor = '10' | ||
door = 'c' | ||
city = 'La Coruña' | ||
postal_code = '15005' | ||
region = 'Galicia' | ||
nation = 'Spain' | ||
main_url = reverse('place-list') | ||
|
||
address_payload = {'street_name': street, | ||
'civic': civic, | ||
'floor': floor, | ||
'door': door, | ||
'city': city, | ||
'postal_code': postal_code, | ||
'region': region, | ||
'nation': nation | ||
} | ||
|
||
place_payload = {'name': name, | ||
'description': description, | ||
'address': address_payload | ||
} | ||
|
||
DEFAULT_USERS_NUMBER = 3 | ||
DEFAULT_PLACES_NUMBER = 3 | ||
|
||
def setUp(self): | ||
create_n_users(self.DEFAULT_USERS_NUMBER) | ||
|
||
def create_n_places(self, n): | ||
payload = self.place_payload | ||
for i in range(0, n): | ||
owner_url = reverse('user-detail', kwargs={'pk': n % self.DEFAULT_USERS_NUMBER + 1}) | ||
payload.update({'owner': owner_url, 'name': self.name + str(n)}) | ||
response = self.client.post(self.main_url, payload, format='json') | ||
self.assertEqual(response.status_code, status.HTTP_201_CREATED) | ||
|
||
def test_create_place(self): | ||
payload = self.place_payload | ||
payload.update({'owner': reverse('user-detail', kwargs={'pk': 1})}) | ||
response = self.client.post(self.main_url, payload, format='json') | ||
self.assertEqual(response.status_code, status.HTTP_201_CREATED) | ||
|
||
def test_create_n_places(self): | ||
self.create_n_places(self.DEFAULT_PLACES_NUMBER) | ||
queryset = Place.objects.all() | ||
self.assertEqual(len(queryset), self.DEFAULT_PLACES_NUMBER) | ||
|
||
def test_owner_updates_place(self): | ||
self.create_n_places(self.DEFAULT_PLACES_NUMBER) | ||
url = reverse('place-update-place-data', kwargs={'pk': 1}) | ||
place = Place.objects.get(pk=1) | ||
owner = place.owner | ||
self.client.force_login(owner) | ||
new_place_name = 'newname' | ||
response = self.client.patch(path=url, data={'name': new_place_name}, format='json') | ||
place = Place.objects.get(pk=1) | ||
self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
self.assertEqual(place.name, new_place_name) | ||
|
||
def test_anonym_updates_place(self): | ||
self.create_n_places(self.DEFAULT_PLACES_NUMBER) | ||
new_place_name = 'newname' | ||
url = reverse('place-update-place-data', kwargs={'pk': 1}) | ||
response = self.client.patch(path=url, data={'name': new_place_name}) | ||
self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) | ||
|
||
def logged_user_updates_not_owned_place(self): | ||
self.create_n_places(self.DEFAULT_PLACES_NUMBER) | ||
url = reverse('place-update-place-data', kwargs={'pk': 1}) | ||
owner = get_user_model().objects.get(pk=2) | ||
self.client.force_login(owner) | ||
new_place_name = 'newname' | ||
response = self.client.patch(path=url, data={'name': new_place_name}, format='json') | ||
place = Place.objects.get(pk=1) | ||
self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) | ||
self.assertEqual(place.name, new_place_name) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
from django.conf.urls import url, include | ||
from rest_framework import routers | ||
|
||
from .viewsets import PlaceViewSet | ||
|
||
router = routers.SimpleRouter(trailing_slash=False) | ||
router.register(r'', PlaceViewSet) | ||
|
||
|
||
urlpatterns = [ | ||
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. If this is so simple, you can just do |
||
|
||
url(r'', include(router.urls)), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
from rest_framework import status | ||
from rest_framework import viewsets | ||
from rest_framework.decorators import detail_route | ||
from rest_framework.response import Response | ||
|
||
from api.permissions import IsPlaceOwnerOrStaff | ||
from .models import Place | ||
from .serializers import PlaceSerializer | ||
|
||
|
||
class PlaceViewSet(viewsets.ModelViewSet): | ||
""" | ||
API endpoint that allows 'spaces' to be viewed or edited. | ||
""" | ||
queryset = Place.objects.all() | ||
serializer_class = PlaceSerializer | ||
|
||
@detail_route(methods=['put','patch'], permission_classes=[IsPlaceOwnerOrStaff]) | ||
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. Would it be "cleaner" to just reimplement the "update" method instead of doing this? |
||
def update_place_data(self, request, pk=None): | ||
if not pk: | ||
return Response(status=status.HTTP_400_BAD_REQUEST) | ||
user = self.get_object() | ||
place = Place.objects.get(pk=pk) | ||
self.check_object_permissions(request, user) | ||
serializer = PlaceSerializer(data=request.data, context={'request': request}, partial=True) | ||
if serializer.is_valid(): | ||
serializer.update(place, serializer.validated_data) | ||
return Response(status=status.HTTP_200_OK) | ||
else: | ||
return Response(serializer.errors, | ||
status=status.HTTP_400_BAD_REQUEST) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ | |
'jwt_knox', | ||
'api', | ||
'accounts', | ||
'spaces', | ||
] | ||
|
||
MIDDLEWARE_CLASSES = [ | ||
|
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.
Es buena idea hacer este refactor, pero si esto fuese una pull request por si mismo, ya estaría probablemente listo para mergear.