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                                            

The controller looks mostly ok.

You went from a fat controller to a fat form. I wonder how the form UI actually look like. Can you show a screenshot? The form object is definitely too big, it's responsibilities should be just collecting params and run very simple validations (presence, size), not much more. All else goes to the service and the domain objects, in theory.

https://imageshack.com/i/jwjngkp
https://imageshack.com/i/eu3r1wp
Those are UIs.
So you are saying that all stuff with initializing association data in proper format should be done in service instead of form object? So all my "set_#{products/installments/etc}" should be in service?
The products_amount and installments_amount is for displayin amount of given record in the form - so i think those 2 can stay here ?

Also i thinking about extracting PolicyForm to NewPolicyForm and EditPolicyForm so i won't have so many ifs in almost each method.

Don't check for errors like this if @policy_form.errors.messages.blank? , just redirect to policies_path, raise an exception from the form object and add rescue_from FormObjectExceptionClass { |_| render :new }

About those errors: but if first error will raise exception i will not have @form.errors array to display all validation errors / highlight required inputs f.e.

A good starting point for refactoring might be all those methods that mention different objects (users products attachments) - those are different objects, so how about making them work within smaller form objects that you only instantiate here?

So i in service i should initalize my policy object and all associations object. Then give those associated objects to policy form, and render it ? That way i get smaller policy form but more "form objects" and i can use them when other resource will have f.e. products assocaition also?

I think that #save is not a responsibility of a form object. It's the service responsibility. Form object should run only trivial validations and pass necessary data to the service. I really appreciate Adam Hawkins approach to form objects: http://hawkins.io/2014/01/form_objects_with_virtus/
Using rescue_from in controller is not a good idea imho. Why leak such things to controller layer?

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