-
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
Consent use dropdown #4251
Consent use dropdown #4251
Conversation
Passing run #4578 ↗︎
Details:
Review all test suite changes for PR #4251 ↗︎ |
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.
nice job @jpople !! I found one maybe bug (or maybe it already existed?) but it looks good overall!
const DictionaryValidationSchema = Yup.object().shape( | ||
{ | ||
// to allow creation/editing of non-dictionary systems even with | ||
// dictionary enabled, only require one of either vendor_id or name |
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.
hmm I think this change makes it so when you:
- have the dictionary
- enter a new name for a vendor (one not in the dictionary)
- the sparkle icon is enabled, when it shouldn't be.
I'm not sure if this line is exactly the culprit, but I remember something sneaky going on between vendor_id and name
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.
Gotcha-- I think that's actually by caused by line ~220 in this file, which disabling if the vendor_id
was specifically null
, so entering anything else meant it was still enabled. I've changed it to check whether the vendor_id
is one of the options or not.
privacy_declarations: passedInSystem.privacy_declarations.map( | ||
(dec) => ({ | ||
privacy_declarations: passedInSystem.privacy_declarations | ||
.filter((dec) => |
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.
just so I understand, this is so that we only ever save data uses which are subsets of the 4 consent use options?
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 makes it so that non-consent-use data uses aren't shown on the form and so they can't be created or edited from this view. If there are some on the vendor (say, if I went in some other time and added one from the data map) they'll still save in the background, that's handled by lines 154-164.
...dec, | ||
name: dec.name ?? "", | ||
cookies: dec.cookies ?? [], | ||
cookieNames: dec.cookies ? dec.cookies.map((c) => c.name) : [], | ||
}) | ||
), | ||
consent_use: dec.data_use.split(".")[0], |
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.
what happens if this is a data use that isn't one of the 4?
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.
Those should all be handled by the filter just above 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.
💥
Closes #4055 and #4119
Description Of Changes
Changes "configure consent" view to use a two-part select input for the new consent uses, and filters the view to only show information in the the view relevant to the consent uses.
Code Changes
MinimalPrivacyDeclaration
type to have a newconsent_use
field which holds the higher-level consent useSteps to Confirm
Pre-Merge Checklist
CHANGELOG.md