Skip to content

Moved some validations to the serializer #88

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

Closed
wants to merge 13 commits into from
Closed

Moved some validations to the serializer #88

wants to merge 13 commits into from

Conversation

marianoeramirez
Copy link
Contributor

I moved some of the validations to the serializer.

@ghost ghost requested a review from anx-cbenke July 27, 2020 10:23
Copy link
Contributor

@anx-cbenke anx-cbenke left a comment

Choose a reason for hiding this comment

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

I would recommend to merge the code-cleanup-parts, but can you resubmit without the phone-related changes as they would clash with #93, which I plan to merge?

Make sure to rebase from master and resolve the current conflicts.

Also please squash your commits, so there is just one commit in this PR. See https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git in case you've not done this before.

@@ -160,7 +136,7 @@ def post(self, request, *args, **kwargs):
if not active_user_found and not getattr(settings, 'DJANGO_REST_PASSWORDRESET_NO_INFORMATION_LEAKAGE', False):
raise exceptions.ValidationError({
'email': [_(
"There is no active user associated with this e-mail address or the password can not be changed")],
"We couldn't find an account associate with that email. Please enter another email account.")],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "We couldn't find an account associated with that email. Please try a different e-mail address."

Comment on lines +25 to +35
def validate_email(self, value):
phone_number = to_python(value)
if phone_number and phone_number.is_valid():
return value

try:
validator = EmailValidator()
validator(value)
return value
except ValidationError:
raise ValidationError(_('Enter a valid phone number or email address.'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Conflicts with #93, which implements a universal lookup field (Not limited to phone/email)

Comment on lines +150 to +153
# if user.password_reset_tokens.all().count() > 0:
# # yes, already has a token, re-use this token
# token = user.password_reset_tokens.all()[0]
# else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove obsolete code entirely

Comment on lines +43 to +71
# get token validation time
password_reset_token_validation_time = get_password_reset_token_expiry_time()

# find token
try:
reset_password_token = _get_object_or_404(models.ResetPasswordToken, key=token)
except (TypeError, ValueError, ValidationError, Http404,
models.ResetPasswordToken.DoesNotExist):
raise Http404(_("The OTP password entered is not valid. Please check and try again."))

# check expiry date
expiry_date = reset_password_token.created_at + timedelta(
hours=password_reset_token_validation_time)

if timezone.now() > expiry_date:
# delete expired token
reset_password_token.delete()
raise Http404(_("The token has expired"))
return data


class PasswordTokenSerializer(PasswordValidateMixin, serializers.Serializer):
password = serializers.CharField(label=_("Password"), style={'input_type': 'password'})
token = serializers.CharField()


class TokenSerializer(serializers.Serializer):
class TokenSerializer(PasswordValidateMixin, serializers.Serializer):
token = serializers.CharField()

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks much cleaner than the functionality in the post-method.

@marianoeramirez
Copy link
Contributor Author

I close this because i created a new one, #105

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants