Widen scope of 'unmerged branches' to 'inactive branches'.

Rename the 'unmerged branches' metric to 'inactive branches' and enable it to also count branches that have been merged but not deleted.

Closes #1253.

components/api_server/src/initialization/migrations.py

@@ -22,6 +22,7 @@ def perform_migrations(database: Database) -> None:
                 remove_test_cases_manual_number(report),
                 change_ci_subject_types_to_development_environment(report),
                 change_sonarqube_parameters(report),
+                change_unmerged_branches_metrics_to_inactive_branches(report),
             ]
         ):
             change_description = " and to ".join([change for change in changes if change])
@@ -184,6 +185,20 @@ def change_sonarqube_parameter(
     return changed
 
 
+def change_unmerged_branches_metrics_to_inactive_branches(report) -> str:
+    """Change unmerged branches metrics to inactive branches metrics."""
+    # Added after Quality-time v5.19.0, see https://github.com/ICTU/quality-time/issues/1253
+    change = ""
+    for metric in metrics(report, ["unmerged_branches"]):
+        metric["type"] = "inactive_branches"
+        for source in metric.get("sources", {}).values():
+            source["parameters"]["branch_merge_status"] = ["unmerged"]
+        if not metric.get("name"):
+            metric["name"] = "Unmerged branches"
+        change = "metric type 'unmerged_branches' to 'inactive_branches'"
+    return change
+
+
 def log_unknown_parameter_values(value_mapping: dict[str, str], old_values: list[str], value_type: str, report) -> None:
     """Log old parameter values that do not exist in the mapping."""
     if unknown_values := [old_value for old_value in old_values if old_value not in value_mapping]:

components/api_server/tests/initialization/test_migrations.py

@@ -42,6 +42,10 @@ def inserted_report(self, **kwargs):
         del report["_id"]
         return report
 
+    def check_inserted_report(self, inserted_report):
+        """Check that the report was inserted."""
+        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+
 
 class NoOpMigrationTest(MigrationTestCase):
     """Unit tests for empty database and empty reports."""
@@ -80,18 +84,15 @@ def test_report_with_accessibility_metric(self):
         """Test that the migration succeeds with an accessibility metric."""
         self.database.reports.find.return_value = [self.existing_report(metric_type="accessibility")]
         perform_migrations(self.database)
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, self.inserted_report())
+        self.check_inserted_report(self.inserted_report())
 
     def test_accessibility_metric_with_name_and_unit(self):
         """Test that the migration succeeds with an accessibility metric, and existing name and unit are kept."""
         self.database.reports.find.return_value = [
             self.existing_report(metric_type="accessibility", metric_name="name", metric_unit="unit"),
         ]
         perform_migrations(self.database)
-        self.database.reports.replace_one.assert_called_once_with(
-            {"_id": "id"},
-            self.inserted_report(metric_name="name", metric_unit="unit"),
-        )
+        self.check_inserted_report(self.inserted_report(metric_name="name", metric_unit="unit"))
 
     def test_report_with_accessibility_metric_and_other_types(self):
         """Test that the migration succeeds with an accessibility metric and other metric types."""
@@ -103,7 +104,7 @@ def test_report_with_accessibility_metric_and_other_types(self):
         inserted_report = self.inserted_report()
         inserted_report["subjects"][SUBJECT_ID]["metrics"][METRIC_ID2] = {"type": "violations"}
         inserted_report["subjects"][SUBJECT_ID]["metrics"][METRIC_ID3] = {"type": "security_warnings"}
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
 
 class BranchParameterTest(MigrationTestCase):
@@ -139,8 +140,7 @@ def test_report_with_branch_parameter_without_value(self):
             SOURCE_ID: {"type": "gitlab", "parameters": {"branch": "master"}},
             SOURCE_ID2: {"type": "cloc", "parameters": {"branch": ""}},
         }
-        inserted_report = self.inserted_report(metric_type="source_up_to_dateness", sources=inserted_sources)
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(self.inserted_report(metric_type="source_up_to_dateness", sources=inserted_sources))
 
 
 class SourceParameterHashMigrationTest(MigrationTestCase):
@@ -196,7 +196,7 @@ def test_report_with_ci_environment(self):
         inserted_report = self.inserted_report(metric_type="failed_jobs")
         inserted_report["subjects"][SUBJECT_ID]["name"] = "CI-environment"
         inserted_report["subjects"][SUBJECT_ID]["description"] = "A continuous integration environment."
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
     def test_ci_environment_with_title_and_subtitle(self):
         """Test that the migration succeeds with an CI-environment subject, and existing title and subtitle are kept."""
@@ -209,7 +209,7 @@ def test_ci_environment_with_title_and_subtitle(self):
         inserted_report = self.inserted_report(metric_type="failed_jobs")
         inserted_report["subjects"][SUBJECT_ID]["name"] = "CI"
         inserted_report["subjects"][SUBJECT_ID]["description"] = "My CI"
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
 
 class SonarQubeParameterTest(MigrationTestCase):
@@ -245,7 +245,7 @@ def test_report_with_severity_parameter(self):
             metric_type="violations",
             sources=self.sources(impact_severities=["low"]),
         )
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
     def test_report_with_multiple_old_severity_values_that_map_to_the_same_new_value(self):
         """Test a severity parameter with multiple old values that map to the same new value."""
@@ -256,7 +256,7 @@ def test_report_with_multiple_old_severity_values_that_map_to_the_same_new_value
             metric_type="violations",
             sources=self.sources(impact_severities=["low"]),
         )
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
     @disable_logging
     def test_report_with_unknown_old_severity_values(self):
@@ -268,7 +268,7 @@ def test_report_with_unknown_old_severity_values(self):
         inserted_sources = self.sources(impact_severities=["low"])
         inserted_sources[SOURCE_ID2] = {"type": "sonarqube", "parameters": {"branch": "main"}}
         inserted_report = self.inserted_report(metric_type="violations", sources=inserted_sources)
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
     def test_report_with_types_parameter(self):
         """Test that the migration succeeds when the SonarQube source has a types parameter."""
@@ -280,16 +280,15 @@ def test_report_with_types_parameter(self):
             metric_type="violations",
             sources=self.sources(impacted_software_qualities=["reliability"]),
         )
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
     def test_report_with_types_parameter_without_values(self):
         """Test that the migration succeeds when the SonarQube source has a types parameter without values."""
         self.database.reports.find.return_value = [
             self.existing_report(metric_type="violations", sources=self.sources(types=[])),
         ]
         perform_migrations(self.database)
-        inserted_report = self.inserted_report(metric_type="violations", sources=self.sources())
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(self.inserted_report(metric_type="violations", sources=self.sources()))
 
     def test_report_with_security_types_parameter(self):
         """Test that the migration succeeds when the SonarQube source has a security types parameter."""
@@ -302,7 +301,7 @@ def test_report_with_security_types_parameter(self):
         perform_migrations(self.database)
         inserted_sources = self.sources(security_types=["issue with security impact", "security hotspot"])
         inserted_report = self.inserted_report(metric_type="security_warnings", sources=inserted_sources)
-        self.database.reports.replace_one.assert_called_once_with({"_id": "id"}, inserted_report)
+        self.check_inserted_report(inserted_report)
 
     def test_report_with_security_types_parameter_without_values(self):
         """Test that the migration succeeds when the SonarQube source has a security types parameter without values."""
@@ -336,18 +335,49 @@ def test_report_with_test_cases_and_manual_number_source(self):
             self.existing_report(metric_type="test_cases", sources=self.sources()),
         ]
         perform_migrations(self.database)
-        self.database.reports.replace_one.assert_called_once_with(
-            {"_id": "id"},
-            self.inserted_report(metric_type="test_cases", sources={}),
-        )
+        self.check_inserted_report(self.inserted_report(metric_type="test_cases", sources={}))
 
     def test_report_with_test_cases_and_jira_and_manual_number_source(self):
         """Test that the manual number source is removed."""
         self.database.reports.find.return_value = [
             self.existing_report(metric_type="test_cases", sources=self.sources("jira", "manual_number")),
         ]
         perform_migrations(self.database)
-        self.database.reports.replace_one.assert_called_once_with(
-            {"_id": "id"},
-            self.inserted_report(metric_type="test_cases", sources=self.sources("jira")),
+        self.check_inserted_report(self.inserted_report(metric_type="test_cases", sources=self.sources("jira")))
+
+
+class UnmergedToInactiveBranchesTest(MigrationTestCase):
+    """Unit tests for the unmerged to inactive branches migration."""
+
+    def setUp(self):
+        """Create test fixture."""
+        super().setUp()
+        self.sources = {SOURCE_ID: {"type": "gitlab", "parameters": {}}}
+        self.expected_sources = {SOURCE_ID: {"type": "gitlab", "parameters": {"branch_merge_status": ["unmerged"]}}}
+
+    def test_report_with_unmerged_branches_metric(self):
+        """Test that an unmerged branches metric is migrated."""
+        self.database.reports.find.return_value = [
+            self.existing_report(metric_type="unmerged_branches", sources=self.sources),
+        ]
+        perform_migrations(self.database)
+        inserted_report = self.inserted_report(
+            metric_type="inactive_branches",
+            metric_name="Unmerged branches",
+            sources=self.expected_sources,
+        )
+        self.check_inserted_report(inserted_report)
+
+    def test_report_with_unmerged_branches_metric_with_name(self):
+        """Test that the metric name is not changed when migrating the type."""
+        metric_name = "Old branches"
+        self.database.reports.find.return_value = [
+            self.existing_report(metric_type="unmerged_branches", metric_name=metric_name, sources=self.sources),
+        ]
+        perform_migrations(self.database)
+        inserted_report = self.inserted_report(
+            metric_type="inactive_branches",
+            metric_name=metric_name,
+            sources=self.expected_sources,
         )
+        self.check_inserted_report(inserted_report)

components/collector/.vulture_ignore_list.py

@@ -14,7 +14,7 @@
 AzureDevopsJobRunsWithinTimePeriod  # unused class (src/source_collectors/azure_devops/job_runs_within_time_period.py:11)
 AzureDevopsSourceUpToDateness  # unused class (src/source_collectors/azure_devops/source_up_to_dateness.py:53)
 __new__  # unused function (src/source_collectors/azure_devops/source_up_to_dateness.py:56)
-AzureDevopsUnmergedBranches  # unused class (src/source_collectors/azure_devops/unmerged_branches.py:15)
+AzureDevopsInactiveBranches  # unused class (src/source_collectors/azure_devops/unmerged_branches.py:15)
 AzureDevopsUnusedJobs  # unused class (src/source_collectors/azure_devops/unused_jobs.py:11)
 AzureDevopsUserStoryPoints  # unused class (src/source_collectors/azure_devops/user_story_points.py:12)
 BanditSecurityWarnings  # unused class (src/source_collectors/bandit/security_warnings.py:10)
@@ -61,7 +61,7 @@
 GitLabSourceUpToDateness  # unused class (src/source_collectors/gitlab/source_up_to_dateness.py:93)
 __new__  # unused function (src/source_collectors/gitlab/source_up_to_dateness.py:96)
 GitLabSourceVersion  # unused class (src/source_collectors/gitlab/source_version.py:11)
-GitLabUnmergedBranches  # unused class (src/source_collectors/gitlab/unmerged_branches.py:15)
+GitLabInactiveBranches  # unused class (src/source_collectors/gitlab/unmerged_branches.py:15)
 GitLabUnusedJobs  # unused class (src/source_collectors/gitlab/unused_jobs.py:11)
 scan_status  # unused variable (src/source_collectors/harbor/security_warnings.py:58)
 HarborSecurityWarnings  # unused class (src/source_collectors/harbor/security_warnings.py:62)

components/collector/src/base_collectors/__init__.py

@@ -11,13 +11,13 @@
 )
 from .metric_collector import MetricCollector
 from .source_collector import (
+    InactiveBranchesSourceCollector,
     SecurityWarningsSourceCollector,
     SlowTransactionsCollector,
     SourceCollector,
     SourceMeasurement,
     TimePassedCollector,
     TimeRemainingCollector,
     TransactionEntity,
-    UnmergedBranchesSourceCollector,
     VersionCollector,
 )

components/collector/src/base_collectors/source_collector.py

@@ -219,24 +219,58 @@ async def _landing_url(self, responses: SourceResponses) -> URL:
         return URL(url.removesuffix("xml") + "html" if url.endswith(".xml") else url)
 
 
-class UnmergedBranchesSourceCollector(SourceCollector, ABC):
-    """Base class for unmerged branches source collectors."""
+class InactiveBranchesSourceCollector(SourceCollector, ABC):
+    """Base class for inactive branches source collectors."""
 
     async def _parse_entities(self, responses: SourceResponses) -> Entities:
-        """Override to get the unmerged branches from the unmerged branches method that subclasses should implement."""
+        """Override to get the inactive branches from the inactive branches method that subclasses should implement."""
         return Entities(
             Entity(
                 key=branch["name"],
                 name=branch["name"],
                 commit_date=str(self._commit_datetime(branch).date()),
                 url=str(self._branch_landing_url(branch)),
             )
-            for branch in await self._unmerged_branches(responses)
+            for branch in await self._inactive_branches(responses)
         )
 
+    async def _inactive_branches(self, responses: SourceResponses) -> list[dict[str, Any]]:
+        """Return the list of inactive branches."""
+        return [
+            branch
+            for branch in await self._branches(responses)
+            if not self._is_default_branch(branch)
+            and self._has_allowed_merge_status(branch)
+            and self._is_branch_inactive(branch)
+            and not self._ignore_branch(branch)
+        ]
+
+    @abstractmethod
+    async def _branches(self, responses: SourceResponses) -> list:
+        """Return a list of branches from the responses."""
+
+    @abstractmethod
+    def _is_default_branch(self, branch) -> bool:
+        """Return whether the branch is the default branch."""
+
+    def _has_allowed_merge_status(self, branch) -> bool:
+        """Return whether the branch has the configured status."""
+        allowed_statuses = self._parameter("branch_merge_status")
+        if self._is_branch_merged(branch):
+            return "merged" in allowed_statuses
+        return "unmerged" in allowed_statuses
+
     @abstractmethod
-    async def _unmerged_branches(self, responses: SourceResponses) -> list[dict[str, Any]]:
-        """Return the list of unmerged branches."""
+    def _is_branch_merged(self, branch) -> bool:
+        """Return whether the branch has been merged with the default branch."""
+
+    def _is_branch_inactive(self, branch) -> bool:
+        """Return whether the branch has not recently been committed to."""
+        return days_ago(self._commit_datetime(branch)) > int(cast(str, self._parameter("inactive_days")))
+
+    def _ignore_branch(self, branch) -> bool:
+        """Return whether to ignore the branch."""
+        return match_string_or_regular_expression(branch["name"], self._parameter("branches_to_ignore"))
 
     @abstractmethod
     def _commit_datetime(self, branch) -> datetime:

components/collector/src/source_collectors/__init__.py

@@ -12,12 +12,12 @@
 from .azure_devops.average_issue_lead_time import AzureDevopsAverageIssueLeadTime
 from .azure_devops.change_failure_rate import AzureDevopsChangeFailureRate
 from .azure_devops.failed_jobs import AzureDevopsFailedJobs
+from .azure_devops.inactive_branches import AzureDevopsInactiveBranches
 from .azure_devops.issues import AzureDevopsIssues
 from .azure_devops.job_runs_within_time_period import AzureDevopsJobRunsWithinTimePeriod
 from .azure_devops.merge_requests import AzureDevopsMergeRequests
 from .azure_devops.source_up_to_dateness import AzureDevopsSourceUpToDateness
 from .azure_devops.tests import AzureDevopsTests
-from .azure_devops.unmerged_branches import AzureDevopsUnmergedBranches
 from .azure_devops.unused_jobs import AzureDevopsUnusedJobs
 from .azure_devops.user_story_points import AzureDevopsUserStoryPoints
 from .bandit.security_warnings import BanditSecurityWarnings
@@ -51,12 +51,12 @@
 from .github.merge_requests import GitHubMergeRequests
 from .gitlab.change_failure_rate import GitLabChangeFailureRate
 from .gitlab.failed_jobs import GitLabFailedJobs
+from .gitlab.inactive_branches import GitLabInactiveBranches
 from .gitlab.job_runs_within_time_period import GitLabJobRunsWithinTimePeriod
 from .gitlab.merge_requests import GitLabMergeRequests
 from .gitlab.pipeline_duration import GitLabPipelineDuration
 from .gitlab.source_up_to_dateness import GitLabSourceUpToDateness
 from .gitlab.source_version import GitLabSourceVersion
-from .gitlab.unmerged_branches import GitLabUnmergedBranches
 from .gitlab.unused_jobs import GitLabUnusedJobs
 from .harbor.security_warnings import HarborSecurityWarnings
 from .harbor_json.security_warnings import HarborJSONSecurityWarnings