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

feat: Add final parameter to onConsentChange #664

Merged
merged 7 commits into from
Feb 13, 2024

Conversation

zekehuntergreen
Copy link
Contributor

@zekehuntergreen zekehuntergreen commented Sep 15, 2022

What does this change?

  • adds an optional final parameter to onConsentChange which defaults to false. When a true value is passed, the callback is pushed onto a new finalCallbackQueue. The finalCallbackQueue's callbacks are guaranteed to execute after the default callBackQueue's when consent state changes.

Why?

  • In Add reload on consent change frontend#25409 we use onConsentChange to register a callback that reloads the page when consent state changes from a state where the user has accepted targeted advertising to one which rejects it or vice versa.
  • reloading the page as part of an onConsentChange callback opens up the possibility of overriding other uses of onConsentChange whose callbacks won't have time to execute before the page is reloaded. This could cause bugs that are very difficult to track down.
  • Another approach considered was to wrap Add reload on consent change frontend#25409 's callback in Promise.resolve().then and / or setTimeout so that the callback that reloads the page is put on the microtask queue or task queue respectively. This would increase the probability but not guarantee that other callbacks registered with onConsentChange could execute before the page is reloaded.

@zekehuntergreen zekehuntergreen marked this pull request as ready for review September 19, 2022 11:27
@Mark-McCracken
Copy link
Contributor

According to @sookburt , @theemilyturner asked Susan Bingham about whether we should reload the page for a consent change, and the answer was no, so this is no longer relevant.
If no-one has anything to say to the contrary, and if still open in a week, we'll close this.

@Mark-McCracken
Copy link
Contributor

as evidenced here: https://trello.com/c/sqmNkN20

@sookburt sookburt requested a review from a team as a code owner May 16, 2023 11:59
@sookburt
Copy link
Contributor

sookburt commented Jun 6, 2023

This might need to be resurrected as part of the cookie deny list changes.

@sookburt sookburt force-pushed the on-consent-change-final-param branch from ad339e6 to 83a9e38 Compare January 24, 2024 12:30
Copy link

changeset-bot bot commented Jan 24, 2024

🦋 Changeset detected

Latest commit: ad2e8ad

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/consent-management-platform Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sookburt sookburt added the [beta] @guardian/consent-management-platform This should debloy to beta label Jan 24, 2024
Copy link
Contributor

github-actions bot commented Jan 24, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
92.81% (+0.11% 🔼)
258/278
🟢 Branches
83.9% (+0.42% 🔼)
99/118
🟢 Functions 90% 63/70
🟢 Lines
92.59% (+0.11% 🔼)
250/270

Test suite run success

331 tests passing in 16 suites.

Report generated by 🧪jest coverage report action from ad2e8ad

Copy link
Contributor

🚀 0.0.0-beta-20240124124059 published to npm as a beta release

@sookburt sookburt added [beta] @guardian/consent-management-platform This should debloy to beta and removed [beta] @guardian/consent-management-platform This should debloy to beta labels Feb 6, 2024
Copy link
Contributor

github-actions bot commented Feb 6, 2024

🚀 0.0.0-beta-20240206103322 published to npm as a beta release

@sookburt sookburt added [beta] @guardian/consent-management-platform This should debloy to beta and removed [beta] @guardian/consent-management-platform This should debloy to beta labels Feb 7, 2024
Copy link
Contributor

github-actions bot commented Feb 7, 2024

🚀 0.0.0-beta-20240207101556 published to npm as a beta release

@sookburt sookburt removed the [beta] @guardian/consent-management-platform This should debloy to beta label Feb 7, 2024
@sookburt sookburt added the [beta] @guardian/consent-management-platform This should debloy to beta label Feb 13, 2024
Copy link
Contributor

🚀 0.0.0-beta-20240213105136 published to npm as a beta release

Copy link
Contributor

@akinsola-guardian akinsola-guardian left a comment

Choose a reason for hiding this comment

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

Thanks for this. This was tested in CODE and worked as expected.

@sookburt sookburt merged commit 8b3c987 into main Feb 13, 2024
16 checks passed
@sookburt sookburt deleted the on-consent-change-final-param branch February 13, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants