mirror of
https://github.com/chatwoot/chatwoot.git
synced 2026-06-04 21:02:35 +08:00
035d2858f5
\`GET /platform/api/v1/agent_bots\` returns 500 when any \`AgentBot\` that was previously registered with a Platform App has since been deleted. The bug was introduced by a missing \`dependent: :destroy\` on the \`AgentBot\` model — deleting a bot left orphaned rows in \`platform_app_permissibles\`, which the index action later iterated over and crashed rendering with a \`NoMethodError\` on \`nil\`. Closes #13407 ## Root cause The index action loads all \`platform_app_permissibles\` for the platform app and passes each \`resource.permissible\` (the associated \`AgentBot\`) to a Jbuilder partial. When the \`AgentBot\` no longer exists, \`resource.permissible\` returns \`nil\` and the partial crashes calling \`.id\`, \`.name\`, etc. on it. Every other \`AgentBot\` association (\`agent_bot_inboxes\`, \`messages\`, \`assigned_conversations\`) had a \`dependent:\` option — \`platform_app_permissibles\` was the only one missing it. There was also an N+1 query: the index fired a separate SQL query per permissible to load each bot. ## What changed **1. Model — prevent orphans at deletion time** \`\`\`ruby has_many :platform_app_permissibles, as: :permissible, dependent: :destroy \`\`\` **2. Controller — eager-load to eliminate N+1** \`\`\`ruby @resources = @platform_app.platform_app_permissibles .where(permissible_type: 'AgentBot') .includes(:permissible) \`\`\` **3. Jbuilder — defensive nil guard for pre-existing orphans** \`\`\`ruby bot = resource.permissible next if bot.nil? json.partial! '...', resource: bot \`\`\` ## Trade-offs considered | Option | Decision | |---|---| | Rescue \`NoMethodError\` in jbuilder | Hides the failure rather than fixing it. Rejected. | | Only add the nil guard, skip the model fix | Leaves the data integrity gap open — future deletions continue creating orphans. Rejected. | | Both layers (chosen) | Model fix prevents new orphans; nil guard is defence-in-depth for any orphans that survived before deployment. | | \`dependent: :nullify\` | Doesn't apply — a nullified permissible would still cause the same nil dereference. Rejected. | ## How to reproduce 1. Create an AgentBot via the Platform API 2. Delete the AgentBot via any path (admin UI, API, or direct model call) 3. Call \`GET /platform/api/v1/agent_bots\` with a Platform App token 4. Observe 500 After this fix, the endpoint returns 200 with an empty array. Co-authored-by: Ramalau Debeila <rdebeila@datacentrix.co.za>
70 lines
2.1 KiB
Ruby
70 lines
2.1 KiB
Ruby
require 'rails_helper'
|
|
require Rails.root.join 'spec/models/concerns/access_tokenable_shared.rb'
|
|
require Rails.root.join 'spec/models/concerns/avatarable_shared.rb'
|
|
|
|
RSpec.describe AgentBot do
|
|
describe 'associations' do
|
|
it { is_expected.to have_many(:agent_bot_inboxes) }
|
|
it { is_expected.to have_many(:inboxes) }
|
|
it { is_expected.to have_many(:platform_app_permissibles) }
|
|
end
|
|
|
|
describe 'concerns' do
|
|
it_behaves_like 'access_tokenable'
|
|
it_behaves_like 'avatarable'
|
|
end
|
|
|
|
context 'when it validates outgoing_url length' do
|
|
let(:agent_bot) { create(:agent_bot) }
|
|
|
|
it 'valid when within limit' do
|
|
agent_bot.outgoing_url = 'a' * Limits::URL_LENGTH_LIMIT
|
|
expect(agent_bot.valid?).to be true
|
|
end
|
|
|
|
it 'invalid when crossed the limit' do
|
|
agent_bot.outgoing_url = 'a' * (Limits::URL_LENGTH_LIMIT + 1)
|
|
agent_bot.valid?
|
|
expect(agent_bot.errors[:outgoing_url]).to include("is too long (maximum is #{Limits::URL_LENGTH_LIMIT} characters)")
|
|
end
|
|
end
|
|
|
|
context 'when agent bot is deleted' do
|
|
let(:agent_bot) { create(:agent_bot) }
|
|
let(:message) { create(:message, sender: agent_bot) }
|
|
|
|
it 'nullifies the message sender key' do
|
|
expect(message.sender).to eq agent_bot
|
|
agent_bot.destroy!
|
|
|
|
expect(message.reload.sender).to be_nil
|
|
end
|
|
|
|
it 'destroys associated platform_app_permissibles' do
|
|
platform_app = create(:platform_app)
|
|
create(:platform_app_permissible, platform_app: platform_app, permissible: agent_bot)
|
|
|
|
expect { agent_bot.destroy! }.to change(PlatformAppPermissible, :count).by(-1)
|
|
end
|
|
end
|
|
|
|
describe '#system_bot?' do
|
|
context 'when account_id is nil' do
|
|
let(:agent_bot) { create(:agent_bot, account_id: nil) }
|
|
|
|
it 'returns true' do
|
|
expect(agent_bot.system_bot?).to be true
|
|
end
|
|
end
|
|
|
|
context 'when account_id is present' do
|
|
let(:account) { create(:account) }
|
|
let(:agent_bot) { create(:agent_bot, account: account) }
|
|
|
|
it 'returns false' do
|
|
expect(agent_bot.system_bot?).to be false
|
|
end
|
|
end
|
|
end
|
|
end
|