From 1a69889a48106734f16f7e78b7f15f80784e6c2a Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 6 Jan 2025 14:08:40 +0000 Subject: [PATCH 1/7] Bump `gds-api-adapters` --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 506e14d32bf..d8f2963df30 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -248,7 +248,7 @@ GEM et-orbi (~> 1, >= 1.2.11) raabro (~> 1.4) fuzzy_match (2.1.0) - gds-api-adapters (98.1.0) + gds-api-adapters (98.2.0) addressable link_header null_logger From 578aab7cfe53b4c420ae02f471d753550404fd93 Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 6 Jan 2025 16:20:15 +0000 Subject: [PATCH 2/7] Add Editor and Organisation classes for Host items This will record assoiated editors and organisations for a given content item --- .../content_block_manager/host_content_item/editor.rb | 6 ++++++ .../host_content_item/editor/organisation.rb | 8 ++++++++ 2 files changed, 14 insertions(+) create mode 100644 lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb create mode 100644 lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb new file mode 100644 index 00000000000..cb432ea3c3c --- /dev/null +++ b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb @@ -0,0 +1,6 @@ +module ContentBlockManager + class HostContentItem + class Editor < Data.define(:uid, :name, :email, :organisation) + end + end +end diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb new file mode 100644 index 00000000000..410cea2769e --- /dev/null +++ b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb @@ -0,0 +1,8 @@ +module ContentBlockManager + class HostContentItem + class Editor + class Organisation < Data.define(:content_id, :name, :slug) + end + end + end +end From fedc0f0cfac84c161978beda13488a449317e708 Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 6 Jan 2025 16:22:10 +0000 Subject: [PATCH 3/7] Add a method to fetch editors given a UUID array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This calls the Signon API’s `get_users` endpoint and casts the results to an array of `Editor` objects. If an organisation is present, it returns an associated organisation too. --- .../host_content_item/editor.rb | 10 ++++ .../host_content_item/editor/organisation.rb | 9 +++ .../models/host_content_item/editor_test.rb | 57 +++++++++++++++++++ lib/services.rb | 7 +++ 4 files changed, 83 insertions(+) create mode 100644 lib/engines/content_block_manager/test/unit/app/models/host_content_item/editor_test.rb diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb index cb432ea3c3c..87ed2c6df5a 100644 --- a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb +++ b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor.rb @@ -1,6 +1,16 @@ module ContentBlockManager class HostContentItem class Editor < Data.define(:uid, :name, :email, :organisation) + def self.with_uuids(uuids) + Services.signon_api_client.get_users(uuids:).map do |user| + new( + uid: user["uid"], + name: user["name"], + email: user["email"], + organisation: ContentBlockManager::HostContentItem::Editor::Organisation.from_user_hash(user), + ) + end + end end end end diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb index 410cea2769e..34d74c4781e 100644 --- a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb +++ b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item/editor/organisation.rb @@ -2,6 +2,15 @@ module ContentBlockManager class HostContentItem class Editor class Organisation < Data.define(:content_id, :name, :slug) + def self.from_user_hash(user) + if user["organisation"].present? + new( + content_id: user["organisation"]["content_id"], + name: user["organisation"]["name"], + slug: user["organisation"]["slug"], + ) + end + end end end end diff --git a/lib/engines/content_block_manager/test/unit/app/models/host_content_item/editor_test.rb b/lib/engines/content_block_manager/test/unit/app/models/host_content_item/editor_test.rb new file mode 100644 index 00000000000..382588d02ae --- /dev/null +++ b/lib/engines/content_block_manager/test/unit/app/models/host_content_item/editor_test.rb @@ -0,0 +1,57 @@ +require "test_helper" + +class ContentBlockManager::HostContentItem::EditorTest < ActiveSupport::TestCase + extend Minitest::Spec::DSL + + describe ".with_uuids" do + let(:signon_api_stub) { stub } + + before do + Services.expects(:signon_api_client).returns(signon_api_stub) + end + + it "returns an empty array when no UUIDs are provided" do + signon_api_stub.expects(:get_users).with(uuids: []).returns([]) + + result = ContentBlockManager::HostContentItem::Editor.with_uuids([]) + + assert_equal [], result + end + + it "fetches users for a given list of UUIDs" do + uuids = [SecureRandom.uuid, SecureRandom.uuid] + api_response = [ + { + "uid" => uuids[0], + "name" => "Someone", + "email" => "someone@example.com", + }, + { + "uid" => uuids[1], + "name" => "Someone else", + "email" => "someoneelse@example.com", + "organisation" => { + "content_id" => SecureRandom.uuid, + "name" => "Organisation", + "slug" => "organisation", + }, + }, + ] + signon_api_stub.expects(:get_users).with(uuids:).returns(api_response) + + result = ContentBlockManager::HostContentItem::Editor.with_uuids(uuids) + + assert_equal result[0].uid, api_response[0]["uid"] + assert_equal result[0].name, api_response[0]["name"] + assert_equal result[0].email, api_response[0]["email"] + assert_nil result[0].organisation + + assert_equal result[1].uid, api_response[1]["uid"] + assert_equal result[1].name, api_response[1]["name"] + assert_equal result[1].email, api_response[1]["email"] + assert_equal result[1].organisation.content_id, api_response[1]["organisation"]["content_id"] + assert_equal result[1].organisation.name, api_response[1]["organisation"]["name"] + assert_equal result[1].organisation.slug, api_response[1]["organisation"]["slug"] + end + end +end diff --git a/lib/services.rb b/lib/services.rb index e66ab886ca8..36cd583fd35 100644 --- a/lib/services.rb +++ b/lib/services.rb @@ -33,4 +33,11 @@ def self.search_api_client bearer_token: ENV["RUMMAGER_BEARER_TOKEN"] || "example", ) end + + def self.signon_api_client + GdsApi::SignonApi.new( + Plek.find("signon", external: true), + bearer_token: ENV.fetch("SIGNON_API_BEARER_TOKEN", "example"), + ) + end end From afcc9e4c085540588573e73c336cea95878406a5 Mon Sep 17 00:00:00 2001 From: pezholio Date: Tue, 7 Jan 2025 09:06:33 +0000 Subject: [PATCH 4/7] Use Mocha stubs for GetHostContentItems tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I find these a bit more readable, especially as we’ll be introducing more stubs in the next commit. --- .../services/get_host_content_items_test.rb | 84 ++++++++----------- 1 file changed, 36 insertions(+), 48 deletions(-) diff --git a/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb b/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb index 0978cba1533..8466c32127c 100644 --- a/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb @@ -48,54 +48,45 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase stub("http_response", code: 200, body: response_body.to_json), ) end - let(:publishing_api_mock) { Minitest::Mock.new } + let(:publishing_api_mock) { stub("GdsApi::PublishingApi") } + let(:document) { mock("content_block_document", content_id: target_content_id) } - it "calls the Publishing API for the content which embeds the target" do - publishing_api_mock.expect :get_host_content_for_content_id, fake_api_response, [target_content_id, { order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER }] - - Services.stub :publishing_api, publishing_api_mock do - document = mock("content_block_document", content_id: target_content_id) + before do + Services.expects(:publishing_api).returns(publishing_api_mock) + end - described_class.by_embedded_document( - content_block_document: document, - ) + it "calls the Publishing API for the content which embeds the target" do + publishing_api_mock.expects(:get_host_content_for_content_id) + .with(target_content_id, { order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER }) + .returns(fake_api_response) - publishing_api_mock.verify - end + described_class.by_embedded_document(content_block_document: document) end it "supports pagination" do - publishing_api_mock.expect :get_host_content_for_content_id, fake_api_response, [target_content_id, { page: 1, order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER }] - - Services.stub :publishing_api, publishing_api_mock do - document = mock("content_block_document", content_id: target_content_id) + publishing_api_mock.expects(:get_host_content_for_content_id) + .with(target_content_id, { page: 1, order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER }) + .returns(fake_api_response) - described_class.by_embedded_document( - content_block_document: document, - page: 1, - ) - - publishing_api_mock.verify - end + described_class.by_embedded_document( + content_block_document: document, + page: 1, + ) end it "supports sorting" do - publishing_api_mock.expect :get_host_content_for_content_id, fake_api_response, [target_content_id, { order: "-abc" }] - - Services.stub :publishing_api, publishing_api_mock do - document = mock("content_block_document", content_id: target_content_id) + publishing_api_mock.expects(:get_host_content_for_content_id) + .with(target_content_id, { order: "-abc" }) + .returns(fake_api_response) - described_class.by_embedded_document( - content_block_document: document, - order: "-abc", - ) - - publishing_api_mock.verify - end + described_class.by_embedded_document( + content_block_document: document, + order: "-abc", + ) end it "returns GetHostContentItems" do - document = mock("content_block_document", content_id: target_content_id) + publishing_api_mock.expects(:get_host_content_for_content_id).returns(fake_api_response) result = described_class.by_embedded_document(content_block_document: document) @@ -127,21 +118,18 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase end it "returns an error if the content that embeds the target can't be loaded" do - exception = GdsApi::HTTPErrorResponse.new( - 500, - "An internal error message", - "error" => { "message" => "Some backend error" }, + publishing_api_mock.expects(:get_host_content_for_content_id).raises( + GdsApi::HTTPErrorResponse.new( + 500, + "An internal error message", + "error" => { "message" => "Some backend error" }, + ), ) - raises_exception = ->(*_args) { raise exception } - - Services.publishing_api.stub :get_host_content_for_content_id, raises_exception do - document = mock("content_block_document", content_id: target_content_id) - - assert_raises(GdsApi::HTTPErrorResponse) do - described_class.by_embedded_document( - content_block_document: document, - ) - end + + assert_raises(GdsApi::HTTPErrorResponse) do + described_class.by_embedded_document( + content_block_document: document, + ) end end end From b75058037cacad50221f8364e0dd001a80ac87da Mon Sep 17 00:00:00 2001 From: pezholio Date: Tue, 7 Jan 2025 09:45:21 +0000 Subject: [PATCH 5/7] Return editor object when fetching host content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This calls `ContentBlockManager::HostContentItem::Editor.with_uuids` with all the editor IDs when fetching a page of results, meaning we only have to make one API call per page. I’ve kept `last_edited_by_editor_id` in the response for now, but will remove this in a later commit. --- .../host_content_item.rb | 1 + .../get_host_content_items.rb | 13 ++++++++ .../content_block_manager_steps.rb | 4 +++ .../test/factories/host_content_item.rb | 2 ++ .../factories/host_content_item_editor.rb | 17 +++++++++++ .../host_content_item_editor_organisation.rb | 15 ++++++++++ .../test/support/embedded_content_helpers.rb | 4 +++ .../services/get_host_content_items_test.rb | 30 +++++++++++++++++-- 8 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 lib/engines/content_block_manager/test/factories/host_content_item_editor.rb create mode 100644 lib/engines/content_block_manager/test/factories/host_content_item_editor_organisation.rb diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb index 41b11c94bf1..3835b7860ca 100644 --- a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb +++ b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb @@ -5,6 +5,7 @@ class HostContentItem < Data.define( :document_type, :publishing_organisation, :publishing_app, + :last_edited_by_editor, :last_edited_by_editor_id, :last_edited_at, :unique_pageviews, diff --git a/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb b/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb index 4f82fbe2579..0f53d284daf 100644 --- a/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb +++ b/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb @@ -26,6 +26,7 @@ def items document_type: item["document_type"], publishing_organisation: item["primary_publishing_organisation"], publishing_app: item["publishing_app"], + last_edited_by_editor: editor_for_uid(item["last_edited_by_editor_id"]), last_edited_by_editor_id: item["last_edited_by_editor_id"], last_edited_at: item["last_edited_at"], unique_pageviews: item["unique_pageviews"], @@ -61,5 +62,17 @@ def content_items response.parsed_content end end + + def editor_for_uid(uid) + editors.find { |editor| editor.uid == uid } + end + + def editors + @editors ||= editor_uuids.present? ? ContentBlockManager::HostContentItem::Editor.with_uuids(editor_uuids) : [] + end + + def editor_uuids + content_items["results"].map { |c| c["last_edited_by_editor_id"] }.compact + end end end diff --git a/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb b/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb index 70b4f084e23..ebdf4ae013c 100644 --- a/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb +++ b/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb @@ -533,6 +533,10 @@ def should_show_edit_form_for_email_address_content_block(document_title, email_ ) stub_publishing_api_has_embedded_content_details(@dependent_content.first) + + stub_request(:get, "#{Plek.find('signon', external: true)}/api/users") + .with(query: { uuids: @dependent_content.map { |item| item["last_edited_by_editor_id"] } }) + .to_return(body: [].to_json) end Then(/^I should see the dependent content listed$/) do diff --git a/lib/engines/content_block_manager/test/factories/host_content_item.rb b/lib/engines/content_block_manager/test/factories/host_content_item.rb index 267bd5e8482..7a39d0dbde5 100644 --- a/lib/engines/content_block_manager/test/factories/host_content_item.rb +++ b/lib/engines/content_block_manager/test/factories/host_content_item.rb @@ -5,6 +5,7 @@ document_type { "something" } publishing_organisation { { name: "organisation", content_id: SecureRandom.uuid } } publishing_app { "publishing_app" } + last_edited_by_editor { build(:host_content_item_editor) } last_edited_by_editor_id { SecureRandom.uuid } last_edited_at { 2.days.ago.to_s } unique_pageviews { 123 } @@ -18,6 +19,7 @@ publishing_organisation:, publishing_app:, last_edited_by_editor_id:, + last_edited_by_editor:, last_edited_at:, unique_pageviews:, host_content_id:, diff --git a/lib/engines/content_block_manager/test/factories/host_content_item_editor.rb b/lib/engines/content_block_manager/test/factories/host_content_item_editor.rb new file mode 100644 index 00000000000..5cce90dc7d6 --- /dev/null +++ b/lib/engines/content_block_manager/test/factories/host_content_item_editor.rb @@ -0,0 +1,17 @@ +FactoryBot.define do + factory :host_content_item_editor, class: "ContentBlockManager::HostContentItem::Editor" do + uid { SecureRandom.uuid } + sequence(:name) { |i| "Someone #{i}" } + sequence(:email) { |i| "someone-#{i}@example.com" } + organisation { build(:host_content_item_editor_organisation) } + + initialize_with do + new( + uid:, + name:, + email:, + organisation:, + ) + end + end +end diff --git a/lib/engines/content_block_manager/test/factories/host_content_item_editor_organisation.rb b/lib/engines/content_block_manager/test/factories/host_content_item_editor_organisation.rb new file mode 100644 index 00000000000..813db453a23 --- /dev/null +++ b/lib/engines/content_block_manager/test/factories/host_content_item_editor_organisation.rb @@ -0,0 +1,15 @@ +FactoryBot.define do + factory :host_content_item_editor_organisation, class: "ContentBlockManager::HostContentItem::Editor::Organisation" do + content_id { SecureRandom.uuid } + sequence(:name) { |i| "organisation #{i}" } + sequence(:slug) { |i| "organisation-#{i}" } + + initialize_with do + new( + content_id:, + name:, + slug:, + ) + end + end +end diff --git a/lib/engines/content_block_manager/test/support/embedded_content_helpers.rb b/lib/engines/content_block_manager/test/support/embedded_content_helpers.rb index ddd9f5a14e2..64fc3fdc9cf 100644 --- a/lib/engines/content_block_manager/test/support/embedded_content_helpers.rb +++ b/lib/engines/content_block_manager/test/support/embedded_content_helpers.rb @@ -19,12 +19,16 @@ def self.it_returns_embedded_content(&block) end end + let(:host_content_item_users) { build_list(:host_content_item_editor, 10) } + before do stub_publishing_api_has_embedded_content_for_any_content_id( results: host_content_items, total: host_content_items.length, order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER, ) + + ContentBlockManager::HostContentItem::Editor.stubs(:with_uuids).with(host_content_items.map { |i| i["last_edited_by_editor_id"] }).returns(host_content_item_users) end it "returns host content items" do diff --git a/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb b/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb index 8466c32127c..a95bdc6e0bf 100644 --- a/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb @@ -9,6 +9,8 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase let(:host_content_id) { SecureRandom.uuid } + let(:last_edited_by_editor_id) { SecureRandom.uuid } + let(:rollup) { build(:rollup) } let(:response_body) do @@ -23,7 +25,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase "base_path" => "/foo", "document_type" => "something", "publishing_app" => "publisher", - "last_edited_by_editor_id" => SecureRandom.uuid, + "last_edited_by_editor_id" => last_edited_by_editor_id, "last_edited_at" => "2023-01-01T08:00:00.000Z", "unique_pageviews" => 123, "instances" => 1, @@ -38,6 +40,8 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase } end + let(:editors) { build_list(:host_content_item_editor, 1) } + setup do stub_publishing_api_has_embedded_content(content_id: target_content_id, total: 111, total_pages: 12, results: response_body["results"], order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER, rollup: response_body["rollup"]) end @@ -53,6 +57,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase before do Services.expects(:publishing_api).returns(publishing_api_mock) + ContentBlockManager::HostContentItem::Editor.stubs(:with_uuids).returns(editors) end it "calls the Publishing API for the content which embeds the target" do @@ -85,6 +90,13 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase ) end + it "calls the editor finder with the correct argument" do + publishing_api_mock.expects(:get_host_content_for_content_id).returns(fake_api_response) + ContentBlockManager::HostContentItem::Editor.expects(:with_uuids).with([last_edited_by_editor_id]).returns(editors) + + described_class.by_embedded_document(content_block_document: document) + end + it "returns GetHostContentItems" do publishing_api_mock.expects(:get_host_content_for_content_id).returns(fake_api_response) @@ -117,6 +129,20 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase assert_equal result[0].publishing_organisation, expected_publishing_organisation end + describe "when last_edited_by_editor_id is nil" do + let(:last_edited_by_editor_id) { nil } + + it "does not call #with_uuids" do + publishing_api_mock.expects(:get_host_content_for_content_id).returns(fake_api_response) + + ContentBlockManager::HostContentItem::Editor.expects(:with_uuids).never + + result = described_class.by_embedded_document(content_block_document: document) + + assert_equal result[0].last_edited_by_editor, nil + end + end + it "returns an error if the content that embeds the target can't be loaded" do publishing_api_mock.expects(:get_host_content_for_content_id).raises( GdsApi::HTTPErrorResponse.new( @@ -125,7 +151,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase "error" => { "message" => "Some backend error" }, ), ) - + assert_raises(GdsApi::HTTPErrorResponse) do described_class.by_embedded_document( content_block_document: document, From f63373325f7b1261e530f2c790363293092a3f80 Mon Sep 17 00:00:00 2001 From: pezholio Date: Tue, 7 Jan 2025 09:54:33 +0000 Subject: [PATCH 6/7] Use `last_edited_by_editor` in component This removes the dependency on the Whitehall database when getting the associated user for a piece of content. --- .../show/host_editions_table_component.rb | 12 +------- .../host_editions_table_component_test.rb | 28 +++++-------------- 2 files changed, 8 insertions(+), 32 deletions(-) diff --git a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/show/host_editions_table_component.rb b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/show/host_editions_table_component.rb index 1beffeaacdf..ceb710c77e3 100644 --- a/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/show/host_editions_table_component.rb +++ b/lib/engines/content_block_manager/app/components/content_block_manager/content_block/document/show/host_editions_table_component.rb @@ -71,15 +71,6 @@ def sort_link(param) helpers.content_block_manager.url_for(only_path: false, params: { order: param }, anchor: TABLE_ID) end - # TODO: Currently, we're only fetching Users from the local Whitehall database, which means - # content updated outside Whitehall with a `last_edited_by_editor_id` where the user - # does not have Whitehall access will show up as an unknown user. We are looking to - # fix this by possibly adding an endpoint to Signon, but this gets us part of the way - # there. Card for this work is here: https://trello.com/c/jVvs4nAP/640-get-author-information-from-signon - def users - @users ||= User.where(uid: host_content_items.map(&:last_edited_by_editor_id)) - end - def frontend_path(content_item) if @is_preview helpers.content_block_manager.content_block_manager_content_block_host_content_preview_path(id: content_block_edition.id, host_content_id: content_item.host_content_id) @@ -122,8 +113,7 @@ def all_publishing_organisations end def updated_field_for(content_item) - last_updated_by_user = content_item.last_edited_by_editor_id && users.find { |u| u.uid == content_item.last_edited_by_editor_id } - user_copy = last_updated_by_user ? mail_to(last_updated_by_user.email, last_updated_by_user.name, { class: "govuk-link" }) : "Unknown user" + user_copy = content_item.last_edited_by_editor ? mail_to(content_item.last_edited_by_editor.email, content_item.last_edited_by_editor.name, { class: "govuk-link" }) : "Unknown user" "#{time_ago_in_words(content_item.last_edited_at)} ago by #{user_copy}".html_safe end end diff --git a/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb b/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb index f53ba13aec6..8e87cb89181 100644 --- a/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb +++ b/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb @@ -6,7 +6,6 @@ class ContentBlockManager::ContentBlock::Document::Show::HostEditionsTableCompon include ActionView::Helpers::DateHelper let(:described_class) { ContentBlockManager::ContentBlock::Document::Show::HostEditionsTableComponent } - let(:user) { create(:user) } let(:caption) { "Some caption" } let(:publishing_organisation) do { @@ -15,16 +14,17 @@ class ContentBlockManager::ContentBlock::Document::Show::HostEditionsTableCompon "base_path" => "/bar", } end - let(:last_edited_by_editor_id) { user.uid } let(:unique_pageviews) { 1_200_000 } + let(:last_edited_by_editor) { build(:host_content_item_editor) } let(:host_content_item) do ContentBlockManager::HostContentItem.new( "title" => "Some title", "base_path" => "/foo", "document_type" => "document_type", "publishing_app" => "publisher", - "last_edited_by_editor_id" => last_edited_by_editor_id, + "last_edited_by_editor_id" => SecureRandom.uuid, + "last_edited_by_editor" => last_edited_by_editor, "last_edited_at" => Time.zone.now.to_s, "publishing_organisation" => publishing_organisation, "unique_pageviews" => unique_pageviews, @@ -88,8 +88,8 @@ def self.it_returns_unknown_user assert_selector "tbody .govuk-table__cell", text: host_content_item.document_type.humanize assert_selector "tbody .govuk-table__cell", text: "1.2m" assert_selector "tbody .govuk-table__cell", text: host_content_item.publishing_organisation["title"] - assert_selector "tbody .govuk-table__cell", text: "#{time_ago_in_words(host_content_item.last_edited_at)} ago by #{user.name}" - assert_link user.name, { href: "mailto:#{user.email}" } + assert_selector "tbody .govuk-table__cell", text: "#{time_ago_in_words(host_content_item.last_edited_at)} ago by #{last_edited_by_editor.name}" + assert_link last_edited_by_editor.name, { href: "mailto:#{last_edited_by_editor.email}" } end context "when the organisation does NOT exist within Whitehall" do @@ -147,22 +147,8 @@ def self.it_returns_unknown_user end end - context "when last_edited_by_editor_id is nil" do - let(:last_edited_by_editor_id) { nil } - - it_returns_unknown_user - - context "and a user exists with a nil uuid" do - before do - create(:user, uid: nil) - end - - it_returns_unknown_user - end - end - - context "when last_edited_by_editor_id refers to a user id which is not present in Whitehall" do - let(:last_edited_by_editor_id) { SecureRandom.uuid } + context "when last_edited_by_editor is nil" do + let(:last_edited_by_editor) { nil } it_returns_unknown_user end From 7a9ac807c87abf69ecb652756b88785b5dc48188 Mon Sep 17 00:00:00 2001 From: pezholio Date: Tue, 7 Jan 2025 11:22:47 +0000 Subject: [PATCH 7/7] Remove last_edited_by_editor_id This is now no longer needed, as we return the entire object --- .../app/models/content_block_manager/host_content_item.rb | 1 - .../content_block_manager/get_host_content_items.rb | 1 - .../document/show/host_editions_table_component_test.rb | 1 - .../test/factories/host_content_item.rb | 2 -- .../test/unit/app/services/get_host_content_items_test.rb | 8 ++++---- 5 files changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb index 3835b7860ca..31482d860fd 100644 --- a/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb +++ b/lib/engines/content_block_manager/app/models/content_block_manager/host_content_item.rb @@ -6,7 +6,6 @@ class HostContentItem < Data.define( :publishing_organisation, :publishing_app, :last_edited_by_editor, - :last_edited_by_editor_id, :last_edited_at, :unique_pageviews, :instances, diff --git a/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb b/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb index 0f53d284daf..8fc9df6468e 100644 --- a/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb +++ b/lib/engines/content_block_manager/app/services/content_block_manager/get_host_content_items.rb @@ -27,7 +27,6 @@ def items publishing_organisation: item["primary_publishing_organisation"], publishing_app: item["publishing_app"], last_edited_by_editor: editor_for_uid(item["last_edited_by_editor_id"]), - last_edited_by_editor_id: item["last_edited_by_editor_id"], last_edited_at: item["last_edited_at"], unique_pageviews: item["unique_pageviews"], instances: item["instances"], diff --git a/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb b/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb index 8e87cb89181..6fcfb9e98d0 100644 --- a/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb +++ b/lib/engines/content_block_manager/test/components/content_block/document/show/host_editions_table_component_test.rb @@ -23,7 +23,6 @@ class ContentBlockManager::ContentBlock::Document::Show::HostEditionsTableCompon "base_path" => "/foo", "document_type" => "document_type", "publishing_app" => "publisher", - "last_edited_by_editor_id" => SecureRandom.uuid, "last_edited_by_editor" => last_edited_by_editor, "last_edited_at" => Time.zone.now.to_s, "publishing_organisation" => publishing_organisation, diff --git a/lib/engines/content_block_manager/test/factories/host_content_item.rb b/lib/engines/content_block_manager/test/factories/host_content_item.rb index 7a39d0dbde5..a320f149a14 100644 --- a/lib/engines/content_block_manager/test/factories/host_content_item.rb +++ b/lib/engines/content_block_manager/test/factories/host_content_item.rb @@ -6,7 +6,6 @@ publishing_organisation { { name: "organisation", content_id: SecureRandom.uuid } } publishing_app { "publishing_app" } last_edited_by_editor { build(:host_content_item_editor) } - last_edited_by_editor_id { SecureRandom.uuid } last_edited_at { 2.days.ago.to_s } unique_pageviews { 123 } instances { 1 } @@ -18,7 +17,6 @@ document_type:, publishing_organisation:, publishing_app:, - last_edited_by_editor_id:, last_edited_by_editor:, last_edited_at:, unique_pageviews:, diff --git a/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb b/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb index a95bdc6e0bf..200327433ca 100644 --- a/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/services/get_host_content_items_test.rb @@ -40,7 +40,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase } end - let(:editors) { build_list(:host_content_item_editor, 1) } + let(:editor) { build(:host_content_item_editor, uid: last_edited_by_editor_id) } setup do stub_publishing_api_has_embedded_content(content_id: target_content_id, total: 111, total_pages: 12, results: response_body["results"], order: ContentBlockManager::GetHostContentItems::DEFAULT_ORDER, rollup: response_body["rollup"]) @@ -57,7 +57,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase before do Services.expects(:publishing_api).returns(publishing_api_mock) - ContentBlockManager::HostContentItem::Editor.stubs(:with_uuids).returns(editors) + ContentBlockManager::HostContentItem::Editor.stubs(:with_uuids).returns([editor]) end it "calls the Publishing API for the content which embeds the target" do @@ -92,7 +92,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase it "calls the editor finder with the correct argument" do publishing_api_mock.expects(:get_host_content_for_content_id).returns(fake_api_response) - ContentBlockManager::HostContentItem::Editor.expects(:with_uuids).with([last_edited_by_editor_id]).returns(editors) + ContentBlockManager::HostContentItem::Editor.expects(:with_uuids).with([last_edited_by_editor_id]).returns([editor]) described_class.by_embedded_document(content_block_document: document) end @@ -120,7 +120,7 @@ class ContentBlockManager::GetHostContentItemsTest < ActiveSupport::TestCase assert_equal result[0].base_path, response_body["results"][0]["base_path"] assert_equal result[0].document_type, response_body["results"][0]["document_type"] assert_equal result[0].publishing_app, response_body["results"][0]["publishing_app"] - assert_equal result[0].last_edited_by_editor_id, response_body["results"][0]["last_edited_by_editor_id"] + assert_equal result[0].last_edited_by_editor, editor assert_equal result[0].last_edited_at, Time.zone.parse(response_body["results"][0]["last_edited_at"]) assert_equal result[0].unique_pageviews, response_body["results"][0]["unique_pageviews"] assert_equal result[0].instances, response_body["results"][0]["instances"]