From 03d05679306eda5e06ac87c3c4596babe7edcca9 Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 16 Dec 2024 15:37:34 +0000 Subject: [PATCH 01/10] Use a decorator instead of a presenter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This better matches what we’re trying to do, and also moves us out of the `Query` namespace, as we’ll soon be handling decorating versions elsewhere. --- .../audit_trail_entry_component.html.erb | 2 +- .../editions/audit_trail_entry_component.rb | 2 +- app/decorators/version_decorator.rb | 33 ++++++++++++++ app/models/document/paginated_timeline.rb | 6 +-- app/presenters/queries/version_presenter.rb | 45 ------------------- .../document/paginated_timeline_query.rb | 4 +- .../audit_trail_entry_component_test.rb | 6 +-- .../version_decorator_test.rb} | 31 ++++++++----- .../document/paginated_timeline_test.rb | 18 ++++---- .../document/paginated_timeline_query_test.rb | 8 ++-- 10 files changed, 76 insertions(+), 79 deletions(-) create mode 100644 app/decorators/version_decorator.rb delete mode 100644 app/presenters/queries/version_presenter.rb rename test/unit/app/{presenters/queries/version_presenter_test.rb => decorators/version_decorator_test.rb} (62%) diff --git a/app/components/admin/editions/audit_trail_entry_component.html.erb b/app/components/admin/editions/audit_trail_entry_component.html.erb index 7eb9ad3f83d..2896f5dec83 100644 --- a/app/components/admin/editions/audit_trail_entry_component.html.erb +++ b/app/components/admin/editions/audit_trail_entry_component.html.erb @@ -3,7 +3,7 @@ <% if compare_with_previous_version? %>

- <%= link_to "[Compare with previous version]", diff_admin_edition_path(@edition, audit_trail_entry_id: entry.version.item_id), class: "govuk-link" %> + <%= link_to "[Compare with previous version]", diff_admin_edition_path(@edition, audit_trail_entry_id: entry.item_id), class: "govuk-link" %>

<% end %> diff --git a/app/components/admin/editions/audit_trail_entry_component.rb b/app/components/admin/editions/audit_trail_entry_component.rb index 46cbaa60138..d55ef6cb6fc 100644 --- a/app/components/admin/editions/audit_trail_entry_component.rb +++ b/app/components/admin/editions/audit_trail_entry_component.rb @@ -25,6 +25,6 @@ def time end def compare_with_previous_version? - entry.action == "published" && edition.id != entry.version.item_id + entry.action == "published" && edition.id != entry.item_id end end diff --git a/app/decorators/version_decorator.rb b/app/decorators/version_decorator.rb new file mode 100644 index 00000000000..377e5c869e4 --- /dev/null +++ b/app/decorators/version_decorator.rb @@ -0,0 +1,33 @@ +class VersionDecorator < SimpleDelegator + def initialize(version, is_first_edition: false, previous_version: nil) + @is_first_edition = is_first_edition + @preloaded_previous_version = previous_version + super(version) + end + + def ==(other) + self.class == other.class && + id == other.id && + action == other.action + end + + def actor + user + end + + def action + case event + when "create" + @is_first_edition ? "created" : "editioned" + else + previous_version&.state != state ? state : "updated" + end + end + + private + + def previous_version + # we can avoid n+1 queries by using our preloaded_prev_version + @previous_version ||= @preloaded_previous_version || previous + end +end diff --git a/app/models/document/paginated_timeline.rb b/app/models/document/paginated_timeline.rb index fa99ff53aa4..593a6dd6d51 100644 --- a/app/models/document/paginated_timeline.rb +++ b/app/models/document/paginated_timeline.rb @@ -33,7 +33,7 @@ def entries_on_newer_editions(edition) if entry.is_a?(EditorialRemark) entry.edition_id > edition.id else - entry.version.item_id > edition.id + entry.item_id > edition.id end end end @@ -43,7 +43,7 @@ def entries_on_current_edition(edition) if entry.is_a?(EditorialRemark) entry.edition_id == edition.id else - entry.version.item_id == edition.id + entry.item_id == edition.id end end end @@ -53,7 +53,7 @@ def entries_on_previous_editions(edition) if entry.is_a?(EditorialRemark) entry.edition_id < edition.id else - entry.version.item_id < edition.id + entry.item_id < edition.id end end end diff --git a/app/presenters/queries/version_presenter.rb b/app/presenters/queries/version_presenter.rb deleted file mode 100644 index 9d92259b92b..00000000000 --- a/app/presenters/queries/version_presenter.rb +++ /dev/null @@ -1,45 +0,0 @@ -module Queries - class VersionPresenter - extend ActiveModel::Naming - - def self.model_name - ActiveModel::Name.new(Version, nil) - end - - attr_reader :version - - delegate :created_at, :to_key, to: :version - - def initialize(version, is_first_edition:, previous_version: nil) - @version = version - @is_first_edition = is_first_edition - @preloaded_previous_version = previous_version - end - - def ==(other) - self.class == other.class && - version == other.version && - action == other.action - end - - def actor - version.user - end - - def action - case version.event - when "create" - @is_first_edition ? "created" : "editioned" - else - previous_version&.state != version.state ? version.state : "updated" - end - end - - private - - def previous_version - # we can avoid n+1 queries by using our preloaded_prev_version - @previous_version ||= @preloaded_previous_version || version.previous - end - end -end diff --git a/app/queries/document/paginated_timeline_query.rb b/app/queries/document/paginated_timeline_query.rb index 83fa7fb3781..fdb64286e3a 100644 --- a/app/queries/document/paginated_timeline_query.rb +++ b/app/queries/document/paginated_timeline_query.rb @@ -28,12 +28,12 @@ def remarks def versions versions = document_versions.where(id: version_ids) versions.map.with_index { |version, index| - presenter = Queries::VersionPresenter.new( + version = VersionDecorator.new( version, is_first_edition: version.item_id == first_edition_id, previous_version: versions[index - 1], ) - [version.id, presenter] + [version.id, version] }.to_h end diff --git a/test/components/admin/editions/audit_trail_entry_component_test.rb b/test/components/admin/editions/audit_trail_entry_component_test.rb index 35446e98827..bd1fb49e1fe 100644 --- a/test/components/admin/editions/audit_trail_entry_component_test.rb +++ b/test/components/admin/editions/audit_trail_entry_component_test.rb @@ -10,7 +10,7 @@ class Admin::Editions::AuditTrailEntryComponentTest < ViewComponent::TestCase edition = build_stubbed(:edition) version = edition.versions.new(event: "create", created_at: Time.zone.local(2020, 1, 1, 11, 11)) version.stubs(:user).returns(actor) - audit = Queries::VersionPresenter.new(version, is_first_edition: true) + audit = VersionDecorator.new(version, is_first_edition: true) render_inline(Admin::Editions::AuditTrailEntryComponent.new(entry: audit, edition:)) @@ -23,7 +23,7 @@ class Admin::Editions::AuditTrailEntryComponentTest < ViewComponent::TestCase test "it constructs output based on the entry when an actor is absent" do edition = build_stubbed(:edition) version = edition.versions.new(event: "create", created_at: Time.zone.local(2020, 1, 1, 11, 11), whodunnit: nil) - audit = Queries::VersionPresenter.new(version, is_first_edition: true) + audit = VersionDecorator.new(version, is_first_edition: true) render_inline(Admin::Editions::AuditTrailEntryComponent.new(entry: audit, edition:)) @@ -37,7 +37,7 @@ class Admin::Editions::AuditTrailEntryComponentTest < ViewComponent::TestCase edition.versions.new(event: "create", created_at: Time.zone.local(2020, 1, 1, 11, 11), whodunnit: actor.id) version = edition.versions.new(event: "published", created_at: Time.zone.local(2020, 1, 1, 11, 11), state: "published") version.stubs(:user).returns(actor) - audit = Queries::VersionPresenter.new(version, is_first_edition: true) + audit = VersionDecorator.new(version, is_first_edition: true) newer_edition = build_stubbed(:edition, :draft) render_inline(Admin::Editions::AuditTrailEntryComponent.new(entry: audit, edition: newer_edition)) diff --git a/test/unit/app/presenters/queries/version_presenter_test.rb b/test/unit/app/decorators/version_decorator_test.rb similarity index 62% rename from test/unit/app/presenters/queries/version_presenter_test.rb rename to test/unit/app/decorators/version_decorator_test.rb index c950f35d2c2..2f2a6eb1200 100644 --- a/test/unit/app/presenters/queries/version_presenter_test.rb +++ b/test/unit/app/decorators/version_decorator_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class Queries::VersionPresenterTest < ActiveSupport::TestCase +class VersionDecoratorTest < ActiveSupport::TestCase extend Minitest::Spec::DSL let(:user) { build(:user) } @@ -9,17 +9,26 @@ class Queries::VersionPresenterTest < ActiveSupport::TestCase let(:is_first_edition) { false } let(:previous_version) { build(:version, user:, item: edition) } - let(:presenter) { Queries::VersionPresenter.new(version, is_first_edition:, previous_version:) } + let(:decorator) { VersionDecorator.new(version, is_first_edition:, previous_version:) } - describe ".model_name" do - it "returns the model name" do - assert_equal Queries::VersionPresenter.model_name, ActiveModel::Name.new(Version, nil) + describe "#==" do + it "is true if the class, ID and action are the same" do + other_decorator = VersionDecorator.new(version, is_first_edition:, previous_version:) + + assert decorator == other_decorator + end + + it "is not true if the class ID and action are different" do + other_version = build(:version, user:, item: edition, id: 444) + other_decorator = VersionDecorator.new(other_version, is_first_edition:, previous_version:) + + assert_not decorator == other_decorator end end describe "#actor" do it "returns the version's user" do - assert_equal presenter.actor, version.user + assert_equal decorator.actor, version.user end end @@ -28,14 +37,14 @@ class Queries::VersionPresenterTest < ActiveSupport::TestCase let(:version) { build(:version, user:, item: edition, event: "create") } it "returns `editioned`" do - assert_equal presenter.action, "editioned" + assert_equal decorator.action, "editioned" end context "and the edition is the first edition" do let(:is_first_edition) { true } it "returns `created`" do - assert_equal presenter.action, "created" + assert_equal decorator.action, "created" end end end @@ -45,7 +54,7 @@ class Queries::VersionPresenterTest < ActiveSupport::TestCase let(:previous_version) { build(:version, user:, item: edition, state: "some_state") } it "returns `updated`" do - assert_equal presenter.action, "updated" + assert_equal decorator.action, "updated" end end @@ -54,7 +63,7 @@ class Queries::VersionPresenterTest < ActiveSupport::TestCase let(:previous_version) { build(:version, user:, item: edition, state: "state1") } it "returns the version's state" do - assert_equal presenter.action, version.state + assert_equal decorator.action, version.state end end @@ -65,7 +74,7 @@ class Queries::VersionPresenterTest < ActiveSupport::TestCase loaded_version = build(:version, user:, item: edition) version.expects(:previous).returns(loaded_version) - presenter.action + decorator.action end end end diff --git a/test/unit/app/models/document/paginated_timeline_test.rb b/test/unit/app/models/document/paginated_timeline_test.rb index c32d245c72e..c7b792241ed 100644 --- a/test/unit/app/models/document/paginated_timeline_test.rb +++ b/test/unit/app/models/document/paginated_timeline_test.rb @@ -13,13 +13,13 @@ class PaginatedTimelineTest < ActiveSupport::TestCase assert_equal entry_timestamps.sort.reverse, entry_timestamps end - test "#entries is a list of VersionPresenter and EditorialRemark objects when no 'only' argument is passed in" do + test "#entries is a list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - assert_equal [Queries::VersionPresenter, EditorialRemark].to_set, + assert_equal [VersionDecorator, EditorialRemark].to_set, timeline.entries.map(&:class).to_set end - test "#total_count counts the list of VersionPresenter and EditorialRemark objects when no 'only' argument is passed in" do + test "#total_count counts the list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) assert_equal 11, timeline.total_count end @@ -58,7 +58,7 @@ class PaginatedTimelineTest < ActiveSupport::TestCase assert_equal 3, timeline.total_count end - test "#entries is a list of VersionPresenter objects when 'history' is passed in as a 'only' argument" do + test "#entries is a list of VersionDecorator objects when 'history' is passed in as a 'only' argument" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") expected_actions = %w[updated editioned @@ -72,7 +72,7 @@ class PaginatedTimelineTest < ActiveSupport::TestCase assert_equal expected_actions, timeline.entries.map(&:action) end - test "#total_count counts the total VersionPresenter objects when 'history' is passed into the 'only' argument" do + test "#total_count counts the total VersionDecorator objects when 'history' is passed into the 'only' argument" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") assert_equal 8, timeline.total_count end @@ -110,10 +110,10 @@ class PaginatedTimelineTest < ActiveSupport::TestCase end end - test "VersionPresenter correctly determines actions" do + test "VersionDecorator correctly determines actions" do mock_pagination(per_page: 30) do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - entries = timeline.entries.select { |e| e.instance_of?(Queries::VersionPresenter) } + entries = timeline.entries.select { |e| e.instance_of?(VersionDecorator) } expected_actions = %w[updated editioned published @@ -127,9 +127,9 @@ class PaginatedTimelineTest < ActiveSupport::TestCase end end - test "VersionPresenter correctly determines actors" do + test "VersionDecorator correctly determines actors" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - entries = timeline.entries.select { |e| e.instance_of?(Queries::VersionPresenter) } + entries = timeline.entries.select { |e| e.instance_of?(VersionDecorator) } expected_actors = [@user, @user, @user2, diff --git a/test/unit/app/queries/document/paginated_timeline_query_test.rb b/test/unit/app/queries/document/paginated_timeline_query_test.rb index 4cf48a56ea2..290cef802f0 100644 --- a/test/unit/app/queries/document/paginated_timeline_query_test.rb +++ b/test/unit/app/queries/document/paginated_timeline_query_test.rb @@ -77,7 +77,7 @@ class Document::PaginatedTimelineQueryTest < ActiveSupport::TestCase end describe "#versions" do - it "returns all the versions, presented as VersionPresenter items" do + it "returns all the versions, presented as VersionDecorator items" do page1_query = Document::PaginatedTimelineQuery.new(document: @document, page: 1) page2_query = Document::PaginatedTimelineQuery.new(document: @document, page: 2) @@ -85,10 +85,10 @@ class Document::PaginatedTimelineQueryTest < ActiveSupport::TestCase version_presenter_mocks = [] versions.each do |version| - version_presenter_mock = mock("Queries::VersionPresenter", id: version.id) - Queries::VersionPresenter.stubs(:new).with { |v, **_args| + version_decorator_mock = mock("VersionDecorator", id: version.id) + VersionDecorator.stubs(:new).with { |v, **_args| v.id == version.id - }.returns(version_presenter_mock) + }.returns(version_decorator_mock) version_presenter_mocks.push(version_presenter_mock) end From fcfa164e77286ffbd587181fc2bc2e4a9d856d8d Mon Sep 17 00:00:00 2001 From: pezholio Date: Fri, 13 Dec 2024 11:05:55 +0000 Subject: [PATCH 02/10] Simplify the timeline query This removes some of the logic to the Document model, menaing the only thing the query class needs to worry about is building and running the SQL --- app/models/document.rb | 25 ++++ app/models/document/paginated_timeline.rb | 4 +- .../document/paginated_timeline_query.rb | 44 ++----- test/unit/app/models/document_test.rb | 110 ++++++++++++++++++ .../document/paginated_timeline_query_test.rb | 38 ------ 5 files changed, 145 insertions(+), 76 deletions(-) diff --git a/app/models/document.rb b/app/models/document.rb index 8630ef58764..fd413b905c8 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -60,6 +60,31 @@ def self.at_slug(document_types, slug) find_by(document_type: document_types, slug:) end + def remarks_by_ids(remark_ids) + editorial_remarks.where(id: remark_ids).index_by(&:id) + end + + def active_edition_versions + edition_versions.where.not(state: "superseded") + end + + def decorated_edition_versions_by_ids(version_ids) + versions = active_edition_versions.where(id: version_ids) + + versions.map.with_index { |version, index| + version = VersionDecorator.new( + version, + is_first_edition: version.item_id == first_edition_id, + previous_version: versions[index - 1], + ) + [version.id, version] + }.to_h + end + + def first_edition_id + @first_edition_id ||= editions.pick(:id) + end + def similar_slug_exists? scope = Document.where(document_type:) sequence_separator = friendly_id_config.sequence_separator diff --git a/app/models/document/paginated_timeline.rb b/app/models/document/paginated_timeline.rb index 593a6dd6d51..80f563b41bf 100644 --- a/app/models/document/paginated_timeline.rb +++ b/app/models/document/paginated_timeline.rb @@ -10,8 +10,8 @@ def initialize(document:, page:, only: nil) def entries @entries ||= begin raw_entries = query.raw_entries - remarks = query.remarks - versions = query.versions + remarks = document.remarks_by_ids(query.remark_ids) + versions = document.decorated_edition_versions_by_ids(query.version_ids) raw_entries.map do |entry| case entry.model diff --git a/app/queries/document/paginated_timeline_query.rb b/app/queries/document/paginated_timeline_query.rb index fdb64286e3a..c4613b0ff5c 100644 --- a/app/queries/document/paginated_timeline_query.rb +++ b/app/queries/document/paginated_timeline_query.rb @@ -21,26 +21,6 @@ def total_count end end - def remarks - document_remarks.where(id: remark_ids).index_by(&:id) - end - - def versions - versions = document_versions.where(id: version_ids) - versions.map.with_index { |version, index| - version = VersionDecorator.new( - version, - is_first_edition: version.item_id == first_edition_id, - previous_version: versions[index - 1], - ) - [version.id, version] - }.to_h - end - - private - - attr_reader :document, :page, :only - def remark_ids raw_entries.select { |r| r.model == "EditorialRemark" }.map(&:id) end @@ -49,6 +29,10 @@ def version_ids raw_entries.select { |r| r.model == "Version" }.map(&:id) end + private + + attr_reader :document, :page, :only + def paginated_query sql = <<~SQL #{timeline_sql} @@ -67,14 +51,6 @@ def paginated_query ApplicationRecord.connection.exec_query(sql, "SQL", bind_params) end - def document_versions - @document.edition_versions.where.not(state: "superseded") - end - - def document_remarks - @document.editorial_remarks - end - def timeline_sql case only when "history" @@ -87,15 +63,15 @@ def timeline_sql end def versions_query - document_versions.select( - "'#{document_versions.class_name}' AS model_name", + document.active_edition_versions.select( + "'#{Version}' AS model_name", *common_fields, ).to_sql end def remarks_query - document_remarks.select( - "'#{document_remarks.class_name}' AS model_name", + document.editorial_remarks.select( + "'#{EditorialRemark}' AS model_name", *common_fields, ).to_sql end @@ -103,9 +79,5 @@ def remarks_query def common_fields %i[id created_at] end - - def first_edition_id - @first_edition_id ||= document.editions.pick(:id) - end end end diff --git a/test/unit/app/models/document_test.rb b/test/unit/app/models/document_test.rb index 445de311f04..6c96c3c99bc 100644 --- a/test/unit/app/models/document_test.rb +++ b/test/unit/app/models/document_test.rb @@ -595,4 +595,114 @@ class DocumentTest < ActiveSupport::TestCase end end end + + describe "#remarks_by_ids" do + it "returns all the remarks keyed by ID" do + document = create(:document) + edition1 = create(:published_edition, document: document) + edition2 = create(:published_edition, document: document) + edition3 = create(:published_edition, document: document) + + editorial_remark1 = create(:editorial_remark, edition: edition1) + editorial_remark2 = create(:editorial_remark, edition: edition2) + editorial_remark3 = create(:editorial_remark, edition: edition3) + _other_remark = create(:editorial_remark, edition: edition3) + + expected_remarks = { + editorial_remark1.id => editorial_remark1, + editorial_remark2.id => editorial_remark2, + editorial_remark3.id => editorial_remark3, + } + + actual_remarks = document.remarks_by_ids([editorial_remark1.id, editorial_remark2.id, editorial_remark3.id]) + + assert_equal actual_remarks, expected_remarks + end + end + + describe "#decorated_edition_versions_by_ids" do + it "returns all the versions, presented as VersionDecorator items" do + document = create(:document) + edition1 = create(:published_edition, document: document) + edition2 = create(:published_edition, document: document) + edition3 = create(:published_edition, document: document) + + version1 = create(:version, item: edition1, item_type: "Edition", state: "published") + version2 = create(:version, item: edition2, item_type: "Edition", state: "published") + version3 = create(:version, item: edition3, item_type: "Edition", state: "published") + _excluded_version = create(:version, item: edition3) + _superseded_version = create(:version, item: edition2, state: "superseded") + + versions = [version1, version2, version3] + + version1_stub = stub(id: version1.id) + version2_stub = stub(id: version2.id) + version3_stub = stub(id: version3.id) + + VersionDecorator.stubs(:new).with { |v, **args| + v.id == version1.id && + args[:is_first_edition] == true && + args[:previous_version] == version3 + }.returns(version1_stub) + + VersionDecorator.stubs(:new).with { |v, **args| + v.id == version2.id && + args[:is_first_edition] == false && + args[:previous_version] == version1 + }.returns(version2_stub) + + VersionDecorator.stubs(:new).with { |v, **args| + v.id == version3.id && + args[:is_first_edition] == false && + args[:previous_version] == version2 + }.returns(version3_stub) + + all_versions = document.decorated_edition_versions_by_ids( + versions.map(&:id), + ) + + expected_versions = { + version1.id => version1_stub, + version2.id => version2_stub, + version3.id => version3_stub, + } + + assert_equal all_versions.count, versions.count + assert_equal all_versions.to_h, expected_versions + end + end + + describe "#first_edition_id" do + it "returns the ID of the first edition" do + document = create(:document) + edition1 = create(:published_edition, document: document) + _edition2 = create(:published_edition, document: document) + + assert_equal document.first_edition_id, edition1.id + end + end + + describe "#active_edition_versions" do + it "returns only active edition versions" do + document = create(:document) + edition1 = create(:published_edition, document: document) + edition2 = create(:published_edition, document: document) + + create(:version, item: edition1, item_type: "Edition", state: "published") + create(:version, item: edition1, item_type: "Edition", state: "published") + create(:version, item: edition2, item_type: "Edition", state: "published") + + expected_versions = [ + *edition1.versions, + *edition2.versions, + ] + + _superseded_version = create(:version, item: edition2, state: "superseded") + _document_version = create(:version, item: document) + + actual_versions = document.active_edition_versions + + assert_same_elements expected_versions, actual_versions + end + end end diff --git a/test/unit/app/queries/document/paginated_timeline_query_test.rb b/test/unit/app/queries/document/paginated_timeline_query_test.rb index 290cef802f0..4ff2a0fd2f2 100644 --- a/test/unit/app/queries/document/paginated_timeline_query_test.rb +++ b/test/unit/app/queries/document/paginated_timeline_query_test.rb @@ -63,44 +63,6 @@ class Document::PaginatedTimelineQueryTest < ActiveSupport::TestCase end end - describe "#remarks" do - it "returns all the remarks keyed by ID" do - query = Document::PaginatedTimelineQuery.new(document: @document, page: 1) - expected_remarks = { - @editorial_remark1.id => @editorial_remark1, - @editorial_remark2.id => @editorial_remark2, - @editorial_remark3.id => @editorial_remark3, - } - - assert_equal query.remarks, expected_remarks - end - end - - describe "#versions" do - it "returns all the versions, presented as VersionDecorator items" do - page1_query = Document::PaginatedTimelineQuery.new(document: @document, page: 1) - page2_query = Document::PaginatedTimelineQuery.new(document: @document, page: 2) - - versions = @document.edition_versions.where.not(state: "superseded") - version_presenter_mocks = [] - - versions.each do |version| - version_decorator_mock = mock("VersionDecorator", id: version.id) - VersionDecorator.stubs(:new).with { |v, **_args| - v.id == version.id - }.returns(version_decorator_mock) - version_presenter_mocks.push(version_presenter_mock) - end - - all_versions = [*page1_query.versions, *page2_query.versions] - - expected_versions = version_presenter_mocks.index_by(&:id) - - assert_equal all_versions.count, versions.count - assert_equal all_versions.to_h, expected_versions - end - end - def seed_document_event_history acting_as(@user) do @document = create(:document) From e1cd19758549fe26b0bf630520929df5b3d9fe8f Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 16 Dec 2024 13:58:55 +0000 Subject: [PATCH 03/10] Add a `HostContentUpdateEvent` model This allows us to define and fetch when a piece of host content has been updated for a document and a date window. --- app/models/host_content_update_event.rb | 24 ++++++ .../models/host_content_update_event_test.rb | 77 +++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 app/models/host_content_update_event.rb create mode 100644 test/unit/app/models/host_content_update_event_test.rb diff --git a/app/models/host_content_update_event.rb b/app/models/host_content_update_event.rb new file mode 100644 index 00000000000..6b7f953ea95 --- /dev/null +++ b/app/models/host_content_update_event.rb @@ -0,0 +1,24 @@ +class HostContentUpdateEvent < Data.define(:author, :created_at, :content_id, :content_title) + def self.all_for_date_window(document:, from:, to:) + events = Services.publishing_api.get_events_for_content_id(document.content_id, { + action: "HostContentUpdateJob", + from:, + to:, + }) + + events.map do |event| + HostContentUpdateEvent.new( + author: get_user_for_uuid(event["payload"]["source_block"]["updated_by_user_uid"]), + created_at: Time.zone.parse(event["created_at"]), + content_id: event["payload"]["source_block"]["content_id"], + content_title: event["payload"]["source_block"]["title"], + ) + end + end + + private + + def self.get_user_for_uuid(uuid) + User.find_by(uid: uuid) + end +end diff --git a/test/unit/app/models/host_content_update_event_test.rb b/test/unit/app/models/host_content_update_event_test.rb new file mode 100644 index 00000000000..80afb3b528f --- /dev/null +++ b/test/unit/app/models/host_content_update_event_test.rb @@ -0,0 +1,77 @@ +require "test_helper" + +class HostContentUpdateEventTest < ActiveSupport::TestCase + extend Minitest::Spec::DSL + + let(:document) { create(:document) } + let(:user) { create(:user) } + + describe ".all_for_date_window" do + it "returns all HostContentUpdateJobs" do + from = Time.zone.now - 2.months + to = Time.zone.now - 1.month + + Services.publishing_api.expects(:get_events_for_content_id).with( + document.content_id, { + action: "HostContentUpdateJob", + from:, + to:, + } + ).returns( + [ + { + "id" => 1593, + "action" => "HostContentUpdateJob", + "created_at" => "2024-01-01T00:00:00.000Z", + "updated_at" => "2024-01-01T00:00:00.000Z", + "request_id" => SecureRandom.uuid, + "content_id" => document.content_id, + "payload" => { + "title" => "Host content updated by content block update", + "locale" => "en", + "content_id" => document.content_id, + "source_block" => { + "title" => "An exciting piece of content", + "content_id" => "ef224ae6-7a81-4c59-830b-e9884fe57ec8", + "updated_by_user_uid" => user.uid, + }, + }, + }, + { + "id" => 1593, + "action" => "HostContentUpdateJob", + "user_uid" => SecureRandom.uuid, + "created_at" => "2023-12-01T00:00:00.000Z", + "updated_at" => "2023-12-01T00:00:00.000Z", + "request_id" => SecureRandom.uuid, + "content_id" => document.content_id, + "payload" => { + "title" => "Host content updated by content block update", + "locale" => "en", + "content_id" => document.content_id, + "source_block" => { + "title" => "Another exciting piece of content", + "content_id" => "5c5520ce-6677-4a76-bd6e-4515f46a804e", + "updated_by_user_uid" => nil, + }, + }, + }, + ], + ) + + result = HostContentUpdateEvent.all_for_date_window(document:, from:, to:) + + assert_equal result.count, 2 + + assert_equal result.first.author, user + assert_equal result.first.created_at, Time.zone.parse("2024-01-01T00:00:00.000Z") + assert_equal result.first.content_id, "ef224ae6-7a81-4c59-830b-e9884fe57ec8" + assert_equal result.first.content_title, "An exciting piece of content" + + assert_nil result.second.author + assert_equal result.second.created_at, Time.zone.parse("2023-12-01T00:00:00.000Z") + assert_equal result.second.content_id, "5c5520ce-6677-4a76-bd6e-4515f46a804e" + assert_equal result.second.content_title, "Another exciting piece of content" + end + end +end From 00332b75705a4a3e357b6e9221ca430b6e439a43 Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 16 Dec 2024 14:45:44 +0000 Subject: [PATCH 04/10] Refactor PaginatedTimelineTest This uses the Minitest Spec DSL to make the tests easier to follow --- .../document/paginated_timeline_test.rb | 213 ++++++++++-------- 1 file changed, 114 insertions(+), 99 deletions(-) diff --git a/test/unit/app/models/document/paginated_timeline_test.rb b/test/unit/app/models/document/paginated_timeline_test.rb index c7b792241ed..6259f388e56 100644 --- a/test/unit/app/models/document/paginated_timeline_test.rb +++ b/test/unit/app/models/document/paginated_timeline_test.rb @@ -1,66 +1,60 @@ require "test_helper" class PaginatedTimelineTest < ActiveSupport::TestCase + extend Minitest::Spec::DSL + setup do @user = create(:departmental_editor) @user2 = create(:departmental_editor) + HostContentUpdateEvent.stubs(:all_for_date_window).returns([]) seed_document_event_history end - test "#entries are ordered newest first (reverse chronological order)" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - entry_timestamps = timeline.entries.map(&:created_at) - assert_equal entry_timestamps.sort.reverse, entry_timestamps - end - - test "#entries is a list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - assert_equal [VersionDecorator, EditorialRemark].to_set, - timeline.entries.map(&:class).to_set - end + describe "#entries" do + it "orders newest first (reverse chronological order)" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + entry_timestamps = timeline.entries.map(&:created_at) + assert_equal entry_timestamps.sort.reverse, entry_timestamps + end - test "#total_count counts the list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - assert_equal 11, timeline.total_count - end + it "is is a list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + assert_equal [VersionDecorator, EditorialRemark].to_set, + timeline.entries.map(&:class).to_set + end - test "#entries are paginated correctly" do - expect_total_count = 11 - results_per_page = 4 - expect_total_pages = 3 + it "paginates correctly" do + expect_total_count = 11 + results_per_page = 4 + expect_total_pages = 3 + + # Get paginated results + paginated_results = nil + mock_pagination(per_page: results_per_page) do + paginated_results = (1..expect_total_pages).map do |page| + Document::PaginatedTimeline.new(document: @document, page:).entries + end + end - # Get paginated results - paginated_results = nil - mock_pagination(per_page: results_per_page) do - paginated_results = (1..expect_total_pages).map do |page| - Document::PaginatedTimeline.new(document: @document, page:).entries + # Get unpaginated results by setting per_page to the total count + unpaginated_results = mock_pagination(per_page: expect_total_count) do + Document::PaginatedTimeline.new(document: @document, page: 1).entries end - end - # Get unpaginated results by setting per_page to the total count - unpaginated_results = mock_pagination(per_page: expect_total_count) do - Document::PaginatedTimeline.new(document: @document, page: 1).entries + # Compare unpaginated results to paginated results + assert_equal unpaginated_results.each_slice(results_per_page).to_a, paginated_results end - # Compare unpaginated results to paginated results - assert_equal unpaginated_results.each_slice(results_per_page).to_a, paginated_results - end - - test "#entries is a list of EditorialRemark objects when 'internal_notes' is passed into the 'only' argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") - expected_entries = [@editorial_remark3, @editorial_remark2, @editorial_remark1] - - assert_equal expected_entries, timeline.entries - end + it "is a list of EditorialRemark objects when 'internal_notes' is passed into the 'only' argument" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") + expected_entries = [@editorial_remark3, @editorial_remark2, @editorial_remark1] - test "#total_count counts the total EditorialRemark objects when 'internal_notes' is passed into the 'only' argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") - assert_equal 3, timeline.total_count - end + assert_equal expected_entries, timeline.entries + end - test "#entries is a list of VersionDecorator objects when 'history' is passed in as a 'only' argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") - expected_actions = %w[updated + it "is a list of VersionDecorator objects when 'history' is passed in as a 'only' argument" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") + expected_actions = %w[updated editioned published submitted @@ -69,26 +63,72 @@ class PaginatedTimelineTest < ActiveSupport::TestCase submitted created] - assert_equal expected_actions, timeline.entries.map(&:action) + assert_equal expected_actions, timeline.entries.map(&:action) + end + + it "correctly determines actions" do + mock_pagination(per_page: 30) do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + entries = timeline.entries.select { |e| e.instance_of?(VersionDecorator) } + expected_actions = %w[updated + editioned + published + submitted + updated + rejected + submitted + created] + + assert_equal expected_actions, entries.map(&:action) + end + end + + it "correctly determines actors" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + entries = timeline.entries.select { |e| e.instance_of?(VersionDecorator) } + expected_actors = [@user, + @user, + @user2, + @user, + @user, + @user2, + @user] + + assert_equal expected_actors, entries.map(&:actor) + end end - test "#total_count counts the total VersionDecorator objects when 'history' is passed into the 'only' argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") - assert_equal 8, timeline.total_count + describe "#total_count" do + it "counts the list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + assert_equal 11, timeline.total_count + end + + it "counts the total EditorialRemark objects when 'internal_notes' is passed into the 'only' argument" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") + assert_equal 3, timeline.total_count + end + + it "counts the total VersionDecorator objects when 'history' is passed into the 'only' argument" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") + assert_equal 8, timeline.total_count + end end - test "#only is the 'only' constructor argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - assert_nil timeline.only + describe "#only" do + it "is the 'only' constructor argument" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + assert_nil timeline.only - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") - assert_equal "history", timeline.only + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") + assert_equal "history", timeline.only - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") - assert_equal "internal_notes", timeline.only + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") + assert_equal "internal_notes", timeline.only + end end - test "it implements methods required by Kaminari for pagination" do + it "implements methods required by Kaminari for pagination" do results_per_page = 4 expect_total_count = 11 expect_total_pages = 3 @@ -110,56 +150,31 @@ class PaginatedTimelineTest < ActiveSupport::TestCase end end - test "VersionDecorator correctly determines actions" do - mock_pagination(per_page: 30) do + describe "#entries_on_newer_editions" do + it "returns entries on newer editions than the one passed in" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - entries = timeline.entries.select { |e| e.instance_of?(VersionDecorator) } - expected_actions = %w[updated - editioned - published - submitted - updated - rejected - submitted - created] + expected_entries = timeline.entries.slice(1, 2) - assert_equal expected_actions, entries.map(&:action) + assert_equal expected_entries, timeline.entries_on_newer_editions(@first_edition) end end + + describe "#entries_on_current_edition" do + it "returns entries for the edition passed in" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + expected_entries = timeline.entries - timeline.entries.slice(1, 2) - test "VersionDecorator correctly determines actors" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - entries = timeline.entries.select { |e| e.instance_of?(VersionDecorator) } - expected_actors = [@user, - @user, - @user2, - @user, - @user, - @user2, - @user] - - assert_equal expected_actors, entries.map(&:actor) - end - - test "#entries_on_newer_editions returns entries on newer editions than the one passed in" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - expected_entries = timeline.entries.slice(1, 2) - - assert_equal expected_entries, timeline.entries_on_newer_editions(@first_edition) - end - - test "#entries_on_current_edition returns entries for the edition passed in" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - expected_entries = timeline.entries - timeline.entries.slice(1, 2) - - assert_equal expected_entries, timeline.entries_on_current_edition(@first_edition) + assert_equal expected_entries, timeline.entries_on_current_edition(@first_edition) + end end - test "#entries_on_previous_editions returns entries on previous editions" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - expected_entries = timeline.entries - timeline.entries.slice(1, 2) + describe "#entries_on_previous_editions" do + it "returns entries on previous editions" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + expected_entries = timeline.entries - timeline.entries.slice(1, 2) - assert_equal expected_entries, timeline.entries_on_previous_editions(@newest_edition) + assert_equal expected_entries, timeline.entries_on_previous_editions(@newest_edition) + end end def seed_document_event_history From 5b44220e6a7f1c914ce6eb3636a1b4c79dbf7171 Mon Sep 17 00:00:00 2001 From: pezholio Date: Mon, 16 Dec 2024 15:13:58 +0000 Subject: [PATCH 05/10] Include events in paginated timeline --- app/models/document/paginated_timeline.rb | 34 +++- features/support/publishing_api.rb | 1 + test/factories/host_content_update_events.rb | 12 ++ test/test_helper.rb | 1 + .../document/paginated_timeline_test.rb | 157 +++++++++++++++--- 5 files changed, 182 insertions(+), 23 deletions(-) create mode 100644 test/factories/host_content_update_events.rb diff --git a/app/models/document/paginated_timeline.rb b/app/models/document/paginated_timeline.rb index 80f563b41bf..c4ee3862cab 100644 --- a/app/models/document/paginated_timeline.rb +++ b/app/models/document/paginated_timeline.rb @@ -13,7 +13,7 @@ def entries remarks = document.remarks_by_ids(query.remark_ids) versions = document.decorated_edition_versions_by_ids(query.version_ids) - raw_entries.map do |entry| + versions_and_remarks = raw_entries.map do |entry| case entry.model when "Version" versions.fetch(entry.id) @@ -21,6 +21,12 @@ def entries remarks.fetch(entry.id) end end + + if only.present? + versions_and_remarks + else + [*versions_and_remarks, *host_content_update_events].sort_by(&:created_at).reverse! + end end end @@ -89,4 +95,30 @@ def prev_page false end end + +private + + def next_page_entries + if next_page + Document::PaginatedTimelineQuery.new(document:, page: next_page, only:).raw_entries + end + end + + def host_content_update_events + return [] if query.raw_entries.empty? + + @host_content_update_events ||= HostContentUpdateEvent.all_for_date_window( + document:, + from: date_window.last, + to: date_window.first, + ) + end + + def date_window + @date_window ||= begin + start = page == 1 ? Time.zone.now : query.raw_entries.first.created_at + ends = next_page_entries ? next_page_entries.first.created_at : query.raw_entries.last.created_at + (start.to_time.round.utc..ends.to_time.round.utc) + end + end end diff --git a/features/support/publishing_api.rb b/features/support/publishing_api.rb index cc68cb9ab98..c284d67e7da 100644 --- a/features/support/publishing_api.rb +++ b/features/support/publishing_api.rb @@ -14,6 +14,7 @@ GdsApi::PublishingApi.any_instance.stubs(:put_content) GdsApi::PublishingApi.any_instance.stubs(:patch_links) GdsApi::PublishingApi.any_instance.stubs(:unpublish) + GdsApi::PublishingApi.any_instance.stubs(:get_events_for_content_id).returns([]) need1 = { "content_id" => SecureRandom.uuid, diff --git a/test/factories/host_content_update_events.rb b/test/factories/host_content_update_events.rb new file mode 100644 index 00000000000..c59620bf1ad --- /dev/null +++ b/test/factories/host_content_update_events.rb @@ -0,0 +1,12 @@ +FactoryBot.define do + factory :host_content_update_event do + author { create(:user) } + created_at { Time.zone.now } + content_id { SecureRandom.uuid } + content_title { "Some title" } + + initialize_with do + new(author:, created_at:, content_id:, content_title:) + end + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 90f5bbf431c..f595527b719 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -69,6 +69,7 @@ class ActiveSupport::TestCase Sidekiq::Job.clear_all stub_any_publishing_api_call stub_publishing_api_publish_intent + Services.publishing_api.stubs(:get_events_for_content_id).returns([]) Services.stubs(:asset_manager).returns(stub_everything("asset-manager")) end diff --git a/test/unit/app/models/document/paginated_timeline_test.rb b/test/unit/app/models/document/paginated_timeline_test.rb index 6259f388e56..628a710c66b 100644 --- a/test/unit/app/models/document/paginated_timeline_test.rb +++ b/test/unit/app/models/document/paginated_timeline_test.rb @@ -17,7 +17,7 @@ class PaginatedTimelineTest < ActiveSupport::TestCase assert_equal entry_timestamps.sort.reverse, entry_timestamps end - it "is is a list of VersionDecorator and EditorialRemark objects when no 'only' argument is passed in" do + it "is is a list of VersionDecorator and EditorialRemark objects when no 'only' argument is not passed in" do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) assert_equal [VersionDecorator, EditorialRemark].to_set, timeline.entries.map(&:class).to_set @@ -45,27 +45,6 @@ class PaginatedTimelineTest < ActiveSupport::TestCase assert_equal unpaginated_results.each_slice(results_per_page).to_a, paginated_results end - it "is a list of EditorialRemark objects when 'internal_notes' is passed into the 'only' argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") - expected_entries = [@editorial_remark3, @editorial_remark2, @editorial_remark1] - - assert_equal expected_entries, timeline.entries - end - - it "is a list of VersionDecorator objects when 'history' is passed in as a 'only' argument" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") - expected_actions = %w[updated - editioned - published - submitted - updated - rejected - submitted - created] - - assert_equal expected_actions, timeline.entries.map(&:action) - end - it "correctly determines actions" do mock_pagination(per_page: 30) do timeline = Document::PaginatedTimeline.new(document: @document, page: 1) @@ -96,6 +75,140 @@ class PaginatedTimelineTest < ActiveSupport::TestCase assert_equal expected_actors, entries.map(&:actor) end + + describe "when there are HostContentUpdateEvents present" do + let(:host_content_update_events) { build_list(:host_content_update_event, 3) } + + before do + HostContentUpdateEvent.stubs(:all_for_date_window).returns(host_content_update_events) + end + + it "orders newest first (reverse chronological order)" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + entry_timestamps = timeline.entries.flatten.map(&:created_at) + assert_equal entry_timestamps.sort.reverse, entry_timestamps + end + + it "is is a list of VersionDecorator, EditorialRemark and HostContentUpdateEvent objects when no 'only' argument is passed in" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + assert_equal [VersionDecorator, EditorialRemark, HostContentUpdateEvent].to_set, + timeline.entries.map(&:class).to_set + end + + it "includes all of the HostContentUpdateEvent objects" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + + host_content_update_events.each do |event| + assert_includes timeline.entries, event + end + end + + describe "fetching HostContentUpdateEvents" do + before do + HostContentUpdateEvent.reset_mocha + end + + describe "when on the first page" do + it "requests a date window from the first created_at date of the next page to the current datetime" do + Timecop.freeze do + page2_entries = Document::PaginatedTimeline.new(document: @document, page: 2).query.raw_entries + + HostContentUpdateEvent.expects(:all_for_date_window).with( + document: @document, + from: page2_entries.first.created_at.to_time.utc, + to: Time.zone.now.to_time.round.utc, + ).at_least_once.returns(host_content_update_events) + + Document::PaginatedTimeline.new(document: @document, page: 1).entries + end + end + + describe "when there are no results" do + it "does not request any events" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + timeline.query.expects(:raw_entries).at_least_once.returns([]) + + HostContentUpdateEvent.expects(:all_for_date_window).never + + timeline.entries + end + end + end + + describe "when not on the first page" do + it "requests a window between the first created_at date of the next page to the first created_at date of the current page" do + mock_pagination(per_page: 3) do + page2_entries = Document::PaginatedTimeline.new(document: @document, page: 2).query.raw_entries + page3_entries = Document::PaginatedTimeline.new(document: @document, page: 3).query.raw_entries + + HostContentUpdateEvent.expects(:all_for_date_window).with( + document: @document, + from: page3_entries.first.created_at.to_time.utc, + to: page2_entries.first.created_at.to_time.utc, + ).at_least_once.returns(host_content_update_events) + + Document::PaginatedTimeline.new(document: @document, page: 2).entries + end + end + end + + describe "when on the last page" do + it "fetches with the created_at date for the last item" do + Timecop.freeze do + mock_pagination(per_page: 30) do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1) + timeline_entries = timeline.query.raw_entries + + HostContentUpdateEvent.expects(:all_for_date_window).with( + document: @document, + from: timeline_entries.last.created_at.to_time.utc, + to: timeline_entries.first.created_at.to_time.utc, + ).at_least_once.returns(host_content_update_events) + + timeline.entries + end + end + end + end + end + end + + describe "when only argument is set to history" do + it "is a list of VersionDecorator objects" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "history") + expected_actions = %w[updated + editioned + published + submitted + updated + rejected + submitted + created] + + assert_equal expected_actions, timeline.entries.map(&:action) + end + + it "does not fetch any HostContentUpdateEvents" do + HostContentUpdateEvent.expects(:all_for_date_window).never + + Document::PaginatedTimeline.new(document: @document, page: 1, only: "history").entries + end + end + + describe "when only argument is set to internal_notes" do + it "is a list of EditorialRemark objects when 'internal_notes'" do + timeline = Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes") + expected_entries = [@editorial_remark3, @editorial_remark2, @editorial_remark1] + + assert_equal expected_entries, timeline.entries + end + + it "does not fetch any HostContentUpdateEvents" do + HostContentUpdateEvent.expects(:all_for_date_window).never + + Document::PaginatedTimeline.new(document: @document, page: 1, only: "internal_notes").entries + end + end end describe "#total_count" do From da9ce3962195fdc045bf2d61cdbd2abc9bcdecd1 Mon Sep 17 00:00:00 2001 From: pezholio Date: Wed, 18 Dec 2024 10:48:52 +0000 Subject: [PATCH 06/10] Add methods to get when events occured on editions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ll need this to be able to work out what upstream changed apply to what edition, as Publishing API doesn’t have the concept of “editions” --- app/models/concerns/edition/workflow.rb | 8 ++++ test/unit/app/models/edition/workflow_test.rb | 42 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/app/models/concerns/edition/workflow.rb b/app/models/concerns/edition/workflow.rb index b75b4f3a5b2..d3548653cf8 100644 --- a/app/models/concerns/edition/workflow.rb +++ b/app/models/concerns/edition/workflow.rb @@ -119,6 +119,14 @@ def has_workflow? true end + def superseded_at + versions.find { |v| v.state == "superseded" }&.created_at + end + + def published_at + versions.find { |v| v.state == "published" }&.created_at + end + private def destroy_associations_with_edition_dependencies_and_dependants diff --git a/test/unit/app/models/edition/workflow_test.rb b/test/unit/app/models/edition/workflow_test.rb index f55c97a6282..65121a1c9da 100644 --- a/test/unit/app/models/edition/workflow_test.rb +++ b/test/unit/app/models/edition/workflow_test.rb @@ -192,4 +192,46 @@ class Edition::WorkflowTest < ActiveSupport::TestCase edition = create(:publication) assert edition.has_workflow? end + + test "#superseded_at returns the date an edition was superseded at when a superseded version exists" do + superseded_at = Time.zone.now - 3.days + + edition = build(:edition) + edition.versions = [ + build(:version, state: "published", item: edition), + build(:version, state: "superseded", created_at: superseded_at, item: edition), + ] + + assert_equal edition.superseded_at, superseded_at + end + + test "#superseded_at returns nil when a superseded version does not exist" do + edition = build(:edition) + edition.versions = [ + build(:version, state: "published", item: edition), + ] + + assert_nil edition.superseded_at + end + + test "#published_at returns the date an edition was published at when a published version exists" do + published_at = Time.zone.now - 3.days + + edition = build(:edition) + edition.versions = [ + build(:version, state: "published", created_at: published_at, item: edition), + build(:version, state: "superseded", item: edition), + ] + + assert_equal edition.published_at, published_at + end + + test "#published_at returns nil when a published version does not exist" do + edition = build(:edition) + edition.versions = [ + build(:version, state: "created", item: edition), + ] + + assert_nil edition.published_at + end end From d99f6dbd4a01f592274df131b3e1efe3d2f7a599 Mon Sep 17 00:00:00 2001 From: pezholio Date: Wed, 18 Dec 2024 10:50:13 +0000 Subject: [PATCH 07/10] Add history helpers to versions and remarks This will be used in the paginated timeline in a future commit --- app/decorators/version_decorator.rb | 14 ++++- app/models/editorial_remark.rb | 12 ++++ .../app/decorators/version_decorator_test.rb | 39 +++++++++++- test/unit/app/models/editorial_remark_test.rb | 62 ++++++++++++++++--- 4 files changed, 116 insertions(+), 11 deletions(-) diff --git a/app/decorators/version_decorator.rb b/app/decorators/version_decorator.rb index 377e5c869e4..11d11311c7c 100644 --- a/app/decorators/version_decorator.rb +++ b/app/decorators/version_decorator.rb @@ -24,7 +24,19 @@ def action end end - private + def is_for_newer_edition?(edition) + item_id > edition.id + end + + def is_for_current_edition?(edition) + item_id == edition.id + end + + def is_for_older_edition?(edition) + item_id < edition.id + end + +private def previous_version # we can avoid n+1 queries by using our preloaded_prev_version diff --git a/app/models/editorial_remark.rb b/app/models/editorial_remark.rb index 7520d5bf28a..1f5b93f760c 100644 --- a/app/models/editorial_remark.rb +++ b/app/models/editorial_remark.rb @@ -3,4 +3,16 @@ class EditorialRemark < ApplicationRecord belongs_to :author, class_name: "User" validates :edition, :body, :author, presence: true + + def is_for_newer_edition?(edition) + edition_id > edition.id + end + + def is_for_current_edition?(edition) + edition_id == edition.id + end + + def is_for_older_edition?(edition) + edition_id < edition.id + end end diff --git a/test/unit/app/decorators/version_decorator_test.rb b/test/unit/app/decorators/version_decorator_test.rb index 2f2a6eb1200..a4d2e85ef7f 100644 --- a/test/unit/app/decorators/version_decorator_test.rb +++ b/test/unit/app/decorators/version_decorator_test.rb @@ -4,7 +4,8 @@ class VersionDecoratorTest < ActiveSupport::TestCase extend Minitest::Spec::DSL let(:user) { build(:user) } - let(:edition) { build(:edition) } + let(:item_id) { 5 } + let(:edition) { build(:edition, id: item_id) } let(:version) { build(:version, user:, item: edition) } let(:is_first_edition) { false } let(:previous_version) { build(:version, user:, item: edition) } @@ -78,4 +79,40 @@ class VersionDecoratorTest < ActiveSupport::TestCase end end end + + describe "#is_for_newer_edition?" do + it "returns true if the version's item_id is less than the edition's id" do + edition_to_compare = build(:edition, id: item_id - 1) + assert decorator.is_for_newer_edition?(edition_to_compare) + end + + it "returns false if the version's item_id is greater than the edition's id" do + edition_to_compare = build(:edition, id: item_id + 1) + assert_not decorator.is_for_newer_edition?(edition_to_compare) + end + end + + describe "#is_for_current_edition?" do + it "returns true if the version's item_id is equal to the edition's id" do + edition_to_compare = build(:edition, id: item_id) + assert decorator.is_for_current_edition?(edition_to_compare) + end + + it "returns true if the version's item_id is not equal to the edition's id" do + edition_to_compare = build(:edition, id: item_id + 1) + assert_not decorator.is_for_current_edition?(edition_to_compare) + end + end + + describe "#is_for_older_edition?" do + it "returns true if the version's item_id is greater than the edition's id" do + edition_to_compare = build(:edition, id: item_id + 1) + assert decorator.is_for_older_edition?(edition_to_compare) + end + + it "returns false if the version's item_id is less than the edition's id" do + edition_to_compare = build(:edition, id: item_id - 1) + assert_not decorator.is_for_older_edition?(edition_to_compare) + end + end end diff --git a/test/unit/app/models/editorial_remark_test.rb b/test/unit/app/models/editorial_remark_test.rb index 2c31ad2daca..f6b28d7a646 100644 --- a/test/unit/app/models/editorial_remark_test.rb +++ b/test/unit/app/models/editorial_remark_test.rb @@ -1,18 +1,62 @@ require "test_helper" class EditorialRemarkTest < ActiveSupport::TestCase - test "should be invalid without a edition" do - editorial_remark = build(:editorial_remark, edition: nil) - assert_not editorial_remark.valid? + extend Minitest::Spec::DSL + + let(:edition_id) { 5 } + let(:edition) { build(:edition, id: edition_id) } + let(:editorial_remark) { build(:editorial_remark, edition:) } + + describe "#valid?" do + it "should be invalid without a edition" do + editorial_remark.edition = nil + assert_not editorial_remark.valid? + end + + it "should be invalid without a body" do + editorial_remark.body = nil + assert_not editorial_remark.valid? + end + + it "should be invalid without an author" do + editorial_remark.author = nil + assert_not editorial_remark.valid? + end end - test "should be invalid without a body" do - editorial_remark = build(:editorial_remark, body: nil) - assert_not editorial_remark.valid? + describe "#is_for_newer_edition?" do + it "returns true if the edition_id is less than the comparing edition's id" do + edition_to_compare = build(:edition, id: edition_id - 1) + assert editorial_remark.is_for_newer_edition?(edition_to_compare) + end + + it "returns false if the edition_id is greater than the comparing edition's id" do + edition_to_compare = build(:edition, id: edition_id + 1) + assert_not editorial_remark.is_for_newer_edition?(edition_to_compare) + end end - test "should be invalid without an author" do - editorial_remark = build(:editorial_remark, author: nil) - assert_not editorial_remark.valid? + describe "#is_for_current_edition?" do + it "returns true if the edition_id is equal to the comparing edition's id" do + edition_to_compare = build(:edition, id: edition_id) + assert editorial_remark.is_for_current_edition?(edition_to_compare) + end + + it "returns false if the edition_id is not equal to the comparing edition's id" do + edition_to_compare = build(:edition, id: edition_id + 1) + assert_not editorial_remark.is_for_current_edition?(edition_to_compare) + end + end + + describe "#is_for_older_edition?" do + it "returns true if the edition_id is greater than the comparing edition's id" do + edition_to_compare = build(:edition, id: edition_id + 1) + assert editorial_remark.is_for_older_edition?(edition_to_compare) + end + + it "returns true if the edition_id is less than the comparing edition's id" do + edition_to_compare = build(:edition, id: edition_id - 1) + assert_not editorial_remark.is_for_older_edition?(edition_to_compare) + end end end From 8d4a7845924ca91e6eb131be05de3c9f166b3e07 Mon Sep 17 00:00:00 2001 From: pezholio Date: Wed, 18 Dec 2024 10:58:25 +0000 Subject: [PATCH 08/10] Add timeline helpers to the HostContentUpdateEvent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Working out what event “belongs” to an edition is tricky, as Publishing API doesn’t have the concept of editions. With this in mind we have a bunch of logic: - If an edition is superseded and the event happened after the edition was superseded, it’s for a newer edition - If an edition is published, and the event happened after the edition was published AND the event is not for a newer edition, it’s for the current edition - If neither of the above is true, it’s for an older edition --- app/models/host_content_update_event.rb | 12 ++- .../models/host_content_update_event_test.rb | 85 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/app/models/host_content_update_event.rb b/app/models/host_content_update_event.rb index 6b7f953ea95..1265f5e15d6 100644 --- a/app/models/host_content_update_event.rb +++ b/app/models/host_content_update_event.rb @@ -16,7 +16,17 @@ def self.all_for_date_window(document:, from:, to:) end end - private + def is_for_newer_edition?(edition) + edition.superseded? && created_at.after?(edition.superseded_at) + end + + def is_for_current_edition?(edition) + edition.published_at && created_at.after?(edition.published_at) && !is_for_newer_edition?(edition) + end + + def is_for_older_edition?(edition) + !is_for_newer_edition?(edition) && !is_for_current_edition?(edition) + end def self.get_user_for_uuid(uuid) User.find_by(uid: uuid) diff --git a/test/unit/app/models/host_content_update_event_test.rb b/test/unit/app/models/host_content_update_event_test.rb index 80afb3b528f..dafa2b696ec 100644 --- a/test/unit/app/models/host_content_update_event_test.rb +++ b/test/unit/app/models/host_content_update_event_test.rb @@ -74,4 +74,89 @@ class HostContentUpdateEventTest < ActiveSupport::TestCase assert_equal result.second.content_title, "Another exciting piece of content" end end + + describe "Timeline helpers" do + let(:created_at) { Time.zone.now - 1.month } + let(:host_content_update_event) { build(:host_content_update_event, created_at:) } + + describe "when the edition is published" do + let(:edition) { build(:edition) } + + before do + edition.stubs(:published_at).returns(created_at - 12.months) + end + + describe "when the event occurred after the edition was superseded" do + before do + edition.stubs(:superseded?).returns(true) + edition.stubs(:superseded_at).returns(created_at - 2.days) + end + + describe "#is_for_newer_edition?" do + it "returns true" do + assert host_content_update_event.is_for_newer_edition?(edition) + end + end + + describe "#is_for_current_edition?" do + it "returns false" do + assert_not host_content_update_event.is_for_current_edition?(edition) + end + end + + describe "#is_for_older_edition?" do + it "returns false" do + assert_not host_content_update_event.is_for_older_edition?(edition) + end + end + end + + describe "when the event occurred before the edition was superseded" do + before do + edition.stubs(:superseded?).returns(true) + edition.stubs(:superseded_at).returns(created_at + 2.days) + end + + describe "#is_for_newer_edition?" do + it "returns false" do + assert_not host_content_update_event.is_for_newer_edition?(edition) + end + end + + describe "#is_for_current_edition?" do + it "returns true" do + assert host_content_update_event.is_for_current_edition?(edition) + end + end + + describe "#is_for_older_edition?" do + it "returns false" do + assert_not host_content_update_event.is_for_older_edition?(edition) + end + end + end + end + + describe "when the edition is draft" do + let(:edition) { build(:edition, :draft) } + + describe "#is_for_newer_edition?" do + it "returns false" do + assert_not host_content_update_event.is_for_newer_edition?(edition) + end + end + + describe "#is_for_current_edition?" do + it "returns false" do + assert_not host_content_update_event.is_for_current_edition?(edition) + end + end + + describe "#is_for_older_edition?" do + it "returns true" do + assert host_content_update_event.is_for_older_edition?(edition) + end + end + end + end end From 090f1d6e6ff6786e4ead3373eb4daf98543cc5d5 Mon Sep 17 00:00:00 2001 From: pezholio Date: Wed, 18 Dec 2024 11:38:34 +0000 Subject: [PATCH 09/10] Use new methods in PaginatedTimeline This calls the new methods on each entry, simplifying the code, and allowing us to stub the response, rather than relying on the objects set up in `seed_document_event_history` --- app/models/document/paginated_timeline.rb | 24 +----- .../document/paginated_timeline_test.rb | 81 ++++++++++++++----- 2 files changed, 66 insertions(+), 39 deletions(-) diff --git a/app/models/document/paginated_timeline.rb b/app/models/document/paginated_timeline.rb index c4ee3862cab..7993d2dd67a 100644 --- a/app/models/document/paginated_timeline.rb +++ b/app/models/document/paginated_timeline.rb @@ -35,33 +35,15 @@ def query end def entries_on_newer_editions(edition) - @entries_on_newer_editions ||= entries.select do |entry| - if entry.is_a?(EditorialRemark) - entry.edition_id > edition.id - else - entry.item_id > edition.id - end - end + entries.select { |e| e.is_for_newer_edition?(edition) } end def entries_on_current_edition(edition) - @entries_on_current_edition ||= entries.select do |entry| - if entry.is_a?(EditorialRemark) - entry.edition_id == edition.id - else - entry.item_id == edition.id - end - end + entries.select { |e| e.is_for_current_edition?(edition) } end def entries_on_previous_editions(edition) - @entries_on_previous_editions ||= entries.select do |entry| - if entry.is_a?(EditorialRemark) - entry.edition_id < edition.id - else - entry.item_id < edition.id - end - end + entries.select { |e| e.is_for_older_edition?(edition) } end def total_count diff --git a/test/unit/app/models/document/paginated_timeline_test.rb b/test/unit/app/models/document/paginated_timeline_test.rb index 628a710c66b..b8074b1d665 100644 --- a/test/unit/app/models/document/paginated_timeline_test.rb +++ b/test/unit/app/models/document/paginated_timeline_test.rb @@ -263,30 +263,75 @@ class PaginatedTimelineTest < ActiveSupport::TestCase end end - describe "#entries_on_newer_editions" do - it "returns entries on newer editions than the one passed in" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - expected_entries = timeline.entries.slice(1, 2) + describe "filtering entries" do + let(:entries_for_newer_editions) do + 2.times.map do + stub("entry", is_for_newer_edition?: true, is_for_current_edition?: false, is_for_older_edition?: false) + end + end - assert_equal expected_entries, timeline.entries_on_newer_editions(@first_edition) + let(:entries_for_current_edition) do + 4.times.map do + stub("entry", is_for_newer_edition?: false, is_for_current_edition?: true, is_for_older_edition?: false) + end end - end - - describe "#entries_on_current_edition" do - it "returns entries for the edition passed in" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - expected_entries = timeline.entries - timeline.entries.slice(1, 2) - assert_equal expected_entries, timeline.entries_on_current_edition(@first_edition) + let(:entries_for_older_editions) do + 3.times.map do + stub("entry", is_for_newer_edition?: false, is_for_current_edition?: false, is_for_older_edition?: true) + end end - end - describe "#entries_on_previous_editions" do - it "returns entries on previous editions" do - timeline = Document::PaginatedTimeline.new(document: @document, page: 1) - expected_entries = timeline.entries - timeline.entries.slice(1, 2) + let(:all_entries) do + [*entries_for_newer_editions, *entries_for_current_edition, *entries_for_older_editions] + end + + let(:timeline) { Document::PaginatedTimeline.new(document: @document, page: 1) } + + before do + timeline.stubs(:entries).returns(all_entries) + end + + describe "#entries_on_newer_editions" do + it "returns entries on newer editions than the one passed in" do + assert_equal entries_for_newer_editions, timeline.entries_on_newer_editions(@first_edition) + end + + it "calls the entries with the expected edition" do + all_entries.each do |entry| + entry.expects(:is_for_newer_edition?).with(@first_edition) + end + + timeline.entries_on_newer_editions(@first_edition) + end + end + + describe "#entries_on_current_edition" do + it "returns entries for the edition passed in" do + assert_equal entries_for_current_edition, timeline.entries_on_current_edition(@first_edition) + end + + it "calls the entries with the expected edition" do + all_entries.each do |entry| + entry.expects(:is_for_current_edition?).with(@first_edition) + end - assert_equal expected_entries, timeline.entries_on_previous_editions(@newest_edition) + timeline.entries_on_current_edition(@first_edition) + end + end + + describe "#entries_on_previous_editions" do + it "returns entries on previous editions" do + assert_equal entries_for_older_editions, timeline.entries_on_previous_editions(@newest_edition) + end + + it "calls the entries with the expected edition" do + all_entries.each do |entry| + entry.expects(:is_for_older_edition?).with(@first_edition) + end + + timeline.entries_on_previous_editions(@first_edition) + end end end From 297f3b53f4ae23fcac4baea4f97222e92a349a5a Mon Sep 17 00:00:00 2001 From: pezholio Date: Thu, 19 Dec 2024 09:18:51 +0000 Subject: [PATCH 10/10] Add component for HostContentUpdateEvents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’ve also added a lightweight Cucumber test to ensure everything fits together as we expect. I had to make a slight change to one of the steps to ensure we’re always visiting the edit page for the latest edition --- .../views/_host-content-update-event.scss | 20 ++++++++++++ app/assets/stylesheets/application.scss | 1 + .../document_history_tab_component.rb | 2 ++ ...st_content_update_event_component.html.erb | 11 +++++++ .../host_content_update_event_component.rb | 23 +++++++++++++ ...dmin-history-content-block-updates.feature | 17 ++++++++++ .../content_block_update_steps.rb | 13 ++++++++ features/step_definitions/document_steps.rb | 12 ++++++- .../document_history_tab_component_test.rb | 13 ++++++++ ...ost_content_update_event_component_test.rb | 32 +++++++++++++++++++ 10 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 app/assets/stylesheets/admin/views/_host-content-update-event.scss create mode 100644 app/components/admin/editions/host_content_update_event_component.html.erb create mode 100644 app/components/admin/editions/host_content_update_event_component.rb create mode 100644 features/admin-history-content-block-updates.feature create mode 100644 features/step_definitions/content_block_update_steps.rb create mode 100644 test/components/admin/editions/host_content_update_event_component_test.rb diff --git a/app/assets/stylesheets/admin/views/_host-content-update-event.scss b/app/assets/stylesheets/admin/views/_host-content-update-event.scss new file mode 100644 index 00000000000..15e8e392043 --- /dev/null +++ b/app/assets/stylesheets/admin/views/_host-content-update-event.scss @@ -0,0 +1,20 @@ +.app-view-editions-host-content-update-event-entry { + &__list-item { + margin-bottom: govuk-spacing(4); + } + + &__detail { + margin-top: govuk-spacing(0); + margin-bottom: govuk-spacing(0); + } + + &__heading { + margin-bottom: govuk-spacing(1); + } + + &__datetime { + margin-top: govuk-spacing(0); + margin-bottom: govuk-spacing(0); + color: $govuk-secondary-text-colour; + } +} diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 15a3d953592..f69dcfc37b6 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -34,6 +34,7 @@ $govuk-page-width: 1140px; @import "./admin/views/filter"; @import "./admin/views/govspeak-help"; @import "./admin/views/groups-index"; +@import "./admin/views/host-content-update-event"; @import "./admin/views/historical-accounts-index"; @import "./admin/views/organisations-index"; @import "./admin/views/organisations-edit"; diff --git a/app/components/admin/editions/document_history_tab_component.rb b/app/components/admin/editions/document_history_tab_component.rb index 033887c495c..283715292ed 100644 --- a/app/components/admin/editions/document_history_tab_component.rb +++ b/app/components/admin/editions/document_history_tab_component.rb @@ -26,6 +26,8 @@ def entries_on_previous_editions def render_entry(entry) if entry.is_a?(EditorialRemark) render(Admin::Editions::EditorialRemarkComponent.new(editorial_remark: entry)) + elsif entry.is_a?(HostContentUpdateEvent) + render(Admin::Editions::HostContentUpdateEventComponent.new(entry)) else render(Admin::Editions::AuditTrailEntryComponent.new(entry:, edition:)) end diff --git a/app/components/admin/editions/host_content_update_event_component.html.erb b/app/components/admin/editions/host_content_update_event_component.html.erb new file mode 100644 index 00000000000..3f85397a9ac --- /dev/null +++ b/app/components/admin/editions/host_content_update_event_component.html.erb @@ -0,0 +1,11 @@ +
+

Content Block Update

+ +

+ <%= activity %> +

+ +

+ <%= time %> by <%= actor %> +

+
diff --git a/app/components/admin/editions/host_content_update_event_component.rb b/app/components/admin/editions/host_content_update_event_component.rb new file mode 100644 index 00000000000..f224b858300 --- /dev/null +++ b/app/components/admin/editions/host_content_update_event_component.rb @@ -0,0 +1,23 @@ +class Admin::Editions::HostContentUpdateEventComponent < ViewComponent::Base + include ApplicationHelper + + def initialize(event) + @event = event + end + +private + + attr_reader :event + + def activity + "#{event.content_title.strip} updated" + end + + def time + absolute_time(event.created_at) + end + + def actor + event.author ? linked_author(event.author, class: "govuk-link") : "User (removed)" + end +end diff --git a/features/admin-history-content-block-updates.feature b/features/admin-history-content-block-updates.feature new file mode 100644 index 00000000000..a3808cd478e --- /dev/null +++ b/features/admin-history-content-block-updates.feature @@ -0,0 +1,17 @@ +Feature: Showing content block updates in history + + Background: + Given I am an editor + And a published news article "Stubble to be Outlawed" exists + And the document has been updated by a change to the content block "Some email address" + + Scenario: Content block update exists for current edition + When I am on the edit page for news article "Stubble to be Outlawed" + And I click the "History" tab + Then I should see an entry for the content block "Some email address" on the current edition + + Scenario: Content block update exists for a previous edition + When I force publish a new edition of the news article "Stubble to be Outlawed" + And I am on the edit page for news article "Stubble to be Outlawed" + And I click the "History" tab + Then I should see an entry for the content block "Some email address" on the previous edition diff --git a/features/step_definitions/content_block_update_steps.rb b/features/step_definitions/content_block_update_steps.rb new file mode 100644 index 00000000000..662775d0146 --- /dev/null +++ b/features/step_definitions/content_block_update_steps.rb @@ -0,0 +1,13 @@ +And(/^the document has been updated by a change to the content block "([^"]*)"$/) do |content_block_title| + host_content_update_event = build(:host_content_update_event, content_title: content_block_title) + HostContentUpdateEvent.expects(:all_for_date_window).at_least_once.returns([host_content_update_event]) +end + +Then(/^I should see an entry for the content block "([^"]*)" on the (current|previous) edition$/) do |content_block, current_or_previous| + selector = current_or_previous == "current" ? ".app-view-editions__current-edition-entries" : ".app-view-editions__previous-edition-entries" + + within selector do + assert_text "Content Block Update" + assert_text "#{content_block} updated" + end +end diff --git a/features/step_definitions/document_steps.rb b/features/step_definitions/document_steps.rb index 46bbf4b6642..9e006a89b3e 100644 --- a/features/step_definitions/document_steps.rb +++ b/features/step_definitions/document_steps.rb @@ -92,6 +92,16 @@ publish end +When("I force publish a new edition of {edition}") do |edition| + stub_publishing_api_links_with_taxons(edition.content_id, %w[a-taxon-content-id]) + visit_edition_admin edition.title + click_button "Create new edition" + fill_in_change_note_if_required + apply_to_all_nations_if_required + click_button "Save and go to document summary" + publish(force: true) +end + When("I force publish {edition}") do |edition| stub_publishing_api_links_with_taxons(edition.content_id, %w[a-taxon-content-id]) visit_edition_admin edition.title, :draft @@ -139,7 +149,7 @@ When(/^I am on the edit page for (.*?) "(.*?)"$/) do |document_type, title| document_type = document_type.tr(" ", "_") - document = document_type.classify.constantize.find_by(title:) + document = document_type.classify.constantize.where(title:).last visit send("edit_admin_#{document_type}_path", document) end diff --git a/test/components/admin/editions/document_history_tab_component_test.rb b/test/components/admin/editions/document_history_tab_component_test.rb index 7cb766cc04d..7aa70aec28a 100644 --- a/test/components/admin/editions/document_history_tab_component_test.rb +++ b/test/components/admin/editions/document_history_tab_component_test.rb @@ -54,17 +54,22 @@ class Admin::Editions::DocumentHistoryTabComponentTest < ViewComponent::TestCase assert_selector ".app-view-editions__newer-edition-entries h3", text: "On newer editions" assert_selector ".app-view-editions__newer-edition-entries div.app-view-editions-audit-trail-entry__list-item", count: 2 assert_selector ".app-view-editions__newer-edition-entries div.app-view-editions-editorial-remark__list-item", count: 1 + assert_selector ".app-view-editions__newer-edition-entries div.app-view-editions-host-content-update-event-entry__list-item", count: 0 assert_selector ".app-view-editions__current-edition-entries h3", text: "On this edition" assert_selector ".app-view-editions__current-edition-entries div.app-view-editions-audit-trail-entry__list-item", count: 4 assert_selector ".app-view-editions__current-edition-entries div.app-view-editions-editorial-remark__list-item", count: 1 + assert_selector ".app-view-editions__current-edition-entries div.app-view-editions-host-content-update-event-entry__list-item", count: 1 assert_selector ".app-view-editions__previous-edition-entries h3", text: "On previous editions" assert_selector ".app-view-editions__previous-edition-entries div.app-view-editions-audit-trail-entry__list-item", count: 2 assert_selector ".app-view-editions__previous-edition-entries div.app-view-editions-editorial-remark__list-item", count: 0 + assert_selector ".app-view-editions__previous-edition-entries div.app-view-editions-host-content-update-event-entry__list-item", count: 2 end def seed_document_event_history + @events = [] + acting_as(@user) do @document = create(:document) @first_edition = create(:draft_edition, document: @document, major_change_published_at: Time.zone.now) @@ -96,6 +101,10 @@ def seed_document_event_history @first_edition.publish! end + some_time_passes + @events << build(:host_content_update_event, created_at: Time.zone.now) + some_time_passes + @events << build(:host_content_update_event, created_at: Time.zone.now) some_time_passes acting_as(@user) do @@ -108,6 +117,8 @@ def seed_document_event_history @second_edition.publish! end + some_time_passes + @events << build(:host_content_update_event, created_at: Time.zone.now) some_time_passes acting_as(@user2) do @@ -117,6 +128,8 @@ def seed_document_event_history some_time_passes create(:editorial_remark, edition: @third_edition, author: @user, body: "Drafted to include newer changes.") end + + HostContentUpdateEvent.stubs(:all_for_date_window).returns(@events) end def some_time_passes diff --git a/test/components/admin/editions/host_content_update_event_component_test.rb b/test/components/admin/editions/host_content_update_event_component_test.rb new file mode 100644 index 00000000000..7379fd9684d --- /dev/null +++ b/test/components/admin/editions/host_content_update_event_component_test.rb @@ -0,0 +1,32 @@ +require "test_helper" + +class Admin::Editions::HostContentUpdateEventComponentTest < ViewComponent::TestCase + extend Minitest::Spec::DSL + include Rails.application.routes.url_helpers + + let(:created_at) { Time.zone.local(2020, 1, 1, 11, 11) } + let(:content_title) { "Some content" } + let(:user) { build_stubbed(:user) } + + let(:host_content_update_event) do + build(:host_content_update_event, content_title:, created_at:, author: user) + end + + it "constructs output based on the entry when an actor is present" do + render_inline(Admin::Editions::HostContentUpdateEventComponent.new(host_content_update_event)) + + assert_equal page.find("h4").text, "Content Block Update" + assert_equal page.all("p")[0].text.strip, "#{content_title} updated" + assert_equal page.all("p")[1].text.strip, "1 January 2020 11:11am by #{user.name}" + end + + describe "when an actor is not present" do + let(:user) { nil } + + it "shows removed user when an actor is not present" do + render_inline(Admin::Editions::HostContentUpdateEventComponent.new(host_content_update_event)) + + assert_equal page.all("p")[1].text.strip, "1 January 2020 11:11am by User (removed)" + end + end +end