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

Don't expand all topical event fields #1372

Merged
merged 1 commit into from Nov 5, 2018

Conversation

steventux
Copy link
Contributor

@steventux steventux commented Nov 2, 2018

There aren't any fields from the topical event details hash which are used in rendering.
The rendering is done by Whitehall but for some formats metatags depend on the content store content item, these don't apply to topical events though, so limit the fields included in link expansion, this will also help with payload size which helps queue processing speed.

DNM until I've done some testing

@steventux steventux changed the title Don't expand all topical event fields [Do not merge] Don't expand all topical event fields Nov 2, 2018
@@ -62,7 +62,7 @@ def details_fields(*fields)
{ document_type: :redirect, fields: [] },
{ document_type: :gone, fields: [] },
{ document_type: :contact, fields: (DEFAULT_FIELDS + details_fields(:description, :title, :contact_form_links, :post_addresses, :email_addresses, :phone_numbers)) },
{ document_type: :topical_event, fields: DEFAULT_FIELDS_WITH_DETAILS },
{ document_type: :topical_event, fields: DEFAULT_FIELDS },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure exactly what it is, but I think you'll also want to change the one below placeholder_topical_event.

There aren't any fields from the topical_event details hash
which are used in rendering, so limit the fields included in
link expansion, this will also help with payload size which
helps queue processing speed.
@steventux steventux force-pushed the dont-expand-topical-event-fields branch from 3355b81 to 87f04fc Compare November 2, 2018 15:25
Copy link
Contributor

@thomasleese thomasleese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Very pleased we can just get rid of details!

@steventux steventux changed the title [Do not merge] Don't expand all topical event fields Don't expand all topical event fields Nov 5, 2018
@steventux steventux merged commit 7c168a8 into master Nov 5, 2018
@thomasleese thomasleese deleted the dont-expand-topical-event-fields branch October 30, 2019 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants