fix(jira): close deleted findings

[samiat4911]

Description

Fixes a Jira synchronization bug where deleting a Finding in DefectDojo did not close the linked Jira issue.

This PR updates the Finding delete flow so that:

• A linked Jira issue is closed during finding_pre_delete.
• Jira closure is skipped when the linked Jira issue is reassigned to a new duplicate-cluster original.
• Local Jira issue records can be reassigned when deleting an original Finding that has duplicates.

Key implementation added:

if instance.has_jira_issue and not getattr(instance, "_skip_jira_close_on_delete", False):
    jira_services.close_issue_for_deleted_finding(instance)

When a deleted original Finding has a duplicate cluster and a new original is selected, the Jira issue is moved instead of closed:

new_original = reconfigure_duplicate_cluster(instance, duplicate_cluster)
jira_issue = jira_services.get_issue(instance)
if new_original and jira_issue:
    jira_services.reassign_issue_to_finding(jira_issue, new_original)
    instance._skip_jira_close_on_delete = True

The Jira helper now closes active linked Jira issues using the configured close transition and updates the local Jira change timestamp after a successful transition.

Test results
Added unit coverage in unittests/test_jira_helper.py for:

• Closing an active linked Jira issue before deleting a Finding.
• Reassigning a local Jira issue record to another Finding.
• Calling Jira close logic from finding_pre_delete.
• Skipping Jira close after reassigning the Jira issue to a new duplicate-cluster original.

Tested locally:

python3 manage.py test unittests.test_jira_helper -v 2 --keepdb --no-input
Ran 9 tests
OK

Also ran:

python -m compileall dojo\finding\helper.py dojo\jira\helper.py dojo\jira\services.py unittests\test_jira_helper.py
git diff --check
ruff check

Documentation

No documentation changes were needed because this fixes existing Jira delete behavior and does not add user-facing configuration.

Checklist

This checklist is for your information.

• Make sure to rebase your PR against the very latest dev.
• Features/Changes should be submitted against the dev.
• Bugfixes should be submitted against the bugfix branch.
• Give a meaningful name to your PR, as it may end up being used in the release notes.
• Your code is Ruff compliant (see ruff.toml).
• Your code is python 3.13 compliant.
• If this is a new feature and not a bug fix, you've included the proper documentation in the docs at https://github.com/DefectDojo/django-DefectDojo/tree/dev/docs as part of this PR.
• Model changes must include the necessary migrations in the dojo/db_migrations folder.
• Add applicable tests to the unit tests.
• Add the proper label to categorize your PR.

[mtesauro]

@valentijnscholten Didn't you do something similar recently?

[valentijnscholten]

Yeah, I raised #14567 :-)

[valentijnscholten]

Thanks for the PR. I think this is a usefule/needed addition. Some improvement areas:

• currently all pushing to JIRA is guarded by either the user explicitly selecting to push to JIRA in the UI or API request. Or by checking the value of finding_jira_sync or push_all_isues. At first glance the delete feature in this PR doesn't seem to align with that?
• it could be possible that the new original already has a JIRA issues linked. Can you make sure that scenario is handled correctly (by skipping the reassign)
• could you add a comment to each JIRA issue that is closed or reassigned?

[samiat4911]

hello @valentijnscholten I made an update regarding your feedback. please review thanks :)

[valentijnscholten]

Two things I don't see accomplished yet:

• users cannot delete a finding via UI or API and instruct Defect Dojo to "push_to_jira"
• if the new original already has an existing Jira_Issue, the current code might crash

[samiat4911]

@valentijnscholten could you review this again thanks

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[valentijnscholten]

Thanks for tackling this — the close/reassign-on-delete flow is genuinely useful and the helper/services layering + unit tests are nicely done. A few things before it's ready, two of them blocking.

Major

1. Cascade deletes auto-close issues and bypass both the opt-in and the reassign logic

finding_pre_delete is a @receiver(pre_delete, sender=Finding), so it fires for every finding deletion. But the reassign + the _skip_jira_close_on_delete flag live in finding_delete(), which is only called from Finding.delete() (models.py, before super().delete()). On a cascade delete (deleting a Test / Engagement / Product), Django's collector fires pre_delete directly and does not call each Finding.delete(). So for cascades:

• finding_delete() never runs → a deleted original's issue is closed instead of reassigned, and the duplicate cluster isn't reconfigured for JIRA.
• _push_to_jira_on_delete is never set → is_delete_sync_allowed falls back to keep_in_sync/push_all → the issue is closed with no user opt-in.

Net effect: deleting a product/engagement/test with JIRA-synced findings silently closes all their tickets. This is the riskiest path and no test covers it. Please route cascade deletes through the same opt-in/reassign logic, or explicitly exclude them.

2. Synchronous blocking JIRA calls inside the delete path

close_jira_issue_for_deleted_finding makes live REST calls (get_jira_connection, jira.issue(...), jira_transition) directly inside finding_pre_delete. Everywhere else in DefectDojo, JIRA pushes are dispatched asynchronously (dojo_dispatch_task / celery — see push_to_jira() in jira/helper.py). Doing it inline means a bulk or cascade delete performs N sequential blocking network calls in a single request → slow and timeout-prone. This should be dispatched async like the rest of the integration.

Design / behavioral

3. push_to_jira=False is silently ignored — inconsistent with is_push_to_jira

The established gate for create/update is is_push_to_jira(instance, push_to_jira_parameter=None) (jira/helper.py:85), which implements explicit three-state precedence:

# caller explicitly stated true or false (False is different from None!)
if push_to_jira_parameter is not None:
    return push_to_jira_parameter
return jira_project.push_all_issues

i.e. an explicit push_to_jira=False suppresses the push even when push_all_issues is on. is_delete_sync_allowed breaks that contract:

return bool(push_to_jira or is_keep_in_sync_with_jira(finding) or is_push_all_issues(finding))

bool(False or keep_in_sync or push_all) collapses False and None, so unchecking the box does nothing on a keep_in_sync/push_all instance — the opposite of every other JIRA push decision (and it also folds in keep_in_sync, which is_push_to_jira doesn't consult). Please mirror is_push_to_jira's precedence (honor an explicit True/False; only fall back to push_all/keep_in_sync when None), ideally by reusing it rather than adding parallel semantics.

4. Behavior change → needs an upgrade note

On keep_in_sync/push_all instances, deleting a finding now closes its linked JIRA issue where previously it did nothing. That's a meaningful behavior change for existing deployments and must be documented in the upgrade notes (docs/content/en/open_source/upgrading/<next-version>.md), ideally with the opt-out story once #3 is fixed.

5. Closing external state in pre_delete

The close happens before the DB delete commits; if the surrounding transaction rolls back, the issue is already closed in JIRA. Minor consistency nit — post-delete (or async-after-commit) is safer.

Minor

• Double confirm() ("delete?" then "push to JIRA?") in the delete JS is clunky UX.
• get_request_boolean — worth checking whether an existing request-boolean helper can be reused.
• Tests are thorough but entirely mocked; none exercise the cascade path in Same name as commercial software #1 (the riskiest one) — please add one that deletes a Test/Engagement containing a synced finding.
• Branch is CONFLICTING with dev and needs a rebase (it touches finding/helper.py next to post_process_findings_batch).

Happy to re-review once #1–#3 are addressed.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[samiat4911]

@valentijnscholten could give it a review I just pushed an update thanks

[valentijnscholten]

@samiat4911 Did you test this latest version? I don't think the on_commit thing will work as it will fire the close task after the finding has been deleted.

[valentijnscholten]

Thanks for iterating on this — the earlier feedback is addressed well: the JIRA push is now gated by is_delete_sync_allowed (proper three-state push_to_jira, plus the keep-in-sync/push-all fallback), users can opt in via the delete form / API param, the "new original already has a linked JIRA issue" case is guarded so it no longer crashes, and both the close and reassign paths post an explanatory comment. Test coverage for those scenarios is solid.

There are a few things to change before this can merge.

1. (Blocking) The close path doesn't actually close the issue in the real flow

JIRA_Issue.finding is OneToOneField(..., on_delete=models.CASCADE), so deleting the finding cascade-deletes the linked JIRA_Issue row in the same transaction.

The close is queued with transaction.on_commit(...), which fires after the delete commits — i.e. after the JIRA_Issue row is already gone. The task then does get_object_or_none(JIRA_Issue, id=jira_issue_id), gets None, and returns "Local JIRA issue ... was not found." without closing anything. So in production the linked issue is never closed.

The reassign path works only because it re-parents the JIRA_Issue to the new original before the cascade, so that row survives. The close path leaves it pointed at the doomed finding.

Fix: dispatch the close task before the delete (no on_commit) and pass the values it needs as arguments, so the task never re-queries the deleted row:

# while the JIRA_Issue is still alive (pre-delete):
dojo_dispatch_task(
    close_deleted_finding_jira_issue,
    jira_issue.jira_id,                              # remote issue id
    get_jira_project(jira_issue).jira_instance_id,   # JIRA_Instance survives the delete
    finding.id,
)

@app.task
def close_deleted_finding_jira_issue(jira_id, jira_instance_id, finding_id):
    jira_instance = JIRA_Instance.objects.get(id=jira_instance_id)
    jira = get_jira_connection(jira_instance)
    issue = jira.issue(jira_id)
    if issue_from_jira_is_active(issue) and jira_transition(jira, issue, jira_instance.close_status_key):
        jira.add_comment(jira_id, f"DefectDojo finding {finding_id} was deleted. Closed automatically.")

The task can stay async — it just must not depend on a row that gets cascade-deleted. (JIRA_Instance/JIRA_Project are fine to load: they survive the delete.) The jira_issue.save(update_fields=["jira_change"]) can be dropped — that row is about to be deleted anyway.

2. (Blocking) Tests don't catch the above — please add an end-to-end test

The current tests pass because they mock the gap away: test_close_jira_issue_for_deleted_finding_queues_close_after_commit patches on_commit to run inline and patches dojo_dispatch_task, with a Mock finding that's never deleted; test_close_deleted_finding_jira_issue_closes_active_issue patches get_object_or_none to return a live issue. Neither exercises "real finding.delete() -> cascade -> task runs -> issue actually transitions."

Please add a test that deletes a real finding with a linked JIRA_Issue and asserts the JIRA transition was actually invoked (mocking only the JIRA HTTP boundary, not the DB lookups).

3. Replace the instance-attribute flags with parameters on delete()

The _jira_delete_sync_requested / _push_to_jira_on_delete / _skip_jira_close_on_delete attributes stashed on the instance (via set_push_to_jira_on_delete) only exist to smuggle parameters into the pre_delete signal. All three user-facing paths already call the model's Finding.delete() (single, bulk loop, and API perform_destroy), so this can be a normal parameter instead:

def delete(self, *args, push_to_jira=<UNSET sentinel>, product_grading_option=True, **kwargs):
    finding_helper.finding_delete(self, push_to_jira=push_to_jira)
    super().delete(*args, **kwargs)

finding_delete then owns the reassign-vs-close decision directly (no _skip_* attribute — it's just if reassigned: ... else: close), and the JIRA block can move out of finding_pre_delete (the found_by.clear() cleanup stays). Use a sentinel default so delete() with no argument (programmatic deletes, cascades that bypass the method) means "no JIRA sync", preserving today's behavior, while None/True/False keep the three-state. Bulk delete just becomes find.delete(push_to_jira=push_to_jira).

4. Make the reassign comment async too

_reassign_jira_issue_to_new_original calls add_simple_comment(...) synchronously on the request/delete path (and before commit, so a rolled-back delete leaves an orphan comment in JIRA). For consistency with the close path and to keep deletes off the JIRA round-trip, dispatch the reassign comment the same async way.

Minor

• The UI checkbox is two-state, so it can only send True/False, never the None/"automatic" mode the API supports. Worth a line in the upgrade note so keep-in-sync users know they must tick the box in the UI.

[samiat4911]

@valentijnscholten I made an update could you pls review? thanks

[valentijnscholten]

can you disconnect this from on_commit? It brings more risks/complications than it solves.

[samiat4911]

done :)

[valentijnscholten]

Thanks for the thorough revision — all the earlier feedback is addressed. 👍

• The close no longer no-ops: close_deleted_finding_jira_issue is dispatched before the delete with durable primitives (jira_id, jira_instance_id, finding_id) and the task uses get_object_or_none(JIRA_Instance, …), so it never reloads the cascade-deleted JIRA_Issue row.
• Real end-to-end coverage: test_deleting_finding_with_push_to_jira_closes_linked_jira_issue deletes an actual finding and asserts the JIRA transition fires (mocking only the JIRA HTTP boundary), plus no-push / cascade-safety / pre-delete-cleanup tests.
• The instance-attribute flags are gone — replaced by a push_to_jira parameter on Finding.delete() with the DELETE_JIRA_SYNC_UNSET sentinel; reassign-vs-close is now plain control flow and the JIRA logic is out of the pre_delete signal.
• Both delete-sync paths are async and symmetric (primitive args, graceful get_object_or_none), and transaction.on_commit is removed from both the close and reassign paths as requested.

JIRA_Instance is on_delete=PROTECT, so deleting a finding/engagement/product never removes the row the tasks load, and a missing instance degrades to a logged no-op rather than a crash.

One thing to keep in mind (not blocking): with on_commit removed, a rolled-back delete would still close the ticket / post the reassign comment. That's the intended consequence of the simpler async dispatch.

LGTM.

[mtesauro]

Approved

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.