2
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, your post looks good, hope you get some good reviews! \$\endgroup\$ – ferada Oct 23 '18 at 22:05
1
\$\begingroup\$
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.

  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

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Erebus Oct 24 '18 at 14:06
  • \$\begingroup\$ @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. \$\endgroup\$ – Rashmirathi Oct 25 '18 at 13:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.