Merge pull request #133 from alphagov/dont-desync-locale

Do not desync documents based on locale

app/models/concerns/publishing_api/action.rb

@@ -14,15 +14,19 @@ def sync?
     end
 
     def desync?
-      UNPUBLISH_DOCUMENT_TYPES.include?(document_type)
+      unpublish_type? || on_ignorelist? || unaddressable? || withdrawn?
     end
 
     def skip?
-      on_ignorelist? || ignored_locale? || unaddressable? || withdrawn?
+      # Do not proactively desync documents with an ignored locale, as they may have content with a
+      # non-ignored locale with the same content_id.
+      ignored_locale?
     end
 
     def action_reason
-      if on_ignorelist?
+      if unpublish_type?
+        "unpublish type (#{document_type})"
+      elsif on_ignorelist?
         "document_type on ignorelist (#{document_type})"
       elsif ignored_locale?
         "locale not permitted (#{locale})"
@@ -35,6 +39,10 @@ def action_reason
 
   private
 
+    def unpublish_type?
+      UNPUBLISH_DOCUMENT_TYPES.include?(document_type)
+    end
+
     # rubocop:disable Style/CaseEquality (no semantically equal alternative to compare String/Regex)
     def on_ignorelist?
       return false if ignorelist_excepted_path?

app/models/publishing_api_document.rb

@@ -27,9 +27,6 @@ def synchronize
       delete_service.call(content_id, payload_version:)
     else
       log("skip (#{action_reason})")
-      # Eagerly attempt to delete the document anyway in case it has been previously synchronised
-      # (for example, if the ignorelist has changed and it should now no longer be in search)
-      delete_service.call(content_id, payload_version:)
     end
   end
 

spec/integration/document_synchronization_spec.rb

@@ -254,18 +254,15 @@
   describe "for a non-English 'worldwide_organisation' message" do
     let(:payload) { json_fixture_as_hash("message_queue/non_english_worldwide_organisation_message.json") }
 
-    it "is ignored and deleted from Discovery Engine through the Delete service" do
-      expect(delete_service).to have_received(:call).with(
-        "f4c394f9-7a30-11e4-a3cb-005056011aef",
-        payload_version: 12_345,
-      )
+    it "is skipped completely and not deleted" do
+      expect(delete_service).not_to have_received(:call)
     end
   end
 
   describe "for a withdrawn 'notice' message" do
     let(:payload) { json_fixture_as_hash("message_queue/withdrawn_notice_message.json") }
 
-    it "is ignored and deleted from Discovery Engine through the Delete service" do
+    it "is proactively deleted from Discovery Engine through the Delete service" do
       expect(delete_service).to have_received(:call).with(
         "e3b7c15d-1928-4101-9912-c9b40a6d6e78",
         payload_version: 12_345,
@@ -276,7 +273,7 @@
   describe "for an 'html_publication' message" do
     let(:payload) { json_fixture_as_hash("message_queue/html_publication_message.json") }
 
-    it "is ignored and deleted from Discovery Engine through the Delete service" do
+    it "is proactively deleted from Discovery Engine through the Delete service" do
       expect(delete_service).to have_received(:call).with(
         "1f1f2c96-5a14-4d2a-9d0c-be6ac6c62c3b",
         payload_version: 12_345,

spec/models/concerns/publishing_api/action_spec.rb

@@ -19,7 +19,7 @@
   context "when the document type is on the ignore list as a string" do
     let(:document_type) { "test_ignored_type" } # see test section in YAML config
 
-    it { is_expected.to be_skip }
+    it { is_expected.to be_desync }
 
     it "has the expected action_reason" do
       expect(action.action_reason).to eq("document_type on ignorelist (test_ignored_type)")
@@ -41,7 +41,7 @@
   context "when the document type is on the ignore list as a pattern" do
     let(:document_type) { "another_test_ignored_type_foo" } # see test section in YAML config
 
-    it { is_expected.to be_skip }
+    it { is_expected.to be_desync }
 
     it "has the expected action_reason" do
       expect(action.action_reason).to eq(
@@ -55,7 +55,7 @@
     let(:base_path) { nil }
     let(:url) { nil }
 
-    it { is_expected.to be_skip }
+    it { is_expected.to be_desync }
 
     it "has the expected action_reason" do
       expect(action.action_reason).to eq("unaddressable")
@@ -84,14 +84,14 @@
     let(:document_type) { "test_ignored_type" } # see test section in YAML config
     let(:base_path) { "/test_ignored_path" } # see test section in YAML config
 
-    it { is_expected.to be_skip }
+    it { is_expected.to be_desync }
   end
 
   context "when the document is withdrawn" do
     let(:document_type) { "notice" }
     let(:withdrawn_notice) { { explanation: "test" } }
 
-    it { is_expected.to be_skip }
+    it { is_expected.to be_desync }
 
     it "has the expected action_reason" do
       expect(action.action_reason).to eq("withdrawn")