2

I'm using an ActiveModel-based Form Object to handle signup (registration) for an application. The signup class abstracts away information for an account and a user (the primary user for the account).

However, I find that I'm duplicating the validation logic for the account and user inside the signup class. As I was writing my specs (using rspec), I realized that this duplication is probably pointing to a problem with the way I'm handling this.

Is there a way to pass the validation in the signup class off to the account and user models without duplicating it? That way the validation stays in those models and I can reference/call it in the signup class.

Below is the signup class I have that works, but seems to be duplicating code...

class Signup
  include ActiveModel::Model

  # Scopes
  #----------------------------------------------------------------------

  # NOOP

  # Macros
  #----------------------------------------------------------------------

  attr_accessor :slug, :email, :password, :password_confirmation

  # Associations
  #----------------------------------------------------------------------

  # NOOP

  # Validations
  #----------------------------------------------------------------------

  validate :verify_unique_email
  validate :verify_unique_slug
  validates :email, presence: true, format: { with: /@/, message: "is invalid" }
  validates :password, presence: true, length: { minimum: 8 }, confirmation: true
  validates :password_confirmation, presence: true
  validates :slug,
    presence: true,
    format: { with: /\A[\w-]+\z/, message: "is invalid" },
    exclusion: { in: %w[signup signups login] }


  # Methods
  #----------------------------------------------------------------------

  def account
    @account ||= Account.new
  end

  def user
    @user ||= account.build_primary_user
  end

  def save
    account.active = true
    account.slug = slug

    user.email = email
    user.password = password
    user.password_confirmation = password_confirmation

    if valid?
      ActiveRecord::Base.transaction do
        account.save!
        user.save!
      end

      true
    else
      false
    end
  end

  def save!
    save
  end

  private

  def verify_unique_email
    if User.exists?(email: email)
      errors.add :email, "is invalid"
    end
  end

  def verify_unique_slug
    if Account.exists?(slug: slug)
      errors.add :slug, "has already been taken"
    end
  end
end

Here's the account model, note the duplication to validations:

class Account < ActiveRecord::Base
  has_one :primary_user, -> { where(primary: true) }, class_name: User
  has_many :users, dependent: :destroy

  validates :slug,
    uniqueness: true,
    presence: true,
    format: { with: /\A[\w-]+\z/, message: "is invalid" },
    exclusion: { in: %w[signup signups login] }
end
aceofspades
  • 7,568
  • 1
  • 35
  • 48
Dan L
  • 4,319
  • 5
  • 41
  • 74

2 Answers2

1

I like what you're doing using a form object. validates_associated :user, :account might help, but the error messages might be kind of odd. Rather, I might use mixins for common validations:

class Account < ActiveRecord::Base
  module Validations
    extend ActiveSupport::Concern
    included do
      validates :slug, presence: true
    end
  end
  include Validations
end

class Signup
  include ActiveModel::Model
  include Account::Validations
  extend Forwardable
  def_delegators :account, :slug
end
Community
  • 1
  • 1
aceofspades
  • 7,568
  • 1
  • 35
  • 48
  • Unfortunately I tend to agree that an included module may be the cleanest DRY solution to my particular problem. I was hoping there was a way to say something equivalent to: `signup.errors = account.errors + user.errors` but there doesn't seem to be a clean way to implement that without possibly monkey-patching Rails, which I have fairly strict policies against. I'll test out your solution and accept it if it works out. – Dan L May 18 '15 at 23:58
  • For my specific case I decided to stick with duplicating the validations as I may want to have separate validations later on in the `signup` class, but extracting lengthy duplicate validations into a separate module is the best solution I've seen so far. I've looked at delegating validation to the associated objects, copying the actual `object.errors` array, etc..., and none of them are "clean" solutions. – Dan L May 19 '15 at 00:39
  • Have you tried validates_associated maybe with custom messages? – aceofspades May 19 '15 at 00:41
  • @Dan L did you try the above? – Timmy Von Heiss Sep 09 '16 at 20:46
  • @TimmyVonHeiss: I think I actually just ended up using duplicate validations in multiple models. That way everything was in the same file and it would be easier for another developer to understand what's going on instead of jumping around multiple files. – Dan L Sep 10 '16 at 00:18
0

The simplest thing that would work would be:

def valid?
  user.valid? && account.valid? && super
end

You can then get rid of the duplicated validations.

The super is only needed if you need additional validations beyond those on User and Account. This will also add the errors onto the User and Account objects, which might be more useful than on the Signup class.

ptd
  • 3,024
  • 16
  • 29
  • in my particular use case, I actually want the errors to show up on the `signup` object itself as it is an abstraction of the `account` and `user` models. – Dan L May 18 '15 at 23:56