From da3ba66277605a7abb339471e92e9420d3d89c08 Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Fri, 9 May 2025 19:39:42 +0200 Subject: [PATCH 1/9] Add settings entries for suppressable logging messages around email processing. --- docs/configuration.rst | 10 +++++++++- helpdesk/settings.py | 8 ++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index 9cc30d7a..b26d7891 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -94,4 +94,12 @@ You may add your own site specific navigation header to be included inside the < helpdesk/custom_navigation_header.html -2. Update the contents to display your custom navigation. \ No newline at end of file +2. Update the contents to display your custom navigation. + +Suppressable Log Messages +------------------------- +Some logging messages support being switched on or off accroding to deplpoyment preferences. + +The following settings variables control emiting log messages for specific scenarios: + LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER - there is no user matching the email address in the CC list. Defaults to False + LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER - there is more than 1 user matching the email address in the CC list. Defaults to True diff --git a/helpdesk/settings.py b/helpdesk/settings.py index 426cf1dd..179267f3 100644 --- a/helpdesk/settings.py +++ b/helpdesk/settings.py @@ -487,3 +487,11 @@ HELPDESK_GET_NEW_TICKET_WEBHOOK_URLS = getattr( ) HELPDESK_WEBHOOK_TIMEOUT = getattr(settings, "HELPDESK_WEBHOOK_TIMEOUT", 3) + + +LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER = getattr( + settings, "HELPDESK_LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER", False +) +LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER = getattr( + settings, "HELPDESK_LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER", True +) From 9da65f05eb4bea3772ed6702b2711d31cd0055f4 Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Fri, 9 May 2025 19:44:06 +0200 Subject: [PATCH 2/9] Just pass the user ID rather than the entire User object. --- 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 cc2b1ed4..63f9ea9d 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -412,7 +412,7 @@ def view_ticket(request, ticket_id): ] if show_subscribe: - subscribe_to_ticket_updates(ticket, request.user) + subscribe_to_ticket_updates(ticket, request.user.id) return HttpResponseRedirect(reverse("helpdesk:view", args=[ticket.id])) if "close" in request.GET and ticket.status == Ticket.RESOLVED_STATUS: From 1757166638606433998809987d7d5f3c5e8c76ef Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Fri, 9 May 2025 19:45:18 +0200 Subject: [PATCH 3/9] Unit test multiple user IDs associated with the same email address. --- helpdesk/email.py | 35 ++++++++++++++++----- helpdesk/tests/test_ticket_submission.py | 40 ++++++++++++++++++++++-- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index a85e35e0..33f15ca7 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -46,6 +46,7 @@ from typing import List # import User model, which may be a custom model User = get_user_model() +MULTIPLE_USERS_SAME_EMAIL_MSG = "CC email address is linked to more than 1 active user" STRIPPED_SUBJECT_STRINGS = [ "Re: ", "Fw: ", @@ -531,7 +532,7 @@ def is_autoreply(message): return any(any_if_this) -def create_ticket_cc(ticket, cc_list): +def create_ticket_cc(ticket, cc_list, logger): if not cc_list: return [] # Local import to deal with non-defined / circular reference problem @@ -544,16 +545,34 @@ def create_ticket_cc(ticket, cc_list): if cced_email == ticket.queue.email_address: continue - user = None + user_id = None - try: - user = User.objects.get(email=cced_email) # @UndefinedVariable - except User.DoesNotExist: - pass + user_list = User.objects.filter(email=cced_email, is_active=True).values_list( + "id", flat=True + ) + count = user_list.count() + if count == 0: + if getattr( + helpdesk_settings, "LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER", False + ): + logger.warning( + f"CC email address is not linked to an active user: {cced_email}" + ) + elif count > 1: + if getattr( + helpdesk_settings, + "LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER", + True, + ): + logger.warning( + f"{MULTIPLE_USERS_SAME_EMAIL_MSG}: {cced_email}" + ) + else: + user_id = user_list[0] try: ticket_cc = subscribe_to_ticket_updates( - ticket=ticket, user=user, email=cced_email + ticket=ticket, user_id=user_id, email=cced_email ) new_ticket_ccs.append(ticket_cc) except ValidationError: @@ -673,7 +692,7 @@ def create_object_from_email_message(message, ticket_id, payload, files, logger) context = safe_template_context(ticket) new_ticket_ccs = [] - new_ticket_ccs.append(create_ticket_cc(ticket, to_list + cc_list)) + new_ticket_ccs.append(create_ticket_cc(ticket, to_list + cc_list, logger)) autoreply = is_autoreply(message) if autoreply: diff --git a/helpdesk/tests/test_ticket_submission.py b/helpdesk/tests/test_ticket_submission.py index bbe37e67..10951844 100644 --- a/helpdesk/tests/test_ticket_submission.py +++ b/helpdesk/tests/test_ticket_submission.py @@ -4,7 +4,7 @@ from django.test import TestCase from django.test.client import Client from django.urls import reverse import email -from helpdesk.email import extract_email_metadata +from helpdesk.email import extract_email_metadata, MULTIPLE_USERS_SAME_EMAIL_MSG from helpdesk.models import ( CustomField, FollowUp, @@ -14,10 +14,13 @@ from helpdesk.models import ( Ticket, TicketCC, ) +from . import utils import logging from urllib.parse import urlparse import uuid +import mock +User = get_user_model() logger = logging.getLogger("helpdesk") @@ -46,7 +49,7 @@ class TicketBasicsTestCase(TestCase): "description": "Some Test Ticket", } - self.user = get_user_model().objects.create( + self.user = User.objects.create( username="User_1", ) @@ -480,6 +483,39 @@ class EmailInteractionsTestCase(TestCase): self.assertTrue(ticket_cc.ticket, ticket) self.assertTrue(ticket_cc.email, cc_email) + def test_create_ticket_from_email_with_cc_duplicate_users_same_email(self): + """ + Verify that the duplicated email warning log is invoked + """ + dup_email = "dup@helpdesk.test" + user_dup1 = User.objects.create( + username="User_dup1", email=dup_email, + ) + user_dup2 = User.objects.create( + username="User_dup2", email=dup_email, + ) + message, _, _ = utils.generate_text_email(locale="fr_FR") + + submitter_email = "foo@bar.py" + cc_list = [dup_email, "alpha@acme.test", "beta@acme.test", "gamma@delta.test"] + message_id = uuid.uuid4().hex + + message["Message-ID"] = message_id + message["Cc"] = ",".join(cc_list) + + with self.assertLogs('helpdesk', level='WARNING') as dup_warn: + extract_email_metadata(str(message), self.queue_public, logger=logger) + self.assertTrue(MULTIPLE_USERS_SAME_EMAIL_MSG in dup_warn.output[0], "The duplicated email across user ID's was was not logged.") + # Check that the CC duplicate was still sent a notification email + email_count = len(mail.outbox) + found = False + for i in range(0, email_count-1): + if dup_email in mail.outbox[i].to: + found = True + break + self.assertTrue(found, "The duplicated email across user ID's was not sent a n otification.") + + def test_create_followup_from_email_with_valid_message_id_with_no_initial_cc_list( self, ): From 2aedf7b3b0533db18acbd83971027417cfcdf5ed Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Fri, 9 May 2025 19:47:16 +0200 Subject: [PATCH 4/9] Just pass the User ID isntead of the entire User object to subscribe_to_ticket_updates --- helpdesk/update_ticket.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/helpdesk/update_ticket.py b/helpdesk/update_ticket.py index ed4fabdb..4941386a 100644 --- a/helpdesk/update_ticket.py +++ b/helpdesk/update_ticket.py @@ -29,7 +29,7 @@ def add_staff_subscription(user: User, ticket: Ticket) -> None: and user.is_authenticated and return_ticketccstring_and_show_subscribe(user, ticket)[1] ): - subscribe_to_ticket_updates(ticket, user) + subscribe_to_ticket_updates(ticket, user.id) def return_ticketccstring_and_show_subscribe(user, ticket): @@ -73,16 +73,16 @@ def return_ticketccstring_and_show_subscribe(user, ticket): def subscribe_to_ticket_updates( - ticket, user=None, email=None, can_view=True, can_update=False + ticket, user_id=None, email=None, can_view=True, can_update=False ): if ticket is not None: - queryset = TicketCC.objects.filter(ticket=ticket, user=user, email=email) + queryset = TicketCC.objects.filter(ticket=ticket, user_id=user_id, email=email) # Don't create duplicate entries for subscribers if queryset.count() > 0: return queryset.first() - if user is None and len(email) < 5: + if user_id is None and len(email) < 5: raise ValidationError( _( "When you add somebody on Cc, you must provide either a User or a valid email. Email: %s" @@ -91,7 +91,7 @@ def subscribe_to_ticket_updates( ) return ticket.ticketcc_set.create( - user=user, email=email, can_view=can_view, can_update=can_update + user_id=user_id, email=email, can_view=can_view, can_update=can_update ) From c713e3ea16a93a61f0d8272dcac88c78258c33fd Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Fri, 9 May 2025 20:02:57 +0200 Subject: [PATCH 5/9] Formatting fixes --- helpdesk/email.py | 4 +--- helpdesk/tests/test_ticket_submission.py | 26 ++++++++++++++---------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/helpdesk/email.py b/helpdesk/email.py index 33f15ca7..2af33f64 100644 --- a/helpdesk/email.py +++ b/helpdesk/email.py @@ -564,9 +564,7 @@ def create_ticket_cc(ticket, cc_list, logger): "LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER", True, ): - logger.warning( - f"{MULTIPLE_USERS_SAME_EMAIL_MSG}: {cced_email}" - ) + logger.warning(f"{MULTIPLE_USERS_SAME_EMAIL_MSG}: {cced_email}") else: user_id = user_list[0] diff --git a/helpdesk/tests/test_ticket_submission.py b/helpdesk/tests/test_ticket_submission.py index 10951844..e2e5b3a2 100644 --- a/helpdesk/tests/test_ticket_submission.py +++ b/helpdesk/tests/test_ticket_submission.py @@ -18,7 +18,6 @@ from . import utils import logging from urllib.parse import urlparse import uuid -import mock User = get_user_model() @@ -488,33 +487,38 @@ class EmailInteractionsTestCase(TestCase): Verify that the duplicated email warning log is invoked """ dup_email = "dup@helpdesk.test" - user_dup1 = User.objects.create( - username="User_dup1", email=dup_email, + User.objects.create( + username="User_dup1", + email=dup_email, ) - user_dup2 = User.objects.create( - username="User_dup2", email=dup_email, + User.objects.create( + username="User_dup2", + email=dup_email, ) message, _, _ = utils.generate_text_email(locale="fr_FR") - submitter_email = "foo@bar.py" cc_list = [dup_email, "alpha@acme.test", "beta@acme.test", "gamma@delta.test"] message_id = uuid.uuid4().hex message["Message-ID"] = message_id message["Cc"] = ",".join(cc_list) - with self.assertLogs('helpdesk', level='WARNING') as dup_warn: + with self.assertLogs("helpdesk", level="WARNING") as dup_warn: extract_email_metadata(str(message), self.queue_public, logger=logger) - self.assertTrue(MULTIPLE_USERS_SAME_EMAIL_MSG in dup_warn.output[0], "The duplicated email across user ID's was was not logged.") + self.assertTrue( + MULTIPLE_USERS_SAME_EMAIL_MSG in dup_warn.output[0], + "The duplicated email across user ID's was was not logged.", + ) # Check that the CC duplicate was still sent a notification email email_count = len(mail.outbox) found = False - for i in range(0, email_count-1): + for i in range(0, email_count - 1): if dup_email in mail.outbox[i].to: found = True break - self.assertTrue(found, "The duplicated email across user ID's was not sent a n otification.") - + self.assertTrue( + found, "The duplicated email across user ID's was not sent a n otification." + ) def test_create_followup_from_email_with_valid_message_id_with_no_initial_cc_list( self, From a3c563e1045407bd72b9c4ac3925011749c8bfa9 Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Sat, 10 May 2025 13:16:35 +0100 Subject: [PATCH 6/9] Optimise check --- helpdesk/tests/test_ticket_submission.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/helpdesk/tests/test_ticket_submission.py b/helpdesk/tests/test_ticket_submission.py index e2e5b3a2..e86691b1 100644 --- a/helpdesk/tests/test_ticket_submission.py +++ b/helpdesk/tests/test_ticket_submission.py @@ -511,13 +511,9 @@ class EmailInteractionsTestCase(TestCase): ) # Check that the CC duplicate was still sent a notification email email_count = len(mail.outbox) - found = False - for i in range(0, email_count - 1): - if dup_email in mail.outbox[i].to: - found = True - break + found = any(dup_email in email.to for email in mail.outbox[:email_count - 1]) self.assertTrue( - found, "The duplicated email across user ID's was not sent a n otification." + found, "The duplicated email across user ID's was not sent a notification." ) def test_create_followup_from_email_with_valid_message_id_with_no_initial_cc_list( From ae381bd2a6524df15ccb4e89ccd19fd0267b542b Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Sat, 10 May 2025 13:16:54 +0100 Subject: [PATCH 7/9] Fix spelling errors --- docs/configuration.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/configuration.rst b/docs/configuration.rst index b26d7891..bbe99e38 100644 --- a/docs/configuration.rst +++ b/docs/configuration.rst @@ -96,10 +96,10 @@ You may add your own site specific navigation header to be included inside the < 2. Update the contents to display your custom navigation. -Suppressable Log Messages +Suppressible Log Messages ------------------------- -Some logging messages support being switched on or off accroding to deplpoyment preferences. +Some logging messages support being switched on or off according to deployment preferences. -The following settings variables control emiting log messages for specific scenarios: +The following settings variables control emitting log messages for specific scenarios: LOG_WARN_WHEN_CC_EMAIL_NOT_LINKED_TO_A_USER - there is no user matching the email address in the CC list. Defaults to False LOG_WARN_WHEN_CC_EMAIL_LINKED_TO_MORE_THAN_1_USER - there is more than 1 user matching the email address in the CC list. Defaults to True From 7953be751eb6e3dca25f895c7189b456547ac61d Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Sat, 10 May 2025 13:18:10 +0100 Subject: [PATCH 8/9] Fix linting error --- helpdesk/tests/test_ticket_submission.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helpdesk/tests/test_ticket_submission.py b/helpdesk/tests/test_ticket_submission.py index e86691b1..0b29e8ac 100644 --- a/helpdesk/tests/test_ticket_submission.py +++ b/helpdesk/tests/test_ticket_submission.py @@ -511,7 +511,7 @@ class EmailInteractionsTestCase(TestCase): ) # Check that the CC duplicate was still sent a notification email email_count = len(mail.outbox) - found = any(dup_email in email.to for email in mail.outbox[:email_count - 1]) + found = any(dup_email in email.to for email in mail.outbox[: email_count - 1]) self.assertTrue( found, "The duplicated email across user ID's was not sent a notification." ) From af91113995087fbc880dc6f373135ec3147ceb75 Mon Sep 17 00:00:00 2001 From: Christopher Broderick Date: Mon, 12 May 2025 17:23:57 +0100 Subject: [PATCH 9/9] Preset new version in anticipation of new release. --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index d8ebe85f..3cbc10b9 100644 --- a/setup.py +++ b/setup.py @@ -7,7 +7,7 @@ from setuptools import find_packages, setup import sys -version = "1.4.0" +version = "1.5.0" # Provided as an attribute, so you can append to these instead