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

Prevent government_frontend test failure if meta tag key doesn't exist #3741

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Nov 30, 2023

What

  • To generate a new meta tag for GA4, we were diving into a nested value within the content_item JSON: government[:details][:current]
  • government_frontend tests generate random but valid schemas to test against. Therefore, the test schema sometimes would contain the government object, but the government object wouldn't contain [:details][:current] and therefore crash.
  • The error in government_frontend is:
Minitest::UnexpectedError:         ActionView::Template::Error: undefined method `[]' for nil:NilClass
            app/views/layouts/application.html.erb:63` 
  • and application.html.erb:63 was the call to render meta_tags.html.erb. in government-frontend. I used puts statements within meta_tags.rb in this repo to determine which line in our GA4 meta tag code was crashing during the government-frontend test.
  • This PR should fix the issue. It uses .dig in Ruby instead, which will return nil instead of crashing if the key doesn't exist.
  • I tested government_frontend against this local branch, and forced the test to give me the same erroring content schema each time (instead of giving random ones) and the government-frontend test passes now. I also tested it multiple times against random schemas and have not experienced any test failures.
  • I also modified the meta tag code slightly as I noticed the tests for it were passing but were false-positives. This was because I was using .merge to try and remove values in the existing content item, but this was actually not working reliably. Therefore it was better to use .delete when removing values, and to just override keys that needed changing. This surfaced some issues with the meta tag code which are now fixed, and the code is simpler as a result.

Why

Visual Changes

None.

@AshGDS AshGDS self-assigned this Nov 30, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 17:20 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 17:23 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 17:30 Inactive
@AshGDS AshGDS changed the title Prevent crash if meta tag dig encounters wrong data structure/type [WIP] Prevent crash if meta tag dig encounters wrong data structure/type Nov 30, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 18:29 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 18:47 Inactive
@AshGDS AshGDS changed the title [WIP] Prevent crash if meta tag dig encounters wrong data structure/type [WIP] Prevent crash if meta tag generation encounters missing data structure Nov 30, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 18:49 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 November 30, 2023 18:55 Inactive
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 December 1, 2023 10:15 Inactive
@AshGDS AshGDS changed the title [WIP] Prevent crash if meta tag generation encounters missing data structure Prevent crash if meta tag generation encounters missing data structure Dec 1, 2023
@AshGDS AshGDS changed the title Prevent crash if meta tag generation encounters missing data structure Prevent government_frontend test failure if meta tag key doesn't exist Dec 1, 2023
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 December 1, 2023 10:34 Inactive
This change also fixes issues with tests by overriding/deleting content item values instead of trying to merge hashes together.
Values that existed in the content_item were not being overwritten in our merge. This surfaced some issues with the tests overall. Fixing this led to cleaner/simpler code.
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3741 December 1, 2023 10:35 Inactive
@AshGDS AshGDS requested a review from andysellick December 1, 2023 10:38
if government_title && !government[:details][:current]
meta_tags["govuk:ga4-publishing-government"] = government_title
current_government = government.dig(:details, :current)
unless current_government
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 struggling to get my head round this a bit so bear with me. This seems like quite a significant change? It used to always set the ga4-political-status meta tag (either to political or historic) but now it seems like it might not set it at all, if there's a current_government. And it can only ever be historic, not political?

Copy link
Contributor Author

@AshGDS AshGDS Dec 1, 2023

Choose a reason for hiding this comment

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

@andysellick The PAs wanted this meta tag to show on "This was published by X government" pages, and that only appears if the page was not created during the current government. If the page was written during the current government you get the "political" value, but if it was published by a previous government, it gives "historic". I had a test which was meant to check the meta tag doesn't render for the current government previously, but it was passing due to the false positive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, sorry, just a bit confused. I can see that ga4-political-status gets set to historic on line 99, but where does it get set to political? Does that come directly from the content somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andysellick No worries - It no longer gets set to political at all with this change since that's only relevant for pages that are published during the current government. Because the PAs have only requested to have the information of previous governments I have removed it from being set.

Political would get set if current is true for the government in the content item. In the old meta tag, it was setting the meta tag to political based on that:

if details[:political]
  political_status = details[:government][:current] ? "political" : "historic"
  end

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay that's clarified things. Thanks.

Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Still not sure I fully understand this change, but I'm reassured that the worst that can happen is that the status of some pages might be misreported in the analytics data.

@@ -92,13 +92,12 @@ def add_political_tags(meta_tags)
def add_ga4_political_tags(meta_tags)
government = content_item.dig(:links, :government, 0)

# political: true/false is in a different place to current: true/false, which is why we have 'details' and 'government[:details]'
# political: true/false is in a different place to current: true/false, which is why we have 'details' and 'government.dig(:details)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still relevant? I'm not sure what it means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andysellick Populating the meta tag relies on two values existing:

details[:political]: (true/false)
government[:details][:current]: (true/false)

And the comment was trying to explain why the code is referring to two different details objects.

@AshGDS AshGDS merged commit e65586e into main Dec 4, 2023
9 checks passed
@AshGDS AshGDS deleted the ga4-meta-tag-fix branch December 4, 2023 11:36
@AshGDS AshGDS mentioned this pull request Dec 4, 2023
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.

3 participants