From d09c35a881d4e52201aecabb8954f82abbeedbc5 Mon Sep 17 00:00:00 2001 From: Timothy Hobbs Date: Tue, 14 Nov 2023 19:44:43 +0100 Subject: [PATCH 1/4] Show stack traces in stdout on standalone helpdesk --- standalone/config/settings.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/standalone/config/settings.py b/standalone/config/settings.py index 2a6340f5..237be0c5 100644 --- a/standalone/config/settings.py +++ b/standalone/config/settings.py @@ -229,3 +229,20 @@ MEDIA_ROOT = '/data/media' # for Django 3.2+, set default for autofields: DEFAULT_AUTO_FIELD = 'django.db.models.AutoField' + +LOGGING = { + 'version': 1, + 'disable_existing_loggers': False, + 'handlers': { + 'console': { + 'class': 'logging.StreamHandler', + }, + }, + 'loggers': { + 'django': { + 'handlers': ['console'], + 'level': 'ERROR', # Change to 'DEBUG' if you want to print all debug messages as well + 'propagate': True, + }, + }, +} From 6cd5522099476db9061f9672d0cec3085de50f0c Mon Sep 17 00:00:00 2001 From: Timothy Hobbs Date: Tue, 14 Nov 2023 20:07:25 +0100 Subject: [PATCH 2/4] Move to ruff, its faster & catches more --- .github/workflows/pythonpackage.yml | 17 +++-------------- .../migrations/0002_populate_usersettings.py | 2 +- helpdesk/migrations/0003_initial_data_import.py | 3 +-- .../migrations/0010_remove_queuemembership.py | 2 +- helpdesk/tests/helpers.py | 2 +- helpdesk/tests/test_kb.py | 2 +- helpdesk/tests/test_navigation.py | 5 +---- helpdesk/tests/test_query.py | 2 +- helpdesk/tests/test_time_spent.py | 13 ------------- helpdesk/tests/test_usersettings.py | 9 --------- helpdesk/validators.py | 2 +- helpdesk/views/staff.py | 2 +- pyproject.toml | 2 ++ requirements-dev.txt | 3 +-- 14 files changed, 15 insertions(+), 51 deletions(-) create mode 100644 pyproject.toml diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 9e880b53..45ae1727 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -28,21 +28,10 @@ jobs: python -m pip install --upgrade pip pip install -r requirements.txt -r requirements-testing.txt -c constraints-Django${{ matrix.django-version }}.txt - - name: Format style check with 'autopep8' + - name: Lint with ruff run: | - pip install autopep8 - autopep8 --exit-code --global-config .flake8 helpdesk - - - name: Lint with 'flake8' - run: | - pip install flake8 - # stop the build if there are Python syntax errors or undefined names - # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide - 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 + pip install ruff + ruff helpdesk - name: Test with pytest run: | diff --git a/helpdesk/migrations/0002_populate_usersettings.py b/helpdesk/migrations/0002_populate_usersettings.py index b35bfc80..42c94cf5 100644 --- a/helpdesk/migrations/0002_populate_usersettings.py +++ b/helpdesk/migrations/0002_populate_usersettings.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- from django.contrib.auth import get_user_model -from django.db import models, migrations +from django.db import migrations from helpdesk.settings import DEFAULT_USER_SETTINGS diff --git a/helpdesk/migrations/0003_initial_data_import.py b/helpdesk/migrations/0003_initial_data_import.py index c13bb47c..b3fcbb43 100644 --- a/helpdesk/migrations/0003_initial_data_import.py +++ b/helpdesk/migrations/0003_initial_data_import.py @@ -1,8 +1,7 @@ # -*- coding: utf-8 -*- import os -from sys import path -from django.db import models, migrations +from django.db import migrations from django.core import serializers fixture_dir = os.path.abspath(os.path.join(os.path.dirname(__file__), '../fixtures')) diff --git a/helpdesk/migrations/0010_remove_queuemembership.py b/helpdesk/migrations/0010_remove_queuemembership.py index b0df7ee4..98775daa 100644 --- a/helpdesk/migrations/0010_remove_queuemembership.py +++ b/helpdesk/migrations/0010_remove_queuemembership.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -from django.db import models, migrations +from django.db import migrations class Migration(migrations.Migration): diff --git a/helpdesk/tests/helpers.py b/helpdesk/tests/helpers.py index fe4d65de..e91e7d85 100644 --- a/helpdesk/tests/helpers.py +++ b/helpdesk/tests/helpers.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- from django.contrib.auth import get_user_model -from helpdesk.models import Queue, Ticket, UserSettings +from helpdesk.models import Queue, Ticket import sys diff --git a/helpdesk/tests/test_kb.py b/helpdesk/tests/test_kb.py index 511aae67..23525586 100644 --- a/helpdesk/tests/test_kb.py +++ b/helpdesk/tests/test_kb.py @@ -2,7 +2,7 @@ from django.test import TestCase from django.urls import reverse from helpdesk.models import KBCategory, KBItem, Queue, Ticket -from helpdesk.tests.helpers import create_ticket, get_staff_user, print_response, reload_urlconf, User +from helpdesk.tests.helpers import get_staff_user class KBTests(TestCase): diff --git a/helpdesk/tests/test_navigation.py b/helpdesk/tests/test_navigation.py index fef74fce..b4a427ea 100644 --- a/helpdesk/tests/test_navigation.py +++ b/helpdesk/tests/test_navigation.py @@ -6,7 +6,7 @@ from django.test.utils import override_settings from django.urls import reverse from helpdesk import settings as helpdesk_settings from helpdesk.models import Queue -from helpdesk.tests.helpers import create_ticket, get_staff_user, print_response, reload_urlconf, User +from helpdesk.tests.helpers import create_ticket, get_staff_user, reload_urlconf, User from importlib import reload import sys @@ -109,7 +109,6 @@ class StaffUsersOnlyTestCase(StaffUserTestCaseMixin, TestCase): """When HELPDESK_ALLOW_NON_STAFF_TICKET_UPDATE is False, staff users should be able to access the dashboard. """ - from helpdesk.decorators import is_helpdesk_staff user = get_staff_user() self.client.login(username=user.username, password='password') @@ -120,8 +119,6 @@ class StaffUsersOnlyTestCase(StaffUserTestCaseMixin, TestCase): """When HELPDESK_ALLOW_NON_STAFF_TICKET_UPDATE is False, non-staff users should not be able to access the dashboard. """ - from helpdesk.decorators import is_helpdesk_staff - user = self.non_staff_user self.client.login(username=user.username, password=self.non_staff_user_password) diff --git a/helpdesk/tests/test_query.py b/helpdesk/tests/test_query.py index 8e2c1533..bee5b664 100644 --- a/helpdesk/tests/test_query.py +++ b/helpdesk/tests/test_query.py @@ -3,7 +3,7 @@ from django.test import TestCase from django.urls import reverse from helpdesk.models import KBCategory, KBItem, Queue, Ticket from helpdesk.query import query_to_base64 -from helpdesk.tests.helpers import create_ticket, get_staff_user, print_response, reload_urlconf, User +from helpdesk.tests.helpers import get_staff_user class QueryTests(TestCase): diff --git a/helpdesk/tests/test_time_spent.py b/helpdesk/tests/test_time_spent.py index f89214e3..83027c56 100644 --- a/helpdesk/tests/test_time_spent.py +++ b/helpdesk/tests/test_time_spent.py @@ -1,25 +1,12 @@ import datetime -from django.contrib.auth import get_user_model from django.contrib.auth.hashers import make_password from django.contrib.auth.models import User -from django.contrib.sites.models import Site -from django.core import mail from django.test import TestCase from django.test.client import Client -from django.urls import reverse -from helpdesk import settings as helpdesk_settings from helpdesk.models import FollowUp, Queue, Ticket -from helpdesk.templatetags.ticket_to_link import num_to_link import uuid - -try: # python 3 - from urllib.parse import urlparse -except ImportError: # python 2 - from urlparse import urlparse - - class TimeSpentTestCase(TestCase): def setUp(self): diff --git a/helpdesk/tests/test_usersettings.py b/helpdesk/tests/test_usersettings.py index b7d72866..7ba26c96 100644 --- a/helpdesk/tests/test_usersettings.py +++ b/helpdesk/tests/test_usersettings.py @@ -1,15 +1,6 @@ from django.contrib.auth import get_user_model -from django.core import mail from django.test import TestCase -from django.test.client import Client from django.urls import reverse -from helpdesk.models import CustomField, Queue, Ticket - - -try: # python 3 - from urllib.parse import urlparse -except ImportError: # python 2 - from urlparse import urlparse class TicketActionsTestCase(TestCase): diff --git a/helpdesk/validators.py b/helpdesk/validators.py index 5fb80e39..72589111 100644 --- a/helpdesk/validators.py +++ b/helpdesk/validators.py @@ -28,7 +28,7 @@ def validate_file_extension(value): valid_extensions = ['.txt', '.asc', '.htm', '.html', '.pdf', '.doc', '.docx', '.odt', '.jpg', '.png', '.eml'] - if not ext.lower() in valid_extensions: + if ext.lower() not in valid_extensions: # TODO: one more check in case it is a file with no extension; we # should always allow that? if not (ext.lower() == '' or ext.lower() == '.'): diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index d25dac0c..9b8565ac 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -1340,7 +1340,7 @@ def ticket_list(request): ('kbitem', 'kbitem__isnull'), ]) for param, filter_command in filter_in_params: - if not request.GET.get(param) is None: + if request.GET.get(param) is not None: patterns = request.GET.getlist(param) try: pattern_pks = [int(pattern) for pattern in patterns] diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 00000000..211ea9fe --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,2 @@ +[tool.ruff] +ignore = ["E501", "E731", "F841", "E721"] diff --git a/requirements-dev.txt b/requirements-dev.txt index b26ca6de..973bf2e3 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -1,6 +1,5 @@ tox -autopep8 -flake8 +ruff pycodestyle isort freezegun From ade4c3115e134ff78b449afc49c2fd5fe4940ac2 Mon Sep 17 00:00:00 2001 From: Timothy Hobbs Date: Tue, 14 Nov 2023 21:17:37 +0100 Subject: [PATCH 3/4] Move update ticket logic out of staff.py --- helpdesk/update_ticket.py | 385 ++++++++++++++++++++++++++++++++++++++ helpdesk/urls.py | 2 +- helpdesk/views/public.py | 4 +- helpdesk/views/staff.py | 374 +++--------------------------------- 4 files changed, 411 insertions(+), 354 deletions(-) create mode 100644 helpdesk/update_ticket.py diff --git a/helpdesk/update_ticket.py b/helpdesk/update_ticket.py new file mode 100644 index 00000000..0a3f2c85 --- /dev/null +++ b/helpdesk/update_ticket.py @@ -0,0 +1,385 @@ +import typing + +from django.core.exceptions import ValidationError +from django.contrib.auth import get_user_model +from django.utils import timezone +from django.utils.translation import gettext as _ + +from helpdesk.lib import safe_template_context +from helpdesk import settings as helpdesk_settings +from helpdesk.lib import process_attachments +from helpdesk.decorators import ( + is_helpdesk_staff, +) +from helpdesk.models import ( + FollowUp, + Ticket, + TicketCC, + TicketChange, +) + +User = get_user_model() + +def add_staff_subscription( + user: User, + 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 user.is_authenticated \ + and return_ticketccstring_and_show_subscribe(user, ticket)[1]: + subscribe_to_ticket_updates(ticket, user) + + +def return_ticketccstring_and_show_subscribe(user, ticket): + """used in view_ticket() and followup_edit()""" + # create the ticketcc_string and check whether current user is already + # subscribed + username = user.get_username().upper() + try: + useremail = user.email.upper() + except AttributeError: + useremail = "" + strings_to_check = list() + strings_to_check.append(username) + strings_to_check.append(useremail) + + ticketcc_string = '' + all_ticketcc = ticket.ticketcc_set.all() + counter_all_ticketcc = len(all_ticketcc) - 1 + show_subscribe = True + for i, ticketcc in enumerate(all_ticketcc): + ticketcc_this_entry = str(ticketcc.display) + ticketcc_string += ticketcc_this_entry + if i < counter_all_ticketcc: + ticketcc_string += ', ' + if strings_to_check.__contains__(ticketcc_this_entry.upper()): + show_subscribe = False + + # check whether current user is a submitter or assigned to ticket + assignedto_username = str(ticket.assigned_to).upper() + strings_to_check = list() + if ticket.submitter_email is not None: + submitter_email = ticket.submitter_email.upper() + strings_to_check.append(submitter_email) + strings_to_check.append(assignedto_username) + if strings_to_check.__contains__(username) or strings_to_check.__contains__(useremail): + show_subscribe = False + + return ticketcc_string, show_subscribe + + +def subscribe_to_ticket_updates(ticket, user=None, email=None, can_view=True, can_update=False): + + if ticket is not None: + + queryset = TicketCC.objects.filter( + ticket=ticket, user=user, email=email) + + # Don't create duplicate entries for subscribers + if queryset.count() > 0: + return queryset.first() + + if user 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' % email) + ) + + return ticket.ticketcc_set.create( + user=user, + email=email, + can_view=can_view, + can_update=can_update + ) + + +def get_and_set_ticket_status( + new_status: int, + ticket: Ticket, + follow_up: FollowUp +) -> typing.Tuple[str, int]: + """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_messages_sent_to_by_public_and_status( + public: bool, + ticket: Ticket, + follow_up: FollowUp, + context: str, + messages_sent_to: typing.Set[str], + files: typing.List[typing.Tuple[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 get_template_staff_and_template_cc( + reassigned, follow_up: FollowUp +) -> typing.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( + user, + ticket, + title=None, + comment=None, + files=None, + public=False, + owner=-1, + priority=-1, + new_status=None, + time_spent=None, + due_date=None, + new_checklists=None, +): + # We need to allow the 'ticket' and 'queue' contexts to be applied to the + # comment. + context = safe_template_context(ticket) + if priority == -1: + priority = ticket.priority + if new_status is None: + new_status = ticket.status + if new_checklists is None: + new_checklists = {} + + from django.template import engines + template_func = engines['django'].from_string + # this prevents system from trying to render any template tags + # broken into two stages to prevent changes from first replace being themselves + # changed by the second replace due to conflicting syntax + comment = comment.replace( + '{%', 'X-HELPDESK-COMMENT-VERBATIM').replace('%}', 'X-HELPDESK-COMMENT-ENDVERBATIM') + comment = comment.replace( + 'X-HELPDESK-COMMENT-VERBATIM', '{% verbatim %}{%' + ).replace( + 'X-HELPDESK-COMMENT-ENDVERBATIM', '%}{% endverbatim %}' + ) + # render the neutralized template + comment = template_func(comment).render(context) + + if owner == -1 and ticket.assigned_to: + owner = ticket.assigned_to.id + + f = FollowUp(ticket=ticket, date=timezone.now(), comment=comment, + time_spent=time_spent) + + if is_helpdesk_staff(user): + f.user = user + + f.public = public + + reassigned = False + + old_owner = ticket.assigned_to + if owner != -1: + if owner != 0 and ((ticket.assigned_to and owner != ticket.assigned_to.id) or not ticket.assigned_to): + new_user = User.objects.get(id=owner) + f.title = _('Assigned to %(username)s') % { + 'username': new_user.get_username(), + } + ticket.assigned_to = new_user + reassigned = True + # user changed owner to 'unassign' + elif owner == 0 and ticket.assigned_to is not None: + f.title = _('Unassigned') + ticket.assigned_to = None + + old_status_str, old_status = get_and_set_ticket_status(new_status, ticket, f) + + files = process_attachments(f, files) if files else [] + + if title and title != ticket.title: + c = TicketChange( + followup=f, + field=_('Title'), + old_value=ticket.title, + new_value=title, + ) + c.save() + ticket.title = title + + if new_status != old_status: + c = TicketChange( + followup=f, + field=_('Status'), + old_value=old_status_str, + new_value=ticket.get_status_display(), + ) + c.save() + + if ticket.assigned_to != old_owner: + c = TicketChange( + followup=f, + field=_('Owner'), + old_value=old_owner, + new_value=ticket.assigned_to, + ) + c.save() + + if priority != ticket.priority: + c = TicketChange( + followup=f, + field=_('Priority'), + old_value=ticket.priority, + new_value=priority, + ) + c.save() + ticket.priority = priority + + if due_date != ticket.due_date: + c = TicketChange( + followup=f, + field=_('Due on'), + old_value=ticket.due_date, + new_value=due_date, + ) + c.save() + ticket.due_date = due_date + + for checklist in ticket.checklists.all(): + new_completed_tasks = new_checklists[checklist.id] + for task in checklist.tasks.all(): + changed = None + + # Add completion if it was not done yet + if not task.completion_date and task.id in new_completed_tasks: + task.completion_date = timezone.now() + changed = 'completed' + # Remove it if it was done before + elif task.completion_date and task.id not in new_completed_tasks: + task.completion_date = None + changed = 'uncompleted' + + # Save and add ticket change if task state has changed + if changed: + task.save(update_fields=['completion_date']) + f.ticketchange_set.create( + field=f'[{checklist.name}] {task.description}', + old_value=_('To do') if changed == 'completed' else _('Completed'), + new_value=_('Completed') if changed == 'completed' else _('To do'), + ) + + 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. + context = safe_template_context(ticket) + context.update( + resolution=ticket.resolution, + comment=f.comment, + ) + + messages_sent_to = set() + try: + messages_sent_to.add(user.email) + except AttributeError: + pass + ticket = update_messages_sent_to_by_public_and_status( + public, + ticket, + f, + context, + messages_sent_to, + files + ) + + 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) + ): + messages_sent_to.update(ticket.send( + {'assigned_to': (template_staff, context)}, + dont_send_to=messages_sent_to, + fail_silently=True, + files=files, + )) + + messages_sent_to.update(ticket.send( + {'ticket_cc': (template_cc, context)}, + dont_send_to=messages_sent_to, + fail_silently=True, + files=files, + )) + ticket.save() + + # auto subscribe user if enabled + add_staff_subscription(user, ticket) + diff --git a/helpdesk/urls.py b/helpdesk/urls.py index 6f7587d3..eb538cfd 100644 --- a/helpdesk/urls.py +++ b/helpdesk/urls.py @@ -64,7 +64,7 @@ urlpatterns = [ ), path("tickets//edit/", staff.edit_ticket, name="edit"), path("tickets//update/", - staff.update_ticket, name="update"), + staff.update_ticket_view, name="update"), path("tickets//delete/", staff.delete_ticket, name="delete"), path("tickets//hold/", staff.hold_ticket, name="hold"), diff --git a/helpdesk/views/public.py b/helpdesk/views/public.py index dfe42e53..f2eca65f 100644 --- a/helpdesk/views/public.py +++ b/helpdesk/views/public.py @@ -210,7 +210,7 @@ def view_ticket(request): return HttpResponseRedirect(redirect_url) if 'close' in request.GET and ticket.status == Ticket.RESOLVED_STATUS: - from helpdesk.views.staff import update_ticket + from helpdesk.views.staff import update_ticket_view # Trick the update_ticket() view into thinking it's being called with # a valid POST. @@ -224,7 +224,7 @@ def view_ticket(request): request.POST['owner'] = ticket.assigned_to.id request.GET = {} - return update_ticket(request, ticket_id, public=True) + return update_ticket_view(request, ticket_id, public=True) # redirect user back to this ticket if possible. redirect_url = '' diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index 9b8565ac..f1ca91e6 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -17,7 +17,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.decorators import user_passes_test from django.contrib.auth.views import redirect_to_login from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import PermissionDenied, ValidationError +from django.core.exceptions import PermissionDenied from django.core.handlers.wsgi import WSGIRequest from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator from django.db.models import Q @@ -56,7 +56,7 @@ from helpdesk.forms import ( TicketForm, UserSettingsForm ) -from helpdesk.lib import process_attachments, queue_template_context, safe_template_context +from helpdesk.lib import queue_template_context, safe_template_context from helpdesk.models import ( Checklist, ChecklistTask, @@ -70,13 +70,13 @@ from helpdesk.models import ( SavedSearch, Ticket, TicketCC, - TicketChange, TicketCustomFieldValue, TicketDependency, UserSettings ) from helpdesk.query import get_query_class, query_from_base64, query_to_base64 from helpdesk.user import HelpdeskUser +from helpdesk.update_ticket import update_ticket, subscribe_to_ticket_updates, return_ticketccstring_and_show_subscribe import helpdesk.views.abstract_views as abstract_views from helpdesk.views.permissions import MustBeStaffMixin import json @@ -357,7 +357,7 @@ def view_ticket(request, ticket_id): 'title': ticket.title, 'comment': '' } - return update_ticket(request, ticket_id) + return update_ticket_view(request, ticket_id) if 'subscribe' in request.GET: # Allow the user to subscribe him/herself to the ticket whilst viewing @@ -386,7 +386,7 @@ def view_ticket(request, ticket_id): 'comment': _('Accepted resolution and closed ticket'), } - return update_ticket(request, ticket_id) + return update_ticket_view(request, ticket_id) if helpdesk_settings.HELPDESK_STAFF_ONLY_TICKET_OWNERS: users = User.objects.filter( @@ -491,68 +491,6 @@ def delete_ticket_checklist(request, ticket_id, checklist_id): }) -def return_ticketccstring_and_show_subscribe(user, ticket): - """used in view_ticket() and followup_edit()""" - # create the ticketcc_string and check whether current user is already - # subscribed - username = user.get_username().upper() - try: - useremail = user.email.upper() - except AttributeError: - useremail = "" - strings_to_check = list() - strings_to_check.append(username) - strings_to_check.append(useremail) - - ticketcc_string = '' - all_ticketcc = ticket.ticketcc_set.all() - counter_all_ticketcc = len(all_ticketcc) - 1 - show_subscribe = True - for i, ticketcc in enumerate(all_ticketcc): - ticketcc_this_entry = str(ticketcc.display) - ticketcc_string += ticketcc_this_entry - if i < counter_all_ticketcc: - ticketcc_string += ', ' - if strings_to_check.__contains__(ticketcc_this_entry.upper()): - show_subscribe = False - - # check whether current user is a submitter or assigned to ticket - assignedto_username = str(ticket.assigned_to).upper() - strings_to_check = list() - if ticket.submitter_email is not None: - submitter_email = ticket.submitter_email.upper() - strings_to_check.append(submitter_email) - strings_to_check.append(assignedto_username) - if strings_to_check.__contains__(username) or strings_to_check.__contains__(useremail): - show_subscribe = False - - return ticketcc_string, show_subscribe - - -def subscribe_to_ticket_updates(ticket, user=None, email=None, can_view=True, can_update=False): - - if ticket is not None: - - queryset = TicketCC.objects.filter( - ticket=ticket, user=user, email=email) - - # Don't create duplicate entries for subscribers - if queryset.count() > 0: - return queryset.first() - - if user 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' % email) - ) - - return ticket.ticketcc_set.create( - user=user, - email=email, - can_view=can_view, - can_update=can_update - ) - - def get_ticket_from_request_with_authorisation( request: WSGIRequest, ticket_id: str, @@ -614,38 +552,6 @@ def get_due_date_from_request_or_ticket( return due_date -def get_and_set_ticket_status( - new_status: int, - ticket: Ticket, - follow_up: FollowUp -) -> typing.Tuple[str, int]: - """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 get_time_spent_from_request(request: WSGIRequest) -> typing.Optional[timedelta]: if request.POST.get("time_spent"): (hours, minutes) = [int(f) @@ -654,99 +560,24 @@ 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: typing.Set[str], - files: typing.List[typing.Tuple[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 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 \ - and return_ticketccstring_and_show_subscribe(request.user, ticket)[1]: - subscribe_to_ticket_updates(ticket, request.user) - - -def get_template_staff_and_template_cc( - reassigned, follow_up: FollowUp -) -> typing.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): +def update_ticket_view(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)) # Check if a change happened on checklists + new_checklists = {} changes_in_checklists = False for checklist in ticket.checklists.all(): - old_completed_id = sorted(list(checklist.tasks.completed().values_list('id', flat=True))) - new_completed_id = sorted(list(map(int, request.POST.getlist(f'checklist-{checklist.id}', [])))) - if old_completed_id != new_completed_id: + old_completed = set(checklist.tasks.completed().values_list('id', flat=True)) + new_checklist = set(map(int, request.POST.getlist(f'checklist-{checklist.id}', []))) + new_checklists[checklist.id] = new_checklist + if new_checklist != old_completed: changes_in_checklists = True time_spent = get_time_spent_from_request(request) @@ -768,180 +599,21 @@ def update_ticket(request, ticket_id, public=False): if no_changes: return return_to_ticket(request.user, helpdesk_settings, ticket) - # We need to allow the 'ticket' and 'queue' contexts to be applied to the - # comment. - context = safe_template_context(ticket) - - from django.template import engines - template_func = engines['django'].from_string - # this prevents system from trying to render any template tags - # broken into two stages to prevent changes from first replace being themselves - # changed by the second replace due to conflicting syntax - comment = comment.replace( - '{%', 'X-HELPDESK-COMMENT-VERBATIM').replace('%}', 'X-HELPDESK-COMMENT-ENDVERBATIM') - comment = comment.replace( - 'X-HELPDESK-COMMENT-VERBATIM', '{% verbatim %}{%' - ).replace( - 'X-HELPDESK-COMMENT-ENDVERBATIM', '%}{% endverbatim %}' - ) - # render the neutralized template - comment = template_func(comment).render(context) - - if owner == -1 and ticket.assigned_to: - owner = ticket.assigned_to.id - - f = FollowUp(ticket=ticket, date=timezone.now(), comment=comment, - time_spent=time_spent) - - if is_helpdesk_staff(request.user): - f.user = request.user - - f.public = public - - reassigned = False - - old_owner = ticket.assigned_to - if owner != -1: - if owner != 0 and ((ticket.assigned_to and owner != ticket.assigned_to.id) or not ticket.assigned_to): - new_user = User.objects.get(id=owner) - f.title = _('Assigned to %(username)s') % { - 'username': new_user.get_username(), - } - ticket.assigned_to = new_user - reassigned = True - # user changed owner to 'unassign' - elif owner == 0 and ticket.assigned_to is not None: - f.title = _('Unassigned') - ticket.assigned_to = None - - old_status_str, old_status = get_and_set_ticket_status(new_status, ticket, f) - - files = process_attachments(f, request.FILES.getlist('attachment')) if request.FILES else [] - - if title and title != ticket.title: - c = TicketChange( - followup=f, - field=_('Title'), - old_value=ticket.title, - new_value=title, - ) - c.save() - ticket.title = title - - if new_status != old_status: - c = TicketChange( - followup=f, - field=_('Status'), - old_value=old_status_str, - new_value=ticket.get_status_display(), - ) - c.save() - - if ticket.assigned_to != old_owner: - c = TicketChange( - followup=f, - field=_('Owner'), - old_value=old_owner, - new_value=ticket.assigned_to, - ) - c.save() - - if priority != ticket.priority: - c = TicketChange( - followup=f, - field=_('Priority'), - old_value=ticket.priority, - new_value=priority, - ) - c.save() - ticket.priority = priority - - if due_date != ticket.due_date: - c = TicketChange( - followup=f, - field=_('Due on'), - old_value=ticket.due_date, - new_value=due_date, - ) - c.save() - ticket.due_date = due_date - - if changes_in_checklists: - for checklist in ticket.checklists.all(): - new_completed_tasks = list(map(int, request.POST.getlist(f'checklist-{checklist.id}', []))) - for task in checklist.tasks.all(): - changed = None - - # Add completion if it was not done yet - if not task.completion_date and task.id in new_completed_tasks: - task.completion_date = timezone.now() - changed = 'completed' - # Remove it if it was done before - elif task.completion_date and task.id not in new_completed_tasks: - task.completion_date = None - changed = 'uncompleted' - - # Save and add ticket change if task state has changed - if changed: - task.save(update_fields=['completion_date']) - f.ticketchange_set.create( - field=f'[{checklist.name}] {task.description}', - old_value=_('To do') if changed == 'completed' else _('Completed'), - new_value=_('Completed') if changed == 'completed' else _('To do'), - ) - - 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. - context = safe_template_context(ticket) - context.update( - resolution=ticket.resolution, - comment=f.comment, - ) - - messages_sent_to = set() - try: - messages_sent_to.add(request.user.email) - except AttributeError: - pass - ticket = update_messages_sent_to_by_public_and_status( - public, + update_ticket( + request.user, ticket, - f, - context, - messages_sent_to, - files + title = title, + comment = comment, + files = request.FILES.getlist('attachment'), + public = request.POST.get('public', False), + owner = int(request.POST.get('owner', -1)), + priority = int(request.POST.get('priority', -1)), + new_status = new_status, + time_spent = get_time_spent_from_request(request), + due_date = get_due_date_from_request_or_ticket(request, ticket), + new_checklists = new_checklists, ) - 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) - ): - messages_sent_to.update(ticket.send( - {'assigned_to': (template_staff, context)}, - dont_send_to=messages_sent_to, - fail_silently=True, - files=files, - )) - - messages_sent_to.update(ticket.send( - {'ticket_cc': (template_cc, context)}, - dont_send_to=messages_sent_to, - fail_silently=True, - files=files, - )) - ticket.save() - - # auto subscribe user if enabled - add_staff_subscription(request, ticket) - return return_to_ticket(request.user, helpdesk_settings, ticket) From b0ef6a54847edd1c43ec95fbfbee26c2a4fec8f0 Mon Sep 17 00:00:00 2001 From: Timothy Hobbs Date: Tue, 14 Nov 2023 21:47:12 +0100 Subject: [PATCH 4/4] Fix #1138 by calling update_ticket directly from non-update views --- helpdesk/update_ticket.py | 6 +++++- helpdesk/views/public.py | 23 +++++++++-------------- helpdesk/views/staff.py | 35 +++++++++++++---------------------- 3 files changed, 27 insertions(+), 37 deletions(-) diff --git a/helpdesk/update_ticket.py b/helpdesk/update_ticket.py index 0a3f2c85..d26a9ed3 100644 --- a/helpdesk/update_ticket.py +++ b/helpdesk/update_ticket.py @@ -194,7 +194,7 @@ def update_ticket( user, ticket, title=None, - comment=None, + comment="", files=None, public=False, owner=-1, @@ -207,6 +207,8 @@ def update_ticket( # We need to allow the 'ticket' and 'queue' contexts to be applied to the # comment. context = safe_template_context(ticket) + if title is None: + title = ticket.title if priority == -1: priority = ticket.priority if new_status is None: @@ -309,6 +311,8 @@ def update_ticket( ticket.due_date = due_date for checklist in ticket.checklists.all(): + if checklist.id not in new_checklists: + continue new_completed_tasks = new_checklists[checklist.id] for task in checklist.tasks.all(): changed = None diff --git a/helpdesk/views/public.py b/helpdesk/views/public.py index f2eca65f..c2d15b6d 100644 --- a/helpdesk/views/public.py +++ b/helpdesk/views/public.py @@ -210,21 +210,16 @@ def view_ticket(request): return HttpResponseRedirect(redirect_url) if 'close' in request.GET and ticket.status == Ticket.RESOLVED_STATUS: - from helpdesk.views.staff import update_ticket_view + from helpdesk.update_ticket import update_ticket + update_ticket( + request.user, + ticket, + public = True, + comment = _('Submitter accepted resolution and closed ticket'), + new_status = Ticket.CLOSED_STATUS, + ) + return HttpResponseRedirect(ticket.ticket_url) - # Trick the update_ticket() view into thinking it's being called with - # a valid POST. - request.POST = { - 'new_status': Ticket.CLOSED_STATUS, - 'public': 1, - 'title': ticket.title, - 'comment': _('Submitter accepted resolution and closed ticket'), - } - if ticket.assigned_to: - request.POST['owner'] = ticket.assigned_to.id - request.GET = {} - - return update_ticket_view(request, ticket_id, public=True) # redirect user back to this ticket if possible. redirect_url = '' diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index f1ca91e6..44ee9b29 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -347,17 +347,12 @@ def view_ticket(request, ticket_id): ticket_perm_check(request, ticket) if 'take' in request.GET: - # Allow the user to assign the ticket to themselves whilst viewing it. - - # Trick the update_ticket() view into thinking it's being called with - # a valid POST. - request.POST = { - 'owner': request.user.id, - 'public': 1, - 'title': ticket.title, - 'comment': '' - } - return update_ticket_view(request, ticket_id) + update_ticket( + request.user, + ticket, + owner=request.user.id + ) + return return_to_ticket(request.user, helpdesk_settings, ticket) if 'subscribe' in request.GET: # Allow the user to subscribe him/herself to the ticket whilst viewing @@ -376,17 +371,13 @@ def view_ticket(request, ticket_id): else: owner = ticket.assigned_to.id - # Trick the update_ticket() view into thinking it's being called with - # a valid POST. - request.POST = { - 'new_status': Ticket.CLOSED_STATUS, - 'public': 1, - 'owner': owner, - 'title': ticket.title, - 'comment': _('Accepted resolution and closed ticket'), - } - - return update_ticket_view(request, ticket_id) + update_ticket( + request.user, + ticket, + owner=owner, + comment= _('Accepted resolution and closed ticket'), + ) + return return_to_ticket(request.user, helpdesk_settings, ticket) if helpdesk_settings.HELPDESK_STAFF_ONLY_TICKET_OWNERS: users = User.objects.filter(