From 3ca4c5bb25e289359ba132fd84363592a2648536 Mon Sep 17 00:00:00 2001
From: ryneeverett <ryneeverett@gmail.com>
Date: Fri, 18 Oct 2024 00:20:26 -0400
Subject: [PATCH] Remove issue filtering from base Service class

We here remove the abstract `get_owner` method and the concrete
`include` method from the base class and copy the `include` method to
the handful of services which make use of it. The `include` method was
only being used in 6 of 26 services, so I think the duplication is
acceptable in exchange for the benefits:

- This eliminates the last instance of bidirectional communication
  between the base classes and the service implementations, which in my
  opinion is an architectural problem.
- Since most of the services don't use this filtering mechanism, it is
  annoying to have to define `get_owner`.
- This filtering mechanism is somewhat legacy from the days when API's
  were generally far less capable. I don't think we should encourage it
  going forward because it is better to implement filtering through the
  API query rather than pulling all data and iterating over it locally,
  and this is a reasonable expectation from third party API's these
  days.

One might suspect that eliminating these NotImplementedError's would
eliminate the warnings from services that do not support
only_if_assigned, but in fact these would only get raised if the service
itself called `self.include`, which they do not. So a user trying to use
these options already would find them silently ignored, which isn't
great but is a problem for another day.
---
 bugwarrior/docs/contributing/new-service.rst |  8 ------
 bugwarrior/services/activecollab.py          |  4 ---
 bugwarrior/services/activecollab2.py         |  5 ----
 bugwarrior/services/azuredevops.py           |  4 ---
 bugwarrior/services/base.py                  | 18 --------------
 bugwarrior/services/bitbucket.py             | 13 ++++++++++
 bugwarrior/services/bts.py                   |  5 ----
 bugwarrior/services/bz.py                    | 13 ++++++++++
 bugwarrior/services/deck.py                  | 13 ++++++++++
 bugwarrior/services/gerrit.py                |  5 ----
 bugwarrior/services/gitbug.py                |  6 -----
 bugwarrior/services/github.py                | 13 +++++++++-
 bugwarrior/services/gmail.py                 |  4 ---
 bugwarrior/services/jira.py                  |  5 ----
 bugwarrior/services/kanboard.py              |  5 ----
 bugwarrior/services/logseq.py                |  6 -----
 bugwarrior/services/pagure.py                | 13 ++++++++++
 bugwarrior/services/phab.py                  |  4 ---
 bugwarrior/services/pivotaltracker.py        |  8 ------
 bugwarrior/services/redmine.py               |  4 ---
 bugwarrior/services/taiga.py                 |  5 ----
 bugwarrior/services/teamwork_projects.py     |  3 ---
 bugwarrior/services/trac.py                  | 13 ++++++++++
 bugwarrior/services/trello.py                |  4 ---
 bugwarrior/services/youtrack.py              |  5 ----
 tests/test_service.py                        | 26 ++++++++++++++++++++
 26 files changed, 103 insertions(+), 109 deletions(-)

diff --git a/bugwarrior/docs/contributing/new-service.rst b/bugwarrior/docs/contributing/new-service.rst
index e6827535..434d3566 100644
--- a/bugwarrior/docs/contributing/new-service.rst
+++ b/bugwarrior/docs/contributing/new-service.rst
@@ -187,12 +187,6 @@ Now for the main service class which bugwarrior will invoke to fetch issues.
 
           self.client = GitBugClient(path=self.config.path, port=self.config.port)
 
-      def get_owner(self, issue):
-          # Issue assignment hasn't been implemented in upstream git-bug yet.
-          # See https://github.com/MichaelMure/git-bug/issues/112.
-          raise NotImplementedError(
-              "This service has not implemented support for 'only_if_assigned'.")
-
       def issues(self):
           for issue in self.client.get_issues():
               comments = issue.pop('comments')
@@ -209,8 +203,6 @@ Now for the main service class which bugwarrior will invoke to fetch issues.
 
 Here we see two required class attributes (pointing to the classes we previously defined) and two required methods.
 
-The ``get_owner`` method takes an individual issue and returns the "assigned" user, so that bugwarrior can filter issues on this basis. In this case git-bug has not yet implemented this feature, but it generally will just involve returning a value from the ``issue`` dictionary.
-
 The ``issues`` method is a generator which yields individual issue dictionaries.
 
 7. Service Registration
diff --git a/bugwarrior/services/activecollab.py b/bugwarrior/services/activecollab.py
index 7d1fa4ce..abe824a9 100644
--- a/bugwarrior/services/activecollab.py
+++ b/bugwarrior/services/activecollab.py
@@ -193,10 +193,6 @@ def _comments(self, issue):
                          body=comment['body']))
         return comments_formatted
 
-    def get_owner(self, issue):
-        if issue['assignee_id']:
-            return issue['assignee_id']
-
     def annotations(self, issue, issue_obj):
         if 'type' not in issue:
             # Subtask
diff --git a/bugwarrior/services/activecollab2.py b/bugwarrior/services/activecollab2.py
index ae0181f4..44150010 100644
--- a/bugwarrior/services/activecollab2.py
+++ b/bugwarrior/services/activecollab2.py
@@ -204,11 +204,6 @@ def __init__(self, *args, **kw):
                                           self.config.projects,
                                           self.config.target)
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def issues(self):
         # Loop through each project
         start = time.time()
diff --git a/bugwarrior/services/azuredevops.py b/bugwarrior/services/azuredevops.py
index 0dbb253e..ecdcdd76 100644
--- a/bugwarrior/services/azuredevops.py
+++ b/bugwarrior/services/azuredevops.py
@@ -249,10 +249,6 @@ def issues(self):
             issue_obj.extra.update(extra)
             yield issue_obj
 
-    def get_owner(self, issue):
-        # Issue filtering is implemented as part of issue aggregation.
-        pass
-
     @staticmethod
     def get_keyring_service(config):
         return f"azuredevops://{config.organization}@{config.host}"
diff --git a/bugwarrior/services/base.py b/bugwarrior/services/base.py
index d2f98244..3fac1591 100644
--- a/bugwarrior/services/base.py
+++ b/bugwarrior/services/base.py
@@ -106,24 +106,6 @@ def build_annotations(self, annotations, url=None):
                 final.append('@%s - %s' % (author, message))
         return final
 
-    def include(self, issue):
-        """ Return true if the issue in question should be included """
-        if self.config.only_if_assigned:
-            owner = self.get_owner(issue)
-            include_owners = [self.config.only_if_assigned]
-
-            if self.config.also_unassigned:
-                include_owners.append(None)
-
-            return owner in include_owners
-
-        return True
-
-    @abc.abstractmethod
-    def get_owner(self, issue):
-        """ Override this for filtering on tickets """
-        raise NotImplementedError()
-
     @abc.abstractmethod
     def issues(self):
         """ Returns a list of Issue instances representing issues from a remote service.
diff --git a/bugwarrior/services/bitbucket.py b/bugwarrior/services/bitbucket.py
index a0609a28..6ca5efa1 100644
--- a/bugwarrior/services/bitbucket.py
+++ b/bugwarrior/services/bitbucket.py
@@ -179,6 +179,19 @@ def get_owner(self, issue):
         if assignee is not None:
             return assignee.get('username', None)
 
+    def include(self, issue):
+        """ Return true if the issue in question should be included """
+        if self.config.only_if_assigned:
+            owner = self.get_owner(issue)
+            include_owners = [self.config.only_if_assigned]
+
+            if self.config.also_unassigned:
+                include_owners.append(None)
+
+            return owner in include_owners
+
+        return True
+
     def issues(self):
         user = self.config.username
         response = self.get_collection('/repositories/' + user + '/')
diff --git a/bugwarrior/services/bts.py b/bugwarrior/services/bts.py
index 83f18361..e41208ad 100644
--- a/bugwarrior/services/bts.py
+++ b/bugwarrior/services/bts.py
@@ -142,11 +142,6 @@ class BTSService(Service, Client):
     ISSUE_CLASS = BTSIssue
     CONFIG_SCHEMA = BTSConfig
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def _record_for_bug(self, bug):
         return {'number': bug.bug_num,
                 'url': 'https://bugs.debian.org/' + str(bug.bug_num),
diff --git a/bugwarrior/services/bz.py b/bugwarrior/services/bz.py
index 81963815..c24df161 100644
--- a/bugwarrior/services/bz.py
+++ b/bugwarrior/services/bz.py
@@ -189,6 +189,19 @@ def get_keyring_service(config):
     def get_owner(self, issue):
         return issue['assigned_to']
 
+    def include(self, issue):
+        """ Return true if the issue in question should be included """
+        if self.config.only_if_assigned:
+            owner = self.get_owner(issue)
+            include_owners = [self.config.only_if_assigned]
+
+            if self.config.also_unassigned:
+                include_owners.append(None)
+
+            return owner in include_owners
+
+        return True
+
     def annotations(self, tag, issue):
         base_url = "%s/show_bug.cgi?id=" % self.config.base_uri
         long_url = base_url + str(issue['id'])
diff --git a/bugwarrior/services/deck.py b/bugwarrior/services/deck.py
index 8c2e80c4..039056b2 100644
--- a/bugwarrior/services/deck.py
+++ b/bugwarrior/services/deck.py
@@ -166,6 +166,19 @@ def __init__(self, *args, **kwargs):
     def get_owner(self, issue):
         return issue[issue.ASSIGNEE]
 
+    def include(self, issue):
+        """ Return true if the issue in question should be included """
+        if self.config.only_if_assigned:
+            owner = self.get_owner(issue)
+            include_owners = [self.config.only_if_assigned]
+
+            if self.config.also_unassigned:
+                include_owners.append(None)
+
+            return owner in include_owners
+
+        return True
+
     def filter_boards(self, board):
         # include_board_ids takes precedence over exclude_board_ids
         if self.config.include_board_ids:
diff --git a/bugwarrior/services/gerrit.py b/bugwarrior/services/gerrit.py
index 54221e04..fd133c06 100644
--- a/bugwarrior/services/gerrit.py
+++ b/bugwarrior/services/gerrit.py
@@ -116,11 +116,6 @@ def __init__(self, *args, **kw):
     def get_keyring_service(config):
         return f"gerrit://{config.base_uri}"
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def issues(self):
         # Construct the whole url by hand here, because otherwise requests will
         # percent-encode the ':' characters, which gerrit doesn't like.
diff --git a/bugwarrior/services/gitbug.py b/bugwarrior/services/gitbug.py
index daf30388..98efbcf1 100644
--- a/bugwarrior/services/gitbug.py
+++ b/bugwarrior/services/gitbug.py
@@ -157,12 +157,6 @@ def __init__(self, *args, **kwargs):
             port=self.config.port,
             annotation_comments=self.main_config.annotation_comments)
 
-    def get_owner(self, issue):
-        # Issue assignment hasn't been implemented in upstream git-bug yet.
-        # See https://github.com/MichaelMure/git-bug/issues/112.
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def issues(self):
         for issue in self.client.get_issues():
             comments = issue.pop('comments')
diff --git a/bugwarrior/services/github.py b/bugwarrior/services/github.py
index 03f2e457..4b1475ce 100644
--- a/bugwarrior/services/github.py
+++ b/bugwarrior/services/github.py
@@ -439,12 +439,23 @@ def filter_repo_name(self, name):
         return True
 
     def include(self, issue):
+        """ Return true if the issue in question should be included """
         if 'pull_request' in issue[1]:
             if self.config.exclude_pull_requests:
                 return False
             if not self.config.filter_pull_requests:
                 return True
-        return super().include(issue)
+
+        if self.config.only_if_assigned:
+            owner = self.get_owner(issue)
+            include_owners = [self.config.only_if_assigned]
+
+            if self.config.also_unassigned:
+                include_owners.append(None)
+
+            return owner in include_owners
+
+        return True
 
     def issues(self):
         issues = {}
diff --git a/bugwarrior/services/gmail.py b/bugwarrior/services/gmail.py
index f9c960aa..5ee27150 100644
--- a/bugwarrior/services/gmail.py
+++ b/bugwarrior/services/gmail.py
@@ -204,10 +204,6 @@ def annotations(self, issue):
         issue_url = issue.extra['url']
         return self.build_annotations([(sender, subj)], issue_url)
 
-    def get_owner(self, issue):
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def issues(self):
         labels = self.get_labels()
         for thread in self.get_threads():
diff --git a/bugwarrior/services/jira.py b/bugwarrior/services/jira.py
index 4005d003..8019b57e 100644
--- a/bugwarrior/services/jira.py
+++ b/bugwarrior/services/jira.py
@@ -413,11 +413,6 @@ def __init__(self, *args, **kw):
     def get_keyring_service(config):
         return f"jira://{config.username}@{config.base_uri}"
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def body(self, issue):
         body = issue.record.get('fields', {}).get('description')
 
diff --git a/bugwarrior/services/kanboard.py b/bugwarrior/services/kanboard.py
index 731da585..d5b1bd73 100644
--- a/bugwarrior/services/kanboard.py
+++ b/bugwarrior/services/kanboard.py
@@ -163,11 +163,6 @@ def issues(self):
 
             yield self.get_issue_for_record(task, extra)
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     @staticmethod
     def get_keyring_service(config):
         parsed = urlparse(config.url)
diff --git a/bugwarrior/services/logseq.py b/bugwarrior/services/logseq.py
index dc57eb18..cd143c0f 100644
--- a/bugwarrior/services/logseq.py
+++ b/bugwarrior/services/logseq.py
@@ -296,12 +296,6 @@ def __init__(self, *args, **kwargs):
             filter=filter,
         )
 
-    def get_owner(self, issue):
-        # Issue assignment hasn't been implemented yet.
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'."
-        )
-
     def issues(self):
         graph_name = self.client.get_graph_name()
         for issue in self.client.get_issues():
diff --git a/bugwarrior/services/pagure.py b/bugwarrior/services/pagure.py
index 89c8ec18..ffceb553 100644
--- a/bugwarrior/services/pagure.py
+++ b/bugwarrior/services/pagure.py
@@ -154,6 +154,19 @@ def get_owner(self, issue):
         if issue[1]['assignee']:
             return issue[1]['assignee']['name']
 
+    def include(self, issue):
+        """ Return true if the issue in question should be included """
+        if self.config.only_if_assigned:
+            owner = self.get_owner(issue)
+            include_owners = [self.config.only_if_assigned]
+
+            if self.config.also_unassigned:
+                include_owners.append(None)
+
+            return owner in include_owners
+
+        return True
+
     def filter_repos(self, repo):
         if repo in self.config.exclude_repos:
             return False
diff --git a/bugwarrior/services/phab.py b/bugwarrior/services/phab.py
index 3aaf9354..57ae2da1 100644
--- a/bugwarrior/services/phab.py
+++ b/bugwarrior/services/phab.py
@@ -243,10 +243,6 @@ def revisions(self):
             }
             yield self.get_issue_for_record(diff, extra)
 
-    def get_owner(self, issue):
-        # Issue filtering is implemented as part of issue aggregation.
-        pass
-
     def issues(self):
         yield from self.tasks()
         yield from self.revisions()
diff --git a/bugwarrior/services/pivotaltracker.py b/bugwarrior/services/pivotaltracker.py
index e2b57d56..d2bd1b89 100644
--- a/bugwarrior/services/pivotaltracker.py
+++ b/bugwarrior/services/pivotaltracker.py
@@ -71,10 +71,6 @@ class PivotalTrackerIssue(Issue):
 
     UNIQUE_KEY = (URL,)
 
-    def get_owner(self, issue):
-        _, issue = issue
-        return issue.get('pivotalowners')
-
     def to_taskwarrior(self):
         description = self.record.get('description')
         created = self.parse_date(self.record.get('created_at'))
@@ -148,10 +144,6 @@ def __init__(self, *args, **kwargs):
             if self.config.only_if_author:
                 self.query += f" requester:{self.config.user_id}"
 
-    def get_owner(self, issue):
-        # Issue filtering is implemented as part of the api query.
-        pass
-
     def annotations(self, annotations, story):
         final_annotations = []
         if self.main_config.annotation_comments:
diff --git a/bugwarrior/services/redmine.py b/bugwarrior/services/redmine.py
index e56055f8..8bc7eafa 100644
--- a/bugwarrior/services/redmine.py
+++ b/bugwarrior/services/redmine.py
@@ -265,10 +265,6 @@ def __init__(self, *args, **kw):
     def get_keyring_service(config):
         return f"redmine://{config.login}@{config.url}/"
 
-    def get_owner(self, issue):
-        # Issue filtering is implemented as part of the api query.
-        pass
-
     def issues(self):
         issues = self.client.find_issues(
             self.config.issue_limit, self.config.query, self.config.only_if_assigned)
diff --git a/bugwarrior/services/taiga.py b/bugwarrior/services/taiga.py
index df763794..d5b90ddd 100644
--- a/bugwarrior/services/taiga.py
+++ b/bugwarrior/services/taiga.py
@@ -80,11 +80,6 @@ def __init__(self, *args, **kw):
     def get_keyring_service(config):
         return f"taiga://{config.base_uri}"
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def _issues(self, userid, task_type, task_type_plural, task_type_short):
         log.debug('Getting %s' % task_type_plural)
 
diff --git a/bugwarrior/services/teamwork_projects.py b/bugwarrior/services/teamwork_projects.py
index bc6cbd05..8c7d220f 100644
--- a/bugwarrior/services/teamwork_projects.py
+++ b/bugwarrior/services/teamwork_projects.py
@@ -145,9 +145,6 @@ def get_comments(self, issue):
                 return self.build_annotations(comment_list, None)
         return []
 
-    def get_owner(self, issue):
-        return issue.get_owner()
-
     def issues(self):
         response = self.client.call_api("GET", "/tasks.json")
         for issue in response["todo-items"]:
diff --git a/bugwarrior/services/trac.py b/bugwarrior/services/trac.py
index 208a7b53..1ed5d4ab 100644
--- a/bugwarrior/services/trac.py
+++ b/bugwarrior/services/trac.py
@@ -131,6 +131,19 @@ def get_owner(self, issue):
         tag, issue = issue
         return issue.get('owner', None) or None
 
+    def include(self, issue):
+        """ Return true if the issue in question should be included """
+        if self.config.only_if_assigned:
+            owner = self.get_owner(issue)
+            include_owners = [self.config.only_if_assigned]
+
+            if self.config.also_unassigned:
+                include_owners.append(None)
+
+            return owner in include_owners
+
+        return True
+
     def issues(self):
         base_url = "https://" + self.config.base_uri
         if self.trac:
diff --git a/bugwarrior/services/trello.py b/bugwarrior/services/trello.py
index 03760572..2786480e 100644
--- a/bugwarrior/services/trello.py
+++ b/bugwarrior/services/trello.py
@@ -85,10 +85,6 @@ class TrelloService(Service, Client):
     ISSUE_CLASS = TrelloIssue
     CONFIG_SCHEMA = TrelloConfig
 
-    def get_owner(self, issue):
-        # Issue filtering is implemented as part of the api query.
-        pass
-
     @staticmethod
     def get_keyring_service(config):
         return f"trello://{config.api_key}@trello.com"
diff --git a/bugwarrior/services/youtrack.py b/bugwarrior/services/youtrack.py
index 5a401d86..8f9d5145 100644
--- a/bugwarrior/services/youtrack.py
+++ b/bugwarrior/services/youtrack.py
@@ -147,11 +147,6 @@ def __init__(self, *args, **kw):
     def get_keyring_service(config):
         return f"youtrack://{config.login}@{config.host}"
 
-    def get_owner(self, issue):
-        # TODO
-        raise NotImplementedError(
-            "This service has not implemented support for 'only_if_assigned'.")
-
     def issues(self):
         params = {
             'query': self.config.query,
diff --git a/tests/test_service.py b/tests/test_service.py
index 13109d7d..02ae4820 100644
--- a/tests/test_service.py
+++ b/tests/test_service.py
@@ -1,3 +1,4 @@
+import re
 import unittest.mock
 
 import typing_extensions
@@ -68,9 +69,31 @@ def makeIssue(self):
         service = self.makeService()
         return service.get_issue_for_record({})
 
+    def checkArchitecture(self, klass):
+        """
+        Bidirectional communication between the base classes and their children
+        has been a source of complication as changes to any part of the
+        circular data flow can create unpredictable side-effects. The concrete
+        methods of the base classes exist as utilities for children to call;
+        they should not call the abstract methods which children implement.
+
+        Here, we cheaply check that the names of the abstract methods only
+        appear once. This should ensure that these methods are declared here
+        but not called.
+        """
+        with open(services.base.__file__, 'r') as f:
+            base = f.read()
+
+        for method in klass.__abstractmethods__:
+            references = re.findall(rf'{method}\(', base)
+            self.assertEqual(len(references), 1, references)
+
 
 class TestService(ServiceBase):
 
+    def test_architecture(self):
+        self.checkArchitecture(services.base.Service)
+
     def test_build_annotations_default(self):
         service = self.makeService()
 
@@ -101,6 +124,9 @@ def test_build_annotations_limitless(self):
 
 class TestIssue(ServiceBase):
 
+    def test_architecture(self):
+        self.checkArchitecture(services.base.Issue)
+
     def test_build_default_description_default(self):
         issue = self.makeIssue()