-
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
Undeclared data categories #4781
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Passing run #7320 ↗︎
Details:
Review all test suite changes for PR #4781 ↗︎ |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4781 +/- ##
==========================================
+ Coverage 86.61% 86.62% +0.01%
==========================================
Files 339 340 +1
Lines 20090 20132 +42
Branches 2586 2595 +9
==========================================
+ Hits 17400 17440 +40
- Misses 2217 2218 +1
- Partials 473 474 +1 ☔ View full report in Codecov by Sentry. |
@@ -47,7 +47,8 @@ export const ColumnSettingsModal = <T,>({ | |||
.map((c) => ({ | |||
id: c.id, | |||
displayText: c.columnDef?.meta?.displayText || c.id, | |||
isVisible: c.getIsVisible(), | |||
isVisible: | |||
tableInstance.getState().columnVisibility[c.id] ?? c.getIsVisible(), |
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 is to allow us to set default visibility, see: https://github.com/ethyca/fides/pull/4781/files#diff-cd47c40887e69a55fee3a54b819214c466bf33045c6be67473345c87cef25bbcR983-R986
system = relationship( | ||
System, back_populates="privacy_declarations", lazy="selectin" | ||
) |
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.
Need to be careful of this change, it makes the system retrieval on privacy declarations eager
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.
Are you concerned about performance on this PR?
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.
Yes, that's my main concern with that change. I'm planning on running nox -s performance_tests
between main
and this branch to see if there are any significant timing changes.
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.
Systems already feel very bogged down, at some point we may reach a tipping point where we need to rearchitect some of this - not this specifically I just mean generally System/Privacy Declaration
Plan to start review Wed 4/17 - |
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 is looking great, I just have a few questions, also these are the best testing instructions I've ever seen. Really helpful for someone that wasn't involved in the conversations for this feature.
Closes PROD-1873
Description Of Changes
Updating datamap report table with two new columns:
System undeclared data categories
- A list of data categories aggregated from all of a system's datasets that are not declared in any of the system's data usesData use undeclared data categories
- A list of data categories from a data use's dataset that are not declared in the current data use or any sibling data usesCode Changes
DatamapReportTable.tsx
System
andPrivacyDeclaration
models to aggregate and report on associated data categoriesSteps to Confirm
System undeclared data categories
to the Postgres sample dataset, click Save
System undeclared data categories
should not be visibleSystem undeclared data categories
column, click SaveSystem undeclared data categories
should say "12 system undeclared data categories"user
data categorySystem undeclared data categories
column should now show a single undeclared data category,System Data: Operations Data
Data use undeclared data categories
System
and add the Postgres datasetData use undeclared data categories
should not be visibleData use undeclared data categories
column, click SaveData use undeclared data categories
should say "11 system undeclared data categories"user
Data use undeclared data categories
column should now show a single undeclared data category,System Data: Operations Data
Pre-Merge Checklist
CHANGELOG.md