Refactor commenting logic

todosrht/blueprints/ticket.py

@@ -9,6 +9,7 @@ from todosrht.types import Event, EventType
 from todosrht.types import Label, TicketLabel
 from todosrht.types import TicketAccess, TicketResolution, TicketSeen
 from todosrht.types import TicketSubscription
+from todosrht.urls import ticket_url
 
 ticket = Blueprint("ticket", __name__)
 
@@ -146,14 +147,7 @@ def ticket_comment_POST(owner, name, ticket_id):
     comment = add_comment(current_user, ticket,
         text=text, resolve=resolve, resolution=resolution, reopen=reopen)
 
-    ticket_url = url_for(".ticket_GET",
-            owner=tracker.owner.canonical_name(),
-            name=tracker.name,
-            ticket_id=ticket.scoped_id)
-    if comment:
-         ticket_url += "#comment-" + str(comment.id)
-
-    return redirect(ticket_url)
+    return redirect(ticket_url(ticket, comment))
 
 @ticket.route("/<owner>/<name>/<int:ticket_id>/edit")
 @loginrequired

todosrht/tickets.py

@@ -1,118 +1,137 @@
+from collections import namedtuple
 from datetime import datetime
-from flask import url_for
 from srht.config import cfg
 from srht.database import db
 from todosrht.email import notify
 from todosrht.types import Event, EventType, EventNotification
 from todosrht.types import TicketComment, TicketStatus, TicketSubscription
+from todosrht.urls import ticket_url
 
 smtp_user = cfg("mail", "smtp-user", default=None)
 smtp_from = cfg("mail", "smtp-from", default=None)
 notify_from = cfg("todo.sr.ht", "notify-from", default=smtp_from)
 
+StatusChange = namedtuple("StatusChange", [
+    "old_status",
+    "new_status",
+    "old_resolution",
+    "new_resolution",
+])
 
-def add_comment(user, ticket,
-        text=None, resolve=False, resolution=None, reopen=False):
+def _create_comment(ticket, user, text):
+    comment = TicketComment()
+    comment.text = text
+    # TODO: anonymous comments (when configured appropriately)
+    comment.submitter_id = user.id
+    comment.ticket_id = ticket.id
 
-    assert text or resolve or reopen
+    db.session.add(comment)
+    db.session.flush()
+    return comment
 
-    tracker = ticket.tracker
+def _create_comment_event(ticket, user, comment, status_change):
+    event = Event()
+    event.event_type = 0
+    event.user_id = user.id
+    event.ticket_id = ticket.id
 
-    if text:
-        comment = TicketComment()
-        comment.text = text
-        # TODO: anonymous comments (when configured appropriately)
-        comment.submitter_id = user.id
-        comment.ticket_id = ticket.id
-        db.session.add(comment)
-        ticket.updated = comment.created
-    else:
-        comment = None
+    if comment:
+        event.event_type |= EventType.comment
+        event.comment_id = comment.id
+
+    if status_change:
+        event.event_type |= EventType.status_change
+        event.old_status = status_change.old_status
+        event.old_resolution = status_change.old_resolution
+        event.new_status = status_change.new_status
+        event.new_resolution = status_change.new_resolution
+
+    db.session.add(event)
+    db.session.flush()
+    return event
+
+def _create_event_notification(user, event):
+    notification = EventNotification()
+    notification.user_id = user.id
+    notification.event_id = event.id
+    db.session.add(notification)
+    return notification
+
+def _send_comment_notification(subscription, ticket, user, comment, resolution):
+    subject = "Re: {}/{}/#{}: {}".format(
+        ticket.tracker.owner.canonical_name(),
+        ticket.tracker.name,
+        ticket.scoped_id,
+        ticket.title)
+
+    headers = {
+        "From": "~{} <{}>".format(user.username, notify_from),
+        "Sender": smtp_user,
+    }
+
+    url = ticket_url(ticket, comment=comment).replace("%7E", "~")  # hack
+
+    notify(subscription, "ticket_comment", subject,
+        headers=headers,
+        ticket=ticket,
+        comment=comment,
+        resolution=resolution.name if resolution else None,
+        ticket_url=url)
+
+def _change_ticket_status(ticket, resolve, resolution, reopen):
+    if not (resolve or reopen):
+        return None
 
     old_status = ticket.status
     old_resolution = ticket.resolution
 
-    if resolution:
+    if resolve:
         ticket.status = TicketStatus.resolved
         ticket.resolution = resolution
 
     if reopen:
         ticket.status = TicketStatus.reported
 
-    tracker.updated = datetime.utcnow()
-    db.session.flush()
+    return StatusChange(
+        old_status, ticket.status, old_resolution, ticket.resolution)
 
-    subscribed = False
+def _send_comment_notifications(user, ticket, event, comment, resolution):
+    # Find subscribers, eliminate duplicates
+    subscriptions = {sub.user: sub
+        for sub in ticket.tracker.subscriptions + ticket.subscriptions}
 
-    ticket_url = url_for("ticket.ticket_GET",
-            owner=tracker.owner.canonical_name(),
-            name=tracker.name,
-            ticket_id=ticket.scoped_id)
-    if comment:
-        ticket_url += "#comment-" + str(comment.id)
+    # Subscribe commenter if not already subscribed
+    if user not in subscriptions:
+        subscription = TicketSubscription()
+        subscription.ticket_id = ticket.id
+        subscription.user_id = user.id
+        db.session.add(subscription)
+        subscriptions[user] = subscription
 
-    def _notify(sub):
-        notify(sub, "ticket_comment", "Re: {}/{}/#{}: {}".format(
-            tracker.owner.canonical_name(), tracker.name,
-            ticket.scoped_id, ticket.title),
-                headers={
-                    "From": "~{} <{}>".format(
-                        user.username, notify_from),
-                    "Sender": smtp_user,
-                },
-                ticket=ticket,
-                comment=comment,
-                resolution=resolution.name if resolution else None,
-                ticket_url=ticket_url.replace("%7E", "~")) # hack
+    for subscriber, subscription in subscriptions.items():
+        _create_event_notification(subscriber, event)
+        if subscriber != user:
+            _send_comment_notification(
+                subscription, ticket, user, comment, resolution)
 
-    event = Event()
-    event.event_type = 0
-    event.user_id = user.id
-    event.ticket_id = ticket.id
-    if comment:
-        event.event_type |= EventType.comment
-        event.comment_id = comment.id
-    if ticket.status != old_status or ticket.resolution != old_resolution:
-        event.event_type |= EventType.status_change
-        event.old_status = old_status
-        event.old_resolution = old_resolution
-        event.new_status = ticket.status
-        event.new_resolution = ticket.resolution
-    db.session.add(event)
-    db.session.flush()
+def add_comment(user, ticket,
+        text=None, resolve=False, resolution=None, reopen=False):
+    """
+    Comment on a ticket, optionally resolve or reopen the ticket.
+    """
+    # TODO better error handling
+    assert text or resolve or reopen
+    assert not (resolve and reopen)
+    if resolve:
+        assert resolution is not None
 
-    def _add_notification(sub):
-        notification = EventNotification()
-        notification.user_id = sub.user_id
-        notification.event_id = event.id
-        db.session.add(notification)
-
-    subscribed = False
-    updated_users = set()
-    for sub in tracker.subscriptions:
-        updated_users.update([sub.user_id])
-        _add_notification(sub)
-        if sub.user_id == user.id:
-            subscribed = True
-            continue
-        _notify(sub)
-
-    for sub in ticket.subscriptions:
-        if sub.user_id in updated_users:
-            continue
-        _add_notification(sub)
-        if sub.user_id == user.id:
-            subscribed = True
-            continue
-        _notify(sub)
-
-    if not subscribed:
-        sub = TicketSubscription()
-        sub.ticket_id = ticket.id
-        sub.user_id = user.id
-        db.session.add(sub)
-        _add_notification(sub)
+    comment = _create_comment(ticket, user, text) if text else None
+    status_change = _change_ticket_status(ticket, resolve, resolution, reopen)
+    event = _create_comment_event(ticket, user, comment, status_change)
+    _send_comment_notifications(user, ticket, event, comment, resolution)
 
+    ticket.updated = datetime.utcnow()
+    ticket.tracker.updated = datetime.utcnow()
     db.session.commit()
 
     return comment

todosrht/types/event.py

@@ -31,7 +31,7 @@ class Event(Base):
     user = sa.orm.relationship("User", backref=sa.orm.backref("events"))
 
     ticket_id = sa.Column(sa.Integer, sa.ForeignKey("ticket.id"), nullable=False)
-    ticket = sa.orm.relationship("Ticket")
+    ticket = sa.orm.relationship("Ticket", backref=sa.orm.backref("events"))
 
     comment_id = sa.Column(sa.Integer, sa.ForeignKey("ticket_comment.id"))
     comment = sa.orm.relationship("TicketComment")

todosrht/urls.py

@@ -6,6 +6,17 @@ def tracker_url(tracker):
         owner=tracker.owner.canonical_name(),
         name=tracker.name)
 
+def ticket_url(ticket, comment=None):
+    ticket_url = url_for("ticket.ticket_GET",
+            owner=ticket.tracker.owner.canonical_name(),
+            name=ticket.tracker.name,
+            ticket_id=ticket.scoped_id)
+
+    if comment:
+        ticket_url += "#comment-" + str(comment.id)
+
+    return ticket_url
+
 def label_search_url(label):
     """Return the URL to the tracker page listing all tickets which have the
     label applied."""