django-rest-passwordreset icon indicating copy to clipboard operation
django-rest-passwordreset copied to clipboard

Wrong field lookup in views

Open mciszczon opened this issue 5 years ago • 13 comments

Hi, thanks for this plugin! It looks like it'll save at least few hours of unnecessary work!

I just have installed the plugin and started getting internal server error from views.py. The issue is the line 102:

users = User.objects.filter(email__iexact=email)

In an app that I'm currently working on, there's no email field, but it's actually a username field that stores the email.

How about updating the plugin, so that it allows to parametrize the name of the field used throughout the data flow? This parameter could be use in views.py, but also in serializers.py (so there are no inconsistencies in parameter between them) and elsewhere.

Thanks!

mciszczon avatar Aug 28 '18 09:08 mciszczon

That's a cool idea.

I guess that it wouldn't be too hard to implement the part with .filter(), though I don't believe that making the serializer dynamic would be a good thing to do. In the end, it should always be an e-mail address to the serializer, it doesn't need to know more then that.

Unfortunately, I'm a little bit out of time at the moment, but I'm happy for someone to make a pull request.

anx-ckreuzberger avatar Aug 30 '18 11:08 anx-ckreuzberger

This is great.

The Django user model by default does not have a 'unique' constraint on the email field, so I'd like to have users enter their username instead.

When can we expect this feature to be released?

J-F-Far avatar May 27 '19 16:05 J-F-Far

@J-F-Far Just created a release candidate for it.

pip install "django-rest-passwordreset==1.1.0rc1"

anx-ckreuzberger avatar May 28 '19 06:05 anx-ckreuzberger

Thanks for your quick response.

I'm having problems with this. I just installed the latest version 1.1.0rc1 into my project, and configured the settings value: 'DJANGO_REST_LOOKUP_FIELD = 'username'.

This hasn't made any difference. The view is still expecting an 'email'.

In the commit tagged '1.1.0rc1', the view 'ResetPasswordRequestToken' still uses the 'EmailSerializer', which is expecting the POSTed value to be a properly formatted EmailField, right?

Have you been able to verify that this works?

J-F-Far avatar May 28 '19 15:05 J-F-Far

@anx-ckreuzberger I forked your repository and fixed this issue, by making the serializer used for the field configurable as well. Perhaps you'd like to review my branch named 'configurable_lookup'. Thanks.

J-F-Far avatar Jun 11 '19 20:06 J-F-Far

Hi,

sorry for the long response time. Thanks for trying it out, and for reporting back, and also for the pull request :+1:

I will try to take a look at it this week.

anx-ckreuzberger avatar Jun 12 '19 05:06 anx-ckreuzberger

I just took a look at it. It looks like in the original PR we did not consider that the lookup field can have a different type (just a different name). The implementation using the serializer in #47 seems to fix this, I've added some comments there.

anx-ckreuzberger avatar Jun 18 '19 15:06 anx-ckreuzberger

Did you pull the code the branch master in my forked repository? As mentioned above, I had these changed on a branch called 'configurable_lookup' which I've since merged into master. I think you may need to build and pull these changes in again as I'm not seeing them in your tag 1.1.0rc1.

Thanks, sorry for the confusion.

J-F-Far avatar Jun 24 '19 19:06 J-F-Far

Sorry but this discussion has exactly what I need it and get the user by username, any news in relation to this?

PunisherGu avatar Jul 18 '19 19:07 PunisherGu

Thanks for the testing and the comments.

Unfortunately the Pull Request #47 by @J-F-Far has been closed. We did find several issues with the original implementation that need a rework before this feature is production ready. At this point also sorry to everyone that I didn't reply in such a long time.

At the moment the release candidate only allows to query users by email address by using an alternative field defined inDJANGO_REST_LOOKUP_FIELD.

The feature Lookup another field that is not neccessarily an email address is currently not possible.

I will see if I can find spare time to add this feature.

anx-ckreuzberger avatar Jul 24 '19 11:07 anx-ckreuzberger

@anx-ckreuzberger Did you find some problems with my implementation of this? As mentioned in a previous comment, I've merged this change into the master branch of my forked project. The original pull request was on a branch in my project called configurable_lookup, which has since been merged into master.

Forgive me is this has been confusing, I'm new to OSS contributing.

J-F-Far avatar Aug 07 '19 22:08 J-F-Far

I've restored the previous branch that the pull request was opened on. Will this allow you to roll the feature into a release candidate?

J-F-Far avatar Aug 07 '19 22:08 J-F-Far

No worries :)

I'll take a look at it, thank you!

anx-ckreuzberger avatar Aug 08 '19 05:08 anx-ckreuzberger