Skip to content
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

allow options hash as an argument to change_password! #718

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alexagranov
Copy link

change_password! delegates to Sorcery::Adapters::ActiveRecordAdapter to save the model.

since Sorcery::Adapters::ActiveRecordAdapter#save already supports an options hash, this would allow passing options such as validate: false to change_password!

@arnvald
Copy link
Collaborator

arnvald commented Sep 18, 2015

Hi @alexagranov

thanks for your contribution! Can you tell me what's the example use case here? I mean, why would you provide validate: false or other params here?

My concern is that Sorcery tries to provide a common API that can be used in various adapters, ActiveRecord being just one of them. If we allow the hash options here, we need to make sure that it is possible to implement the same behaviour in other adapters

@alexagranov
Copy link
Author

Hi @arnvald

I have a case where data about User could have changed between the time they requested a password reset and the moment when they submit the new password. At such time, I do not want to confuse the user with details about the other (invalid) data when all they've done is try to submit a new password (i.e, they're trying to reestablish access to fix the problem). In short, I'm trying to avoid a "chicken & egg" scenario.

I see your point about maintaining a consistent interface but as #save is not available on the parent BaseAdapter, it seems that this is specific to the ActiveRecordAdapter. Perhaps when other adapters are added and #save is abstracted up to the parent BaseAdapter, it may prove prudent to allow that options hash just in case.

Thanks for responding!

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