updated code as per code review

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'),
 ]
 

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):

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)

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

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')

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

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()