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

PROD-1667 Link properties and experiences #4658

Conversation

galvana
Copy link
Contributor

@galvana galvana commented Mar 5, 2024

Closes PROD-1667, PROD-1670

Description Of Changes

Adding the ability to bidirectionally link properties and privacy experience configs.

Code Changes

  • Refactored Property.key usages to Property.id
  • Added property selector to the PrivacyExperienceForm component
  • Added experience selector to the PropertyForm component
  • Added plus_privacy_experience_config_property linking table
  • Added Property.experiences and PrivacyExperienceConfig.properties relationships
  • Util functions link_properties_to_experience_config and link_experience_configs_to_property

Steps to Confirm

  • Checkout the branch from https://github.com/ethyca/fidesplus/pull/1335
  • Start fidesplus
  • In the fides repo, navigate to the clients directory and start the Admin UI
  • Navigate to the Properties page and attempt to create a new property with a related experience
  • Navigate to the Experiences page and verify the new property is visible in the related experience
  • Navigate to a different experience without a property and verify you can add one

Pre-Merge Checklist

Copy link

vercel bot commented Mar 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Mar 11, 2024 7:16pm

Copy link

cypress bot commented Mar 5, 2024

Passing run #6591 ↗︎

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 f07df1d into ecbcfbf...
Project: fides Commit: 8923b48d64 ℹ️
Status: Passed Duration: 00:37 💡
Started: Mar 11, 2024 3:26 AM Ended: Mar 11, 2024 3:27 AM

Review all test suite changes for PR #4658 ↗︎

@galvana galvana changed the title Prod 1667 link properties and experiences PROD-1667 Link properties and experiences Mar 6, 2024
@galvana galvana requested a review from adamsachs March 6, 2024 16:20
@galvana galvana marked this pull request as ready for review March 6, 2024 16:20
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 94.28571% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.60%. Comparing base (565e083) to head (f07df1d).
Report is 3 commits behind head on PROD-1458_multi_language_support.

Files Patch % Lines
src/fides/api/models/property.py 91.48% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##           PROD-1458_multi_language_support    #4658      +/-   ##
====================================================================
+ Coverage                             86.58%   86.60%   +0.01%     
====================================================================
  Files                                   335      335              
  Lines                                 19867    19924      +57     
  Branches                               2545     2549       +4     
====================================================================
+ Hits                                  17202    17255      +53     
- Misses                                 2198     2201       +3     
- Partials                                467      468       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

BE is looking solid to me here too @galvana!

again, the only thing i'm a bit stuck on is just whether we can get rid of the cicular dep. i'm not trying to be a stickler here. but after looking through the code more closely, i'm pretty confident we could get rid of this without requiring any major restructuring. i feel that it's worthwhile - establishing a circular dependency in core DB models just doesn't feel great to me. we could always unwind it later, i'd just prefer we do it now.

happy to talk through that more. i won't hold this up if you're really feeling like it's not worth the effort!

src/fides/api/models/property.py Show resolved Hide resolved
src/fides/api/models/property.py Show resolved Hide resolved
@galvana galvana merged commit c0d062c into PROD-1458_multi_language_support Mar 11, 2024
21 checks passed
@galvana galvana deleted the PROD-1667-link-properties-and-experiences branch March 11, 2024 19:19
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