Skip to content

Commit

Permalink
Send notifications when comments are created
Browse files Browse the repository at this point in the history
Added a new interface - ICodeCommentChangeListener - to use for
notifications.

Added a new component - CodeCommentSystem - to act as the extension
point for the above interface. (This could have been added to one of the
other components, but I have plans for other refactorings and wanted a
clean base for them.)

Extended Comments.create() to trigger comment_created events.

Added a new component - CodeCommentChangeListener - to respond to
comment_created events.

Sub-classed trac.notification.NotifyEmail to generate notification
emails. Notifications are sent to the author of the resouce being
commented on, and anyone else who has commented on the same resource.
There is additional logic to determine who the notification should be
*sent* and *copied* to.

Added an option to control whether notifications are sent to the person
making the comment (off by default).

Removed unused variable.
  • Loading branch information
schwuk committed Aug 4, 2014
1 parent 7e475ca commit ee51e04
Show file tree
Hide file tree
Showing 5 changed files with 207 additions and 2 deletions.
12 changes: 12 additions & 0 deletions code_comments/api.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from trac.core import Component, ExtensionPoint, Interface


class ICodeCommentChangeListener(Interface):
"""An interface for receiving comment change events."""

def comment_created(comment):
"""New comment created."""


class CodeCommentSystem(Component):

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

How did you choose the name CodeCommentSystem? I am having a hard time to figure out what its responsibilities are. In the rest of Trac, systems are big things like permissions system, or user system.

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

This I don't have a strong opinion on. I drew inspiration from both Trac (the Tickets system) and several plugins, all of which followed the *System naming convention. As referenced in Pull Request description, this was superfluous but I intend to add permissions (addressing trac-hacks#15) for comments alongside subscriptions. I'm quite happy to move the ExtensionPoint into either comments.Comments or web.CodeComments.

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

Cool, let’s leave it at that and see how we feel when we add more stuff to it.

change_listeners = ExtensionPoint(ICodeCommentChangeListener)
6 changes: 6 additions & 0 deletions code_comments/comments.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import os.path
from time import time
from code_comments.api import CodeCommentSystem
from code_comments.comment import Comment

class Comments:
Expand Down Expand Up @@ -117,4 +118,9 @@ def insert_comment(db):
self.env.log.debug(sql)
cursor.execute(sql, values)
comment_id[0] = db.get_last_id(cursor, 'code_comments')

for listener in CodeCommentSystem(self.env).change_listeners:

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

Should the logic for telling all the listeners live where the event is triggered?

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

I drew inspiration from Trac's tickets, and there the listen event is triggered in the model create/update/delete methods: http://trac.edgewall.org/browser/tags/trac-0.12.5/trac/ticket/model.py#L245. This seemed an appropriate equivalent since the Comment model does not have a create (or equivalent) method.

This comment has been minimized.

Copy link
@nb

nb Aug 7, 2014

If we look at the CodeCommentSystem as a event emitter/dispatcher, we shouldn't know too much about its internals (listeners, stuff like this), we should just tell it that something happened.

In this case, we should just tell CodeCommentSystem that a new comment was created and that the value was Comments(self.req, self.env).by_id(comment_id[0])). Then CodeCommentSystem should take care of informing all the listeners, or do whatever it needs to.

There are few reasons for this:

  • We might end up creating comments in various places. In this case we will need to replicate the same logic more than once, which is less than ideal.
  • The logic for informing listeners can become more complicated, in which case we will need to change the logic in all the places, we’re creating comments.
  • In the end of the day we are not complicating anything, just moving the code where it makes more sense.

This comment has been minimized.

Copy link
@schwuk

schwuk via email Aug 7, 2014

Author Owner
listener.comment_created(
Comments(self.req, self.env).by_id(comment_id[0]))

return comment_id[0]
169 changes: 169 additions & 0 deletions code_comments/notification.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
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


class CodeCommentChangeListener(Component):
"""
Sends email notifications when comments have been created.
"""
implements(ICodeCommentChangeListener)

# ICodeCommentChangeListener methods

def comment_created(self, comment):
notifier = CodeCommentNotifyEmail(self.env)
notifier.notify(comment)


class CodeCommentNotifyEmail(NotifyEmail):
"""
Sends code comment notifications by email.
"""

notify_self = BoolOption('code_comments', 'notify_self', False,
doc="Send comment notifications to the author of "
"the comment.")

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
in order of id.
"""
comments = Comments(None, self.env)
args = {'type': comment.type,
'revision': comment.revision,
'path': comment.path,
'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.
Required by NotifyEmail.
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
* 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
"""
torcpts = set()

# Get the original author
original_author = self._get_original_author(comment)

# Get other commenters
ccrcpts = set(self._get_commenters(comment))

# 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:
torcpts = torcpts.difference([comment.author])
ccrcpts = ccrcpts.difference([comment.author])

# Remove duplicates
ccrcpts = ccrcpts.difference(torcpts)

self.env.log.debug("Sending notification to: %s" % torcpts)

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

Why are we logging that we are sending a notification in the get_recipients() method?

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

The phrasing could be clearer; this is actually for validating the logic for getting recipients. Perhaps "Will send notification to:"?

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

Clarified as above in 420baf9.

This comment has been minimized.

Copy link
@nb

nb Aug 7, 2014

Do we need to log those at all? If somebody happens to call get_recipients() many times without actually sending anything, we will get a lot of noise. Don't we want to only log the recipients when actually sending the notification?

self.env.log.debug("Copying notification to: %s" % ccrcpts)

return (torcpts, ccrcpts)

def notify(self, comment):
from_name = comment.author

# See if we can get a real name for the comment author
for username, name, email in self.env.get_known_users():
if username == comment.author and name:
from_name = name

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

The logic for getting a real name should live outside of the notify() method, not very much related.

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

I will extract this.

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

Extracted in c8fe49a.

This comment has been minimized.

Copy link
@nb

nb Aug 7, 2014

Cool, thanks.


self.data.update({
"comment": comment,
})

projname = self.config.get("project", "name")
subject = "Re: [%s] %s" % (projname, comment.link_text())

# Temporarily switch the smtp_from_name setting so we can pretend

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

Changing the config option looks like a distraction in this method, I think we should move it to a different method, or ideally to something like:

with config_value(('notification', 'smtp_from_name'), from_name):
  NotifyEmail.notify(self, comment, subject)

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

Definitely cleaner - I will try that.

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

Found an even cleaner way of doing this by overriding NotifyEmail.send() in 929dae3.

This comment has been minimized.

Copy link
@nb

nb Aug 7, 2014

Good find!

# the mail came from the author of the comment
try:
self.env.log.debug("Changing smtp_from_name to %s" % from_name)
old_setting = self.config['notification'].get('smtp_from_name')
self.config.set('notification', 'smtp_from_name', from_name)
try:
NotifyEmail.notify(self, comment, subject)
except:
pass
finally:
self.env.log.debug("Changing smtp_from_name back to %s" %
old_setting)
self.config.set('notification', 'smtp_from_name', old_setting)
7 changes: 7 additions & 0 deletions code_comments/templates/code_comment_notify_email.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
${comment.text}

View the comment: ${project.url or abs_href()}${comment.href()}

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

Could we easily move out the url logic outside of the template?

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

Moved in ffea863.


--
${project.name} <${project.url or abs_href()}>
${project.descr}
15 changes: 13 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,21 @@
author_email='[email protected], [email protected]',
description='Tool for leaving inline code comments',
packages=find_packages(exclude=['*.tests*']),
entry_points = {
entry_points={
'trac.plugins': [
'code_comments = code_comments',
'code_comments.api = code_comments.api',

This comment has been minimized.

Copy link
@nb

nb Aug 4, 2014

Why do we need separate lines for api and notification modules?

This comment has been minimized.

Copy link
@schwuk

schwuk Aug 4, 2014

Author Owner

We don't. Fixed in ac97850.

'code_comments.notification = code_comments.notification',
],
},
package_data={
'code_comments': [
'templates/*.html',
'templates/js/*.html',
'htdocs/*.*',
'htdocs/jquery-ui/*.*',
'htdocs/jquery-ui/images/*.*',
'htdocs/sort/*.*',
],
},
package_data = {'code_comments': ['templates/*.html', 'templates/js/*.html', 'htdocs/*.*','htdocs/jquery-ui/*.*', 'htdocs/jquery-ui/images/*.*', 'htdocs/sort/*.*']},
)

0 comments on commit ee51e04

Please sign in to comment.