(688) Add content block updates to a document's timeline #9754

pezholio · 2024-12-19T09:24:09Z

Trello card: https://trello.com/c/PTDFVt1z/688-pull-in-when-content-has-been-updated-by-an-update-to-a-content-block-within-publishing-apps

As discussed in #9666, we need a way of pulling in content block updates to a document's history. After much to-ing and fro-ing, we have settled on doing this in-app, when fetching the history. This is complicated slightly by the fact that the history is paginated. To compensate for this, we are having to "look ahead" to the next page to get the date of the next event to give us a window to query Publishing API for events.

We also have an extra complication as we don't know what updates "belong" to an edition. We've got around this by adding some event-based helpers to an edition, so we know when an edition was published and superseded. To simplify the logic in the paginated timeline class, we've moved the logic to work out what object belongs to an edition to the model classes themselves.

Additionally, there's a bit more tidying up and refactoring of the history-related work so we're doing away with complexity before adding any more!

Hopefully this isn't too much of a monster to review (especially with 🎄 on the horizon). I've tried to break it down as much as possible, but if you want me to break it up into seperate PRs, I'm happy to do that!

Screenshot

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.

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

This allows us to define and fetch when a piece of host content has been updated for a document and a date window.

This uses the Minitest Spec DSL to make the tests easier to follow

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”

This will be used in the paginated timeline in a future commit

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

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`

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

ChrisBAshton

Nice and easy to follow 👍 some suggestions for code refactoring and commit structure, but nothing blocking.

ChrisBAshton · 2024-12-20T16:14:33Z

test/unit/app/models/host_content_update_event_test.rb

Nice tests ⭐

ChrisBAshton · 2024-12-20T16:16:19Z

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


This is difficult to parse 😅 can we reword to remove the double negative?

Similarly on line 92

ChrisBAshton · 2024-12-20T16:19:26Z

app/models/document/paginated_timeline.rb

+  def next_page_entries
+    if next_page
+      Document::PaginatedTimelineQuery.new(document:, page: next_page, only:).raw_entries
+    end
+  end


I'd move this below def date_window as it isn't called until further down in the file (nice to have methods defined in the order they're called)

ChrisBAshton · 2024-12-20T16:21:51Z

app/models/document/paginated_timeline.rb

+  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


The next_page logic is surprising - can you add to the commit detail what you've added to the PR description?

This is complicated slightly by the fact that the history is paginated. To compensate for this, we are having to "look ahead" to the next page to get the date of the next event to give us a window to query Publishing API for events.

Better yet, perhaps just handle the first page in this first commit, and then add the "next page" logic in a subsequent commit, rather than doing it all at once.

ChrisBAshton · 2024-12-20T16:24:29Z

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)


Seems a shame to have to duplicate this - did you consider any alternatives (e.g. extract into a module?)

I don't feel strongly about it though, agree that duplication is better than the wrong abstraction! And at least the code is simple.

I agree with Stu's duplication choice - the fact that the algorithm for sorting editorial remarks and versions is identical seems like coincidental rather than fundamental duplication to me.

ChrisBAshton · 2024-12-20T16:26:57Z

app/models/document/paginated_timeline.rb

-      else
-        entry.item_id > edition.id
-      end
-    end


ryanb-gds

Thought I'd chuck in my two penneth as well, hope you don't mind.

On a macro level I do look forward to revisiting the entirety of how document history works at some point, it has always felt a lot harder than it needs to be for some reason

ryanb-gds · 2024-12-20T16:34:50Z

app/models/concerns/edition/workflow.rb

+  def superseded_at
+    versions.find { |v| v.state == "superseded" }&.created_at
+  end
+
+  def published_at
+    versions.find { |v| v.state == "published" }&.created_at
+  end


I don't think these should live in this concern, because we've created an implicit dependency on the versions association declared in AuditTrail concern. We should either move these methods up to the main model, in to the AudtiTrail concern, or move the dependency on AuditTrail in to this concern (but that doesn't feel right)

ryanb-gds · 2024-12-20T17:07:11Z

app/decorators/version_decorator.rb

@@ -0,0 +1,45 @@
+class VersionDecorator < SimpleDelegator


I think it would be nice if the name of this decorator said something about why we have chosen to use a decorator rather than simply adding these behaviours on to the version model. If I understand your intentions correctly, it's because we only expect these behaviours to be useful in the context of constructing a document history timeline, but I'm struggling to think of a way to communicate that pithily via a class name

ryanb-gds · 2024-12-20T17:16:15Z

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)


I agree with Stu's duplication choice - the fact that the algorithm for sorting editorial remarks and versions is identical seems like coincidental rather than fundamental duplication to me.

pezholio force-pushed the content-modelling/688-pull-in-when-content-has-been-updated-by-an-update-to-a-content-block-within-publishing-apps branch 6 times, most recently from 54a8428 to b4e2cbd Compare December 19, 2024 09:52

pezholio added 4 commits December 19, 2024 10:08

Use a decorator instead of a presenter

03d0567

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.

Simplify the timeline query

fcfa164

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

Add a HostContentUpdateEvent model

e1cd197

This allows us to define and fetch when a piece of host content has been updated for a document and a date window.

Refactor PaginatedTimelineTest

00332b7

This uses the Minitest Spec DSL to make the tests easier to follow

pezholio force-pushed the content-modelling/688-pull-in-when-content-has-been-updated-by-an-update-to-a-content-block-within-publishing-apps branch 4 times, most recently from 6e8ccbc to 0cee0a5 Compare December 19, 2024 10:31

pezholio added 5 commits December 19, 2024 11:57

Include events in paginated timeline

5b44220

Add methods to get when events occured on editions

da9ce39

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”

Add history helpers to versions and remarks

d99f6db

This will be used in the paginated timeline in a future commit

Use new methods in PaginatedTimeline

090f1d6

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`

pezholio force-pushed the content-modelling/688-pull-in-when-content-has-been-updated-by-an-update-to-a-content-block-within-publishing-apps branch from 0cee0a5 to e79be6f Compare December 19, 2024 11:57

pezholio changed the title ~~Content modelling/688 pull in when content has been updated by an update to a content block within publishing apps~~ (688) Add content block updates to a document's timeline Dec 19, 2024

pezholio added the Content Modelling label Dec 19, 2024

pezholio marked this pull request as ready for review December 19, 2024 12:29

pezholio force-pushed the content-modelling/688-pull-in-when-content-has-been-updated-by-an-update-to-a-content-block-within-publishing-apps branch from e79be6f to 14b321a Compare December 19, 2024 14:51

Add component for HostContentUpdateEvents

297f3b5

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

pezholio force-pushed the content-modelling/688-pull-in-when-content-has-been-updated-by-an-update-to-a-content-block-within-publishing-apps branch from 14b321a to 297f3b5 Compare December 19, 2024 14:56

ChrisBAshton approved these changes Dec 20, 2024

View reviewed changes

ryanb-gds reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(688) Add content block updates to a document's timeline #9754

(688) Add content block updates to a document's timeline #9754

pezholio commented Dec 19, 2024 •

edited

Loading

ChrisBAshton left a comment

ChrisBAshton Dec 20, 2024

ChrisBAshton Dec 20, 2024

ChrisBAshton Dec 20, 2024

ChrisBAshton Dec 20, 2024

ChrisBAshton Dec 20, 2024

ryanb-gds Dec 20, 2024

ChrisBAshton Dec 20, 2024

ryanb-gds left a comment •

edited

Loading

ryanb-gds Dec 20, 2024

ryanb-gds Dec 20, 2024

ryanb-gds Dec 20, 2024

(688) Add content block updates to a document's timeline #9754

Are you sure you want to change the base?

(688) Add content block updates to a document's timeline #9754

Conversation

pezholio commented Dec 19, 2024 • edited Loading

Screenshot

ChrisBAshton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanb-gds left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pezholio commented Dec 19, 2024 •

edited

Loading

ryanb-gds left a comment •

edited

Loading