From 3c47ea3d4355b6381ac6b57e129054436e9c4407 Mon Sep 17 00:00:00 2001 From: Gabriel Jablonski Date: Thu, 5 Feb 2026 18:42:46 -0300 Subject: [PATCH] fix: prevent deletion of scheduled messages that have been sent or failed (#212) * fix: prevent deletion of scheduled messages that have been sent or failed * fix: update error message for deletion of processed scheduled messages --- .../scheduled_messages_controller.rb | 6 ++- app/models/scheduled_message.rb | 8 ---- config/locales/en.yml | 2 + .../scheduled_messages_controller_spec.rb | 13 ++++++ spec/models/scheduled_message_spec.rb | 44 ++++++++++++++++--- 5 files changed, 57 insertions(+), 16 deletions(-) diff --git a/app/controllers/api/v1/accounts/conversations/scheduled_messages_controller.rb b/app/controllers/api/v1/accounts/conversations/scheduled_messages_controller.rb index 70c4dc7de..eda5e9937 100644 --- a/app/controllers/api/v1/accounts/conversations/scheduled_messages_controller.rb +++ b/app/controllers/api/v1/accounts/conversations/scheduled_messages_controller.rb @@ -27,11 +27,13 @@ class Api::V1::Accounts::Conversations::ScheduledMessagesController < Api::V1::A end def destroy + if @scheduled_message.sent? || @scheduled_message.failed? + return render json: { error: I18n.t('errors.scheduled_messages.cannot_delete_processed') }, status: :unprocessable_entity + end + scheduled_message = @scheduled_message scheduled_message.destroy! dispatch_event(SCHEDULED_MESSAGE_DELETED, scheduled_message: scheduled_message) - rescue ActiveRecord::RecordNotDestroyed => e - render json: { errors: e.record.errors.full_messages }, status: :unprocessable_entity end private diff --git a/app/models/scheduled_message.rb b/app/models/scheduled_message.rb index 8c857b1a4..b6de61c7b 100644 --- a/app/models/scheduled_message.rb +++ b/app/models/scheduled_message.rb @@ -51,7 +51,6 @@ class ScheduledMessage < ApplicationRecord enum status: { draft: 0, pending: 1, sent: 2, failed: 3 } before_validation :process_message_variables, if: :content_changed? - before_destroy :prevent_destroy_if_processed validates :scheduled_at, presence: true, unless: -> { status == 'draft' } validates :content, presence: true, unless: :content_optional? @@ -121,13 +120,6 @@ class ScheduledMessage < ApplicationRecord changed_attributes.keys == ['status'] end - def prevent_destroy_if_processed - return unless sent? || failed? - - errors.add(:base, 'Cannot delete a scheduled message that has already been sent or failed') - throw(:abort) - end - def scheduled_at_must_be_in_future return if scheduled_at.blank? return if scheduled_at > Time.current diff --git a/config/locales/en.yml b/config/locales/en.yml index f80422efe..f87d1670a 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -68,6 +68,8 @@ en: scheduled_message: delay_out_of_range: Delay must be between 1 minute and 999 days content_attachment_or_template_required: Scheduled message must have content, attachment, or template + scheduled_messages: + cannot_delete_processed: Cannot delete a scheduled message that has already been sent or failed saml: feature_not_enabled: SAML feature not enabled for this account sso_not_enabled: SAML SSO is not enabled for this installation diff --git a/spec/controllers/api/v1/accounts/conversations/scheduled_messages_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations/scheduled_messages_controller_spec.rb index dbb0c025c..4d7df5170 100644 --- a/spec/controllers/api/v1/accounts/conversations/scheduled_messages_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations/scheduled_messages_controller_spec.rb @@ -114,6 +114,19 @@ RSpec.describe 'Scheduled Messages API', type: :request do expect(ScheduledMessage.exists?(scheduled_message.id)).to be(false) end + it 'deletes pending scheduled messages with attachments' do + scheduled_message.attachment.attach( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + delete scheduled_message_url(scheduled_message), headers: agent.create_new_auth_token, as: :json + + expect(response).to have_http_status(:success) + expect(ScheduledMessage.exists?(scheduled_message.id)).to be(false) + end + it 'rejects delete for sent messages' do scheduled_message.update!(status: :sent) diff --git a/spec/models/scheduled_message_spec.rb b/spec/models/scheduled_message_spec.rb index f8c04ebbb..f282c385a 100644 --- a/spec/models/scheduled_message_spec.rb +++ b/spec/models/scheduled_message_spec.rb @@ -182,20 +182,52 @@ RSpec.describe ScheduledMessage, type: :model do expect { scheduled_message.destroy }.to change(described_class, :count).by(-1) end - it 'does not allow deleting sent messages' do + it 'allows deleting pending messages with attachments' do + scheduled_message = create_scheduled_message + scheduled_message.attachment.attach( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + expect { scheduled_message.destroy! }.to change(described_class, :count).by(-1) + end + + it 'allows destroying conversation with pending scheduled messages with attachments' do + scheduled_message = create_scheduled_message + scheduled_message.attachment.attach( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + + expect { conversation.destroy! }.to change(described_class, :count).by(-1) + end + + it 'allows destroying conversation with sent scheduled messages with attachments' do + scheduled_message = create_scheduled_message + scheduled_message.attachment.attach( + io: Rails.root.join('spec/assets/avatar.png').open, + filename: 'avatar.png', + content_type: 'image/png' + ) + scheduled_message.update!(status: :sent) + + expect { conversation.destroy! }.to change(described_class, :count).by(-1) + end + + it 'allows deleting sent messages at model level (controller enforces restriction)' do scheduled_message = create_scheduled_message scheduled_message.update!(status: :sent) - expect { scheduled_message.destroy }.not_to change(described_class, :count) - expect(scheduled_message.errors[:base]).to include('Cannot delete a scheduled message that has already been sent or failed') + expect { scheduled_message.destroy! }.to change(described_class, :count).by(-1) end - it 'does not allow deleting failed messages' do + it 'allows deleting failed messages at model level (controller enforces restriction)' do scheduled_message = create_scheduled_message scheduled_message.update!(status: :failed) - expect { scheduled_message.destroy }.not_to change(described_class, :count) - expect(scheduled_message.errors[:base]).to include('Cannot delete a scheduled message that has already been sent or failed') + expect { scheduled_message.destroy! }.to change(described_class, :count).by(-1) end end end