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

Merge privacy declaration components #3254

Merged
merged 8 commits into from
May 9, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented May 8, 2023

Closes #2976

Code Changes

  • Make the datamap drawer call the same privacy declaration component that the system portal calls
  • Delete the datamap drawer specific privacy declaration components
  • Add exceptions so that the panel version doesn't render custom fields or the dataset dropdown
  • Consolidate duplicate code

In other words, hopefully keep all existing functionality as-is, but everyone uses the same component!

Steps to Confirm

  • Visit a system page and click the "Data uses" tab. Things should still look and act the same. Navigating away from a dirty form won't work yet, as I didn't hook up any attemptActions yet, but this form didn't originally have that check either
    image
  • Trigger a system panel by going to "View map" and clicking on a node. Things should still look and act the same, including navigating away from a "dirty" form
    image

Pre-Merge Checklist

Description Of Changes

There's still a good chunk of refactoring these components could go through, but I'll split those into a separate ticket since I wanted to make sure we could get on-par functionality first. Some things would be:

  • Use the new actual type PrivacyDeclarationResponse instead of the composed PrivacyDeclarationWithId
  • Totally remove PrivacyDeclarationWithId—that was just a stopgap while privacy declarations didn't have IDs on the backend
  • Optional: refactor to have PrivacyDeclarationForm call the taxonomy data APIs instead of the parent component which then prop drills down. This was useful for reusing a component that had separate redux hooks (back when we had different repos for the datamap-ui and the admin-ui) but is now just a lot of prop drilling
  • If we add more differences between the portal version and the panel version, we should consider breaking the components up into its parts even more, so that we don't have to pass all the if props down

@allisonking allisonking force-pushed the aking/2976/merge-privacy-declarations branch from 94dd6dd to 0c98c57 Compare May 8, 2023 21:29
@cypress
Copy link

cypress bot commented May 8, 2023

Passing run #1841 ↗︎

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 9f7beb6 into 5c75e86...
Project: fides Commit: 95aceff908 ℹ️
Status: Passed Duration: 01:05 💡
Started: May 9, 2023 7:54 PM Ended: May 9, 2023 7:55 PM

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

@allisonking allisonking marked this pull request as ready for review May 9, 2023 14:01
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.

Things are looking good! I think I found a small bug. When editing a PD on the datamap the custom fields list shows up

Screenshot 2023-05-09 at 13 55 41

@allisonking
Copy link
Contributor Author

Things are looking good! I think I found a small bug. When editing a PD on the datamap the custom fields list shows up

Screenshot 2023-05-09 at 13 55 41

🤦‍♀️ missed one prop for the accordion component! should be fixed now, thanks for catching 🙏

image

@TheAndrewJackson TheAndrewJackson self-requested a review May 9, 2023 19:52
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.

Thanks for fixing that tiny datamap issue! LGTM

🚀

@allisonking allisonking merged commit d94bf2c into main May 9, 2023
@allisonking allisonking deleted the aking/2976/merge-privacy-declarations branch May 9, 2023 19:59
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.

Merge "forked" PrivacyDeclaration components
2 participants