Skip to content

Adds Flexibility to change the "reconfirmable_attribute" #1884

Closed
wants to merge 3 commits into from

2 participants

@rmello
rmello commented May 30, 2012

I am using the user's mobile_phone_number as the "confirmation column" since i am writing a system that interacts using SMS (instead of emails).

The confirmation/reconfirmations was tied to :email field so I added two config parameters (kept the default on :email and :unconfirmed_email) so I can change the attribute from the Devise config file.

@rmello rmello Define :confirmable_attribute and :unconfirmed_attribute options to c…
…hange the attribute used on reconfirmations (kept the :email/:unconfirmed_email as defaults)
42e5b08
@josevalim josevalim and 1 other commented on an outdated diff May 30, 2012
lib/devise/models/confirmable.rb
after_update :send_confirmation_instructions, :if => :reconfirmation_required?
+
+ alias_attribute :confirmable_attribute, confirmable_attribute
@josevalim
Plataformatec member
josevalim added a note May 30, 2012

This is not going to work. The module is included before the configuration options are applied.

@rmello
rmello added a note May 30, 2012

Hmm... I get your point. I confess this caused me some problems during development, but at the end it passed all the tests (and it seems to be working correctly on my application). I could not figure out a way to test these other than just running the original devise tests.

@josevalim
Plataformatec member
josevalim added a note May 30, 2012

Just define:

def confirmable_attribute
  send(self.class.confirmable_attribute)
end

And friends.

@rmello
rmello added a note May 30, 2012

I used the alias_attribute to keep the code cleaner. I was avoid the creation of the getter, setter and also the attribute_was and attribute present? methods.
Even if the tests are passing you would suggest the formal definition of the methods?

@josevalim
Plataformatec member
josevalim added a note May 30, 2012

The tests are passing by accident. Add a new test that inherits from the Admin model and changes the value of confirmable_attribute and you will see the change won't propagate.

@rmello
rmello added a note May 30, 2012

Hmmm true. I was changing the confirmable_attribute on the initializer config (and not on the model). I`ll make the changes and add more tests.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim
Plataformatec member

One thing feels odd here: this code is tied to send e-mails with tokens, since you don't send the e-mail, what are you doing? Overriding the delivery method to send a SMS?

@rmello
rmello commented May 30, 2012

Exactly. That felt super-odd on the headers_to method.

@rmello
rmello commented Jun 4, 2012

Added the tests like you suggested and indeed the erros popped out. Defined the accessors (only the necessary ones).
But now i feel like this change is very specific to my application - Not really sure if it belongs on the master repository anyway.
So feel free to pull it or close it ;)

@josevalim
Plataformatec member

Thanks a lot for your patch but I believe it adds too much indirection, making the code harder to maintain. That said, I am closing this. Tks.

@josevalim josevalim closed this Jun 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.