Do not merge- random change to test coderabbit CR#7561
Do not merge- random change to test coderabbit CR#7561eastandwestwind wants to merge 1 commit intomainfrom
Conversation
eastandwestwind
requested review from
adamsachs and
lucanovera
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request introduces three independent modifications: an additional UI button in the custom fields table, a return value change in consent migration logic from undefined to an empty object, and a new test enum member in manual task execution timing configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Greptile SummaryThis PR contains three intentional test changes (as indicated by the "Do not merge" title) that introduce real, critical bugs across unrelated parts of the codebase:
Confidence Score: 0/5
Last reviewed commit: 408dd35 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!options.otFidesMapping) { | ||
| return undefined; | ||
| } | ||
| return {} |
There was a problem hiding this comment.
The return {} statement on line 30 makes the entire try/catch block unreachable. This causes convertToFidesConsent to always return an empty object, breaking OneTrust cookie parsing and consent migration completely.
| return {} | |
| // Remove this line to allow the try/catch block to execute |
| <Button | ||
| onClick={onAddClick} | ||
| type="primary" | ||
| data-testid="add-custom-field-btn" | ||
| > | ||
| Hi I'm a new button | ||
| </Button> |
There was a problem hiding this comment.
A duplicate <Button> element was added with the same data-testid="add-custom-field-btn" as the existing button above it. This breaks any tests that query this selector, as they will now find two elements instead of one. The placeholder text "Hi I'm a new button" also indicates this is incomplete.
| <Button | |
| onClick={onAddClick} | |
| type="primary" | |
| data-testid="add-custom-field-btn" | |
| > | |
| Hi I'm a new button | |
| </Button> |
| class ManualTaskExecutionTiming(StrEnum): | ||
| """Enum for when a manual task should be executed in the privacy request DAG.""" | ||
|
|
||
| test_new_enum = "test_new_enum" |
There was a problem hiding this comment.
The test-only enum value test_new_enum was added to the ManualTaskExecutionTiming enum. Since this enum is persisted to the database via EnumColumn, this pollutes the production enum space with an unhandled timing type that has no corresponding DAG logic.
| test_new_enum = "test_new_enum" |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/fides-js/src/lib/consent-migration/onetrust.ts`:
- Line 30: Remove the unconditional return {} so the decoding and parsing logic
runs: let the code call decodeURIComponent, JSON.parse and parseCookieValue (the
parseCookieValue helper) to compute the migrated consent value and return that
result; if you need a safe fallback, wrap the decode/parse steps in a try/catch
and only return {} inside the catch (or when parsing yields no data) so parsing
errors fall back gracefully instead of short-circuiting the function.
In `@src/fides/api/models/manual_task/manual_task.py`:
- Line 41: Remove the unused test artifact by deleting the test_new_enum member
from the ManualTaskExecutionTiming enum (symbol: ManualTaskExecutionTiming) in
manual_task.py, search the repo to ensure no references to test_new_enum remain,
run the test suite, and if your project persists enum values in the DB schema
update the related enum migration or schema migration to remove that value so
runtime/DB enums stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8e9902c-670e-47e8-b5cc-66abc396f5d4
📒 Files selected for processing (3)
clients/admin-ui/src/features/custom-fields/CustomFieldsTable.tsxclients/fides-js/src/lib/consent-migration/onetrust.tssrc/fides/api/models/manual_task/manual_task.py
| if (!options.otFidesMapping) { | ||
| return undefined; | ||
| } | ||
| return {} |
There was a problem hiding this comment.
Line 30 makes the parser unreachable and breaks consent migration.
The unconditional return {} exits before decodeURIComponent, JSON.parse, and parseCookieValue run, so migrated consent is never computed.
Suggested fix
if (!options.otFidesMapping) {
- return undefined;
+ return {};
}
- return {}
try {Also applies to: 32-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/fides-js/src/lib/consent-migration/onetrust.ts` at line 30, Remove
the unconditional return {} so the decoding and parsing logic runs: let the code
call decodeURIComponent, JSON.parse and parseCookieValue (the parseCookieValue
helper) to compute the migrated consent value and return that result; if you
need a safe fallback, wrap the decode/parse steps in a try/catch and only return
{} inside the catch (or when parsing yields no data) so parsing errors fall back
gracefully instead of short-circuiting the function.
| class ManualTaskExecutionTiming(StrEnum): | ||
| """Enum for when a manual task should be executed in the privacy request DAG.""" | ||
|
|
||
| test_new_enum = "test_new_enum" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) First, check if the file exists and examine the enum definition
echo "=== Checking enum definition ==="
if [ -f "src/fides/api/models/manual_task/manual_task.py" ]; then
head -50 "src/fides/api/models/manual_task/manual_task.py"
else
echo "File not found"
find . -name "manual_task.py" -type f 2>/dev/null | head -5
fiRepository: ethyca/fides
Length of output: 1643
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 2) Search for all usages of test_new_enum in the codebase
echo "=== Searching for test_new_enum usage ==="
rg -n "test_new_enum" --type-list | head -5 2>/dev/null || true
rg -n "test_new_enum" . 2>/dev/null || echo "No results found"Repository: ethyca/fides
Length of output: 593
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 3) Search for ManualTaskExecutionTiming usage
echo "=== Searching for ManualTaskExecutionTiming usage ==="
rg -n "ManualTaskExecutionTiming" . 2>/dev/null | head -30Repository: ethyca/fides
Length of output: 779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 4) Check for migrations related to execution_timing or manual_task
echo "=== Checking for migrations ==="
find . -path "*/migrations*" -name "*.py" -type f 2>/dev/null | head -20
rg -n "execution_timing|manual_task.*enum" . --type=py 2>/dev/null | head -30Repository: ethyca/fides
Length of output: 1645
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search specifically for migrations related to execution_timing
echo "=== Searching migrations for execution_timing ==="
rg -n "execution_timing" ./src/fides/api/alembic/migrations/versions/ 2>/dev/null || echo "Not found in migrations"
# Also check for manual_task column definitions in migrations
rg -n "manual_task" ./src/fides/api/alembic/migrations/versions/ -A 2 -B 2 2>/dev/null | head -50Repository: ethyca/fides
Length of output: 6696
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any test files using test_new_enum
echo "=== Searching test files for test_new_enum ==="
find . -path "*/test*" -name "*.py" -type f 2>/dev/null | xargs rg -l "test_new_enum" 2>/dev/null || echo "No test files using test_new_enum"
# Search for any references in test files
rg -n "test_new_enum" . --type=py 2>/dev/null || echo "No additional occurrences"Repository: ethyca/fides
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check schema definitions for execution_timing
echo "=== Checking schema definitions ==="
find . -path "*/schemas*" -name "*.py" -type f 2>/dev/null | xargs rg -l "execution_timing" 2>/dev/null || echo "Not found"
rg -n "execution_timing" . --type=py -B 3 -A 3 2>/dev/null | head -50Repository: ethyca/fides
Length of output: 4455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the actual manual_task model more closely around the enum column definitions
echo "=== Checking manual_task model columns ==="
sed -n '495,510p' src/fides/api/models/manual_task/manual_task.py
echo "---"
sed -n '605,615p' src/fides/api/models/manual_task/manual_task.pyRepository: ethyca/fides
Length of output: 1118
Remove the unused test_new_enum test artifact from the production enum.
This value is defined but never used anywhere in the codebase (no production code, tests, or QA scenarios reference it), making it a test artifact that should not be in the persisted ManualTaskExecutionTiming enum. While migrations support the execution_timing column, introducing unused enum values creates maintenance debt and potential confusion about valid runtime values.
Suggested fix
class ManualTaskExecutionTiming(StrEnum):
"""Enum for when a manual task should be executed in the privacy request DAG."""
- test_new_enum = "test_new_enum"
pre_execution = "pre_execution" # Execute before the main DAG
post_execution = "post_execution" # Execute after the main DAG
parallel = "parallel" # Execute in parallel with the main DAG📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test_new_enum = "test_new_enum" | |
| class ManualTaskExecutionTiming(StrEnum): | |
| """Enum for when a manual task should be executed in the privacy request DAG.""" | |
| pre_execution = "pre_execution" # Execute before the main DAG | |
| post_execution = "post_execution" # Execute after the main DAG | |
| parallel = "parallel" # Execute in parallel with the main DAG |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/models/manual_task/manual_task.py` at line 41, Remove the
unused test artifact by deleting the test_new_enum member from the
ManualTaskExecutionTiming enum (symbol: ManualTaskExecutionTiming) in
manual_task.py, search the repo to ensure no references to test_new_enum remain,
run the test suite, and if your project persists enum values in the DB schema
update the related enum migration or schema migration to remove that value so
runtime/DB enums stay in sync.
1 participant
Ticket []
Description Of Changes
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit
Release Notes
New Features
UI Updates
Other Changes