2
\$\begingroup\$

I have an Order object that belongs_to a BillingAddress and a ShippingAddress. I want to present my user with only ShippingAddress fields and a checked checkbox indicating that the billing address matches the shipping address. If the user unchecks the box, the BillingAddress fields appear.

This design proved painful to implement, and my Order object has a lot of callbacks I would rather not have.

class Order < ActiveRecord::Base
  attr_accessor :bill_to_shipping_address

  belongs_to :billing_address,  class_name: 'Address'
  belongs_to :shipping_address, class_name: 'Address'

  accepts_nested_attributes_for :billing_address, :shipping_address

  after_initialize  :set_billing_to_shipping_address
  before_validation :set_billing_address
  after_validation  :clear_billing_address_errors

  # Init the object with option checked
  def set_billing_to_shipping_address
    self.bill_to_shipping_address ||= '1'
  end

  # Copy shipping address attrs to billing address
  def set_billing_address
    self.billing_address = self.shipping_address if bill_to_shipping_address?
  end

  def bill_to_shipping_address?
    bill_to_shipping_address == '1'
  end

  # If shipping address matches billing, we copy the attrs, and thus duplicate errors too.
  # We only need to show the user one set of errors if addresses are the same, so remove them for billing address.
  def clear_billing_address_errors
    if bill_to_shipping_address?
      self.errors.messages.each { |k,v| self.errors.messages.delete(k) if k.to_s.split('.')[0] == 'billing_address' }
    end
  end
end

This feels smelly, and I have four methods along with three registered callbacks to satisfy this need. I have no logic in the controller and the form is relatively simple.

= form_for @order do |f|
    # ...
    = f.label :bill_to_shipping_address, class: 'checkbox' do
      #{f.check_box :bill_to_shipping_address} Use my shipping address as my billing address.

This doesn't feel right. I'm also hacking around the error messages.

Questions:

  1. How can I improve my design?
  2. Would switching the relationships help?

    Order has_one :billing_address and has_one :shipping_address instead of belongs_to. Nested forms will feel more natural; in that, a parent creates children, not the other way around.

I'm reading a fair bit of refactoring books, but I can never map their examples to my own object design. I'm not that experienced I guess.

\$\endgroup\$
2
\$\begingroup\$

Solution still relying on callbacks

validations in rails accept a "condition" option:

validates :shipping_address, presence: true, unless: :ship_to_billing_address?

for simplicity and consistency, i would advise to use a "real" attribute instead of a virtual one for ship_to_billing_address (attributes accessor generated from boolean columns integrate nicely with checkboxes, and rails automatically generates a ship_to_billing_address? method).

In order to ease access to the shipping address, you could either override the shipping_address method :

def shipping_address
  ship_to_billing_address? ? billing_address : super
end

...or create another method, if you fear overrides. Problem is, this approach could induce confusion (modifying in place a shipping address that is in fact a billing address without knowing it, for instance)

Solution without callbacks

Have an eye on DCI architecture. Some gems implement it. Using multiple form objects with different validation rules is another option.

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.