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

Should we have a setup_activation method that saves to database? #605

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

Conversation

weimeng
Copy link
Contributor

@weimeng weimeng commented Aug 5, 2014

Just raising this pull request as a discussion to see if we should be adding this behaviour, or leaving it to the individual to implement on their own.

The reason is that when I saw that there was a setup_activation method, I assumed that it also wrote to the database -- but this turned out not to be true.

I added the following instance method:

def setup_activation!
  setup_activation
  sorcery_adapter.save(:validate => false, :raise_on_failure => true)
end

Then I realised I could just have easily did the following in my own project:

user.setup_activation
user.save

Which should we prefer?

@weimeng
Copy link
Contributor Author

weimeng commented Aug 5, 2014

I also just realised that the existing setup_activation method does not also send an activation email along with it. Should we think about doing this?

@arnvald
Copy link
Collaborator

arnvald commented Aug 18, 2014

Hi @weimeng,

thanks for contribution!

Currently setup_activation is used as a callback and is done before creating user, while sending email is done after user is created. That's why setup_activation does not have sending email itself.
The name of this method is therefore quite confusing, because by calling it you probably expect to do everything what's needed, including sending e-mail.

I need to think about good solution. Maybe indeed it's good idea to have the bang method as well. I'll get back to this issue later this week.

@weimeng
Copy link
Contributor Author

weimeng commented Aug 18, 2014

Glad to help! Let me know, I'll be happy to contribute further code to this as I'm using this functionality in a current project.

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