ENG-3146: Support Jira DSR lifecycle with pending_external status#7772
ENG-3146: Support Jira DSR lifecycle with pending_external status#7772
Conversation
Auto-create default ManualTaskConfig (access + erasure) with a "complete" checkbox when a jira_ticket connection is created, so the DSR graph blocks until the Jira ticket is closed or the operator submits manually. Use pending_external (not requires_input) for Jira manual tasks, and treat it equivalently in the watchdog and duplication detection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
8a24b62 to
e0fe93b
Compare
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e0fe93b to
e6746ca
Compare
There was a problem hiding this comment.
Code Review Summary
The overall approach is sound — using pending_external to distinguish "waiting on Jira" from "waiting on operator input" is a clean semantic improvement, and the watchdog/duplication detection guards follow the same pattern as the existing requires_input handling. The docstring on _create_default_jira_configs is clear and the reasoning for access-only is well explained.
Critical (Must Fix)
- Import ordering (isort violation) in
connection_service.py—from fides.api.schemas.policy import ActionTypeis placed between twofides.api.modelsimports. This will likely fail CI linting. See inline comment.
Suggestions
- Unnecessary single-element loop —
for action_type in (ActionType.access,):loops exactly once by design. A direct call without the loop would be clearer. See inline comment. - Stale log message — the warning in
request_service.pystill says "may be waiting for manual input" even though it now coverspending_external(an external system). See inline comment. - Mixed-task status overwrite — the status guard in
manual_task_graph_task.pycould overwrite a sibling task's status if a request ever had both regular manual tasks and Jira tasks. Probably not a real scenario today, but worth a comment noting that assumption. See inline comment.
Nice to Have
- Test coverage for
_create_default_jira_configs— the new method has no direct tests. Adding assertions for the created config and field (and verifying the update-path idempotency via thenot connection_config.manual_taskguard) would give confidence this wiring holds. See inline comment.
Also note: the PR description's "Steps to Confirm" (step 2) says "Verify a ManualTask is created with two ManualTaskConfig records (access + erasure)" — but the code only creates one access config. The code and docstring are correct; the PR description should be updated to match.
| ManualTaskParentEntityType, | ||
| ManualTaskType, | ||
| ) | ||
| from fides.api.schemas.policy import ActionType |
There was a problem hiding this comment.
Import ordering (isort violation): from fides.api.schemas.policy import ActionType is inserted between two fides.api.models imports. models.saas_template_dataset on line 36 should appear before this schemas import. The correct order is:
from fides.api.models.manual_task import (...)
from fides.api.models.saas_template_dataset import SaasTemplateDataset
from fides.api.schemas.policy import ActionType
from fides.api.schemas.connection_configuration import (...)This will likely fail isort CI checks.
| and the erasure node finds no erasure config so it completes immediately. | ||
| The Jira ticket tracks the whole request, not individual action types. | ||
| """ | ||
| for action_type in (ActionType.access,): |
There was a problem hiding this comment.
Unnecessary loop over a single-element tuple: for action_type in (ActionType.access,): loops exactly once. The docstring clearly states only an access config is created by design, so this should just be a direct call without the loop. The loop implies iteration that never actually happens and leaves the reader wondering if other action types were intended.
# Instead of the for loop:
config = ManualTaskConfig.create(
db=self.db,
data={
"task_id": manual_task.id,
"config_type": ActionType.access,
...
},
)
ManualTaskConfigField.create(...)| f"(privacy request {privacy_request.id}) in requires_input status - " | ||
| f"(privacy request {privacy_request.id}) in {privacy_request.status.value} status - " | ||
| f"keeping request in current status as it may be waiting for manual input" | ||
| ) |
There was a problem hiding this comment.
Stale log message: The warning still says "keeping request in current status as it may be waiting for manual input" even after the status was generalized. For pending_external, "waiting for manual input" is misleading — it's waiting on an external system (Jira), not a human operator filling in a form. Consider something like "keeping request in current status as it is intentionally waiting (manual input or external system)" to match the comment above it.
| if manual_task.task_type == ManualTaskType.jira_ticket | ||
| else PrivacyRequestStatus.requires_input | ||
| ) | ||
| if self.resources.request.status != awaiting_status: |
There was a problem hiding this comment.
Status transition guard may produce unexpected behavior for mixed-task requests: If a request has both a regular manual task (setting requires_input) and a Jira task (setting pending_external), the last task to run will overwrite the status set by the first. The current guard only skips the save if the status already equals the new awaiting status — it doesn't protect against overwriting a sibling task's status.
This may not be a real scenario today, but worth calling out or adding a comment explaining that mixed task types on a single connection aren't expected.
|
|
||
| return ConnectionConfigurationResponse.model_validate(connection_config) | ||
|
|
||
| def _create_default_jira_configs(self, manual_task: ManualTask) -> None: |
There was a problem hiding this comment.
No tests for _create_default_jira_configs: This new method has no direct test coverage in this PR. The existing tests/service/test_jira_ticket_connection.py doesn't appear to cover this path. It would be valuable to add tests verifying:
- A
ManualTaskConfigwithconfig_type=accessis created when ajira_ticketconnection is set up. - A
ManualTaskConfigFieldwithfield_key="complete"andfield_type=checkboxis created. - That re-creating the connection (update path) does not create duplicate configs (gated by
not connection_config.manual_task).
Greptile SummaryThis PR wires up the Jira DSR lifecycle in Fides by (1) auto-creating a
Two minor style findings in Confidence Score: 4/5PR is safe to merge; the functional logic is correct and all three status-list sites are updated consistently. The core changes — status discrimination, watchdog/requeue/duplication updates, and auto-config creation — are all logically sound and internally consistent. The only findings are two non-blocking style issues in connection_service.py (import ordering and an unnecessary single-element loop). Neither affects runtime behaviour. src/fides/service/connection/connection_service.py — minor import grouping and loop-style issues worth tidying before merge. Important Files Changed
Reviews (1): Last reviewed commit: "Merge branch 'main' into ENG-3146-jira-d..." | Re-trigger Greptile |
| from fides.api.schemas.policy import ActionType | ||
| from fides.api.models.saas_template_dataset import SaasTemplateDataset |
There was a problem hiding this comment.
ActionType was added between two models.* imports, interrupting the import grouping convention in this file. It should be moved to sit alongside the other schemas.* imports (lines 37–51).
| from fides.api.schemas.policy import ActionType | |
| from fides.api.models.saas_template_dataset import SaasTemplateDataset | |
| from fides.api.models.saas_template_dataset import SaasTemplateDataset | |
| from fides.api.schemas.policy import ActionType |
Rule Used: Python imports should always be placed at the top ... (source)
Learnt From
ethyca/fides#6536
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| and the erasure node finds no erasure config so it completes immediately. | ||
| The Jira ticket tracks the whole request, not individual action types. | ||
| """ | ||
| for action_type in (ActionType.access,): |
There was a problem hiding this comment.
The docstring directly above explicitly states "Only an access config is needed," yet the code iterates over a single-element tuple (ActionType.access,). This adds cognitive overhead for no benefit — a reader will wonder whether this is intentional future-proofing or a bug. If there is a plan to add more action_type variants later, a short comment would clarify that intent; if not, the loop should be removed in favour of a direct call.
| for action_type in (ActionType.access,): | |
| config = ManualTaskConfig.create( | |
| db=self.db, | |
| data={ | |
| "task_id": manual_task.id, | |
| "config_type": ActionType.access, | |
| "version": 1, | |
| "is_current": True, | |
| }, | |
| ) | |
| ManualTaskConfigField.create( | |
| db=self.db, | |
| data={ | |
| "task_id": manual_task.id, | |
| "config_id": config.id, | |
| "field_key": "complete", | |
| "field_type": ManualTaskFieldType.checkbox, | |
| "field_metadata": { | |
| "label": "Mark as complete", | |
| "required": True, | |
| }, | |
| }, | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
- Fix stale log message to cover pending_external status Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough reviews @claude and @greptile-apps! Addressing each finding: Import ordering — Already in the correct Unnecessary single-element loop — Fixed — now loops Stale log message — Fixed in PR description mismatch ("two configs") — Now accurate since we create both access + erasure configs. Mixed-task status overwrite — Valid observation. Today a request can't have both regular manual tasks and Jira tasks on the same connection, so this is safe. Will add a comment if we ever support mixed types. Test coverage — Agreed this would be valuable. Will address in a follow-up. |
|
Thanks for the comprehensive fixes! I've verified everything: ✅ Import ordering — Confirmed at line 58 (between other ✅ Loop fix — Verified in for action_type in (ActionType.access, ActionType.erasure):Both configs are now created as needed. ✅ Log message — Updated to "waiting for input or an external system" in ✅ PR description — Now accurately reflects the two-config setup. ✅ Mixed-task concern — Reasonable approach. The current architecture prevents mixing types per connection, so the status-setting logic is safe. ✅ Test coverage — Follow-up approach makes sense given the scope. All the critical issues have been addressed. The implementation looks solid for the Jira DSR lifecycle support! |
Ticket ENG-3146
Description Of Changes
Add infrastructure in fides to support the Jira DSR integration lifecycle:
Auto-create ManualTaskConfig — When a
jira_ticketconnection is created, automatically generate access and erasure configs with a "complete" checkbox field. Without these, the DSR graph node completes immediately because it finds no configs to wait on.Use
pending_externalstatus — Jira manual tasks now setpending_externalinstead ofrequires_input, distinguishing "waiting on Jira" from "operator needs to fill in fields in Fides." The Admin UI already has labels and badges for this status.Treat
pending_externallikerequires_input— The watchdog, duplication detection, and interrupted task requeue all now includepending_externalalongsiderequires_inputto prevent false-positive error transitions.Code Changes
ManualTaskConfigwith a "complete" checkbox field when ajira_ticketManualTaskis created inConnectionService._create_default_jira_configspending_externalinstead ofrequires_inputforjira_tickettask types inManualTaskGraphTask._set_submitted_data_or_raise_awaiting_async_task_callbackpending_externalin the watchdog's in-progress query and the no-auto-error guard inrequest_service.pypending_externalinACTIONED_REQUEST_STATUSESfor duplication detectionSteps to Confirm
jira_ticketconnection config via the Admin UI or APIManualTaskis created with twoManualTaskConfigrecords (access + erasure), each with a "complete" checkbox fieldpending_external(notrequires_input)pending_externalpending_externalbadge should appear in the Admin UI request detailsPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works🤖 Generated with Claude Code