From 170ea7691f10ff4ef130ab288fdc126ff09b727d Mon Sep 17 00:00:00 2001 From: Vinay Keerthi <11478411+stonecharioteer@users.noreply.github.com> Date: Wed, 8 Oct 2025 20:23:43 +0530 Subject: [PATCH 1/9] feat: Add company model and API with tests (#12548) # Pull Request Template ## Description * add Company model with validations for name, domain, description and avatar * Add database migration fo * Implement endpoints for company CRUD operations * Add optional company relationship for contacts * Add test for models, controllers, factories and policies * Add authorization policies restricting delete to admins * support JSON API responses Please include a summary of the change and issue(s) fixed. Also, mention relevant motivation, context, and any dependencies that this change requires. Fixes #(cw-5650) ## Type of change Please delete options that are not relevant. - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality not to work as expected) - [ ] This change requires a documentation update ## How Has This Been Tested? Tests are implemented using `RSpec` ``` $ bundle exec rails db:migrate $ bundle exec rspec spec/models/company_spec.rb spec/controllers/api/v1/accounts/companies_controller_spec.rb ``` ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --- app/models/contact.rb | 3 + app/models/super_admin.rb | 2 +- app/models/user.rb | 2 +- config/locales/en.yml | 3 + config/routes.rb | 1 + db/migrate/20250929105219_create_companies.rb | 14 ++ .../20250929132305_add_company_to_contacts.rb | 5 + db/schema.rb | 14 ++ .../api/v1/accounts/companies_controller.rb | 40 +++++ enterprise/app/models/company.rb | 33 ++++ .../app/models/enterprise/concerns/account.rb | 1 + .../app/models/enterprise/concerns/contact.rb | 6 + enterprise/app/policies/company_policy.rb | 21 +++ .../accounts/companies/_company.json.jbuilder | 7 + .../accounts/companies/create.json.jbuilder | 3 + .../v1/accounts/companies/index.json.jbuilder | 5 + .../v1/accounts/companies/show.json.jbuilder | 3 + .../accounts/companies/update.json.jbuilder | 3 + lib/limits.rb | 2 + .../v1/accounts/companies_controller_spec.rb | 141 ++++++++++++++++++ spec/enterprise/models/company_spec.rb | 38 +++++ .../policies/company_policy_spec.rb | 33 ++++ spec/factories/companies.rb | 20 +++ 23 files changed, 398 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20250929105219_create_companies.rb create mode 100644 db/migrate/20250929132305_add_company_to_contacts.rb create mode 100644 enterprise/app/controllers/api/v1/accounts/companies_controller.rb create mode 100644 enterprise/app/models/company.rb create mode 100644 enterprise/app/models/enterprise/concerns/contact.rb create mode 100644 enterprise/app/policies/company_policy.rb create mode 100644 enterprise/app/views/api/v1/accounts/companies/_company.json.jbuilder create mode 100644 enterprise/app/views/api/v1/accounts/companies/create.json.jbuilder create mode 100644 enterprise/app/views/api/v1/accounts/companies/index.json.jbuilder create mode 100644 enterprise/app/views/api/v1/accounts/companies/show.json.jbuilder create mode 100644 enterprise/app/views/api/v1/accounts/companies/update.json.jbuilder create mode 100644 spec/enterprise/controllers/api/v1/accounts/companies_controller_spec.rb create mode 100644 spec/enterprise/models/company_spec.rb create mode 100644 spec/enterprise/policies/company_policy_spec.rb create mode 100644 spec/factories/companies.rb diff --git a/app/models/contact.rb b/app/models/contact.rb index a3570b2af..0dc92b51e 100644 --- a/app/models/contact.rb +++ b/app/models/contact.rb @@ -21,6 +21,7 @@ # created_at :datetime not null # updated_at :datetime not null # account_id :integer not null +# company_id :bigint # # Indexes # @@ -28,6 +29,7 @@ # index_contacts_on_account_id_and_contact_type (account_id,contact_type) # index_contacts_on_account_id_and_last_activity_at (account_id,last_activity_at DESC NULLS LAST) # index_contacts_on_blocked (blocked) +# index_contacts_on_company_id (company_id) # index_contacts_on_lower_email_account_id (lower((email)::text), account_id) # index_contacts_on_name_email_phone_number_identifier (name,email,phone_number,identifier) USING gin # index_contacts_on_nonempty_fields (account_id,email,phone_number,identifier) WHERE (((email)::text <> ''::text) OR ((phone_number)::text <> ''::text) OR ((identifier)::text <> ''::text)) @@ -244,3 +246,4 @@ class Contact < ApplicationRecord Rails.configuration.dispatcher.dispatch(CONTACT_DELETED, Time.zone.now, contact: self) end end +Contact.include_mod_with('Concerns::Contact') diff --git a/app/models/super_admin.rb b/app/models/super_admin.rb index 316d60c7b..9bcee9b8a 100644 --- a/app/models/super_admin.rb +++ b/app/models/super_admin.rb @@ -19,7 +19,7 @@ # message_signature :text # name :string not null # otp_backup_codes :text -# otp_required_for_login :boolean default(FALSE), not null +# otp_required_for_login :boolean default(FALSE) # otp_secret :string # provider :string default("email"), not null # pubsub_token :string diff --git a/app/models/user.rb b/app/models/user.rb index 4923d0a35..cc25357f6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -19,7 +19,7 @@ # message_signature :text # name :string not null # otp_backup_codes :text -# otp_required_for_login :boolean default(FALSE), not null +# otp_required_for_login :boolean default(FALSE) # otp_secret :string # provider :string default("email"), not null # pubsub_token :string diff --git a/config/locales/en.yml b/config/locales/en.yml index 0c76f8beb..6afab9253 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -76,6 +76,9 @@ en: invalid: Invalid email phone_number: invalid: should be in e164 format + companies: + domain: + invalid: must be a valid domain name categories: locale: unique: should be unique in the category and portal diff --git a/config/routes.rb b/config/routes.rb index 757d20620..639c51da7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -153,6 +153,7 @@ Rails.application.routes.draw do end end + resources :companies, only: [:index, :show, :create, :update, :destroy] resources :contacts, only: [:index, :show, :update, :create, :destroy] do collection do get :active diff --git a/db/migrate/20250929105219_create_companies.rb b/db/migrate/20250929105219_create_companies.rb new file mode 100644 index 000000000..10fa415c1 --- /dev/null +++ b/db/migrate/20250929105219_create_companies.rb @@ -0,0 +1,14 @@ +class CreateCompanies < ActiveRecord::Migration[7.1] + def change + create_table :companies do |t| + t.string :name, null: false + t.string :domain + t.text :description + t.references :account, null: false + + t.timestamps + end + add_index :companies, [:name, :account_id] + add_index :companies, [:domain, :account_id] + end +end diff --git a/db/migrate/20250929132305_add_company_to_contacts.rb b/db/migrate/20250929132305_add_company_to_contacts.rb new file mode 100644 index 000000000..e79de34b8 --- /dev/null +++ b/db/migrate/20250929132305_add_company_to_contacts.rb @@ -0,0 +1,5 @@ +class AddCompanyToContacts < ActiveRecord::Migration[7.1] + def change + add_reference :contacts, :company, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index f31d05cc3..c0d539f6a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -570,6 +570,18 @@ ActiveRecord::Schema[7.1].define(version: 2025_10_03_091242) do t.index ["phone_number"], name: "index_channel_whatsapp_on_phone_number", unique: true end + create_table "companies", force: :cascade do |t| + t.string "name", null: false + t.string "domain" + t.text "description" + t.bigint "account_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["account_id"], name: "index_companies_on_account_id" + t.index ["domain", "account_id"], name: "index_companies_on_domain_and_account_id" + t.index ["name", "account_id"], name: "index_companies_on_name_and_account_id" + end + create_table "contact_inboxes", force: :cascade do |t| t.bigint "contact_id" t.bigint "inbox_id" @@ -602,6 +614,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_10_03_091242) do t.string "location", default: "" t.string "country_code", default: "" t.boolean "blocked", default: false, null: false + t.bigint "company_id" t.index "lower((email)::text), account_id", name: "index_contacts_on_lower_email_account_id" t.index ["account_id", "contact_type"], name: "index_contacts_on_account_id_and_contact_type" t.index ["account_id", "email", "phone_number", "identifier"], name: "index_contacts_on_nonempty_fields", where: "(((email)::text <> ''::text) OR ((phone_number)::text <> ''::text) OR ((identifier)::text <> ''::text))" @@ -609,6 +622,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_10_03_091242) do t.index ["account_id"], name: "index_contacts_on_account_id" t.index ["account_id"], name: "index_resolved_contact_account_id", where: "(((email)::text <> ''::text) OR ((phone_number)::text <> ''::text) OR ((identifier)::text <> ''::text))" t.index ["blocked"], name: "index_contacts_on_blocked" + t.index ["company_id"], name: "index_contacts_on_company_id" t.index ["email", "account_id"], name: "uniq_email_per_account_contact", unique: true t.index ["identifier", "account_id"], name: "uniq_identifier_per_account_contact", unique: true t.index ["name", "email", "phone_number", "identifier"], name: "index_contacts_on_name_email_phone_number_identifier", opclass: :gin_trgm_ops, using: :gin diff --git a/enterprise/app/controllers/api/v1/accounts/companies_controller.rb b/enterprise/app/controllers/api/v1/accounts/companies_controller.rb new file mode 100644 index 000000000..a33e4c6b2 --- /dev/null +++ b/enterprise/app/controllers/api/v1/accounts/companies_controller.rb @@ -0,0 +1,40 @@ +class Api::V1::Accounts::CompaniesController < Api::V1::Accounts::EnterpriseAccountsController + before_action :check_authorization + before_action :fetch_company, only: [:show, :update, :destroy] + + def index + @companies = Current.account.companies.ordered_by_name + end + + def show; end + + def create + @company = Current.account.companies.build(company_params) + @company.save! + end + + def update + @company.update!(company_params) + end + + def destroy + @company.destroy! + head :ok + end + + private + + def check_authorization + raise Pundit::NotAuthorizedError unless ChatwootApp.enterprise? + + authorize(Company) + end + + def fetch_company + @company = Current.account.companies.find(params[:id]) + end + + def company_params + params.require(:company).permit(:name, :domain, :description, :avatar) + end +end diff --git a/enterprise/app/models/company.rb b/enterprise/app/models/company.rb new file mode 100644 index 000000000..764cb2a9c --- /dev/null +++ b/enterprise/app/models/company.rb @@ -0,0 +1,33 @@ +# == Schema Information +# +# Table name: companies +# +# id :bigint not null, primary key +# description :text +# domain :string +# name :string not null +# created_at :datetime not null +# updated_at :datetime not null +# account_id :bigint not null +# +# Indexes +# +# index_companies_on_account_id (account_id) +# index_companies_on_domain_and_account_id (domain,account_id) +# index_companies_on_name_and_account_id (name,account_id) +# +class Company < ApplicationRecord + include Avatarable + validates :account_id, presence: true + validates :name, presence: true, length: { maximum: Limits::COMPANY_NAME_LENGTH_LIMIT } + validates :domain, allow_blank: true, format: { + with: /\A[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+\z/, + message: I18n.t('errors.companies.domain.invalid') + } + validates :description, length: { maximum: Limits::COMPANY_DESCRIPTION_LENGTH_LIMIT } + + belongs_to :account + has_many :contacts, dependent: :nullify + + scope :ordered_by_name, -> { order(:name) } +end diff --git a/enterprise/app/models/enterprise/concerns/account.rb b/enterprise/app/models/enterprise/concerns/account.rb index b82d84b0a..cae32e86c 100644 --- a/enterprise/app/models/enterprise/concerns/account.rb +++ b/enterprise/app/models/enterprise/concerns/account.rb @@ -13,6 +13,7 @@ module Enterprise::Concerns::Account has_many :captain_custom_tools, dependent: :destroy_async, class_name: 'Captain::CustomTool' has_many :copilot_threads, dependent: :destroy_async + has_many :companies, dependent: :destroy_async has_many :voice_channels, dependent: :destroy_async, class_name: '::Channel::Voice' has_one :saml_settings, dependent: :destroy_async, class_name: 'AccountSamlSettings' diff --git a/enterprise/app/models/enterprise/concerns/contact.rb b/enterprise/app/models/enterprise/concerns/contact.rb new file mode 100644 index 000000000..9139fc67e --- /dev/null +++ b/enterprise/app/models/enterprise/concerns/contact.rb @@ -0,0 +1,6 @@ +module Enterprise::Concerns::Contact + extend ActiveSupport::Concern + included do + belongs_to :company, optional: true + end +end diff --git a/enterprise/app/policies/company_policy.rb b/enterprise/app/policies/company_policy.rb new file mode 100644 index 000000000..1c252967c --- /dev/null +++ b/enterprise/app/policies/company_policy.rb @@ -0,0 +1,21 @@ +class CompanyPolicy < ApplicationPolicy + def index? + true + end + + def show? + true + end + + def create? + true + end + + def update? + true + end + + def destroy? + @account_user.administrator? + end +end diff --git a/enterprise/app/views/api/v1/accounts/companies/_company.json.jbuilder b/enterprise/app/views/api/v1/accounts/companies/_company.json.jbuilder new file mode 100644 index 000000000..71c4d3b9b --- /dev/null +++ b/enterprise/app/views/api/v1/accounts/companies/_company.json.jbuilder @@ -0,0 +1,7 @@ +json.id company.id +json.name company.name +json.domain company.domain +json.description company.description +json.avatar_url company.avatar_url +json.created_at company.created_at +json.updated_at company.updated_at diff --git a/enterprise/app/views/api/v1/accounts/companies/create.json.jbuilder b/enterprise/app/views/api/v1/accounts/companies/create.json.jbuilder new file mode 100644 index 000000000..b3bc80cfd --- /dev/null +++ b/enterprise/app/views/api/v1/accounts/companies/create.json.jbuilder @@ -0,0 +1,3 @@ +json.payload do + json.partial! 'company', company: @company +end diff --git a/enterprise/app/views/api/v1/accounts/companies/index.json.jbuilder b/enterprise/app/views/api/v1/accounts/companies/index.json.jbuilder new file mode 100644 index 000000000..e68bd8543 --- /dev/null +++ b/enterprise/app/views/api/v1/accounts/companies/index.json.jbuilder @@ -0,0 +1,5 @@ +json.payload do + json.array! @companies do |company| + json.partial! 'company', company: company + end +end diff --git a/enterprise/app/views/api/v1/accounts/companies/show.json.jbuilder b/enterprise/app/views/api/v1/accounts/companies/show.json.jbuilder new file mode 100644 index 000000000..b3bc80cfd --- /dev/null +++ b/enterprise/app/views/api/v1/accounts/companies/show.json.jbuilder @@ -0,0 +1,3 @@ +json.payload do + json.partial! 'company', company: @company +end diff --git a/enterprise/app/views/api/v1/accounts/companies/update.json.jbuilder b/enterprise/app/views/api/v1/accounts/companies/update.json.jbuilder new file mode 100644 index 000000000..b3bc80cfd --- /dev/null +++ b/enterprise/app/views/api/v1/accounts/companies/update.json.jbuilder @@ -0,0 +1,3 @@ +json.payload do + json.partial! 'company', company: @company +end diff --git a/lib/limits.rb b/lib/limits.rb index 5da178bf4..c0fc03806 100644 --- a/lib/limits.rb +++ b/lib/limits.rb @@ -6,6 +6,8 @@ module Limits GREETING_MESSAGE_MAX_LENGTH = 10_000 CATEGORIES_PER_PAGE = 1000 AUTO_ASSIGNMENT_BULK_LIMIT = 100 + COMPANY_NAME_LENGTH_LIMIT = 100 + COMPANY_DESCRIPTION_LENGTH_LIMIT = 1000 MAX_CUSTOM_FILTERS_PER_USER = 1000 def self.conversation_message_per_minute_limit diff --git a/spec/enterprise/controllers/api/v1/accounts/companies_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/companies_controller_spec.rb new file mode 100644 index 000000000..f62991ad1 --- /dev/null +++ b/spec/enterprise/controllers/api/v1/accounts/companies_controller_spec.rb @@ -0,0 +1,141 @@ +require 'rails_helper' + +RSpec.describe 'Companies API', type: :request do + let(:account) { create(:account) } + + describe 'GET /api/v1/accounts/{account.id}/companies' do + context 'when it is an unauthenticated user' do + it 'returns unauthorized' do + get "/api/v1/accounts/#{account.id}/companies" + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when it is an authenticated user' do + let(:admin) { create(:user, account: account, role: :administrator) } + let!(:company1) { create(:company, name: 'Company 1', account: account) } + let!(:company2) { create(:company, account: account) } + + it 'returns all companies' do + get "/api/v1/accounts/#{account.id}/companies", + headers: admin.create_new_auth_token, + as: :json + expect(response).to have_http_status(:success) + response_body = response.parsed_body + expect(response_body['payload'].size).to eq(2) + expect(response_body['payload'].map { |c| c['name'] }).to contain_exactly(company1.name, company2.name) + end + end + end + + describe 'GET /api/v1/accounts/{account.id}/companies/{id}' do + context 'when it is an authenticated user' do + let(:admin) { create(:user, account: account, role: :administrator) } + let(:company) { create(:company, account: account) } + + it 'returns the company' do + get "/api/v1/accounts/#{account.id}/companies/#{company.id}", + headers: admin.create_new_auth_token, + as: :json + expect(response).to have_http_status(:success) + response_body = response.parsed_body + expect(response_body['payload']['name']).to eq(company.name) + expect(response_body['payload']['id']).to eq(company.id) + end + end + end + + describe 'POST /api/v1/accounts/{account.id}/companies' do + context 'when it is an authenticated user' do + let(:admin) { create(:user, account: account, role: :administrator) } + let(:valid_params) do + { + company: { + name: 'New Company', + domain: 'newcompany.com', + description: 'A new company' + } + } + end + + it 'creates a new company' do + expect do + post "/api/v1/accounts/#{account.id}/companies", + params: valid_params, + headers: admin.create_new_auth_token, + as: :json + end.to change(Company, :count).by(1) + + expect(response).to have_http_status(:success) + response_body = response.parsed_body + expect(response_body['payload']['name']).to eq('New Company') + expect(response_body['payload']['domain']).to eq('newcompany.com') + end + + it 'returns error for invalid params' do + invalid_params = { company: { name: '' } } + + post "/api/v1/accounts/#{account.id}/companies", + params: invalid_params, + headers: admin.create_new_auth_token, + as: :json + expect(response).to have_http_status(:unprocessable_entity) + end + end + end + + describe 'PATCH /api/v1/accounts/{account.id}/companies/{id}' do + context 'when it is an authenticated user' do + let(:admin) { create(:user, account: account, role: :administrator) } + let(:company) { create(:company, account: account) } + let(:update_params) do + { + company: { + name: 'Updated Company Name', + domain: 'updated.com' + } + } + end + + it 'updates the company' do + patch "/api/v1/accounts/#{account.id}/companies/#{company.id}", + params: update_params, + headers: admin.create_new_auth_token, + as: :json + expect(response).to have_http_status(:success) + response_body = response.parsed_body + expect(response_body['payload']['name']).to eq('Updated Company Name') + expect(response_body['payload']['domain']).to eq('updated.com') + end + end + end + + describe 'DELETE /api/v1/accounts/{account.id}/companies/{id}' do + context 'when it is an authenticated administrator' do + let(:admin) { create(:user, account: account, role: :administrator) } + let(:company) { create(:company, account: account) } + + it 'deletes the company' do + company + expect do + delete "/api/v1/accounts/#{account.id}/companies/#{company.id}", + headers: admin.create_new_auth_token, + as: :json + end.to change(Company, :count).by(-1) + expect(response).to have_http_status(:ok) + end + end + + context 'when it is a regular agent' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:company) { create(:company, account: account) } + + it 'returns unauthorized' do + delete "/api/v1/accounts/#{account.id}/companies/#{company.id}", + headers: agent.create_new_auth_token, + as: :json + expect(response).to have_http_status(:unauthorized) + end + end + end +end diff --git a/spec/enterprise/models/company_spec.rb b/spec/enterprise/models/company_spec.rb new file mode 100644 index 000000000..820e6ee7d --- /dev/null +++ b/spec/enterprise/models/company_spec.rb @@ -0,0 +1,38 @@ +require 'rails_helper' + +RSpec.describe Company, type: :model do + context 'with validations' do + it { is_expected.to validate_presence_of(:account_id) } + it { is_expected.to validate_presence_of(:name) } + it { is_expected.to validate_length_of(:name).is_at_most(100) } + it { is_expected.to validate_length_of(:description).is_at_most(1000) } + + describe 'domain validation' do + it { is_expected.to allow_value('example.com').for(:domain) } + it { is_expected.to allow_value('sub.example.com').for(:domain) } + it { is_expected.to allow_value('').for(:domain) } + it { is_expected.to allow_value(nil).for(:domain) } + it { is_expected.not_to allow_value('invalid-domain').for(:domain) } + it { is_expected.not_to allow_value('.example.com').for(:domain) } + end + end + + context 'with associations' do + it { is_expected.to belong_to(:account) } + it { is_expected.to have_many(:contacts).dependent(:nullify) } + end + + describe 'scopes' do + let(:account) { create(:account) } + let!(:company_b) { create(:company, name: 'B Company', account: account) } + let!(:company_a) { create(:company, name: 'A Company', account: account) } + let!(:company_c) { create(:company, name: 'C Company', account: account) } + + describe '.ordered_by_name' do + it 'orders companies by name alphabetically' do + companies = described_class.where(account: account).ordered_by_name + expect(companies.map(&:name)).to eq([company_a.name, company_b.name, company_c.name]) + end + end + end +end diff --git a/spec/enterprise/policies/company_policy_spec.rb b/spec/enterprise/policies/company_policy_spec.rb new file mode 100644 index 000000000..9f7d9f0cc --- /dev/null +++ b/spec/enterprise/policies/company_policy_spec.rb @@ -0,0 +1,33 @@ +require 'rails_helper' + +RSpec.describe CompanyPolicy, type: :policy do + subject(:company_policy) { described_class } + + let(:account) { create(:account) } + let(:administrator) { create(:user, :administrator, account: account) } + let(:agent) { create(:user, account: account) } + let(:company) { create(:company, account: account) } + + let(:administrator_context) { { user: administrator, account: account, account_user: account.account_users.first } } + let(:agent_context) { { user: agent, account: account, account_user: account.account_users.first } } + + permissions :index?, :show?, :create?, :update? do + context 'when administrator' do + it { expect(company_policy).to permit(administrator_context, company) } + end + + context 'when agent' do + it { expect(company_policy).to permit(agent_context, company) } + end + end + + permissions :destroy? do + context 'when administrator' do + it { expect(company_policy).to permit(administrator_context, company) } + end + + context 'when agent' do + it { expect(company_policy).not_to permit(agent_context, company) } + end + end +end diff --git a/spec/factories/companies.rb b/spec/factories/companies.rb new file mode 100644 index 000000000..bdf7e9e9f --- /dev/null +++ b/spec/factories/companies.rb @@ -0,0 +1,20 @@ +FactoryBot.define do + factory :company do + sequence(:name) { |n| "Company #{n}" } + sequence(:domain) { |n| "company#{n}.com" } + description { 'A sample company description' } + account + + trait :without_domain do + domain { nil } + end + + trait :with_avatar do + avatar { fixture_file_upload(Rails.root.join('spec/assets/avatar.png'), 'image/png') } + end + + trait :with_long_description do + description { 'A' * 500 } + end + end +end From 7c5bb343c67ae371ee47eeb4fadb354794a0505d Mon Sep 17 00:00:00 2001 From: Vishnu Narayanan Date: Thu, 9 Oct 2025 16:04:50 +0530 Subject: [PATCH 2/9] fix: Optimize message reindexing to reduce sidekiq job creation (#12618) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes searchkick callback behavior to check `should_index?` before creating reindex jobs, preventing unnecessary job creation for messages that don't need indexing (activity messages, unpaid accounts, etc.). Previously, `callbacks: :async` created reindex jobs for all messages (~5,100/min or 7.3M/day in production), which were then filtered by `should_index?` inside the job worker - resulting in 98% wasted jobs, Redis memory pressure, and avoidable p0 alerts. Now, `should_index?` is checked before job creation via `after_commit` callback, reducing job creation to actual incoming/outgoing messages from paid accounts. Changes: - Disable automatic searchkick callbacks - Add manual `after_commit` callback with `should_index?` condition - Add specs to verify callback behavior Expected impact: - 98% reduction in sidekiq job creation (~7.3M → ~150K jobs/day) - Reduced redis memory usage - Same async indexing behavior for eligible messages --- app/models/message.rb | 7 ++++- spec/models/message_spec.rb | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/app/models/message.rb b/app/models/message.rb index dbab19df3..2079e31a9 100644 --- a/app/models/message.rb +++ b/app/models/message.rb @@ -39,7 +39,7 @@ # class Message < ApplicationRecord - searchkick callbacks: :async if ChatwootApp.advanced_search_allowed? + searchkick callbacks: false if ChatwootApp.advanced_search_allowed? include MessageFilterHelpers include Liquidable @@ -135,6 +135,7 @@ class Message < ApplicationRecord after_create_commit :execute_after_create_commit_callbacks after_update_commit :dispatch_update_event + after_commit :reindex_for_search, if: :should_index?, on: [:create, :update] def channel_token @token ||= inbox.channel.try(:page_access_token) @@ -436,6 +437,10 @@ class Message < ApplicationRecord conversation.update_columns(last_activity_at: created_at) # rubocop:enable Rails/SkipsModelValidations end + + def reindex_for_search + reindex(mode: :async) + end end Message.prepend_mod_with('Message') diff --git a/spec/models/message_spec.rb b/spec/models/message_spec.rb index a0bd48e39..c5e080677 100644 --- a/spec/models/message_spec.rb +++ b/spec/models/message_spec.rb @@ -4,6 +4,12 @@ require 'rails_helper' require Rails.root.join 'spec/models/concerns/liquidable_shared.rb' RSpec.describe Message do + before do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(described_class).to receive(:reindex_for_search).and_return(true) + # rubocop:enable RSpec/AnyInstance + end + context 'with validations' do it { is_expected.to validate_presence_of(:inbox_id) } it { is_expected.to validate_presence_of(:conversation_id) } @@ -678,4 +684,54 @@ RSpec.describe Message do end end end + + describe '#reindex_for_search callback' do + let(:account) { create(:account) } + let(:conversation) { create(:conversation, account: account) } + + before do + allow(ChatwootApp).to receive(:advanced_search_allowed?).and_return(true) + account.enable_features('advanced_search_indexing') + end + + context 'when message should be indexed' do + it 'calls reindex_for_search for incoming message on create' do + message = build(:message, conversation: conversation, account: account, message_type: :incoming) + expect(message).to receive(:reindex_for_search) + message.save! + end + + it 'calls reindex_for_search for outgoing message on update' do + # rubocop:disable RSpec/AnyInstance + allow_any_instance_of(described_class).to receive(:reindex_for_search).and_return(true) + # rubocop:enable RSpec/AnyInstance + message = create(:message, conversation: conversation, account: account, message_type: :outgoing) + expect(message).to receive(:reindex_for_search).and_return(true) + message.update!(content: 'Updated content') + end + end + + context 'when message should not be indexed' do + it 'does not call reindex_for_search for activity message' do + message = build(:message, conversation: conversation, account: account, message_type: :activity) + expect(message).not_to receive(:reindex_for_search) + message.save! + end + + it 'does not call reindex_for_search for unpaid account on cloud' do + allow(ChatwootApp).to receive(:chatwoot_cloud?).and_return(true) + account.disable_features('advanced_search_indexing') + message = build(:message, conversation: conversation, account: account, message_type: :incoming) + expect(message).not_to receive(:reindex_for_search) + message.save! + end + + it 'does not call reindex_for_search when advanced search is not allowed' do + allow(ChatwootApp).to receive(:advanced_search_allowed?).and_return(false) + message = build(:message, conversation: conversation, account: account, message_type: :incoming) + expect(message).not_to receive(:reindex_for_search) + message.save! + end + end + end end From f89ed562586a27dfc13877c12ac519a8b26245ca Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Thu, 9 Oct 2025 16:50:28 +0530 Subject: [PATCH 3/9] feat: update rack version (#12628) Fixes CI failing at bundle audit for a [rack vulnerability](https://github.com/rack/rack/security/advisories/GHSA-wpv5-97wm-hp9c) --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 283cf3b83..18eeffc3b 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -644,7 +644,7 @@ GEM activesupport (>= 3.0.0) raabro (1.4.0) racc (1.8.1) - rack (3.2.0) + rack (3.2.2) rack-attack (6.7.0) rack (>= 1.0, < 4) rack-contrib (2.5.0) From 6cc69f444b83d4a0db11f14b8ff1ee75510daab8 Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Thu, 9 Oct 2025 16:51:25 +0530 Subject: [PATCH 4/9] chore: Include 11:59 PM slot in business hours display (#12610) --- .../settings/inbox/components/BusinessDay.vue | 13 +++-- .../settings/inbox/helpers/businessHour.js | 8 +++ .../inbox/helpers/specs/businessHour.spec.js | 58 ++++++++++++++++++- 3 files changed, 70 insertions(+), 9 deletions(-) diff --git a/app/javascript/dashboard/routes/dashboard/settings/inbox/components/BusinessDay.vue b/app/javascript/dashboard/routes/dashboard/settings/inbox/components/BusinessDay.vue index 14ba5b371..b90ea92ad 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/inbox/components/BusinessDay.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/inbox/components/BusinessDay.vue @@ -96,11 +96,12 @@ export default { return parse(this.toTime, 'hh:mm a', new Date()); }, totalHours() { - if (this.timeSlot.openAllDay) { - return 24; - } - const totalHours = differenceInMinutes(this.toDate, this.fromDate) / 60; - return totalHours; + if (this.timeSlot.openAllDay) return '24h'; + + const totalMinutes = differenceInMinutes(this.toDate, this.fromDate); + const [h, m] = [Math.floor(totalMinutes / 60), totalMinutes % 60]; + + return [h && `${h}h`, m && `${m}m`].filter(Boolean).join(' ') || '0m'; }, hasError() { return !this.timeSlot.valid; @@ -211,7 +212,7 @@ export default { v-if="isDayEnabled && !hasError" class="label bg-n-brand/10 dark:bg-n-brand/30 text-n-blue-text text-xs inline-block px-2 py-1 rounded-lg cursor-default whitespace-nowrap" > - {{ totalHours }} {{ $t('INBOX_MGMT.BUSINESS_HOURS.DAY.HOURS') }} + {{ totalHours }} diff --git a/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/businessHour.js b/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/businessHour.js index 69089bf3c..b73368035 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/businessHour.js +++ b/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/businessHour.js @@ -53,6 +53,7 @@ export const generateTimeSlots = (step = 15) => { Generates a list of time strings from 12:00 AM to next 24 hours. Each new string will be generated by adding `step` minutes to the previous one. The list is generated by starting with a random day and adding step minutes till end of the same day. + Always includes 11:59 PM as the final slot to complete the day. */ const date = new Date(1970, 1, 1); const slots = []; @@ -66,6 +67,13 @@ export const generateTimeSlots = (step = 15) => { ); date.setMinutes(date.getMinutes() + step); } + + // Always add 11:59 PM as the final slot if it's not already included + const lastSlot = '11:59 PM'; + if (!slots.includes(lastSlot)) { + slots.push(lastSlot); + } + return slots; }; diff --git a/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/specs/businessHour.spec.js b/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/specs/businessHour.spec.js index 077337ae6..c9cfa2d24 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/specs/businessHour.spec.js +++ b/app/javascript/dashboard/routes/dashboard/settings/inbox/helpers/specs/businessHour.spec.js @@ -7,10 +7,19 @@ import { } from '../businessHour'; describe('#generateTimeSlots', () => { - it('returns correct number of time slots', () => { - expect(generateTimeSlots(15).length).toStrictEqual((60 / 15) * 24); + it('returns correct number of time slots for 15-minute intervals', () => { + const slots = generateTimeSlots(15); + // 24 hours * 4 slots per hour + 1 for 11:59 PM = 97 slots + expect(slots.length).toStrictEqual(97); }); - it('returns correct time slots', () => { + + it('returns correct number of time slots for 30-minute intervals', () => { + const slots = generateTimeSlots(30); + // 24 hours * 2 slots per hour + 1 for 11:59 PM = 49 slots + expect(slots.length).toStrictEqual(49); + }); + + it('returns correct time slots for 4-hour intervals', () => { expect(generateTimeSlots(240)).toStrictEqual([ '12:00 AM', '04:00 AM', @@ -18,8 +27,51 @@ describe('#generateTimeSlots', () => { '12:00 PM', '04:00 PM', '08:00 PM', + '11:59 PM', ]); }); + + it('always starts with 12:00 AM', () => { + expect(generateTimeSlots(15)[0]).toStrictEqual('12:00 AM'); + expect(generateTimeSlots(30)[0]).toStrictEqual('12:00 AM'); + expect(generateTimeSlots(60)[0]).toStrictEqual('12:00 AM'); + }); + + it('always ends with 11:59 PM', () => { + const slots15 = generateTimeSlots(15); + const slots30 = generateTimeSlots(30); + const slots60 = generateTimeSlots(60); + + expect(slots15[slots15.length - 1]).toStrictEqual('11:59 PM'); + expect(slots30[slots30.length - 1]).toStrictEqual('11:59 PM'); + expect(slots60[slots60.length - 1]).toStrictEqual('11:59 PM'); + }); + + it('includes 11:59 PM even when it would not be in regular intervals', () => { + const slots = generateTimeSlots(30); + expect(slots).toContain('11:59 PM'); + expect(slots).toContain('11:30 PM'); // Regular interval + }); + + it('does not duplicate 11:59 PM if it already exists in regular intervals', () => { + // Test with a step that would naturally include 11:59 PM + const slots = generateTimeSlots(1); // 1-minute intervals + const count11_59 = slots.filter(slot => slot === '11:59 PM').length; + expect(count11_59).toStrictEqual(1); + }); + + it('generates correct time format', () => { + const slots = generateTimeSlots(60); + expect(slots).toContain('01:00 AM'); + expect(slots).toContain('12:00 PM'); + expect(slots).toContain('01:00 PM'); + expect(slots).toContain('11:00 PM'); + }); + + it('handles edge case with very large step', () => { + const slots = generateTimeSlots(1440); // 24 hours + expect(slots).toStrictEqual(['12:00 AM', '11:59 PM']); + }); }); describe('#getTime', () => { From 6829328182df37a66e4afe5cff50bf60ab8b87d2 Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Thu, 9 Oct 2025 18:19:20 +0530 Subject: [PATCH 5/9] fix(filters): correct null matching logic [CW-5741] (#12627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes a bug came from assuming the old null check only mattered for the `is_not_present` filter. The fix keeps `not_equal_to` working but lets each operator decide what to do with `null`. Presence filters look at a shared `isNullish` flag, text filters still rely on `contains`, and date filters skip conversations with no timestamp. The new spec covers the null-assignee scenario for both `equal_to` and `not_equal_to` so we don’t miss this again. --- .../conversations/helpers/filterHelpers.js | 19 +++-- .../helpers/specs/filterHelpers.spec.js | 78 +++++++++++++++++++ 2 files changed, 90 insertions(+), 7 deletions(-) diff --git a/app/javascript/dashboard/store/modules/conversations/helpers/filterHelpers.js b/app/javascript/dashboard/store/modules/conversations/helpers/filterHelpers.js index 3d627e3ef..96f4a0123 100644 --- a/app/javascript/dashboard/store/modules/conversations/helpers/filterHelpers.js +++ b/app/javascript/dashboard/store/modules/conversations/helpers/filterHelpers.js @@ -154,7 +154,10 @@ const equalTo = (filterValue, conversationValue) => { * It only works with string values and returns false for non-string types. */ const contains = (filterValue, conversationValue) => { - if (typeof conversationValue === 'string') { + if ( + typeof conversationValue === 'string' && + typeof filterValue === 'string' + ) { return conversationValue.toLowerCase().includes(filterValue.toLowerCase()); } return false; @@ -190,10 +193,8 @@ const compareDates = (conversationValue, filterValue, compareFn) => { const matchesCondition = (conversationValue, filter) => { const { filter_operator: filterOperator, values } = filter; - // Handle null/undefined values - if (conversationValue === null || conversationValue === undefined) { - return filterOperator === 'is_not_present'; - } + const isNullish = + conversationValue === null || conversationValue === undefined; const filterValue = Array.isArray(values) ? values.map(resolveValue) @@ -213,10 +214,10 @@ const matchesCondition = (conversationValue, filter) => { return !contains(filterValue, conversationValue); case 'is_present': - return true; // We already handled null/undefined above + return !isNullish; case 'is_not_present': - return false; // We already handled null/undefined above + return isNullish; case 'is_greater_than': return compareDates(conversationValue, filterValue, (a, b) => a > b); @@ -225,6 +226,10 @@ const matchesCondition = (conversationValue, filter) => { return compareDates(conversationValue, filterValue, (a, b) => a < b); case 'days_before': { + if (isNullish) { + return false; + } + const today = new Date(); const daysInMilliseconds = filterValue * 24 * 60 * 60 * 1000; const targetDate = new Date(today.getTime() - daysInMilliseconds); diff --git a/app/javascript/dashboard/store/modules/conversations/helpers/specs/filterHelpers.spec.js b/app/javascript/dashboard/store/modules/conversations/helpers/specs/filterHelpers.spec.js index 096481c69..db1017407 100644 --- a/app/javascript/dashboard/store/modules/conversations/helpers/specs/filterHelpers.spec.js +++ b/app/javascript/dashboard/store/modules/conversations/helpers/specs/filterHelpers.spec.js @@ -192,6 +192,32 @@ describe('filterHelpers', () => { expect(matchesFilters(conversation, filters)).toBe(true); }); + it('should not match conversation with equal_to operator when assignee is null', () => { + const conversation = { meta: { assignee: null } }; + const filters = [ + { + attribute_key: 'assignee_id', + filter_operator: 'equal_to', + values: { id: 1, name: 'John Doe' }, + query_operator: 'and', + }, + ]; + expect(matchesFilters(conversation, filters)).toBe(false); + }); + + it('should match conversation with not_equal_to operator when assignee is null', () => { + const conversation = { meta: { assignee: null } }; + const filters = [ + { + attribute_key: 'assignee_id', + filter_operator: 'not_equal_to', + values: { id: 1, name: 'John Doe' }, + query_operator: 'and', + }, + ]; + expect(matchesFilters(conversation, filters)).toBe(true); + }); + it('should match conversation with is_not_present operator for assignee_id', () => { const conversation = { meta: { assignee: null } }; const filters = [ @@ -285,6 +311,58 @@ describe('filterHelpers', () => { expect(matchesFilters(conversation, filters)).toBe(false); }); + it('should not match contains operator when display_id is null', () => { + const conversation = { id: null }; + const filters = [ + { + attribute_key: 'display_id', + filter_operator: 'contains', + values: '234', + query_operator: 'and', + }, + ]; + expect(matchesFilters(conversation, filters)).toBe(false); + }); + + it('should not match contains operator when filter value is null', () => { + const conversation = { id: '12345' }; + const filters = [ + { + attribute_key: 'display_id', + filter_operator: 'contains', + values: null, + query_operator: 'and', + }, + ]; + expect(matchesFilters(conversation, filters)).toBe(false); + }); + + it('should match does_not_contain operator when display_id is null', () => { + const conversation = { id: null }; + const filters = [ + { + attribute_key: 'display_id', + filter_operator: 'does_not_contain', + values: '234', + query_operator: 'and', + }, + ]; + expect(matchesFilters(conversation, filters)).toBe(true); + }); + + it('should match does_not_contain operator when filter value is null', () => { + const conversation = { id: '12345' }; + const filters = [ + { + attribute_key: 'display_id', + filter_operator: 'does_not_contain', + values: null, + query_operator: 'and', + }, + ]; + expect(matchesFilters(conversation, filters)).toBe(true); + }); + it('should match conversation with does_not_contain operator when value is not present', () => { const conversation = { id: '12345' }; const filters = [ From d8da1f5bf30a6485f644eee3f7ff056717ad23b8 Mon Sep 17 00:00:00 2001 From: Muhsin Keloth Date: Thu, 9 Oct 2025 21:05:53 +0530 Subject: [PATCH 6/9] fix: Handle video file types in Slack file shares (#12630) Fixes https://linear.app/chatwoot/issue/CW-5752/fix-nomethoderror-when-processing-video-files-in-slack-integration #### Problem When users shared video files (like MP4) through Slack, the `file_type` method in `SlackMessageHelper` would return `nil` for unsupported file types. This caused a `NoMethodError (undefined method 'to_sym' for nil)` when the attachment was being processed, as the system expected a symbol value for the `file_type` attribute. #### Solution - Added video file type support in the `file_type` method case statement - Added `else` clause to default unknown file types to `:file` instead of returning `nil` - This ensures `file_type` always returns a symbol, preventing the `to_sym` error --- lib/integrations/slack/slack_message_helper.rb | 4 +++- .../slack/incoming_message_builder_spec.rb | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/integrations/slack/slack_message_helper.rb b/lib/integrations/slack/slack_message_helper.rb index 52ec4caad..0ee328fb3 100644 --- a/lib/integrations/slack/slack_message_helper.rb +++ b/lib/integrations/slack/slack_message_helper.rb @@ -70,7 +70,9 @@ module Integrations::Slack::SlackMessageHelper case attachment[:filetype] when 'png', 'jpeg', 'gif', 'bmp', 'tiff', 'jpg' :image - when 'pdf' + when 'mp4', 'avi', 'mov', 'wmv', 'flv', 'webm' + :video + else :file end end diff --git a/spec/lib/integrations/slack/incoming_message_builder_spec.rb b/spec/lib/integrations/slack/incoming_message_builder_spec.rb index 608324e8f..2ce206489 100644 --- a/spec/lib/integrations/slack/incoming_message_builder_spec.rb +++ b/spec/lib/integrations/slack/incoming_message_builder_spec.rb @@ -157,6 +157,19 @@ describe Integrations::Slack::IncomingMessageBuilder do expect(conversation.messages.count).to eql(messages_count) end + + it 'handles different file types correctly' do + expect(hook).not_to be_nil + video_attachment_params = message_with_attachments.deep_dup + video_attachment_params[:event][:files][0][:filetype] = 'mp4' + video_attachment_params[:event][:files][0][:mimetype] = 'video/mp4' + + builder = described_class.new(video_attachment_params) + allow(builder).to receive(:sender).and_return(nil) + + expect { builder.perform }.not_to raise_error + expect(conversation.messages.last.attachments).to be_any + end end context 'when link shared' do From cb65d615eaa7a20acfc6a29783a82a35c82faae1 Mon Sep 17 00:00:00 2001 From: Sivin Varghese <64252451+iamsivin@users.noreply.github.com> Date: Thu, 9 Oct 2025 23:27:30 +0530 Subject: [PATCH 7/9] chore: Add auto-refresh and self-hosted redirect logic to the billing page (#12615) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Pull Request Template ## Description This PR includes billing page improvements with the following updates: ### Self-hosted Users * Automatically redirected to the dashboard when accessing the billing page. ### Cloud Users – No Billing Plan (First Visit) * Shows a loading spinner with the `Your billing account is being configured. Please refresh the page and try again.` message. * Automatically refreshes the page after 5 seconds to check for billing setup. ### Cloud Users – No Billing Plan (After Refresh) * Prevents infinite refresh loops using `sessionStorage` tracking. * Displays the standard `Your billing account is being configured. Please refresh the page and try again.` message without further refresh attempts. * Cleans up session flags for future visits. ### Cloud Users – With Billing Plan * Displays the existing billing page normally with no refresh or redirection logic. Fixes https://linear.app/chatwoot/issue/CW-5559/your-billing-page-is-being-set-up-message-on-billing-page-is-confusing ## Type of change - [x] New feature (non-breaking change which adds functionality) ## How Has This Been Tested? ### Loom video https://www.loom.com/share/d0ea13d6b90b4ab1acbc581b524f6382?sid=d3dd19f3-85aa-4127-9233-7eecb1be0884 ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules --------- Co-authored-by: Muhsin Keloth --- .../dashboard/settings/billing/Index.vue | 61 ++++++++++++++++--- 1 file changed, 54 insertions(+), 7 deletions(-) diff --git a/app/javascript/dashboard/routes/dashboard/settings/billing/Index.vue b/app/javascript/dashboard/routes/dashboard/settings/billing/Index.vue index fc3905014..8ba3d69a9 100644 --- a/app/javascript/dashboard/routes/dashboard/settings/billing/Index.vue +++ b/app/javascript/dashboard/routes/dashboard/settings/billing/Index.vue @@ -1,9 +1,11 @@