-
Notifications
You must be signed in to change notification settings - Fork 72
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
Send property id and also override path. #5532
Send property id and also override path. #5532
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
fides Run #11155
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5532/merge
|
Run status |
Passed #11155
|
Run duration | 00m 41s |
Commit |
ebfa33e455 ℹ️: Merge 2a5bdc5e1db9fd87b8ca8b2903795d200374c345 into f80dddb59f1fdf36534a2069ff34...
|
Committer | Tom Van Dort |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
@@ -16,7 +16,9 @@ const loadEnvironmentVariables = () => { | |||
"file:///app/config/config.css", | |||
SHOW_BRAND_LINK: | |||
process.env.FIDES_PRIVACY_CENTER__SHOW_BRAND_LINK === "true" || false, | |||
CUSTOM_PROPERTIES: process.env.CUSTOM_PROPERTIES === "true" || true, | |||
CUSTOM_PROPERTIES: process.env.CUSTOM_PROPERTIES !== "false", // default: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the "why" behind this change, can you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixing a bug I noticed. || true will always be true even if you set it to "false". So I fix that.
It shouldn't matter too much, the default is true and I doubt anyone will set it to false. But just in case I wanted to fix it.
…to-experience-and-experiment-with-manual-override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the fix for fetching the correct experience when using properties and it works correctly. Tested the override env variable and works correctly too. Approved.
fides Run #11158
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #11158
|
Run duration | 00m 39s |
Commit |
955a866ed1: Send property id and also override path. (#5532)
|
Committer | Tom Van Dort |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
Upgrade your plan to view test results. | |
View all changes introduced in this branch ↗︎ |
Co-authored-by: Lucano Vera <[email protected]>
Description Of Changes
Fetch correct experience when a privacy center property is being use. Add env variable to override use a specific property for the root domain (when no property paths are used).
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.md
updated