-
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
Update privacy experience endpoints to use new experience config #3313
Update privacy experience endpoints to use new experience config #3313
Conversation
Passing run #2031 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
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.
@allisonking thanks for jumping on this to help get the backend mergeable 🎉
Yeah, I remember wondering this too! The designs didn't indicate that so I kept them always showing, but now that I'm touching the component UI side more, I think you're right that we may not need these. We might not even need |
There isn't right now, that was intentionally left out of scope of this initial version (since users can't make their own privacy experiences). Does that still work with how the new model for experiences? |
it's probably because there's nothing in the required field of "Link label" in this case (the "Save" button is disabled as long as the form is invalid). also, perhaps link label shouldn't be required in this case... |
I think it would be okay if we get these default Experience Configs in the database that I can link to automatically that Michael's working on. For example, someone creates an overlay Privacy Notice for CA, I create an overlay PrivacyExperience in the backend, and then link it to the "default" overlay PrivacyExperience temporarily. But I think it's functionality we would need soon! |
Ah okay, it sounds like on the backend what is "required" for each experience might not be correct on my end then. I was assuming a link label wasn't required for a banner overlay. |
That's probably the correct assumption! I'm starting to think we don't need link label at all, so I'll at least make it not required for now |
That's great! |
Thanks for bringing this up! I think we got this logic sorted out, captured here: #3320 I think I've got most of the things you caught (thanks for the sharp eyes!!), and am thinking I will defer the banner/link conditional rendering logic to #3320 so that we can unblock your backend branch from merging, does that sound okay? |
that sounds great @allisonking thanks for all your work on this |
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.
This looks great, so perhaps I just leave the other fields in the backend as they are, and adjust in a followup to go with #3320
f295e7e
into
fides_3193_syncing_privacy_notices_and_experiences
Closes #3259
Code Changes
Mostly the behavior has not changed, except for the part marked by ❗
privacy-experience
endpoints to useexperience-config
endpoints instead. This involved:regions
to always be in the payloadnotice_only
. Since experience configs are now, in the data model, independent of notices, we instead render all form fields and ask for the user to fill them all in.Steps to Confirm
/api/v1/experience-config
Here's one that worksPre-Merge Checklist
CHANGELOG.md
Description Of Changes