Remove the publishing step when trying to republish HTML attachments …

…for unpublished documents

The publishing of the HTML attachment could leave the document in a bad state and especially dangerous around sensitive documents where they should be unpublished, but are then inadvertently published during the republishing process.
We should be able to reapply the redirects for unpublishing without the need to publish the document first.
Withdrawn document process has been left as is for the moment.

app/services/service_listeners/publishing_api_html_attachments.rb

@@ -18,8 +18,20 @@ def publish
     alias_method :unwithdraw, :publish
 
     def republish
-      update_publishing_api_content
-      unpublish_if_required
+      if Edition::PRE_PUBLICATION_STATES.include?(edition.state)
+        update_draft(update_type: "republish")
+      elsif edition.unpublishing && edition.withdrawn?
+        do_publish("republish")
+        discard_drafts(deleted_html_attachments)
+        withdraw
+      elsif edition.unpublishing
+        update_draft(update_type: "republish")
+        patch_links
+        unpublish(allow_draft: true)
+      else
+        do_publish("republish")
+        discard_drafts(deleted_html_attachments)
+      end
     end
 
     def update_draft(update_type: nil)
@@ -32,7 +44,6 @@ def update_draft(update_type: nil)
       end
       discard_drafts(deleted_html_attachments)
     end
-
     # We don't care whether this is a translation or the main
     # document, we just send the correct html attachments regardless.
     alias_method :update_draft_translation, :update_draft
@@ -81,22 +92,9 @@ def discard_drafts(html_attachments)
       end
     end
 
-    def update_publishing_api_content
-      if Edition::PRE_PUBLICATION_STATES.include?(edition.state)
-        update_draft(update_type: "republish")
-      else
-        do_publish("republish")
-        discard_drafts(deleted_html_attachments)
-      end
-    end
-
-    def unpublish_if_required
-      if edition.unpublishing
-        if edition.withdrawn?
-          withdraw
-        else
-          unpublish(allow_draft: false)
-        end
+    def patch_links
+      current_html_attachments.each do |html_attachment|
+        Whitehall::PublishingApi.patch_links(html_attachment, bulk_publishing: false)
       end
     end
 

test/integration/workers/publishing_api_document_republishing_worker_integration_test.rb

@@ -156,11 +156,10 @@ class PublishingApiDocumentRepublishingWorkerIntegrationTest < ActiveSupport::Te
       }),
       stub_publishing_api_put_content(html_attachment_presenter.content_id, html_attachment_presenter.content),
       stub_publishing_api_patch_links(html_attachment_presenter.content_id, links: html_attachment_presenter.links),
-      stub_publishing_api_publish(html_attachment_presenter.content_id, locale: html_attachment_presenter.content[:locale], update_type: nil),
       stub_publishing_api_unpublish(html_attachment_presenter.content_id, body: {
         type: "redirect",
         alternative_path: edition.base_path,
-        discard_drafts: true,
+        allow_draft: true,
         locale: "en",
       }),
     ]
@@ -199,11 +198,10 @@ class PublishingApiDocumentRepublishingWorkerIntegrationTest < ActiveSupport::Te
       }),
       stub_publishing_api_put_content(html_attachment_presenter.content_id, html_attachment_presenter.content),
       stub_publishing_api_patch_links(html_attachment_presenter.content_id, links: html_attachment_presenter.links),
-      stub_publishing_api_publish(html_attachment_presenter.content_id, locale: html_attachment_presenter.content[:locale], update_type: nil),
       stub_publishing_api_unpublish(html_attachment_presenter.content_id, body: {
         type: "redirect",
         alternative_path: edition.base_path,
-        discard_drafts: true,
+        allow_draft: true,
         locale: "en",
       }),
       stub_publishing_api_put_content(draft_publication_presenter.content_id, with_locale(:en) { draft_publication_presenter.content }),
@@ -236,11 +234,10 @@ class PublishingApiDocumentRepublishingWorkerIntegrationTest < ActiveSupport::Te
       }),
       stub_publishing_api_put_content(html_attachment_presenter.content_id, html_attachment_presenter.content),
       stub_publishing_api_patch_links(html_attachment_presenter.content_id, links: html_attachment_presenter.links),
-      stub_publishing_api_publish(html_attachment_presenter.content_id, locale: html_attachment_presenter.content[:locale], update_type: nil),
       stub_publishing_api_unpublish(html_attachment_presenter.content_id, body: {
         type: "redirect",
         alternative_path: edition.base_path,
-        discard_drafts: true,
+        allow_draft: true,
         locale: "en",
       }),
     ]

test/unit/app/services/service_listeners/publishing_api_html_attachments_test.rb

@@ -436,7 +436,7 @@ class Republish < PublishingApiHtmlAttachmentsTest
         end
       end
 
-      test "for a withdrawn publicaton with an attachment withdraws the attachment" do
+      test "for a withdrawn publication with an attachment withdraws the attachment" do
         publication = create(:withdrawn_publication)
         attachment = publication.html_attachments.first
         PublishingApiWorker.any_instance.expects(:perform).with(
@@ -462,7 +462,7 @@ class Republish < PublishingApiHtmlAttachmentsTest
           attachment.content_id,
           "/government/another/page",
           "en",
-          false,
+          true,
         )
         call(publication)
       end
@@ -474,7 +474,7 @@ class Republish < PublishingApiHtmlAttachmentsTest
           attachment.content_id,
           "/government/another/page",
           "en",
-          false,
+          true,
         )
         call(publication)
       end
@@ -486,10 +486,16 @@ class Republish < PublishingApiHtmlAttachmentsTest
           attachment.content_id,
           publication.search_link,
           "en",
-          false,
+          true,
         )
         call(publication)
       end
+
+      test "for a publication that has been unpublished does not publish the attachment in order to unpublish again" do
+        publication = create(:unpublished_publication_in_error_no_redirect)
+        PublishingApiWorker.any_instance.expects(:perform).never
+        call(publication)
+      end
     end
 
     class Unwithdraw < PublishingApiHtmlAttachmentsTest