-
Notifications
You must be signed in to change notification settings - Fork 3
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
#Improve check password on profile delete #267
Changes from 3 commits
bcd9e6f
79bf2eb
362ada3
19c3e84
5eb105e
54350d0
0613517
cb27ec6
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 |
---|---|---|
|
@@ -204,6 +204,15 @@ class Meta: | |
) | ||
read_only_fields = ("person",) | ||
|
||
def validate_for_delete(self, data): | ||
password = data.get("password") | ||
if not password: | ||
raise serializers.ValidationError("Password is required") | ||
user = self.context["request"].user | ||
if not user.check_password(password): | ||
raise serializers.ValidationError("Invalid password") | ||
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. This is not exactly correct. There should be a field name, + probably validation error should not be a string, but a list of strings (with one string in the list). 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. This code is outdated. Here is how code looks like now: 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. Replaced with field validation. |
||
return data | ||
|
||
|
||
class ProfileSensitiveDataROSerializer(serializers.ModelSerializer): | ||
email = serializers.ReadOnlyField(source="person.email") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
import django_filters | ||
from django.shortcuts import get_object_or_404 | ||
from django.contrib.auth.hashers import check_password | ||
from rest_framework import status | ||
from rest_framework.generics import ( | ||
CreateAPIView, | ||
|
@@ -175,19 +174,12 @@ def get_serializer_class(self): | |
else: | ||
return ProfileOwnerDetailEditSerializer | ||
|
||
def destroy(self, request, *args, **kwargs): | ||
instance = self.get_object() | ||
user = self.request.user | ||
password = self.request.data.get("password") | ||
if not password or not check_password(password, user.password): | ||
return Response(status=status.HTTP_400_BAD_REQUEST) | ||
else: | ||
self.perform_destroy(instance) | ||
return Response(status=status.HTTP_204_NO_CONTENT) | ||
|
||
def perform_destroy(self, instance): | ||
instance.is_deleted = True | ||
instance.save() | ||
request_data = {"password": self.request.data.get("password")} | ||
serializer = self.get_serializer(instance) | ||
if serializer.validate_for_delete(request_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. Why do we manually call the validation method? Write the validation method. Use it on serializer field. DRF will care about the rest. 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. Fixed |
||
instance.is_deleted = True | ||
instance.save() | ||
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. def perform_destroy(self, instance):
instance.is_deleted = True
instance.save() ↑ woudn't it be enough? 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. The serializer is not engaging in this case. |
||
|
||
|
||
class ViewedCompanyList(ListCreateAPIView): | ||
|
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.
just write
required=True
in the serializer field, and there will be no need to make it 'manually'.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.
Fixed