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

Amend explicit-cross-domain-links.js code #2464

Merged
merged 2 commits into from Nov 25, 2021

Conversation

danacotoran
Copy link
Contributor

@danacotoran danacotoran commented Nov 23, 2021

Amend explicit-cross-domain-links code to account for the possibility of someone accepting/rejecting cookies on the page that the cross-domain link is present.

This was raised as a bug on the Brexit page, where the Sign in link is a cross-domain link (leading to DI Sign In territory).
If someone has interacted with the cookie banner on this page, the link treats them as if they have not, i.e the ?cookie_consent=not_engaged parameter remains unchanged, and the GA parameter is not added (if they have accepted cookies). The setting applies only after the page is reloaded.

Before (the correct parameters are appended to the cross domain link only after the page is reloaded)

Screen.Recording.2021-11-23.at.11.37.33.mov

After (the correct parameters are appended to the cross domain link instantly when cookies are accepted or rejected)

Screen.Recording.2021-11-23.at.13.30.20.mov

Upon detecting the cookie consent or rejection event, the cross-domain link module should re-run and append the correct parameters to any cross domain links present on the page.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2464 November 23, 2021 15:32 Inactive
@danacotoran danacotoran changed the title Amend explicit-cross-domain-links.js code Amend explicit-cross-domain-links.js code Nov 23, 2021
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2464 November 23, 2021 15:49 Inactive
@danacotoran danacotoran marked this pull request as ready for review November 23, 2021 15:50
@danacotoran danacotoran force-pushed the explicit-x-domain-tracking-issue branch from d002a77 to 37fdda1 Compare November 24, 2021 17:11
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2464 November 24, 2021 17:11 Inactive
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.

Overall looks good, one minor comment.

The code for explicit-cross-domain-links was already a bit of a spaghetti before these changes, I think once this change has gone in we should look at refactoring the code a little bit to make it clearer. Specifically there are a lot of guard clauses in the start function that maybe could be consolidated, and the whole thing could be restructured to make it more understandable. Something like:

window.addEventListener('cookie-consent', this.cookiesAreAccepted)
window.addEventListener('cookie-reject', this.cookiesAreRejected)

...

this.cookiesAreNotAnything = function () {
  this.decorate(element, 'cookie_consent=not-engaged')
}

this.cookiesAreAccepted = function () {
  // something here
}

this.cookiesAreRejected = function () {
  this.decorate(element, 'cookie_consent=reject')
}

GOVUK.cookie('cookies_preferences_set', null)
explicitCrossDomainLinks.start(element)

GOVUK.cookie('cookies_preferences_set', 'true')
Copy link
Contributor

Choose a reason for hiding this comment

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

These three lines appear to be duplicated in the previous test - could they be moved into a beforeEach function for this describe block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, have updated as suggested.

@danacotoran danacotoran force-pushed the explicit-x-domain-tracking-issue branch from 37fdda1 to 2cfa80f Compare November 25, 2021 12:37
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2464 November 25, 2021 12:37 Inactive
Amend explicit-cross-domain-links code to account for the possibility of
someone accepting/rejecting cookies on the page that the cross-domain
link is present. On detecting the cookie consent or rejection event, the
cross-domain link module should re-run and append the correct parameters
to any cross domain links present on the page.
@danacotoran danacotoran force-pushed the explicit-x-domain-tracking-issue branch from 2cfa80f to 359bbe5 Compare November 25, 2021 12:43
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-2464 November 25, 2021 12:43 Inactive
@danacotoran danacotoran merged commit da4beb6 into master Nov 25, 2021
@danacotoran danacotoran deleted the explicit-x-domain-tracking-issue branch November 25, 2021 12:48
@danacotoran danacotoran mentioned this pull request Nov 25, 2021
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

3 participants