From 0610a6645d2e4bd80ed86ae52cfdf09bd61bf4d3 Mon Sep 17 00:00:00 2001 From: Jonathan Barratt Date: Mon, 16 Nov 2015 18:45:27 +0700 Subject: [PATCH] Fix QueueMembership bug revealed by django.test's DiscoverRunner If HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP was True but a user had no QueueMembership entries, then restricting queue access generated RelatedObjectDoesNotExist exceptions. - Ask for forgiveness whenever we try to limit a queryset by the queuemembership related object set. - Since tests can now be run with the project's settings active, rather than only with quicktest.py's settings, restore the initial HELPDESK_ENABLE_PER_QUEUE_MEMBERSHIP value after having tested the related functionality. --- .../tests/test_per_queue_staff_membership.py | 13 +++--- helpdesk/views/staff.py | 40 ++++++++++++------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/helpdesk/tests/test_per_queue_staff_membership.py b/helpdesk/tests/test_per_queue_staff_membership.py index 3b281ed4..65ff7605 100644 --- a/helpdesk/tests/test_per_queue_staff_membership.py +++ b/helpdesk/tests/test_per_queue_staff_membership.py @@ -17,6 +17,7 @@ class PerQueueStaffMembershipTestCase(TestCase): and user_2 with access to queue_2 containing 2 tickets and superuser who should be able to access both queues """ + self.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP = settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP = True self.client = Client() User = get_user_model() @@ -59,6 +60,12 @@ class PerQueueStaffMembershipTestCase(TestCase): assigned_to=user, ) + def tearDown(self): + """ + Reset HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP to original value + """ + settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP = self.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP + def test_dashboard_ticket_counts(self): """ Check that the regular users' dashboard only shows 1 of the 2 queues, @@ -213,9 +220,3 @@ class PerQueueStaffMembershipTestCase(TestCase): 3, 'Queue choices were improperly limited by queue membership for a superuser' ) - - def tearDown(self): - """ - Don't interfere with subsequent tests that do not expect this setting - """ - settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP = False diff --git a/helpdesk/views/staff.py b/helpdesk/views/staff.py index c44073f1..af30017d 100644 --- a/helpdesk/views/staff.py +++ b/helpdesk/views/staff.py @@ -39,7 +39,7 @@ except ImportError: from helpdesk.forms import TicketForm, UserSettingsForm, EmailIgnoreForm, EditTicketForm, TicketCCForm, EditFollowUpForm, TicketDependencyForm from helpdesk.lib import send_templated_mail, query_to_dict, apply_query, safe_template_context -from helpdesk.models import Ticket, Queue, FollowUp, TicketChange, PreSetReply, Attachment, SavedSearch, IgnoreEmail, TicketCC, TicketDependency +from helpdesk.models import Ticket, Queue, FollowUp, TicketChange, PreSetReply, Attachment, SavedSearch, IgnoreEmail, TicketCC, TicketDependency, QueueMembership from helpdesk import settings as helpdesk_settings if helpdesk_settings.HELPDESK_ALLOW_NON_STAFF_TICKET_UPDATE: @@ -78,9 +78,12 @@ def dashboard(request): ) limit_queues_by_user = helpdesk_settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP and not request.user.is_superuser if limit_queues_by_user: - unassigned_tickets = unassigned_tickets.filter( - queue__in=request.user.queuemembership.queues.all(), - ) + try: + unassigned_tickets = unassigned_tickets.filter( + queue__in=request.user.queuemembership.queues.all(), + ) + except QueueMembership.DoesNotExist: + unassigned_tickets = unassigned_tickets.none() # all tickets, reported by current user all_tickets_reported_by_current_user = '' @@ -92,9 +95,12 @@ def dashboard(request): Tickets = Ticket.objects if limit_queues_by_user: - Tickets = Tickets.filter( - queue__in=request.user.queuemembership.queues.all(), - ) + try: + Tickets = Tickets.filter( + queue__in=request.user.queuemembership.queues.all(), + ) + except QueueMembership.DoesNotExist: + Tickets = Tickets.none() basic_ticket_stats = calc_basic_ticket_stats(Tickets) # The following query builds a grid of queues & ticket statuses, @@ -367,7 +373,7 @@ def update_ticket(request, ticket_id, public=False): # if comment contains some django code, like "why does {% if bla %} crash", # then the following line will give us a crash, since django expects {% if %} # to be closed with an {% endif %} tag. - + # get_template_from_string was removed in Django 1.8 http://django.readthedocs.org/en/1.8.x/ref/templates/upgrading.html try: @@ -1001,9 +1007,12 @@ def run_report(request, report): report_queryset = Ticket.objects.all().select_related() limit_queues_by_user = helpdesk_settings.HELPDESK_ENABLE_PER_QUEUE_STAFF_MEMBERSHIP and not request.user.is_superuser if limit_queues_by_user: - report_queryset = report_queryset.filter( - queue__in=request.user.queuemembership.queues.all(), - ) + try: + report_queryset = report_queryset.filter( + queue__in=request.user.queuemembership.queues.all(), + ) + except QueueMembership.DoesNotExist: + report_queryset = report_queryset.none() from_saved_query = False saved_query = None @@ -1065,9 +1074,12 @@ def run_report(request, report): col1heading = _('User') queue_options = Queue.objects.all() if limit_queues_by_user: - queue_options = queue_options.filter( - pk__in=request.user.queuemembership.queues.all(), - ) + try: + queue_options = queue_options.filter( + pk__in=request.user.queuemembership.queues.all(), + ) + except QueueMembership.DoesNotExist: + queue_options = queue_options.none() possible_options = [q.title.encode('utf-8') for q in queue_options] charttype = 'bar'