fix: guard against KeyError in get_priority_change_date by using .get()

bugbot/rules/assignee_no_login.py

@@ -136,10 +136,19 @@ def get_priority_change_date(self, bug):
         current_priority = bug["priority"]
 
         for change in reversed(bug["history"]):
-            if (
-                change["field_name"] == "priority"
-                and change["added"] == current_priority
-            ):
+            field_name = change.get("field_name")
+            added = change.get("added")
+            if field_name is None or added is None:
+                # Log so the upstream root cause (history entries missing
+                # expected keys) is visible in production, but continue
+                # iterating so a later valid entry can still match.
+                logger.warning(
+                    "Bug %s: history entry missing 'field_name' or 'added' keys: %r",
+                    bug.get("id"),
+                    change,
+                )
+                continue
+            if field_name == "priority" and added == current_priority:
                 return datetime.strptime(change["when"], "%Y-%m-%dT%H:%M:%SZ")
         return None
 

tests/rules/test_assignee_no_login_priority.py

@@ -0,0 +1,163 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
+# You can obtain one at http://mozilla.org/MPL/2.0/.
+
+"""Regression tests for the AssigneeNoLogin rule's priority-change lookup.
+
+The production fix changed ``change["field_name"]`` and ``change["added"]`` to
+``change.get("field_name")`` and ``change.get("added")`` in
+``bugbot/rules/assignee_no_login.py:get_priority_change_date``. These tests
+exercise both the pre-fix failure mode (a KeyError when history entries are
+missing one of the expected keys) and the fixed behaviour.
+"""
+
+from datetime import datetime
+from unittest.mock import patch
+
+import pytest
+
+from bugbot.people import People
+from bugbot.rules.assignee_no_login import AssigneeNoLogin
+
+
+def _mock_people():
+    """Build a minimal People object that satisfies the Person TypedDict."""
+    return People(
+        [
+            {
+                "mail": "test@example.com",
+                "cn": "Test User",
+                "ismanager": "FALSE",
+                "title": "Tester",
+                "bugzillaEmail": "test@example.com",
+                "bugzillaID": "1",
+                "dn": "mail=test@example.com,o=com,dc=test",
+                "found_on_bugzilla": True,
+                "im": [],
+                "isdirector": "FALSE",
+                "manager": {
+                    "cn": "",
+                    "dn": "mail=manager@example.com,o=com,dc=test",
+                },
+            }
+        ]
+    )
+
+
+@pytest.fixture
+def rule():
+    """Return an AssigneeNoLogin rule with People stubbed out."""
+    with patch.object(People, "get_instance", return_value=_mock_people()):
+        yield AssigneeNoLogin()
+
+
+def test_get_priority_change_date_returns_date_when_history_entry_matches(rule):
+    when = "2024-01-15T12:00:00Z"
+    bug = {
+        "priority": "P1",
+        "history": [
+            {"field_name": "priority", "added": "P3", "when": "2023-01-01T00:00:00Z"},
+            {"field_name": "priority", "added": "P1", "when": when},
+        ],
+    }
+
+    result = rule.get_priority_change_date(bug)
+
+    assert result == datetime(2024, 1, 15, 12, 0, 0)
+
+
+def test_get_priority_change_date_handles_missing_field_name_without_keyerror(rule):
+    """A history entry without ``field_name`` must not crash the lookup."""
+    when = "2024-05-20T08:30:00Z"
+    bug = {
+        "priority": "P2",
+        "history": [
+            # Missing both "field_name" and "added" - this is what crashed
+            # before the fix.
+            {"when": "2024-04-01T00:00:00Z"},
+            {"field_name": "status", "added": "NEW", "when": "2024-04-15T00:00:00Z"},
+            {"field_name": "priority", "added": "P2", "when": when},
+        ],
+    }
+
+    result = rule.get_priority_change_date(bug)
+
+    assert result == datetime(2024, 5, 20, 8, 30, 0)
+
+
+def test_get_priority_change_date_returns_none_when_no_match(rule):
+    bug = {
+        "priority": "P1",
+        "history": [
+            {"field_name": "priority", "added": "P2", "when": "2024-01-01T00:00:00Z"},
+            {
+                "field_name": "status",
+                "added": "RESOLVED",
+                "when": "2024-02-01T00:00:00Z",
+            },
+        ],
+    }
+
+    assert rule.get_priority_change_date(bug) is None
+
+
+def test_get_priority_change_date_skips_entries_missing_field_name(rule):
+    """Entries that lack ``field_name`` must be skipped, not raise KeyError."""
+    bug = {
+        "priority": "P1",
+        "history": [
+            # No "field_name" key at all.
+            {"added": "P1", "when": "2024-03-10T00:00:00Z"},
+        ],
+    }
+
+    # Should not raise KeyError. The fixed implementation falls back to None
+    # when the field_name check fails (None != "priority").
+    assert rule.get_priority_change_date(bug) is None
+
+
+def test_get_priority_change_date_picks_most_recent_match(rule):
+    bug = {
+        "priority": "P3",
+        "history": [
+            {"field_name": "priority", "added": "P1", "when": "2023-06-01T00:00:00Z"},
+            {"field_name": "priority", "added": "P2", "when": "2024-02-01T00:00:00Z"},
+            {"field_name": "priority", "added": "P3", "when": "2024-08-15T00:00:00Z"},
+        ],
+    }
+
+    result = rule.get_priority_change_date(bug)
+
+    assert result == datetime(2024, 8, 15, 0, 0, 0)
+
+
+def test_get_priority_change_date_logs_warning_for_malformed_entry(rule, caplog):
+    """Malformed history entries must surface as a warning, not be silently
+    swallowed, so the upstream root cause is visible in production logs."""
+    import logging
+
+    when = "2024-09-01T10:00:00Z"
+    bug = {
+        "id": 12345,
+        "priority": "P2",
+        "history": [
+            # Valid match placed first so the malformed entry is what the
+            # reversed() iteration actually visits (and the iteration
+            # continues past the warning to find this earlier match).
+            {"field_name": "priority", "added": "P2", "when": when},
+            # Malformed: missing both expected keys.
+            {"when": "2024-07-01T00:00:00Z"},
+        ],
+    }
+
+    with caplog.at_level(logging.WARNING, logger="bugbot"):
+        result = rule.get_priority_change_date(bug)
+
+    # The lookup must still find the earlier, valid entry.
+    assert result == datetime(2024, 9, 1, 10, 0, 0)
+    # And the malformed entry must have been logged.
+    assert any(
+        "history entry missing" in record.getMessage()
+        and "12345" in record.getMessage()
+        for record in caplog.records
+    )