Merge pull request #1774 from alphagov/ldeb-fix-destroy-routes

Fix delete all routes journey

app/services/condition_repository.rb

@@ -29,7 +29,15 @@ def save!(record)
     def destroy(record)
       condition = Api::V1::ConditionResource.new(record.attributes, true)
       condition.prefix_options = record.prefix_options
-      condition.destroy # rubocop:disable Rails/SaveBang
+
+      begin
+        condition.destroy # rubocop:disable Rails/SaveBang
+      rescue ActiveResource::ResourceNotFound
+        # ActiveRecord::Persistence#destroy doesn't raise an error
+        # if record has already been destroyed, let's emulate that
+      end
+
+      record
     end
   end
 end

app/services/form_repository.rb

@@ -40,7 +40,15 @@ def archive!(record)
 
     def destroy(record)
       form = Api::V1::FormResource.new(record.attributes, true)
-      form.destroy # rubocop:disable Rails/SaveBang
+
+      begin
+        form.destroy # rubocop:disable Rails/SaveBang
+      rescue ActiveResource::ResourceNotFound
+        # ActiveRecord::Persistence#destroy doesn't raise an error
+        # if record has already been destroyed, let's emulate that
+      end
+
+      record
     end
 
     def pages(record)

app/services/page_repository.rb

@@ -34,7 +34,15 @@ def save!(record)
     def destroy(record)
       page = Api::V1::PageResource.new(record.attributes, true)
       page.prefix_options = record.prefix_options
-      page.destroy # rubocop:disable Rails/SaveBang
+
+      begin
+        page.destroy # rubocop:disable Rails/SaveBang
+      rescue ActiveResource::ResourceNotFound
+        # ActiveRecord::Persistence#destroy doesn't raise an error
+        # if record has already been destroyed, let's emulate that
+      end
+
+      record
     end
 
     def move_page(record, direction)

spec/requests/pages/routes_controller_spec.rb

@@ -60,9 +60,9 @@
   end
 
   describe "#destroy" do
-    let(:condition) { build :condition, routing_page_id: selected_page.id, check_page_id: selected_page.id, goto_page_id: pages.last.id, answer_value: "Option 1" }
+    let(:condition) { build :condition, id: 1, routing_page_id: selected_page.id, check_page_id: selected_page.id, goto_page_id: pages.last.id, answer_value: "Option 1" }
     let(:secondary_skip_page) { form.pages[2] }
-    let(:secondary_skip) { build :condition, routing_page_id: secondary_skip_page.id, check_page_id: selected_page.id, goto_page_id: pages[3].id }
+    let(:secondary_skip) { build :condition, id: 2, routing_page_id: secondary_skip_page.id, check_page_id: selected_page.id, goto_page_id: pages[3].id }
 
     before do
       allow(PageRepository).to receive(:find).with(page_id: "101", form_id: 1).and_return(selected_page)
@@ -84,6 +84,22 @@
         expect(ConditionRepository).to receive(:destroy).with(have_attributes(id: secondary_skip.id))
         delete destroy_routes_path(form_id: form.id, page_id: selected_page.id, pages_routes_delete_confirmation_input: { confirm: "yes" })
       end
+
+      context "but one of the routes is already deleted" do
+        before do
+          # forms-api may choose to delete the secondary skip when the condition is deleted
+          allow(ConditionRepository).to receive(:destroy).and_call_original
+          ActiveResource::HttpMock.respond_to do |mock|
+            mock.delete "/api/v1/forms/#{form.id}/pages/#{selected_page.id}/conditions/#{condition.id}", delete_headers, nil, 204
+            mock.delete "/api/v1/forms/#{form.id}/pages/#{secondary_skip_page.id}/conditions/#{secondary_skip.id}", delete_headers, nil, 404
+          end
+        end
+
+        it "does not render an error page" do
+          delete destroy_routes_path(form_id: form.id, page_id: selected_page.id, pages_routes_delete_confirmation_input: { confirm: "yes" })
+          expect(response).not_to be_client_error
+        end
+      end
     end
 
     context "when given invalid params" do

spec/services/condition_repository_spec.rb

@@ -72,5 +72,36 @@
       described_class.destroy(condition)
       expect(Api::V1::ConditionResource.new(id: 4, form_id: 1, page_id: 2)).to have_been_deleted
     end
+
+    it "returns the deleted condition" do
+      condition = described_class.find(condition_id: 4, form_id: 1, page_id: 2)
+      expect(described_class.destroy(condition)).to eq condition
+    end
+
+    context "when the condition has already been deleted" do
+      it "does not raise an error" do
+        condition = described_class.find(condition_id: 4, form_id: 1, page_id: 2)
+        described_class.destroy(condition)
+
+        ActiveResource::HttpMock.respond_to do |mock|
+          mock.delete "/api/v1/forms/1/pages/2/conditions/4", delete_headers, nil, 404
+        end
+
+        expect {
+          described_class.destroy(condition)
+        }.not_to raise_error
+      end
+
+      it "returns the deleted condition" do
+        condition = described_class.find(condition_id: 4, form_id: 1, page_id: 2)
+        described_class.destroy(condition)
+
+        ActiveResource::HttpMock.respond_to do |mock|
+          mock.delete "/api/v1/forms/1/pages/2/conditions/4", delete_headers, nil, 404
+        end
+
+        expect(described_class.destroy(condition)).to eq condition
+      end
+    end
   end
 end

spec/services/form_repository_spec.rb

@@ -144,6 +144,37 @@
       described_class.destroy(form)
       expect(Api::V1::FormResource.new(id: 2)).to have_been_deleted
     end
+
+    it "returns the deleted form" do
+      form = described_class.find(form_id: 2)
+      expect(described_class.destroy(form)).to eq form
+    end
+
+    context "when the form has already been deleted" do
+      it "does not raise an error" do
+        form = described_class.find(form_id: 2)
+        described_class.destroy(form)
+
+        ActiveResource::HttpMock.respond_to do |mock|
+          mock.delete "/api/v1/forms/2", delete_headers, nil, 404
+        end
+
+        expect {
+          described_class.destroy(form)
+        }.not_to raise_error
+      end
+
+      it "returns the deleted form" do
+        form = described_class.find(form_id: 2)
+        described_class.destroy(form)
+
+        ActiveResource::HttpMock.respond_to do |mock|
+          mock.delete "/api/v1/forms/2", delete_headers, nil, 404
+        end
+
+        expect(described_class.destroy(form)).to eq form
+      end
+    end
   end
 
   describe "#pages" do

spec/services/page_repository_spec.rb

@@ -74,6 +74,37 @@
       described_class.destroy(page)
       expect(Api::V1::PageResource.new(id: 2, form_id: 1)).to have_been_deleted
     end
+
+    it "returns the deleted page" do
+      page = described_class.find(page_id: 2, form_id: 1)
+      expect(described_class.destroy(page)).to eq page
+    end
+
+    context "when the page has already been deleted" do
+      it "does not raise an error" do
+        page = described_class.find(page_id: 2, form_id: 1)
+        described_class.destroy(page)
+
+        ActiveResource::HttpMock.respond_to do |mock|
+          mock.delete "/api/v1/forms/1/pages/2", delete_headers, nil, 404
+        end
+
+        expect {
+          described_class.destroy(page)
+        }.not_to raise_error
+      end
+
+      it "returns the deleted page" do
+        page = described_class.find(page_id: 2, form_id: 1)
+        described_class.destroy(page)
+
+        ActiveResource::HttpMock.respond_to do |mock|
+          mock.delete "/api/v1/forms/1/pages/2", delete_headers, nil, 404
+        end
+
+        expect(described_class.destroy(page)).to eq page
+      end
+    end
   end
 
   describe "#move_page" do