User types and SRP

Clean Code
1

I think this is a very common situation in Rails apps though I’ve never seen a proposed best practice to solve it.

So far I have 2 classes, User and BusinessUser which use Devise and have the following behaviors:

  1. User instances can sign-up/sign-in with 3 strategies: Email, Twitter and Facebook. Besides, they have certain logic to decide whether it’s necessary to validate email/password.
  2. BusinessUser instances can sign-up/sign-in only with Email, and they run a few AR callbacks before being persisted.

I had them by separate for obvious reasons, but still the Devise logic was coupled across them so I decided to extract it to an EmailAuthenticator AR model which now is solely in charge of handling Devise related matters.

 class User < ActiveRecord::Base
  has_one :email_authenticator, as: :authenticable
  accepts_nested_attributes_for :email_authenticator

  has_many :identities, dependent: :destroy, validate: true
end

 class BusinessUser < ActiveRecord::Base
  has_one :email_authenticator, as: :authenticable
  accepts_nested_attributes_for :email_authenticator
  
  validates :email_authenticator, presence: true 
end

class EmailAuthenticator < ActiveRecord::Base
  attr_accessor :force_password_validation

  belongs_to :authenticable, polymorphic: true

  devise :database_authenticatable, :registerable, :recoverable, :rememberable,
    :trackable, :validatable

  before_validation :set_password, on: :create

  after_create :send_welcome_email

 # Devise method overwrite
  def password_required?
    return true unless authenticable_type == "User"

    force_password_validation || (password.present? ||
      password_confirmation.present?)
  end

  # Devise method overwrite
  def email_required?
    return true unless authenticable_type == "User"

    password_required?
  end

  # Devise method overwrite
  def active_for_authentication?
    if authenticable_type == "BusinessUser"
      super && active
    else
      super
    end
  end

  protected
  def set_password
    return unless authenticable_type == "BusinessUser"

    self.password ||= Devise.friendly_token.first(8)
  end

  def send_welcome_email
    return unless authenticable_type == "BusinessUser"

    Mailer.business_user_welcome(self).deliver
  end
end

As you may notice, EmailAuthenticator is carrying unnecessary logic. Every time we’d need to change any existing auth strategy logic, we’d need to come here. Even worst, if we add new strategies we’d need to insert its custom behavior here as well.

So, the solution I came up with is, to create an EmailAuthenticator subclass for each type of user.

 class User < ActiveRecord::Base
  has_one :email_authenticator, as: :authenticable, class_name: UserEmailAuthenticator
  accepts_nested_attributes_for :email_authenticator

  has_many :identities, dependent: :destroy, validate: true
end

class UserEmailAuthenticator < EmailAuthenticator
  attr_accessor :force_password_validation

  # Devise method overwrite
  def password_required?
    force_password_validation || (password.present? ||
      password_confirmation.present?)
  end

  # Devise method overwrite
  def email_required?
    password_required?
  end
end

I think this way every class would have its own behaviour isolated and only one reason to change, specially EmailAuthenticator.

What do you guys think? Any better approach in mind?

Cheers!

2

Hey guys, I’d love to hear your thoughts on this @jferris @benorenstein @Justin_Gordon @raul