# Rails thin controller method to change account plan and fire slack message via a service object

I'm a beginner with Rails and am trying to figure out how to keep my controllers clean and thin, and how to work with service objects. I am building an internal tool to manage our SaaS customers, and the specific feature I'm building is to allow admins to change a customer's plan.

In accounts_controller.rb, I have the following method:

def change_plan
account = Account.find(params[:id])

old_plan = account.plan
new_plan = Plan.find(params[:account][:plan_id])

account.change_plan!(params[:account][:plan_id])

SlackService.new.plan_change(current_user, account, old_plan, new_plan).deliver
redirect_to action: "show", id: params[:id]
end


My first question is around the lines 2 and 3 of the method. Is there some place I should be moving this logic? Some sort of SlackService container object? Or should the SlackService method itself be handling the retrieval of the old_plan and new_plan? I realize this is fairly trivial but would love to build it properly.

Here's the code for the SlackService itself, heavily inspired by this article:

require 'net/http'

class SlackService
NAME_AND_ICON = {
icon_emoji: ':bat:'
}

SLACK_WEBHOOK_URL =  "https://hooks.slack.com/services/###/###/###"
SLACK_WEBHOOK_CHANNEL = Rails.env.production? ? "###" : "###"

def initialize(channel = SLACK_WEBHOOK_CHANNEL)
@uri = URI(SLACK_WEBHOOK_URL)
@channel = channel
end

def plan_change(user, account, old_plan, new_plan)
params = {
attachments: [
{
author_name: "#{user.first_name} #{user.last_name}",
text: "Account: #{account.name} (#{account.id})",
title: 'Dashboard Plan Change',
fields: [
{
title: 'Old Plan',
value: "#{old_plan.name} (#{ActionController::Base.helpers.number_to_currency(old_plan.price)})",
short: true
},
{
title: 'New Plan',
value: "#{new_plan.name} (\$#{ActionController::Base.helpers.number_to_currency(new_plan.price)})",
short: true
},
]
}
]
}
self
end

def deliver
begin
Net::HTTP.post_form(@uri, @params)
rescue => e
Rails.logger.error("SlackService: Error when sending: #{e.message}")
end
end

private

{
.merge(channel: @channel)
.merge(params).to_json
}
end
end

1. It seemed that I had to require net/http here in the service, but wasn't sure if that was the proper place for it.
2. I think the SLACK_WEBHOOK_URL and SLACK_WEBHOOK_CHANNEL constants should be defined here.
3. I see many examples of services where the methods are class level, but it seemed to make sense to keep it instance level so you can define the channel on instantiation.
4. Tying in with the above, the plan_change method accepts 4 params, but not sure if there's a cleaner way.
5. Wasn't sure if my approach to loading the NumberHelper is correct.
• Welcome to Code Review, your post looks good, hope you get some good reviews! – ferada Oct 23 '18 at 22:05

def change_plan
account = Account.find(params[:id])

old_plan = account.plan
new_plan = Plan.find(params[:account][:plan_id])

# maybe this method ought to return the new_plan, since it's basically repeating the line above
account.change_plan!(params[:account][:plan_id])

# since account has plan method on you don't necessarily need to pass the new_plan explicitly
SlackService.new.plan_change(current_user, account, old_plan, new_plan).deliver
redirect_to action: "show", id: params[:id]
end


1. It seemed that I had to require net/http here in the service, but wasn't sure if that was the proper place for it.

That's absolutely fine.

1. I think the SLACK_WEBHOOK_URL and SLACK_WEBHOOK_CHANNEL constants should be defined here. These could be defined in a config file.

2. I see many examples of services where the methods are class level, but it seemed to make sense to keep it instance level so you can define the channel on instantiation.

It's fine to use instance methods. However, you can go one step further and define something like

def SlackService.plane_change(current_user:, account:, old_plan: new_plan:)
SlackService.new.plan_change(current_user, account, old_plan, new_plan).deliver
end


The nice thing about this is in your controller, you only have to do SlackService.plane_change(current_user:, account:, old_plan: new_plan:) this in the contoller, and you rid the controller of instantiating the class, calling method etc.

1. Tying in with the above, the plan_change method accepts 4 params, but not sure if there's a cleaner way.

4 is just about ok (see Sandi Metz's article https://robots.thoughtbot.com/sandi-metz-rules-for-developers#four-method-arguments)

1. Wasn't sure if my approach to loading the NumberHelper is correct.

I am not sure I understand this question

• Thanks for the response! I really like your idea of the class level plan change method, maybe with a ! because it delivers? Regarding the NumberHelper, I wasn't sure if ActionController::Base.helpers was the correct way. How do you feel about the controller method as a whole, considering it's a "custom" action? – Erebus Oct 24 '18 at 14:06
• @Erebus The original ruby convention for bang method was that if only have a bang method, if there was a counterpart safer non-bang method alternative i.e there's a Hash#merge! because there's Hash#merge. – Rashmirathi Oct 25 '18 at 13:03