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

support new query string initialize=false #4900

Merged
merged 10 commits into from
May 21, 2024

Conversation

gilluminate
Copy link
Contributor

@gilluminate gilluminate commented May 20, 2024

Closes PROD-2086

Description Of Changes

Add a /fides.js?initialize=false option to the Privacy Center that disables the initialization but still stores the server-side “config” in the window.Fides object that FidesJS can access later via init().

Takes advantage of the window.Fides.config property that reinitialize is using. When that property exists, we use it for the initialization (which in turn simplifies reinitialize since it doesn't need to be passed in as a property every time).

Code Changes

  • Update fides.ts and fides-tcf.ts to take advantage of window.Fides.config (this.config), if it exists, when initializing fides.js
  • update reinitialize() to not pass it in to init() now that it's already self aware of that property
  • Update Next's route for fides-js.ts to not automatically trigger the init() function if query string initialize=false is present.

Steps to Confirm

  • Visit demo page of Privacy Center with initialization set to false /fides-js-demo.html?initialize=false
  • Notice in the devTools console log that initialization has been skipped and nothing else.
  • In devTools console, update some config settings at this point (eg window.Fides.config.options.fidesPrimaryColor = "#008000")
  • In devTools console, execute the window.Fides.init() command.
  • Everything will initialize as it normally would have if the query string wasn't present (feel free to compare the 2)
  • The config setting you injected should take effect (eg. color should be green when modal gets opened).

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
    • added to developer docs
  • Issue Requirements are Met
  • Update CHANGELOG.md

Copy link

vercel bot commented May 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview May 21, 2024 10:34pm

Copy link

cypress bot commented May 20, 2024

Passing run #7840 ↗︎

0 4 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 415b70a into 587eda6...
Project: fides Commit: 5c5f5961a2 ℹ️
Status: Passed Duration: 00:34 💡
Started: May 21, 2024 10:45 PM Ended: May 21, 2024 10:45 PM

Review all test suite changes for PR #4900 ↗︎

@gilluminate gilluminate marked this pull request as ready for review May 20, 2024 20:21
@gilluminate gilluminate force-pushed the PROD-2086-fides-js-support-ability-for branch from 542a15b to d7b4b87 Compare May 20, 2024 21:14
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

Minor nits, and one suggestion to make it easier to reason about the Fides.config internal variable for our future selves...

clients/privacy-center/pages/api/fides-js.ts Outdated Show resolved Hide resolved
clients/privacy-center/pages/api/fides-js.ts Outdated Show resolved Hide resolved
clients/privacy-center/pages/api/fides-js.ts Show resolved Hide resolved
Copy link
Contributor

@guncha guncha left a comment

Choose a reason for hiding this comment

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

Looks great! Do we still need the reinitialize function? Seems like it's no longer needed as it's the same as init being called without any arguments.

clients/fides-js/src/fides.ts Outdated Show resolved Hide resolved
@gilluminate
Copy link
Contributor Author

Do we still need the reinitialize function?

@guncha I don't mind keeping it as a more explicit action that can be taken, especially since it's so tiny. I could also see it doing more things down the road. @NevilleS thoughts?

@NevilleS
Copy link
Contributor

NevilleS commented May 21, 2024

Do we still need the reinitialize function?

@guncha I don't mind keeping it as a more explicit action that can be taken, especially since it's so tiny. I could also see it doing more things down the road. @NevilleS thoughts?

Since reinitialize is part of our documented API, we should still maintain it. If it's equivalent to just calling Fides.init() though, we could add that API to the docs and deprecate reinitialize 😄

@gilluminate
Copy link
Contributor Author

If it's equivalent to just calling Fides.init() though, we could add that API to the docs and deprecate reinitialize 😄

@NevilleS @guncha I've updated the docs to reflect that change! ded903e

@gilluminate gilluminate merged commit 8a9b9b6 into main May 21, 2024
13 checks passed
@gilluminate gilluminate deleted the PROD-2086-fides-js-support-ability-for branch May 21, 2024 22:52
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