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

Fix global bar multiple cookies #2026

Merged
merged 3 commits into from
Feb 5, 2020

Conversation

vanitabarrett
Copy link
Contributor

@vanitabarrett vanitabarrett commented Feb 5, 2020

Trello: https://trello.com/c/hA65u9qV/460-global-banner-setting-two-cookies-on-some-pages

What

Use cookie helper functions in the global bar init logic, to match the rest of the global bar logic.

Why

The global banner was setting two cookies on some pages, one with a path of '/' and one with the path of the page. This had the side effect of the banner not being able to be dismissed (it also didn't disappear after 3 page views). This looked like it was happening because some of the global bar functions set the cookie manually without specifying a path, whereas other functions use the cookie helper function which always set the path to '/'.

Example page to check:
www.gov.uk/government/organisations/companies-house

@vanitabarrett vanitabarrett force-pushed the fix-global-bar-multiple-cookies branch from 1005202 to 75980f2 Compare February 5, 2020 09:31
@vanitabarrett vanitabarrett marked this pull request as ready for review February 5, 2020 09:48
@vanitabarrett vanitabarrett requested review from alex-ju and removed request for alex-ju February 5, 2020 10:03
@vanitabarrett
Copy link
Contributor Author

(sorry, still need to test on various frontend apps in integration. Will invite for review once I've done more testing)

Vanita Barrett added 3 commits February 5, 2020 11:26
We had an issue where the global bar was setting 2 global_bar_seen cookies on some pages. The initialise logic was setting a cookie with the path of the page, whereas other logic was using GOVUK.setCookie() which sets with a path of "/".

This commit switches the initialise logic to use GOVUK.setCookie() too, so the path is the same and we only have one cookie.
@vanitabarrett vanitabarrett force-pushed the fix-global-bar-multiple-cookies branch from 75980f2 to e739f44 Compare February 5, 2020 11:27
@vanitabarrett vanitabarrett requested a review from alex-ju February 5, 2020 12:16
@vanitabarrett
Copy link
Contributor Author

Ready for review now - seems to fix the issue when I tested on integration

Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Much easier to follow the new code – and less cryptic. Thanks for fixing this so quickly!

@vanitabarrett vanitabarrett merged commit 6d3dceb into master Feb 5, 2020
@vanitabarrett vanitabarrett deleted the fix-global-bar-multiple-cookies branch February 5, 2020 12:51
vanitabarrett pushed a commit that referenced this pull request Feb 10, 2020
We had a bug which meant that the global_bar_seen cookie was sometimes set more than once.
This bug has now been fixed (#2026), but some users will be left with these duplicate cookies and therefore will continue to see the issue.

This commit introduces a function that checks for these duplicate cookies and deletes them if they're present.
vanitabarrett pushed a commit that referenced this pull request Feb 10, 2020
We had a bug which meant that the global_bar_seen cookie was sometimes set more than once.
This bug has now been fixed (#2026), but some users will be left with these duplicate cookies and therefore will continue to see the issue.

This commit introduces a function that checks for these duplicate cookies and deletes them if they're present.
vanitabarrett pushed a commit that referenced this pull request Feb 10, 2020
We had a bug which meant that the global_bar_seen cookie was sometimes set more than once.
This bug has now been fixed (#2026), but some users will be left with these duplicate cookies and therefore will continue to see the issue.

This commit introduces a function that checks for these duplicate cookies and deletes them if they're present.
vanitabarrett pushed a commit that referenced this pull request Feb 10, 2020
We had a bug which meant that the global_bar_seen cookie was sometimes set more than once.
This bug has now been fixed (#2026), but some users will be left with these duplicate cookies and therefore will continue to see the issue.

This commit introduces a function that checks for these duplicate cookies and deletes them if they're present.
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.

2 participants