Extract services to reduce logic in Active Record models

[joemasilotti]

This PR extracts a few services to reduce the amount of business logic in Active Record models, namely notifications and welcome emails. It also helps reduce noise when running bin/check locally because emails are not being triggered when seeding the database.

Pull request checklist

• My code contains tests covering the code I modified
• I linted and tested the project with bin/check
• I added significant changes and product updates to the changelog

[mcansky]

mostly questions about your approach, curious to read about your approach
I do agree there is a nice improvement in separation of responsibility over all but you use two different approaches (object based vs exception catching) for flow control; curious to why

[mcansky]

using exceptions for control flow is a debated practice : sure it makes the code quite clearer and indeed makes this quite nice to read.

yet, in https://github.com/joemasilotti/railsdevs.com/pull/387/files#diff-1fffcf1a4effb60cbfb062145b6ececaffc723517e646b154034d46a0f447e46R12 you relied on a more object approach : return an object with easy to use status methods to check what happened.
why not use this approach here ? this could be less "magical", more explicit and focused to the places where it's actually needed

related :

• https://www.honeybadger.io/blog/benchmarking-exceptions-in-ruby-yep-theyre-slow/

[joemasilotti]

Agreed. Thanks for the push! I refactored to use a third state on Result and have the controller redirect dynamically. Overall I like this approach much better!

[pinzonjulian]

If you instantiate a new invisible developer, the name sort of implies it's already invisible regardless of its invisibility status.

Maybe this could be written in a more job/command style:

InvisibilizeDeveloper.new(developer).perform


InvisibilizeDeveloper.new(developer).execute

[pinzonjulian]

Or you could keep it as is but rename mark as create.

It's not a create in the creating-a-record-in-a-database sense but it does imply that something that didn't exist now exists

[pinzonjulian]

I believe option 1 reads better. What do you think?

[fractaledmind]

Just a passing thought:

ActiveJobs can be run both asynchronously and asynchronously, which makes them (IMO) a wonderful "standard Rails" way to build "service objects that do things".

[joemasilotti]

ActiveJobs can be run both asynchronously and asynchronously, which makes them (IMO) a wonderful "standard Rails" way to build "service objects that do things".

Sure, these POROs could be shoved under Active Job. But what does that buy me? I still have the same problems of where to put authorization and such.

[mcansky]

why not use a guard clause ? such as redirect_to result.message.conversion and return if result.success? (the one liner isn't the nicest way to do.
why : it's mostly esthetic, the guard clause would look more independent and separate

[joemasilotti]

Before

def create
  result = ColdMessage.new(message_params, developer_id:, user: current_user).send_message

  if result.success?
    redirect_to result.message.conversation
  elsif result.redirect?
    redirect_to_result(result)
  else
    @context = context(result.message)
    render :new, status: :unprocessable_entity
  end
end

After

def create
  result = ColdMessage.new(message_params, developer_id:, user: current_user).send_message

  redirect_to result.message.conversation if result.success?
  redirect_to_result(result) if result.redirect?

  @context = context(result.message)
  render :new, status: :unprocessable_entity
end

[joemasilotti]

Saving 4 lines is nice, but I don't think this read any cleaner than what was written before. I'd actually argue it is harder to understand at a glance.

[pinzonjulian]

If you went with the "after" version, you'd need to prepend return to both redirect calls.

As it's written, Rails wouldn't know if it should redirect or render.

For example, if the operation is successful, redirect_to would be executed but the program (i.e the controller action) would keep on running. It'd still execute the action until it hit the render :new call. At this stage Rails would get confused and error out.

[joemasilotti]

Ah, forgot about those. Another reason why I don't think this approach reads cleaner!

[jaredcwhite]

Maybe in this particular case it gets a little messy-looking with guard classes, but in general I agree with @mcansky. I'm nearly always refactoring conditional statements out of the bottom of methods because that feels like an anti-pattern (and one of RuboCop's potential configs is to recommend guard clauses which I like enabling).

[mcansky]

could that be a sign that this action is trying to do too much ?

  if result.success?
    redirect_to result.message.conversation
  elsif result.redirect?
    redirect_to_result(result)

both are, in the end, redirects, couldn't your result object just respond to ''redirect" and have the url in an attr_reader ?, and if you need a notice too have it as another attr_reader or method ?

that way the "logic" is bundled in the result object and the action just has to wonder if it has to redirect or jump back to the form render

[joemasilotti]

Hmm. That is an excellent point. The redirect already includes an optional notice, why not use it here, too?

[joemasilotti]

Closed in favor of #404.