Merge pull request #3387 from AlchemyCMS/delete-pictures-in-background

Delete multiple pictures in background

Author: tvdeyen

app/controllers/alchemy/admin/pictures_controller.rb

@@ -102,32 +102,12 @@ def update_multiple
       end
 
       def delete_multiple
-        if request.delete? && params[:picture_ids].present?
-          pictures = Picture.find(params[:picture_ids])
-          names = []
-          not_deletable = []
-          pictures.each do |picture|
-            if picture.deletable?
-              names << picture.name
-              picture.destroy
-            else
-              not_deletable << picture.name
-            end
-          end
-          if not_deletable.any?
-            flash[:warn] = Alchemy.t(
-              "These pictures could not be deleted, because they were in use",
-              names: not_deletable.to_sentence
-            )
-          else
-            flash[:notice] = Alchemy.t("Pictures deleted successfully", names: names.to_sentence)
-          end
+        if params[:picture_ids].present?
+          params[:picture_ids].each { DeletePictureJob.perform_later(_1) }
+          flash[:notice] = Alchemy.t(:pictures_will_be_deleted_now)
         else
           flash[:warn] = Alchemy.t("Could not delete Pictures")
         end
-      rescue => e
-        flash[:error] = e.message
-      ensure
         redirect_to_index
       end
 

app/jobs/alchemy/delete_picture_job.rb

@@ -0,0 +1,12 @@
+module Alchemy
+  class DeletePictureJob < BaseJob
+    queue_as :default
+
+    def perform(picture_id)
+      picture = Alchemy::Picture.find_by(id: picture_id)
+      return if picture.nil? || !picture.deletable?
+
+      picture.destroy
+    end
+  end
+end

config/locales/alchemy.en.yml

@@ -340,7 +340,6 @@ en:
     "Size": "Size"
     "Successfully deleted element": "Successfully deleted %{element}"
     "Tags": "Tags"
-    "These pictures could not be deleted, because they were in use": "These pictures could not be deleted, because they were in use: %{names}"
     "This page is locked": "This page is currently locked by %{name}"
     "Title": "Title"
     "Trash": "Trash"
@@ -568,6 +567,7 @@ en:
     password: "Password"
     paste: "paste"
     pictures_in_page: "%{page} in %{pictures}"
+    pictures_will_be_deleted_now: Pictures will be deleted now
     place_link: "Add link"
     player_version: "Flash Player version"
     "please enter subject and mail address": "Please enter recipient and subject."

spec/controllers/alchemy/admin/pictures_controller_spec.rb

@@ -309,7 +309,9 @@ module Alchemy
     end
 
     describe "#delete_multiple" do
-      subject { delete :delete_multiple, params: {picture_ids: picture_ids} }
+      subject do
+        delete :delete_multiple, params: {picture_ids: picture_ids}
+      end
 
       it_behaves_like :redirecting_to_picture_library do
         let(:subject) do
@@ -319,12 +321,8 @@ module Alchemy
         end
       end
 
-      let(:deletable_picture) do
-        mock_model("Picture", name: "pic of the pig", deletable?: true)
-      end
-
-      let(:not_deletable_picture) do
-        mock_model("Picture", name: "pic of the chick", deletable?: false)
+      let(:picture) do
+        build_stubbed(:alchemy_picture)
       end
 
       context "no picture_ids given" do
@@ -337,49 +335,12 @@ module Alchemy
       end
 
       context "picture_ids given" do
-        context "all are deletable" do
-          let(:picture_ids) { deletable_picture.id.to_s }
-
-          before do
-            allow(Picture).to receive(:find).and_return([deletable_picture])
-          end
-
-          it "should delete the pictures give a notice about deleting them" do
-            subject
-            expect(flash[:notice]).to match("successfully")
-          end
-        end
-
-        context "deletable and not deletable" do
-          let(:picture_ids) { "#{deletable_picture.id},#{not_deletable_picture.id}" }
-
-          before do
-            allow(Picture).to receive(:find).and_return([deletable_picture, not_deletable_picture])
-          end
+        let(:picture_ids) { [picture.id] }
 
-          it "should give a warning for the non deletable pictures and delete the others" do
-            expect(deletable_picture).to receive(:destroy)
-            subject
-            expect(flash[:warn]).to match("could not be deleted")
-          end
-        end
-
-        context "with error happening" do
-          let(:picture_ids) { deletable_picture.id.to_s }
-
-          before do
-            expect(Picture).to receive(:find).and_raise("yada")
-          end
-
-          it "sets error message" do
-            subject
-            expect(flash[:error]).not_to be_blank
-          end
-
-          it "redirects to index" do
-            subject
-            expect(response).to redirect_to admin_pictures_path
-          end
+        it "enqueues the picture delete job" do
+          subject
+          expect(DeletePictureJob).to have_been_enqueued.with(picture.id.to_s)
+          expect(flash[:notice]).to match("Pictures will be deleted now")
         end
       end
     end

spec/features/admin/picture_library_integration_spec.rb

@@ -102,13 +102,9 @@
       end
 
       within "#flash_notices" do
-        expect(page).to have_content("Pictures deleted successfully")
+        expect(page).to have_content("Pictures will be deleted now")
       end
 
-      expect(page).to_not have_selector("#picture_#{picture_2.id}")
-      expect(page).to_not have_selector("#picture_#{picture_1.id}")
-      expect(page).to have_selector("#picture_#{picture_3.id}")
-
       # Keeps existing params
       within "#filter_bar" do
         expect(page).to have_checked_field("Without tag")

spec/jobs/alchemy/delete_picture_job_spec.rb

@@ -0,0 +1,41 @@
+require "rails_helper"
+
+RSpec.describe Alchemy::DeletePictureJob, type: :job do
+  describe "#perform" do
+    subject(:perform) { described_class.new.perform(5) }
+
+    context "when picture exists" do
+      let(:picture) { build(:alchemy_picture) }
+
+      before do
+        allow(Alchemy::Picture).to receive(:find_by).with(id: 5) { picture }
+      end
+
+      context "when picture is deletable" do
+        it "deletes the picture" do
+          expect(picture).to receive(:deletable?) { true }
+          expect(picture).to receive(:destroy)
+          perform
+        end
+      end
+
+      context "when picture is not deletable" do
+        it "does not delete the picture" do
+          expect(picture).to receive(:deletable?) { false }
+          expect(picture).to_not receive(:destroy)
+          perform
+        end
+      end
+    end
+
+    context "when picture does not exist" do
+      before do
+        allow(Alchemy::Picture).to receive(:find_by).with(id: 5) { nil }
+      end
+
+      it "does nothing" do
+        perform
+      end
+    end
+  end
+end