Conversation
…des into ENG-2185-add-new-jsob-tree-col
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ENG-2185-add-new-jsob-tree-col #7172 +/- ##
==================================================================
- Coverage 87.24% 85.04% -2.20%
==================================================================
Files 535 535
Lines 35324 35264 -60
Branches 4117 4107 -10
==================================================================
- Hits 30817 29992 -825
- Misses 3615 4348 +733
- Partials 892 924 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
...rations/versions/xx_2025_12_30_1421_9cf7bb472a7c_remove_un_used_columns_from_conditional_.py
Outdated
Show resolved
Hide resolved
...rations/versions/xx_2025_12_30_1421_9cf7bb472a7c_remove_un_used_columns_from_conditional_.py
Outdated
Show resolved
Hide resolved
|
@greptile please review |
src/fides/api/models/conditional_dependency/conditional_dependency_base.py
Show resolved
Hide resolved
| condition_row = ( | ||
| db.query(cls) | ||
| .filter(cls.manual_task_id == manual_task_id, cls.parent_id.is_(None)) | ||
| .first() | ||
| db.query(cls).filter(cls.manual_task_id == manual_task_id).first() | ||
| ) |
There was a problem hiding this comment.
nit: using .one() here would make it so we throw an error if there's somehow more than one matching row
| "conditions": [ | ||
| { | ||
| "logical_operator": "and", | ||
| "logical_operator": GroupOperator.and_, |
There was a problem hiding this comment.
is this change intentional?
There was a problem hiding this comment.
yeah - moving away from strings to the actual enums etc.
| privacy_request=privacy_request, | ||
| ) | ||
|
|
||
| # Verify the extracted location convenience fields match expected |
There was a problem hiding this comment.
are the comments removed on purpose? I like comments in long tests, it helps read through what the test is doing
There was a problem hiding this comment.
Some of them felt redundant, I am probably just too close to it! Thanks for the feedback, I will put them back!
| # No conditional dependencies created | ||
|
|
||
| # Extract the data |
There was a problem hiding this comment.
same question for this test
There was a problem hiding this comment.
Putting them back :)
| """Test that manual task traversal executes correctly with conditional dependencies and upstream data""" | ||
|
|
||
| @pytest.mark.usefixtures("condition_gt_18", "condition_eq_active") | ||
| @pytest.mark.usefixtures("group_condition") |
There was a problem hiding this comment.
before we were using some other conditions here, are those not relevant for this test anymore?
There was a problem hiding this comment.
Since all the conditions will now just be in the condition tree instead of spread out across rows. We only need the group condition.
| @pytest.fixture() | ||
| def condition_gt_18(db: Session, manual_task: ManualTask): | ||
| """Create a conditional dependency with field_address 'user.age' and operator 'gte' and value 18""" |
There was a problem hiding this comment.
there were tests that used this fixture but no longer do so, is that an intentional decision?
There was a problem hiding this comment.
Its being used, but it looks like it is only used in 1 place now. I can probably just clean that up. It is using the create_condition_gt_18_tree() which adds the condition in dict form and is used a lot more widely now.
Co-authored-by: Jade Wibbels <jade@ethyca.com>
Co-authored-by: Jade Wibbels <jade@ethyca.com>
Ticket ENG-2185
Description Of Changes
🎯 Create manual tasks on the Consent Graph.
There are going to be a few PRs cleaning up some un-used functionality or adjusting functionality for improved usage patterns. This PR removes the columns which are now un-used and have been replaced by the JSONB storage tree. It is a clean up based on the first few rounds of customer use and learning actual usage patterns.
Past PRs:
This PR:
Future PRs:
Code Changes
.fides/db_dataset.yml- removed un used columnssrc/fides/api/alembic/migrations/versions/remove the unused columns.src/fides/api/models/conditional_dependency/conditional_dependency_base.pysrc/fides/api/models/digest/conditional_dependencies.pysrc/fides/api/models/manual_task/conditional_dependency.pySteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works