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

Question

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 = {
 username: 'Dashboard',
 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
 },
 ]
 }
 ]
 }
 @params = generate_payload(params)
 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
 def generate_payload(params)
 {
 payload: NAME_AND_ICON
 .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.

• ferada: Welcome to Code Review, your post looks good, hope you get some good reviews!

Answer 1

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

---

Now to address your questions

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.

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

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.

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.

4. 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)

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

I am not sure I understand this question

• Erebus: 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?
• Rashmirathi: @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.