From b52f2658c2bdf1093707c9cc0ec2017bfb2380b0 Mon Sep 17 00:00:00 2001
From: Kevin Dew <kevindew@me.com>
Date: Fri, 7 Feb 2020 20:11:59 +0000
Subject: [PATCH 1/4] Add tests for Editions::DestroyInteractor

The impetus to add tests for this was learning that there is actually a
problem where the edition model used in this interactor is not up to
date and thus a timeline entry is created with the wrong status.

This adds tests for the whole file however the one on line 20 is a
failing test that captures this problem.
---
 .../editions/destroy_interactor_spec.rb       | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 spec/interactors/editions/destroy_interactor_spec.rb

diff --git a/spec/interactors/editions/destroy_interactor_spec.rb b/spec/interactors/editions/destroy_interactor_spec.rb
new file mode 100644
index 0000000000..7ffb907c65
--- /dev/null
+++ b/spec/interactors/editions/destroy_interactor_spec.rb
@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+RSpec.describe Editions::DestroyInteractor do
+  describe ".call" do
+    let(:edition) { create(:edition) }
+    let(:user) { create :user }
+
+    let(:params) do
+      ActionController::Parameters.new(document: edition.document.to_param)
+    end
+
+    before do
+      stub_publishing_api_unreserve_path(edition.base_path)
+      stub_publishing_api_discard_draft(edition.content_id)
+    end
+
+    it "discards an edition" do
+      result = Editions::DestroyInteractor.call(params: params, user: user)
+      expect(result).to be_success
+      expect(result.edition).to be_discarded
+    end
+
+    it "delegates to the DeleteDraftEditionService" do
+      expect(DeleteDraftEditionService)
+        .to receive(:call)
+        .with(edition.document, user)
+      Editions::DestroyInteractor.call(params: params, user: user)
+    end
+
+    it "creates a timeline entry" do
+      expect { Editions::DestroyInteractor.call(params: params, user: user) }
+        .to change { TimelineEntry.where(entry_type: :draft_discarded).count }
+        .by(1)
+    end
+
+    context "when the Publishing API is down" do
+      before { stub_publishing_api_isnt_available }
+
+      it "fails with an api_error flag" do
+        result = Editions::DestroyInteractor.call(params: params, user: user)
+        expect(result).to be_failure
+        expect(result.api_error).to be(true)
+      end
+    end
+
+    context "when the edition isn't editable" do
+      let(:edition) { create(:edition, :published) }
+
+      it "raises a state error" do
+        expect { Editions::DestroyInteractor.call(params: params, user: user) }
+          .to raise_error(EditionAssertions::StateError)
+      end
+    end
+  end
+end

From 2ff78aa0832a32c458a1b1fa0907e1af05fc5c51 Mon Sep 17 00:00:00 2001
From: Kevin Dew <kevindew@me.com>
Date: Fri, 7 Feb 2020 21:58:50 +0000
Subject: [PATCH 2/4] Use edition as subject of DeleteDraftEditionService

This changes this service to take an edition as it's argument rather
than a document. This change has been done in part to resolve a problem
where operating on a document meant a stale edition was used in the
interactor. When trying to address that problem I noticed that it seemed
quite strange that this class operated on a document when its name
implies an edition.

I also added reload methods for associations of a document so that were
a similar problem to occur again the document is up to date.
---
 app/services/delete_draft_edition_service.rb  | 23 +++++-----
 .../delete_draft_edition_service_spec.rb      | 46 +++++++++----------
 2 files changed, 35 insertions(+), 34 deletions(-)

diff --git a/app/services/delete_draft_edition_service.rb b/app/services/delete_draft_edition_service.rb
index b0eee819fe..a59ebb932b 100644
--- a/app/services/delete_draft_edition_service.rb
+++ b/app/services/delete_draft_edition_service.rb
@@ -1,32 +1,32 @@
 # frozen_string_literal: true
 
 class DeleteDraftEditionService < ApplicationService
-  def initialize(document, user)
-    @document = document
+  def initialize(edition, user)
+    @edition = edition
     @user = user
   end
 
   def call
-    edition = document.current_edition
-
-    raise "Trying to delete a document without a current edition" unless edition
-    raise "Trying to delete a live document" if edition.live?
+    raise "Only current editions can be deleted" unless edition.current?
+    raise "Trying to delete a live edition" if edition.live?
 
     begin
       delete_assets(edition.assets)
       discard_draft(edition)
     rescue GdsApi::BaseError
-      document.current_edition.update!(revision_synced: false)
+      edition.update!(revision_synced: false)
       raise
     end
 
     reset_live_edition if document.live_edition
     discard_path_reservations(edition) if edition.first?
+    document.reload_current_edition
   end
 
 private
 
-  attr_reader :document, :user
+  attr_reader :edition, :user
+  delegate :document, to: :edition
 
   def discard_path_reservations(edition)
     paths = edition.revisions.map(&:base_path).uniq.compact
@@ -41,18 +41,19 @@ def discard_path_reservations(edition)
 
   def reset_live_edition
     document.live_edition.update!(current: true)
+    document.reload_live_edition
   end
 
   def discard_draft(edition)
     begin
-      GdsApi.publishing_api.discard_draft(document.content_id)
+      GdsApi.publishing_api.discard_draft(edition.content_id)
     rescue GdsApi::HTTPNotFound
-      Rails.logger.warn("No draft to discard for content id #{document.content_id}")
+      Rails.logger.warn("No draft to discard for content id #{edition.content_id}")
     rescue GdsApi::HTTPUnprocessableEntity => e
       no_draft_message = "There is not a draft edition of this document to discard"
 
       if e.error_details.respond_to?(:dig) && e.error_details.dig("error", "message") == no_draft_message
-        Rails.logger.warn("No draft to discard for content id #{document.content_id}")
+        Rails.logger.warn("No draft to discard for content id #{edition.content_id}")
       else
         raise
       end
diff --git a/spec/services/delete_draft_edition_service_spec.rb b/spec/services/delete_draft_edition_service_spec.rb
index 2455604fad..e6ee7af47b 100644
--- a/spec/services/delete_draft_edition_service_spec.rb
+++ b/spec/services/delete_draft_edition_service_spec.rb
@@ -4,25 +4,25 @@
   let(:user) { create :user }
 
   describe ".call" do
-    it "raises an exception if there is not a current edition" do
-      document = create :document
+    it "raises an exception if the edition is not current" do
+      edition = create :edition, current: false
 
-      expect { DeleteDraftEditionService.call(document, user) }
-        .to raise_error "Trying to delete a document without a current edition"
+      expect { DeleteDraftEditionService.call(edition, user) }
+        .to raise_error "Only current editions can be deleted"
     end
 
     it "raises an exception if the current edition is live" do
       document = create :document, :with_live_edition
 
-      expect { DeleteDraftEditionService.call(document, user) }
-        .to raise_error "Trying to delete a live document"
+      expect { DeleteDraftEditionService.call(document.current_edition, user) }
+        .to raise_error "Trying to delete a live edition"
     end
 
     it "attempts to delete the document preview" do
       document = create :document, :with_current_edition
       stub_publishing_api_unreserve_path(document.current_edition.base_path)
       request = stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(request).to have_been_requested
     end
 
@@ -37,7 +37,7 @@
       stub_publishing_api_unreserve_path(edition.base_path)
       delete_request = stub_asset_manager_deletes_any_asset
 
-      DeleteDraftEditionService.call(edition.document, user)
+      DeleteDraftEditionService.call(edition, user)
 
       expect(delete_request).to have_been_requested.at_least_once
       expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
@@ -61,7 +61,7 @@
         PreviewDraftEditionService::Payload::PUBLISHING_APP,
       )
 
-      DeleteDraftEditionService.call(edition.document, user)
+      DeleteDraftEditionService.call(edition, user)
 
       expect(unreserve_request1).to have_been_requested
       expect(unreserve_request2).to have_been_requested
@@ -70,7 +70,7 @@
     it "does not delete path reservations for published documents" do
       document = create :document, :with_current_and_live_editions
       stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(document.reload.current_edition).to eq document.live_edition
     end
 
@@ -78,7 +78,7 @@
       document = create :document, :with_current_edition
       stub_publishing_api_unreserve_path(document.current_edition.base_path)
       stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(document.reload.current_edition).to be_nil
     end
 
@@ -87,7 +87,7 @@
       live_edition = document.live_edition
       stub_publishing_api_unreserve_path(document.current_edition.base_path)
       stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(document.reload.current_edition).to eq(live_edition)
     end
 
@@ -96,7 +96,7 @@
       edition = document.current_edition
       stub_publishing_api_unreserve_path(document.current_edition.base_path)
       stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(edition.status).to be_discarded
     end
 
@@ -104,7 +104,7 @@
       document = create :document, :with_current_edition
       stub_publishing_api_unreserve_path(document.current_edition.base_path)
       stub_any_publishing_api_call_to_return_not_found
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(document.reload.current_edition).to be_nil
     end
 
@@ -120,7 +120,7 @@
       stub_any_publishing_api_discard_draft
         .to_return(status: 422, body: discard_draft_error.to_json)
 
-      DeleteDraftEditionService.call(document, user)
+      DeleteDraftEditionService.call(document.current_edition, user)
       expect(document.reload.current_edition).to be_nil
     end
 
@@ -136,7 +136,7 @@
       stub_any_publishing_api_discard_draft
         .to_return(status: 422, body: discard_draft_error.to_json)
 
-      expect { DeleteDraftEditionService.call(document, user) }
+      expect { DeleteDraftEditionService.call(document.current_edition, user) }
         .to raise_error(GdsApi::HTTPUnprocessableEntity)
     end
 
@@ -149,7 +149,7 @@
 
       stub_publishing_api_unreserve_path(edition.base_path)
       stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition.document, user)
+      DeleteDraftEditionService.call(edition, user)
 
       expect(edition.reload.status).to be_discarded
       expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
@@ -161,7 +161,7 @@
 
       stub_publishing_api_unreserve_path_not_found(edition.base_path)
       stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition.document, user)
+      DeleteDraftEditionService.call(edition, user)
 
       expect(edition.reload.status).to be_discarded
     end
@@ -170,7 +170,7 @@
       edition = create :edition, base_path: nil
 
       stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition.document, user)
+      DeleteDraftEditionService.call(edition, user)
 
       expect(edition.reload.status).to be_discarded
     end
@@ -188,7 +188,7 @@
 
       stub_publishing_api_unreserve_path(edition.base_path)
       stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition.document, user)
+      DeleteDraftEditionService.call(edition, user)
 
       expect(edition.reload.status).to be_discarded
       expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
@@ -201,7 +201,7 @@
       stub_publishing_api_discard_draft(edition.content_id)
       stub_publishing_api_unreserve_path_invalid(edition.base_path)
 
-      expect { DeleteDraftEditionService.call(edition.document, user) }
+      expect { DeleteDraftEditionService.call(edition, user) }
         .to raise_error(GdsApi::BaseError)
 
       expect(edition.reload.revision_synced?).to be true
@@ -211,7 +211,7 @@
       edition = create :edition
       stub_publishing_api_isnt_available
 
-      expect { DeleteDraftEditionService.call(edition.document, user) }
+      expect { DeleteDraftEditionService.call(edition, user) }
         .to raise_error(GdsApi::BaseError)
 
       expect(edition.reload.revision_synced?).to be false
@@ -226,7 +226,7 @@
 
       stub_asset_manager_isnt_available
 
-      expect { DeleteDraftEditionService.call(edition.document, user) }
+      expect { DeleteDraftEditionService.call(edition, user) }
         .to raise_error(GdsApi::BaseError)
 
       expect(edition.reload.revision_synced?).to be false

From e1f15f418bc7b0b305d97c636dcd8e57f9392239 Mon Sep 17 00:00:00 2001
From: Kevin Dew <kevindew@me.com>
Date: Fri, 7 Feb 2020 22:07:26 +0000
Subject: [PATCH 3/4] Rearrange and refactor DeleteDraftEditionService tests

I found these tests quite hard to follow as there are quite a lot of
them. I decided that it would be easier to work with them if they were
in certain groupings so I've moved them into individual describe blocks.
This might well be a sign that this class has too many responsibilities
but that seems like a problem for another day.

Aside from these I've adapted these tests to be much more edition
centric and to no longer rely on any reloading. I've had to tweak a
bunch of factory calls and to resolve some inconsistencies with use of
brackets or not I've made them all use brackets - hopefully this isn't
too controversial.
---
 .../delete_draft_edition_service_spec.rb      | 341 ++++++++----------
 1 file changed, 158 insertions(+), 183 deletions(-)

diff --git a/spec/services/delete_draft_edition_service_spec.rb b/spec/services/delete_draft_edition_service_spec.rb
index e6ee7af47b..462079c15d 100644
--- a/spec/services/delete_draft_edition_service_spec.rb
+++ b/spec/services/delete_draft_edition_service_spec.rb
@@ -1,235 +1,210 @@
 # frozen_string_literal: true
 
 RSpec.describe DeleteDraftEditionService do
-  let(:user) { create :user }
+  let(:user) { create(:user) }
 
   describe ".call" do
-    it "raises an exception if the edition is not current" do
-      edition = create :edition, current: false
-
-      expect { DeleteDraftEditionService.call(edition, user) }
-        .to raise_error "Only current editions can be deleted"
-    end
-
-    it "raises an exception if the current edition is live" do
-      document = create :document, :with_live_edition
-
-      expect { DeleteDraftEditionService.call(document.current_edition, user) }
-        .to raise_error "Trying to delete a live edition"
-    end
-
-    it "attempts to delete the document preview" do
-      document = create :document, :with_current_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      request = stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(request).to have_been_requested
+    before do
+      stub_any_publishing_api_discard_draft
+      stub_any_publishing_api_unreserve_path
     end
 
-    it "attempts to delete the assets from asset manager" do
-      image_revision = create :image_revision, :on_asset_manager
-      file_attachment_revision = create :file_attachment_revision, :on_asset_manager
-      edition = create(:edition,
-                       lead_image_revision: image_revision,
-                       file_attachment_revisions: [file_attachment_revision])
+    describe "invalid edition" do
+      it "raises an error when the edition isn't current" do
+        edition = create(:edition, current: false)
 
-      stub_publishing_api_discard_draft(edition.content_id)
-      stub_publishing_api_unreserve_path(edition.base_path)
-      delete_request = stub_asset_manager_deletes_any_asset
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to raise_error("Only current editions can be deleted")
+      end
 
-      DeleteDraftEditionService.call(edition, user)
+      it "raises an exception if the current edition is live" do
+        edition = create(:edition, live: true)
 
-      expect(delete_request).to have_been_requested.at_least_once
-      expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
-      expect(file_attachment_revision.reload.asset).to be_absent
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to raise_error("Trying to delete a live edition")
+      end
     end
 
-    it "attempts to delete path reservations for a first draft" do
-      edition = create :edition
-      previous_revision = create :revision
-      edition.revisions << previous_revision
-
-      stub_publishing_api_discard_draft(edition.content_id)
-
-      unreserve_request1 = stub_publishing_api_unreserve_path(
-        edition.base_path,
-        PreviewDraftEditionService::Payload::PUBLISHING_APP,
-      )
-
-      unreserve_request2 = stub_publishing_api_unreserve_path(
-        previous_revision.base_path,
-        PreviewDraftEditionService::Payload::PUBLISHING_APP,
-      )
+    describe "changing edition status" do
+      it "changes the editions current flag" do
+        edition = create(:edition)
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to change { edition.current? }.to(false)
+      end
 
-      DeleteDraftEditionService.call(edition, user)
+      it "sets a live edition to be current after a draft is discarded" do
+        document = create(:document, :with_current_and_live_editions)
+        live_edition = document.live_edition
+        expect { DeleteDraftEditionService.call(document.current_edition, user) }
+          .to change { document.current_edition }.to(live_edition)
+      end
 
-      expect(unreserve_request1).to have_been_requested
-      expect(unreserve_request2).to have_been_requested
+      it "sets the status of the edition to discarded" do
+        edition = create(:edition)
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to change { edition.discarded? }.to(true)
+      end
     end
 
-    it "does not delete path reservations for published documents" do
-      document = create :document, :with_current_and_live_editions
-      stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(document.reload.current_edition).to eq document.live_edition
-    end
+    describe "Publishing API communication" do
+      it "discards the draft from the Publishing API" do
+        edition = create(:edition)
+        request = stub_publishing_api_discard_draft(edition.content_id)
+        DeleteDraftEditionService.call(edition, user)
+        expect(request).to have_been_requested
+      end
 
-    it "sets the current edition to nil if there is no live edition" do
-      document = create :document, :with_current_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(document.reload.current_edition).to be_nil
-    end
+      it "attempts to delete path reservations for a first draft" do
+        edition = create(:edition)
+        previous_revision = create(:revision)
+        edition.revisions << previous_revision
 
-    it "sets the current edition to live_edition if there is a live edition" do
-      document = create :document, :with_current_and_live_editions
-      live_edition = document.live_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(document.reload.current_edition).to eq(live_edition)
-    end
+        unreserve_request1 = stub_publishing_api_unreserve_path(
+          edition.base_path,
+          PreviewDraftEditionService::Payload::PUBLISHING_APP,
+        )
 
-    it "sets the status of the edition to discarded" do
-      document = create :document, :with_current_edition
-      edition = document.current_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      stub_publishing_api_discard_draft(document.content_id)
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(edition.status).to be_discarded
-    end
+        unreserve_request2 = stub_publishing_api_unreserve_path(
+          previous_revision.base_path,
+          PreviewDraftEditionService::Payload::PUBLISHING_APP,
+        )
 
-    it "copes if the document preview does not exist" do
-      document = create :document, :with_current_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      stub_any_publishing_api_call_to_return_not_found
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(document.reload.current_edition).to be_nil
-    end
+        DeleteDraftEditionService.call(edition, user)
 
-    it "copes if the publishing API has a live but not draft edition" do
-      document = create :document, :with_current_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      discard_draft_error = {
-        error: {
-          code: 422,
-          message: "There is not a draft edition of this document to discard",
-        },
-      }
-      stub_any_publishing_api_discard_draft
-        .to_return(status: 422, body: discard_draft_error.to_json)
-
-      DeleteDraftEditionService.call(document.current_edition, user)
-      expect(document.reload.current_edition).to be_nil
-    end
+        expect(unreserve_request1).to have_been_requested
+        expect(unreserve_request2).to have_been_requested
+      end
 
-    it "doesn't capture all Publishing API unprocessable entity issues" do
-      document = create :document, :with_current_edition
-      stub_publishing_api_unreserve_path(document.current_edition.base_path)
-      discard_draft_error = {
-        error: {
-          code: 422,
-          message: "New Publishing API problem",
-        },
-      }
-      stub_any_publishing_api_discard_draft
-        .to_return(status: 422, body: discard_draft_error.to_json)
+      it "does not delete path reservations for published documents" do
+        document = create(:document, :with_current_and_live_editions)
+        unreserve_request = stub_publishing_api_unreserve_path(
+          document.current_edition.base_path,
+          PreviewDraftEditionService::Payload::PUBLISHING_APP,
+        )
 
-      expect { DeleteDraftEditionService.call(document.current_edition, user) }
-        .to raise_error(GdsApi::HTTPUnprocessableEntity)
-    end
+        DeleteDraftEditionService.call(document.current_edition, user)
+        expect(unreserve_request).not_to have_been_requested
+      end
 
-    it "copes if an asset is not in Asset Manager" do
-      image_revision = create :image_revision
-      file_attachment_revision = create :file_attachment_revision
-      edition = create(:edition,
-                       lead_image_revision: image_revision,
-                       file_attachment_revisions: [file_attachment_revision])
 
-      stub_publishing_api_unreserve_path(edition.base_path)
-      stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition, user)
+      it "copes if the document preview does not exist" do
+        edition = create(:edition)
+        stub_any_publishing_api_discard_draft.to_return(status: 404)
+        DeleteDraftEditionService.call(edition, user)
+        expect(edition).to be_discarded
+      end
 
-      expect(edition.reload.status).to be_discarded
-      expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
-      expect(file_attachment_revision.reload.asset).to be_absent
-    end
+      it "copes if the publishing API has a live but not draft edition" do
+        edition = create(:edition)
+        discard_draft_error = {
+          error: {
+            code: 422,
+            message: "There is not a draft edition of this document to discard",
+          },
+        }
+        stub_any_publishing_api_discard_draft
+          .to_return(status: 422, body: discard_draft_error.to_json)
+
+        DeleteDraftEditionService.call(edition, user)
+        expect(edition).to be_discarded
+      end
 
-    it "copes if the base path is not reserved" do
-      edition = create :edition
+      it "doesn't capture all Publishing API unprocessable entity issues" do
+        edition = create(:edition)
+        discard_draft_error = {
+          error: {
+            code: 422,
+            message: "New Publishing API problem",
+          },
+        }
+        stub_any_publishing_api_discard_draft
+          .to_return(status: 422, body: discard_draft_error.to_json)
+
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to raise_error(GdsApi::HTTPUnprocessableEntity)
+      end
 
-      stub_publishing_api_unreserve_path_not_found(edition.base_path)
-      stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition, user)
+      it "copes if the base path is not reserved" do
+        edition = create(:edition)
 
-      expect(edition.reload.status).to be_discarded
-    end
+        stub_publishing_api_unreserve_path_not_found(edition.base_path)
+        DeleteDraftEditionService.call(edition, user)
 
-    it "copes if the base path is not valid" do
-      edition = create :edition, base_path: nil
+        expect(edition).to be_discarded
+      end
 
-      stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition, user)
+      it "copes if the base path is not valid" do
+        edition = create(:edition, base_path: nil)
+        DeleteDraftEditionService.call(edition, user)
+        expect(edition).to be_discarded
+      end
 
-      expect(edition.reload.status).to be_discarded
-    end
+      it "raises an error and marks an edition as not synced when an API error occurs during discarding" do
+        edition = create(:edition)
+        stub_publishing_api_isnt_available
 
-    it "removes assets if the asset is on Asset Manager" do
-      image_revision = create :image_revision, :on_asset_manager
-      file_attachment_revision = create :file_attachment_revision, :on_asset_manager
-      edition = create(:edition,
-                       lead_image_revision: image_revision,
-                       file_attachment_revisions: [file_attachment_revision])
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to raise_error(GdsApi::BaseError)
 
-      (image_revision.assets + [file_attachment_revision.asset]).map do |asset|
-        stub_asset_manager_delete_asset(asset.asset_manager_id)
+        expect(edition.revision_synced?).to be(false)
       end
 
-      stub_publishing_api_unreserve_path(edition.base_path)
-      stub_publishing_api_discard_draft(edition.content_id)
-      DeleteDraftEditionService.call(edition, user)
+      it "doesn't mark an edition as not synced when an API error occurs whilst unreserving paths" do
+        edition = create(:edition)
+        stub_publishing_api_unreserve_path_invalid(edition.base_path)
 
-      expect(edition.reload.status).to be_discarded
-      expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
-      expect(file_attachment_revision.reload.asset).to be_absent
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to raise_error(GdsApi::BaseError)
+
+        expect(edition.revision_synced?).to be(true)
+      end
     end
 
-    it "raises an error when a base path cannot be deleted" do
-      edition = create :edition
+    describe "Asset Manager communication" do
+      it "attempts to delete the assets from asset manager" do
+        image_revision = create(:image_revision, :on_asset_manager)
+        file_attachment_revision = create(:file_attachment_revision, :on_asset_manager)
+        edition = create(:edition,
+                         lead_image_revision: image_revision,
+                         file_attachment_revisions: [file_attachment_revision])
 
-      stub_publishing_api_discard_draft(edition.content_id)
-      stub_publishing_api_unreserve_path_invalid(edition.base_path)
+        delete_request = stub_asset_manager_deletes_any_asset
 
-      expect { DeleteDraftEditionService.call(edition, user) }
-        .to raise_error(GdsApi::BaseError)
+        DeleteDraftEditionService.call(edition, user)
 
-      expect(edition.reload.revision_synced?).to be true
-    end
+        expect(delete_request).to have_been_requested.at_least_once
+        expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
+        expect(file_attachment_revision.reload.asset).to be_absent
+      end
 
-    it "raises an error when the Pubishing API is down" do
-      edition = create :edition
-      stub_publishing_api_isnt_available
+      it "copes if an asset is not in Asset Manager" do
+        image_revision = create(:image_revision, :on_asset_manager)
+        file_attachment_revision = create(:file_attachment_revision, :on_asset_manager)
+        edition = create(:edition,
+                         lead_image_revision: image_revision,
+                         file_attachment_revisions: [file_attachment_revision])
+        stub_any_asset_manager_call.to_return(status: 404)
 
-      expect { DeleteDraftEditionService.call(edition, user) }
-        .to raise_error(GdsApi::BaseError)
+        DeleteDraftEditionService.call(edition, user)
 
-      expect(edition.reload.revision_synced?).to be false
-    end
+        expect(image_revision.reload.assets.map(&:state).uniq).to eq(%w[absent])
+        expect(file_attachment_revision.reload.asset).to be_absent
+      end
 
-    it "raises an error when Asset Manager is down" do
-      image_revision = create :image_revision, :on_asset_manager
-      file_attachment_revision = create :file_attachment_revision, :on_asset_manager
-      edition = create(:edition,
-                       lead_image_revision: image_revision,
-                       file_attachment_revisions: [file_attachment_revision])
+      it "raises an error and marks as an edition as not synced when Asset Manager is down" do
+        image_revision = create(:image_revision, :on_asset_manager)
+        file_attachment_revision = create(:file_attachment_revision, :on_asset_manager)
+        edition = create(:edition,
+                         lead_image_revision: image_revision,
+                         file_attachment_revisions: [file_attachment_revision])
 
-      stub_asset_manager_isnt_available
+        stub_asset_manager_isnt_available
 
-      expect { DeleteDraftEditionService.call(edition, user) }
-        .to raise_error(GdsApi::BaseError)
+        expect { DeleteDraftEditionService.call(edition, user) }
+          .to raise_error(GdsApi::BaseError)
 
-      expect(edition.reload.revision_synced?).to be false
+        expect(edition.revision_synced?).to be(false)
+      end
     end
   end
 end

From bf4385e9714ca48b039403b59fda6a7ea150d5eb Mon Sep 17 00:00:00 2001
From: Kevin Dew <kevindew@me.com>
Date: Mon, 10 Feb 2020 11:56:30 +0000
Subject: [PATCH 4/4] Switch to passing edition into DeleteDraftEditionService

---
 app/interactors/editions/destroy_interactor.rb       | 2 +-
 spec/interactors/editions/destroy_interactor_spec.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/interactors/editions/destroy_interactor.rb b/app/interactors/editions/destroy_interactor.rb
index 434a324cb6..16821f9791 100644
--- a/app/interactors/editions/destroy_interactor.rb
+++ b/app/interactors/editions/destroy_interactor.rb
@@ -23,7 +23,7 @@ def find_and_lock_edition
   end
 
   def discard_draft
-    DeleteDraftEditionService.call(edition.document, user)
+    DeleteDraftEditionService.call(edition, user)
   rescue GdsApi::BaseError => e
     GovukError.notify(e)
     context.fail!(api_error: true)
diff --git a/spec/interactors/editions/destroy_interactor_spec.rb b/spec/interactors/editions/destroy_interactor_spec.rb
index 7ffb907c65..4796e8c95e 100644
--- a/spec/interactors/editions/destroy_interactor_spec.rb
+++ b/spec/interactors/editions/destroy_interactor_spec.rb
@@ -23,7 +23,7 @@
     it "delegates to the DeleteDraftEditionService" do
       expect(DeleteDraftEditionService)
         .to receive(:call)
-        .with(edition.document, user)
+        .with(edition, user)
       Editions::DestroyInteractor.call(params: params, user: user)
     end