Skip to content

Commit

Permalink
Fix government association bug
Browse files Browse the repository at this point in the history
Those with the relevant permissions can override a government
associated with a particular item of content - see
https://docs.publishing.service.gov.uk/repos/whitehall/history_mode.html#overrides.

The implementation was buggy insofar as Publishing API (and thus
Content Store, and the frontend) would always be one version
behind.

1. Document is associated with Government A.
2. You edit via the UI, to associate with Government B instead.
3. In the UI, the government shows as Government B, but the draft
   preview continues to show Government A. (This is the case
   even if you go on to publish the content).
4. You edit via the UI, to associate with Government C instead.
5. In the UI, the government shows as Government C, but the draft
   preview now shows Government B.

This appears to be down to unexpected caching within
`PublishingApi::PayloadBuilder::Links`, whereby calling
`item.government` returns the old government association. Calling
`item.reload.government` did not help, but fetching the government
ID and using that to instantiate a new `Government` object seemed
to work. This was established by adding a `byebug` within the
method and prying the `news_article_presenter` locally.

```
$ news_article.government
<Government id: 19, slug: "1935-to-1945-national-government", name: "1935 to 1945 National government", start_date: "1935-11-15", end_date: "1945-07-26", created_at: "2015-03-02 15:33:46.000000000 +0000", updated_at: "2019-11-25 11:05:27.000000000 +0000", content_id: "a05c2130-60c3-4d92-9f30-1cd3fb9d4a00">

$ news_article.government_id
25
```

This led me to narrow it down to the `@government` inline caching
in the Edition model. Fetchign the government 'fresh' on each
call eliminates this kind of bug.

Trello: https://trello.com/c/dGAmUBKc/2833-investigate-republishing-docs-reassociated-with-different-government

Stop caching `government` - see previous commits
  • Loading branch information
ChrisBAshton committed Sep 23, 2024
1 parent e6bdcf3 commit 6c93870
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 3 deletions.
8 changes: 5 additions & 3 deletions app/models/edition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,11 @@ def can_set_previously_published?
end

def government
return @government ||= Government.find(government_id) if government_id.present?

@government ||= Government.on_date(date_for_government) unless date_for_government.nil?
if government_id.present?
Government.find(government_id)
elsif date_for_government.present?
Government.on_date(date_for_government)
end
end

def search_government_name
Expand Down
12 changes: 12 additions & 0 deletions test/unit/app/models/edition_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,18 @@ class EditionTest < ActiveSupport::TestCase
assert_equal previous_government, edition.government
end

test "instantiated edition fetches its most up to date #government association" do
current_government = create(:current_government)
previous_government = create(:previous_government)
edition = create(:edition, first_published_at: Time.zone.now, government_id: previous_government.id)
assert_equal previous_government, edition.government
# at this point, edition.government == previous_government
# But if the user updates the association to a different government, we don't want the old one to be returned
edition.government_id = current_government.id
edition.save

Check failure on line 817 in test/unit/app/models/edition_test.rb

View workflow job for this annotation

GitHub Actions / Lint Ruby / Run RuboCop

Rails/SaveBang: Use `save!` instead of `save` if the return value is not checked. (https://rails.rubystyle.guide#save-bang)
assert_equal current_government, edition.government
end

test "#government returns the current government for a newly published edition" do
government = create(:current_government)
edition = create(:edition, first_published_at: Time.zone.now)
Expand Down

0 comments on commit 6c93870

Please sign in to comment.