From cf804a586aca06be8dad141ef611e263862c568d Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:16:46 +0200 Subject: [PATCH 01/37] Add verbosity argument to quicktest Enables verbose output for analysis on what is happening --- quicktest.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/quicktest.py b/quicktest.py index c5001732..e387be6e 100755 --- a/quicktest.py +++ b/quicktest.py @@ -14,7 +14,7 @@ import os import sys -class QuickDjangoTest(object): +class QuickDjangoTest: """ A quick way to run the Django test suite without a fully-configured project. @@ -78,6 +78,7 @@ class QuickDjangoTest(object): def __init__(self, *args, **kwargs): self.tests = args + self.kwargs = kwargs or {"verbosity": 1} self._tests() def _tests(self): @@ -112,7 +113,7 @@ class QuickDjangoTest(object): ) from django.test.runner import DiscoverRunner - test_runner = DiscoverRunner(verbosity=1) + test_runner = DiscoverRunner(verbosity=self.kwargs["verbosity"]) django.setup() failures = test_runner.run_tests(self.tests) @@ -134,7 +135,8 @@ if __name__ == '__main__': description="Run Django tests." ) parser.add_argument('tests', nargs="*", type=str) + parser.add_argument("--verbosity", "-v", nargs="?", type=int, default=1) args = parser.parse_args() if not args.tests: args.tests = ['helpdesk'] - QuickDjangoTest(*args.tests) + QuickDjangoTest(*args.tests, verbosity=args.verbosity) From e863609cbecb64183c423ca66702f7d7255a9ab5 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:17:12 +0200 Subject: [PATCH 02/37] Add complexity to flake8 configuration Set to maximum value 20 --- .flake8 | 1 + 1 file changed, 1 insertion(+) diff --git a/.flake8 b/.flake8 index 4bbce879..80233cdf 100644 --- a/.flake8 +++ b/.flake8 @@ -2,6 +2,7 @@ max-line-length = 120 exclude = .git,__pycache__,.tox,.eggs,*.egg,node_modules,.venv,migrations,docs,demo,tests,setup.py import-order-style = pep8 +max-complexity = 20 [pycodestyle] max-line-length = 120 From 1ac78955c03c218c73db50fb04ec262d905e51e4 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:22:05 +0200 Subject: [PATCH 03/37] Removed `notifications_to_be_sent` list The whole loop appeared to be doing nothing other than appending email addresses to a list, which was never used. --- helpdesk/email.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 8f92b521..54b592ca 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -466,16 +466,6 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger) new_ticket_ccs = [] new_ticket_ccs.append(create_ticket_cc(ticket, to_list + cc_list)) - notifications_to_be_sent = [sender_email] - - if queue.enable_notifications_on_email_events and len(notifications_to_be_sent): - - ticket_cc_list = TicketCC.objects.filter( - ticket=ticket).all().values_list('email', flat=True) - - for email_address in ticket_cc_list: - notifications_to_be_sent.append(email_address) - autoreply = is_autoreply(message) if autoreply: logger.info( From 0b1de1eeadaff998ca22a37a3ac83e8258952b48 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:23:12 +0200 Subject: [PATCH 04/37] Removed unused import Result of previous code removal --- helpdesk/email.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index ce27e093..2606f766 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -21,7 +21,7 @@ from email.utils import getaddresses from email_reply_parser import EmailReplyParser from helpdesk import settings from helpdesk.lib import process_attachments, safe_template_context -from helpdesk.models import FollowUp, IgnoreEmail, Queue, Ticket, TicketCC +from helpdesk.models import FollowUp, IgnoreEmail, Queue, Ticket import imaplib import logging import mimetypes From 80f415230174db7af88ac68e72ad9fb47fa9c0e9 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:43:43 +0200 Subject: [PATCH 05/37] Simplify return statement Rename `ticket` to `ticket_id` for clarity --- helpdesk/email.py | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 2606f766..c7ab39ce 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -531,20 +531,18 @@ def object_from_message(message, queue, logger): for ignore in IgnoreEmail.objects.filter(Q(queues=queue) | Q(queues__isnull=True)): if ignore.test(sender_email): - if ignore.keep_in_mailbox: - # By returning 'False' the message will be kept in the mailbox, - # and the 'True' will cause the message to be deleted. - return False - return True + # By returning 'False' the message will be kept in the mailbox, + # and the 'True' will cause the message to be deleted. + return not ignore.keep_in_mailbox matchobj = re.match(r".*\[" + queue.slug + r"-(?P\d+)\]", subject) if matchobj: # This is a reply or forward. - ticket = matchobj.group('id') - logger.info("Matched tracking ID %s-%s" % (queue.slug, ticket)) + ticket_id = matchobj.group('id') + logger.info("Matched tracking ID %s-%s" % (queue.slug, ticket_id)) else: logger.info("No tracking ID matched.") - ticket = None + ticket_id = None body = None full_body = None @@ -568,7 +566,7 @@ def object_from_message(message, queue, logger): body = decodeUnknown(part.get_content_charset(), body) # have to use django_settings here so overwritting it works in tests # the default value is False anyway - if ticket is None and getattr(django_settings, 'HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL', False): + if ticket_id is None and getattr(django_settings, 'HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL', False): # first message in thread, we save full body to avoid # losing forwards and things like that body_parts = [] @@ -690,4 +688,4 @@ def object_from_message(message, queue, logger): 'files': files, } - return create_object_from_email_message(message, ticket, payload, files, logger=logger) + return create_object_from_email_message(message, ticket_id, payload, files, logger=logger) From 283f052c0e72ce9d282eb290707b7bac221cd6ee Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:47:00 +0200 Subject: [PATCH 06/37] Annotate function signature --- helpdesk/email.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index c7ab39ce..8f998823 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -506,7 +506,10 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger) return ticket -def object_from_message(message, queue, logger): +def object_from_message(message: str, + queue: Queue, + logger: logging.Logger + ) -> Ticket: # 'message' must be an RFC822 formatted message. message = email.message_from_string(message) From a5e74d6449a2a78690e75e276efa0e021b643ec1 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 01:56:13 +0200 Subject: [PATCH 07/37] Extract getting ticket_id from subject to helper function --- helpdesk/email.py | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 8f998823..43ff0195 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -33,6 +33,7 @@ import socket import ssl import sys from time import ctime +import typing # import User model, which may be a custom model @@ -506,6 +507,27 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger) return ticket +def get_ticket_id_from_subject_slug( + queue_slug: str, + subject: str, + logger: logging.Logger +) -> typing.Optional[int]: + """Get a ticket id from the subject string + + Performs a match on the subject using the queue_slug as reference, + returning the ticket id if a match is found. + """ + matchobj = re.match(r".*\[" + queue_slug + r"-(?P\d+)\]", subject) + ticket_id = None + if matchobj: + # This is a reply or forward. + ticket_id = matchobj.group('id') + logger.info("Matched tracking ID %s-%s" % (queue_slug, ticket_id)) + else: + logger.info("No tracking ID matched.") + return ticket_id + + def object_from_message(message: str, queue: Queue, logger: logging.Logger @@ -538,14 +560,11 @@ def object_from_message(message: str, # and the 'True' will cause the message to be deleted. return not ignore.keep_in_mailbox - matchobj = re.match(r".*\[" + queue.slug + r"-(?P\d+)\]", subject) - if matchobj: - # This is a reply or forward. - ticket_id = matchobj.group('id') - logger.info("Matched tracking ID %s-%s" % (queue.slug, ticket_id)) - else: - logger.info("No tracking ID matched.") - ticket_id = None + ticket_id: typing.Optional[int] = get_ticket_id_from_subject_slug( + queue.slug, + subject, + logger + ) body = None full_body = None From 4e2b7deefb9d709a08f25a1fce47c284a78d9686 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 02:22:32 +0200 Subject: [PATCH 08/37] Reduces complexity of `object_from_message` Helper functions created to help break up the flow --- helpdesk/email.py | 78 ++++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 28 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 43ff0195..9a3a7076 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -528,6 +528,52 @@ def get_ticket_id_from_subject_slug( return ticket_id +def add_file_if_always_save_incoming_email_message( + files_: list[SimpleUploadedFile], + message: str +) -> None: + """When `settings.HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE` is `True` + add a file to the files_ list""" + if getattr(django_settings, 'HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE', False): + # save message as attachment in case of some complex markup renders + # wrong + files_.append( + SimpleUploadedFile( + _("original_message.eml").replace( + ".eml", + timezone.localtime().strftime("_%d-%m-%Y_%H:%M") + ".eml" + ), + str(message).encode("utf-8"), + 'text/plain' + ) + ) + + +def get_encoded_body(body: str) -> str: + try: + return body.encode('ascii').decode('unicode_escape') + except UnicodeEncodeError: + return body.encode('utf-8') + + +def get_body_from_fragments(body) -> str: + """Gets a body from the fragments, joined by a double line break""" + return "\n\n".join(f.content for f in EmailReplyParser.read(body).fragments) + + +def get_email_body_from_part_payload(part) -> str: + """Gets an decoded body from the payload part, if the decode fails, + returns without encoding""" + try: + return encoding.smart_str( + part.get_payload(decode=True) + ) + except UnicodeDecodeError: + return encoding.smart_str( + part.get_payload(decode=False) + ) + + def object_from_message(message: str, queue: Queue, logger: logging.Logger @@ -591,29 +637,17 @@ def object_from_message(message: str, if ticket_id is None and getattr(django_settings, 'HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL', False): # first message in thread, we save full body to avoid # losing forwards and things like that - body_parts = [] - for f in EmailReplyParser.read(body).fragments: - body_parts.append(f.content) - full_body = '\n\n'.join(body_parts) - body = EmailReplyParser.parse_reply(body) + body = EmailReplyParser.parse_reply(get_body_from_fragments(body)) else: # second and other reply, save only first part of the # message body = EmailReplyParser.parse_reply(body) full_body = body # workaround to get unicode text out rather than escaped text - try: - body = body.encode('ascii').decode('unicode_escape') - except UnicodeEncodeError: - body.encode('utf-8') + body = get_encoded_body(body) logger.debug("Discovered plain text MIME part") else: - try: - email_body = encoding.smart_str( - part.get_payload(decode=True)) - except UnicodeDecodeError: - email_body = encoding.smart_str( - part.get_payload(decode=False)) + email_body = get_email_body_from_part_payload(part) if not body and not full_body: # no text has been parsed so far - try such deep parsing @@ -680,19 +714,7 @@ def object_from_message(message: str, if not body: body = "" - if getattr(django_settings, 'HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE', False): - # save message as attachment in case of some complex markup renders - # wrong - files.append( - SimpleUploadedFile( - _("original_message.eml").replace( - ".eml", - timezone.localtime().strftime("_%d-%m-%Y_%H:%M") + ".eml" - ), - str(message).encode("utf-8"), - 'text/plain' - ) - ) + add_file_if_always_save_incoming_email_message(files, message) smtp_priority = message.get('priority', '') smtp_importance = message.get('importance', '') From 8d63d65a7d5e0ab94a5a95ae4be319c4a1363720 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 02:41:40 +0200 Subject: [PATCH 09/37] Removed encoding to 'utf-8', breaks tests. This needs to be looked into further. --- helpdesk/email.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 9a3a7076..cac0d788 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -553,7 +553,7 @@ def get_encoded_body(body: str) -> str: try: return body.encode('ascii').decode('unicode_escape') except UnicodeEncodeError: - return body.encode('utf-8') + return body def get_body_from_fragments(body) -> str: @@ -637,7 +637,8 @@ def object_from_message(message: str, if ticket_id is None and getattr(django_settings, 'HELPDESK_FULL_FIRST_MESSAGE_FROM_EMAIL', False): # first message in thread, we save full body to avoid # losing forwards and things like that - body = EmailReplyParser.parse_reply(get_body_from_fragments(body)) + full_body = get_body_from_fragments(body) + body = EmailReplyParser.parse_reply(body) else: # second and other reply, save only first part of the # message From 574395ee2814486e6bdca54dad83081279cb20b7 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 02:46:33 +0200 Subject: [PATCH 10/37] Easy pickings Simple code violations of reserved symbols etc. --- helpdesk/views/staff.py | 50 ++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 23 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 21e25d92..e7d8a12a 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -268,7 +268,7 @@ def followup_edit(request, ticket_id, followup_id): 'time_spent': format_time_spent(followup.time_spent), }) - ticketcc_string, show_subscribe = \ + ticketcc_string, __ = \ return_ticketccstring_and_show_subscribe(request.user, ticket) return render(request, 'helpdesk/followup_edit.html', { @@ -346,8 +346,10 @@ def view_ticket(request, ticket_id): if 'subscribe' in request.GET: # Allow the user to subscribe him/herself to the ticket whilst viewing # it. - ticket_cc, show_subscribe = \ - return_ticketccstring_and_show_subscribe(request.user, ticket) + show_subscribe = return_ticketccstring_and_show_subscribe( + request.user, ticket + )[1] + if show_subscribe: subscribe_staff_member_to_ticket(ticket, request.user) return HttpResponseRedirect(reverse('helpdesk:view', args=[ticket.id])) @@ -760,8 +762,10 @@ def update_ticket(request, ticket_id, public=False): # auto subscribe user if enabled if helpdesk_settings.HELPDESK_AUTO_SUBSCRIBE_ON_TICKET_RESPONSE and request.user.is_authenticated: - ticketcc_string, SHOW_SUBSCRIBE = return_ticketccstring_and_show_subscribe( - request.user, ticket) + SHOW_SUBSCRIBE = return_ticketccstring_and_show_subscribe( + request.user, ticket + )[1] + if SHOW_SUBSCRIBE: subscribe_staff_member_to_ticket(ticket, request.user) @@ -924,7 +928,7 @@ def merge_tickets(request): for ticket in tickets: ticket.values = {} # Prepare the value for each attributes of this ticket - for attribute, display_name in ticket_attributes: + for attribute, __ in ticket_attributes: value = getattr(ticket, attribute, default) # Check if attr is a get_FIELD_display if attribute.startswith('get_') and attribute.endswith('_display'): @@ -959,7 +963,7 @@ def merge_tickets(request): ) else: # Save ticket fields values - for attribute, display_name in ticket_attributes: + for attribute, __ in ticket_attributes: id_for_attribute = request.POST.get(attribute) if id_for_attribute != chosen_ticket.id: try: @@ -1086,16 +1090,16 @@ def ticket_list(request): if request.GET.get('search_type', None) == 'header': query = request.GET.get('q') - filter = None + filter_ = None if query.find('-') > 0: try: - queue, id = Ticket.queue_and_id_from_query(query) - id = int(id) + queue, id_ = Ticket.queue_and_id_from_query(query) + id_ = int(id) except ValueError: - id = None + id_ = None - if id: - filter = {'queue__slug': queue, 'id': id} + if id_: + filter_ = {'queue__slug': queue, 'id': id_} else: try: query = int(query) @@ -1103,11 +1107,11 @@ def ticket_list(request): query = None if query: - filter = {'id': int(query)} + filter_ = {'id': int(query)} - if filter: + if filter_: try: - ticket = huser.get_tickets_in_queues().get(**filter) + ticket = huser.get_tickets_in_queues().get(**filter_) return HttpResponseRedirect(ticket.staff_url) except Ticket.DoesNotExist: # Go on to standard keyword searching @@ -1308,15 +1312,15 @@ class CreateTicketView(MustBeStaffMixin, abstract_views.AbstractCreateTicketMixi @helpdesk_staff_member_required -def raw_details(request, type): +def raw_details(request, type_): # TODO: This currently only supports spewing out 'PreSetReply' objects, # in the future it needs to be expanded to include other items. All it # does is return a plain-text representation of an object. - if type not in ('preset',): + if type_ not in ('preset',): raise Http404 - if type == 'preset' and request.GET.get('id', False): + if type_ == 'preset' and request.GET.get('id', False): try: preset = PreSetReply.objects.get(id=request.GET.get('id')) return HttpResponse(preset.body) @@ -1634,8 +1638,8 @@ save_query = staff_member_required(save_query) @helpdesk_staff_member_required -def delete_saved_query(request, id): - query = get_object_or_404(SavedSearch, id=id, user=request.user) +def delete_saved_query(request, pk): + query = get_object_or_404(SavedSearch, id=pk, user=request.user) if request.method == 'POST': query.delete() @@ -1684,8 +1688,8 @@ email_ignore_add = superuser_required(email_ignore_add) @helpdesk_superuser_required -def email_ignore_del(request, id): - ignore = get_object_or_404(IgnoreEmail, id=id) +def email_ignore_del(request, pk): + ignore = get_object_or_404(IgnoreEmail, id=pk) if request.method == 'POST': ignore.delete() return HttpResponseRedirect(reverse('helpdesk:email_ignore')) From ecefd5e407355d41fbacbb028c43d3fbf7f42491 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:01:50 +0200 Subject: [PATCH 11/37] Extract the `due_date` to helper function --- helpdesk/views/staff.py | 81 +++++++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index e7d8a12a..fae230a6 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -16,6 +16,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.decorators import user_passes_test from django.contrib.contenttypes.models import ContentType from django.core.exceptions import PermissionDenied, ValidationError +from django.core.handlers.wsgi import WSGIRequest from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.db.models import Q from django.http import Http404, HttpResponse, HttpResponseRedirect, JsonResponse @@ -70,11 +71,15 @@ import json import re from rest_framework import status from rest_framework.decorators import api_view +import typing if helpdesk_settings.HELPDESK_KB_ENABLED: from helpdesk.models import KBItem +DATE_RE: re.Pattern = re.compile( + r'(?P\d{1,2})/(?P\d{1,2})/(?P\d{4})$' +) User = get_user_model() Query = get_query_class() @@ -483,10 +488,20 @@ def subscribe_staff_member_to_ticket(ticket, user, email='', can_view=True, can_ return subscribe_to_ticket_updates(ticket=ticket, user=user, email=email, can_view=can_view, can_update=can_update) -def update_ticket(request, ticket_id, public=False): +def get_ticket_from_request_with_authorisation( + request: WSGIRequest, + ticket_id: str, + public: bool +) -> typing.Union[ + Ticket, typing.NoReturn +]: + """Gets a ticket from the public status and if the user is authenticated and + has permissions to update tickets - ticket = None + Raises: + Http404 when the ticket can not be found or the user lacks permission + """ if not (public or ( request.user.is_authenticated and request.user.is_active and ( @@ -508,41 +523,29 @@ def update_ticket(request, ticket_id, public=False): '%s?next=%s' % (reverse('helpdesk:login'), request.path) ) - if not ticket: - ticket = get_object_or_404(Ticket, id=ticket_id) + return get_object_or_404(Ticket, id=ticket_id) - date_re = re.compile( - r'(?P\d{1,2})/(?P\d{1,2})/(?P\d{4})$' - ) - comment = request.POST.get('comment', '') - new_status = int(request.POST.get('new_status', ticket.status)) - title = request.POST.get('title', '') - public = request.POST.get('public', False) - owner = int(request.POST.get('owner', -1)) - priority = int(request.POST.get('priority', ticket.priority)) - due_date_year = int(request.POST.get('due_date_year', 0)) - due_date_month = int(request.POST.get('due_date_month', 0)) - due_date_day = int(request.POST.get('due_date_day', 0)) - if request.POST.get("time_spent"): - (hours, minutes) = [int(f) - for f in request.POST.get("time_spent").split(":")] - time_spent = timedelta(hours=hours, minutes=minutes) - else: - time_spent = None - # NOTE: jQuery's default for dates is mm/dd/yy - # very US-centric but for now that's the only format supported - # until we clean up code to internationalize a little more +def get_due_date_from_request_or_ticket( + request: WSGIRequest, + ticket: Ticket +) -> typing.Optional[datetime.date]: + """Tries to locate the due date for a ticket from the `request.POST` + 'due_date' parameter or the `due_date_*` paramaters. + """ due_date = request.POST.get('due_date', None) or None if due_date is not None: # based on Django code to parse dates: # https://docs.djangoproject.com/en/2.0/_modules/django/utils/dateparse/ - match = date_re.match(due_date) + match = DATE_RE.match(due_date) if match: kw = {k: int(v) for k, v in match.groupdict().items()} due_date = date(**kw) else: + due_date_year = int(request.POST.get('due_date_year', 0)) + due_date_month = int(request.POST.get('due_date_month', 0)) + due_date_day = int(request.POST.get('due_date_day', 0)) # old way, probably deprecated? if not (due_date_year and due_date_month and due_date_day): due_date = ticket.due_date @@ -553,9 +556,31 @@ def update_ticket(request, ticket_id, public=False): due_date = ticket.due_date else: due_date = timezone.now() - due_date = due_date.replace( - due_date_year, due_date_month, due_date_day) + due_date = due_date.replace( + due_date_year, due_date_month, due_date_day) + return due_date + +def update_ticket(request, ticket_id, public=False): + + ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public) + + comment = request.POST.get('comment', '') + new_status = int(request.POST.get('new_status', ticket.status)) + title = request.POST.get('title', '') + public = request.POST.get('public', False) + owner = int(request.POST.get('owner', -1)) + priority = int(request.POST.get('priority', ticket.priority)) + if request.POST.get("time_spent"): + (hours, minutes) = [int(f) + for f in request.POST.get("time_spent").split(":")] + time_spent = timedelta(hours=hours, minutes=minutes) + else: + time_spent = None + # NOTE: jQuery's default for dates is mm/dd/yy + # very US-centric but for now that's the only format supported + # until we clean up code to internationalize a little more + due_date = get_due_date_from_request_or_ticket(request, ticket) no_changes = all([ not request.FILES, not comment, From 9294eca5d6417d590cf7c698c40072e954527fd2 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:11:30 +0200 Subject: [PATCH 12/37] Add `get_and_set_ticket_status` helper Extracts some futher code from `update_ticket` --- helpdesk/views/staff.py | 51 ++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index fae230a6..cec32bf4 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -561,6 +561,38 @@ def get_due_date_from_request_or_ticket( return due_date +def get_and_set_ticket_status( + new_status: str, + ticket: Ticket, + follow_up: FollowUp +) -> tuple[str, str]: + """Performs comparision on previous status to new status, + updating the title as required. + + Returns: + The old status as a display string, old status code string + """ + old_status_str = ticket.get_status_display() + old_status = ticket.status + if new_status != ticket.status: + ticket.status = new_status + ticket.save() + follow_up.new_status = new_status + if follow_up.title: + follow_up.title += ' and %s' % ticket.get_status_display() + else: + follow_up.title = '%s' % ticket.get_status_display() + + if not follow_up.title: + if follow_up.comment: + follow_up.title = _('Comment') + else: + follow_up.title = _('Updated') + + follow_up.save() + return (old_status_str, old_status) + + def update_ticket(request, ticket_id, public=False): ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public) @@ -640,24 +672,7 @@ def update_ticket(request, ticket_id, public=False): f.title = _('Unassigned') ticket.assigned_to = None - old_status_str = ticket.get_status_display() - old_status = ticket.status - if new_status != ticket.status: - ticket.status = new_status - ticket.save() - f.new_status = new_status - if f.title: - f.title += ' and %s' % ticket.get_status_display() - else: - f.title = '%s' % ticket.get_status_display() - - if not f.title: - if f.comment: - f.title = _('Comment') - else: - f.title = _('Updated') - - f.save() + old_status_str, old_status = get_and_set_ticket_status(new_status, ticket, f) files = [] if request.FILES: From f815ebbb5c73e5af339bf6d688cb9712658eaa9b Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:17:10 +0200 Subject: [PATCH 13/37] Add `get_time_spent_from_request` helper Extracts further code --- helpdesk/views/staff.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index cec32bf4..635f5352 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -593,6 +593,14 @@ def get_and_set_ticket_status( return (old_status_str, old_status) +def get_time_spent_from_request(request: WSGIRequest) -> typing.Optional[timedelta]: + if request.POST.get("time_spent"): + (hours, minutes) = [int(f) + for f in request.POST.get("time_spent").split(":")] + return timedelta(hours=hours, minutes=minutes) + return None + + def update_ticket(request, ticket_id, public=False): ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public) @@ -603,12 +611,8 @@ def update_ticket(request, ticket_id, public=False): public = request.POST.get('public', False) owner = int(request.POST.get('owner', -1)) priority = int(request.POST.get('priority', ticket.priority)) - if request.POST.get("time_spent"): - (hours, minutes) = [int(f) - for f in request.POST.get("time_spent").split(":")] - time_spent = timedelta(hours=hours, minutes=minutes) - else: - time_spent = None + + time_spent = get_time_spent_from_request(request) # NOTE: jQuery's default for dates is mm/dd/yy # very US-centric but for now that's the only format supported # until we clean up code to internationalize a little more @@ -674,9 +678,7 @@ def update_ticket(request, ticket_id, public=False): old_status_str, old_status = get_and_set_ticket_status(new_status, ticket, f) - files = [] - if request.FILES: - files = process_attachments(f, request.FILES.getlist('attachment')) + files = process_attachments(f, request.FILES.getlist('attachment')) if request.FILES else [] if title and title != ticket.title: c = TicketChange( From fe619b5ff2183504ef45d7191b6aea514c45977e Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:22:59 +0200 Subject: [PATCH 14/37] Combine conditionals to single line --- helpdesk/views/staff.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 635f5352..28a170ff 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -728,9 +728,12 @@ def update_ticket(request, ticket_id, public=False): c.save() ticket.due_date = due_date - if new_status in (Ticket.RESOLVED_STATUS, Ticket.CLOSED_STATUS): - if new_status == Ticket.RESOLVED_STATUS or ticket.resolution is None: - ticket.resolution = comment + if new_status in ( + Ticket.RESOLVED_STATUS, Ticket.CLOSED_STATUS + ) and ( + new_status == Ticket.RESOLVED_STATUS or ticket.resolution is None + ): + ticket.resolution = comment # ticket might have changed above, so we re-instantiate context with the # (possibly) updated ticket. From f678c63496653e0903361451f9b56df3ca45204f Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:30:07 +0200 Subject: [PATCH 15/37] Add `update_messages_sent_to_by_public_and_status` helper function Handles updating ticket and sending ticket reply --- helpdesk/views/staff.py | 67 ++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 28a170ff..ac2365a5 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -601,6 +601,47 @@ def get_time_spent_from_request(request: WSGIRequest) -> typing.Optional[timedel return None +def update_messages_sent_to_by_public_and_status( + public: bool, + ticket: Ticket, + follow_up: FollowUp, + context: str, + messages_sent_to: list[str], + files: list[str, str] +) -> Ticket: + """Sets the status of the ticket""" + if public and ( + follow_up.comment or ( + follow_up.new_status in ( + Ticket.RESOLVED_STATUS, + Ticket.CLOSED_STATUS + ) + ) + ): + if follow_up.new_status == Ticket.RESOLVED_STATUS: + template = 'resolved_' + elif follow_up.new_status == Ticket.CLOSED_STATUS: + template = 'closed_' + else: + template = 'updated_' + + roles = { + 'submitter': (template + 'submitter', context), + 'ticket_cc': (template + 'cc', context), + } + if ticket.assigned_to and ticket.assigned_to.usersettings_helpdesk.email_on_ticket_change: + roles['assigned_to'] = (template + 'cc', context) + messages_sent_to.update( + ticket.send( + roles, + dont_send_to=messages_sent_to, + fail_silently=True, + files=files + ) + ) + return ticket + + def update_ticket(request, ticket_id, public=False): ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public) @@ -748,24 +789,14 @@ def update_ticket(request, ticket_id, public=False): messages_sent_to.add(request.user.email) except AttributeError: pass - if public and (f.comment or ( - f.new_status in (Ticket.RESOLVED_STATUS, - Ticket.CLOSED_STATUS))): - if f.new_status == Ticket.RESOLVED_STATUS: - template = 'resolved_' - elif f.new_status == Ticket.CLOSED_STATUS: - template = 'closed_' - else: - template = 'updated_' - - roles = { - 'submitter': (template + 'submitter', context), - 'ticket_cc': (template + 'cc', context), - } - if ticket.assigned_to and ticket.assigned_to.usersettings_helpdesk.email_on_ticket_change: - roles['assigned_to'] = (template + 'cc', context) - messages_sent_to.update(ticket.send( - roles, dont_send_to=messages_sent_to, fail_silently=True, files=files,)) + ticket = update_messages_sent_to_by_public_and_status( + public, + ticket, + f, + context, + messages_sent_to, + files + ) if reassigned: template_staff = 'assigned_owner' From a2f33c97998d9b94defb8ec5a16a7f64d06dcdff Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:32:45 +0200 Subject: [PATCH 16/37] Add `add_staff_subscription` helper Further reduces complexity by checking for subscription in helper function --- helpdesk/views/staff.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index ac2365a5..6ea6cd2f 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -642,6 +642,19 @@ def update_messages_sent_to_by_public_and_status( return ticket +def add_staff_subscription( + request: WSGIRequest, + ticket: Ticket +) -> None: + if helpdesk_settings.HELPDESK_AUTO_SUBSCRIBE_ON_TICKET_RESPONSE and request.user.is_authenticated: + SHOW_SUBSCRIBE = return_ticketccstring_and_show_subscribe( + request.user, ticket + )[1] + + if SHOW_SUBSCRIBE: + subscribe_staff_member_to_ticket(ticket, request.user) + + def update_ticket(request, ticket_id, public=False): ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public) @@ -833,17 +846,10 @@ def update_ticket(request, ticket_id, public=False): fail_silently=True, files=files, )) - + add_staff_subscription(request, ticket) ticket.save() # auto subscribe user if enabled - if helpdesk_settings.HELPDESK_AUTO_SUBSCRIBE_ON_TICKET_RESPONSE and request.user.is_authenticated: - SHOW_SUBSCRIBE = return_ticketccstring_and_show_subscribe( - request.user, ticket - )[1] - - if SHOW_SUBSCRIBE: - subscribe_staff_member_to_ticket(ticket, request.user) return return_to_ticket(request.user, helpdesk_settings, ticket) From 256af24daa78da5900885e59a98e40a002d91072 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:34:03 +0200 Subject: [PATCH 17/37] Comment function --- helpdesk/views/staff.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 6ea6cd2f..e1de3326 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -646,6 +646,8 @@ def add_staff_subscription( request: WSGIRequest, ticket: Ticket ) -> None: + """Auto subscribe the staff member if that's what the settigs say and the + user is authenticated and a staff member""" if helpdesk_settings.HELPDESK_AUTO_SUBSCRIBE_ON_TICKET_RESPONSE and request.user.is_authenticated: SHOW_SUBSCRIBE = return_ticketccstring_and_show_subscribe( request.user, ticket @@ -846,10 +848,10 @@ def update_ticket(request, ticket_id, public=False): fail_silently=True, files=files, )) - add_staff_subscription(request, ticket) ticket.save() # auto subscribe user if enabled + add_staff_subscription(request, ticket) return return_to_ticket(request.user, helpdesk_settings, ticket) From 595dae1cf7d913592fb5ed3969c0d3c368caae2c Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:38:16 +0200 Subject: [PATCH 18/37] Add `get_template_staff_and_template_cc` function Furhter reduxes complexity by combining creation of templates --- helpdesk/views/staff.py | 42 +++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index e1de3326..074141c9 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -657,6 +657,29 @@ def add_staff_subscription( subscribe_staff_member_to_ticket(ticket, request.user) +def get_template_staff_and_template_cc( + reassigned, follow_up: FollowUp +) -> tuple[str, str]: + if reassigned: + template_staff = 'assigned_owner' + elif follow_up.new_status == Ticket.RESOLVED_STATUS: + template_staff = 'resolved_owner' + elif follow_up.new_status == Ticket.CLOSED_STATUS: + template_staff = 'closed_owner' + else: + template_staff = 'updated_owner' + if reassigned: + template_cc = 'assigned_cc' + elif follow_up.new_status == Ticket.RESOLVED_STATUS: + template_cc = 'resolved_cc' + elif follow_up.new_status == Ticket.CLOSED_STATUS: + template_cc = 'closed_cc' + else: + template_cc = 'updated_cc' + + return template_staff, template_cc + + def update_ticket(request, ticket_id, public=False): ticket = get_ticket_from_request_with_authorisation(request, ticket_id, public) @@ -813,15 +836,7 @@ def update_ticket(request, ticket_id, public=False): files ) - if reassigned: - template_staff = 'assigned_owner' - elif f.new_status == Ticket.RESOLVED_STATUS: - template_staff = 'resolved_owner' - elif f.new_status == Ticket.CLOSED_STATUS: - template_staff = 'closed_owner' - else: - template_staff = 'updated_owner' - + template_staff, template_cc = get_template_staff_and_template_cc(reassigned, f) if ticket.assigned_to and ( ticket.assigned_to.usersettings_helpdesk.email_on_ticket_change or (reassigned and ticket.assigned_to.usersettings_helpdesk.email_on_ticket_assign) @@ -833,15 +848,6 @@ def update_ticket(request, ticket_id, public=False): files=files, )) - if reassigned: - template_cc = 'assigned_cc' - elif f.new_status == Ticket.RESOLVED_STATUS: - template_cc = 'resolved_cc' - elif f.new_status == Ticket.CLOSED_STATUS: - template_cc = 'closed_cc' - else: - template_cc = 'updated_cc' - messages_sent_to.update(ticket.send( {'ticket_cc': (template_cc, context)}, dont_send_to=messages_sent_to, From 749ebbe16be87a128069f673a9cbf62a961d5d3b Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:42:16 +0200 Subject: [PATCH 19/37] Fix annotations for py3.8 --- helpdesk/views/staff.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 074141c9..981091bb 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -565,7 +565,7 @@ def get_and_set_ticket_status( new_status: str, ticket: Ticket, follow_up: FollowUp -) -> tuple[str, str]: +) -> typing.Tuple[str, str]: """Performs comparision on previous status to new status, updating the title as required. @@ -659,7 +659,7 @@ def add_staff_subscription( def get_template_staff_and_template_cc( reassigned, follow_up: FollowUp -) -> tuple[str, str]: +) -> typing.Tuple[str, str]: if reassigned: template_staff = 'assigned_owner' elif follow_up.new_status == Ticket.RESOLVED_STATUS: From 7b4d53cfc040bf7bb13de45eb51dcd56db995f7a Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:44:04 +0200 Subject: [PATCH 20/37] Fix 'list' annoation for py3.8 --- helpdesk/views/staff.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 981091bb..450ffac8 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -606,8 +606,8 @@ def update_messages_sent_to_by_public_and_status( ticket: Ticket, follow_up: FollowUp, context: str, - messages_sent_to: list[str], - files: list[str, str] + messages_sent_to: typing.List[str], + files: typing.List[str, str] ) -> Ticket: """Sets the status of the ticket""" if public and ( From 45e47846fe35ef63dbcc6774078fef8f2c418315 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:45:36 +0200 Subject: [PATCH 21/37] py3.8 annotation fix --- helpdesk/views/staff.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 450ffac8..ac405b6d 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -607,7 +607,7 @@ def update_messages_sent_to_by_public_and_status( follow_up: FollowUp, context: str, messages_sent_to: typing.List[str], - files: typing.List[str, str] + files: typing.List[typing.Tuple[str, str]] ) -> Ticket: """Sets the status of the ticket""" if public and ( From 57cd2f147121112c1cc9939ceed2d4d855bbac96 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:47:57 +0200 Subject: [PATCH 22/37] Remove annoation for py3.8 --- helpdesk/email.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index cac0d788..03f26e9a 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -529,7 +529,7 @@ def get_ticket_id_from_subject_slug( def add_file_if_always_save_incoming_email_message( - files_: list[SimpleUploadedFile], + files_, message: str ) -> None: """When `settings.HELPDESK_ALWAYS_SAVE_INCOMING_EMAIL_MESSAGE` is `True` From b326103d825cf286b4b77e9ecd6c2f6d9619c0f8 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:48:06 +0200 Subject: [PATCH 23/37] Fix spacing --- helpdesk/views/staff.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index ac405b6d..8702e0b2 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -563,8 +563,8 @@ def get_due_date_from_request_or_ticket( def get_and_set_ticket_status( new_status: str, - ticket: Ticket, - follow_up: FollowUp + ticket: Ticket, + follow_up: FollowUp ) -> typing.Tuple[str, str]: """Performs comparision on previous status to new status, updating the title as required. From 14689820433c01015f393b8e576382727e45662c Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:50:49 +0200 Subject: [PATCH 24/37] Remove unused variables, extract correct index --- helpdesk/email.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 03f26e9a..60e476a9 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -177,13 +177,13 @@ def imap_sync(q, logger, server): sys.exit() try: - status, data = server.search(None, 'NOT', 'DELETED') + data = server.search(None, 'NOT', 'DELETED')[1] if data: msgnums = data[0].split() logger.info("Received %d messages from IMAP server" % len(msgnums)) for num in msgnums: logger.info("Processing message %s" % num) - status, data = server.fetch(num, '(RFC822)') + data = server.fetch(num, '(RFC822)')[1] full_message = encoding.force_str(data[0][1], errors='replace') try: ticket = object_from_message( @@ -346,7 +346,7 @@ def create_ticket_cc(ticket, cc_list): from helpdesk.views.staff import subscribe_to_ticket_updates, User new_ticket_ccs = [] - for cced_name, cced_email in cc_list: + for __, cced_email in cc_list: cced_email = cced_email.strip() if cced_email == ticket.queue.email_address: From 46f8e9d21f07c6282f60baac884ef1658dadf26e Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 03:52:58 +0200 Subject: [PATCH 25/37] Clear error Use.objects.get causes undefined variable when using get_user_model --- helpdesk/email.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 60e476a9..1a0b7d50 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -355,7 +355,7 @@ def create_ticket_cc(ticket, cc_list): user = None try: - user = User.objects.get(email=cced_email) + user = User.objects.get(email=cced_email) # @UndefinedVariable except User.DoesNotExist: pass From bed7f0e493d389f414a3934bf33f79ba2a048f41 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:00:57 +0200 Subject: [PATCH 26/37] Add default value property to TicketCustomField Property of the model, so add it there to keep consistency --- helpdesk/models.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/helpdesk/models.py b/helpdesk/models.py index 291e7323..73b2ba7c 100644 --- a/helpdesk/models.py +++ b/helpdesk/models.py @@ -1964,6 +1964,10 @@ class TicketCustomFieldValue(models.Model): def __str__(self): return '%s / %s' % (self.ticket, self.field) + @property + def default_value(self) -> str: + return _("Not defined") + class Meta: unique_together = (('ticket', 'field'),) verbose_name = _('Ticket custom field value') From a783156b611111f367cd1a3b677fdf3c708e3f2a Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:01:27 +0200 Subject: [PATCH 27/37] Add `merge_ticket_values` helper Extract a large portion of code from `merge_tickets` --- helpdesk/views/staff.py | 58 +++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 8702e0b2..88f1e130 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1000,6 +1000,37 @@ ticket_attributes = ( ) +def merge_ticket_values( + request: WSGIRequest, + tickets: typing.List[Ticket], + custom_fields +) -> None: + for ticket in tickets: + ticket.values = {} + # Prepare the value for each attributes of this ticket + for attribute, __ in ticket_attributes: + value = getattr(ticket, attribute, TicketCustomFieldValue.default_value) + # Check if attr is a get_FIELD_display + if attribute.startswith('get_') and attribute.endswith('_display'): + # Hack to call methods like get_FIELD_display() + value = getattr(ticket, attribute, TicketCustomFieldValue.default_value)() + ticket.values[attribute] = { + 'value': value, + 'checked': str(ticket.id) == request.POST.get(attribute) + } + # Prepare the value for each custom fields of this ticket + for custom_field in custom_fields: + try: + value = ticket.ticketcustomfieldvalue_set.get( + field=custom_field).value + except (TicketCustomFieldValue.DoesNotExist, ValueError): + value = TicketCustomFieldValue.default_value + ticket.values[custom_field.name] = { + 'value': value, + 'checked': str(ticket.id) == request.POST.get(custom_field.name) + } + + @staff_member_required def merge_tickets(request): """ @@ -1014,31 +1045,8 @@ def merge_tickets(request): tickets = ticket_select_form.cleaned_data.get('tickets') custom_fields = CustomField.objects.all() - default = _('Not defined') - for ticket in tickets: - ticket.values = {} - # Prepare the value for each attributes of this ticket - for attribute, __ in ticket_attributes: - value = getattr(ticket, attribute, default) - # Check if attr is a get_FIELD_display - if attribute.startswith('get_') and attribute.endswith('_display'): - # Hack to call methods like get_FIELD_display() - value = getattr(ticket, attribute, default)() - ticket.values[attribute] = { - 'value': value, - 'checked': str(ticket.id) == request.POST.get(attribute) - } - # Prepare the value for each custom fields of this ticket - for custom_field in custom_fields: - try: - value = ticket.ticketcustomfieldvalue_set.get( - field=custom_field).value - except (TicketCustomFieldValue.DoesNotExist, ValueError): - value = default - ticket.values[custom_field.name] = { - 'value': value, - 'checked': str(ticket.id) == request.POST.get(custom_field.name) - } + + merge_ticket_values(request, tickets, custom_fields) if request.method == 'POST': # Find which ticket has been chosen to be the main one From eb11c4fe0ee78bf0c37c96d329d3945be355ed20 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:03:12 +0200 Subject: [PATCH 28/37] Rename `ticket_attriubtes` to upper, module level constant. --- helpdesk/views/staff.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 88f1e130..82b4c5d3 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -989,7 +989,7 @@ mass_update = staff_member_required(mass_update) # Prepare ticket attributes which will be displayed in the table to choose # which value to keep when merging -ticket_attributes = ( +TICKET_ATTRIBUTES = ( ('created', _('Created date')), ('due_date', _('Due on')), ('get_status_display', _('Status')), @@ -1008,7 +1008,7 @@ def merge_ticket_values( for ticket in tickets: ticket.values = {} # Prepare the value for each attributes of this ticket - for attribute, __ in ticket_attributes: + for attribute, __ in TICKET_ATTRIBUTES: value = getattr(ticket, attribute, TicketCustomFieldValue.default_value) # Check if attr is a get_FIELD_display if attribute.startswith('get_') and attribute.endswith('_display'): @@ -1061,7 +1061,7 @@ def merge_tickets(request): ) else: # Save ticket fields values - for attribute, __ in ticket_attributes: + for attribute, __ in TICKET_ATTRIBUTES: id_for_attribute = request.POST.get(attribute) if id_for_attribute != chosen_ticket.id: try: @@ -1151,7 +1151,7 @@ def merge_tickets(request): return render(request, 'helpdesk/ticket_merge.html', { 'tickets': tickets, - 'ticket_attributes': ticket_attributes, + 'TICKET_ATTRIBUTES': TICKET_ATTRIBUTES, 'custom_fields': custom_fields, 'ticket_select_form': ticket_select_form }) From a248181857a2ff9f43428cc621a3cdcd004070d8 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:08:16 +0200 Subject: [PATCH 29/37] Add `redirect_from_chosen_ticket` helper function Moves the whole handling to own block, reducing complexity greatly. --- helpdesk/views/staff.py | 190 +++++++++++++++++++++------------------- 1 file changed, 102 insertions(+), 88 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 82b4c5d3..3ced684e 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1031,6 +1031,102 @@ def merge_ticket_values( } +def redirect_from_chosen_ticket( + request, + chosen_ticket, + tickets, + custom_fields +) -> HttpResponseRedirect: + # Save ticket fields values + for attribute, __ in TICKET_ATTRIBUTES: + id_for_attribute = request.POST.get(attribute) + if id_for_attribute != chosen_ticket.id: + try: + selected_ticket = tickets.get(id=id_for_attribute) + except (Ticket.DoesNotExist, ValueError): + continue + + # Check if attr is a get_FIELD_display + if attribute.startswith('get_') and attribute.endswith('_display'): + # Keep only the FIELD part + attribute = attribute[4:-8] + # Get value from selected ticket and then save it on + # the chosen ticket + value = getattr(selected_ticket, attribute) + setattr(chosen_ticket, attribute, value) + # Save custom fields values + for custom_field in custom_fields: + id_for_custom_field = request.POST.get(custom_field.name) + if id_for_custom_field != chosen_ticket.id: + try: + selected_ticket = tickets.get( + id=id_for_custom_field) + except (Ticket.DoesNotExist, ValueError): + continue + + # Check if the value for this ticket custom field + # exists + try: + value = selected_ticket.ticketcustomfieldvalue_set.get( + field=custom_field).value + except TicketCustomFieldValue.DoesNotExist: + continue + + # Create the custom field value or update it with the + # value from the selected ticket + custom_field_value, created = chosen_ticket.ticketcustomfieldvalue_set.get_or_create( + field=custom_field, + defaults={'value': value} + ) + if not created: + custom_field_value.value = value + custom_field_value.save(update_fields=['value']) + # Save changes + chosen_ticket.save() + + # For other tickets, save the link to the ticket in which they have been merged to + # and set status to DUPLICATE + for ticket in tickets.exclude(id=chosen_ticket.id): + ticket.merged_to = chosen_ticket + ticket.status = Ticket.DUPLICATE_STATUS + ticket.save() + + # Send mail to submitter email and ticket CC to let them + # know ticket has been merged + context = safe_template_context(ticket) + if ticket.submitter_email: + send_templated_mail( + template_name='merged', + context=context, + recipients=[ticket.submitter_email], + bcc=[ + cc.email_address for cc in ticket.ticketcc_set.select_related('user')], + sender=ticket.queue.from_address, + fail_silently=True + ) + + # Move all followups and update their title to know they + # come from another ticket + ticket.followup_set.update( + ticket=chosen_ticket, + # Next might exceed maximum 200 characters limit + title=_('[Merged from #%(id)d] %(title)s') % { + 'id': ticket.id, 'title': ticket.title} + ) + + # Add submitter_email, assigned_to email and ticketcc to + # chosen ticket if necessary + chosen_ticket.add_email_to_ticketcc_if_not_in( + email=ticket.submitter_email) + if ticket.assigned_to and ticket.assigned_to.email: + chosen_ticket.add_email_to_ticketcc_if_not_in( + email=ticket.assigned_to.email) + for ticketcc in ticket.ticketcc_set.all(): + chosen_ticket.add_email_to_ticketcc_if_not_in( + ticketcc=ticketcc) + return redirect(chosen_ticket) + + @staff_member_required def merge_tickets(request): """ @@ -1060,94 +1156,12 @@ def merge_tickets(request): 'Please choose a ticket in which the others will be merged into.') ) else: - # Save ticket fields values - for attribute, __ in TICKET_ATTRIBUTES: - id_for_attribute = request.POST.get(attribute) - if id_for_attribute != chosen_ticket.id: - try: - selected_ticket = tickets.get(id=id_for_attribute) - except (Ticket.DoesNotExist, ValueError): - continue - - # Check if attr is a get_FIELD_display - if attribute.startswith('get_') and attribute.endswith('_display'): - # Keep only the FIELD part - attribute = attribute[4:-8] - # Get value from selected ticket and then save it on - # the chosen ticket - value = getattr(selected_ticket, attribute) - setattr(chosen_ticket, attribute, value) - # Save custom fields values - for custom_field in custom_fields: - id_for_custom_field = request.POST.get(custom_field.name) - if id_for_custom_field != chosen_ticket.id: - try: - selected_ticket = tickets.get( - id=id_for_custom_field) - except (Ticket.DoesNotExist, ValueError): - continue - - # Check if the value for this ticket custom field - # exists - try: - value = selected_ticket.ticketcustomfieldvalue_set.get( - field=custom_field).value - except TicketCustomFieldValue.DoesNotExist: - continue - - # Create the custom field value or update it with the - # value from the selected ticket - custom_field_value, created = chosen_ticket.ticketcustomfieldvalue_set.get_or_create( - field=custom_field, - defaults={'value': value} - ) - if not created: - custom_field_value.value = value - custom_field_value.save(update_fields=['value']) - # Save changes - chosen_ticket.save() - - # For other tickets, save the link to the ticket in which they have been merged to - # and set status to DUPLICATE - for ticket in tickets.exclude(id=chosen_ticket.id): - ticket.merged_to = chosen_ticket - ticket.status = Ticket.DUPLICATE_STATUS - ticket.save() - - # Send mail to submitter email and ticket CC to let them - # know ticket has been merged - context = safe_template_context(ticket) - if ticket.submitter_email: - send_templated_mail( - template_name='merged', - context=context, - recipients=[ticket.submitter_email], - bcc=[ - cc.email_address for cc in ticket.ticketcc_set.select_related('user')], - sender=ticket.queue.from_address, - fail_silently=True - ) - - # Move all followups and update their title to know they - # come from another ticket - ticket.followup_set.update( - ticket=chosen_ticket, - # Next might exceed maximum 200 characters limit - title=_('[Merged from #%(id)d] %(title)s') % { - 'id': ticket.id, 'title': ticket.title} - ) - - # Add submitter_email, assigned_to email and ticketcc to - # chosen ticket if necessary - chosen_ticket.add_email_to_ticketcc_if_not_in( - email=ticket.submitter_email) - if ticket.assigned_to and ticket.assigned_to.email: - chosen_ticket.add_email_to_ticketcc_if_not_in( - email=ticket.assigned_to.email) - for ticketcc in ticket.ticketcc_set.all(): - chosen_ticket.add_email_to_ticketcc_if_not_in( - ticketcc=ticketcc) - return redirect(chosen_ticket) + return redirect_from_chosen_ticket( + request, + chosen_ticket, + tickets, + custom_fields + ) return render(request, 'helpdesk/ticket_merge.html', { 'tickets': tickets, From 40a243c23b51cb883c7f6dd8818031d88e1f6eb5 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:15:53 +0200 Subject: [PATCH 30/37] Revert changes, updating objects missed somewhere --- helpdesk/views/staff.py | 196 +++++++++++++++++++--------------------- 1 file changed, 91 insertions(+), 105 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 3ced684e..88f1e130 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -989,7 +989,7 @@ mass_update = staff_member_required(mass_update) # Prepare ticket attributes which will be displayed in the table to choose # which value to keep when merging -TICKET_ATTRIBUTES = ( +ticket_attributes = ( ('created', _('Created date')), ('due_date', _('Due on')), ('get_status_display', _('Status')), @@ -1008,7 +1008,7 @@ def merge_ticket_values( for ticket in tickets: ticket.values = {} # Prepare the value for each attributes of this ticket - for attribute, __ in TICKET_ATTRIBUTES: + for attribute, __ in ticket_attributes: value = getattr(ticket, attribute, TicketCustomFieldValue.default_value) # Check if attr is a get_FIELD_display if attribute.startswith('get_') and attribute.endswith('_display'): @@ -1031,102 +1031,6 @@ def merge_ticket_values( } -def redirect_from_chosen_ticket( - request, - chosen_ticket, - tickets, - custom_fields -) -> HttpResponseRedirect: - # Save ticket fields values - for attribute, __ in TICKET_ATTRIBUTES: - id_for_attribute = request.POST.get(attribute) - if id_for_attribute != chosen_ticket.id: - try: - selected_ticket = tickets.get(id=id_for_attribute) - except (Ticket.DoesNotExist, ValueError): - continue - - # Check if attr is a get_FIELD_display - if attribute.startswith('get_') and attribute.endswith('_display'): - # Keep only the FIELD part - attribute = attribute[4:-8] - # Get value from selected ticket and then save it on - # the chosen ticket - value = getattr(selected_ticket, attribute) - setattr(chosen_ticket, attribute, value) - # Save custom fields values - for custom_field in custom_fields: - id_for_custom_field = request.POST.get(custom_field.name) - if id_for_custom_field != chosen_ticket.id: - try: - selected_ticket = tickets.get( - id=id_for_custom_field) - except (Ticket.DoesNotExist, ValueError): - continue - - # Check if the value for this ticket custom field - # exists - try: - value = selected_ticket.ticketcustomfieldvalue_set.get( - field=custom_field).value - except TicketCustomFieldValue.DoesNotExist: - continue - - # Create the custom field value or update it with the - # value from the selected ticket - custom_field_value, created = chosen_ticket.ticketcustomfieldvalue_set.get_or_create( - field=custom_field, - defaults={'value': value} - ) - if not created: - custom_field_value.value = value - custom_field_value.save(update_fields=['value']) - # Save changes - chosen_ticket.save() - - # For other tickets, save the link to the ticket in which they have been merged to - # and set status to DUPLICATE - for ticket in tickets.exclude(id=chosen_ticket.id): - ticket.merged_to = chosen_ticket - ticket.status = Ticket.DUPLICATE_STATUS - ticket.save() - - # Send mail to submitter email and ticket CC to let them - # know ticket has been merged - context = safe_template_context(ticket) - if ticket.submitter_email: - send_templated_mail( - template_name='merged', - context=context, - recipients=[ticket.submitter_email], - bcc=[ - cc.email_address for cc in ticket.ticketcc_set.select_related('user')], - sender=ticket.queue.from_address, - fail_silently=True - ) - - # Move all followups and update their title to know they - # come from another ticket - ticket.followup_set.update( - ticket=chosen_ticket, - # Next might exceed maximum 200 characters limit - title=_('[Merged from #%(id)d] %(title)s') % { - 'id': ticket.id, 'title': ticket.title} - ) - - # Add submitter_email, assigned_to email and ticketcc to - # chosen ticket if necessary - chosen_ticket.add_email_to_ticketcc_if_not_in( - email=ticket.submitter_email) - if ticket.assigned_to and ticket.assigned_to.email: - chosen_ticket.add_email_to_ticketcc_if_not_in( - email=ticket.assigned_to.email) - for ticketcc in ticket.ticketcc_set.all(): - chosen_ticket.add_email_to_ticketcc_if_not_in( - ticketcc=ticketcc) - return redirect(chosen_ticket) - - @staff_member_required def merge_tickets(request): """ @@ -1156,16 +1060,98 @@ def merge_tickets(request): 'Please choose a ticket in which the others will be merged into.') ) else: - return redirect_from_chosen_ticket( - request, - chosen_ticket, - tickets, - custom_fields - ) + # Save ticket fields values + for attribute, __ in ticket_attributes: + id_for_attribute = request.POST.get(attribute) + if id_for_attribute != chosen_ticket.id: + try: + selected_ticket = tickets.get(id=id_for_attribute) + except (Ticket.DoesNotExist, ValueError): + continue + + # Check if attr is a get_FIELD_display + if attribute.startswith('get_') and attribute.endswith('_display'): + # Keep only the FIELD part + attribute = attribute[4:-8] + # Get value from selected ticket and then save it on + # the chosen ticket + value = getattr(selected_ticket, attribute) + setattr(chosen_ticket, attribute, value) + # Save custom fields values + for custom_field in custom_fields: + id_for_custom_field = request.POST.get(custom_field.name) + if id_for_custom_field != chosen_ticket.id: + try: + selected_ticket = tickets.get( + id=id_for_custom_field) + except (Ticket.DoesNotExist, ValueError): + continue + + # Check if the value for this ticket custom field + # exists + try: + value = selected_ticket.ticketcustomfieldvalue_set.get( + field=custom_field).value + except TicketCustomFieldValue.DoesNotExist: + continue + + # Create the custom field value or update it with the + # value from the selected ticket + custom_field_value, created = chosen_ticket.ticketcustomfieldvalue_set.get_or_create( + field=custom_field, + defaults={'value': value} + ) + if not created: + custom_field_value.value = value + custom_field_value.save(update_fields=['value']) + # Save changes + chosen_ticket.save() + + # For other tickets, save the link to the ticket in which they have been merged to + # and set status to DUPLICATE + for ticket in tickets.exclude(id=chosen_ticket.id): + ticket.merged_to = chosen_ticket + ticket.status = Ticket.DUPLICATE_STATUS + ticket.save() + + # Send mail to submitter email and ticket CC to let them + # know ticket has been merged + context = safe_template_context(ticket) + if ticket.submitter_email: + send_templated_mail( + template_name='merged', + context=context, + recipients=[ticket.submitter_email], + bcc=[ + cc.email_address for cc in ticket.ticketcc_set.select_related('user')], + sender=ticket.queue.from_address, + fail_silently=True + ) + + # Move all followups and update their title to know they + # come from another ticket + ticket.followup_set.update( + ticket=chosen_ticket, + # Next might exceed maximum 200 characters limit + title=_('[Merged from #%(id)d] %(title)s') % { + 'id': ticket.id, 'title': ticket.title} + ) + + # Add submitter_email, assigned_to email and ticketcc to + # chosen ticket if necessary + chosen_ticket.add_email_to_ticketcc_if_not_in( + email=ticket.submitter_email) + if ticket.assigned_to and ticket.assigned_to.email: + chosen_ticket.add_email_to_ticketcc_if_not_in( + email=ticket.assigned_to.email) + for ticketcc in ticket.ticketcc_set.all(): + chosen_ticket.add_email_to_ticketcc_if_not_in( + ticketcc=ticketcc) + return redirect(chosen_ticket) return render(request, 'helpdesk/ticket_merge.html', { 'tickets': tickets, - 'TICKET_ATTRIBUTES': TICKET_ATTRIBUTES, + 'ticket_attributes': ticket_attributes, 'custom_fields': custom_fields, 'ticket_select_form': ticket_select_form }) From f89f5b91da72264c0d5f0ed4a97aa6e644ba545d Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:23:03 +0200 Subject: [PATCH 31/37] Reinstate changes, fixed missing update --- helpdesk/views/staff.py | 196 +++++++++++++++++++++------------------- 1 file changed, 105 insertions(+), 91 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 88f1e130..dac7ed42 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -989,7 +989,7 @@ mass_update = staff_member_required(mass_update) # Prepare ticket attributes which will be displayed in the table to choose # which value to keep when merging -ticket_attributes = ( +TICKET_ATTRIBUTES = ( ('created', _('Created date')), ('due_date', _('Due on')), ('get_status_display', _('Status')), @@ -1008,7 +1008,7 @@ def merge_ticket_values( for ticket in tickets: ticket.values = {} # Prepare the value for each attributes of this ticket - for attribute, __ in ticket_attributes: + for attribute, __ in TICKET_ATTRIBUTES: value = getattr(ticket, attribute, TicketCustomFieldValue.default_value) # Check if attr is a get_FIELD_display if attribute.startswith('get_') and attribute.endswith('_display'): @@ -1031,6 +1031,102 @@ def merge_ticket_values( } +def redirect_from_chosen_ticket( + request, + chosen_ticket, + tickets, + custom_fields +) -> HttpResponseRedirect: + # Save ticket fields values + for attribute, __ in TICKET_ATTRIBUTES: + id_for_attribute = request.POST.get(attribute) + if id_for_attribute != chosen_ticket.id: + try: + selected_ticket = tickets.get(id=id_for_attribute) + except (Ticket.DoesNotExist, ValueError): + continue + + # Check if attr is a get_FIELD_display + if attribute.startswith('get_') and attribute.endswith('_display'): + # Keep only the FIELD part + attribute = attribute[4:-8] + # Get value from selected ticket and then save it on + # the chosen ticket + value = getattr(selected_ticket, attribute) + setattr(chosen_ticket, attribute, value) + # Save custom fields values + for custom_field in custom_fields: + id_for_custom_field = request.POST.get(custom_field.name) + if id_for_custom_field != chosen_ticket.id: + try: + selected_ticket = tickets.get( + id=id_for_custom_field) + except (Ticket.DoesNotExist, ValueError): + continue + + # Check if the value for this ticket custom field + # exists + try: + value = selected_ticket.ticketcustomfieldvalue_set.get( + field=custom_field).value + except TicketCustomFieldValue.DoesNotExist: + continue + + # Create the custom field value or update it with the + # value from the selected ticket + custom_field_value, created = chosen_ticket.ticketcustomfieldvalue_set.get_or_create( + field=custom_field, + defaults={'value': value} + ) + if not created: + custom_field_value.value = value + custom_field_value.save(update_fields=['value']) + # Save changes + chosen_ticket.save() + + # For other tickets, save the link to the ticket in which they have been merged to + # and set status to DUPLICATE + for ticket in tickets.exclude(id=chosen_ticket.id): + ticket.merged_to = chosen_ticket + ticket.status = Ticket.DUPLICATE_STATUS + ticket.save() + + # Send mail to submitter email and ticket CC to let them + # know ticket has been merged + context = safe_template_context(ticket) + if ticket.submitter_email: + send_templated_mail( + template_name='merged', + context=context, + recipients=[ticket.submitter_email], + bcc=[ + cc.email_address for cc in ticket.ticketcc_set.select_related('user')], + sender=ticket.queue.from_address, + fail_silently=True + ) + + # Move all followups and update their title to know they + # come from another ticket + ticket.followup_set.update( + ticket=chosen_ticket, + # Next might exceed maximum 200 characters limit + title=_('[Merged from #%(id)d] %(title)s') % { + 'id': ticket.id, 'title': ticket.title} + ) + + # Add submitter_email, assigned_to email and ticketcc to + # chosen ticket if necessary + chosen_ticket.add_email_to_ticketcc_if_not_in( + email=ticket.submitter_email) + if ticket.assigned_to and ticket.assigned_to.email: + chosen_ticket.add_email_to_ticketcc_if_not_in( + email=ticket.assigned_to.email) + for ticketcc in ticket.ticketcc_set.all(): + chosen_ticket.add_email_to_ticketcc_if_not_in( + ticketcc=ticketcc) + return redirect(chosen_ticket) + + @staff_member_required def merge_tickets(request): """ @@ -1060,98 +1156,16 @@ def merge_tickets(request): 'Please choose a ticket in which the others will be merged into.') ) else: - # Save ticket fields values - for attribute, __ in ticket_attributes: - id_for_attribute = request.POST.get(attribute) - if id_for_attribute != chosen_ticket.id: - try: - selected_ticket = tickets.get(id=id_for_attribute) - except (Ticket.DoesNotExist, ValueError): - continue - - # Check if attr is a get_FIELD_display - if attribute.startswith('get_') and attribute.endswith('_display'): - # Keep only the FIELD part - attribute = attribute[4:-8] - # Get value from selected ticket and then save it on - # the chosen ticket - value = getattr(selected_ticket, attribute) - setattr(chosen_ticket, attribute, value) - # Save custom fields values - for custom_field in custom_fields: - id_for_custom_field = request.POST.get(custom_field.name) - if id_for_custom_field != chosen_ticket.id: - try: - selected_ticket = tickets.get( - id=id_for_custom_field) - except (Ticket.DoesNotExist, ValueError): - continue - - # Check if the value for this ticket custom field - # exists - try: - value = selected_ticket.ticketcustomfieldvalue_set.get( - field=custom_field).value - except TicketCustomFieldValue.DoesNotExist: - continue - - # Create the custom field value or update it with the - # value from the selected ticket - custom_field_value, created = chosen_ticket.ticketcustomfieldvalue_set.get_or_create( - field=custom_field, - defaults={'value': value} - ) - if not created: - custom_field_value.value = value - custom_field_value.save(update_fields=['value']) - # Save changes - chosen_ticket.save() - - # For other tickets, save the link to the ticket in which they have been merged to - # and set status to DUPLICATE - for ticket in tickets.exclude(id=chosen_ticket.id): - ticket.merged_to = chosen_ticket - ticket.status = Ticket.DUPLICATE_STATUS - ticket.save() - - # Send mail to submitter email and ticket CC to let them - # know ticket has been merged - context = safe_template_context(ticket) - if ticket.submitter_email: - send_templated_mail( - template_name='merged', - context=context, - recipients=[ticket.submitter_email], - bcc=[ - cc.email_address for cc in ticket.ticketcc_set.select_related('user')], - sender=ticket.queue.from_address, - fail_silently=True - ) - - # Move all followups and update their title to know they - # come from another ticket - ticket.followup_set.update( - ticket=chosen_ticket, - # Next might exceed maximum 200 characters limit - title=_('[Merged from #%(id)d] %(title)s') % { - 'id': ticket.id, 'title': ticket.title} - ) - - # Add submitter_email, assigned_to email and ticketcc to - # chosen ticket if necessary - chosen_ticket.add_email_to_ticketcc_if_not_in( - email=ticket.submitter_email) - if ticket.assigned_to and ticket.assigned_to.email: - chosen_ticket.add_email_to_ticketcc_if_not_in( - email=ticket.assigned_to.email) - for ticketcc in ticket.ticketcc_set.all(): - chosen_ticket.add_email_to_ticketcc_if_not_in( - ticketcc=ticketcc) - return redirect(chosen_ticket) + return redirect_from_chosen_ticket( + request, + chosen_ticket, + tickets, + custom_fields + ) return render(request, 'helpdesk/ticket_merge.html', { 'tickets': tickets, - 'ticket_attributes': ticket_attributes, + 'ticket_attributes': TICKET_ATTRIBUTES, 'custom_fields': custom_fields, 'ticket_select_form': ticket_select_form }) From d858c40416ef0e6db3e640bb126ca472c4b011f4 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:29:43 +0200 Subject: [PATCH 32/37] Add `check_redirect_on_user_query` helper function Extract the checking for a redirect to reduce complexity --- helpdesk/views/staff.py | 65 +++++++++++++++++++++++------------------ 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index dac7ed42..586ccadd 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1171,35 +1171,12 @@ def merge_tickets(request): }) -@helpdesk_staff_member_required -def ticket_list(request): - context = {} - - huser = HelpdeskUser(request.user) - - # Query_params will hold a dictionary of parameters relating to - # a query, to be saved if needed: - query_params = { - 'filtering': {}, - 'filtering_or': {}, - 'sorting': None, - 'sortreverse': False, - 'search_string': '', - } - default_query_params = { - 'filtering': { - 'status__in': [1, 2], - }, - 'sorting': 'created', - 'search_string': '', - 'sortreverse': False, - } - - # If the user is coming from the header/navigation search box, lets' first - # look at their query to see if they have entered a valid ticket number. If - # they have, just redirect to that ticket number. Otherwise, we treat it as - # a keyword search. - +def check_redirect_on_user_query(request, huser): + """If the user is coming from the header/navigation search box, lets' first + look at their query to see if they have entered a valid ticket number. If + they have, just redirect to that ticket number. Otherwise, we treat it as + a keyword search. + """ if request.GET.get('search_type', None) == 'header': query = request.GET.get('q') filter_ = None @@ -1228,7 +1205,37 @@ def ticket_list(request): except Ticket.DoesNotExist: # Go on to standard keyword searching pass + return None + +@helpdesk_staff_member_required +def ticket_list(request): + context = {} + + huser = HelpdeskUser(request.user) + + # Query_params will hold a dictionary of parameters relating to + # a query, to be saved if needed: + query_params = { + 'filtering': {}, + 'filtering_or': {}, + 'sorting': None, + 'sortreverse': False, + 'search_string': '', + } + default_query_params = { + 'filtering': { + 'status__in': [1, 2], + }, + 'sorting': 'created', + 'search_string': '', + 'sortreverse': False, + } + + #: check for a redirect, see function doc for details + redirect = check_redirect_on_user_query(request, huser) + if redirect: + return redirect try: saved_query, query_params = load_saved_query(request, query_params) except QueryLoadError: From 50bd72ac7a0fc75981fa9346902762bf5336de65 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:31:17 +0200 Subject: [PATCH 33/37] Move import to top --- helpdesk/views/staff.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 586ccadd..783135ff 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -6,9 +6,9 @@ django-helpdesk - A Django powered ticket tracker for small enterprise. views/staff.py - The bulk of the application - provides most business logic and renders all staff-facing views. """ - from ..lib import format_time_spent from ..templated_email import send_templated_mail +from collections import defaultdict from copy import deepcopy from datetime import date, datetime, timedelta from django.conf import settings @@ -1558,7 +1558,6 @@ def run_report(request, report): if request.GET.get('saved_query', None): Query(report_queryset, query_to_base64(query_params)) - from collections import defaultdict summarytable = defaultdict(int) # a second table for more complex queries summarytable2 = defaultdict(int) From b1bf2cab46a99251965e5a7d17009a23d70c45b1 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:35:49 +0200 Subject: [PATCH 34/37] Add `get_report_queryset_or_redirect` helper Gets required objects or redirects --- helpdesk/views/staff.py | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 783135ff..eb850223 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1539,12 +1539,18 @@ def report_index(request): report_index = staff_member_required(report_index) -@helpdesk_staff_member_required -def run_report(request, report): +def get_report_queryset_or_redirect(request, report): if Ticket.objects.all().count() == 0 or report not in ( - 'queuemonth', 'usermonth', 'queuestatus', 'queuepriority', 'userstatus', - 'userpriority', 'userqueue', 'daysuntilticketclosedbymonth'): - return HttpResponseRedirect(reverse("helpdesk:report_index")) + "queuemonth", + "usermonth", + "queuestatus", + "queuepriority", + "userstatus", + "userpriority", + "userqueue", + "daysuntilticketclosedbymonth" + ): + return None, None, HttpResponseRedirect(reverse("helpdesk:report_index")) report_queryset = Ticket.objects.all().select_related().filter( queue__in=HelpdeskUser(request.user).get_queues() @@ -1553,8 +1559,18 @@ def run_report(request, report): try: saved_query, query_params = load_saved_query(request) except QueryLoadError: - return HttpResponseRedirect(reverse('helpdesk:report_index')) + return None, HttpResponseRedirect(reverse('helpdesk:report_index')) + return report_queryset, query_params, saved_query, None + +@helpdesk_staff_member_required +def run_report(request, report): + + report_queryset, query_params, saved_query, redirect = get_report_queryset_or_redirect( + request, report + ) + if redirect: + return redirect if request.GET.get('saved_query', None): Query(report_queryset, query_to_base64(query_params)) From 205c69b539537b7b581fea3a9fff85323c0ae6d4 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:41:01 +0200 Subject: [PATCH 35/37] Add `get_report_table_and_totals` helper function Extracts a large portion of run_report handling --- helpdesk/views/staff.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index eb850223..30dc88a1 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1563,6 +1563,21 @@ def get_report_queryset_or_redirect(request, report): return report_queryset, query_params, saved_query, None +def get_report_table_and_totals(header1, summarytable, possible_options): + table = [] + totals = {} + for item in header1: + data = [] + for hdr in possible_options: + if hdr not in totals.keys(): + totals[hdr] = summarytable[item, hdr] + else: + totals[hdr] += summarytable[item, hdr] + data.append(summarytable[item, hdr]) + table.append([item] + data) + return table, totals + + @helpdesk_staff_member_required def run_report(request, report): @@ -1690,8 +1705,6 @@ def run_report(request, report): if report == 'daysuntilticketclosedbymonth': summarytable2[metric1, metric2] += metric3 - table = [] - if report == 'daysuntilticketclosedbymonth': for key in summarytable2.keys(): summarytable[key] = summarytable2[key] / summarytable[key] @@ -1701,18 +1714,9 @@ def run_report(request, report): column_headings = [col1heading] + possible_options # Prepare a dict to store totals for each possible option - totals = {} + table, totals = get_report_table_and_totals(header1, summarytable, possible_options) # Pivot the data so that 'header1' fields are always first column # in the row, and 'possible_options' are always the 2nd - nth columns. - for item in header1: - data = [] - for hdr in possible_options: - if hdr not in totals.keys(): - totals[hdr] = summarytable[item, hdr] - else: - totals[hdr] += summarytable[item, hdr] - data.append(summarytable[item, hdr]) - table.append([item] + data) # Zip data and headers together in one list for Morris.js charts # will get a list like [(Header1, Data1), (Header2, Data2)...] From 72392a3f500f5f1cc575d3f8f11549a092c3e328 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:44:46 +0200 Subject: [PATCH 36/37] Add `update_summary_tables` helper function Reduces complexity of 'run_report' and handles updating summary table in own function --- helpdesk/views/staff.py | 86 +++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 42 deletions(-) diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 30dc88a1..f31b89f6 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1578,6 +1578,49 @@ def get_report_table_and_totals(header1, summarytable, possible_options): return table, totals +def update_summary_tables(report_queryset, report, summarytable, summarytable2): + metric3 = False + for ticket in report_queryset: + if report == 'userpriority': + metric1 = u'%s' % ticket.get_assigned_to + metric2 = u'%s' % ticket.get_priority_display() + + elif report == 'userqueue': + metric1 = u'%s' % ticket.get_assigned_to + metric2 = u'%s' % ticket.queue.title + + elif report == 'userstatus': + metric1 = u'%s' % ticket.get_assigned_to + metric2 = u'%s' % ticket.get_status_display() + + elif report == 'usermonth': + metric1 = u'%s' % ticket.get_assigned_to + metric2 = u'%s-%s' % (ticket.created.year, ticket.created.month) + + elif report == 'queuepriority': + metric1 = u'%s' % ticket.queue.title + metric2 = u'%s' % ticket.get_priority_display() + + elif report == 'queuestatus': + metric1 = u'%s' % ticket.queue.title + metric2 = u'%s' % ticket.get_status_display() + + elif report == 'queuemonth': + metric1 = u'%s' % ticket.queue.title + metric2 = u'%s-%s' % (ticket.created.year, ticket.created.month) + + elif report == 'daysuntilticketclosedbymonth': + metric1 = u'%s' % ticket.queue.title + metric2 = u'%s-%s' % (ticket.created.year, ticket.created.month) + metric3 = ticket.modified - ticket.created + metric3 = metric3.days + + summarytable[metric1, metric2] += 1 + if metric3: + if report == 'daysuntilticketclosedbymonth': + summarytable2[metric1, metric2] += metric3 + + @helpdesk_staff_member_required def run_report(request, report): @@ -1663,48 +1706,7 @@ def run_report(request, report): col1heading = _('Queue') possible_options = periods charttype = 'date' - - metric3 = False - for ticket in report_queryset: - if report == 'userpriority': - metric1 = u'%s' % ticket.get_assigned_to - metric2 = u'%s' % ticket.get_priority_display() - - elif report == 'userqueue': - metric1 = u'%s' % ticket.get_assigned_to - metric2 = u'%s' % ticket.queue.title - - elif report == 'userstatus': - metric1 = u'%s' % ticket.get_assigned_to - metric2 = u'%s' % ticket.get_status_display() - - elif report == 'usermonth': - metric1 = u'%s' % ticket.get_assigned_to - metric2 = u'%s-%s' % (ticket.created.year, ticket.created.month) - - elif report == 'queuepriority': - metric1 = u'%s' % ticket.queue.title - metric2 = u'%s' % ticket.get_priority_display() - - elif report == 'queuestatus': - metric1 = u'%s' % ticket.queue.title - metric2 = u'%s' % ticket.get_status_display() - - elif report == 'queuemonth': - metric1 = u'%s' % ticket.queue.title - metric2 = u'%s-%s' % (ticket.created.year, ticket.created.month) - - elif report == 'daysuntilticketclosedbymonth': - metric1 = u'%s' % ticket.queue.title - metric2 = u'%s-%s' % (ticket.created.year, ticket.created.month) - metric3 = ticket.modified - ticket.created - metric3 = metric3.days - - summarytable[metric1, metric2] += 1 - if metric3: - if report == 'daysuntilticketclosedbymonth': - summarytable2[metric1, metric2] += metric3 - + update_summary_tables(report_queryset, report, summarytable, summarytable2) if report == 'daysuntilticketclosedbymonth': for key in summarytable2.keys(): summarytable[key] = summarytable2[key] / summarytable[key] From 4a429f5498bdc6669a79ab98719e42e532843552 Mon Sep 17 00:00:00 2001 From: Martin Whitehouse Date: Mon, 25 Jul 2022 04:47:59 +0200 Subject: [PATCH 37/37] Add flake8 exit failure to workflow --- .github/workflows/pythonpackage.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 83ba7e3b..70b2482d 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -28,9 +28,8 @@ jobs: run: | pip install flake8 # stop the build if there are Python syntax errors or undefined names - flake8 helpdesk --count --show-source --statistics --exit-zero # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide - # flake8 . --count --exit-zero --max-complexity=10 --statistics + flake8 helpdesk --count --show-source --statistics --exit-zero --max-complexity=20 - name: Sort style check with 'isort' run: | isort --line-length=120 --src helpdesk . --check