Remove the publishing step when trying to republish HTML attachments …

…for unpublished or withdrawn 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 or withdrawn, 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 article first.

app/services/service_listeners/publishing_api_html_attachments.rb

@@ -18,8 +18,16 @@ 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
+        update_draft(update_type: "republish")
+        patch_links
+        edition.withdrawn? ? withdraw : unpublish(allow_draft: true)
+      else
+        do_publish("republish")
+        discard_drafts(deleted_html_attachments)
+      end
     end
 
     def update_draft(update_type: nil)
@@ -32,7 +40,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 +88,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",
       }),
     ]
@@ -278,7 +275,6 @@ class PublishingApiDocumentRepublishingWorkerIntegrationTest < ActiveSupport::Te
     repeated_requests = [
       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: "en", update_type: nil),
       stub_publishing_api_unpublish(html_attachment_presenter.content_id, body: withdrawal_content.merge(locale: "en")),
     ]
 

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

@@ -439,12 +439,6 @@ class Republish < PublishingApiHtmlAttachmentsTest
       test "for a withdrawn publicaton with an attachment withdraws the attachment" do
         publication = create(:withdrawn_publication)
         attachment = publication.html_attachments.first
-        PublishingApiWorker.any_instance.expects(:perform).with(
-          "HtmlAttachment",
-          attachment.id,
-          "republish",
-          "en",
-        )
         PublishingApiWithdrawalWorker.any_instance.expects(:perform).with(
           attachment.content_id,
           "content was withdrawn",
@@ -490,6 +484,13 @@ class Republish < PublishingApiHtmlAttachmentsTest
         )
         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)
+        attachment = publication.html_attachments.first
+        PublishingApiWorker.any_instance.expects(:perform).never
+        call(publication)
+      end
     end
 
     class Unwithdraw < PublishingApiHtmlAttachmentsTest