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,17 @@ 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)
+      attachable_in_pre_publication_state = Edition::PRE_PUBLICATION_STATES.include?(significant_attachable.state)
+      attachable_in_pre_publication_state ? significant_attachable.public_url(draft: true) : significant_attachable.public_url
+    elsif significant_attachable.respond_to?(:parent_attachable) && significant_attachable.parent_attachable.is_a?(Edition)
+      parent_attachable_in_pre_publication_state = Edition::PRE_PUBLICATION_STATES.include?(significant_attachable.parent_attachable.state)
+      parent_attachable_in_pre_publication_state ? significant_attachable.parent_attachable.public_url(draft: true) : significant_attachable.parent_attachable.public_url
+    elsif significant_attachable.is_a?(PolicyGroup)
+      significant_attachable.public_url
     end
   end
 

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,101 @@ 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
+      # This test never passed
+      # We never call it with a user so no need to worry about visibility - can remove
+      # it "returns draft url if edition in draft, when called with specific user" do
+      #   edition = create(:draft_standard_edition, :with_file_attachment)
+      #   attachment_data = edition.attachments.first.attachment_data
+      #   user = create(:user)
+      #
+      #   assert_equal edition.public_url(draft: true), attachment_data.attachable_url(user)
+      # end
+
+      # Works because visible_to?(user) returns true because !draft
+      # We never call it with a user so no need to worry about visibility - can remove
+      # it "returns live url if edition is published, when called with specific user" do
+      #   edition = create(:published_standard_edition, :with_file_attachment)
+      #   attachment_data = edition.attachments.first.attachment_data
+      #   user = create(:user)
+      #
+      #   assert_equal edition.public_url, attachment_data.attachable_url(user)
+      # end
+
+      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
+
+      it "returns live url for published edition" do
+        # Works because visible_to?(user) returns true because !draft
+        edition = create(:published_standard_edition, :with_file_attachment)
+        attachment_data = edition.attachments.first.attachment_data
+
+        assert_equal edition.public_url, attachment_data.attachable_url
+      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
+    end
+
+    context "when the attachable is a consultation outcome" do
+      it "returns draft url for pre-publication consultation" do
+        consultation = create(:draft_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
+
+      it "returns live url for published consultation" do
+        consultation = create(:published_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
+
+      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
+    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
+    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