foo.diff

diff --git a/billing_address.rb b/billing_address.rb
new file mode 100644
index 0000000..e3ade88
--- /dev/null
+++ b/billing_address.rb
@@ -0,0 +1,13 @@
+class BillingAddress < ActiveRecord::Base
+  belongs_to :profile
+
+  after_initialize :assign_name_if_empty
+
+  private
+
+  def assign_name_if_empty
+    if self.name.blank? && profile.present?
+      self.name = profile.main_user.full_name 
+    end
+  end
+end
diff --git a/inquery.rb b/inquery.rb
new file mode 100644
index 0000000..5e76f34
--- /dev/null
+++ b/inquery.rb
@@ -0,0 +1,40 @@
+class Inquiry < ActiveRecord::Base
+  attr_accessor :original_user #virtual attribute to access current_user in model
+
+  before_create :copy_riders_from_artist_if_needed 
+  after_commit :copy_riders_to_artist_if_needed, on: :create
+
+  private
+
+  def copy_riders_from_artist_if_needed
+    return if original_user.nil?
+
+    if technical_rider.present? && artist.technical_rider.present? && technical_rider.id == artist.technical_rider.id
+      rider = create_technical_rider(user_id: original_user.id)
+      copy_rider_async(artist.technical_rider.id, rider.id)
+    end
+
+    if catering_rider.present? && artist.catering_rider.present? && catering_rider.id == artist.catering_rider.id
+      rider = create_catering_rider(user_id: original_user.id)
+      copy_rider_async(artist.catering_rider.id, rider.id)
+    end
+  end
+
+  def copy_riders_to_artist_if_needed
+    return if original_user.nil?
+
+    if technical_rider.present? && artist.technical_rider.blank?
+      rider = artist.create_technical_rider(user_id: original_user.id)
+      copy_rider_async(technical_rider.id, rider.id)
+    end
+
+    if catering_rider.present? && artist.catering_rider.blank?
+      rider = artist.create_catering_rider(user_id: original_user.id)
+      copy_rider_async(catering_rider.id, rider.id)
+    end
+  end
+
+  def copy_rider_async(from_rider_id, to_rider_id)
+    MediaItemWorker.perform_async(from_rider_id, to_rider_id)
+  end
+end
diff --git a/inquiries_controller.rb b/inquiries_controller.rb
index a313b04..1f2534d 100644
--- a/inquiries_controller.rb
+++ b/inquiries_controller.rb
@@ -4,92 +4,31 @@ class Gigs::InquiriesController < Gigs::ApplicationController
   respond_to :html, only: [:new, :show]
   respond_to :json, only: [:create]
 
-  before_filter :load_gig,       only: [:create, :new]
+  before_filter :load_data, only: [:create, :new]
 
   def new
-    @inquiry.gig                   = @gig
-    @inquiry.deal_possible_fee_min = @gig.deal_possible_fee_min
-    @inquiry.artist_contact        = current_profile.last_inquired(:artist_contact)
-    @inquiry.travel_party_count    = current_profile.last_inquired(:travel_party_count)
-    @inquiry.custom_fields         = @gig.custom_fields
-
-    if @gig.fixed_fee_option && @gig.fixed_fee_max == 0
-      @inquiry.fixed_fee = 0
-    end
-
-    if @gig.fixed_fee_negotiable
-      @inquiry.gig.fixed_fee_option = true
-      @inquiry.gig.fixed_fee_max    = 0
-    end
-
-    # set this rider here for new
-    # if user keeps it until create, they will be copied async
-    # otherwise he can pseudo delete the riders in the Inquiry#new form and
-    # add new ones
-    @inquiry.technical_rider = current_profile.technical_rider
-    @inquiry.catering_rider  = current_profile.catering_rider
+    prefill_inquiry_from_gig
+    prefill_inquiry_from_current_profile
 
     @is_matching = Gigmit::Matcher.new(@gig, current_profile).matches?
 
-    if current_profile.billing_address.blank? || current_profile.tax_rate.blank?
+    unless current_profile.has_a_complete_billing_address?
       @profile = current_profile
-      if @profile.billing_address.blank?
-        @profile.build_billing_address
-        @profile.billing_address.name = [
-          @profile.main_user.first_name,
-          @profile.main_user.last_name
-        ].join(' ')
-      end
+      @profile.build_billing_address if @profile.billing_address.blank?
     end
 
-    Gigmit::Intercom::Event::ApplicationSawIncompleteBillingDataWarning.emit(@gig.id, current_profile.id) unless current_profile.has_a_complete_billing_address?
-    Gigmit::Intercom::Event::ApplicationSawIncompleteEpkWarning.emit(@gig.id, current_profile.id) unless current_profile.epk_complete?
-
-    Gigmit::Intercom::Event::ApplicationVisitedGigApplicationForm.emit(@gig.id, current_profile.id) if current_profile.complete_for_inquiry?
+    send_events_on_new
   end
 
   def create
-    @inquiry.gig        = @gig
-    @inquiry.artist     = current_profile
-    @inquiry.user       = current_profile.main_user
-    @inquiry.promoter   = @gig.promoter
-    existing_gig_invite = current_profile.gig_invites.where(gig_id: params[:gig_id]).first
-
-    #if inquiry is valid, which means we will definitivly after this, copy
-    #the riders from the current profile to the inquiry
-    if @inquiry.valid?
-      if current_profile.technical_rider.present? && current_profile.technical_rider.item_hash == params[:inquiry][:technical_rider_hash]
-        @inquiry.build_technical_rider(user_id: current_user.id).save!
-        MediaItemWorker.perform_async(current_profile.technical_rider.id, @inquiry.technical_rider.id)
-      end
-
-      if current_profile.catering_rider.present? && current_profile.catering_rider.item_hash == params[:inquiry][:catering_rider_hash]
-        @inquiry.build_catering_rider(user_id: current_user.id).save!
-        MediaItemWorker.perform_async(current_profile.catering_rider.id, @inquiry.catering_rider.id)
-      end
-    end
+    @inquiry.promoter = @gig.promoter
+    @inquiry.artist = current_profile
+    @inquiry.user = current_profile.main_user
+    @inquiry.original_user = current_user
 
     if @inquiry.save
-      #if profile has no rides yet, which means, this is the profiles first inquiry ever
-      #copy the riders from the inquiry to the profile
-      if current_profile.technical_rider.blank? && @inquiry.technical_rider.present?
-        current_profile.build_technical_rider(user_id: current_user.id).save!
-        MediaItemWorker.perform_async(@inquiry.technical_rider.id, current_profile.technical_rider.id)
-      end
-
-      if current_profile.catering_rider.blank? && @inquiry.catering_rider.present?
-        current_profile.build_catering_rider(user_id: current_user.id).save!
-        MediaItemWorker.perform_async(@inquiry.catering_rider.id, current_profile.catering_rider.id)
-      end
-
-      Event::WatchlistArtistInquiry.emit(@inquiry.id)
-
-      Gigmit::Intercom::Event::Simple.emit('gig-received-application', @gig.promoter_id)
-      IntercomCreateOrUpdateUserWorker.perform_async(@gig.promoter_id)
-
-      if existing_gig_invite.present?
-        Event::Read.emit(:gig_invite, existing_gig_invite.id)
-      end
+      send_events_on_create
+
       render json: @inquiry, status: :created
     else
       render json: @inquiry.errors, status: :unprocessable_entity
@@ -107,14 +46,47 @@ class Gigs::InquiriesController < Gigs::ApplicationController
 
   private
 
-  def load_gig
+  def load_data
     @gig = Gig.where(slug: params[:gig_id]).first
+    @inquiry.gig = @gig
   end
 
-  def paywall_chroot
-    if current_profile.artist? && flash[:bypass_trial_chroot] != true
-      # subscribe to premium-trial first to be able to use the platform at all
-      redirect_to '/ab/gigmit-pro-free-trial' and return
+  def prefill_inquiry_from_gig
+    @inquiry.deal_possible_fee_min = @gig.deal_possible_fee_min
+    @inquiry.custom_fields         = @gig.custom_fields
+
+    if @gig.fixed_fee_option && @gig.fixed_fee_max == 0
+      @inquiry.fixed_fee = 0
+    elsif @gig.fixed_fee_negotiable
+      @gig.fixed_fee_option = true # don't know why we assign these values here.Probably for some UI logic
+      @gig.fixed_fee_max    = 0
     end
   end
+
+  def prefill_inquiry_from_current_profile
+    @inquiry.artist_contact        = current_profile.last_inquired(:artist_contact)
+    @inquiry.travel_party_count    = current_profile.last_inquired(:travel_party_count)
+
+    # set this rider here for new
+    # if user keeps it until create, they will be copied async
+    # otherwise he can pseudo delete the riders in the Inquiry#new form and
+    # add new ones
+    @inquiry.technical_rider       = current_profile.technical_rider
+    @inquiry.catering_rider        = current_profile.catering_rider
+  end
+
+  def send_events_on_new
+    Gigmit::Intercom::Event::ApplicationSawIncompleteBillingDataWarning.emit(@gig.id, current_profile.id) unless current_profile.has_a_complete_billing_address?
+    Gigmit::Intercom::Event::ApplicationSawIncompleteEpkWarning.emit(@gig.id, current_profile.id) unless current_profile.epk_complete?
+    Gigmit::Intercom::Event::ApplicationVisitedGigApplicationForm.emit(@gig.id, current_profile.id) if current_profile.complete_for_inquiry?
+  end
+
+  def send_events_on_create
+    Event::WatchlistArtistInquiry.emit(@inquiry.id)
+    Gigmit::Intercom::Event::Simple.emit('gig-received-application', @gig.promoter_id)
+    IntercomCreateOrUpdateUserWorker.perform_async(@gig.promoter_id)
+
+    existing_gig_invite = current_profile.gig_invites.where(gig_id: @gig.id).first
+    Event::Read.emit(:gig_invite, existing_gig_invite.id) if existing_gig_invite.present?
+  end
 end
diff --git a/user.rb b/user.rb
new file mode 100644
index 0000000..03a85b8
--- /dev/null
+++ b/user.rb
@@ -0,0 +1,6 @@
+class User < ActiveRecord::Base
+
+  def full_name
+    "#{first_name} #{last_name}"
+  end
+end

To start with refactoring, I realized new and create actions are doing too much stuff and that makes it hard to read and keep everything in mind. I thought it will be better to move stuff into smaller methods or move some logic into their respective models if possible. So I added these models to be able to move logic there

I have made some assumptions while doing this refactoring because I did not have any access to rest of code

User (user.rb)
I added this model to add a method to get full_name from user. Because I feel its the responsibility of User class to find out how to generate full name from first and last name

BillingAddress (billing_address.rb)
This model I added to add the hook to put the full_name of the user into name field of address on initialization

Inquiry (inquiry.rb)

1. I added this model to be able to use hooks provided by the active record. I saw in our controller, we wanted to move riders from/to profile and vice versa during or after the creation of inquiry. I thought this is a greater use case of using hooks given Inquiry model has everything it needs except current_user. So I added original_user as virtual attribute to be able to access it inside model. In Rails community, there is some debate around using hooks as some people feel that it can make our models quite heavy which I also agree to some extent. I feel controllers should also be quite slim and doing only the job delegating stuff and we can have another layer (services) in between if needed

2. In new action, we assign riders from the profile into inquiry. So if they haven't changed, I should be able to compare them via id (technical_rider.id == artist.technical_rider.id) instead of against other fields like this (current_profile.technical_rider.item_hash == params[:inquiry][:technical_rider_hash]) during creation. This is, of course, an assumption from my side.

3. I can use artist for comparison as it gets assigned as current_profile in the controller

InquiriesController (inquiries_controller.rb)

1. Move all events related code into dedicated methods (send_events_on_new, send_events_on_create) so that our main actions can become small and easier to grasp at first glance. With this refactoring fetching of existing_gig_invite is done after inquiry is saved. I hope this is fine and no gig_invite gets created during the creation of an inquiry

2. Moved some logic into prefill_inquiry_from_gig and prefill_inquiry_from_current_profile. They could have also been assigned during creation but my assumption is that on UI we show them as text field so it can be updated.

3. I replaced current_profile.has_a_complete_billing_address? for current_profile.billing_address.blank? || current_profile.tax_rate.blank?. This is again an assumption. I assume @Profile variable is used to check probably in view if we want user to also enter billing information while filling up the inquiry form.

4. Got rid of paywall_chroot method as it was not used

Final code without diff (for easy reading)

class Gigs::InquiriesController < Gigs::ApplicationController
  load_and_authorize_resource

  respond_to :html, only: [:new, :show]
  respond_to :json, only: [:create]

  before_filter :load_data, only: [:create, :new]

  def new
    prefill_inquiry_from_gig
    prefill_inquiry_from_current_profile

    @is_matching = Gigmit::Matcher.new(@gig, current_profile).matches?

    unless current_profile.has_a_complete_billing_address?
      @profile = current_profile
      @profile.build_billing_address if @profile.billing_address.blank?
    end

    send_events_on_new
  end

  def create
    @inquiry.promoter = @gig.promoter
    @inquiry.artist = current_profile
    @inquiry.user = current_profile.main_user
    @inquiry.original_user = current_user

    if @inquiry.save
      send_events_on_create

      render json: @inquiry, status: :created
    else
      render json: @inquiry.errors, status: :unprocessable_entity
    end
  end

  #only promoter use this
  def show
    #this redirect is for unfixed legacy links, because artist see inquiries
    #not prefixed with gig in the url
    redirect_to inquiry_path(@inquiry.id) and return if current_profile.artist?

    Event::Read.emit(:inquiry, @inquiry.id)
  end

  private

  def load_data
    @gig = Gig.where(slug: params[:gig_id]).first
    @inquiry.gig = @gig
  end

  def prefill_inquiry_from_gig
    @inquiry.deal_possible_fee_min = @gig.deal_possible_fee_min
    @inquiry.custom_fields         = @gig.custom_fields

    if @gig.fixed_fee_option && @gig.fixed_fee_max == 0
      @inquiry.fixed_fee = 0
    elsif @gig.fixed_fee_negotiable
      @gig.fixed_fee_option = true # don't know why we assign these values here.Probably for some UI logic
      @gig.fixed_fee_max    = 0
    end
  end

  def prefill_inquiry_from_current_profile
    @inquiry.artist_contact        = current_profile.last_inquired(:artist_contact)
    @inquiry.travel_party_count    = current_profile.last_inquired(:travel_party_count)

    # set this rider here for new
    # if user keeps it until create, they will be copied async
    # otherwise he can pseudo delete the riders in the Inquiry#new form and
    # add new ones
    @inquiry.technical_rider       = current_profile.technical_rider
    @inquiry.catering_rider        = current_profile.catering_rider
  end

  def send_events_on_new
    Gigmit::Intercom::Event::ApplicationSawIncompleteBillingDataWarning.emit(@gig.id, current_profile.id) unless current_profile.has_a_complete_billing_address?
    Gigmit::Intercom::Event::ApplicationSawIncompleteEpkWarning.emit(@gig.id, current_profile.id) unless current_profile.epk_complete?
    Gigmit::Intercom::Event::ApplicationVisitedGigApplicationForm.emit(@gig.id, current_profile.id) if current_profile.complete_for_inquiry?
  end

  def send_events_on_create
    Event::WatchlistArtistInquiry.emit(@inquiry.id)
    Gigmit::Intercom::Event::Simple.emit('gig-received-application', @gig.promoter_id)
    IntercomCreateOrUpdateUserWorker.perform_async(@gig.promoter_id)

    existing_gig_invite = current_profile.gig_invites.where(gig_id: @gig.id).first
    Event::Read.emit(:gig_invite, existing_gig_invite.id) if existing_gig_invite.present?
  end
end

class Inquiry < ActiveRecord::Base
  attr_accessor :original_user #virtual attribute to access current_user in model

  before_create :copy_riders_from_artist_if_needed 
  after_commit :copy_riders_to_artist_if_needed, on: :create

  private

  def copy_riders_from_artist_if_needed
    return if original_user.nil?

    if technical_rider.present? && artist.technical_rider.present? && technical_rider.id == artist.technical_rider.id
      rider = create_technical_rider(user_id: original_user.id)
      copy_rider_async(artist.technical_rider.id, rider.id)
    end

    if catering_rider.present? && artist.catering_rider.present? && catering_rider.id == artist.catering_rider.id
      rider = create_catering_rider(user_id: original_user.id)
      copy_rider_async(artist.catering_rider.id, rider.id)
    end
  end

  def copy_riders_to_artist_if_needed
    return if original_user.nil?

    if technical_rider.present? && artist.technical_rider.blank?
      rider = artist.create_technical_rider(user_id: original_user.id)
      copy_rider_async(technical_rider.id, rider.id)
    end

    if catering_rider.present? && artist.catering_rider.blank?
      rider = artist.create_catering_rider(user_id: original_user.id)
      copy_rider_async(catering_rider.id, rider.id)
    end
  end

  def copy_rider_async(from_rider_id, to_rider_id)
    MediaItemWorker.perform_async(from_rider_id, to_rider_id)
  end
end

class User < ActiveRecord::Base

  def full_name
    "#{first_name} #{last_name}"
  end
end


class BillingAddress < ActiveRecord::Base
  belongs_to :profile

  after_initialize :assign_name_if_empty

  private

  def assign_name_if_empty
    if self.name.blank? && profile.present?
      self.name = profile.main_user.full_name 
    end
  end
end