Check statusCategory instead of the resolution field for Jira issue status

[derda17]

Description

PR for #14347

When a finding is reactivated, the linked jira issue may not be reopened.

Root cause

When a finding is reactivated and before the linked jira issue is reopened, issue_from_jira_is_active checks whether a jira issue is active based on the resolution field. If it is considered active, the issue will not be transitioned to an active state again.
The check based on resolution is not reliable in all situations:

• default resolution cannot be mapped with user screens to choose a resolution, the default would win. Providing separate transitions for user flow and API flow may not be possible, resulting in missing resolutions when the issue is closed via API.
• a default resolution might be set on creation, e.g. 'unresolved', leading to a false positive
• the reopen transition might not properly clear resolutions, leading to a false positive

Fix

• Use the statusCategory to decide if a issue is still active. This is the canonical way provided by Jira since Jira 6.1, released in 2013. Categories are a mandatory field with a fixed enum
  • category value "new" describes a new issue, so it is active
  • category value "indeterminate" describe an issue that is in progress, so it is active
  • category value "done" is an end status, so the issue is not active
  • category value "undefined" is used in case the status has not category assigned, which may happen for old projects not migrated properly
• As a fallback for "undefined" or future enum extension, a fallback to the resolution check is provided

Tests

It seems like there were no unit tests for issue_from_jira_is_active. Several tests are provided, for the happy paths of all status categories, for the fallback checking the resolution, for conflicting category and resolution where the category takes precedence, and some edge cases of missing data.

Backward compatibility

If stated, status category takes precedence. If none is stated or it is "undefined", the old behavior checking the resolution will be used. This includes a bugfix for the resolution check, since the resolution object was compared to a "None" string in the last return statement, always returning false.

Additional Information

This PR is complimentary to #14716 by @paulOsinski and addresses the same root cause.

[derda17]

Some Unit tests failed. I've returned the PR to draft state and will look into the failing tests.

[paulOsinski]

I've opened #14716 which addresses the same root cause you identified in #14347 (resolution is unreliable, statusCategory is canonical), but on the inbound webhook side (process_resolution_from_jira) rather than the outbound issue_from_jira_is_active check. Both of these PRs should work together without interfering with each other.

[github-actions]

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

[derda17]

I will have a look at the merge conflicts this week. Besides that, is there any other blocker or anything I can provide to speed things up?

[github-actions]

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

[derda17]

The branch is updated and merge conflicts are resolved. The ruff linter fails, but the complains are regarding files not touched by this PR and seem to be pre-existing on DEV.
I have updated my initial comment to provide more information about the change and its behaviour.

[valentijnscholten]

Can you try to organize the imports or use ruff check --fix to get rid of the violation?

[derda17]

@valentijnscholten running ruff locally passes all checks just fine. And the imports look similar to other files like test_jira_webhook.py or test_finding_helper.py.

It seems like the jira modules got restructured since I wrote the PR and one of the imports was broken. This should have caused the test failures and maybe also threw off ruff.

[valentijnscholten]

With #14716 now merged we already have code looking at the status category. The only decision we make there is to check if status category == Done. Can you make sure the code in this PR here aligns / re-uses or deduplicates logic around the status category field?

[derda17]

I compared my changes to the ones in #14716. I noticed that I was far more defensive and explicit with my attribute guards, category states and the fallback to the old functionality. If it is fine to skip the fallback to the old functionality, this could drastically simplify the methods. I will provide an updated PR

[derda17]

I aligned the changes with #14716. There is no fallback to the resolution approach anymore. This drastically simplified the function issue_from_jira_is_active and related tests. I extracted the string-based comparison of the category key into issue_status_category_is_done so that it can be re-used instead of introducing the magic string a second time.

A new set of unit tests regarding the status category is introduced. Since they are pure logic tests and there is no need for database verification, they inherit from unittest.TestCase instead of DojoTestCase. The jira status category is a fixed enum, hence I decided to test each individual value anyway instead of just checking "done" and "not done".

[valentijnscholten]

Thank you, but what I meant is that if possible we want to have only one place where we do status_category_key == "done". I believe even with this PR there are still at least two places in the codebase?

[derda17]

Hi @valentijnscholten,
no, there is only the centralized helper method issue_status_category_is_done now that does the string comparison and is called by both flows, #14716 and my proposal.

There is one other place in dojo/jira/view.py:378 that compares the name of the statusCategory against "Done", but this was introduced like 5 years ago and seems to have nothing to do with the current changes. While it would also benefit from switching from a name-based comparison to the key, I would like to avoid extending the scope of the change at hand further and recommend to handle that improvement and it's consequences separately.

[mtesauro]

Approved