Refactoring ChatChannel - distributing into services (#9253)

app/controllers/chat_channels_controller.rb

@@ -36,17 +36,21 @@ class ChatChannelsController < ApplicationController
 
   def create
     authorize ChatChannel
-    @chat_channel = ChatChannelCreationService.new(current_user, params[:chat_channel]).create
+    @chat_channel = current_user.chat_channels.create(
+      channel_type: "invite_only",
+      channel_name: chat_channel_params[:channel_name],
+      slug: chat_channel_params[:slug],
+    )
     render_chat_channel
   end
 
   def update
-    chat_channel = ChatChannelUpdateService.perform(@chat_channel, chat_channel_params)
-    if chat_channel.errors.any?
-      flash[:error] = chat_channel.errors.full_messages.to_sentence
+    @chat_channel.update(chat_channel_params)
+    if @chat_channel.errors.any?
+      flash[:error] = @chat_channel.errors.full_messages.to_sentence
     else
       if chat_channel_params[:discoverable].to_i.zero?
-        ChatChannelMembership.create(user_id: SiteConfig.mascot_user_id, chat_channel_id: chat_channel.id,
+        ChatChannelMembership.create(user_id: SiteConfig.mascot_user_id, chat_channel_id: @chat_channel.id,
                                      role: "member", status: "active")
       else
         ChatChannelMembership.find_by(user_id: SiteConfig.mascot_user_id)&.destroy
@@ -59,13 +63,13 @@ class ChatChannelsController < ApplicationController
   end
 
   def update_channel
-    chat_channel = ChatChannelUpdateService.perform(@chat_channel, chat_channel_params)
-    if chat_channel.errors.any?
-      render json: { success: false, errors: chat_channel.errors.full_messages,
+    @chat_channel.update(chat_channel_params)
+    if @chat_channel.errors.any?
+      render json: { success: false, errors: @chat_channel.errors.full_messages,
                      message: "Channel settings updation failed. Try again later." }, success: :bad_request
     else
       if chat_channel_params[:discoverable]
-        ChatChannelMembership.create(user_id: SiteConfig.mascot_user_id, chat_channel_id: chat_channel.id,
+        ChatChannelMembership.create(user_id: SiteConfig.mascot_user_id, chat_channel_id: @chat_channel.id,
                                      role: "member", status: "active")
       else
         ChatChannelMembership.find_by(user_id: SiteConfig.mascot_user_id)&.destroy
@@ -127,7 +131,7 @@ class ChatChannelsController < ApplicationController
     authorize ChatChannel
 
     if chat_recipient.inbox_type == "open" || valid_listing.length == 1
-      chat = ChatChannel.create_with_users(users: [current_user, chat_recipient], channel_type: "direct")
+      chat = ChatChannels::CreateWithUsers.call(users: [current_user, chat_recipient], channel_type: "direct")
       message_markdown = params[:message]
       message = Message.new(
         chat_channel: chat,

app/controllers/internal/chat_channels_controller.rb

@@ -8,7 +8,7 @@ module Internal
     end
 
     def create
-      ChatChannel.create_with_users(
+      ChatChannels::CreateWithUsers.call(
         users: users_by_param,
         channel_type: "invite_only",
         contrived_name: chat_channel_params[:channel_name],

app/labor/assign_tag_moderator.rb

@@ -32,7 +32,7 @@ module AssignTagModerator
     if tag.mod_chat_channel_id
       ChatChannel.find(tag.mod_chat_channel_id).add_users(user) unless channels.exists?(id: tag.mod_chat_channel_id)
     else
-      channel = ChatChannel.create_with_users(
+      channel = ChatChannels::CreateWithUsers.call(
         users: ([user] + User.with_role(:mod_relations_admin)).flatten.uniq,
         channel_type: "invite_only",
         contrived_name: "##{tag.name} mods",

app/models/chat_channel.rb

@@ -70,51 +70,6 @@ class ChatChannel < ApplicationRecord
     chat_channel_memberships.where(user_id: user.id).pick(:last_opened_at)
   end
 
-  class << self
-    def create_with_users(users:, channel_type: "direct", contrived_name: "New Channel", membership_role: "member")
-      raise "Invalid direct channel" if invalid_direct_channel?(users, channel_type)
-
-      usernames = users.map(&:username).sort
-      slug = if channel_type == "direct"
-               usernames.join("/")
-             else
-               "#{contrived_name.to_s.parameterize}-#{rand(100_000).to_s(26)}"
-             end
-
-      contrived_name = "Direct chat between " + usernames.join(" and ") if channel_type == "direct"
-      channel = find_or_create_chat_channel(channel_type, slug, contrived_name)
-      if channel_type == "direct"
-        channel.add_users(users)
-      else
-        channel.invite_users(users: users, membership_role: membership_role)
-      end
-      channel
-    end
-
-    def find_or_create_chat_channel(channel_type, slug, contrived_name)
-      channel = ChatChannel.find_by(slug: slug)
-      if channel
-        raise "Blocked channel" if channel.status == "blocked"
-
-        channel.status = "active"
-        channel.save
-      else
-        channel = create(
-          channel_type: channel_type,
-          channel_name: contrived_name,
-          slug: slug,
-          last_message_at: 1.week.ago,
-          status: "active",
-        )
-      end
-      channel
-    end
-
-    def invalid_direct_channel?(users, channel_type)
-      (users.size != 2 || users.map(&:id).uniq.count < 2) && channel_type == "direct"
-    end
-  end
-
   def add_users(users)
     now = Time.current
     users_params = Array.wrap(users).map do |user|

app/models/organization_membership.rb

@@ -25,7 +25,7 @@ class OrganizationMembership < ApplicationRecord
     name = "@#{organization.slug} private group chat"
     channel = ChatChannel.find_by(channel_name: name)
 
-    channel ||= ChatChannel.find_or_create_chat_channel("invite_only", "#{organization.slug}-private-group-chat", name)
+    channel ||= ChatChannels::FindOrCreate.call("invite_only", "#{organization.slug}-private-group-chat", name)
 
     add_chat_channel_membership(user, channel, role)
   end

app/services/chat_channel_creation_service.rb

@@ -1,13 +0,0 @@
-class ChatChannelCreationService
-  attr_accessor :user, :params
-
-  def initialize(user, params)
-    @user = user
-    @params = params
-  end
-
-  def create
-    user.chat_channels.create(channel_type: "invite_only",
-                              channel_name: params[:channel_name], slug: params[:slug])
-  end
-end

app/services/chat_channel_update_service.rb

@@ -1,13 +0,0 @@
-class ChatChannelUpdateService < ApplicationService
-  attr_accessor :chat_channel, :params
-
-  def initialize(chat_channel, params)
-    @chat_channel = chat_channel
-    @params = params
-  end
-
-  def perform
-    chat_channel.update(params)
-    chat_channel
-  end
-end

app/services/chat_channels/create_with_users.rb

@@ -0,0 +1,45 @@
+module ChatChannels
+  class CreateWithUsers
+    def initialize(users: [], channel_type: "direct", contrived_name: "New Channel", membership_role: "member")
+      @users = users
+      @channel_type = channel_type
+      @contrived_name = contrived_name
+      @membership_role = membership_role
+    end
+
+    def self.call(*args)
+      new(*args).call
+    end
+
+    def call
+      raise "Invalid direct channel" if invalid_direct_channel?(users, channel_type)
+
+      usernames = users.map(&:username).sort
+      slug = if channel_type == "direct"
+               usernames.join("/")
+             else
+               "#{contrived_name.to_s.parameterize}-#{rand(100_000).to_s(26)}"
+             end
+
+      channel = ChatChannels::FindOrCreate.call(channel_type, slug, verify_contrived_name(usernames))
+      if channel_type == "direct"
+        channel.add_users(users)
+      else
+        channel.invite_users(users: users, membership_role: membership_role)
+      end
+      channel
+    end
+
+    private
+
+    attr_reader :users, :channel_type, :membership_role, :contrived_name
+
+    def invalid_direct_channel?(users, channel_type)
+      (users.size != 2 || users.map(&:id).uniq.count < 2) && channel_type == "direct"
+    end
+
+    def verify_contrived_name(usernames)
+      channel_type == "direct" ? "Direct chat between " + usernames.join(" and ") : contrived_name
+    end
+  end
+end

app/services/chat_channels/find_or_create.rb

@@ -0,0 +1,35 @@
+module ChatChannels
+  class FindOrCreate
+    def initialize(channel_type, slug, contrived_name)
+      @channel_type = channel_type
+      @slug = slug
+      @contrived_name = contrived_name
+    end
+
+    def self.call(*args)
+      new(*args).call
+    end
+
+    def call
+      channel = ChatChannel.find_by(slug: slug)
+      if channel
+        raise "Blocked channel" if channel.status == "blocked"
+
+        channel.update(status: "active")
+      else
+        channel = ChatChannel.create(
+          channel_type: channel_type,
+          channel_name: contrived_name,
+          slug: slug,
+          last_message_at: 1.week.ago,
+          status: "active",
+        )
+      end
+      channel
+    end
+
+    private
+
+    attr_reader :channel_type, :slug, :contrived_name
+  end
+end

app/workers/follows/create_chat_channel_worker.rb

@@ -9,7 +9,7 @@ module Follows
         .find_by(id: follow_id, follower_type: "User", followable_type: "User")
       return unless follow&.followable&.following?(follow.follower)
 
-      ChatChannel.create_with_users(users: [follow.followable, follow.follower])
+      ChatChannels::CreateWithUsers.call(users: [follow.followable, follow.follower])
     end
   end
 end

spec/mailers/notify_mailer_spec.rb

@@ -343,7 +343,7 @@ RSpec.describe NotifyMailer, type: :mailer do
   end
 
   describe "#new_message_email" do
-    let(:direct_channel) { ChatChannel.create_with_users(users: [user, user2], channel_type: "direct") }
+    let(:direct_channel) { ChatChannels::CreateWithUsers.call(users: [user, user2], channel_type: "direct") }
     let(:direct_message) { create(:message, user: user, chat_channel: direct_channel) }
     let(:email) { described_class.with(message: direct_message).new_message_email }
 

spec/models/chat_channel_spec.rb

@@ -34,44 +34,6 @@ RSpec.describe ChatChannel, type: :model do
     end
   end
 
-  describe "#create_with_users" do
-    it "creates channel with users" do
-      chat_channel = described_class.create_with_users(users: users)
-      expect(chat_channel.users.size).to eq(users.size)
-      expect(chat_channel.has_member?(users.first)).to be(true)
-      expect(chat_channel.has_member?(users.last)).to be(true)
-    end
-
-    it "lists active memberships" do
-      chat_channel = described_class.create_with_users(users: users)
-      expect(chat_channel.active_users.size).to eq(users.size)
-      expect(chat_channel.channel_users.size).to eq(users.size)
-    end
-
-    context "when direct channel is invalid" do
-      it "raises an error if users are the same" do
-        user = users.first
-        expect { described_class.create_with_users(users: [user, user]) }.to raise_error("Invalid direct channel")
-      end
-
-      it "raises an error if more than 2 users" do
-        more_users = users + [create(:user)]
-        expect { described_class.create_with_users(users: more_users) }.to raise_error("Invalid direct channel")
-      end
-    end
-  end
-
-  describe "#active_users" do
-    it "decreases active users if one leaves" do
-      chat_channel = described_class.create_with_users(users: users)
-      expect(chat_channel.active_users.size).to eq(users.size)
-      expect(chat_channel.channel_users.size).to eq(users.size)
-      ChatChannelMembership.last.update(status: "left_channel")
-      expect(chat_channel.active_users.size).to eq(users.size - 1)
-      expect(chat_channel.channel_users.size).to eq(users.size - 1)
-    end
-  end
-
   describe "#add_users" do
     it "adds users" do
       expect do

spec/requests/chat_channels_spec.rb

@@ -305,7 +305,7 @@ RSpec.describe "ChatChannels", type: :request do
     end
 
     it "returns error message if create_with_users fails" do
-      allow(ChatChannel).to receive(:create_with_users).and_raise(StandardError.new("Blocked"))
+      allow(ChatChannels::CreateWithUsers).to receive(:call).and_raise(StandardError.new("Blocked"))
       post "/chat_channels/create_chat",
            params: { user_id: user_open_inbox.id }
       expect(response.parsed_body["message"]).to eq("Blocked")

spec/requests/internal/users_manage_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe "Internal::Users", type: :request do
       badge_id: badge.id,
       rewarding_context_message_markdown: "message",
     )
-    ChatChannel.create_with_users(users: [user2, user3], channel_type: "direct")
+    ChatChannels::CreateWithUsers.call(users: [user2, user3], channel_type: "direct")
     user2.follow(user3)
     user.follow(super_admin)
     user3.follow(user2)

spec/requests/video_chats_spec.rb

@@ -12,13 +12,13 @@ RSpec.describe "VideoChats", type: :request do
       end
 
       it "displays basic html for working" do
-        channel = ChatChannel.create_with_users(users: [user, second_user])
+        channel = ChatChannels::CreateWithUsers.call(users: [user, second_user])
         get "/video_chats/#{channel.id}"
         expect(response.body).to include("<div class=\"video-chat-wrapper")
       end
 
       it "disallows unauthorized user" do
-        channel = ChatChannel.create_with_users(users: [second_user, third_user])
+        channel = ChatChannels::CreateWithUsers.call(users: [second_user, third_user])
         expect { get "/video_chats/#{channel.id}" }.to raise_error(Pundit::NotAuthorizedError)
       end
     end

spec/services/chat_channels/create_with_users_spec.rb

@@ -0,0 +1,44 @@
+require "rails_helper"
+
+RSpec.describe ChatChannels::CreateWithUsers, type: :service do
+  let(:chat_channel) { create(:chat_channel) }
+  let(:users) { create_list(:user, 2) }
+
+  describe "#call" do
+    it "creates channel with users" do
+      chat_channel = described_class.call(users: users)
+      expect(chat_channel.users.size).to eq(users.size)
+      expect(chat_channel.has_member?(users.first)).to be(true)
+      expect(chat_channel.has_member?(users.last)).to be(true)
+    end
+
+    it "lists active memberships" do
+      chat_channel = described_class.call(users: users)
+      expect(chat_channel.active_users.size).to eq(users.size)
+      expect(chat_channel.channel_users.size).to eq(users.size)
+    end
+
+    context "when direct channel is invalid" do
+      it "raises an error if users are the same" do
+        user = users.first
+        expect { described_class.call(users: [user, user]) }.to raise_error("Invalid direct channel")
+      end
+
+      it "raises an error if more than 2 users" do
+        more_users = users + [create(:user)]
+        expect { described_class.call(users: more_users) }.to raise_error("Invalid direct channel")
+      end
+    end
+  end
+
+  describe "#active_users" do
+    it "decreases active users if one leaves" do
+      chat_channel = described_class.call(users: users)
+      expect(chat_channel.active_users.size).to eq(users.size)
+      expect(chat_channel.channel_users.size).to eq(users.size)
+      ChatChannelMembership.last.update(status: "left_channel")
+      expect(chat_channel.active_users.size).to eq(users.size - 1)
+      expect(chat_channel.channel_users.size).to eq(users.size - 1)
+    end
+  end
+end

spec/services/chat_channels/find_or_create_spec.rb

@@ -0,0 +1,37 @@
+require "rails_helper"
+
+RSpec.describe ChatChannels::FindOrCreate, type: :service do
+  let(:users) { create_list(:user, 2) }
+  let(:usernames) { users.map(&:username).sort }
+  let(:direct_slug) { usernames.join("/") }
+  let(:other_slug) { "New Channel-#{rand(100_000).to_s(26)}" }
+  let(:contrived_name) { "Direct chat between " + usernames.join(" and ") }
+  let(:open_slug) { "Test channel-#{rand(100_000).to_s(26)}" }
+  let(:chat_channel) do
+    ChatChannel.create(
+      channel_type: "open",
+      channel_name: "Open chat channel for test",
+      slug: open_slug,
+      last_message_at: 1.week.ago,
+      status: "active",
+    )
+  end
+
+  describe "#call find a chat channel" do
+    it "did not find a channel" do
+      new_chat_channel = described_class.call("direct", direct_slug, contrived_name)
+      expect(new_chat_channel.direct?).to be(true)
+      expect(new_chat_channel.status).to eql("active")
+      expect(new_chat_channel.slug).to eql(direct_slug)
+      expect(new_chat_channel.channel_name).to eql(contrived_name)
+    end
+
+    it "found a channel" do
+      found_chat_channel = described_class.call("open", open_slug, "Open chat channel for test")
+      expect(found_chat_channel.open?).to be(true)
+      expect(found_chat_channel.slug).to eql(open_slug)
+      expect(found_chat_channel.channel_name).to eql("Open chat channel for test")
+      expect(found_chat_channel.last_message_at.day).to eq(chat_channel.last_message_at.day)
+    end
+  end
+end

spec/services/users/cleanup_chat_channels_spec.rb

@@ -4,10 +4,10 @@ RSpec.describe Users::CleanupChatChannels, type: :service do
   let(:user) { create(:user) }
   let(:other_user) { create(:user) }
   let!(:dm_channel) do
-    ChatChannel.create_with_users(users: [user, other_user])
+    ChatChannels::CreateWithUsers.call(users: [user, other_user])
   end
   let!(:open_channel) do
-    ChatChannel.create_with_users(users: [user, other_user], channel_type: "open")
+    ChatChannels::CreateWithUsers.call(users: [user, other_user], channel_type: "open")
   end
 
   it "deletes direct chat channels" do

spec/services/users/delete_spec.rb

@@ -167,13 +167,13 @@ RSpec.describe Users::Delete, type: :service do
     let(:other_user) { create(:user) }
 
     it "deletes the user's private chat channels" do
-      chat_channel = ChatChannel.create_with_users(users: [user, other_user])
+      chat_channel = ChatChannels::CreateWithUsers.call(users: [user, other_user])
       described_class.call(user)
       expect(ChatChannel.find_by(id: chat_channel.id)).to be_nil
     end
 
     it "does not delete the user's open channels" do
-      chat_channel = ChatChannel.create_with_users(users: [user, other_user], channel_type: "open")
+      chat_channel = ChatChannels::CreateWithUsers.call(users: [user, other_user], channel_type: "open")
       described_class.call(user)
       expect(ChatChannel.find_by(id: chat_channel.id)).not_to be_nil
     end

spec/workers/moderator/banish_user_worker_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Moderator::BanishUserWorker, type: :worker do
       create(:article, user_id: user.id)
       create(:article, user_id: user.id)
       create(:listing, user: user)
-      ChatChannel.create_with_users(users: [user, user2])
+      ChatChannels::CreateWithUsers.call(users: [user, user2])
       user.follow(user2)
       described_class.new.perform(admin.id, user.id)
       user.reload