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

Updated privacy declarations #2648

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Feb 17, 2023

Closes https://github.com/ethyca/fidesplus/issues/598

Code Changes

Okay this is a hefty one but hopefully for good reason!! On the surface, it just changes the privacy declaration form to a nifty accordion setup.

But if you go deeper, it's also trying to be modular so that it can be imported more easily into the datamap-ui.

  • Update privacy declaration form to new accordion design
    • Note that this isn't really a straight up accordion design, since the "new" declaration is always outside the accordion
    • Pulled these files into a subfolder of system called privacy-declarations which has:
      • PrivacyDeclarationStep.tsx: the parent component which handles redux calls and the copy surrounding the more complex parts
      • PrivacyDeclarationManager.tsx: basically the complex component of Accordion + new declaration form + "Add a Data Use" button. No redux calls. In theory, this component can be plopped into Datamap UI
      • PrivacyDeclarationForm.tsx: the individual parts of the privacy declaration form, exported into modular parts. This is because the form is used both as a normal looking form and as an accordion, where the title needs to be an accordion button ⬇️
      • PrivacyDeclarationAccordion.tsx: the parts of the form reused for Accordion form.
      • At first I thought it might make sense for datamap UI to pull its parts from PrivacyDeclarationForm, and you can refer to PrivacyDeclarationAccordion to see how that can be done. But I think it might work to pull PrivacyDeclarationManager directly, which would handle all the logic. provided the datamap UI designs don't look any different. If they do, then you may need to use the component parts f rom PrivacyDeclarationForm instead.
  • No more extended privacy declaration form
  • Cypress tests
    • Update existing tests
    • Added new cypress tests to cover data use collisions
    • Pulled system related fixtures into their own folder for better organization

Steps to Confirm

  • Try adding privacy declarations through the add systems flow as well as the edit system flow

Pre-Merge Checklist

Description Of Changes

https://www.loom.com/share/561080864cb944fc89b7f67ae18fc695

@allisonking allisonking changed the base branch from main to aking/2434/new-manual-add-design February 17, 2023 22:33
@@ -24,6 +24,9 @@ export const dataUseApi = createApi({
transformResponse: (uses: DataUse[]) =>
uses.sort((a, b) => a.fides_key.localeCompare(b.fides_key)),
}),
getDataUseByKey: build.query<DataUse, string>({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

turns out I didn't need this, but might still be useful to keep around

* If true, when isMulti=false, the selected value will be rendered as a block,
* similar to how the multi values are rendered
*/
singleValueBlock?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the difference between

singleValueBlock = true
image

singleValueBlock = false
image

@cypress
Copy link

cypress bot commented Feb 21, 2023

Passing run #257 ↗︎

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

Details:

Merge ce88493 into 79ac1dd...
Project: fides Commit: 1fb52975c0 ℹ️
Status: Passed Duration: 00:37 💡
Started: Feb 21, 2023 9:18 PM Ended: Feb 21, 2023 9:19 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Base: 86.23% // Head: 86.23% // No change to project coverage 👍

Coverage data is based on head (ce88493) compared to base (79ac1dd).
Patch has no changes to coverable lines.

Additional details and impacted files
@@                        Coverage Diff                        @@
##           aking/2434/new-manual-add-design    #2648   +/-   ##
=================================================================
  Coverage                             86.23%   86.23%           
=================================================================
  Files                                   289      289           
  Lines                                 15796    15796           
  Branches                               1986     1986           
=================================================================
  Hits                                  13622    13622           
  Misses                                 1786     1786           
  Partials                                388      388           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@allisonking allisonking marked this pull request as ready for review February 21, 2023 23:08
Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

This is awesome! Very modular. It does look like I'll be able to drop PrivacyDeclarationManager.tsx right into the datamap ui since the redux calls are passed down from the parent.

So far everything makes sense. I think this is ready to merge into the upstream branch and I'll start testing things out in fidesplus. If I hit any snags we can merge the fixes into the new-manual-add-design branch.

@allisonking allisonking merged commit f1d5aa2 into aking/2434/new-manual-add-design Feb 22, 2023
@allisonking allisonking deleted the aking/fidesplus-598/privacy-declarations branch February 22, 2023 16:14
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.

2 participants