Refactored CodeCommentNotifyEmail to use subscriptions

Added a `for_comment` query helper.

Changed the notification scheme so that resource authors **always**
receive a "to" notification.

Changed the `notify` column to be a boolean.

Changed the `for_*` signatures to accept an optional `notify` flag.

Changed the `select` signature to accept an optional `notify` flag, and
the method to include the flag in the query.

Changed the `select` method to handle `dict`, `list, and `bool` query
arguments.

Author: schwuk

code_comments/db.py

@@ -31,7 +31,7 @@
         Column('path'),
         Column('repos'),
         Column('rev'),
-        Column('notify'),
+        Column('notify', type='bool'),
         Index(['user']),
         Index(['path']),
     ],

code_comments/notification.py

@@ -1,11 +1,10 @@
-from trac.attachment import Attachment
 from trac.config import BoolOption
 from trac.core import Component, implements
 from trac.notification import NotifyEmail
-from trac.resource import ResourceNotFound
-from trac.versioncontrol import RepositoryManager, NoSuchChangeset
+
 from code_comments.api import ICodeCommentChangeListener
 from code_comments.comments import Comments
+from code_comments.subscription import Subscription
 
 
 class CodeCommentChangeListener(Component):
@@ -33,41 +32,6 @@ class CodeCommentNotifyEmail(NotifyEmail):
     template_name = "code_comment_notify_email.txt"
     from_email = "trac+comments@localhost"
 
-    def _get_attachment_author(self, parent, parent_id, filename):
-        """
-        Returns the author of a given attachment.
-        """
-        try:
-            attachment = Attachment(self.env, parent, parent_id, filename)
-            return attachment.author
-        except ResourceNotFound:
-            self.env.log.debug("Invalid attachment, unable to determine "
-                               "author.")
-
-    def _get_changeset_author(self, revision, reponame=None):
-        """
-        Returns the author of a changeset for a given revision.
-        """
-        try:
-            repos = RepositoryManager(self.env).get_repository(reponame)
-            changeset = repos.get_changeset(revision)
-            return changeset.author
-        except NoSuchChangeset:
-            self.env.log.debug("Invalid changeset, unable to determine author")
-
-    def _get_original_author(self, comment):
-        """
-        Returns the author for the target of a given comment.
-        """
-        if comment.type == 'attachment':
-            parent, parent_id, filename = comment.path.split("/")[1:]
-            return self._get_attachment_author(parent, parent_id,
-                                               filename)
-        elif (comment.type == 'changeset' or comment.type == "browser"):
-            # TODO: When support is added for multiple repositories, this
-            # will need updated
-            return self._get_changeset_author(comment.revision)
-
     def _get_comment_thread(self, comment):
         """
         Returns all comments in the same location as a given comment, sorted
@@ -80,16 +44,6 @@ def _get_comment_thread(self, comment):
                 'line': comment.line}
         return comments.search(args, order_by='id')
 
-    def _get_commenters(self, comment):
-        """
-        Returns a list of all commenters for the same thing.
-        """
-        comments = Comments(None, self.env)
-        args = {'type': comment.type,
-                'revision': comment.revision,
-                'path': comment.path}
-        return comments.get_all_comment_authors(comments.search(args))
-
     def get_recipients(self, comment):
         """
         Determine who should receive the notification.
@@ -99,31 +53,27 @@ def get_recipients(self, comment):
         Current scheme is as follows:
 
          * For the first comment in a given location, the notification is sent
-         'to' the original author of the thing being commented on, and 'copied'
-         to the authors of any other comments on that thing
+         'to' the author of the resource being commented on, and 'copied'
+         to any other subscribers to that resource
          * For any further comments in a given location, the notification is
          sent 'to' the author of the last comment in that location, and
-         'copied' to both the original author of the thing and the authors of
-         any other comments on that thing
+         the author of the resource and 'copied' to any other subscribers
         """
         torcpts = set()
+        ccrcpts = set()
 
-        # Get the original author
-        original_author = self._get_original_author(comment)
-
-        # Get other commenters
-        ccrcpts = set(self._get_commenters(comment))
+        for subscription in Subscription.for_comment(self.env, comment,
+                                                     notify=True):
+            if subscription.role == 'author':
+                torcpts.add(subscription.user)
+            else:
+                ccrcpts.add(subscription.user)
 
         # Is this a reply, or a new comment?
         thread = self._get_comment_thread(comment)
         if len(thread) > 1:
             # The author of the comment before this one
             torcpts.add(thread[-2].author)
-            # Copy to the original author
-            ccrcpts.add(original_author)
-        else:
-            # This is the first comment in this thread
-            torcpts.add(original_author)
 
         # Should we notify the comment author?
         if not self.notify_self:

code_comments/subscription.py

@@ -19,23 +19,30 @@ class Subscription(object):
     path = ''
     rev = ''
     repos = ''
-    notify = 'always'
+    notify = True
 
     def __init__(self, env, data=None):
         if isinstance(data, dict):
             self.__dict__ = data
         self.env = env
 
     @classmethod
-    def select(cls, env, args={}):
+    def select(cls, env, args={}, notify=None):
         select = 'SELECT * FROM code_comments_subscriptions'
+        if notify:
+            args['notify'] = bool(notify)
         if len(args) > 0:
             select += ' WHERE '
             criteria = []
             for key, value in args.iteritems():
                 template = '{0}={1}'
                 if isinstance(value, str):
                     template = '{0}=\'{1}\''
+                if (isinstance(value, tuple) or isinstance(value, list)):
+                    template = '{0} IN (\'{1}\')'
+                    value = '\',\''.join(value)
+                if isinstance(value, bool):
+                    value = int(value)
                 criteria.append(template.format(key, value))
             select += ' AND '.join(criteria)
         cursor = env.get_read_db().cursor()
@@ -112,7 +119,7 @@ def _from_row(cls, env, row):
             subscription.path = row[4]
             subscription.repos = row[5]
             subscription.rev = row[6]
-            subscription.notify = row[7]
+            subscription.notify = bool(row[7])
             return subscription
         except IndexError:
             # Invalid row
@@ -145,7 +152,7 @@ def _from_dict(cls, env, dict_):
 
     @classmethod
     def from_attachment(cls, env, attachment, user=None, role='author',
-                        notify='always'):
+                        notify=True):
         """
         Creates a subscription from an Attachment object.
         """
@@ -166,7 +173,7 @@ def from_attachment(cls, env, attachment, user=None, role='author',
 
     @classmethod
     def from_changeset(cls, env, changeset, user=None, role='author',
-                       notify='always'):
+                       notify=True):
         """
         Creates a subscription from a Changeset object.
         """
@@ -183,13 +190,13 @@ def from_changeset(cls, env, changeset, user=None, role='author',
 
     @classmethod
     def from_comment(cls, env, comment, user=None, role='commenter',
-                     notify='always'):
+                     notify=True):
         """
         Creates a subscription from a Comment object.
         """
         sub = {
             'user': user or comment.author,
-            'role': user,
+            'role': 'commenter',
             'type': comment.type,
             'notify': notify,
         }
@@ -221,7 +228,7 @@ def from_comment(cls, env, comment, user=None, role='commenter',
         return cls._from_dict(env, sub)
 
     @classmethod
-    def for_attachment(cls, env, attachment, path=None):
+    def for_attachment(cls, env, attachment, path=None, notify=None):
         """
         Returns all subscriptions for an attachment. The path can be
         overridden.
@@ -234,10 +241,10 @@ def for_attachment(cls, env, attachment, path=None):
             'type': 'attachment',
             'path': _path,
         }
-        return cls.select(env, args)
+        return cls.select(env, args, notify)
 
     @classmethod
-    def for_changeset(cls, env, changeset):
+    def for_changeset(cls, env, changeset, notify=None):
         """
         Returns all subscriptions for an changeset.
         """
@@ -246,7 +253,28 @@ def for_changeset(cls, env, changeset):
             'repos': changeset.repos.reponame,
             'rev': changeset.rev,
         }
-        return cls.select(env, args)
+        return cls.select(env, args, notify)
+
+    @classmethod
+    def for_comment(cls, env, comment, notify=None):
+        """
+        Return all subscriptions for a comment.
+        """
+        args = {}
+        if comment.type == 'attachment':
+            args['type'] = comment.type
+            args['path'] = comment.path.split(':')[1]
+
+        if comment.type == 'changeset':
+            args['type'] = comment.type
+            args['rev'] = str(comment.revision)
+
+        if comment.type == 'browser':
+            args['type'] = ('browser', 'changeset')
+            args['path'] = (comment.path, '')
+            args['rev'] = str(comment.revision)
+
+        return cls.select(env, args, notify)
 
 
 class SubscriptionAdmin(Component):