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 = {
    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 =&gt; 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



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.
I think the SLACK_WEBHOOK_URL and SLACK_WEBHOOK_CHANNEL constants should be defined here.
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.
Tying in with the above, the plan_change method accepts 4 params, but not sure if there's a cleaner way.
Wasn't sure if my approach to loading the NumberHelper is correct.
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


  
  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.


  
  I think the SLACK_WEBHOOK_URL and SLACK_WEBHOOK_CHANNEL constants should be defined here.
  These could be defined in a config file.
  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.


  
  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)


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


I am not sure I understand this question