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.

Sign up for GitHub

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

Jump to bottom

Fix bug causing missing datamap rows #1124

Merged
merged 5 commits into from
Sep 29, 2022
Merged

Fix bug causing missing datamap rows #1124

merged 5 commits into from
Sep 29, 2022

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Sep 27, 2022 •
edited by SteveDMurphy
Loading

Closes #1123

Code Changes

  • fix rows getting mistakenly dropped
  • add tests to verify functionality

Steps to Confirm

  • test complex use-cases

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Fixed a bug where the datmap dataframe join was dropping rows

@ThomasLaPiana ThomasLaPiana self-assigned this Sep 27, 2022
@ThomasLaPiana
Copy link
Contributor Author

I confirmed its this line causing the issue: https://github.com/ethyca/fides/blob/main/src/fidesctl/ctl/core/export.py#L401-L405

@ThomasLaPiana
Copy link
Contributor Author

@SteveDMurphy and I found the bug, next step is to create test cases to catch it

@SteveDMurphy
Copy link
Contributor

Lost power and internet last night for a bit! Should be able to get those test cases out here shortly 🙌🏽

@SteveDMurphy
Copy link
Contributor

@ThomasLaPiana added a couple of commits, was going to have the tests fail for posterity but figured that can be done locally for speed purposes. In the end, I think we needed to account for the handling of differences in multiple privacy declarations, which isn't something we were fully accounting for here. With this, I think we should be able to ship this (👀 @mfbrown )

@SteveDMurphy
Copy link
Contributor

Just some interesting mypy errors and the changelog left, should have time shortly 🤔

@SteveDMurphy SteveDMurphy force-pushed the ThomasLaPiana-fix-missing-datamap-rows branch from 8b5ccbd to 09205b5 Compare September 28, 2022 19:09
@SteveDMurphy SteveDMurphy marked this pull request as ready for review September 28, 2022 19:53
@SteveDMurphy SteveDMurphy requested a review from a team September 28, 2022 19:53
@SteveDMurphy SteveDMurphy changed the base branch from main to release-v1.8.6 September 29, 2022 00:03
@SteveDMurphy SteveDMurphy changed the base branch from release-v1.8.6 to main September 29, 2022 00:05
@SteveDMurphy SteveDMurphy merged commit 6c723e6 into main Sep 29, 2022
@SteveDMurphy SteveDMurphy deleted the ThomasLaPiana-fix-missing-datamap-rows branch September 29, 2022 00:07
SteveDMurphy added a commit that referenced this pull request Sep 29, 2022
@ThomasLaPiana @SteveDMurphy
Fix bug causing missing datamap rows (#1124)
1e71b2a 
* Update export.py

* fix mistakenly dropped datamap rows

* add tests for bug that fail with original state

* fix test failure with grouping in delete_df

* fix mypy reassignment issues, changelog

Co-authored-by: SteveDMurphy <steven.d.murphy@gmail.com>
ssangervasi added a commit that referenced this pull request Oct 1, 2022
@ssangervasi
Merge branch 'main' into aking-1077-classify-data-category-dropdown
f6ab5ff 
* main:
  Restrict startsWith comparisons in CheckboxTree more (#1126)
  [1076] ui/plus: Approve dataset classification button (#1129)
  Do not rely on order for checking intercept results (#1131)
  Prepare 1.9.1 release (#1137)
  Bump fideslang to 1.3.1 (#1136)
  Prepare changelog for 1.9.0 release (#1134)
  Update CHANGELOG.md (#1132)
  Fix bug causing missing datamap rows (#1124)
  cls migration (#1060)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveDMurphy SteveDMurphy SteveDMurphy approved these changes

Assignees

@ThomasLaPiana ThomasLaPiana

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Datamap Endpoint doesn't handle multiple data subjects in a privacy declaration
2 participants
@ThomasLaPiana @SteveDMurphy