From c7ad961ef327909f39cbc275143f0338db25fb74 Mon Sep 17 00:00:00 2001 From: multun Date: Mon, 11 Mar 2019 08:04:19 +0100 Subject: [PATCH] Implement mail tracker system * Implement mail tracking Signed-off-by: Victor "multun" Collod * Implement task merging * Add a mail tracker title format pattern * Autocomplete task names * Fix comment display * Track notification answers * Add a socket timeout for the mail worker A mail worker is a long running application. And sometimes, the IMAP server just hangs for hours for no apparent reason. imaplib doesn't enable setting a timeout, and setting it globally seems fine. * Only validate the merge form when submitted * Redirect to the new form when merging * Prettier task edit UI * Make task merging optional * Test mail tracking * Update documentation for mail tracking * Update dependencies * Add the TODO_COMMENT_CLASSES setting * Fix dependencies install order * Remove debug leftovers, improve documentation * Fail on missing from_address --- .travis.yml | 4 +- Pipfile | 2 + Pipfile.lock | 17 ++- README.md | 99 +++++++++++++- test_settings.py | 29 ++++ todo/__init__.py | 2 + todo/check.py | 19 +++ todo/features.py | 11 ++ todo/mail/__init__.py | 0 todo/mail/consumers/__init__.py | 9 ++ todo/mail/consumers/tracker.py | 150 +++++++++++++++++++++ todo/mail/delivery.py | 25 ++++ todo/mail/producers/__init__.py | 9 ++ todo/mail/producers/imap.py | 100 ++++++++++++++ todo/management/commands/mail_worker.py | 44 ++++++ todo/migrations/0008_mail_tracker.py | 46 +++++++ todo/models.py | 82 ++++++++++- todo/static/todo/css/styles.css | 2 +- todo/templates/todo/include/task_edit.html | 19 +-- todo/templates/todo/list_detail.html | 4 +- todo/templates/todo/task_detail.html | 149 ++++++++++++-------- todo/tests/conftest.py | 6 + todo/tests/test_tracker.py | 67 +++++++++ todo/tests/test_utils.py | 6 - todo/urls.py | 19 ++- todo/utils.py | 145 ++++++++++++++++---- todo/views/task_autocomplete.py | 29 ++++ todo/views/task_detail.py | 111 +++++++++++---- 28 files changed, 1069 insertions(+), 136 deletions(-) create mode 100644 todo/check.py create mode 100644 todo/features.py create mode 100644 todo/mail/__init__.py create mode 100644 todo/mail/consumers/__init__.py create mode 100644 todo/mail/consumers/tracker.py create mode 100644 todo/mail/delivery.py create mode 100644 todo/mail/producers/__init__.py create mode 100644 todo/mail/producers/imap.py create mode 100644 todo/management/commands/mail_worker.py create mode 100644 todo/migrations/0008_mail_tracker.py create mode 100644 todo/tests/test_tracker.py create mode 100644 todo/views/task_autocomplete.py diff --git a/.travis.yml b/.travis.yml index f5a25f3..0f581d6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,9 +9,9 @@ addons: postgresql: "9.6" install: - - "pip3 install -e . --upgrade" - - "pip3 install git+https://github.com/pypa/pipenv.git" + - "pip3 install pipenv" - "pipenv install --dev" + - "pip3 install -e . --upgrade" language: python python: diff --git a/Pipfile b/Pipfile index b3cad6f..58131f2 100644 --- a/Pipfile +++ b/Pipfile @@ -11,6 +11,8 @@ django-extensions = "*" factory-boy = "*" titlecase = "*" bleach = "*" +django-autocomplete-light = "*" +html2text = "*" [dev-packages] mypy = "*" diff --git a/Pipfile.lock b/Pipfile.lock index 874fa9e..63388b6 100644 --- a/Pipfile.lock +++ b/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "12d4ee05e41a375e62fe7f3b8256abc7ffb047263e5d91291dced74be8b64192" + "sha256": "c6fb601fc8a197ca280960d831a5386313c93ebe19d932afa01034d5520f2f94" }, "pipfile-spec": 6, "requires": { @@ -32,6 +32,13 @@ "index": "pypi", "version": "==2.1.7" }, + "django-autocomplete-light": { + "hashes": [ + "sha256:996cc62519a6e2e9cd1c26e57ddc5f14541209a93e62e83d7b3df3ba65c1f458" + ], + "index": "pypi", + "version": "==3.3.2" + }, "django-extensions": { "hashes": [ "sha256:109004f80b6f45ad1f56addaa59debca91d94aa0dc1cb19678b9364b4fe9b6f4", @@ -70,6 +77,14 @@ "index": "pypi", "version": "==3.7.7" }, + "html2text": { + "hashes": [ + "sha256:490db40fe5b2cd79c461cf56be4d39eb8ca68191ae41ba3ba79f6cb05b7dd662", + "sha256:627514fb30e7566b37be6900df26c2c78a030cc9e6211bda604d8181233bcdd4" + ], + "index": "pypi", + "version": "==2018.1.9" + }, "mccabe": { "hashes": [ "sha256:ab8a6258860da4b6677da4bd2fe5dc2c659cff31b3ee4f7f5d64e79735b80d42", diff --git a/README.md b/README.md index fabdbb5..4b74a5d 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,7 @@ assignment application for Django, designed to be dropped into an existing site * jQuery (full version, not "slim", for drag/drop prioritization) * Bootstrap (to work with provided templates, though you can override them) * bleach (`pip install bleach`) +* django-autocomplete-light (optional, required for task merging) ## Overview @@ -53,7 +54,7 @@ If using your own site, be sure you have jQuery and Bootstrap wired up and worki django-todo pages that require it will insert additional CSS/JavaScript into page heads, so your project's base templates must include: -``` +```jinja {% block extrahead %}{% endblock extrahead %} {% block extra_js %}{% endblock extra_js %} ``` @@ -107,7 +108,7 @@ If you wish to use the public ticket-filing system, first create the list into w Optional configuration options: -``` +```python # Restrict access to ALL todo lists/views to `is_staff` users. # If False or unset, all users can see all views (but more granular permissions are still enforced # within views, such as requiring staff for adding and deleting lists). @@ -126,6 +127,9 @@ TODO_DEFAULT_LIST_SLUG = 'tickets' # Defaults to "/" TODO_PUBLIC_SUBMIT_REDIRECT = 'dashboard' +# additionnal classes the comment body should hold +# adding "text-monospace" makes comment monospace +TODO_COMMENT_CLASSES = [] ``` The current django-todo version number is available from the [todo package](https://github.com/shacker/django-todo/blob/master/todo/__init__.py): @@ -133,6 +137,95 @@ The current django-todo version number is available from the [todo package](http python -c "import todo; print(todo.__version__)" +## Mail tracking + +What if you could turn django-todo into a shared mailbox? +Django-todo includes an optional feature that allows emails sent to a +dedicated mailbox to be pushed into todo as new tasks, and responses to +be added as comments on that original tasks. + +This allows support teams to work with a fully unified email + bug +tracking system to avoid confusion over who's seen or responded to what. + +To enable the feature, you need to: + + - define an email backend for outgoing emails + - define an email backend for incoming emails + - start a worker, which will wait for new emails + +```python +from todo.mail.producers import imap_producer +from todo.mail.consumers import tracker_consumer +from todo.mail.delivery import smtp_backend, console_backend + +# email notifications configuration +# each task list can get its own delivery method +TODO_MAIL_BACKENDS = { + # mail-queue is the name of the task list, not the worker name + "mail-queue": smtp_backend( + host="smtp.example.com", + port=465, + use_ssl=True, + username="test@example.com", + password="foobar", + # used as the From field when sending notifications. + # a username might be prepended later on + from_address="test@example.com", + # additionnal headers + headers={} + ), +} + +# incoming mail worker configuration +TODO_MAIL_TRACKERS = { + # configuration for worker "test_tracker" + "test_tracker": { + "producer": imap_producer( + host="imap.example.com", + username="text@example.com", + password="foobar", + # process_all=False, # by default, only unseen emails are processed + # preserve=False, # delete emails if False + # nap_duration=1, # duration of the pause between polling rounds + # input_folder="INBOX", # where to read emails from + ), + "consumer": tracker_consumer( + group="Mail Queuers", + task_list_slug="mail-queue", + priority=1, + task_title_format="[TEST_MAIL] {subject}", + ) + } +} +``` + +A mail worker can be started this way: + +```sh +./manage.py mail_worker test_tracker +``` + +If you want to log mail events, make sure to properly configure django logging: + +```python +LOGGING = { + 'version': 1, + 'disable_existing_loggers': False, + 'handlers': { + 'console': { + 'class': 'logging.StreamHandler', + }, + }, + 'loggers': { + '': { + 'handlers': ['console'], + 'level': 'DEBUG', + 'propagate': True, + }, + }, +} +``` + ## Upgrade Notes django-todo 2.0 was rebuilt almost from the ground up, and included some radical changes, including model name changes. As a result, it is *not compatible* with data from django-todo 1.x. If you would like to upgrade an existing installation, try this: @@ -229,5 +322,3 @@ ALL groups, not just the groups they "belong" to) **0.9.1** - Removed context_processors.py - leftover turdlet **0.9** - First release - - diff --git a/test_settings.py b/test_settings.py index 157b21b..14068d3 100644 --- a/test_settings.py +++ b/test_settings.py @@ -29,6 +29,8 @@ "django.contrib.sites", "django.contrib.staticfiles", "todo", + "dal", + "dal_select2", ) ROOT_URLCONF = "base_urls" @@ -61,3 +63,30 @@ }, } ] + +LOGGING = { + 'version': 1, + 'disable_existing_loggers': False, + 'handlers': { + 'console': { + 'class': 'logging.StreamHandler', + }, + }, + 'loggers': { + '': { + 'handlers': ['console'], + 'level': 'DEBUG', + 'propagate': True, + }, + 'django': { + 'handlers': ['console'], + 'level': 'DEBUG', + 'propagate': True, + }, + 'django.request': { + 'handlers': ['console'], + 'level': 'DEBUG', + 'propagate': True, + }, + }, +} diff --git a/todo/__init__.py b/todo/__init__.py index 71f82f5..aeb0a7a 100644 --- a/todo/__init__.py +++ b/todo/__init__.py @@ -8,3 +8,5 @@ __url__ = 'https://github.com/shacker/django-todo' __license__ = 'BSD License' + +from . import check diff --git a/todo/check.py b/todo/check.py new file mode 100644 index 0000000..3b72496 --- /dev/null +++ b/todo/check.py @@ -0,0 +1,19 @@ +from django.core.checks import Error, register + +# the sole purpose of this warning is to prevent people who have +# django-autocomplete-light installed but not configured to start the app +@register() +def dal_check(app_configs, **kwargs): + from django.conf import settings + from todo.features import HAS_AUTOCOMPLETE + + if not HAS_AUTOCOMPLETE: + return + + errors = [] + missing_apps = {'dal', 'dal_select2'} - set(settings.INSTALLED_APPS) + for missing_app in missing_apps: + errors.append( + Error('{} needs to be in INSTALLED_APPS'.format(missing_app)) + ) + return errors diff --git a/todo/features.py b/todo/features.py new file mode 100644 index 0000000..b55a9ea --- /dev/null +++ b/todo/features.py @@ -0,0 +1,11 @@ +HAS_AUTOCOMPLETE = True +try: + import dal +except ImportError: + HAS_AUTOCOMPLETE = False + +HAS_TASK_MERGE = False +if HAS_AUTOCOMPLETE: + import dal.autocomplete + if getattr(dal.autocomplete, 'Select2QuerySetView', None) is not None: + HAS_TASK_MERGE = True diff --git a/todo/mail/__init__.py b/todo/mail/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/todo/mail/consumers/__init__.py b/todo/mail/consumers/__init__.py new file mode 100644 index 0000000..ea1f7d0 --- /dev/null +++ b/todo/mail/consumers/__init__.py @@ -0,0 +1,9 @@ +def tracker_consumer(**kwargs): + def tracker_factory(producer): + # the import needs to be delayed until call to enable + # using the wrapper in the django settings + from .tracker import tracker_consumer + + return tracker_consumer(producer, **kwargs) + + return tracker_factory diff --git a/todo/mail/consumers/tracker.py b/todo/mail/consumers/tracker.py new file mode 100644 index 0000000..7d5df53 --- /dev/null +++ b/todo/mail/consumers/tracker.py @@ -0,0 +1,150 @@ +import re +import logging + +from email.charset import Charset as EMailCharset +from django.db import transaction +from django.db.models import Count +from html2text import html2text +from todo.models import Comment, Task, TaskList + +logger = logging.getLogger(__name__) + + +def part_decode(message): + charset = ("ascii", "ignore") + email_charset = message.get_content_charset() + if email_charset: + charset = (EMailCharset(email_charset).input_charset,) + + body = message.get_payload(decode=True) + return body.decode(*charset) + + +def message_find_mime(message, mime_type): + for submessage in message.walk(): + if submessage.get_content_type() == mime_type: + return submessage + return None + + +def message_text(message): + text_part = message_find_mime(message, "text/plain") + if text_part is not None: + return part_decode(text_part) + + html_part = message_find_mime(message, "text/html") + if html_part is not None: + return html2text(part_decode(html_part)) + + # TODO: find something smart to do when no text if found + return "" + + +def format_task_title(format_string, message): + return format_string.format( + subject=message["subject"], + author=message["from"], + ) + + +DJANGO_TODO_THREAD = re.compile(r'') + + +def parse_references(task_list, references): + related_messages = [] + answer_thread = None + for related_message in references.split(): + logger.info("checking reference: %r", related_message) + match = re.match(DJANGO_TODO_THREAD, related_message) + if match is None: + related_messages.append(related_message) + continue + + thread_id = int(match.group(1)) + new_answer_thread = Task.objects.filter( + task_list=task_list, + pk=thread_id + ).first() + if new_answer_thread is not None: + answer_thread = new_answer_thread + + if answer_thread is None: + logger.info("no answer thread found in references") + else: + logger.info("found an answer thread: %d", answer_thread) + return related_messages, answer_thread + + +def insert_message(task_list, message, priority, task_title_format): + if "message-id" not in message: + logger.warning("missing message id, ignoring message") + return + + if "from" not in message: + logger.warning('missing "From" header, ignoring message') + return + + if "subject" not in message: + logger.warning('missing "Subject" header, ignoring message') + return + + logger.info( + "received message:\t" + f"[Subject: {message['subject']}]\t" + f"[Message-ID: {message['message-id']}]\t" + f"[References: {message['references']}]\t" + f"[To: {message['to']}]\t" + f"[From: {message['from']}]" + ) + + message_id = message["message-id"] + message_from = message["from"] + text = message_text(message) + + related_messages, answer_thread = \ + parse_references(task_list, message.get("references", "")) + + # find the most relevant task to add a comment on. + # among tasks in the selected task list, find the task having the + # most email comments the current message references + best_task = ( + Task.objects.filter( + task_list=task_list, comment__email_message_id__in=related_messages + ) + .annotate(num_comments=Count("comment")) + .order_by("-num_comments") + .only("id") + .first() + ) + + # if no related comment is found but a thread message-id + # (generated by django-todo) could be found, use it + if best_task is None and answer_thread is not None: + best_task = answer_thread + + with transaction.atomic(): + if best_task is None: + best_task = Task.objects.create( + priority=priority, + title=format_task_title(task_title_format, message), + task_list=task_list + ) + logger.info("using task: %r", best_task) + + comment, comment_created = Comment.objects.get_or_create( + task=best_task, + email_message_id=message_id, + defaults={"email_from": message_from, "body": text}, + ) + logger.info("created comment: %r", comment) + + +def tracker_consumer(producer, group=None, task_list_slug=None, + priority=1, task_title_format="[MAIL] {subject}"): + task_list = TaskList.objects.get(group__name=group, slug=task_list_slug) + for message in producer: + try: + insert_message(task_list, message, priority, task_title_format) + except Exception: + # ignore exceptions during insertion, in order to avoid + logger.exception("got exception while inserting message") diff --git a/todo/mail/delivery.py b/todo/mail/delivery.py new file mode 100644 index 0000000..33f916f --- /dev/null +++ b/todo/mail/delivery.py @@ -0,0 +1,25 @@ +import importlib + +def _declare_backend(backend_path): + backend_path = backend_path.split('.') + backend_module_name = '.'.join(backend_path[:-1]) + class_name = backend_path[-1] + + def backend(*args, headers={}, from_address=None, **kwargs): + def _backend(): + backend_module = importlib.import_module(backend_module_name) + backend = getattr(backend_module, class_name) + return backend(*args, **kwargs) + + if from_address is None: + raise ValueError("missing from_address") + + _backend.from_address = from_address + _backend.headers = headers + return _backend + return backend + + +smtp_backend = _declare_backend('django.core.mail.backends.smtp.EmailBackend') +console_backend = _declare_backend('django.core.mail.backends.console.EmailBackend') +locmem_backend = _declare_backend('django.core.mail.backends.locmem.EmailBackend') diff --git a/todo/mail/producers/__init__.py b/todo/mail/producers/__init__.py new file mode 100644 index 0000000..8ff313b --- /dev/null +++ b/todo/mail/producers/__init__.py @@ -0,0 +1,9 @@ +def imap_producer(**kwargs): + def imap_producer_factory(): + # the import needs to be delayed until call to enable + # using the wrapper in the django settings + from .imap import imap_producer + + return imap_producer(**kwargs) + + return imap_producer_factory diff --git a/todo/mail/producers/imap.py b/todo/mail/producers/imap.py new file mode 100644 index 0000000..11740c5 --- /dev/null +++ b/todo/mail/producers/imap.py @@ -0,0 +1,100 @@ +import email +import email.parser +import imaplib +import logging +import time + +from email.policy import default +from contextlib import contextmanager + +logger = logging.getLogger(__name__) + + +def imap_check(command_tuple): + status, ids = command_tuple + assert status == "OK", ids + + +@contextmanager +def imap_connect(host, port, username, password): + conn = imaplib.IMAP4_SSL(host=host, port=port) + conn.login(username, password) + imap_check(conn.list()) + try: + yield conn + finally: + conn.close() + + +def parse_message(message): + for response_part in message: + if not isinstance(response_part, tuple): + continue + + message_metadata, message_content = response_part + email_parser = email.parser.BytesFeedParser(policy=default) + email_parser.feed(message_content) + return email_parser.close() + + +def search_message(conn, *filters): + status, message_ids = conn.search(None, *filters) + for message_id in message_ids[0].split(): + status, message = conn.fetch(message_id, "(RFC822)") + yield message_id, parse_message(message) + + +def imap_producer( + process_all=False, + preserve=False, + host=None, + port=993, + username=None, + password=None, + nap_duration=1, + input_folder="INBOX", +): + logger.debug("starting IMAP worker") + imap_filter = "(ALL)" if process_all else "(UNSEEN)" + + def process_batch(): + logger.debug("starting to process batch") + # reconnect each time to avoid repeated failures due to a lost connection + with imap_connect(host, port, username, password) as conn: + # select the requested folder + imap_check(conn.select(input_folder, readonly=False)) + + try: + for message_uid, message in search_message(conn, imap_filter): + logger.info(f"received message {message_uid}") + try: + yield message + except Exception: + logger.exception( + f"something went wrong while processing {message_uid}" + ) + raise + + if not preserve: + # tag the message for deletion + conn.uid("STORE", message_uid, "+FLAGS", "(\\Deleted)") + else: + logger.debug("did not receive any message") + finally: + if not preserve: + # flush deleted messages + conn.expunge() + + while True: + try: + yield from process_batch() + except (GeneratorExit, KeyboardInterrupt): + # the generator was closed, due to the consumer + # breaking out of the loop, or an exception occuring + raise + except Exception: + logger.exception("mail fetching went wrong, retrying") + + # sleep to avoid using too much resources + # TODO: get notified when a new message arrives + time.sleep(nap_duration) diff --git a/todo/management/commands/mail_worker.py b/todo/management/commands/mail_worker.py new file mode 100644 index 0000000..b0750fd --- /dev/null +++ b/todo/management/commands/mail_worker.py @@ -0,0 +1,44 @@ +import logging +import socket +import sys + +from django.core.management.base import BaseCommand +from django.conf import settings + +logger = logging.getLogger(__name__) + + +DEFAULT_IMAP_TIMEOUT = 20 + + +class Command(BaseCommand): + help = "Starts a mail worker" + + def add_arguments(self, parser): + parser.add_argument("--imap_timeout", type=int, default=30) + parser.add_argument("worker_name") + + def handle(self, *args, **options): + if not hasattr(settings, "TODO_MAIL_TRACKERS"): + logger.error("missing TODO_MAIL_TRACKERS setting") + sys.exit(1) + + worker_name = options["worker_name"] + tracker = settings.TODO_MAIL_TRACKERS.get(worker_name, None) + if tracker is None: + logger.error( + "couldn't find configuration for %r in TODO_MAIL_TRACKERS", + worker_name + ) + sys.exit(1) + + # set the default socket timeout (imaplib doesn't enable configuring it) + timeout = options["imap_timeout"] + if timeout: + socket.setdefaulttimeout(timeout) + + # run the mail polling loop + producer = tracker["producer"] + consumer = tracker["consumer"] + + consumer(producer()) diff --git a/todo/migrations/0008_mail_tracker.py b/todo/migrations/0008_mail_tracker.py new file mode 100644 index 0000000..ea7a484 --- /dev/null +++ b/todo/migrations/0008_mail_tracker.py @@ -0,0 +1,46 @@ +# Generated by Django 2.1.4 on 2018-12-21 14:01 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [("todo", "0007_auto_update_created_date")] + + operations = [ + migrations.AddField( + model_name="comment", + name="email_from", + field=models.CharField(blank=True, max_length=320, null=True), + ), + migrations.AddField( + model_name="comment", + name="email_message_id", + field=models.TextField(blank=True, null=True), + ), + migrations.AlterField( + model_name="comment", + name="author", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterField( + model_name="task", + name="created_by", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="todo_created_by", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterUniqueTogether( + name="comment", unique_together={("task", "email_message_id")} + ), + ] diff --git a/todo/models.py b/todo/models.py index 89a1837..b378b02 100644 --- a/todo/models.py +++ b/todo/models.py @@ -1,13 +1,50 @@ from __future__ import unicode_literals import datetime +import textwrap from django.conf import settings from django.contrib.auth.models import Group -from django.db import models +from django.db import models, DEFAULT_DB_ALIAS +from django.db.transaction import Atomic, get_connection from django.urls import reverse from django.utils import timezone +class LockedAtomicTransaction(Atomic): + """ + modified from https://stackoverflow.com/a/41831049 + this is needed for safely merging + + Does a atomic transaction, but also locks the entire table for any transactions, for the duration of this + transaction. Although this is the only way to avoid concurrency issues in certain situations, it should be used with + caution, since it has impacts on performance, for obvious reasons... + """ + + def __init__(self, *models, using=None, savepoint=None): + if using is None: + using = DEFAULT_DB_ALIAS + super().__init__(using, savepoint) + self.models = models + + def __enter__(self): + super(LockedAtomicTransaction, self).__enter__() + + # Make sure not to lock, when sqlite is used, or you'll run into problems while running tests!!! + if settings.DATABASES[self.using]["ENGINE"] != "django.db.backends.sqlite3": + cursor = None + try: + cursor = get_connection(self.using).cursor() + for model in self.models: + cursor.execute( + "LOCK TABLE {table_name}".format( + table_name=model._meta.db_table + ) + ) + finally: + if cursor and not cursor.closed: + cursor.close() + + class TaskList(models.Model): name = models.CharField(max_length=60) slug = models.SlugField(default="") @@ -32,7 +69,10 @@ class Task(models.Model): completed = models.BooleanField(default=False) completed_date = models.DateField(blank=True, null=True) created_by = models.ForeignKey( - settings.AUTH_USER_MODEL, related_name="todo_created_by", on_delete=models.CASCADE + settings.AUTH_USER_MODEL, + null=True, + related_name="todo_created_by", + on_delete=models.CASCADE, ) assigned_to = models.ForeignKey( settings.AUTH_USER_MODEL, @@ -63,6 +103,17 @@ def save(self, **kwargs): self.completed_date = datetime.datetime.now() super(Task, self).save() + def merge_into(self, merge_target): + if merge_target.pk == self.pk: + raise ValueError("can't merge a task with self") + + # lock the comments to avoid concurrent additions of comments after the + # update request. these comments would be irremediably lost because of + # the cascade clause + with LockedAtomicTransaction(Comment): + Comment.objects.filter(task=self).update(task=merge_target) + self.delete() + class Meta: ordering = ["priority"] @@ -73,14 +124,35 @@ class Comment(models.Model): a comment and change task details at the same time. Rolling our own since it's easy. """ - author = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) + author = models.ForeignKey( + settings.AUTH_USER_MODEL, on_delete=models.CASCADE, blank=True, null=True + ) task = models.ForeignKey(Task, on_delete=models.CASCADE) date = models.DateTimeField(default=datetime.datetime.now) + email_from = models.CharField(max_length=320, blank=True, null=True) + email_message_id = models.TextField(blank=True, null=True) + body = models.TextField(blank=True) + class Meta: + # an email should only appear once per task + unique_together = ("task", "email_message_id") + + @property + def author_text(self): + if self.author is not None: + return str(self.author) + + assert self.email_message_id is not None + return str(self.email_from) + + @property def snippet(self): + body_snippet = textwrap.shorten(self.body, width=35, placeholder="...") # Define here rather than in __str__ so we can use it in the admin list_display - return "{author} - {snippet}...".format(author=self.author, snippet=self.body[:35]) + return "{author} - {snippet}...".format( + author=self.author_text, snippet=body_snippet + ) def __str__(self): - return self.snippet() + return self.snippet diff --git a/todo/static/todo/css/styles.css b/todo/static/todo/css/styles.css index 7ce2568..44b68e3 100644 --- a/todo/static/todo/css/styles.css +++ b/todo/static/todo/css/styles.css @@ -1,4 +1,4 @@ label { display: block; font-weight: bold; -} \ No newline at end of file +} diff --git a/todo/templates/todo/include/task_edit.html b/todo/templates/todo/include/task_edit.html index cabee0c..936d465 100644 --- a/todo/templates/todo/include/task_edit.html +++ b/todo/templates/todo/include/task_edit.html @@ -2,9 +2,7 @@
{% csrf_token %} - - -
+
- - - - Email notifications will only be sent if task is assigned to someone other than yourself. - +
+ + + + Email notifications will only be sent if task is assigned to someone other than yourself. + +
Add Task {# Task edit / new task form #} - {% include 'todo/include/task_edit.html' %} +
+ {% include 'todo/include/task_edit.html' %} +
{% endif %} diff --git a/todo/templates/todo/task_detail.html b/todo/templates/todo/task_detail.html index eddb130..78ffa05 100644 --- a/todo/templates/todo/task_detail.html +++ b/todo/templates/todo/task_detail.html @@ -2,50 +2,63 @@ {% block title %}Task:{{ task.title }}{% endblock %} -{% block content %} -
-
-

{{ task.title }}

- {% if task.note %} -

{{ task.note|safe|urlize|linebreaks }}

- {% endif %} -
+{% block extrahead %} + +{{ form.media }} +{{ merge_form.media }} +{% endblock %} - - {% csrf_token %} -
- -
- -
- {% csrf_token %} -
- -
-
+{% block content %} +
+
+
+

{{ task.title }}

+ {% if task.note %} +
{{ task.note|safe|urlize|linebreaks }}
+ {% endif %}
+
-
    +
    +
      +
    • + + +
      + {% csrf_token %} +
      + +
      +
      +
      + {% csrf_token %} +
      + +
      +
      +
    • Assigned to: {% if task.assigned_to %} {{ task.assigned_to.get_full_name }} {% else %} Anyone {% endif %} @@ -77,35 +90,65 @@

      {{ task.title }}

-
+
{# Task edit / new task form #} {% include 'todo/include/task_edit.html' %} + {% if merge_form is not None %} +
+
+
Merge task
+
+
+

Merging is a destructive operation. This task will not exist anymore, and comments will be moved to the target task.

+ {% csrf_token %} + {% for field in merge_form.visible_fields %} +

+ {{ field.errors }} + {{ field }} +

+ {% endfor %} + +
+
+
+
+ {% endif %}
-
Add comment
-
- {% csrf_token %} -
- -
- -
+
+
Add comment
+
+ {% csrf_token %} +
+ +
+ +
+
{% if comment_list %}
Comments on this task
{% for comment in comment_list %} -

- {{ comment.author.first_name }} - {{ comment.author.last_name }}, +

+
+
+ {% if comment.email_message_id %} + email + {% endif %} + {{ comment.author_text }} +
+ {{ comment.date|date:"F d Y P" }} - -

- {{ comment.body|safe|urlize|linebreaks }} + +
+
+ {{ comment.body|safe|urlize|linebreaks }} +
+
{% endfor %} {% else %}
No comments (yet).
{% endif %}
- {% endblock %} diff --git a/todo/tests/conftest.py b/todo/tests/conftest.py index dc6c6bc..8e6138e 100644 --- a/todo/tests/conftest.py +++ b/todo/tests/conftest.py @@ -28,3 +28,9 @@ def todo_setup(django_user_model): Task.objects.create(created_by=u2, title="Task 1", task_list=tlist2, priority=1) Task.objects.create(created_by=u2, title="Task 2", task_list=tlist2, priority=2, completed=True) Task.objects.create(created_by=u2, title="Task 3", task_list=tlist2, priority=3) + + +@pytest.fixture() +# Set up an in-memory mail server to receive test emails +def email_backend_setup(settings): + settings.EMAIL_BACKEND = "django.core.mail.backends.locmem.EmailBackend" diff --git a/todo/tests/test_tracker.py b/todo/tests/test_tracker.py new file mode 100644 index 0000000..3387d75 --- /dev/null +++ b/todo/tests/test_tracker.py @@ -0,0 +1,67 @@ +import pytest + +from django.core import mail + +from todo.models import Task, Comment +from todo.mail.consumers import tracker_consumer +from email.message import EmailMessage + + +def consumer(*args, title_format="[TEST] {subject}", **kwargs): + return tracker_consumer( + group="Workgroup One", + task_list_slug="zip", + priority=1, + task_title_format=title_format, + )(*args, **kwargs) + + +def make_message(subject, content): + msg = EmailMessage() + msg.set_content(content) + msg['Subject'] = subject + return msg + + +def test_tracker_task_creation(todo_setup, django_user_model): + msg = make_message("test1 subject", "test1 content") + msg['From'] = 'test1@example.com' + msg['Message-ID'] = '' + + # test task creation + task_count = Task.objects.count() + consumer([msg]) + + assert task_count + 1 == Task.objects.count(), "task wasn't created" + task = Task.objects.filter(title="[TEST] test1 subject").first() + assert task is not None, "task was created with the wrong name" + + # test thread answers + msg = make_message("test2 subject", "test2 content") + msg['From'] = 'test1@example.com' + msg['Message-ID'] = '' + msg['References'] = ' ' + + task_count = Task.objects.count() + consumer([msg]) + assert task_count == Task.objects.count(), "comment created another task" + Comment.objects.get( + task=task, + body__contains="test2 content", + email_message_id='' + ) + + # test notification answer + msg = make_message("test3 subject", "test3 content") + msg['From'] = 'test1@example.com' + msg['Message-ID'] = '' + msg['References'] = ' '.format(task.pk) + + task_count = Task.objects.count() + consumer([msg]) + assert task_count == Task.objects.count(), "comment created another task" + Comment.objects.get( + task=task, + body__contains="test3 content", + email_message_id='' + ) diff --git a/todo/tests/test_utils.py b/todo/tests/test_utils.py index 0ac4c8a..4cc0008 100644 --- a/todo/tests/test_utils.py +++ b/todo/tests/test_utils.py @@ -6,12 +6,6 @@ from todo.utils import send_notify_mail, send_email_to_thread_participants -@pytest.fixture() -# Set up an in-memory mail server to receive test emails -def email_backend_setup(settings): - settings.EMAIL_BACKEND = "django.core.mail.backends.locmem.EmailBackend" - - def test_send_notify_mail_not_me(todo_setup, django_user_model, email_backend_setup): """Assign a task to someone else, mail should be sent. TODO: Future tests could check for email contents. diff --git a/todo/urls.py b/todo/urls.py index 2c97a8d..eccef35 100644 --- a/todo/urls.py +++ b/todo/urls.py @@ -1,11 +1,12 @@ from django.urls import path from todo import views - +from todo.features import HAS_TASK_MERGE app_name = 'todo' -urlpatterns = [ +from django.conf import settings +urlpatterns = [ path( '', views.list_lists, @@ -55,7 +56,19 @@ 'task//', views.task_detail, name='task_detail'), +] + +if HAS_TASK_MERGE: + # ensure autocomplete is optional + from todo.views.task_autocomplete import TaskAutocomplete + urlpatterns.append( + path( + 'task//autocomplete/', + TaskAutocomplete.as_view(), + name='task_autocomplete') + ) +urlpatterns.extend([ path( 'toggle_done//', views.toggle_done, @@ -70,4 +83,4 @@ 'search/', views.search, name="search"), -] +]) diff --git a/todo/utils.py b/todo/utils.py index dca0a11..c5b58a4 100644 --- a/todo/utils.py +++ b/todo/utils.py @@ -1,8 +1,12 @@ +import email.utils +import functools +import time from django.conf import settings from django.contrib.sites.models import Site +from django.core import mail +from django.core.exceptions import PermissionDenied from django.core.mail import send_mail from django.template.loader import render_to_string - from todo.models import Comment, Task @@ -19,45 +23,134 @@ def staff_check(user): return True -def send_notify_mail(new_task): - # Send email to assignee if task is assigned to someone other than submittor. - # Unassigned tasks should not try to notify. - - if not new_task.assigned_to == new_task.created_by: - current_site = Site.objects.get_current() - email_subject = render_to_string("todo/email/assigned_subject.txt", {"task": new_task}) - email_body = render_to_string( - "todo/email/assigned_body.txt", {"task": new_task, "site": current_site} - ) +def user_can_read_task(task, user): + return task.task_list.group in user.groups.all() or user.is_staff + + +def todo_get_backend(task): + '''returns a mail backend for some task''' + mail_backends = getattr(settings, "TODO_MAIL_BACKENDS", None) + if mail_backends is None: + return None + + task_backend = mail_backends[task.task_list.slug] + if task_backend is None: + return None + + return task_backend + + +def todo_get_mailer(user, task): + """a mailer is a (from_address, backend) pair""" + task_backend = todo_get_backend(task) + if task_backend is None: + return (None, mail.get_connection) + + from_address = getattr(task_backend, "from_address") + from_address = email.utils.formataddr((user.username, from_address)) + return (from_address, task_backend) + + +def todo_send_mail(user, task, subject, body, recip_list): + '''Send an email attached to task, triggered by user''' + references = Comment.objects.filter(task=task).only('email_message_id') + references = (ref.email_message_id for ref in references) + references = ' '.join(filter(bool, references)) + + from_address, backend = todo_get_mailer(user, task) + message_hash = hash(( + subject, + body, + from_address, + frozenset(recip_list), + references, + )) + + message_id = ( + # the task_id enables attaching back notification answers + "" + ).format( + task_id=task.pk, + # avoid the -hexstring case (hashes can be negative) + message_hash=abs(message_hash), + epoch=int(time.time()) + ) + + # the thread message id is used as a common denominator between all + # notifications for some task. This message doesn't actually exist, + # it's just there to make threading possible + thread_message_id = "".format(task.pk) + references = '{} {}'.format(references, thread_message_id) - send_mail( - email_subject, - email_body, - new_task.created_by.email, - [new_task.assigned_to.email], - fail_silently=False, + with backend() as connection: + message = mail.EmailMessage( + subject, + body, + from_address, + recip_list, + [], # Bcc + headers={ + **getattr(backend, 'headers', {}), + 'Message-ID': message_id, + 'References': references, + 'In-reply-to': thread_message_id, + }, + connection=connection, ) + message.send() -def send_email_to_thread_participants(task, msg_body, user, subject=None): - # Notify all previous commentors on a Task about a new comment. +def send_notify_mail(new_task): + ''' + Send email to assignee if task is assigned to someone other than submittor. + Unassigned tasks should not try to notify. + ''' + + if new_task.assigned_to == new_task.created_by: + return current_site = Site.objects.get_current() - email_subject = ( - subject if subject else render_to_string("todo/email/assigned_subject.txt", {"task": task}) + subject = render_to_string("todo/email/assigned_subject.txt", {"task": new_task}) + body = render_to_string( + "todo/email/assigned_body.txt", {"task": new_task, "site": current_site} ) + + recip_list = [new_task.assigned_to.email] + todo_send_mail(new_task.created_by, new_task, subject, body, recip_list) + + +def send_email_to_thread_participants(task, msg_body, user, subject=None): + '''Notify all previous commentors on a Task about a new comment.''' + + current_site = Site.objects.get_current() + email_subject = subject + if not subject: + subject = render_to_string( + "todo/email/assigned_subject.txt", + {"task": task} + ) + email_body = render_to_string( "todo/email/newcomment_body.txt", {"task": task, "body": msg_body, "site": current_site, "user": user}, ) - # Get list of all thread participants - everyone who has commented, plus task creator. + # Get all thread participants commenters = Comment.objects.filter(task=task) - recip_list = [ca.author.email for ca in commenters] - recip_list.append(task.created_by.email) - recip_list = list(set(recip_list)) # Eliminate duplicates + recip_list = set( + ca.author.email + for ca in commenters + if ca.author is not None + ) + for related_user in (task.created_by, task.assigned_to): + if related_user is not None: + recip_list.add(related_user.email) + recip_list = list(m for m in recip_list if m) - send_mail(email_subject, email_body, task.created_by.email, recip_list, fail_silently=False) + todo_send_mail(user, task, email_subject, email_body, recip_list) def toggle_task_completed(task_id: int) -> bool: diff --git a/todo/views/task_autocomplete.py b/todo/views/task_autocomplete.py new file mode 100644 index 0000000..0a5667e --- /dev/null +++ b/todo/views/task_autocomplete.py @@ -0,0 +1,29 @@ +from dal import autocomplete +from django.contrib.auth.decorators import login_required +from django.core.exceptions import PermissionDenied +from django.shortcuts import get_object_or_404 +from django.utils.decorators import method_decorator +from todo.models import Task +from todo.utils import user_can_read_task + + +class TaskAutocomplete(autocomplete.Select2QuerySetView): + @method_decorator(login_required) + def dispatch(self, request, task_id, *args, **kwargs): + self.task = get_object_or_404(Task, pk=task_id) + if not user_can_read_task(self.task, request.user): + raise PermissionDenied + + return super().dispatch(request, task_id, *args, **kwargs) + + def get_queryset(self): + # Don't forget to filter out results depending on the visitor ! + if not self.request.user.is_authenticated: + return Task.objects.none() + + qs = Task.objects.filter(task_list=self.task.task_list).exclude(pk=self.task.pk) + + if self.q: + qs = qs.filter(title__istartswith=self.q) + + return qs diff --git a/todo/views/task_detail.py b/todo/views/task_detail.py index 95432db..8ded8d8 100644 --- a/todo/views/task_detail.py +++ b/todo/views/task_detail.py @@ -1,15 +1,47 @@ +import bleach import datetime -import bleach +from django import forms +from django.conf import settings from django.contrib import messages from django.contrib.auth.decorators import login_required, user_passes_test from django.core.exceptions import PermissionDenied from django.http import HttpResponse -from django.shortcuts import get_object_or_404, redirect, render +from django.shortcuts import get_object_or_404, redirect, render, redirect +from django.urls import reverse +from django.utils.decorators import method_decorator from todo.forms import AddEditTaskForm from todo.models import Comment, Task -from todo.utils import send_email_to_thread_participants, toggle_task_completed, staff_check +from todo.utils import send_email_to_thread_participants, toggle_task_completed, staff_check, user_can_read_task +from todo.features import HAS_TASK_MERGE + + +if HAS_TASK_MERGE: + from dal import autocomplete + from todo.views.task_autocomplete import TaskAutocomplete + + +def handle_add_comment(request, task): + if not request.POST.get("add_comment"): + return + + Comment.objects.create( + author=request.user, + task=task, + body=bleach.clean(request.POST["comment-body"], strip=True), + ) + + send_email_to_thread_participants( + task, + request.POST["comment-body"], + request.user, + subject='New comment posted on task "{}"'.format(task.title), + ) + + messages.success( + request, "Comment posted. Notification email sent to thread participants." + ) @login_required @@ -19,33 +51,55 @@ def task_detail(request, task_id: int) -> HttpResponse: """ task = get_object_or_404(Task, pk=task_id) - comment_list = Comment.objects.filter(task=task_id) + comment_list = Comment.objects.filter(task=task_id).order_by('-date') # Ensure user has permission to view task. Admins can view all tasks. # Get the group this task belongs to, and check whether current user is a member of that group. - if task.task_list.group not in request.user.groups.all() and not request.user.is_staff: + if not user_can_read_task(task, request.user): raise PermissionDenied - # Save submitted comments - if request.POST.get("add_comment"): - Comment.objects.create( - author=request.user, - task=task, - body=bleach.clean(request.POST["comment-body"], strip=True), - ) + # Handle task merging + if not HAS_TASK_MERGE: + merge_form = None + else: + class MergeForm(forms.Form): + merge_target = forms.ModelChoiceField( + queryset=Task.objects.all(), + widget=autocomplete.ModelSelect2( + url=reverse("todo:task_autocomplete", kwargs={"task_id": task_id}) + ), + ) - send_email_to_thread_participants( - task, - request.POST["comment-body"], - request.user, - subject='New comment posted on task "{}"'.format(task.title), - ) - messages.success(request, "Comment posted. Notification email sent to thread participants.") + # Handle task merging + if not request.POST.get("merge_task_into"): + merge_form = MergeForm() + else: + merge_form = MergeForm(request.POST) + if merge_form.is_valid(): + merge_target = merge_form.cleaned_data["merge_target"] + if not user_can_read_task(merge_target, request.user): + raise PermissionDenied + + task.merge_into(merge_target) + return redirect(reverse( + "todo:task_detail", + kwargs={"task_id": merge_target.pk} + )) + + # Save submitted comments + handle_add_comment(request, task) # Save task edits - if request.POST.get("add_edit_task"): + if not request.POST.get("add_edit_task"): form = AddEditTaskForm( - request.user, request.POST, instance=task, initial={"task_list": task.task_list} + request.user, instance=task, initial={"task_list": task.task_list} + ) + else: + form = AddEditTaskForm( + request.user, + request.POST, + instance=task, + initial={"task_list": task.task_list}, ) if form.is_valid(): @@ -54,10 +108,10 @@ def task_detail(request, task_id: int) -> HttpResponse: item.save() messages.success(request, "The task has been edited.") return redirect( - "todo:list_detail", list_id=task.task_list.id, list_slug=task.task_list.slug + "todo:list_detail", + list_id=task.task_list.id, + list_slug=task.task_list.slug, ) - else: - form = AddEditTaskForm(request.user, instance=task, initial={"task_list": task.task_list}) # Mark complete if request.POST.get("toggle_done"): @@ -72,6 +126,13 @@ def task_detail(request, task_id: int) -> HttpResponse: else: thedate = datetime.datetime.now() - context = {"task": task, "comment_list": comment_list, "form": form, "thedate": thedate} + context = { + "task": task, + "comment_list": comment_list, + "form": form, + "merge_form": merge_form, + "thedate": thedate, + "comment_classes": getattr(settings, 'TODO_COMMENT_CLASSES', []), + } return render(request, "todo/task_detail.html", context)