ENG-3464: Configurable Jira completion status#8046
Conversation
Add `is_fides_complete` column to JiraTicketTask to decouple the Fides completion decision from Jira's status category. Add `completion_status` field to JiraTicketSchema so admins can select which exact Jira status triggers completion instead of the entire "done" category. - Model: add `is_fides_complete` boolean column with partial index - Schema: add `completion_status: str | None` to connection secrets - Migration: add column, backfill existing done rows, replace index - Frontend: add "Completion trigger" dropdown to JiraConfigTab - Fix: seed.py mailgun secrets dict, remove --webpack flag from package.json - Tests: updated get_open_tasks tests for new is_fides_complete filter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities found.Scanned Files
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8046 +/- ##
=======================================
Coverage 85.14% 85.14%
=======================================
Files 637 637
Lines 41936 41937 +1
Branches 4927 4927
=======================================
+ Hits 35707 35708 +1
Misses 5121 5121
Partials 1108 1108 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fix stale OAuth token bug: refresh_token_if_needed now returns the DB-fresh ConnectionConfig so build_jira_client always uses the latest access_token (previously the caller's detached copy was used) - Reduce Jira polling interval default from 10 to 3 minutes - Require issue_type when fetching statuses (frontend skip + backend validation) so completion trigger only shows relevant statuses - Fix mypy error in seed.py for MessagingConfig.set_secrets Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n on issue type change - Rename is_fides_complete to is_resolved across model, migration, and tests for clarity (flag means "terminal", not "succeeded") - Rename migration file to match column name - Backfill deleted tickets as resolved in migration - Clear completion_status when issue type changes (handleIssueTypeChange) - Make RTK query params required (skip guard ensures presence) - Add inline comment on seed.py type ignore - Fix test assertion for polling interval default (10 → 3) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The boolean flag controls whether the poller checks a ticket, not whether the ticket "resolved" (which was ambiguous about success vs failure). needs_polling = True (default) means the poller should check it; False means it reached a terminal state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y params Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/code-review |
There was a problem hiding this comment.
Code Review: ENG-3464 — Configurable Jira completion status
Overall this is a clean, well-scoped change. The core design decision — decoupling the polling flag (needs_polling) from Jira's status category — is the right abstraction. It makes future completion-trigger variations easy to add and avoids re-querying solely based on Jira's opinionated categories. The backward-compat default (completion_status = null → any done-category status) is correct.
What's working well
- Migration is safe:
server_default=truefor the new column, followed by a targeted backfill for terminal rows — correct ordering, no data loss risk. - Partial index: replacing
external_status_category IS NULL OR external_status_category != 'done'withneeds_polling = trueis cleaner and more direct. - Frontend cascading: clearing
completion_statuswhen project or issue type changes is handled correctly at both levels. allowClear+showSearchon the completion status<Select>are the right UX choices for an optional field over a potentially long list of statuses.- Test coverage: the new
test_get_open_tasks_includes_done_category_still_needing_pollingtest captures the key behavioral change precisely.
Notable observations
- Core logic is in the companion PR: The fides side adds the schema field and DB column; the actual logic that reads
completion_statusand setsneeds_polling = falsewhen the status matches lives in fidesplus#3477. Reviewers should confirm the two PRs are reviewed together so the full flow is validated end-to-end. - PR description step 4 says "Verify
needs_pollingcolumn exists in DB with default false" — the migration/model actually set the default totrue. Minor doc issue in the checklist.
Inline comments
Six inline comments attached covering: stale class docstring, polling interval impact, inline response type, migration backfill clarification, seed.py type-ignore, and || vs ?? in the save handler.
🔬 Codegraph: connected (47570 nodes)
💡 Write /code-review in a comment to re-run this review.
| logger.info("Initiating scheduler for Jira ticket polling") | ||
| scheduler.add_job( | ||
| func=poll_jira_tickets, | ||
| func=poll_jira_tickets.delay, |
There was a problem hiding this comment.
Is this pattern something we've done before? it feels a bit weird to me -- have we tested this works as expected?
There was a problem hiding this comment.
Yes, this pattern exists in fidesplus already (poll_query_log_task.delay and digest tasks via .apply_async()), but is new for fides OSS. The rationale is better isolation — the polling task makes external HTTP calls to Jira, so running it on a worker avoids blocking an APScheduler thread slot on the webserver.
Verified locally with task_always_eager=False: the worker picks up the task, acquires the Redis lock, queries Jira, and completes in ~1.3s. No queue contention risk since it goes to the default fides queue, not the DSR queue.
Move our migration to the end of the chain so it depends on 3a91e5d4f7b2 instead of d71c7d274c04, which now has two children. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jpople
left a comment
There was a problem hiding this comment.
Frontend code looks good to me!
When a Jira OAuth token expires, both the Connection tab and Ticket Setup tab now surface the issue clearly with a Re-authorize button that triggers the existing OAuth flow. Previously, users saw empty dropdowns and a failed test with no path to fix the connection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Only show the re-authorize alert on the Ticket Setup tab when the error is specifically a token refresh failure (401 or detail contains "token refresh failed"), not for any 400 error which could be a validation issue. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Plumb failureReason from test connection response through to ConnectionStatusData so the Connection tab can distinguish token failures from other errors. Add unit tests for JiraConfigTab auth error detection covering: token refresh failure, 401, non-auth 400, and missing onReauthorize prop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Auto-trigger test connection after successful OAuth re-authorization using a ref flag + effect to avoid closure staleness - Show "Testing connection…" spinner in status bar while test runs - Use broader check for Connection tab re-authorize button (any failed test on OAuth Jira) since failure_reason isn't available on page load - Plumb failureReason through ConnectionStatusData for future use Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Cast action and label to ReactNode instead of unknown - Use AccessLevel.WRITE enum instead of string literal Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use typed props instead of Record<string, unknown> to avoid unknown-to-ReactNode cast that strict tsc rejects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ticket ENG-3464
Description Of Changes
Allows admins to configure which exact Jira status triggers Fides completion, instead of relying on the entire "done" category. This addresses clients like Toast who use multiple statuses within the
donecategory (e.g., "Done" = privacy work finished, "Complete" = all departments done).Key changes:
needs_pollingboolean column onJiraTicketTask— terminal flag that decouples the Fides completion decision from Jira's status categorycompletion_statusfield in connection secrets — when set, only that exact Jira status name triggers completionrefresh_token_if_neededreturns the DB-freshConnectionConfigsobuild_jira_clientalways uses the latest access tokencompletion_status = nullpreserves current behaviorCode Changes
needs_pollingcolumn toJiraTicketTaskmodel with partial indexcompletion_status: str | NonetoJiraTicketSchemaconnection secrets<Select>toJiraConfigTab, scoped to selected issue type; cascading clear on project/issue type changegetJiraStatuseswith requiredprojectKey+issueTypeparamsrefresh_token_if_needednow returns the freshConnectionConfigseed.pymailgun secrets type error (pass dict instead of Pydantic model)Steps to Confirm
jira_ticketconnection via APIcompletion_status: "Complete"— verify it saves and reads backcompletion_status: null— verify it clearsneeds_pollingcolumn exists in DB with defaultfalsePre-Merge Checklist
CHANGELOG.mdupdated