Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete statistic announcements frontend parts #2519

Merged
merged 4 commits into from Mar 30, 2016
Merged

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Mar 22, 2016

Similar to #2467 and #2441

  • Delete template, styles, show controller method and specs
  • Update "View announcement on GOV.UK" link to point at the live URL
  • Delete unused topic_links_sentence and array_of_links_to_topics

https://trello.com/c/7S8YJzFN/253-2-statistics-announcement-migration-delete-whitehall-frontend-code-deploy-2-medium

@@ -31,12 +21,4 @@ def expire_cache_for_index_on_next_announcement_expiry(announcements)
time_to_releases = announcements.map {|ann| ann.release_date - Time.zone.now }.reject {|time_span| time_span <= 0 }
expires_in((time_to_releases << Whitehall.default_cache_max_age).min)
end

def expire_cache_for_show_on_release(announcement)

This comment has been minimized.

@dsingleton

dsingleton Mar 24, 2016
Contributor

I'm not sure the new publishing-api version of Statistics Announcements gives us quite the same behaviour as expire_cache_for_show_on_release. The behaviour tries to minimise the time between a Statistical Announcement's document being published, and the announcement URL redirecting to the newly published Statistics document.

I chatted to @fofr who explained Whitehall with publish a redirect for the announcement base_path, which will be picked up by the router, which ignores the rendering application and makes sense in the new world 👍

But, the behaviour here is to start returning a shorter cache expiry time as the release date approaches. It will issue a cache expiry time that is the shortest of;

  • the planned/exact release date on the announcement
  • the scheduled release date of the announcements child publication (if it's scheduled)
  • the default whitehall expiry (currently, 15 minutes)

So that as a planned/scheduled release date approaches the default expiry time is reduced, eg if this page is fetched from origin 3 minutes before the planned/scheduled release date, then the cache expiry will be set to 3 minutes, ensuring the announcement URL will be refetched from origin as close to the release date as possible.

With the new implementation the statistics announcement URL will always be cached with the default expiry time, which may cause the URL to be cached for up to 15 minutes after the planned/scheduled release date, which would be a regression - I think?

This comment has been minimized.

@danielroseman

danielroseman Mar 28, 2016
Contributor

Publishing API has "publish intents" for precisely this use case - see the docs in content-store.

This comment has been minimized.

@fofr

fofr Mar 29, 2016
Author Contributor

I've added a story to begin using the "publish intents" feature:
https://trello.com/c/zMdu3dHe/343-use-publish-intents-on-statistics-announcements

This comment has been minimized.

@dsingleton

dsingleton Mar 30, 2016
Contributor

Just to check my understand of publish intents is right: in this case, an intent is published (via a different publishing-api endpoint) "alongside" the current Statistics Announcement content item.
When the path of the Statistics Announcement content item is fetched from content-store, the cache control config from the intent is merged into the content item.

The rendering app will then respect the cache control config in the content item, reducing the TTL appropriately. And in the middle of that gds-api-adaptors will also respect the TTL in it's caching.

Have I got that right?

fofr added 4 commits Jan 27, 2016
* Delete show from controller
* Remove associated tests
* Remove features and assertions related to viewing a stat announcement
Whitehall will no longer be serving these pages, so the “View
announcement on GOV.UK” should no longer be a relative URL.

Use the public_and_cachebusted_url_options setup for a similar problem
in #2469
@fofr fofr force-pushed the delete-stat-announcements branch from ed837d3 to d566836 Mar 29, 2016
@fofr
Copy link
Contributor Author

@fofr fofr commented Mar 29, 2016

Rebased to fix base.scss conflicts.

@gpeng
Copy link
Contributor

@gpeng gpeng commented Mar 29, 2016

👍

@fofr fofr merged commit 3db1224 into master Mar 30, 2016
1 check passed
1 check passed
default Build #7967 succeeded on Jenkins
Details
@fofr fofr deleted the delete-stat-announcements branch Mar 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.