diff --git a/app/controllers/v2/content_items_controller.rb b/app/controllers/v2/content_items_controller.rb index 12ad7a677..f9a73b7a3 100644 --- a/app/controllers/v2/content_items_controller.rb +++ b/app/controllers/v2/content_items_controller.rb @@ -22,7 +22,7 @@ def linkables end def embedded - results = GetHostContentService.new( + results = GetEmbeddedContentService.new( path_params[:content_id], query_params[:order], query_params[:page], diff --git a/app/presenters/embedded_content_presenter.rb b/app/presenters/embedded_content_presenter.rb index afe0394f1..39d12a4bd 100644 --- a/app/presenters/embedded_content_presenter.rb +++ b/app/presenters/embedded_content_presenter.rb @@ -32,6 +32,7 @@ def results last_edited_by_editor_id: edition.last_edited_by_editor_id, last_edited_at: edition.last_edited_at, unique_pageviews: edition.unique_pageviews, + instances: edition.instances, primary_publishing_organisation: { content_id: edition.primary_publishing_organisation_content_id, title: edition.primary_publishing_organisation_title, diff --git a/app/queries/get_embedded_content.rb b/app/queries/get_embedded_content.rb index 7035a8dfb..9c9c8a77f 100644 --- a/app/queries/get_embedded_content.rb +++ b/app/queries/get_embedded_content.rb @@ -12,6 +12,7 @@ class GetEmbeddedContent :primary_publishing_organisation_title, :primary_publishing_organisation_base_path, :unique_pageviews, + :instances, ) DEFAULT_PER_PAGE = 10 @@ -27,17 +28,18 @@ class GetEmbeddedContent }.freeze FIELDS = [ - TABLES[:editions][:id], - TABLES[:editions][:title], - TABLES[:editions][:base_path], - TABLES[:editions][:document_type], - TABLES[:editions][:publishing_app], - TABLES[:editions][:last_edited_by_editor_id], - TABLES[:editions][:last_edited_at], - TABLES[:primary_links][:target_content_id].as("primary_publishing_organisation_content_id"), - TABLES[:org_editions][:title].as("primary_publishing_organisation_title"), - TABLES[:org_editions][:base_path].as("primary_publishing_organisation_base_path"), - TABLES[:statistics_caches][:unique_pageviews], + { field: TABLES[:editions][:id], included_in_group?: true }, + { field: TABLES[:editions][:title], included_in_group?: true }, + { field: TABLES[:editions][:base_path], included_in_group?: true }, + { field: TABLES[:editions][:document_type], included_in_group?: true }, + { field: TABLES[:editions][:publishing_app], included_in_group?: true }, + { field: TABLES[:editions][:last_edited_by_editor_id], included_in_group?: true }, + { field: TABLES[:editions][:last_edited_at], included_in_group?: true }, + { field: TABLES[:primary_links][:target_content_id], alias: "primary_publishing_organisation_content_id", included_in_group?: true }, + { field: TABLES[:org_editions][:title], alias: "primary_publishing_organisation_title", included_in_group?: true }, + { field: TABLES[:org_editions][:base_path], alias: "primary_publishing_organisation_base_path", included_in_group?: true }, + { field: TABLES[:statistics_caches][:unique_pageviews], included_in_group?: true }, + { field: TABLES[:editions][:id].count, alias: "instances", included_in_group?: false }, ].freeze ORDER_FIELDS = { @@ -46,6 +48,7 @@ class GetEmbeddedContent unique_pageviews: TABLES[:statistics_caches][:unique_pageviews], primary_publishing_organisation_title: TABLES[:org_editions][:title], last_edited_at: TABLES[:editions][:last_edited_at], + instances: TABLES[:editions][:id].count, }.freeze ORDER_DIRECTIONS = %i[asc desc].freeze @@ -94,9 +97,17 @@ def count_query "SELECT COUNT(*) FROM (#{arel_query.to_sql}) AS full_query" end + def select_fields + FIELDS.map { |f| f[:alias].present? ? f[:field].as(f[:alias]) : f[:field] } + end + + def group_fields + FIELDS.select { |f| f[:included_in_group?] }.pluck(:field) + end + def arel_joins TABLES[:editions] - .project(FIELDS) + .project(select_fields) .join(TABLES[:links]).on( TABLES[:links][:edition_id].eq(TABLES[:editions][:id]), ) @@ -117,6 +128,7 @@ def arel_joins .join(TABLES[:statistics_caches], Arel::Nodes::OuterJoin).on( TABLES[:statistics_caches][:document_id].eq(TABLES[:documents][:id]), ) + .group(group_fields) end def embedded_link_type diff --git a/app/services/embedded_content_finder_service.rb b/app/services/embedded_content_finder_service.rb index 18f6c8fd5..f56a91643 100644 --- a/app/services/embedded_content_finder_service.rb +++ b/app/services/embedded_content_finder_service.rb @@ -8,20 +8,20 @@ class EmbeddedContentFinderService def fetch_linked_content_ids(details, locale) content_references = details.values.map { |value| find_content_references(value) - }.flatten.compact.uniq + }.flatten.compact - check_all_references_exist(content_references, locale) + check_all_references_exist(content_references.uniq, locale) content_references.map(&:content_id) end def find_content_references(value) case value when Array - value.map { |item| find_content_references(item) }.flatten + value.map { |item| find_content_references(item) }.flatten.uniq when Hash - value.map { |_, v| find_content_references(v) }.flatten + value.map { |_, v| find_content_references(v) }.flatten.uniq when String - value.scan(EMBED_REGEX).map { |match| ContentReference.new(document_type: match[1], content_id: match[2], embed_code: match[0]) }.uniq + value.scan(EMBED_REGEX).map { |match| ContentReference.new(document_type: match[1], content_id: match[2], embed_code: match[0]) } else [] end diff --git a/app/services/get_host_content_service.rb b/app/services/get_embedded_content_service.rb similarity index 97% rename from app/services/get_host_content_service.rb rename to app/services/get_embedded_content_service.rb index 341bf3300..34cb78471 100644 --- a/app/services/get_host_content_service.rb +++ b/app/services/get_embedded_content_service.rb @@ -1,4 +1,4 @@ -class GetHostContentService +class GetEmbeddedContentService def initialize(target_content_id, order, page, per_page) @target_content_id = target_content_id @order = order diff --git a/spec/integration/embedded_content_spec.rb b/spec/integration/embedded_content_spec.rb index 1946be45e..3cadbf3eb 100644 --- a/spec/integration/embedded_content_spec.rb +++ b/spec/integration/embedded_content_spec.rb @@ -1,4 +1,6 @@ RSpec.describe "Embedded documents" do + include_context "PutContent call" + let!(:publishing_organisation) do create(:live_edition, title: "bar", @@ -43,16 +45,12 @@ context "when an edition embeds a reference to the content block" do it "returns details of the edition and its publishing organisation in the results" do last_edited_at = "2023-01-01T08:00:00.000Z" - host_edition = create(:live_edition, - publishing_app: "whitehall", - last_edited_at: Time.zone.parse(last_edited_at), - details: { - "body" => "
{{embed:email_address:#{content_block.content_id}}}
\n", - }, - links_hash: { - primary_publishing_organisation: [publishing_organisation.content_id], - embed: [content_block.content_id], - }) + host_edition = Timecop.freeze Time.zone.parse(last_edited_at) do + create_live_edition( + body: "{{embed:content_block_email_address:#{content_block.content_id}}}
\n", + primary_publishing_organisation_uuid: publishing_organisation.content_id, + ) + end statistics_cache = create(:statistics_cache, document: host_edition.document, unique_pageviews: 333) @@ -73,6 +71,7 @@ "last_edited_by_editor_id" => host_edition.last_edited_by_editor_id, "last_edited_at" => last_edited_at, "unique_pageviews" => statistics_cache.unique_pageviews, + "instances" => 1, "primary_publishing_organisation" => { "content_id" => publishing_organisation.content_id, "title" => publishing_organisation.title, @@ -81,6 +80,43 @@ }, ) end + + context "when embedded content appears more than once in a field" do + let!(:host_edition) do + create_live_edition( + body: "{{embed:content_block_email_address:#{content_block.content_id}}} {{embed:content_block_email_address:#{content_block.content_id}}}
\n", + ) + end + + it "should return multiple instances" do + get "/v2/content/#{content_block.content_id}/embedded" + response_body = parsed_response + + expect(response_body["content_id"]).to eq(content_block.content_id) + expect(response_body["total"]).to eq(1) + + expect(response_body["results"][0]["instances"]).to eq(2) + end + + context "when the host edition is changed to reference the content once" do + before do + create_live_edition( + content_id: host_edition.content_id, + body: "{{embed:content_block_email_address:#{content_block.content_id}}}
\n", + ) + end + + it "should return only one instance" do + get "/v2/content/#{content_block.content_id}/embedded" + response_body = parsed_response + + expect(response_body["content_id"]).to eq(content_block.content_id) + expect(response_body["total"]).to eq(1) + + expect(response_body["results"][0]["instances"]).to eq(1) + end + end + end end context "pagination" do @@ -275,4 +311,27 @@ def expect_request_to_order_by(order_argument:, expected_results:) end end end + +private + + def create_live_edition(content_id: SecureRandom.uuid, body: "Some body goes here", primary_publishing_organisation_uuid: nil) + payload.merge!( + document_type: "press_release", + schema_name: "news_article", + details: { body: }, + ) + + if primary_publishing_organisation_uuid.present? + payload.merge!( + links: { + primary_publishing_organisation: [primary_publishing_organisation_uuid], + }, + ) + end + + put "/v2/content/#{content_id}", params: payload.to_json + post "/v2/content/#{content_id}/publish", params: {}.to_json + + Document.find_by(content_id:).live + end end diff --git a/spec/presenters/embedded_content_presenter_spec.rb b/spec/presenters/embedded_content_presenter_spec.rb index a28f413ae..1d2ee05cb 100644 --- a/spec/presenters/embedded_content_presenter_spec.rb +++ b/spec/presenters/embedded_content_presenter_spec.rb @@ -19,7 +19,8 @@ unique_pageviews: 123, primary_publishing_organisation_content_id: organisation_edition_id, primary_publishing_organisation_title: "bar", - primary_publishing_organisation_base_path: "/bar")] + primary_publishing_organisation_base_path: "/bar", + instances: 1)] end let(:result) { described_class.present(target_edition_id, host_editions, total, total_pages) } @@ -38,6 +39,7 @@ last_edited_by_editor_id:, last_edited_at:, unique_pageviews: 123, + instances: 1, primary_publishing_organisation: { content_id: organisation_edition_id, title: "bar", diff --git a/spec/queries/get_embedded_content_spec.rb b/spec/queries/get_embedded_content_spec.rb index c812ffa4c..8e47b8aea 100644 --- a/spec/queries/get_embedded_content_spec.rb +++ b/spec/queries/get_embedded_content_spec.rb @@ -37,6 +37,8 @@ end context "when there are live and draft editions that embed the target content" do + let(:target_content_id) { content_block.content_id } + it "returns the live editions" do target_content_id = content_block.content_id published_host_editions = create_list(:live_edition, 2, @@ -84,8 +86,25 @@ expect(results[i].primary_publishing_organisation_title).to eq(organisation.title) expect(results[i].primary_publishing_organisation_base_path).to eq(organisation.base_path) expect(results[i].unique_pageviews).to eq(expected_pageviews[host_edition.document.id]) + expect(results[i].instances).to eq(1) end end + + it "returns instance counts when the content is embedded more than once" do + create(:live_edition, + details: { + body: "{{embed:email_address:#{target_content_id}}}
\n", + }, + links_hash: { + primary_publishing_organisation: [organisation.content_id], + embed: [target_content_id, target_content_id], + }, + publishing_app: "example-app") + + results = described_class.new(target_content_id).call + + expect(results[0].instances).to eq(2) + end end context "when there are superseded editions that embed the target content" do @@ -147,8 +166,7 @@ def expect_sort_call_for(order_field:, order_direction:) expect(ActiveRecord::Base.connection).to receive(:select_all) { |arel_query| expect(arel_query.orders.length).to eq(1) expect(arel_query.orders[0]).to be_a(order_direction == :asc ? Arel::Nodes::Ascending : Arel::Nodes::Descending) - expect(arel_query.orders[0].expr.relation.name).to eq(order_field.relation.name) - expect(arel_query.orders[0].expr.name).to eq(order_field.name) + expect(arel_query.orders[0].expr).to eq(order_field) }.and_return([]) end end diff --git a/spec/services/embedded_content_finder_service_spec.rb b/spec/services/embedded_content_finder_service_spec.rb index 05d624880..8f6e4a304 100644 --- a/spec/services/embedded_content_finder_service_spec.rb +++ b/spec/services/embedded_content_finder_service_spec.rb @@ -32,6 +32,14 @@ expect(links).to eq([editions[0].content_id, editions[1].content_id]) end + it "returns duplicates when there is more than one content reference in the field" do + details = { field_name => "{{embed:#{document_type}:#{editions[0].content_id}}} {{embed:#{document_type}:#{editions[0].content_id}}} {{embed:#{document_type}:#{editions[1].content_id}}}" } + + links = EmbeddedContentFinderService.new.fetch_linked_content_ids(details, Edition::DEFAULT_LOCALE) + + expect(links).to eq([editions[0].content_id, editions[0].content_id, editions[1].content_id]) + end + it "finds content references when #{field_name} is an array of hashes" do details = { field_name => [{ "content" => "{{embed:#{document_type}:#{editions[0].content_id}}} {{embed:#{document_type}:#{editions[1].content_id}}}" }] } @@ -45,6 +53,10 @@ field_name => [ { body: [ + { + content: "some string with a reference: {{embed:#{document_type}:#{editions[0].content_id}}}", + content_type: "text/html", + }, { content: "some string with a reference: {{embed:#{document_type}:#{editions[0].content_id}}}", content_type: "text/govspeak", @@ -55,6 +67,10 @@ }, { body: [ + { + content: "some string with another reference: {{embed:#{document_type}:#{editions[1].content_id}}}", + content_type: "text/html", + }, { content: "some string with another reference: {{embed:#{document_type}:#{editions[1].content_id}}}", content_type: "text/govspeak", diff --git a/spec/services/get_host_content_service_spec.rb b/spec/services/get_embedded_content_service_spec.rb similarity index 99% rename from spec/services/get_host_content_service_spec.rb rename to spec/services/get_embedded_content_service_spec.rb index 6e660edcc..60f7a62fb 100644 --- a/spec/services/get_host_content_service_spec.rb +++ b/spec/services/get_embedded_content_service_spec.rb @@ -1,4 +1,4 @@ -RSpec.describe GetHostContentService do +RSpec.describe GetEmbeddedContentService do describe "#call" do let(:organisation) do edition_params = {