From 5e9fed2d46d9bf848b65adea1846e2d214210cf5 Mon Sep 17 00:00:00 2001 From: Timothy Hobbs Date: Sat, 9 Mar 2019 00:00:32 +0100 Subject: [PATCH 1/2] Add failing test for #732 --- helpdesk/tests/test_files/all-special-chars.eml | 15 +++++++++++++++ helpdesk/tests/test_get_email.py | 13 ++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 helpdesk/tests/test_files/all-special-chars.eml diff --git a/helpdesk/tests/test_files/all-special-chars.eml b/helpdesk/tests/test_files/all-special-chars.eml new file mode 100644 index 00000000..4d922482 --- /dev/null +++ b/helpdesk/tests/test_files/all-special-chars.eml @@ -0,0 +1,15 @@ +To: helpdesk@auto-mat.cz +From: Timothy Hobbs +Subject: =?UTF-8?B?VGVzdG92w6Fjw60gZW1haWw=?= +Openpgp: preference=signencrypt +Message-ID: <0fd7067e-2842-5b6c-3548-3cf7e6a1c9ea@auto-mat.cz> +Date: Fri, 8 Mar 2019 23:40:04 +0100 +User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 + Thunderbird/60.4.0 +MIME-Version: 1.0 +Content-Type: text/plain; charset=utf-8 +Content-Transfer-Encoding: 8bit +Content-Language: en-US + +íářčšáíéřášč + diff --git a/helpdesk/tests/test_get_email.py b/helpdesk/tests/test_get_email.py index 7a4fe448..d923323c 100644 --- a/helpdesk/tests/test_get_email.py +++ b/helpdesk/tests/test_get_email.py @@ -56,7 +56,7 @@ class GetEmailCommonTests(TestCase): self.assertEqual(ticket.title, "Attachment without body") self.assertEqual(ticket.description, "") - def test_email_with_blank_body_and_attachment(self): + def test_email_with_quoted_printable_body(self): """ Tests that emails with quoted-printable bodies work. """ @@ -73,6 +73,17 @@ class GetEmailCommonTests(TestCase): attachment = attachments[0] self.assertEqual(attachment.file.read().decode("utf-8"), '
Tohle je test českých písmen odeslaných z gmailu.
\n') + def test_email_with_8bit_encoding_and_utf_8(self): + """ + Tests that emails with 8bit transfer encoding and utf-8 charset + https://github.com/django-helpdesk/django-helpdesk/issues/732 + """ + with open(os.path.join(THIS_DIR, "test_files/all-special-chars.eml")) as fd: + test_email = fd.read() + ticket = helpdesk.email.object_from_message(test_email, self.queue_public, self.logger) + self.assertEqual(ticket.title, "Testovácí email") + self.assertEqual(ticket.description, "íářčšáíéřášč") + class GetEmailParametricTemplate(object): """TestCase that checks basic email functionality across methods and socks configs.""" From 912727555774151c39d1eb9039a0882fcdde9550 Mon Sep 17 00:00:00 2001 From: Jachym Cepicky Date: Thu, 7 Mar 2019 21:58:04 +0100 Subject: [PATCH 2/2] adding support for images as knowledgebase attachment --- helpdesk/admin.py | 16 ++- helpdesk/forms.py | 2 +- helpdesk/lib.py | 4 +- .../migrations/0026_kbitem_attachments.py | 36 +++++++ helpdesk/models.py | 98 +++++++++++++++---- .../helpdesk/public_view_ticket.html | 2 +- helpdesk/tests/test_attachments.py | 25 ++++- helpdesk/tests/test_get_email.py | 12 +-- helpdesk/views/staff.py | 6 +- 9 files changed, 161 insertions(+), 40 deletions(-) create mode 100644 helpdesk/migrations/0026_kbitem_attachments.py diff --git a/helpdesk/admin.py b/helpdesk/admin.py index 32bcfd9b..52318ec6 100644 --- a/helpdesk/admin.py +++ b/helpdesk/admin.py @@ -2,7 +2,7 @@ from django.contrib import admin from django.utils.translation import ugettext_lazy as _ from helpdesk.models import Queue, Ticket, FollowUp, PreSetReply, KBCategory from helpdesk.models import EscalationExclusion, EmailTemplate, KBItem -from helpdesk.models import TicketChange, Attachment, IgnoreEmail +from helpdesk.models import TicketChange, KBIAttachment, FollowUpAttachment, IgnoreEmail from helpdesk.models import CustomField @@ -43,15 +43,22 @@ class TicketAdmin(admin.ModelAdmin): class TicketChangeInline(admin.StackedInline): model = TicketChange + extra = 0 -class AttachmentInline(admin.StackedInline): - model = Attachment +class FollowUpAttachmentInline(admin.StackedInline): + model = FollowUpAttachment + extra = 0 + + +class KBIAttachmentInline(admin.StackedInline): + model = KBIAttachment + extra = 0 @admin.register(FollowUp) class FollowUpAdmin(admin.ModelAdmin): - inlines = [TicketChangeInline, AttachmentInline] + inlines = [TicketChangeInline, FollowUpAttachmentInline] list_display = ('ticket_get_ticket_for_url', 'title', 'date', 'ticket', 'user', 'new_status', 'time_spent') list_filter = ('user', 'date', 'new_status') @@ -64,6 +71,7 @@ class FollowUpAdmin(admin.ModelAdmin): @admin.register(KBItem) class KBItemAdmin(admin.ModelAdmin): list_display = ('category', 'title', 'last_updated',) + inlines = [KBIAttachmentInline] readonly_fields = ('voted_by',) list_display_links = ('title',) diff --git a/helpdesk/forms.py b/helpdesk/forms.py index 95eb7e2d..2fc4479f 100644 --- a/helpdesk/forms.py +++ b/helpdesk/forms.py @@ -17,7 +17,7 @@ from django.contrib.auth import get_user_model from django.utils import timezone from helpdesk.lib import safe_template_context, process_attachments -from helpdesk.models import (Ticket, Queue, FollowUp, Attachment, IgnoreEmail, TicketCC, +from helpdesk.models import (Ticket, Queue, FollowUp, IgnoreEmail, TicketCC, CustomField, TicketCustomFieldValue, TicketDependency, UserSettings) from helpdesk import settings as helpdesk_settings diff --git a/helpdesk/lib.py b/helpdesk/lib.py index 2c32e299..b44e4109 100644 --- a/helpdesk/lib.py +++ b/helpdesk/lib.py @@ -15,7 +15,7 @@ from django.db.models import Q from django.utils.encoding import smart_text, smart_str from django.utils.safestring import mark_safe -from helpdesk.models import Attachment, EmailTemplate +from helpdesk.models import FollowUpAttachment, EmailTemplate from model_utils import Choices @@ -218,7 +218,7 @@ def process_attachments(followup, attached_files): if attached.size: filename = smart_text(attached.name) - att = Attachment( + att = FollowUpAttachment( followup=followup, file=attached, filename=filename, diff --git a/helpdesk/migrations/0026_kbitem_attachments.py b/helpdesk/migrations/0026_kbitem_attachments.py new file mode 100644 index 00000000..810672c5 --- /dev/null +++ b/helpdesk/migrations/0026_kbitem_attachments.py @@ -0,0 +1,36 @@ +# Generated by Django 2.0.5 on 2019-03-07 20:30 + +from django.db import migrations, models +import django.db.models.deletion +import helpdesk.models + + +class Migration(migrations.Migration): + + dependencies = [ + ('helpdesk', '0025_queue_dedicated_time'), + ] + + operations = [ + migrations.CreateModel( + name='KBIAttachment', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('file', models.FileField(max_length=1000, upload_to=helpdesk.models.attachment_path, verbose_name='File')), + ('filename', models.CharField(max_length=1000, verbose_name='Filename')), + ('mime_type', models.CharField(max_length=255, verbose_name='MIME Type')), + ('size', models.IntegerField(help_text='Size of this file in bytes', verbose_name='Size')), + ('kbitem', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='helpdesk.KBItem', verbose_name='Knowledge base item')), + ], + options={ + 'verbose_name': 'Attachment', + 'verbose_name_plural': 'Attachments', + 'ordering': ('filename',), + 'abstract': False, + }, + ), + migrations.RenameModel( + old_name='Attachment', + new_name='FollowUpAttachment', + ), + ] diff --git a/helpdesk/models.py b/helpdesk/models.py index 78a02e61..be69b589 100644 --- a/helpdesk/models.py +++ b/helpdesk/models.py @@ -17,6 +17,8 @@ from django.utils import timezone from django.utils.translation import ugettext_lazy as _, ugettext from io import StringIO import re +import os +import mimetypes import datetime from django.utils.safestring import mark_safe @@ -904,18 +906,8 @@ class TicketChange(models.Model): def attachment_path(instance, filename): - """ - Provide a file path that will help prevent files being overwritten, by - putting attachments in a folder off attachments for ticket/followup_id/. - """ - import os - os.umask(0) - path = 'helpdesk/attachments/%s-%s/%s' % (instance.followup.ticket.ticket_for_url, instance.followup.ticket.secret_key, instance.followup.id) - att_path = os.path.join(settings.MEDIA_ROOT, path) - if settings.DEFAULT_FILE_STORAGE == "django.core.files.storage.FileSystemStorage": - if not os.path.exists(att_path): - os.makedirs(att_path, 0o777) - return os.path.join(path, filename) + """Just bridge""" + return instance.attachment_path(filename) class Attachment(models.Model): @@ -924,12 +916,6 @@ class Attachment(models.Model): attachment, or it could be uploaded via the web interface. """ - followup = models.ForeignKey( - FollowUp, - on_delete=models.CASCADE, - verbose_name=_('Follow-up'), - ) - file = models.FileField( _('File'), upload_to=attachment_path, @@ -938,26 +924,102 @@ class Attachment(models.Model): filename = models.CharField( _('Filename'), + blank=True, max_length=1000, ) mime_type = models.CharField( _('MIME Type'), + blank=True, max_length=255, ) size = models.IntegerField( _('Size'), + blank=True, help_text=_('Size of this file in bytes'), ) def __str__(self): return '%s' % self.filename + def save(self, *args, **kwargs): + + if not self.size: + self.size = self.get_size() + + if not self.filename: + self.filename = self.get_filename() + + if not self.mime_type: + self.mime_type = \ + mimetypes.guess_type(self.filename, strict=False)[0] or \ + 'application/octet-stream', + + return super(Attachment, self).save(*args, **kwargs) + + def get_filename(self): + return str(self.file) + + def get_size(self): + return self.file.file.size + + def attachment_path(self, filename): + """Provide a file path that will help prevent files being overwritten, by + putting attachments in a folder off attachments for ticket/followup_id/. + """ + assert NotImplementedError( + "This method is to be implemented by Attachment classes" + ) + class Meta: ordering = ('filename',) verbose_name = _('Attachment') verbose_name_plural = _('Attachments') + abstract = True + + +class FollowUpAttachment(Attachment): + + followup = models.ForeignKey( + FollowUp, + on_delete=models.CASCADE, + verbose_name=_('Follow-up'), + ) + + def attachment_path(self, filename): + + os.umask(0) + path = 'helpdesk/attachments/{ticket_for_url}-{secret_key}/{id_}'.format( + ticket_for_url=self.followup.ticket.ticket_for_url, + secret_key=self.followup.ticket.secret_key, + id_=self.followup.id) + att_path = os.path.join(settings.MEDIA_ROOT, path) + if settings.DEFAULT_FILE_STORAGE == "django.core.files.storage.FileSystemStorage": + if not os.path.exists(att_path): + os.makedirs(att_path, 0o777) + return os.path.join(path, filename) + + +class KBIAttachment(Attachment): + + kbitem = models.ForeignKey( + "KBItem", + on_delete=models.CASCADE, + verbose_name=_('Knowledge base item'), + ) + + def attachment_path(self, filename): + + os.umask(0) + path = 'helpdesk/attachments/kb/{category}/{kbi}'.format( + category=self.kbitem.category, + kbi=self.kbitem.id) + att_path = os.path.join(settings.MEDIA_ROOT, path) + if settings.DEFAULT_FILE_STORAGE == "django.core.files.storage.FileSystemStorage": + if not os.path.exists(att_path): + os.makedirs(att_path, 0o777) + return os.path.join(path, filename) class PreSetReply(models.Model): diff --git a/helpdesk/templates/helpdesk/public_view_ticket.html b/helpdesk/templates/helpdesk/public_view_ticket.html index ccf9ae25..c3ebcce5 100644 --- a/helpdesk/templates/helpdesk/public_view_ticket.html +++ b/helpdesk/templates/helpdesk/public_view_ticket.html @@ -66,7 +66,7 @@
  • {% blocktrans with change.field as field and change.old_value as old_value and change.new_value as new_value %}Changed {{ field }} from {{ old_value }} to {{ new_value }}.{% endblocktrans %}
    • {% endfor %} {% endif %} -{% for attachment in followup.attachment_set.all %}{% if forloop.first %}
      {% endif %} +{% for attachment in followup.followupattachment_set.all %}{% if forloop.first %}
        {% endif %}
      • {{ attachment.filename }} ({{ attachment.mime_type }}, {{ attachment.size|filesizeformat }})
        • {% if forloop.last %}
      {% endif %} {% endfor %} diff --git a/helpdesk/tests/test_attachments.py b/helpdesk/tests/test_attachments.py index 5433e378..5da367b3 100644 --- a/helpdesk/tests/test_attachments.py +++ b/helpdesk/tests/test_attachments.py @@ -58,7 +58,7 @@ class AttachmentIntegrationTests(TestCase): self.assertContains(response, test_file.name) # Ensure attachment is available with correct content - att = models.Attachment.objects.get(followup__ticket=response.context['ticket']) + att = models.FollowUpAttachment.objects.get(followup__ticket=response.context['ticket']) with open(os.path.join(MEDIA_DIR, att.file.name)) as file_on_disk: disk_content = file_on_disk.read() self.assertEqual(disk_content, 'attached file content') @@ -76,7 +76,7 @@ class AttachmentIntegrationTests(TestCase): self.assertContains(response, test_file.name) # Ensure attachment is available with correct content - att = models.Attachment.objects.get(followup__ticket=response.context['ticket']) + att = models.FollowUpAttachment.objects.get(followup__ticket=response.context['ticket']) with open(os.path.join(MEDIA_DIR, att.file.name)) as file_on_disk: disk_content = smart_text(file_on_disk.read(), 'utf-8') self.assertEqual(disk_content, 'โจ') @@ -96,7 +96,7 @@ class AttachmentUnitTests(TestCase): self.test_file = SimpleUploadedFile.from_dict(self.file_attrs) self.follow_up = models.FollowUp(ticket=models.Ticket(queue=models.Queue())) - @mock.patch('helpdesk.lib.Attachment', autospec=True) + @mock.patch('helpdesk.lib.FollowUpAttachment', autospec=True) def test_unicode_attachment_filename(self, mock_att_save, mock_queue_save, mock_ticket_save, mock_follow_up_save): """ check utf-8 data is parsed correcltly """ filename, fileobj = lib.process_attachments(self.follow_up, [self.test_file])[0] @@ -109,7 +109,22 @@ class AttachmentUnitTests(TestCase): ) self.assertEqual(filename, self.file_attrs['filename']) - @mock.patch.object(lib.Attachment, 'save', autospec=True) + @mock.patch('helpdesk.lib.FollowUpAttachment', autospec=True) + def xest_autofill_(self, mock_att_save, mock_queue_save, mock_ticket_save, mock_follow_up_save): + """ check utf-8 data is parsed correcltly """ + filename, fileobj = lib.process_attachments(self.follow_up, [self.test_file])[0] + obj = mock_att_save.assert_called_with( + file=self.test_file, + filename=None, + mime_type=None, + size=None, + followup=self.follow_up + ) + self.assertEqual(obj.filename, self.file_attrs['filename']) + self.assertEqual(obj.size, len(self.file_attrs['content'])) + self.assertEqual(obj.mime_type, self.file_attrs['content-type']) + + @mock.patch.object(lib.FollowUpAttachment, 'save', autospec=True) @override_settings(MEDIA_ROOT=MEDIA_DIR) def test_unicode_filename_to_filesystem(self, mock_att_save, mock_queue_save, mock_ticket_save, mock_follow_up_save): """ don't mock saving to filesystem to test file renames caused by storage layer """ @@ -118,7 +133,7 @@ class AttachmentUnitTests(TestCase): attachment_obj = mock_att_save.call_args[0][0] mock_att_save.assert_called_once_with(attachment_obj) - self.assertIsInstance(attachment_obj, models.Attachment) + self.assertIsInstance(attachment_obj, models.FollowUpAttachment) self.assertEqual(attachment_obj.filename, self.file_attrs['filename']) diff --git a/helpdesk/tests/test_get_email.py b/helpdesk/tests/test_get_email.py index 7a4fe448..56691033 100644 --- a/helpdesk/tests/test_get_email.py +++ b/helpdesk/tests/test_get_email.py @@ -5,7 +5,7 @@ from django.shortcuts import get_object_or_404 from django.contrib.auth.models import User from django.contrib.auth.hashers import make_password -from helpdesk.models import Queue, Ticket, TicketCC, FollowUp, Attachment +from helpdesk.models import Queue, Ticket, TicketCC, FollowUp, FollowUpAttachment import helpdesk.email import itertools @@ -53,7 +53,7 @@ class GetEmailCommonTests(TestCase): with open(os.path.join(THIS_DIR, "test_files/blank-body-with-attachment.eml")) as fd: test_email = fd.read() ticket = helpdesk.email.object_from_message(test_email, self.queue_public, self.logger) - self.assertEqual(ticket.title, "Attachment without body") + self.assertEqual(ticket.title, "FollowUpAttachment without body") self.assertEqual(ticket.description, "") def test_email_with_blank_body_and_attachment(self): @@ -68,7 +68,7 @@ class GetEmailCommonTests(TestCase): followups = FollowUp.objects.filter(ticket=ticket) self.assertEqual(len(followups), 1) followup = followups[0] - attachments = Attachment.objects.filter(followup=followup) + attachments = FollowUpAttachment.objects.filter(followup=followup) self.assertEqual(len(attachments), 1) attachment = attachments[0] self.assertEqual(attachment.file.read().decode("utf-8"), '
      Tohle je test českých písmen odeslaných z gmailu.
      \n') @@ -363,7 +363,7 @@ class GetEmailParametricTemplate(object): # HTML MIME part should be attached to follow up followup1 = get_object_or_404(FollowUp, pk=1) self.assertEqual(followup1.ticket.id, 1) - attach1 = get_object_or_404(Attachment, pk=1) + attach1 = get_object_or_404(FollowUpAttachment, pk=1) self.assertEqual(attach1.followup.id, 1) self.assertEqual(attach1.filename, 'email_html_body.html') cc0 = get_object_or_404(TicketCC, pk=1) @@ -382,7 +382,7 @@ class GetEmailParametricTemplate(object): # HTML MIME part should be attached to follow up followup2 = get_object_or_404(FollowUp, pk=2) self.assertEqual(followup2.ticket.id, 2) - attach2 = get_object_or_404(Attachment, pk=2) + attach2 = get_object_or_404(FollowUpAttachment, pk=2) self.assertEqual(attach2.followup.id, 2) self.assertEqual(attach2.filename, 'email_html_body.html') @@ -449,7 +449,7 @@ class GetEmailParametricTemplate(object): # MIME part should be attached to follow up followup1 = get_object_or_404(FollowUp, pk=1) self.assertEqual(followup1.ticket.id, 1) - attach1 = get_object_or_404(Attachment, pk=1) + attach1 = get_object_or_404(FollowUpAttachment, pk=1) self.assertEqual(attach1.followup.id, 1) self.assertEqual(attach1.filename, 'signature.asc') self.assertEqual(attach1.file.read(), b"""-----BEGIN PGP SIGNATURE----- diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 3acc775d..b09d8cba 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -42,7 +42,7 @@ from helpdesk.lib import ( process_attachments, queue_template_context, ) from helpdesk.models import ( - Ticket, Queue, FollowUp, TicketChange, PreSetReply, Attachment, SavedSearch, + Ticket, Queue, FollowUp, TicketChange, PreSetReply, FollowUpAttachment, SavedSearch, IgnoreEmail, TicketCC, TicketDependency, UserSettings, ) from helpdesk import settings as helpdesk_settings @@ -269,7 +269,7 @@ def followup_edit(request, ticket_id, followup_id): new_followup.user = followup.user new_followup.save() # get list of old attachments & link them to new_followup - attachments = Attachment.objects.filter(followup=followup) + attachments = FolllowUpAttachment.objects.filter(followup=followup) for attachment in attachments: attachment.followup = new_followup attachment.save() @@ -1581,7 +1581,7 @@ def attachment_del(request, ticket_id, attachment_id): if not _is_my_ticket(request.user, ticket): raise PermissionDenied() - attachment = get_object_or_404(Attachment, id=attachment_id) + attachment = get_object_or_404(FolllowUpAttachment, id=attachment_id) if request.method == 'POST': attachment.delete() return HttpResponseRedirect(reverse('helpdesk:view', args=[ticket_id]))