diff --git a/Gemfile b/Gemfile index 1ea1cccf3..32cb0279d 100644 --- a/Gemfile +++ b/Gemfile @@ -5,6 +5,7 @@ gem "rails", "7.2.2" gem "aws-sdk-s3" gem "bootsnap", require: false gem "bunny" +gem "content_block_tools" gem "dalli" gem "fuzzy_match" gem "gds-api-adapters" diff --git a/Gemfile.lock b/Gemfile.lock index 24dcc708f..37513c62f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -119,6 +119,8 @@ GEM climate_control (1.2.0) concurrent-ruby (1.3.4) connection_pool (2.4.1) + content_block_tools (0.2.0) + actionview (= 7.2.2) crack (1.0.0) bigdecimal rexml @@ -868,6 +870,7 @@ DEPENDENCIES bunny byebug climate_control + content_block_tools dalli database_cleaner factory_bot_rails diff --git a/app/presenters/content_embed_presenter.rb b/app/presenters/content_embed_presenter.rb index 7774bb609..f8ee89fb2 100644 --- a/app/presenters/content_embed_presenter.rb +++ b/app/presenters/content_embed_presenter.rb @@ -64,14 +64,13 @@ def render_embedded_editions(content) content end - # This is a temporary solution to get email address content blocks working - # while we agree on a long-term approach that works for everything. def get_content_for_edition(edition) - if edition.document_type == "content_block_email_address" - edition.details[:email_address] - else - edition.title - end + ContentBlockTools::ContentBlock.new( + document_type: edition.document_type, + content_id: edition.document.content_id, + title: edition.title, + details: edition.details, + ).render end end end diff --git a/app/services/embedded_content_finder_service.rb b/app/services/embedded_content_finder_service.rb index f56a91643..1c334b498 100644 --- a/app/services/embedded_content_finder_service.rb +++ b/app/services/embedded_content_finder_service.rb @@ -1,10 +1,4 @@ class EmbeddedContentFinderService - ContentReference = Data.define(:document_type, :content_id, :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}}})/ - def fetch_linked_content_ids(details, locale) content_references = details.values.map { |value| find_content_references(value) @@ -21,7 +15,7 @@ def find_content_references(value) when Hash 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]) } + ContentBlockTools::ContentBlockReference.find_all_in_document(value) else [] end diff --git a/docs/arch/adr-007-move-content-block-rendering-logic-to-external-gem.md b/docs/arch/adr-007-move-content-block-rendering-logic-to-external-gem.md new file mode 100644 index 000000000..16d9cda54 --- /dev/null +++ b/docs/arch/adr-007-move-content-block-rendering-logic-to-external-gem.md @@ -0,0 +1,92 @@ +# Decision Record: Move Content Block rendering logic to external gem + +## Context + +Content blocks within Publishing API are a relatively new concept, that mainly sits with the Content +Modelling Team. Whilst still in development, the basic concept is that a publishing user will be able +to embed a "block" of reusable content within a document, and it will then be rendered within the +document on publication. When the content of the block changes, all pages that use the block will +change to show the latest information. + +When a piece of content that uses a content block (or content blocks) is sent to the publishing API, +we first scan over each field within the content to find embed codes, which currently look like this: + +```markdown +{{embed:$embed_type:$uuid}} +``` + +Where `$embed_type` is the type of content to be embedded and `$uuid` is the content ID of the content +block. + +If there are any embed codes found, these are added as "links" to the content within Publishing API +([See here for more information about links and link expansion](https://github.com/alphagov/publishing-api/blob/main/docs/link-expansion.md)). + +When the piece of content is sent to the Content Store (either draft or live), we scan the content +again for embed codes, and replace the embed code with the actual content block content. + +At the moment, this is fairly simple, contacts are replaced with the Title (as contacts work is +still a work in progress) and email addresses are replaced with the email. However, as this work +progresses, it is envisioned that embedded content rendering will get significantly more complex. + +Additionally, there are places where we need access to the embed logic outside of Publishing API: + +### Whitehall Live Preview + +At the moment, when a user clicks on "Preview" within Whitehall, it takes the content from the +body field, and uses the Govspeak gem (and some additional Whitehall-specific logic) to +render the Govspeak. + +Currently, Whitehall or Govspeak does not understand the embed code, so embed codes are left +untouched: + +![Whitehall editor body field showing link to preview button](./images/live-preview-1.png) + +![Live preview showing embed code left inplace](./images/live-preview-2.png) + +We therefore need a way to handle the embed codes within live preview + +### "In-app" preview within Content Block Manager + +When editors are making changes to content blocks within the Content Block Manager, it's important +for users to be able to see the changes they make within the context of the pages that use the +blocks. + +We can't use the existing draft stack because: + +a) We want to show the content block changes in the context of the live documents; and +b) Publishing content block changes to the draft stack could cause confusion when a user +is previewing the changes to their own content and seeing unpublished changes to a content block + +With this in mind, we are proposing that we fetch the HTML of live content and then run a +find/replace against the HTML of a content block with the HTML of the current draft, then rendering +this HTML within our app. + +Both of these solutions require each application having knowledge of how a content block will be +rendered. + +Initially we considered having an API endpoint within Publishing API to "preview" a content block. +However, this was discounted because: + +a) We run the risk of overloading the Publishing API with extra API endpoints +b) There is not much rendering logic in Publishing API itself, and logic will potentially get more +complex as content blocks evolve +c) If there are a number of content blocks within a page, this may cause a performance issues when +making multiple calls to an API + +## Decisions + +We are proposing moving the Content Block rendering and parsing logic to a new [content_block_tools gem](https://github.com/alphagov/content_block_tools) + +This will give a single source of truth for how content blocks are rendered, which can be easily pulled +into other applications. + +## Consequences + +Once the work to move the logic into the gem is approved, we will then need to add content_block_tools as +a dependency in Publishing API to ensure the gem is the only source of truth. + +We will then be able to carry out the two pieces of work as detailed above. They are both contained within +the Whitehall application, which already has govuk_publishing_components as a dependency. + +One possible downside is we will need to release a new version of a gem and ensure it is used in each application +when a change occurs, but as we don't expect to be making changes regularly, we believe this risk is minimal. diff --git a/docs/arch/images/live-preview-1.png b/docs/arch/images/live-preview-1.png new file mode 100644 index 000000000..825781bc0 Binary files /dev/null and b/docs/arch/images/live-preview-1.png differ diff --git a/docs/arch/images/live-preview-2.png b/docs/arch/images/live-preview-2.png new file mode 100644 index 000000000..f7650715d Binary files /dev/null and b/docs/arch/images/live-preview-2.png differ diff --git a/spec/integration/put_content/content_with_embedded_content_spec.rb b/spec/integration/put_content/content_with_embedded_content_spec.rb index 0a77cf0d6..58e0c9fac 100644 --- a/spec/integration/put_content/content_with_embedded_content_spec.rb +++ b/spec/integration/put_content/content_with_embedded_content_spec.rb @@ -22,7 +22,7 @@ it "should send transformed content to the content store" do put "/v2/content/#{content_id}", params: payload.to_json - expect_content_store_to_have_received_details_including({ "body" => "#{first_contact.title} #{second_contact.title}" }) + expect_content_store_to_have_received_details_including({ "body" => "#{presented_details_for(first_contact)} #{presented_details_for(second_contact)}" }) end end @@ -52,7 +52,7 @@ 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" => presented_details_for(first_contact) }) end end @@ -128,11 +128,11 @@ "body" => [ { "content_type" => "text/govspeak", - "content" => first_contact.title, + "content" => presented_details_for(first_contact), }, { "content_type" => "text/html", - "content" => "

#{first_contact.title}

", + "content" => "

#{presented_details_for(first_contact)}

", }, ], }, @@ -142,11 +142,11 @@ "body" => [ { "content_type" => "text/govspeak", - "content" => second_contact.title, + "content" => presented_details_for(second_contact), }, { "content_type" => "text/html", - "content" => "

#{second_contact.title}

", + "content" => "

#{presented_details_for(second_contact)}

", }, ], }, @@ -176,32 +176,7 @@ it "should send transformed content to the content store" do put "/v2/content/#{content_id}", params: payload.to_json - expect_content_store_to_have_received_details_including({ "body" => array_including({ "content_type" => "text/govspeak", "content" => "#{first_contact.title} #{second_contact.title}" }) }) - end - end - - context "with an embedded email address" do - let(:first_email_address) { create(:edition, state: "published", content_store: "live", document_type: "content_block_email_address", details: { email_address: "foo@example.com" }) } - let(:second_email_address) { create(:edition, state: "published", content_store: "live", document_type: "content_block_email_address", details: { email_address: "bar@example.com" }) } - let(:document) { create(:document, content_id:) } - - 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}}}" }) - end - - it "should create links" do - expect { - put "/v2/content/#{content_id}", params: payload.to_json - }.to change(Link, :count).by(2) - - expect(Link.find_by(target_content_id: first_email_address.content_id)).not_to be_nil - expect(Link.find_by(target_content_id: second_email_address.content_id)).not_to be_nil - end - - it "should send transformed content to the content store" do - put "/v2/content/#{content_id}", params: payload.to_json - - expect_content_store_to_have_received_details_including({ "body" => "#{first_email_address.details[:email_address]} #{second_email_address.details[:email_address]}" }) + expect_content_store_to_have_received_details_including({ "body" => array_including({ "content_type" => "text/govspeak", "content" => "#{presented_details_for(first_contact)} #{presented_details_for(second_contact)}" }) }) end end @@ -226,7 +201,7 @@ it "should send transformed content to the content store" do put "/v2/content/#{content_id}", params: payload.to_json - expect_content_store_to_have_received_details_including({ "body" => "#{email_address.details[:email_address]} #{contact.title}" }) + expect_content_store_to_have_received_details_including({ "body" => "#{presented_details_for(email_address)} #{presented_details_for(contact)}" }) end end diff --git a/spec/presenters/content_embed_presenter_spec.rb b/spec/presenters/content_embed_presenter_spec.rb index 84e2160e3..0c35765b8 100644 --- a/spec/presenters/content_embed_presenter_spec.rb +++ b/spec/presenters/content_embed_presenter_spec.rb @@ -16,7 +16,7 @@ end let(:details) { {} } - before do + let!(:embedded_edition) do embedded_document = create(:document, content_id: embedded_content_id) create( :edition, @@ -29,12 +29,24 @@ end describe "#render_embedded_content" do + let(:expected_value) { "VALUE" } + let(:stub_block) { double(ContentBlockTools::ContentBlock, render: expected_value) } + + before do + expect(ContentBlockTools::ContentBlock).to receive(:new).with( + document_type: embedded_edition.document_type, + content_id: embedded_edition.document.content_id, + title: embedded_edition.title, + details: embedded_edition.details, + ).at_least(:once).and_return(stub_block) + end + context "when body is a string" do let(:details) { { body: "some string with a reference: {{embed:contact:#{embedded_content_id}}}" } } it "returns embedded content references with values from their editions" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ - body: "some string with a reference: VALUE", + body: "some string with a reference: #{expected_value}", }) end end @@ -50,8 +62,8 @@ it "returns embedded content references with values from their editions" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ body: [ - { content_type: "text/govspeak", content: "some string with a reference: VALUE" }, - { content_type: "text/html", content: "some string with a reference: VALUE" }, + { content_type: "text/govspeak", content: "some string with a reference: #{expected_value}" }, + { content_type: "text/html", content: "some string with a reference: #{expected_value}" }, ], }) end @@ -64,7 +76,7 @@ it "returns embedded content references with values from their editions" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ - body: { title: "some string with a reference: VALUE" }, + body: { title: "some string with a reference: #{expected_value}" }, }) end end @@ -91,7 +103,7 @@ parts: [ body: [ { - content: "some string with a reference: VALUE", + content: "some string with a reference: #{expected_value}", content_type: "text/govspeak", }, ], @@ -106,7 +118,7 @@ context "when the embedded content is available in multiple locales" do let(:details) { { body: "some string with a reference: {{embed:contact:#{embedded_content_id}}}" } } - before do + let!(:welsh_edition) do embedded_document = create(:document, content_id: embedded_content_id, locale: "cy") create( :edition, @@ -121,17 +133,18 @@ context "when the document is in the default language" do it "returns embedded content references with values from the same language" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ - body: "some string with a reference: VALUE", + body: "some string with a reference: #{expected_value}", }) end end context "when the document is in an available locale" do let(:document) { create(:document, locale: "cy") } + let(:embedded_edition) { welsh_edition } it "returns embedded content references with values from the same language" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ - body: "some string with a reference: WELSH", + body: "some string with a reference: #{expected_value}", }) end end @@ -141,46 +154,16 @@ it "returns embedded content references with values from the default language" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ - body: "some string with a reference: VALUE", + body: "some string with a reference: #{expected_value}", }) end end end - context "when the document is an email address" do - let(:embedded_document) { create(:document) } - let(:links_hash) do - { - embed: [embedded_document.content_id], - } - end - - before do - create( - :edition, - document: embedded_document, - state: "published", - content_store: "live", - document_type: "content_block_email_address", - details: { - email_address: "foo@example.com", - }, - ) - end - - let(:details) { { body: "some string with a reference: {{embed:content_block_email_address:#{embedded_document.content_id}}}" } } - - it "returns an email address" do - expect(described_class.new(edition).render_embedded_content(details)).to eq({ - body: "some string with a reference: foo@example.com", - }) - end - end - context "when multiple documents are embedded in different parts of the document" do let(:other_embedded_content_id) { SecureRandom.uuid } - before do + let!(:other_embedded_edition) do embedded_document = create(:document, content_id: other_embedded_content_id) create( :edition, @@ -192,6 +175,18 @@ ) end + let(:other_expected_value) { "VALUE2" } + let(:other_stub_block) { double(ContentBlockTools::ContentBlock, render: other_expected_value) } + + before do + expect(ContentBlockTools::ContentBlock).to receive(:new).with( + document_type: other_embedded_edition.document_type, + content_id: other_embedded_edition.document.content_id, + title: other_embedded_edition.title, + details: other_embedded_edition.details, + ).at_least(:once).and_return(other_stub_block) + end + let(:links_hash) do { embed: [embedded_content_id, other_embedded_content_id], @@ -207,8 +202,8 @@ it "returns embedded content references with values from their editions" do expect(described_class.new(edition).render_embedded_content(details)).to eq({ - title: "title string with reference: VALUE2", - body: "some string with a reference: VALUE", + title: "title string with reference: #{other_expected_value}", + body: "some string with a reference: #{expected_value}", }) end end diff --git a/spec/presenters/details_presenter_spec.rb b/spec/presenters/details_presenter_spec.rb index 1a4adfe3f..d687dfdf9 100644 --- a/spec/presenters/details_presenter_spec.rb +++ b/spec/presenters/details_presenter_spec.rb @@ -197,7 +197,7 @@ let(:edition_details) { edition.details } let(:expected_details) do { - field_name => embeddable_content.title, + field_name => presented_details_for(embeddable_content), }.symbolize_keys end @@ -219,8 +219,8 @@ let(:expected_details) do { field_name => [ - { content_type: "text/html", content: embeddable_content.title }, - { content_type: "text/govspeak", content: embeddable_content.title }, + { content_type: "text/html", content: presented_details_for(embeddable_content) }, + { content_type: "text/govspeak", content: presented_details_for(embeddable_content) }, ], }.symbolize_keys end diff --git a/spec/presenters/queries/expanded_link_set_spec.rb b/spec/presenters/queries/expanded_link_set_spec.rb index 577d5274f..acb0fb4d3 100644 --- a/spec/presenters/queries/expanded_link_set_spec.rb +++ b/spec/presenters/queries/expanded_link_set_spec.rb @@ -110,11 +110,11 @@ expect(c[:details][:body]).to match([ { content_type: "text/govspeak", - content: "Some contact", + content: presented_details_for(contact), }, { content_type: "text/html", - content: "

Some contact

\n", + content: "

#{presented_details_for(contact)}

\n", }, ]) end diff --git a/spec/services/embedded_content_finder_service_spec.rb b/spec/services/embedded_content_finder_service_spec.rb index 8f6e4a304..2a29910f9 100644 --- a/spec/services/embedded_content_finder_service_spec.rb +++ b/spec/services/embedded_content_finder_service_spec.rb @@ -111,7 +111,7 @@ RSpec.describe EmbeddedContentFinderService do describe ".fetch_linked_content_ids" do - EmbeddedContentFinderService::SUPPORTED_DOCUMENT_TYPES.each do |document_type| + ContentBlockTools::ContentBlockReference::SUPPORTED_DOCUMENT_TYPES.each do |document_type| include_examples "finds references", document_type end diff --git a/spec/support/content_block_helpers.rb b/spec/support/content_block_helpers.rb new file mode 100644 index 000000000..2c653fbaa --- /dev/null +++ b/spec/support/content_block_helpers.rb @@ -0,0 +1,14 @@ +module ContentBlockHelpers + def presented_details_for(edition) + ContentBlockTools::ContentBlock.new( + document_type: edition.document_type, + content_id: edition.document.content_id, + title: edition.title, + details: edition.details, + ).render + end +end + +RSpec.configure do |c| + c.include ContentBlockHelpers +end