Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use content id aliases over content ids for embedded content #2916

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Use content id aliases over content ids for embedded content #2916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7468dd1
Find embedded content by alias
Gweaton Oct 3, 2024
08bf0ad
Update ContentEmbedPresenter to use aliases
Gweaton Oct 7, 2024
f6eb88a
Update DetailsPresenterSpec
Gweaton Oct 7, 2024
e6c3481
Update ExpandedLinkSet spec
Gweaton Oct 7, 2024
7979c2e
Remove unused test variable
Gweaton Oct 7, 2024
71fd46b
Fix integration tests for putting embedded content
Gweaton Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions app/presenters/content_embed_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,12 @@ def embedded_editions
state_fallback_order: %w[published],
)

Edition.where(id: embedded_edition_ids).index_by(&:content_id)
Edition
.joins("LEFT JOIN documents on documents.id = editions.document_id")
.joins("LEFT JOIN content_id_aliases on content_id_aliases.content_id = documents.content_id")
.where(id: embedded_edition_ids)
.select("editions.title, editions.details, editions.document_type, content_id_aliases.name")
.index_by(&:name)
end
end

Expand All @@ -54,7 +59,7 @@ def render_embedded_editions(content)

embedded_content_references.each do |content_reference|
embed_code = content_reference.embed_code
embedded_edition = embedded_editions[content_reference.content_id]
embedded_edition = embedded_editions[content_reference.alias]
content = content.gsub(
embed_code,
get_content_for_edition(embedded_edition),
Expand Down
43 changes: 29 additions & 14 deletions app/services/embedded_content_finder_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,15 @@
class EmbeddedContentFinderService
ContentReference = Data.define(:document_type, :content_id, :embed_code)
ContentReference = Data.define(:document_type, :alias, :embed_code)

SUPPORTED_DOCUMENT_TYPES = %w[contact content_block_email_address].freeze
UUID_REGEX = /([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})/
EMBED_REGEX = /({{embed:(#{SUPPORTED_DOCUMENT_TYPES.join('|')}):#{UUID_REGEX}}})/
EMBED_REGEX = /({{embed:(#{SUPPORTED_DOCUMENT_TYPES.join('|')}):(.*?)}})/

def fetch_linked_content_ids(details, locale)
content_references = details.values.map { |value|
find_content_references(value)
}.flatten.compact.uniq

check_all_references_exist(content_references, locale)
content_references.map(&:content_id)
get_content_ids_from_content_references(content_references, locale)
end

def find_content_references(value)
Expand All @@ -21,31 +19,48 @@ def find_content_references(value)
when Hash
value.map { |_, v| find_content_references(v) }.flatten
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], alias: match[2], embed_code: match[0]) }.uniq
else
[]
end
end

private

def check_all_references_exist(content_references, locale)
found_editions = live_editions(content_references, locale)
if found_editions.count != content_references.count
not_found_content_ids = content_references.map(&:content_id) - found_editions.map(&:content_id)
def get_content_ids_from_content_references(content_references, locale)
embedded_aliases = content_references.map(&:alias)
content_id_aliases = ContentIdAlias.where(name: embedded_aliases)

if content_id_aliases.count != content_references.count
not_found_aliases = embedded_aliases - content_id_aliases.map(&:name)
raise CommandError.new(
code: 422,
message: "Could not find any live editions in locale #{locale} for: #{not_found_content_ids.join(', ')}, ",
message: "Could not find any live editions in locale #{locale} for: #{not_found_aliases.join(', ')}",
)
end

embedded_content_ids = content_id_aliases.map(&:content_id)
document_types = content_references.map(&:document_type)

found_editions = live_editions(embedded_content_ids, document_types, locale)

if found_editions.count != content_id_aliases.count
not_found_aliases = content_id_aliases.reject { |ecr| found_editions.map(&:content_id).include?(ecr) }.map(&:name)
raise CommandError.new(
code: 422,
message: "Could not find any live editions in locale #{locale} for: #{not_found_aliases.join(', ')}",
)
end

embedded_content_ids
end

def live_editions(content_references, locale)
def live_editions(content_ids, document_types, locale)
Edition.with_document.where(
state: "published",
content_store: "live",
document_type: content_references.map(&:document_type),
documents: { content_id: content_references.map(&:content_id), locale: },
document_type: document_types,
documents: { content_id: content_ids, locale: },
)
end
end
117 changes: 96 additions & 21 deletions spec/integration/put_content/content_with_embedded_content_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,21 @@
let(:first_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:second_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:first_contact_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: first_contact.document.content_id)
end
let(:second_contact_content_id_alias) do
create(:content_id_alias, name: "alias-2", content_id: second_contact.document.content_id)
end

before do
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:contact:#{first_contact.document.content_id}}} {{embed:contact:#{second_contact.document.content_id}}}" })
payload.merge!(
document_type: "press_release",
schema_name: "news_article",
details: {
body: "{{embed:contact:#{first_contact_content_id_alias.name}}} {{embed:contact:#{second_contact_content_id_alias.name}}}",
},
)
end

it "should create links" do
Expand All @@ -27,16 +39,18 @@
end

context "when embedded content is in a details field other than body" do
let(:first_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:second_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: contact.document.content_id)
end

let(:payload_for_multiple_field_embeds) do
payload.merge!(
document_type: "transaction",
schema_name: "transaction",
details: {
downtime_message: "{{embed:contact:#{first_contact.document.content_id}}}",
downtime_message: "{{embed:contact:#{content_id_alias.name}}}",
},
)
end
Expand All @@ -46,20 +60,27 @@
put "/v2/content/#{content_id}", params: payload_for_multiple_field_embeds.to_json
}.to change(Link, :count).by(1)

expect(Link.find_by(target_content_id: first_contact.content_id)).not_to be_nil
expect(Link.find_by(target_content_id: contact.content_id)).not_to be_nil
end

it "should send transformed content to the content store" do
put "/v2/content/#{content_id}", params: payload_for_multiple_field_embeds.to_json

expect_content_store_to_have_received_details_including({ "downtime_message" => first_contact.title.to_s })
expect_content_store_to_have_received_details_including({ "downtime_message" => contact.title.to_s })
end
end

context "with multipart content" do
let(:first_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:second_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:first_contact_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: first_contact.document.content_id)
end
let(:second_contact_content_id_alias) do
create(:content_id_alias, name: "alias-2", content_id: second_contact.document.content_id)
end

let(:details) do
{
country: {
Expand All @@ -78,11 +99,11 @@
body: [
{
"content_type": "text/govspeak",
"content": "{{embed:contact:#{first_contact.document.content_id}}}",
"content": "{{embed:contact:#{first_contact_content_id_alias.name}}}",
},
{
"content_type": "text/html",
"content": "<p>{{embed:contact:#{first_contact.document.content_id}}}</p>",
"content": "<p>{{embed:contact:#{first_contact_content_id_alias.name}}}</p>",
},
],
},
Expand All @@ -92,11 +113,11 @@
body: [
{
"content_type": "text/govspeak",
"content": "{{embed:contact:#{second_contact.document.content_id}}}",
"content": "{{embed:contact:#{second_contact_content_id_alias.name}}}",
},
{
"content_type": "text/html",
"content": "<p>{{embed:contact:#{second_contact.document.content_id}}}</p>",
"content": "<p>{{embed:contact:#{second_contact_content_id_alias.name}}}</p>",
},
],
},
Expand Down Expand Up @@ -159,9 +180,24 @@
let(:first_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:second_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:first_contact_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: first_contact.document.content_id)
end
let(:second_contact_content_id_alias) do
create(:content_id_alias, name: "alias-2", content_id: second_contact.document.content_id)
end

before do
payload.merge!(document_type: "person", schema_name: "person", details: { body: [{ content_type: "text/govspeak", content: "{{embed:contact:#{first_contact.document.content_id}}} {{embed:contact:#{second_contact.document.content_id}}}" }] })
payload.merge!(
document_type: "person",
schema_name: "person",
details: {
body: [{
content_type: "text/govspeak",
content: "{{embed:contact:#{first_contact_content_id_alias.name}}} {{embed:contact:#{second_contact_content_id_alias.name}}}",
}],
},
)
end

it "should create links" do
Expand All @@ -184,9 +220,21 @@
let(:first_email_address) { create(:edition, state: "published", content_store: "live", document_type: "content_block_email_address", details: { email_address: "[email protected]" }) }
let(:second_email_address) { create(:edition, state: "published", content_store: "live", document_type: "content_block_email_address", details: { email_address: "[email protected]" }) }
let(:document) { create(:document, content_id:) }
let(:first_email_address_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: first_email_address.document.content_id)
end
let(:second_email_address_content_id_alias) do
create(:content_id_alias, name: "alias-2", content_id: second_email_address.document.content_id)
end

before do
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:content_block_email_address:#{first_email_address.document.content_id}}} {{embed:content_block_email_address:#{second_email_address.document.content_id}}}" })
payload.merge!(
document_type: "press_release",
schema_name: "news_article",
details: {
body: "{{embed:content_block_email_address:#{first_email_address_content_id_alias.name}}} {{embed:content_block_email_address:#{second_email_address_content_id_alias.name}}}",
},
)
end

it "should create links" do
Expand All @@ -209,9 +257,21 @@
let(:email_address) { create(:edition, state: "published", content_store: "live", document_type: "content_block_email_address", details: { email_address: "[email protected]" }) }
let(:contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:email_address_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: email_address.document.content_id)
end
let(:contact_content_id_alias) do
create(:content_id_alias, name: "alias-2", content_id: contact.document.content_id)
end

before do
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:content_block_email_address:#{email_address.document.content_id}}} {{embed:contact:#{contact.document.content_id}}}" })
payload.merge!(
document_type: "press_release",
schema_name: "news_article",
details: {
body: "{{embed:content_block_email_address:#{email_address_content_id_alias.name}}} {{embed:contact:#{contact_content_id_alias.name}}}",
},
)
end

it "should create links" do
Expand Down Expand Up @@ -258,6 +318,12 @@
let(:first_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:second_contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:first_contact_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: first_contact.document.content_id)
end
let(:second_contact_content_id_alias) do
create(:content_id_alias, name: "alias-2", content_id: second_contact.document.content_id)
end
let(:edition) { create(:edition, document:) }

before do
Expand All @@ -267,7 +333,7 @@
target_content_id: first_contact.content_id,
position: 0,
})
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:contact:#{second_contact.document.content_id}}}" })
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:contact:#{second_contact_content_id_alias.name}}}" })
end

it "should replace the embed link" do
Expand All @@ -282,35 +348,44 @@

context "with embedded content that does not exist" do
let(:document) { create(:document, content_id:) }
let(:fake_content_id) { SecureRandom.uuid }
let(:fake_friendly_id) { "non-existent" }

before do
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:contact:#{fake_content_id}}}" })
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:contact:#{fake_friendly_id}}}" })
end

it "should return a 422 error" do
put "/v2/content/#{content_id}", params: payload.to_json

expect(response).to be_unprocessable
expect(response.body).to match(/Could not find any live editions in locale en for: #{fake_content_id}/)
expect(response.body).to match(/Could not find any live editions in locale en for: #{fake_friendly_id}/)
end
end

context "with a mixture of embedded content that does and does not exist" do
let(:contact) { create(:edition, state: "published", content_store: "live", document_type: "contact") }
let(:document) { create(:document, content_id:) }
let(:first_fake_content_id) { SecureRandom.uuid }
let(:second_fake_content_id) { SecureRandom.uuid }
let(:contact_content_id_alias) do
create(:content_id_alias, name: "alias-1", content_id: contact.document.content_id)
end
let(:first_fake_friendly_id) { "non-existent-1" }
let(:second_fake_friendly_id) { "non-existent-2" }

before do
payload.merge!(document_type: "press_release", schema_name: "news_article", details: { body: "{{embed:contact:#{contact.document.content_id}}} {{embed:contact:#{first_fake_content_id}}} {{embed:contact:#{second_fake_content_id}}}" })
payload.merge!(
document_type: "press_release",
schema_name: "news_article",
details: {
body: "{{embed:contact:#{contact_content_id_alias.name}}} {{embed:contact:#{first_fake_friendly_id}}} {{embed:contact:#{second_fake_friendly_id}}}",
},
)
end

it "should return a 422 error" do
put "/v2/content/#{content_id}", params: payload.to_json

expect(response).to be_unprocessable
expect(response.body).to match(/Could not find any live editions in locale en for: #{first_fake_content_id}, #{second_fake_content_id}/)
expect(response.body).to match(/Could not find any live editions in locale en for: #{first_fake_friendly_id}, #{second_fake_friendly_id}/)
end
end

Expand Down
Loading
Loading