Fix #210: do not always overwrite summary (#540)

* Change return type of changed_fields()

* Fix #210: do not always overwrite summary

* Add test for situation described in github issue

Author: leplatrem

jbi/actions/steps.py

@@ -76,6 +76,10 @@ def update_issue_summary(context: ActionContext, **parameters):
 
     bug = context.bug
     issue_key = context.jira.issue
+
+    if "summary" not in context.event.changed_fields():
+        return context, ()
+
     logger.debug(
         "Update summary of Jira issue %s for Bug %s",
         issue_key,
@@ -122,9 +126,7 @@ def maybe_assign_jira_user(context: ActionContext, **parameters):
             logger.debug(str(exc), extra=context.dict())
 
     if context.operation == Operation.UPDATE:
-        changed_fields = event.changed_fields() or []
-
-        if "assigned_to" not in changed_fields:
+        if "assigned_to" not in event.changed_fields():
             return context, ()
 
         if not bug.is_assigned():
@@ -166,9 +168,7 @@ def maybe_update_issue_resolution(
         return context, (resp,)
 
     if context.operation == Operation.UPDATE:
-        changed_fields = context.event.changed_fields() or []
-
-        if "resolution" in changed_fields:
+        if "resolution" in context.event.changed_fields():
             resp = jira.update_issue_resolution(context, jira_resolution)
             return context, (resp,)
 
@@ -198,7 +198,7 @@ def maybe_update_issue_status(context: ActionContext, **parameters):
         return context, (resp,)
 
     if context.operation == Operation.UPDATE:
-        changed_fields = context.event.changed_fields() or []
+        changed_fields = context.event.changed_fields()
 
         if "status" in changed_fields or "resolution" in changed_fields:
             resp = jira.update_issue_status(context, jira_status)

jbi/models.py

@@ -182,7 +182,7 @@ class BugzillaWebhookEvent(BaseModel):
     target: Optional[str]
     routing_key: Optional[str]
 
-    def changed_fields(self) -> Optional[list[str]]:
+    def changed_fields(self) -> list[str]:
         """Returns the names of changed fields in a bug"""
         if self.changes:
             return [c.field for c in self.changes]
@@ -192,7 +192,7 @@ def changed_fields(self) -> Optional[list[str]]:
         if self.routing_key is not None and self.routing_key[0:11] == "bug.modify:":
             return self.routing_key[11:].split(",")
 
-        return None
+        return []
 
 
 class BugzillaWebhookAttachment(BaseModel):

jbi/runner.py

@@ -96,11 +96,10 @@ def execute_action(
                 )
 
             if event.target == "bug":
-                changed_fields = event.changed_fields() or []
                 action_context = action_context.update(
                     operation=Operation.UPDATE,
                     extra={
-                        "changed_fields": ", ".join(changed_fields),
+                        "changed_fields": ", ".join(event.changed_fields()),
                         **action_context.extra,
                     },
                 )

tests/unit/actions/test_steps.py

@@ -9,7 +9,7 @@
 
 from jbi.actions import default
 from jbi.environment import get_settings
-from jbi.models import ActionContext
+from jbi.models import ActionContext, BugzillaWebhookEventChange
 from jbi.services.jira import JiraCreateError
 from tests.fixtures.factories import comment_factory
 
@@ -62,6 +62,12 @@ 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"}
+        )
+    ]
+
     callable_object = default.init(jira_project_key=context_update_example.jira.project)
 
     callable_object(context=context_update_example)
@@ -195,12 +201,6 @@ def test_clear_assignee(context_update_example: ActionContext, mocked_jira):
 
     mocked_jira.create_issue.assert_not_called()
     mocked_jira.user_find_by_user_string.assert_not_called()
-    mocked_jira.update_issue_field.assert_any_call(
-        key="JBI-234",
-        fields={
-            "summary": "JBI Test",
-        },
-    )
     mocked_jira.update_issue_field.assert_any_call(
         key="JBI-234",
         fields={"assignee": None},
@@ -224,12 +224,6 @@ def test_set_assignee(context_update_example: ActionContext, mocked_jira):
     mocked_jira.user_find_by_user_string.assert_called_once_with(
         query="[email protected]"
     )
-    mocked_jira.update_issue_field.assert_any_call(
-        key="JBI-234",
-        fields={
-            "summary": "JBI Test",
-        },
-    )
     mocked_jira.update_issue_field.assert_any_call(
         key="JBI-234",
         fields={"assignee": {"accountId": "6254"}},
@@ -324,12 +318,6 @@ def test_change_to_unknown_status(context_update_example: ActionContext, mocked_
 
     mocked_jira.create_issue.assert_not_called()
     mocked_jira.user_find_by_user_string.assert_not_called()
-    mocked_jira.update_issue_field.assert_called_once_with(
-        key="JBI-234",
-        fields={
-            "summary": "JBI Test",
-        },
-    )
     mocked_jira.set_issue_status.assert_not_called()
 
 
@@ -351,12 +339,6 @@ def test_change_to_known_status(context_update_example: ActionContext, mocked_ji
 
     mocked_jira.create_issue.assert_not_called()
     mocked_jira.user_find_by_user_string.assert_not_called()
-    mocked_jira.update_issue_field.assert_called_once_with(
-        key="JBI-234",
-        fields={
-            "summary": "JBI Test",
-        },
-    )
     mocked_jira.set_issue_status.assert_called_once_with("JBI-234", "In Progress")
 
 
@@ -378,12 +360,6 @@ def test_change_to_known_resolution(context_update_example: ActionContext, mocke
 
     mocked_jira.create_issue.assert_not_called()
     mocked_jira.user_find_by_user_string.assert_not_called()
-    mocked_jira.update_issue_field.assert_called_once_with(
-        key="JBI-234",
-        fields={
-            "summary": "JBI Test",
-        },
-    )
     mocked_jira.set_issue_status.assert_called_once_with("JBI-234", "Closed")
 
 
@@ -423,10 +399,7 @@ def test_change_to_unknown_resolution_with_resolution_map(
     )
     callable_object(context=context_update_resolution_example)
 
-    mocked_jira.update_issue_field.assert_called_once_with(
-        key="JBI-234",
-        fields={"summary": "JBI Test"},
-    )
+    mocked_jira.update_issue_field.assert_not_called()
 
 
 @pytest.mark.parametrize(

tests/unit/test_bugzilla.py

@@ -86,7 +86,7 @@ def test_lookup_action_not_found(whiteboard, actions_example):
 
 def test_payload_empty_changes_list(webhook_change_status_assignee):
     webhook_change_status_assignee.event.changes = None
-    assert webhook_change_status_assignee.event.changed_fields() is None
+    assert webhook_change_status_assignee.event.changed_fields() == []
 
 
 def test_payload_changes_list(webhook_change_status_assignee):

tests/unit/test_runner.py

@@ -329,3 +329,24 @@ def test_runner_ignores_if_jira_issue_is_not_readable(
         "Handling incoming request",
         "Ignore incoming request: ignore unreadable issue JBI-234",
     ]
+
+
+def test_runner_ignores_request_if_jira_is_linked_but_without_whiteboard(
+    webhook_comment_example: BugzillaWebhookRequest,
+    actions_example: Actions,
+    settings: Settings,
+    mocked_bugzilla,
+):
+    mocked_bugzilla.get_bug.return_value = webhook_comment_example.bug
+    webhook_comment_example.bug.whiteboard = "[not-matching-local-config]"
+
+    assert webhook_comment_example.bug.extract_from_see_also() is not None
+
+    with pytest.raises(IgnoreInvalidRequestError) as exc_info:
+        execute_action(
+            request=webhook_comment_example,
+            actions=actions_example,
+            settings=settings,
+        )
+
+    assert str(exc_info.value) == "no bug whiteboard matching action tags: devtest"