Send attachable_url for non-edition attachables

The previous code did not work at all for policy groups, and consultation and call for evidence responses, which are not editions.

Since the access limitation restrictions should already kick in before this asset payload is constructed, there is no reason to check for visibility at this level. We have removed the calls to `visible_edition` in favour of just using the `significant_attachable`. This should correctly return either visible attachables or the draft attachable. This works correctly for all attachable types, including responses and policy groups.

In order to correctly construct the draft or live attachable URL, we need to determine if it is in draft state. Policy groups are immediately published, so this does not apply to them. For responses, we want to send the parent edition's URL, so we can check the parent state. I have made the `parent_attachable` method public, so we can identify response-like classes - this method is only defined on `ConsultationResponse` and `CallForEvidenceResponse` at the moment.

For context, the method was previously never called with a user argument to check the visibility for. The line `visible_edition.blank? && draft_edition` was particularly confusing, since the visible edition should also return the draft_edition, if called with a user. It also invalidated the whole point of ever checking for visibility, when falling back to the draft_edition anyway.

Author: lauraghiorghisor-tw

app/models/attachment_data.rb

@@ -169,12 +169,23 @@ def redirect_url
     unpublished_attachable.unpublishing.document_url
   end
 
-  def attachable_url(user = nil)
-    visible_edition = visible_edition_for(user)
-    if visible_edition.blank? && draft_edition
-      draft_edition.public_url(draft: true)
-    elsif visible_edition.present?
-      visible_edition.public_url
+  def attachable_url
+    return nil if significant_attachable.blank?
+
+    if significant_attachable.is_a?(Edition)
+      url_for(significant_attachable)
+    elsif significant_attachable.respond_to?(:parent_attachable) && significant_attachable.parent_attachable.is_a?(Edition)
+      url_for(significant_attachable.parent_attachable)
+    elsif significant_attachable.is_a?(PolicyGroup)
+      significant_attachable.public_url
+    end
+  end
+
+  def url_for(edition)
+    if Edition::PRE_PUBLICATION_STATES.include?(edition.state)
+      edition.public_url(draft: true)
+    elsif edition.publicly_visible?
+      edition.public_url
     end
   end
 
@@ -188,7 +199,7 @@ def keep_existing_file?
     to_replace_id.present? && keep_or_replace != "replace"
   end
 
-private
+  private
 
   def filtered_attachments(include_deleted_attachables: false)
     if include_deleted_attachables

app/models/call_for_evidence_response.rb

@@ -57,8 +57,6 @@ def path_name
 
   delegate :public_timestamp, :first_published_version?, :slug, :document, :images, :content_id, to: :call_for_evidence
 
-private
-
   def parent_attachable
     call_for_evidence || Attachable::Null.new
   end

app/models/consultation_response.rb

@@ -57,8 +57,6 @@ def path_name
 
   delegate :public_timestamp, :first_published_version?, :slug, :document, :images, :content_id, to: :consultation
 
-private
-
   def parent_attachable
     consultation || Attachable::Null.new
   end

test/unit/app/models/attachment_data_test.rb

@@ -445,4 +445,158 @@ class AttachmentDataTest < ActiveSupport::TestCase
 
     assert_not attachment_data.all_asset_variants_uploaded?
   end
+
+  context "#attachable_url" do
+    context "when the attachable is an edition" do
+      it "returns draft url for pre-publication edition" do
+        edition = create(:draft_standard_edition, :with_file_attachment)
+        attachment_data = edition.attachments.first.attachment_data
+
+        assert_equal edition.public_url(draft: true), attachment_data.attachable_url
+      end
+
+      Edition::PUBLICLY_VISIBLE_STATES.each do |state|
+        it "returns live url for #{state} edition" do
+          edition = create(:"#{state}_standard_edition", :with_file_attachment)
+          attachment_data = edition.attachments.first.attachment_data
+
+          assert_equal edition.public_url, attachment_data.attachable_url
+        end
+      end
+
+      it "returns nil for deleted edition" do
+        edition = create(:draft_standard_edition, :with_file_attachment)
+        attachment_data = edition.attachments.first.attachment_data
+        assert attachment_data.attachments.first.attachable
+
+        edition.destroy!
+        edition.delete_all_attachments
+        # Tests would still pass without this line which is executed as part of the EditionDeleter, but wanted to make sure it doesn't have anything to do with the attachment being deleted.
+        # This indicates we could write these as controller or other integration tests.
+
+        assert_equal true, attachment_data.reload.attachments.first.deleted?
+        assert_nil attachment_data.attachments.first.attachable
+        assert_nil attachment_data.attachable_url
+      end
+
+      it "returns nil for unpublished edition" do
+        edition = create(:unpublished_standard_edition, :with_file_attachment)
+        attachment_data = edition.attachments.first.attachment_data
+
+        assert_nil attachment_data.attachable_url
+      end
+    end
+
+    context "when the attachable is a consultation outcome" do
+      Edition::PRE_PUBLICATION_STATES.each do |state|
+        it "returns draft url for #{state} consultation" do
+          consultation = create(:"#{state}_consultation")
+          outcome = create(:consultation_outcome, :with_file_attachment, consultation:)
+          attachment_data = outcome.attachments.first.attachment_data
+
+          assert_equal consultation.public_url(draft: true), attachment_data.attachable_url
+        end
+      end
+
+      Edition::PUBLICLY_VISIBLE_STATES.each do |state|
+        it "returns live url for #{state} consultation" do
+          consultation = create(:"#{state}_consultation")
+          outcome = create(:consultation_outcome, :with_file_attachment, consultation:)
+          attachment_data = outcome.attachments.first.attachment_data
+
+          assert_equal consultation.public_url, attachment_data.attachable_url
+        end
+      end
+
+      it "returns nil for deleted consultation" do
+        consultation = create(:consultation)
+        outcome = create(:consultation_outcome, :with_file_attachment, consultation:)
+        attachment_data = outcome.attachments.first.attachment_data
+        assert attachment_data.attachments.first.attachable
+
+        consultation.destroy!
+        consultation.delete_all_attachments
+        # Tests would still pass without this line which is executed as part of the EditionDeleter, but wanted to make sure it doesn't have anything to do with the attachment being deleted.
+        # This indicates we could write these as controller or other integration tests.
+
+        assert_equal true, attachment_data.reload.attachments.first.deleted?
+        assert_nil attachment_data.attachments.first.attachable
+        assert_nil attachment_data.attachable_url
+      end
+
+      it "returns nil for unpublished consultation" do
+        consultation = create(:unpublished_consultation)
+        outcome = create(:consultation_outcome, :with_file_attachment, consultation:)
+        attachment_data = outcome.attachments.first.attachment_data
+
+        assert_nil attachment_data.attachable_url
+      end
+    end
+
+    context "when the attachable is a call for evidence outcome" do
+      Edition::PRE_PUBLICATION_STATES.each do |state|
+        it "returns draft url for #{state} call for evidence" do
+          call_for_evidence = create(:"#{state}_call_for_evidence")
+          outcome = create(:call_for_evidence_outcome, :with_file_attachment, call_for_evidence:)
+          attachment_data = outcome.attachments.first.attachment_data
+
+          assert_equal call_for_evidence.public_url(draft: true), attachment_data.attachable_url
+        end
+      end
+
+      Edition::PUBLICLY_VISIBLE_STATES.each do |state|
+        it "returns live url for #{state} call for evidence" do
+          call_for_evidence = create(:"#{state}_call_for_evidence")
+          outcome = create(:call_for_evidence_outcome, :with_file_attachment, call_for_evidence:)
+          attachment_data = outcome.attachments.first.attachment_data
+
+          assert_equal call_for_evidence.public_url, attachment_data.attachable_url
+        end
+      end
+
+      it "returns nil for deleted call for evidence" do
+        call_for_evidence = create(:call_for_evidence)
+        outcome = create(:call_for_evidence_outcome, :with_file_attachment, call_for_evidence:)
+        attachment_data = outcome.attachments.first.attachment_data
+        assert attachment_data.attachments.first.attachable
+
+        call_for_evidence.destroy!
+        call_for_evidence.delete_all_attachments
+
+        assert_equal true, attachment_data.reload.attachments.first.deleted?
+        assert_nil attachment_data.attachments.first.attachable
+        assert_nil attachment_data.attachable_url
+      end
+
+      it "returns nil for unpublished call for evidence" do
+        call_for_evidence = create(:unpublished_call_for_evidence)
+        outcome = create(:call_for_evidence_outcome, :with_file_attachment, call_for_evidence:)
+        attachment_data = outcome.attachments.first.attachment_data
+
+        assert_nil attachment_data.attachable_url
+      end
+    end
+
+    context "when the attachable is a policy group" do
+      it "returns live url" do
+        policy_group = create(:policy_group, :with_file_attachment)
+        attachment_data = policy_group.attachments.first.attachment_data
+
+        assert_equal policy_group.public_url, attachment_data.attachable_url
+      end
+
+      it "returns nil for deleted policy group" do
+        policy_group = create(:policy_group, :with_file_attachment)
+        attachment_data = policy_group.attachments.first.attachment_data
+        assert attachment_data.attachments.first.attachable
+
+        policy_group.destroy!
+        policy_group.delete_all_attachments
+
+        assert_equal true, attachment_data.reload.attachments.first.deleted?
+        assert_nil attachment_data.attachments.first.attachable
+        assert_nil attachment_data.attachable_url
+      end
+    end
+  end
 end

test/unit/app/services/asset_manager/attachment_updater_test.rb

@@ -191,5 +191,64 @@ class AssetManager::AttachmentUpdaterTest < ActiveSupport::TestCase
         end
       end
     end
+
+    context "when the attachment belongs to a draft consultation's outcome" do
+      let(:consultation) { create(:draft_consultation) }
+      let(:outcome) { create(:consultation_outcome, :with_file_attachment, consultation:) }
+      let(:attachment_data) { outcome.attachments.first.attachment_data }
+      let(:asset_manager_id) { attachment_data.assets.first.asset_manager_id }
+
+      it "sets the expected attributes" do
+        AssetManager::AssetUpdater.expects(:call).with(
+          asset_manager_id,
+          {
+            "draft" => true,
+            "access_limited_organisation_ids" => [],
+            "parent_document_url" => consultation.public_url(draft: true),
+          },
+        )
+
+        AssetManager::AttachmentUpdater.call(attachment_data)
+      end
+    end
+
+    context "when the attachment belongs to a published consultation's outcome" do
+      let(:consultation) { create(:published_consultation) }
+      let(:outcome) { create(:consultation_outcome, :with_file_attachment, consultation:) }
+      let(:attachment_data) { outcome.attachments.first.attachment_data }
+      let(:asset_manager_id) { attachment_data.assets.first.asset_manager_id }
+
+      it "sets the expected attributes" do
+        AssetManager::AssetUpdater.expects(:call).with(
+          asset_manager_id,
+          {
+            "draft" => false,
+            "access_limited_organisation_ids" => [],
+            "parent_document_url" => consultation.public_url,
+          },
+        )
+
+        AssetManager::AttachmentUpdater.call(attachment_data)
+      end
+    end
+
+    context "when the attachment belongs to a policy group" do
+      let(:policy_group) { create(:policy_group, :with_file_attachment) }
+      let(:attachment_data) { policy_group.attachments.first.attachment_data }
+      let(:asset_manager_id) { attachment_data.assets.first.asset_manager_id }
+
+      it "sets the expected attributes" do
+        AssetManager::AssetUpdater.expects(:call).with(
+          asset_manager_id,
+          {
+            "draft" => false,
+            "access_limited_organisation_ids" => [],
+            "parent_document_url" => policy_group.public_url,
+          },
+        )
+
+        AssetManager::AttachmentUpdater.call(attachment_data)
+      end
+    end
   end
 end