Update jira to only add watchers that are missing (#5073)

There are two issues that this PR fixes:
1. When self.client.add_watcher(issue.jira_issue, watcher) fails
previously, it'd throw an exception resulting in the entire .save or
.create call to fail, which would retry continuously hammering the jira
instance.
2. The function would always try to add watchers to issues regardless if
the watchers were already added previously.

Author: qyeo-sc

src/clusterfuzz/_internal/issue_management/jira/issue_tracker_manager.py

@@ -18,13 +18,16 @@
 import jira
 
 from clusterfuzz._internal.config import db_config
+from clusterfuzz._internal.metrics import logs
+
+UNCREATED_JIRA_ISSUE_ID = -1
 
 
 class IssueTrackerManager:
   """Issue tracker manager."""
 
   def __init__(self, project_name):
-    """"Construct an issue tracker manager instance based on parameters."""
+    """Construct an issue tracker manager instance based on parameters."""
     self._client = None
     self.project_name = project_name
 
@@ -47,13 +50,19 @@ def _create_client(self):
 
   def save(self, issue):
     """Save an issue."""
-    if issue.id == -1:
+    if issue.id == UNCREATED_JIRA_ISSUE_ID:
       return self._create(issue)
     return self._update(issue)
 
   def create(self):
     """Create an issue object locally."""
-    raw_fields = {'id': '-1', 'fields': {'components': [], 'labels': []}}
+    raw_fields = {
+        'id': f'{UNCREATED_JIRA_ISSUE_ID}',
+        'fields': {
+            'components': [],
+            'labels': []
+        }
+    }
     # Create jira issue object
     jira_issue = jira.resources.Issue({},
                                       jira.resilientsession.ResilientSession(),
@@ -75,16 +84,27 @@ def _transition_issue_status_if_updated(self, issue):
     if not isinstance(issue.status, jira.resources.Resource):
       self.client.transition_issue(issue.jira_issue, transition=issue.status)
 
-  def _add_watchers(self, issue):
+  def _add_watchers(self, issue, update_if_different=False):
     """Add watchers to the ticket. Jira has a separate endpoint to
     add watchers."""
 
+    # return if issue was not created yet
+    if issue.id == UNCREATED_JIRA_ISSUE_ID:
+      return
+
     # Get watchers from LabelStore.
-    watchers = list(issue.ccs)
+    watchers = set(issue.ccs)
+
+    if update_if_different:
+      # Only add the missing watchers
+      watchers = watchers - set(self.get_watchers(issue.jira_issue))
 
     # Jira weirdness, update watchers this way.
     for watcher in watchers:
-      self.client.add_watcher(issue.jira_issue, watcher)
+      try:
+        self.client.add_watcher(issue.jira_issue, watcher)
+      except Exception:
+        logs.error(f'Error adding watcher {watcher} to issue {issue.id}')
 
   def _get_issue_fields(self, issue):
     """Get issue fields to populate the ticket"""
@@ -128,12 +148,12 @@ def _update(self, issue):
 
     update_fields = self._get_issue_fields(issue)
     self._transition_issue_status_if_updated(issue)
-    self._add_watchers(issue)
+    self._add_watchers(issue, update_if_different=True)
     issue.jira_issue.update(fields=update_fields)
 
   def get_watchers(self, issue):
     """Retrieve list of watchers."""
-    if issue.id == -1:
+    if issue.id == UNCREATED_JIRA_ISSUE_ID:
       return []
     watchlist = self.client.watchers(issue)
     watchers = []

src/clusterfuzz/_internal/tests/appengine/libs/issue_management/jira/jira_test.py

@@ -183,3 +183,44 @@ def test_issue_save(self):
     self.mock.client.add_comment.assert_called_with(self.mock_issue.jira_issue,
                                                     'test comments')
     self.assertEqual(issue.status, 'Closed')
+
+  def test_issue_save_only_update_missing_watchers(self):
+    """Test save only update missing watchers for existing issue."""
+    self.mock.get_issue.return_value = self.mock_issue
+    self.mock_issue.ccs.add('idontexist@cc.com')
+    self.mock_issue.ccs.add('idontexistalso@cc.com')
+    self.mock.get_watchers.return_value = ['cc@cc.com']
+    issue = self.issue_tracker.get_issue('VSEC-3112')
+    issue.save(new_comment='test comments')
+    self.mock.get_watchers.assert_called()
+    self.mock.client.add_comment.assert_called_with(self.mock_issue.jira_issue,
+                                                    'test comments')
+    self.mock.client.add_watcher.assert_has_calls(
+        [
+            unittest.mock.call(self.mock_issue.jira_issue,
+                               'idontexistalso@cc.com'),
+            unittest.mock.call(self.mock_issue.jira_issue, 'idontexist@cc.com')
+        ],
+        any_order=True)
+
+  def test_issue_save_bad_watcher(self):
+    """Test save still works if a watcher doesn't exist."""
+    self.mock.get_issue.return_value = self.mock_issue
+
+    # Simulate adding a watcher that doesn't exist.
+    def side_affect_func(watcher):
+      if watcher == 'idontexist@cc.com':
+        raise jira.exceptions.JIRAError(
+            text=
+            'The user "idontexist@cc.com" does not have permission to view this issue. This user will not be added to the watch list.',
+            status_code=401)
+
+    self.mock_issue.ccs.add('idontexist@cc.com')
+    self.mock.client.add_watcher.side_effect = side_affect_func
+
+    issue = self.issue_tracker.get_issue('VSEC-3112')
+    issue.status = 'Closed'
+    issue.save(new_comment='test comments')
+    self.mock.client.add_comment.assert_called_with(self.mock_issue.jira_issue,
+                                                    'test comments')
+    self.assertEqual(issue.status, 'Closed')