From 630ab47f6c97059c73919645f61ab0a291a61b36 Mon Sep 17 00:00:00 2001 From: Minno Dang Date: Tue, 30 Jan 2024 18:49:12 +0000 Subject: [PATCH] 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. --- .../publishing_api_html_attachments.rb | 36 +++++++++---------- ...nt_republishing_worker_integration_test.rb | 9 ++--- .../publishing_api_html_attachments_test.rb | 14 +++++--- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/app/services/service_listeners/publishing_api_html_attachments.rb b/app/services/service_listeners/publishing_api_html_attachments.rb index 7860b68273e..8cf07700ee2 100644 --- a/app/services/service_listeners/publishing_api_html_attachments.rb +++ b/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 diff --git a/test/integration/workers/publishing_api_document_republishing_worker_integration_test.rb b/test/integration/workers/publishing_api_document_republishing_worker_integration_test.rb index 7a0f3fb325a..0721bb3bc15 100644 --- a/test/integration/workers/publishing_api_document_republishing_worker_integration_test.rb +++ b/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", }), ] diff --git a/test/unit/app/services/service_listeners/publishing_api_html_attachments_test.rb b/test/unit/app/services/service_listeners/publishing_api_html_attachments_test.rb index 8dbbe42ef89..bb2364f764d 100644 --- a/test/unit/app/services/service_listeners/publishing_api_html_attachments_test.rb +++ b/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