Skip to content

allow options hash as an argument to change_password!#718

Open
alexagranov wants to merge 1 commit intoNoamB:masterfrom
alexagranov:change_password_with_options
Open

allow options hash as an argument to change_password!#718
alexagranov wants to merge 1 commit intoNoamB:masterfrom
alexagranov:change_password_with_options

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