-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Edit RegistrationsController#create to use ResourceFinder::provider #998
Conversation
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.
I don't have a problem with these changes, they're fairly straightforward.
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.
Thank you for your PR!
@@ -12,10 +12,9 @@ def get_case_insensitive_field_from_resource_params(field) | |||
q_value | |||
end | |||
|
|||
def find_resource(field, value) | |||
def find_resource(field, value, provider) |
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.
I don't understand why the parameter, this won't call the method.
UPDATE: I tested this PR locally and breaks, I would remove this and just use the method
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.
Hi, could you help me out with how to go about overriding the concern in my project that utilises this repo? I would write a concern that extends DeviseTokenAuth::Concerns::ResourceFinder
with a def provider
and override the controllers to include customResourceFinder
. Is this right?
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.
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.
Change the commit message and PR title because now you aren't changing the parameter of find_resource
@@ -28,7 +28,7 @@ def create | |||
end | |||
|
|||
@email = get_case_insensitive_field_from_resource_params(:email) | |||
@resource = find_resource(:uid, @email) | |||
@resource = find_resource(:uid, @email, provider) |
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.
You forgot to remove from here
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.
Thanks, I will change the commit message from here
As discussed in comments of #997,
I've added a provider paramter to find_resource method. I've updated the calls to find_resource method in passwords controller:
sessions controller:
Unlocks controller, I assume, would require an 'email' provider hence I've hardcoded the paramter there:
P.S I'm fairly new to devise_token_auth and devise in general so not sure about the best practices etc.
@MaicolBen @zachfeldman