From 8d6e944bbf4c7e3516e2fee1fc48f41f1f4d5e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20=C5=9Awietlik?= Date: Fri, 1 Dec 2023 16:08:22 +0100 Subject: [PATCH] updated code as per code review --- Dshop/apps/users/api_urls.py | 6 +- Dshop/apps/users/api_views.py | 14 +- Dshop/apps/users/models.py | 16 +- Dshop/apps/users/serializers.py | 39 ++-- Dshop/apps/users/tests/test_api_login.py | 19 -- Dshop/apps/users/tests/test_api_logout.py | 22 +- .../users/tests/test_api_password_change.py | 51 ++--- .../apps/users/tests/test_api_user_details.py | 199 ++++++++++++++++-- 8 files changed, 251 insertions(+), 115 deletions(-) diff --git a/Dshop/apps/users/api_urls.py b/Dshop/apps/users/api_urls.py index bc8758c..817c9f0 100644 --- a/Dshop/apps/users/api_urls.py +++ b/Dshop/apps/users/api_urls.py @@ -3,10 +3,14 @@ from .api_views import RegistrationViewSet, LoginView, LogoutView, PasswordChangeView, UserDataChangeView urlpatterns = [ + path('', UserDataChangeView.as_view( + {'get': 'retrieve', 'put': 'update', 'patch': 'update'} + ), + name='api-user-details' + ), path('register/', RegistrationViewSet.as_view({'post': 'create'}), name='api-register'), path('login/', LoginView.as_view(), name='api-login'), path('logout/', LogoutView.as_view(), name='api-logout'), path('password/change/', PasswordChangeView.as_view(), name='api-password-change'), - path('', UserDataChangeView.as_view({'get': 'retrieve', 'patch': 'update'}), name='api-user-details'), ] diff --git a/Dshop/apps/users/api_views.py b/Dshop/apps/users/api_views.py index de4e4fa..3723220 100644 --- a/Dshop/apps/users/api_views.py +++ b/Dshop/apps/users/api_views.py @@ -23,7 +23,7 @@ class RegistrationViewSet(mixins.CreateModelMixin, viewsets.GenericViewSet): queryset = User.objects.all() - permission_classes = [AllowAny] + permission_classes = (AllowAny, ) throttle_scope = 'registration' serializer_class = RegistrationSerializer @@ -34,7 +34,7 @@ def perform_create(self, serializer): class LoginView(GenericAPIView): - permission_classes = [AllowAny] + permission_classes = (AllowAny, ) serializer_class = LoginSerializer throttle_scope = 'login' @@ -58,11 +58,11 @@ def post(self, request): class LogoutView(GenericAPIView): - permission_classes = [AllowAny] - serializer_class = EmptySerializer + permission_classes = (AllowAny, ) + # serializer_class = EmptySerializer throttle_scope = 'logout' - def post(self, request): + def get(self, request): try: request.user.auth_token.delete() except (AttributeError, ObjectDoesNotExist): @@ -78,7 +78,7 @@ def post(self, request): class PasswordChangeView(GenericAPIView): serializer_class = PasswordChangeSerializer - permission_classes = [IsAuthenticated] + permission_classes = (IsAuthenticated, ) throttle_scope = 'password_change' def post(self, request): @@ -90,7 +90,7 @@ def post(self, request): class UserDataChangeView(mixins.RetrieveModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet): - permission_classes = [IsAuthenticated] + permission_classes = (IsAuthenticated, ) throttle_scope = 'change_user_data' def get_serializer_class(self): diff --git a/Dshop/apps/users/models.py b/Dshop/apps/users/models.py index 52faf33..5c7a1ba 100644 --- a/Dshop/apps/users/models.py +++ b/Dshop/apps/users/models.py @@ -17,15 +17,15 @@ def __str__(self): class CustomUser(models.Model): user = models.OneToOneField(User, on_delete=models.CASCADE, related_name='customusers', verbose_name=_('User')) - first_name = models.CharField(max_length=100, blank=True, verbose_name=_('First name')) - last_name = models.CharField(max_length=100, blank=True, verbose_name=_('Last name')) - address = models.CharField(max_length=150, blank=True, verbose_name=_('Address')) - postal_code = models.CharField(max_length=7, blank=True, verbose_name='ZIP/Postal code') - city = models.CharField(max_length=50, blank=True, verbose_name=_('City')) + first_name = models.CharField(max_length=100, verbose_name=_('First name')) + last_name = models.CharField(max_length=100, verbose_name=_('Last name')) + address = models.CharField(max_length=150, verbose_name=_('Address')) + postal_code = models.CharField(max_length=7, verbose_name='ZIP/Postal code') + city = models.CharField(max_length=50, verbose_name=_('City')) country = models.ForeignKey(Country, null=True, on_delete=models.SET_NULL, related_name='customusers') - date_of_birth = models.DateField(null=True, blank=True, verbose_name=_('Date of birth')) - phone_number = models.CharField(max_length=15, blank=True, verbose_name=_('Phone number')) - date_joined = models.DateTimeField(auto_now_add=True) + date_of_birth = models.DateField(null=True, verbose_name=_('Date of birth')) + phone_number = models.CharField(max_length=15, verbose_name=_('Phone number')) + date_joined = models.DateTimeField(auto_now_add=True) # date_joined already in User model def __str__(self): return str(self.user) diff --git a/Dshop/apps/users/serializers.py b/Dshop/apps/users/serializers.py index cdfcc69..7566d48 100644 --- a/Dshop/apps/users/serializers.py +++ b/Dshop/apps/users/serializers.py @@ -77,32 +77,39 @@ def validate(self, attrs): class UserDataReadSerializer(serializers.ModelSerializer): - email = serializers.EmailField(source='user.email') + email = serializers.ReadOnlyField(source='user.email') + user_email = serializers.EmailField(required=False, write_only=True) + user = serializers.PrimaryKeyRelatedField(read_only=True) class Meta: model = CustomUser - fields = '__all__' + fields = ( + 'user', + 'email', + 'user_email', + 'first_name', + 'last_name', + 'address', + 'postal_code', + 'city', + 'country', + 'date_of_birth', + 'phone_number' + ) class UserDataChangeSerializer(UserDataReadSerializer): - email = serializers.EmailField(required=False) def update(self, instance, validated_data): - # Update CustomUser fields - instance.first_name = validated_data.get('first_name', instance.first_name) - instance.last_name = validated_data.get('last_name', instance.last_name) - instance.address = validated_data.get('address', instance.address) - instance.postal_code = validated_data.get('postal_code', instance.postal_code) - instance.city = validated_data.get('city', instance.city) - instance.country = validated_data.get('country', instance.country) - instance.date_of_birth = validated_data.get('date_of_birth', instance.date_of_birth) - instance.phone_number = validated_data.get('phone_number', instance.phone_number) - instance.save() - # Update User fields user = instance.user - if 'email' in validated_data: - user.email = validated_data['email'] + if 'user_email' in validated_data: + user.email = validated_data.pop('user_email') user.save() + # Update CustomUser fields + for key, value in validated_data.items(): + setattr(instance, key, value) + instance.save() + return instance diff --git a/Dshop/apps/users/tests/test_api_login.py b/Dshop/apps/users/tests/test_api_login.py index 0fef3a6..bbd7a75 100644 --- a/Dshop/apps/users/tests/test_api_login.py +++ b/Dshop/apps/users/tests/test_api_login.py @@ -4,7 +4,6 @@ from django.contrib.auth import get_user_model from django.urls import reverse from rest_framework import status -from rest_framework.authtoken.models import Token User = get_user_model() @@ -14,24 +13,6 @@ def login_url(): return reverse('api-login') -@pytest.fixture -def login_data(): - return { - 'username': 'testuser', - 'password': 'testpassword', - } - - -@pytest.fixture -def user_instance(login_data): - return User.objects.create_user(**login_data) - - -@pytest.fixture -def user_instance_token(user_instance): - return Token.objects.get_or_create(user=user_instance)[0] - - @pytest.mark.django_db def test_login_success(api_client, login_url, login_data, user_instance, user_instance_token): response = api_client.post(login_url, login_data, format='json') diff --git a/Dshop/apps/users/tests/test_api_logout.py b/Dshop/apps/users/tests/test_api_logout.py index 33ec320..148c109 100644 --- a/Dshop/apps/users/tests/test_api_logout.py +++ b/Dshop/apps/users/tests/test_api_logout.py @@ -12,28 +12,10 @@ def logout_url(): return reverse('api-logout') -@pytest.fixture -def login_data(): - return { - 'username': 'testuser', - 'password': 'testpassword', - } - - -@pytest.fixture -def user_instance(login_data): - return User.objects.create_user(**login_data) - - -@pytest.fixture -def user_instance_token(user_instance): - return Token.objects.get_or_create(user=user_instance)[0] - - @pytest.mark.django_db def test_logout_success(api_client, logout_url, user_instance, user_instance_token): api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - logout_response = api_client.post(logout_url) + logout_response = api_client.get(logout_url) assert logout_response.status_code == status.HTTP_200_OK authenticated_user = authenticate(user_instance=user_instance, token_key=user_instance_token.key) @@ -46,5 +28,5 @@ def test_logout_success(api_client, logout_url, user_instance, user_instance_tok @pytest.mark.django_db def test_logout_success_without_credentials(api_client, logout_url): # If user is not logged in, response should be also 200 because user is already logged out - logout_response = api_client.post(logout_url) + logout_response = api_client.get(logout_url) assert logout_response.status_code == status.HTTP_200_OK \ No newline at end of file diff --git a/Dshop/apps/users/tests/test_api_password_change.py b/Dshop/apps/users/tests/test_api_password_change.py index 638d159..70aad0a 100644 --- a/Dshop/apps/users/tests/test_api_password_change.py +++ b/Dshop/apps/users/tests/test_api_password_change.py @@ -4,7 +4,6 @@ from django.contrib.auth import get_user_model from django.urls import reverse from rest_framework import status -from rest_framework.authtoken.models import Token User = get_user_model() @@ -14,14 +13,6 @@ def password_change_url(): return reverse('api-password-change') -@pytest.fixture -def login_data(): - return { - 'username': 'testuser', - 'password': 'testpassword', - } - - @pytest.fixture def password_change_data(): return { @@ -31,30 +22,40 @@ def password_change_data(): } -@pytest.fixture -def user_instance(login_data): - return User.objects.create_user(**login_data) - +@pytest.mark.django_db +def test_password_change_success( + api_client, + password_change_url, + user_instance_token, + password_change_data, + login_data, + login_url +): + api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') + response = api_client.post(password_change_url, password_change_data) + assert response.status_code == status.HTTP_200_OK -@pytest.fixture -def user_instance_token(user_instance): - return Token.objects.get_or_create(user=user_instance)[0] + # set new login data after password change and perform login + login_data['password'] = password_change_data['new_password'] + response = api_client.post(login_url, login_data) + assert response.status_code == status.HTTP_200_OK @pytest.mark.django_db -def test_password_change_success(api_client, password_change_url, user_instance_token, password_change_data): +def test_password_change_failure(api_client, password_change_url, user_instance_token, password_change_data): api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.post(password_change_url, password_change_data, format='json') + response = api_client.post(password_change_url, password_change_data) assert response.status_code == status.HTTP_200_OK - response = api_client.post(password_change_url, password_change_data, format='json') + # after successful password change, current_password in password_change_data is no longer valid + response = api_client.post(password_change_url, password_change_data) assert response.status_code == status.HTTP_400_BAD_REQUEST @pytest.mark.django_db def test_password_change_empty_token(api_client, password_change_url): api_client.credentials(HTTP_AUTHORIZATION='Token ') - response = api_client.post(password_change_url, data={}, format='json') + response = api_client.post(password_change_url, data={}) assert response.status_code == status.HTTP_401_UNAUTHORIZED field_errors = json.loads(response.content).keys() @@ -64,7 +65,7 @@ def test_password_change_empty_token(api_client, password_change_url): @pytest.mark.django_db def test_password_change_empty_data(api_client, password_change_url, user_instance_token): api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.post(password_change_url, data={}, format='json') + response = api_client.post(password_change_url, data={}) assert response.status_code == status.HTTP_400_BAD_REQUEST field_errors = json.loads(response.content).keys() @@ -78,7 +79,7 @@ def test_password_change_empty_data(api_client, password_change_url, user_instan def test_password_change_wrong_current_password(api_client, password_change_url, user_instance_token, password_change_data): password_change_data['current_password'] = 'wrong_current_password' api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.post(password_change_url, password_change_data, format='json') + response = api_client.post(password_change_url, password_change_data) assert response.status_code == status.HTTP_400_BAD_REQUEST field_errors = json.loads(response.content).keys() @@ -90,7 +91,7 @@ def test_password_change_wrong_current_password(api_client, password_change_url, def test_password_change_no_current_password(api_client, password_change_url, user_instance_token, password_change_data): del password_change_data['current_password'] api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.post(password_change_url, password_change_data, format='json') + response = api_client.post(password_change_url, password_change_data) assert response.status_code == status.HTTP_400_BAD_REQUEST field_errors = json.loads(response.content).keys() @@ -102,7 +103,7 @@ def test_password_change_no_current_password(api_client, password_change_url, us def test_password_change_no_new_password(api_client, password_change_url, user_instance_token, password_change_data): del password_change_data['new_password'] api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.post(password_change_url, password_change_data, format='json') + response = api_client.post(password_change_url, password_change_data) assert response.status_code == status.HTTP_400_BAD_REQUEST field_errors = json.loads(response.content).keys() @@ -115,7 +116,7 @@ def test_password_change_no_new_password_and_password_again(api_client, password del password_change_data['new_password'] del password_change_data['new_password_again'] api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.post(password_change_url, password_change_data, format='json') + response = api_client.post(password_change_url, password_change_data) assert response.status_code == status.HTTP_400_BAD_REQUEST field_errors = json.loads(response.content).keys() diff --git a/Dshop/apps/users/tests/test_api_user_details.py b/Dshop/apps/users/tests/test_api_user_details.py index beabd63..dd94afe 100644 --- a/Dshop/apps/users/tests/test_api_user_details.py +++ b/Dshop/apps/users/tests/test_api_user_details.py @@ -1,14 +1,15 @@ +import datetime import json import pytest from django.urls import reverse from rest_framework import status -from ..models import CustomUser +from ..models import CustomUser, Country @pytest.fixture -def user_details_change_url(): +def user_details_url(): return reverse('api-user-details') @@ -17,49 +18,209 @@ def custom_user_instance(user_instance): return CustomUser.objects.create(user=user_instance) +@pytest.fixture() +def country_instance(): + return Country.objects.create(name="Poland", code="PL") + + +@pytest.fixture() +def user_details_data(): + return { + "user_email": "test@pymasters.pl", + "first_name": "test_first_name", + "last_name": "test_last_name", + "address": "test_address", + "postal_code": "123-456", + "city": "test_city", + "country": 1, + "date_of_birth": "2023-12-01", + "phone_number": "123 123 123" + } + + @pytest.mark.django_db -def test_user_details_get_success(api_client, user_details_change_url, custom_user_instance, user_instance_token): +def test_get_user_details_success(api_client, user_details_url, custom_user_instance, user_instance_token): api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - response = api_client.get(user_details_change_url) + response = api_client.get(user_details_url) assert response.status_code == status.HTTP_200_OK response_content = response.data assert response_content['user'] == custom_user_instance.user.id - assert response_content['country'] == custom_user_instance.country assert response_content['email'] == custom_user_instance.user.email assert response_content['first_name'] == custom_user_instance.first_name assert response_content['last_name'] == custom_user_instance.last_name assert response_content['address'] == custom_user_instance.address assert response_content['postal_code'] == custom_user_instance.postal_code assert response_content['city'] == custom_user_instance.city + assert response_content['country'] == custom_user_instance.country assert response_content['date_of_birth'] == custom_user_instance.date_of_birth assert response_content['phone_number'] == custom_user_instance.phone_number @pytest.mark.django_db -def test_user_details_empty_token(api_client, user_details_change_url, user_instance_token, custom_user_instance): +def test_get_user_details_empty_token_failure( + api_client, + user_details_url, + user_instance_token, + custom_user_instance +): api_client.credentials(HTTP_AUTHORIZATION='Token ') - get_response = api_client.get(user_details_change_url) + get_response = api_client.get(user_details_url) assert get_response.status_code == status.HTTP_401_UNAUTHORIZED - get_response = api_client.patch(user_details_change_url, data={}) + get_response = api_client.patch(user_details_url, data={}) assert get_response.status_code == status.HTTP_401_UNAUTHORIZED @pytest.mark.django_db -def test_user_details_patch_success(api_client, user_details_change_url, user_instance_token, custom_user_instance): +def test_put_user_details_success( + api_client, + user_details_url, + user_details_data, + user_instance_token, + custom_user_instance, + country_instance +): + api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') + response = api_client.put(user_details_url, user_details_data) + assert response.status_code == status.HTTP_200_OK + assert len(response.data) == 10 + assert response.data['user'] == user_instance_token.user.id + assert response.data['email'] == user_details_data['user_email'] + assert response.data['first_name'] == user_details_data['first_name'] + assert response.data['last_name'] == user_details_data['last_name'] + assert response.data['address'] == user_details_data['address'] + assert response.data['postal_code'] == user_details_data['postal_code'] + assert response.data['city'] == user_details_data['city'] + assert response.data['country'] == user_details_data['country'] + assert response.data['date_of_birth'] == user_details_data['date_of_birth'] + assert response.data['phone_number'] == user_details_data['phone_number'] + + custom_user = CustomUser.objects.get(user=user_instance_token.user) + assert custom_user.user.email == user_details_data['user_email'] + assert custom_user.first_name == user_details_data['first_name'] + assert custom_user.last_name == user_details_data['last_name'] + assert custom_user.address == user_details_data['address'] + assert custom_user.postal_code == user_details_data['postal_code'] + assert custom_user.city == user_details_data['city'] + assert custom_user.country.id == user_details_data['country'] + assert custom_user.date_of_birth.strftime("%Y-%m-%d") == user_details_data['date_of_birth'] + assert custom_user.phone_number == user_details_data['phone_number'] + + +@pytest.mark.django_db +def test_put_user_details_with_empty_data( + api_client, + user_details_url, + user_details_data, + user_instance_token, + custom_user_instance, + country_instance +): + api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') + response = api_client.put(user_details_url, data={}) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert len(response.data) == 6 + assert 'first_name' in response.data + assert 'last_name' in response.data + assert 'address' in response.data + assert 'postal_code' in response.data + assert 'city' in response.data + assert 'phone_number' in response.data + + +@pytest.mark.django_db +def test_put_user_details_with_2_empty_fields( + api_client, + user_details_url, + user_details_data, + user_instance_token, + custom_user_instance, + country_instance +): + user_details_data['first_name'] = '' + user_details_data['phone_number'] = '' + + api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') + response = api_client.put(user_details_url, data=user_details_data) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert len(response.data) == 2 + assert 'first_name' in response.data + assert 'phone_number' in response.data + + +@pytest.mark.django_db +def test_patch_user_details_success( + api_client, + user_details_url, + user_details_data, + user_instance_token, + custom_user_instance, + country_instance +): api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') - data = { - 'first_name': "test_name", - 'city': "test_city" - } - response = api_client.patch(user_details_change_url, data) + response = api_client.patch(user_details_url, user_details_data) assert response.status_code == status.HTTP_200_OK + assert len(response.data) == 10 + assert response.data['user'] == user_instance_token.user.id + assert response.data['email'] == user_details_data['user_email'] + assert response.data['first_name'] == user_details_data['first_name'] + assert response.data['last_name'] == user_details_data['last_name'] + assert response.data['address'] == user_details_data['address'] + assert response.data['postal_code'] == user_details_data['postal_code'] + assert response.data['city'] == user_details_data['city'] + assert response.data['country'] == user_details_data['country'] + assert response.data['date_of_birth'] == user_details_data['date_of_birth'] + assert response.data['phone_number'] == user_details_data['phone_number'] - response_content = json.loads(response.content) custom_user = CustomUser.objects.get(user=user_instance_token.user) - assert response_content['first_name'] == "test_name" - assert response_content['city'] == "test_city" + assert custom_user.user.email == user_details_data['user_email'] + assert custom_user.first_name == user_details_data['first_name'] + assert custom_user.last_name == user_details_data['last_name'] + assert custom_user.address == user_details_data['address'] + assert custom_user.postal_code == user_details_data['postal_code'] + assert custom_user.city == user_details_data['city'] + assert custom_user.country.id == user_details_data['country'] + assert custom_user.date_of_birth.strftime("%Y-%m-%d") == user_details_data['date_of_birth'] + assert custom_user.phone_number == user_details_data['phone_number'] - assert custom_user.first_name == "test_name" - assert custom_user.city == "test_city" \ No newline at end of file + +@pytest.mark.django_db +def test_patch_user_details_with_empty_data( + api_client, + user_details_url, + user_details_data, + user_instance_token, + custom_user_instance, + country_instance +): + api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') + response = api_client.patch(user_details_url, data={}) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert len(response.data) == 6 + assert 'first_name' in response.data + assert 'last_name' in response.data + assert 'address' in response.data + assert 'postal_code' in response.data + assert 'city' in response.data + assert 'phone_number' in response.data + + +@pytest.mark.django_db +def test_patch_user_details_with_2_empty_fields( + api_client, + user_details_url, + user_details_data, + user_instance_token, + custom_user_instance, + country_instance +): + user_details_data['first_name'] = '' + user_details_data['phone_number'] = '' + + api_client.credentials(HTTP_AUTHORIZATION=f'Token {user_instance_token.key}') + response = api_client.patch(user_details_url, data=user_details_data) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert len(response.data) == 2 + assert 'first_name' in response.data + assert 'phone_number' in response.data \ No newline at end of file