My first services + form objects

form_object.rb

class FormObject
  include ActiveModel::Model

  def initialize(args={})
    args.each do |k,v|
      instance_variable_set("@#{k}", v) unless v.nil?
    end
  end

  def save
    return false unless valid?
    save_objects
  end

  def persisted?
    @id.present?
  end

end

policies_controller.rb

#encoding: utf-8
class PoliciesController < AuthenticationController


  def new

    @policy_form = PolicyForm.new(insurance_started_at: Time.now, insurance_finished_at: Time.now+60*60*24*365 - 1.day)
  end

  def new_from_import_record
    @import_record = params[:import_record_class].constantize.find(params[:import_record_id])
    policy_params = @import_record.policy_params(params[:is_renewal].present?, params[:renewal_policy_present].present?)
    @policy_form = PolicyForm.new(policy_params)
    render :new
  end

  def create
    result = policy_service.process_create
    @policy_form = result[:policy_form]
    set_flash_message(result)
    if @policy_form.errors.messages.blank?
      redirect_to policies_path
    else
      render :new
    end
  end

  def edit
    @policy_form = PolicyForm.new(Policy.find(params[:id]).attributes)
  end

  def update #there are few cases where user should be redirected so that looks messy (refactorable with some method probably?)
    result = policy_service.process_update
    @policy_form = result[:policy_form]
    set_flash_message(result)
    if @policy_form.errors.blank?
      if params[:redirect_after_save_to].present?
        redirect_to params[:redirect_after_save_to]
      elsif params[:clearing_view] == 'true'
        redirect_to clearing_policy_path(@policy_form.policy.id)
      else
        redirect_to policy_path(@policy_form.policy.id)
      end
    else
      if params[:policy][:users_attributes].present?
        render :edit
      else
        redirect_to policy_path(@policy_form.policy.id)
      end
    end
  end

  private

  def policy_service
    PolicyService.new(current_user, params, request)
  end

end

policy_form.rb

class PolicyForm < FormObject

  POLICY_FIELDS = [:clearing_status, :client_id, :client_type, :insurance_started_at, :insurance_finished_at, :number, :renewal, :renewed_policy_id,
                   :renewal_number, :payment_type, :program_id, :rejected_from_clearing_notes, :series_id, :status, :producted_at,
                   :sent_to_nau_at, :sent_to_client_at, :sent_to_clearing_at, :sent_to_insurance_agency_at ]

  attr_accessor *POLICY_FIELDS
  attr_accessor :id, :product_category_id
  attr_accessor :installments, :users, :products, :current_user
  attr_accessor :rewrite_additional_data, :client_name, :rewrite_attachments, :fake_field

  def initialize(args={})
    super
    @payment_type ||= 1
    set_installments
    set_users
    set_products
    products_amount
  end

  def policy_attrs
    attrs = {}
    POLICY_FIELDS.each do |f|
      attrs[f] = instance_variable_get("@#{f}") if instance_variable_defined?("@#{f}")
    end
    attrs
  end

  validate do
    unless policy.valid?
      policy.errors.each do |key, values|
        errors[key] = values
      end
    end
  end

  validate do
    if @installments.present?
      if payment_type.to_s == '1'
        @installments.first.payment_amount = products_amount
      else
        errors['installments_amount'] = I18n.t('policy.form_errors.installments_amount') if products_amount != installments_amount
      end
    end
  end

  validate do
    @installments.each do |installment|
      installment.valid?
      installment.errors.each do |key, values|
        errors[key] = values
      end
    end
  end

  validate do
    if policy.renewal && @current_user.cannot?(:change_recommender, Policy) && is_recommender_changed?
      errors['recommender_changed'] = I18n.t('policy.form_errors.recommender_changed')
    end
  end

  def policy
    @policy ||= @id.present? ? Policy.find(@id) : Policy.new
    @policy
  end

  def program
    @program_id.present? ? Program.find(@program_id) : nil
  end

  delegate :product_category, to: :program

  def create
    @policy = Policy.new(policy_attrs)
    save
  end

  def update
    @policy = Policy.find(@id)
    @policy.attributes = policy_attrs
    save
  end

  def save_objects
    ActiveRecord::Base.transaction do
      begin
        set_renewal_client_and_insurance_agency if policy.renewal
        policy.save
        assign_attachments
        if @rewrite_additional_data
          assign_users
          assign_products
          assign_installments
          policy.count_commissions
        end
        true
      rescue Exception => e
        e
      end
    end
  end

  def set_products
    @products = []
    if @products_attributes.present?
      @products_attributes.each do |key, value|
       @products << PoliciesProduct.new(product_id: key, amount: value[:amount])
      end
    elsif policy.persisted?
      @products = policy.policies_products
    end
  end

  def set_installments
    @installments = []
    if @installments_attributes.present?
      @installments_attributes.each do |key, installment|
        @installments << Installment.new(policy: policy, payment_date: installment[:payment_date], payment_amount: installment[:payment_amount], payment_number: installment[:payment_number] )
      end
    elsif policy.persisted?
      @installments = policy.installments
    end
  end

  def set_users
    @users = []
    if @users_attributes.present?
      @users_attributes.each do |k, pu|
        if pu['user_id'].present?
          user = User.find(pu['user_id'])
          @users << PoliciesUser.new(user_id: user.id , sale_role_id: pu['sale_role_id'], contract_id: user.contract_id)
        end
      end
    elsif policy.persisted?
      @users = policy.policies_users
    end
  end

  private

  def assign_users
    policy.policies_users.destroy_all
    @users.each do |pu|
      pu.policy = policy
      pu.save!
    end
  end

  def assign_products
    policy.policies_products.destroy_all
    @products.each do |pp|
      pp.policy = policy
      pp.save!
    end
  end

  def assign_attachments
    if @attachments_attributes.present?
      @attachments_attributes.each do |key, attachment|
        Attachment.create!(asset: attachment['asset'], kind: attachment['kind'], comment: attachment['comment'], record: policy)
      end
    end
  end

  def assign_installments
    policy.installments.destroy_all
    @installments.each do |installment|
      installment.policy = policy
      installment.save
    end
  end

  def products_amount
    if @products_attributes.present?
      @products_attributes.values.inject(0){|s,v| s+v[:amount].to_i}
    else
      0
    end
  end

  def installments_amount
    if @installments_attributes.present?
      @installments_attributes.values.map{|i| i['payment_amount'].to_i}.inject(0, :+).to_i
    else
      0
    end
  end

  def is_recommender_changed?
    if @users_attributes.present?
      policy.renewed_policy.policies_users.find_by_sale_role_id(3).try(:user).try(:id).to_s != @users_attributes['2'][:user_id]
    else
      false
    end
  end

  def set_renewal_client_and_insurance_agency
    if policy.renewed_policy.present?
      policy.renewal_client = true if policy.client == policy.renewed_policy.client
      policy.renewal_insurance_agency = true if policy.program.insurance_agency == policy.renewed_policy.program.insurance_agency
    end
  end



end

policy_service.rb

# it is my first service ever so i would be glad if you have some ideas how should i change it . Even if i have to remove whole code, and refactor all controllers ;]


class PolicyService < BaseService

  def audit_params
    remove_attachments_attributes(@params, 'policy')
  end

  def process_create
    policy_form = PolicyForm.new(params[:policy].merge(current_user: @current_user, audit_params: audit_params, audit_referer: @request.referer))
    result = { }
    if policy_form.create
      result[:notice] = I18n.t('policy.create_success')
      PoliciesMailer.past_insurance_notification(policy_form.policy).deliver if policy_form.policy.insurance_started_at + 60*60*24 < Time.now
    end
    result[:policy_form] = policy_form
    result[:errors] = policy_form.errors
    result
  end

  def process_update
    policy_form = PolicyForm.new(params[:policy].merge(current_user: @current_user, id: params[:id], audit_params: audit_params, audit_referer: @request.referer))
    result = { }
    result[:notice] = I18n.t('policy.update_success') if policy_form.update
    result[:policy_form] = policy_form
    result[:errors] = policy_form.errors
    result
  end

end                                            

Adan Hawkins does exactly what you think is not good idea - raises a ValidationError from the form object. I think that having a rescue_from in the controller to handle validation errors from create, update and other actions touching the record in an uniform way is better than checking @obj.errors.any? in every single action. I don't understand "leak such things to controller layer" - what things? Exceptions from the object that the controller collaborates with directly? "Tell, don't ask".

BTW is there any significant difference between new and edit views? Couldn't it be unified?

I agree that form object should only validate basic things regarding input, more complex logic validations belong to the service. Though, I don't agree that FormObject should pass anything to the service. I think that the FormObject should be injected to the service as a data provider, not created in the constructor, more less like this:

class Serivce
  attr_acessor :data_source
  def initalize(data_source)
    self.data_source = data_source
  end

  def update
    data_source.validate! and validate! and process
  end

  private 

  def validate!
    # logic valdiation
  end

  def process
    # do stuff
  end
end

# controller
  def update
     form_object = PolicyForm.new(params[:policy])
     PolicyService.new(form_object).update
  end