I am battling an error with nested attributes and trying to fix the cop error at the same time. So here is the walk through. A coupon code may be submitted with the form using nested attributes that may affect the price of the job. This only occurs if the coupon code is valid. In this scenario the coupon code has already been assigned so the first if coupon_code && coupon.nil? is triggered. When the form comes back around the flash message works correctly but simple form does not display the value. I could adjust simple form to have the value with an instance variable but I'm starting to smell something a bit off here in my logic. Also, the smell of Assignment Branch Condition is starting to worry me. I can move forward with this, but the user would like to see the code. I would too.
Cop Error:
app/controllers/payments_controller.rb:9:3: C: Assignment Branch Condition size for update is too high. [17.97/15]
Controller:
class PaymentsController < ApplicationController
rescue_from ActiveRecord::RecordNotFound, with: :route_not_found_error
Numeric.include CoreExtensions::Numeric::Percentage
def update
@job = Job.find(params[:job_id])
coupon_code = params[:job][:coupon_attributes][:code]
coupon = validate_coupon(coupon_code)
if coupon_code && coupon.nil?
@coupon_code = coupon_code
flash.now[:error] = t('flash_messages.coupons.id.not_found')
render 'payments/new', layout: 'nested/job/payment'
else
update_job(@job, coupon)
update_coupon(coupon, @job) if coupon
redirect_to @job.vanity_url
end
end
def new
@job = Job.find(params[:job_id])
return if reroute?(@job)
render 'payments/new', layout: 'nested/job/payment'
end
private
def update_job(job, coupon)
job.start_at = DateTime.now
job.end_at = AppConfig.product['settings']['job_active_for_day_num'].days.from_now
job.paid_at = DateTime.now
job.price = price_job(coupon)
# job.save
end
def validate_coupon(coupon_code)
return nil unless coupon_code.present?
coupon = Coupon.active.find_by_code(coupon_code)
return nil unless coupon.present?
coupon
end
def price_job(coupon)
price = AppConfig.product['settings']['job_base_price']
return price unless coupon
price = coupon.percent_discount.percent_of(price)
price
end
def update_coupon(coupon, job)
coupon.job_id = job.id
coupon.executed_at = DateTime.now
coupon.save
end
end
View:
ruby:
content_for :body_id_class, 'PaymentNew'
content_for :js_instance, 'viewPaymentNew'
content_for :browser_title, 'Payment'
job_base_price = AppConfig.product['settings']['job_base_price']
coupon_code = @coupon_code ||= ''
= simple_form_for(@job, url: job_payment_path, html: { id: 'payment-processor-form' }) do |j|
div[class='row']
div[class='col-md-12']
div[class='panel panel-default']
div[class='panel-heading']
h3[class='panel-title']
|Total Cost
div[class='panel-body']
h2[class='job-cost' data-initial = "#{job_base_price}"]
= number_to_currency(job_base_price)
div[class='panel-heading']
h3[class='panel-title']
|Have a coupon?
div[class='panel-body']
div[class='row-inline']
div[class='row-block row-block-one']
= j.simple_fields_for :coupon_attributes, @job.coupon do |c|
= c.input_field :code, maxlength: 50, id: 'coupon-code', class: 'form-control', data: { 'initial' => 0 }, value: coupon_code
div[class='row-block']
button[type='button' class='btn btn-primary' id='coupon-verify' ]
|Verify
p[class='help-hint']
= t('simple_form.hints.coupon.code')
div[class='row']
div[class='col-md-12']
= j.button :button, type: 'button', class: 'btn-primary text-uppercase', id: 'purchase-job' do
= job_posting_button_step_label
Updates
You have quite a few code smells going on in that fat old controller. Most of them seem to be symtoms that all is not well on the model layer and that you are not modeling the domain very well.
You might want to consider something like this:
class Job < ActiveRecord::Base
has_many :payments
end
class Payment < ActiveRecord::Base
belongs_to :job
belongs_to :coupon
end
class Coupon < ActiveRecord::Base
validates_uniqueness_of :code
end
This will let our countroller focus on CRUD'ing a single resouce rather than trying to herd a bunch of cats.
So lets look at enforcing the business logic for coupons.
class Payment < ActiveRecord::Base
belongs_to :job
belongs_to :coupon
validate :coupon_must_be_active
attr_writer :coupon_code
def coupon_code=(code)
coupon = Coupon.find_by(code: code)
@coupon_code = code
end
private
def coupon_must_be_active
if coupon
errors[:coupon] << "must be active." unless coupon.active?
elsif @coupon_code.present?
errors[:coupon_code] << "is not valid."
end
end
end
The custom attribute writer loads the coupon from the a code. The validation sets up our business logic rules.
We really should do the same when it comes to the job pricing:
class Job < ActiveRecord::Base
after_initialize :set_price
def set_price
self.price ||= AppConfig.product['settings']['job_base_price']
end
end
class Payment < ActiveRecord::Base
after_initialize :set_price
validates_presence_of :job
def net_price
return job.price unless coupon
job.price * (coupon.percent_discount * 00.1)
end
# ...
end
We can then write our controller like so:
class PaymentsController
before_action :set_job
# GET /jobs/:job_id/payments/new
def new
@payment = @job.payments.new
end
# POST /jobs/:job_id/payments
def create
@payment = @job.payments.create(payment_params)
end
# PATCH /jobs/:job_id/payments/:id
def update
@payment = @job.payments.find(params[:id])
end
private
def set_job
@job = Job.find(params[:job_id])
end
def payment_params
params.require(:payment)
.permit(:coupon_code)
end
end
We can then simply setup the form with:
= simple_form_for([@job, @payment]) do |f|
= f.input :coupon_code
= f.submit
Note that you don't want to take the price from the user unless you intend to implement the honor system - you should get it from your models by setting up association callbacks.