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

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

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

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

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)
 

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