Fix #393: keep existing labels on Jira issue (#539)

* Fix #393: keep existing labels on Jira issue

* Extract parts of sync_whiteboard_labels into private functions

* Add comment to explain sorted()

* Rearrange usage of webhook event factories in test_steps

Author: leplatrem

jbi/actions/steps.py

@@ -5,6 +5,7 @@
 """
 
 import logging
+from typing import Optional
 
 from requests import exceptions as requests_exceptions
 
@@ -244,14 +245,47 @@ def maybe_update_components(context: ActionContext, **parameters):
     return context, (resp,)
 
 
+def _whiteboard_as_labels(whiteboard: Optional[str]) -> list[str]:
+    """Split the whiteboard string into a list of labels"""
+    splitted = whiteboard.replace("[", "").split("]") if whiteboard else []
+    stripped = [x.strip() for x in splitted if x not in ["", " "]]
+    # Jira labels can't contain a " ", convert to "."
+    nospace = [wb.replace(" ", ".") for wb in stripped]
+    with_brackets = [f"[{wb}]" for wb in nospace]
+    return ["bugzilla"] + nospace + with_brackets
+
+
+def _build_labels_update(added, removed=None):
+    after = _whiteboard_as_labels(added)
+    # We don't bother detecting if label was already there.
+    updates = [{"add": label} for label in after]
+    if removed:
+        before = _whiteboard_as_labels(removed)
+        deleted = sorted(set(before).difference(set(after)))  # sorted for unit testing
+        updates.extend([{"remove": label} for label in deleted])
+    return updates
+
+
 def sync_whiteboard_labels(context: ActionContext, **parameters):
     """
     Set whiteboard tags as labels on the Jira issue
     """
+
+    # On update of whiteboard field, add/remove corresponding labels
+    if context.event.changes:
+        changes_by_field = {change.field: change for change in context.event.changes}
+        if change := changes_by_field.get("whiteboard"):
+            updates = _build_labels_update(added=change.added, removed=change.removed)
+        else:
+            # Whiteboard field not changed, ignore.
+            return context, ()
+    else:
+        # On creation, just add them all.
+        updates = _build_labels_update(added=context.bug.whiteboard)
+
     try:
-        # Note: fetch existing first, see mozilla/jira-bugzilla-integration#393
-        resp = jira.get_client().update_issue_field(
-            key=context.jira.issue, fields={"labels": context.bug.get_jira_labels()}
+        resp = jira.get_client().update_issue(
+            issue_key=context.jira.issue, update={"update": {"labels": updates}}
         )
     except requests_exceptions.HTTPError as exc:
         if exc.response.status_code != 400:

jbi/models.py

@@ -246,24 +246,6 @@ def is_assigned(self) -> bool:
         """Return `true` if the bug is assigned to a user."""
         return self.assigned_to != "[email protected]"
 
-    def get_whiteboard_as_list(self) -> list[str]:
-        """Convert string whiteboard into list, splitting on ']' and removing '['."""
-        split_list = (
-            self.whiteboard.replace("[", "").split("]") if self.whiteboard else []
-        )
-        return [x.strip() for x in split_list if x not in ["", " "]]
-
-    def get_jira_labels(self) -> list[str]:
-        """
-        whiteboard labels are added as a convenience for users to search in jira;
-        bugzilla is an expected label in Jira
-        since jira-labels can't contain a " ", convert to "."
-        """
-        wb_list = self.get_whiteboard_as_list()
-        wb_bracket_list = [f"[{wb}]" for wb in wb_list]
-        without_spaces = [wb.replace(" ", ".") for wb in (wb_list + wb_bracket_list)]
-        return ["bugzilla"] + without_spaces
-
     def issue_type(self) -> str:
         """Get the Jira issue type for this bug"""
         type_map: dict = {"enhancement": "Task", "task": "Task", "defect": "Bug"}
@@ -368,6 +350,7 @@ class JiraContext(Context):
 
     project: str
     issue: Optional[str]
+    labels: Optional[list[str]]
 
 
 BugId = TypedDict("BugId", {"id": Optional[int]})

jbi/services/jira.py

@@ -69,6 +69,7 @@ def raise_for_status(self, *args, **kwargs):
     get_permissions = instrumented_method(Jira.get_permissions)
     get_project_components = instrumented_method(Jira.get_project_components)
     projects = instrumented_method(Jira.projects)
+    update_issue = instrumented_method(Jira.update_issue)
     update_issue_field = instrumented_method(Jira.update_issue_field)
     set_issue_status = instrumented_method(Jira.set_issue_status)
     issue_add_comment = instrumented_method(Jira.issue_add_comment)

tests/fixtures/factories.py

@@ -7,6 +7,7 @@
     BugzillaBug,
     BugzillaComment,
     BugzillaWebhookEvent,
+    BugzillaWebhookEventChange,
     BugzillaWebhookRequest,
     BugzillaWebhookUser,
     JiraContext,
@@ -74,6 +75,16 @@ def webhook_event_factory(**overrides):
     return BugzillaWebhookEvent.parse_obj(event)
 
 
+def webhook_event_change_factory(**overrides):
+    event = {
+        "field": "field",
+        "removed": "old value",
+        "added": "new value",
+        **overrides,
+    }
+    return BugzillaWebhookEventChange.parse_obj(event)
+
+
 def webhook_factory(**overrides):
     webhook_event = {
         "bug": bug_factory(),

tests/unit/actions/test_steps.py

@@ -9,9 +9,9 @@
 
 from jbi.actions import default
 from jbi.environment import get_settings
-from jbi.models import ActionContext, BugzillaWebhookEventChange
+from jbi.models import ActionContext
 from jbi.services.jira import JiraCreateError
-from tests.fixtures.factories import comment_factory
+from tests.fixtures.factories import comment_factory, webhook_event_change_factory
 
 ALL_STEPS = {
     "new": [
@@ -63,9 +63,7 @@ def test_created_public(
 
 def test_modified_public(context_update_example: ActionContext, mocked_jira):
     context_update_example.event.changes = [
-        BugzillaWebhookEventChange.parse_obj(
-            {"field": "summary", "removed": "", "added": "JBI Test"}
-        )
+        webhook_event_change_factory(field="summary", removed="", added="JBI Test")
     ]
 
     callable_object = default.init(jira_project_key=context_update_example.jira.project)
@@ -74,11 +72,7 @@ def test_modified_public(context_update_example: ActionContext, mocked_jira):
 
     assert context_update_example.bug.extract_from_see_also(), "see_also is not empty"
 
-    mocked_jira.update_issue_field.assert_any_call(
-        key="JBI-234",
-        fields={"labels": ["bugzilla", "devtest", "[devtest]"]},
-    )
-    mocked_jira.update_issue_field.assert_any_call(
+    mocked_jira.update_issue_field.assert_called_once_with(
         key="JBI-234",
         fields={"summary": "JBI Test"},
     )
@@ -501,16 +495,57 @@ def test_sync_whiteboard_labels(context_create_example: ActionContext, mocked_ji
     )
     callable_object(context=context_create_example)
 
-    mocked_jira.update_issue_field.assert_called_once_with(
-        key=context_create_example.jira.issue,
-        fields={"labels": ["bugzilla", "devtest", "[devtest]"]},
+    mocked_jira.update_issue.assert_called_once_with(
+        issue_key=context_create_example.jira.issue,
+        update={
+            "update": {
+                "labels": [
+                    {"add": "bugzilla"},
+                    {"add": "devtest"},
+                    {"add": "[devtest]"},
+                ]
+            }
+        },
+    )
+
+
+def test_sync_whiteboard_labels_update(
+    context_update_example: ActionContext, mocked_jira
+):
+    context_update_example.event.changes = [
+        webhook_event_change_factory(
+            field="whiteboard",
+            removed="[remotesettings] [server]",
+            added="[remotesettings]",
+        )
+    ]
+
+    callable_object = default.init(
+        jira_project_key=context_update_example.jira.project,
+        steps={"existing": ["sync_whiteboard_labels"]},
+    )
+    callable_object(context=context_update_example)
+
+    mocked_jira.update_issue.assert_called_once_with(
+        issue_key=context_update_example.jira.issue,
+        update={
+            "update": {
+                "labels": [
+                    {"add": "bugzilla"},
+                    {"add": "remotesettings"},
+                    {"add": "[remotesettings]"},
+                    {"remove": "[server]"},
+                    {"remove": "server"},
+                ]
+            }
+        },
     )
 
 
 def test_sync_whiteboard_labels_failing(
     context_update_example: ActionContext, mocked_jira, caplog
 ):
-    mocked_jira.update_issue_field.side_effect = requests.exceptions.HTTPError(
+    mocked_jira.update_issue.side_effect = requests.exceptions.HTTPError(
         "some message", response=mock.MagicMock(status_code=400)
     )
 

tests/unit/test_bugzilla.py

@@ -5,32 +5,6 @@
 from tests.fixtures.factories import bug_factory
 
 
-@pytest.mark.parametrize(
-    "whiteboard,expected",
-    [
-        ("", ["bugzilla"]),
-        ("[tag]", ["bugzilla", "tag", "[tag]"]),
-        ("[test whiteboard]", ["bugzilla", "test.whiteboard", "[test.whiteboard]"]),
-        ("[test-whiteboard]", ["bugzilla", "test-whiteboard", "[test-whiteboard]"]),
-        (
-            "[test whiteboard][test-no-space][test-both space-and-not",
-            [
-                "bugzilla",
-                "test.whiteboard",
-                "test-no-space",
-                "test-both.space-and-not",
-                "[test.whiteboard]",
-                "[test-no-space]",
-                "[test-both.space-and-not]",
-            ],
-        ),
-    ],
-)
-def test_jira_labels(whiteboard, expected):
-    bug = bug_factory(whiteboard=whiteboard)
-    assert bug.get_jira_labels() == expected
-
-
 @pytest.mark.parametrize(
     "see_also,expected",
     [