DEV: Chat service objects #19814

martin-brennan · 2023-01-10T04:56:24Z

This is a combined work of Martin Brennan, Loïc Guitaut, and Joffrey Jaffeux.

This commit implements a base service object when working in chat. The documentation is available at https://discourse.github.io/discourse/chat/backend/Chat/Service.html

Generating documentation has been made as part of this commit with a bigger goal in mind of generally making it easier to dive into the chat project.

Working with services generally involves 3 parts:

The service object itself, which is a series of steps where few of them are specialized (model, transaction, policy)

class UpdateAge
  include Chat::Service::Base

  model :user, :fetch_user
  policy :can_see_user
  contract
  step :update_age

  class Contract
    attribute :age, :integer
  end

  def fetch_user(user_id:, **)
    User.find_by(id: user_id)
  end

  def can_see_user(guardian:, **)
    guardian.can_see_user(user)
  end

  def update_age(age:, **)
    user.update!(age: age)
  end
end

The with_service controller helper, handling success and failure of the service within a service and making easy to return proper response to it from the controller

def update
  with_service(UpdateAge) do
    on_success { render_serialized(result.user, BasicUserSerializer, root: "user") }
  end
end

Rspec matchers and steps inspector, improving the dev experience while creating specs for a service

RSpec.describe(UpdateAge) do
  subject(:result) do
    described_class.call(guardian: guardian, user_id: user.id, age: age)
  end

  fab!(:user) { Fabricate(:user) }
  fab!(:current_user) { Fabricate(:admin) }

  let(:guardian) { Guardian.new(current_user) }
  let(:age) { 1 }

   it { expect(user.reload.age).to eq(age) }
end

Note in case of unexpected failure in your spec, the output will give all the relevant information:

  1) UpdateAge when no channel_id is given is expected to fail to find a model named 'user'
     Failure/Error: it { is_expected.to fail_to_find_a_model(:user) }

       Expected model 'foo' (key: 'result.model.user') was not found in the result object.

       [1/4] [model] 'user' ❌
       [2/4] [policy] 'can_see_user'
       [3/4] [contract] 'default'
       [4/4] [step] 'update_age'

       /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/update_age.rb:32:in `fetch_user': missing keyword: :user_id (ArgumentError)
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:202:in `instance_exec'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:202:in `call'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:219:in `call'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `block in run!'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `each'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:417:in `run!'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:411:in `run'
       	from <internal:kernel>:90:in `tap'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/app/services/base.rb:302:in `call'
       	from /Users/joffreyjaffeux/Code/pr-discourse/plugins/chat/spec/services/update_age_spec.rb:15:in `block (3 levels) in <main>'

lib/service_result.rb

plugins/chat/app/services/services.md

lib/service_base.rb

plugins/chat/app/controllers/chat_controller.rb

martin-brennan · 2023-01-10T05:04:08Z

So far I am liking this, since it's much more standardized and feels cleaner to do things only the one way.

* Add ServiceBase and ServiceResult classes for standardization of service objects * Refactor ChatChannelsController#destroy into ChatChannelDestroyer * Make a ChatMessageCreatorV2 class to demonstrate refactor of existing service

jjaffeux · 2023-01-10T09:40:30Z

OK thanks will need some time to read and think about all of this 👍

martin-brennan · 2023-01-24T07:47:07Z

Prefer raising normal errors (e.g. Discourse::InvalidAccess) for control flow, since they are handled in the controllers etc. If there are no errors raised assume success, and return a hash object if you need to give records back to the caller
Do not force callers to always provide guardian
Always use .call as a class method on the service object class
Pass down ActionController::Parameters, then we can do validations on those in the service object including things like .permit. .to_json then create a new ActionController::Parameters. Probably just bake something on service base class to do this
Do not double up Chat in class names, e.g. only Chat::ChannelDestroyer
No need for things like OpenStruct

Flink · 2023-01-24T11:01:59Z

I like your rationale and where this is going 🙂 The more standardized it is, the easier it is to work with, that’s for sure!

If I may share some thoughts:

I think this is going in the right direction and if we want to standardize how to work with business/service objects we should go even further. At some point, mixing validations, business logic and other stuff is going to make a service object bigger and harder to reason with (at least in my experience 😅). I’ve worked with tools that provided separation of concerns and it was really nice! The best one I can think of is Trailblazer: you have what they call an Operation which is the main business object and is here to organize what happens. Then in your operation you can define steps to be executed with some nice additions like policies, contracts, etc. So, for example, a contract is there to validate and coerce things coming from parameters. That way when doing business logic, you know your data is in a valid state, and you can focus on doing business logic instead of wondering what value is wrong for some reason.

Another small gem I’ve been using is Interactor, this is some kind of very lightweight service object where you create small business logic units and where you can chain them together using what they call an organizer. While it works well, there is no layer for validations or policies, so this is something to add by yourself (which I did at some point).

One thing I haven’t used (yet), is the whole actions from Hanami. If I’m not mistaken, they’re using gems from the DRY ecosystem under the hood, and it seems to provide a rather good experience too.

Now, I’m not saying we should introduce a big set of gems or anything, but we should take a good look at what already exists, so we won’t reinvent the wheel. If we decide to still do things our way, I think there are plenty of good ideas in those tools, and we should probably be inspired by them 🙂

This is, of course, just my opinion 😁

plugins/chat/app/services/services.md

jjaffeux · 2023-01-25T18:25:05Z

@martin-brennan I pushed the results of your ideas, @Flink ideas and my ideas (and interactor, and trailblazer :D). This is not finished, but I pushed it so we can talk of it.

You will also note that I started working on spec and rewrote the destroyer service to use this "ChatService". One of the big focus I have is having very clear spec failures, eg:

  1) Chat::ChannelDestroyer when user is allowed to perform the action trashes the channel
     Failure/Error: expect(result).to succeed

       Contract failed
       ---------------
       Channel can't be blank
     # ./plugins/chat/spec/services/chat_channel_destroyer_spec.rb:23:in `block (3 levels) in <main>'
     # ./spec/rails_helper.rb:355:in `block (2 levels) in <top (required)>'

One of the big missing piece IMO is to write some helper controller to handle failures.

Flink · 2023-01-26T08:39:12Z

One of the big missing piece IMO is to write some helper controller to handle failures.

We’d probably have something by Monday/Tuesday 😁

Now defines before/after/around_call, before/after/around_contract

…jects

ZogStriP · 2023-02-13T10:58:12Z

plugins/chat/spec/services/update_channel_spec.rb

+  context "when channel is a category one" do
+    context "when a valid user provides valid params" do
+      let(:message) do
+        MessageBus.track_publish(ChatPublisher::CHANNEL_EDITS_MESSAGE_BUS_CHANNEL) { result }.first


Not to be addressed in that PR, but we've had some flaky test whenever two pieces of code would publish a message and would break the specs that are looking for the first message in the bus. Do/should we have a way to filter for a certain type of message and then do .first on it? 🤔

yes agreed, generally I think include? on a map of events is just enough

github-actions bot added the chat PRs which include a change to Chat plugin label Jan 10, 2023

martin-brennan force-pushed the dev/chat-service-objects branch 2 times, most recently from 80d49a3 to 2daaa5f Compare January 10, 2023 05:01

martin-brennan commented Jan 10, 2023

View reviewed changes

martin-brennan requested a review from jjaffeux January 10, 2023 05:04

DEV: Chat service objects

9a64eb6

* Add ServiceBase and ServiceResult classes for standardization of service objects * Refactor ChatChannelsController#destroy into ChatChannelDestroyer * Make a ChatMessageCreatorV2 class to demonstrate refactor of existing service

martin-brennan force-pushed the dev/chat-service-objects branch from 2daaa5f to 9a64eb6 Compare January 10, 2023 05:18

nbianca added the 3.1 Post 3.0 release label Jan 10, 2023

martin-brennan added 4 commits January 17, 2023 11:41

Merge branch 'main' into dev/chat-service-objects

6be610c

Merge branch 'main' into dev/chat-service-objects

73cb1f8

Merge branch 'main' into dev/chat-service-objects

36b5750

Merge branch 'main' into dev/chat-service-objects

1efadc5