Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions accounts/viewsets.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
from rest_framework.decorators import detail_route
from rest_framework.response import Response

from .serializers import UserSerializer, UserPasswordSerializer
from .permissions import IsAdminOrSelf
from api.permissions import IsAdminOrSelf
from . import utils
from .models import UserProfile
from .serializers import UserProfileSerializer
from . import utils
from .serializers import UserSerializer, UserPasswordSerializer


class UserViewSet(viewsets.ModelViewSet):
Expand Down
19 changes: 19 additions & 0 deletions api/permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from rest_framework import permissions
Copy link
Member

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.



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

1 change: 1 addition & 0 deletions api/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@
urlpatterns = [
url(r'', include(router.urls)),
url(r'accounts/', include('accounts.urls')),
url(r'places/', include('places.urls')),
]
Empty file added places/__init__.py
Empty file.
5 changes: 5 additions & 0 deletions places/admin.py
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)
19 changes: 19 additions & 0 deletions places/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from django.db import models
from django.contrib.auth.models import User


class Address(models.Model):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tal vez no he sido claro en absoluto sobre como puede ser interesante manejar las direcciones. Si ha sido así, no era mi intención, y espero aclararlo un poco más con la captura de alguien que, en mi opinión, hace un trabajo relativamente bueno al respecto: Amazon (digo bueno porque, en fin, ellos tienen que realmente mandar paquetes, con lo que saben si estos datos son suficientes o no para llegar al usuario).

De aquí, evidentemente el nombre y apellidos se cambia por el nombre del lugar, que no pertenece a la dirección.

Lo que decía además es que es interesante tener un modelo de ciudades fuera de la dirección, ya que ciudad y país suelen ir juntos, y para poder tener más fácil una consulta del tipo "¿cuántos usuarios tengo registrados que sean de Santiago"? (¿Santiago... de Chile o de España? Bueno, si la ciudad tiene su propio modelo, lo que buscas es aquellos objetos con la clave foránea 27). Pero vamos, que eso no tengo una opinión fuerte al respecto.

Lo que sí opino es:

  • Que el país debería de ser el código de dos letras del mismo (esto es un API, queremos datos normalizados, si se puede)
  • Que el número/letra/puerta/escalera/bloque/punto kilométrico y otras nociones de situación del edificio y la vivienda dentro de un municipio son demasiado dependientes de las costumbres del lugar

Para lo demás, me gustaría poder hablar via comentarios por aquí mismamente al respecto, y poder llegar a algo razonable.

screenshot from 2016-09-09 15-27-11

line1 = models.CharField(max_length=70)
line2 = models.CharField(max_length=70)


class Place(models.Model):
class Meta:
app_label = 'places'

name = models.CharField(max_length=140)
owner = models.ForeignKey(User, on_delete=models.CASCADE)
description = models.TextField(max_length=500)
address = models.ForeignKey(Address)
latitude = models.DecimalField(max_digits=9, decimal_places=6)
longitude = models.DecimalField(max_digits=9, decimal_places=6)
48 changes: 48 additions & 0 deletions places/serializers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
from rest_framework import serializers

from .models import Address, Place


class AddressSerializer(serializers.ModelSerializer):
class Meta:
model = Address
fields = ('line1', 'line2')


class PlaceSerializer(serializers.HyperlinkedModelSerializer):
address = AddressSerializer()

class Meta:
model = Place
fields = ('url', 'name', 'owner', 'description', 'address')

def update(self, instance, validated_data):
instance.name = validated_data.get('name', instance.name)
instance.owner = validated_data.get('owner', instance.owner)
instance.description = validated_data.get('nationality', instance.description)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

description is validated_data.get('nationality') ?

address_data = validated_data.get('address', instance.address)
address = Address.objects.create(

line1=address_data.get('line1',''),
line2=address_data.get('line2','')
)
instance.address = address
instance.save()
return instance

def create(self, validated_data):
address_data = validated_data.get('address')
address = Address.objects.create(
line1=address_data.get('line1'),
line2=address_data.get('line2')
)
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
104 changes: 104 additions & 0 deletions places/tests.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
from django.contrib.auth import get_user_model
from rest_framework.reverse import reverse
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'

line1 = street + ' ' + civic + ' ' + floor + door
line2 = postal_code + ' ' + city + ', ' + nation
main_url = reverse('place-list')

address_payload = {'line1': line1,
'line2': line2,
}

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)

def test_update_place_address(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)
line1 = 'newstreetname 5b'
address_payload = {'line1': line1}
response = self.client.patch(path=url, data={'address': address_payload}, format='json')
place = Place.objects.get(pk=1)
self.assertEqual(response.status_code, status.HTTP_200_OK)
self.assertEqual(place.address.line1, line1)
9 changes: 9 additions & 0 deletions places/urls.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from rest_framework import routers

from .viewsets import PlaceViewSet

router = routers.SimpleRouter(trailing_slash=False)
router.register(r'', PlaceViewSet)


urlpatterns = router.urls
31 changes: 31 additions & 0 deletions places/viewsets.py
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 'places' to be viewed or edited.
"""
queryset = Place.objects.all()
serializer_class = PlaceSerializer

@detail_route(methods=['put', 'patch'], permission_classes=[IsPlaceOwnerOrStaff])
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)
1 change: 1 addition & 0 deletions xea_core/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
'jwt_knox',
'api',
'accounts',
'places',
]

MIDDLEWARE_CLASSES = [
Expand Down