Refactor finding conflicting shifts

scheduler/models.py

@@ -1,6 +1,6 @@
 # coding: utf-8
 
-import datetime
+from datetime import timedelta
 
 from django.db import models
 from django.utils import timezone
@@ -78,39 +78,29 @@ class Meta:
         verbose_name_plural = _(u'shifts')
         ordering = ['starting_time', 'ending_time']
 
-    def get_conflicting_needs(self, needs, grace=datetime.timedelta(hours=1)):
-        """
-        Given a list of other needs, this function returns needs that overlap by time.
-        A grace period of overlap is allowed:
+    def __unicode__(self):
+        return u"{title} - {location} ({start} - {end})".format(
+            title=self.topic.title, location=self.location.name,
+            start=localize(self.starting_time), end=localize(self.ending_time))
 
-               Event A: 10 till 14
-               Event B: 13 till 15
 
-        would not conflict if a grace period of 1 hour or more is allowed, but would conflict if
-        the grace period is less.
+class ShiftHelperManager(models.Manager):
+    def conflicting(self, need, user_account=None, grace=timedelta(hours=1)):
 
-        This is not the most efficient implementation, but one of the more obvious. Optimize
-        only if needed. Nicked from:
-        https://stackoverflow.com/questions/3721249/python-date-interval-intersection
+        grace = grace or timedelta(0)
+        graced_start = need.starting_time + grace
+        graced_end = need.ending_time - grace
 
-        :param needs: A Django queryset of Need instances.
-        """
-        latest_start_time = self.starting_time + grace
-        earliest_end_time = self.ending_time - grace
-        if earliest_end_time <= latest_start_time:
-            # Event is shorter than 2 * grace time, can't have overlaps.
-            return []
+        query_set = self.get_queryset().select_related('need', 'user_account')
 
-        return [
-            need for need in needs
-            if (need.starting_time < latest_start_time < need.ending_time) or
-            (latest_start_time < need.starting_time < earliest_end_time)
-            ]
+        if user_account:
+            query_set = query_set.filter(user_account=user_account)
 
-    def __unicode__(self):
-        return u"{title} - {location} ({start} - {end})".format(
-            title=self.topic.title, location=self.location.name,
-            start=localize(self.starting_time), end=localize(self.ending_time))
+        query_set = query_set.exclude(need__starting_time__lt=graced_start,
+                                      need__ending_time__lte=graced_start)
+        query_set = query_set.exclude(need__starting_time__gte=graced_end,
+                                      need__ending_time__gte=graced_end)
+        return query_set
 
 
 class ShiftHelper(models.Model):
@@ -119,9 +109,18 @@ class ShiftHelper(models.Model):
     need = models.ForeignKey('scheduler.Need', related_name='shift_helpers')
     joined_shift_at = models.DateTimeField(auto_now_add=True)
 
+    objects = ShiftHelperManager()
+
     class Meta:
         verbose_name = _('shift helper')
         verbose_name_plural = _('shift helpers')
+        unique_together = ('user_account', 'need')
+
+    def __repr__(self):
+        return "{}".format(self.need)
+
+    def __unicode__(self):
+        return u"{}".format(repr(self))
 
 
 class Topics(models.Model):

scheduler/views.py

@@ -6,6 +6,7 @@
 from django.core.urlresolvers import reverse
 from django.contrib import messages
 from django.db.models import Count
+from django.utils.safestring import mark_safe
 from django.views.generic import TemplateView, FormView, DetailView
 from django.shortcuts import get_object_or_404
 
@@ -82,18 +83,20 @@ def form_valid(self, form):
         print(form.cleaned_data)
 
         if shift_to_join:
-            conflicts = shift_to_join.get_conflicting_needs(
-                user_account.needs.all())
-            if conflicts:
-                conflicts_string = u", ".join(
-                    u'{}'.format(conflict) for conflict in conflicts)
+
+            conflicts = ShiftHelper.objects.conflicting(shift_to_join,
+                                                        user_account=user_account)
+            conflicted_needs = [shift_helper.need for shift_helper in conflicts]
+
+            if conflicted_needs:
+                error_message = _(
+                    u'We can\'t add you to this shift because you\'ve already agreed to other shifts at the same time:')
+                message_list = u'<ul>{}</ul>'.format('\n'.join(
+                    ['<li>{}</li>'.format(conflict) for conflict in conflicted_needs]))
                 messages.warning(self.request,
-                                 _(
-                                     u'We can\'t add you to this shift because you\'ve already agreed to other shifts at the same time: {conflicts}'.format(
-                                         conflicts=
-                                         conflicts_string)))
+                                 mark_safe(u'{}<br/>{}'.format(error_message,
+                                                     message_list)))
             else:
-                # user_account.needs.add(join_shift)
                 shift_helper, created = ShiftHelper.objects.get_or_create(
                     user_account=user_account, need=shift_to_join)
                 if created:
@@ -108,7 +111,8 @@ def form_valid(self, form):
                 ShiftHelper.objects.get(user_account=user_account,
                                         need=shift_to_leave).delete()
             except ShiftHelper.DoesNotExist:
-                # just catch the exception, user seems not to have signed up for this shift tho
+                # just catch the exception,
+                # user seems not to have signed up for this shift
                 pass
             messages.success(self.request, _(
                 u'You successfully left this shift.'))

tests/scheduler/test_models.py

@@ -1,18 +1,24 @@
-import datetime
+# coding: utf-8
+
+from datetime import timedelta, datetime
 
 from django.test import TestCase
 
-from scheduler.models import Need, Location
-from tests.factories import NeedFactory, LocationFactory
+from scheduler.models import Need, Location, ShiftHelper
+from tests.factories import NeedFactory, LocationFactory, UserAccountFactory
 
 
-def create_need(start_hour, end_hour):
+def create_need(start_hour, end_hour, location=None):
     """
     Tiny helper because setting time periods is awkward till we remove the FK relationship.
     """
-    start = datetime.datetime(2015, 1, 1, start_hour)
-    end = datetime.datetime(2015, 1, 1, end_hour)
-    return NeedFactory.create(starting_time=start, ending_time=end)
+    create_args = dict(
+        starting_time=datetime(2015, 1, 1, start_hour),
+        ending_time=datetime(2015, 1, 1, end_hour)
+    )
+    if location:
+        create_args['location'] = location
+    return NeedFactory.create(**create_args)
 
 
 class NeedTestCase(TestCase):
@@ -22,36 +28,101 @@ class NeedTestCase(TestCase):
     """
 
     def setUp(self):
-        self.needs = [create_need(9, 12), create_need(18, 21)]
-
-    def test_non_conflict(self):
-        need = create_need(14, 16)
-        assert not need.get_conflicting_needs(self.needs)
-
-    def test_clear_conflict(self):
+        self.user_account = UserAccountFactory.create()
+        self.morning_shift = create_need(9, 12)
+        ShiftHelper.objects.create(user_account=self.user_account,
+                                   need=self.morning_shift)
+
+        self.evening_shift = create_need(18, 21)
+        ShiftHelper.objects.create(user_account=self.user_account,
+                                   need=self.evening_shift)
+
+    def tearDown(self):
+        Need.objects.all().delete()
+        ShiftHelper.objects.all().delete()
+
+    def test_non_conflict_tight_fitting(self):
+        need = create_need(12, 18)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 0
+
+    def test_non_conflict_gap_after(self):
+        need = create_need(12, 17)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 0
+
+    def test_non_conflict_gap_before(self):
+        need = create_need(13, 18)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 0
+
+    def test_non_conflict_tight_fitting_no_grace(self):
+        need = create_need(12, 18)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 0
+
+    def test_non_conflict_gap_after_no_grace(self):
+        need = create_need(12, 17)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 0
+
+    def test_non_conflict_gap_before_no_grace(self):
+        need = create_need(13, 18)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 0
+
+    def test_non_conflict_gaps(self):
+        need = create_need(13, 17)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 0
+
+    def test_non_conflict_gaps_no_grace(self):
+        need = create_need(13, 17)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 0
+
+    def test_conflict_at_beginning(self):
+        need = create_need(8, 11)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 1
+
+    def test_conflict_at_beginning_no_grace(self):
+        need = create_need(9, 11)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 1
+
+    def test_conflict_at_end(self):
+        need = create_need(10, 15)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 1
+
+    def test_conflict_at_end_no_grace(self):
+        need = create_need(11, 15)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 1
+
+    def test_conflict_within(self):
+        need = create_need(10, 11)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 1
+
+    def test_conflict_within_no_grace(self):
         need = create_need(9, 12)
-        assert need.get_conflicting_needs(self.needs)
+        assert ShiftHelper.objects.conflicting(need=need,
+                                               grace=None).count() == 1
 
-    def test_not_conflict_1h_grace(self):
-        need = create_need(11, 14)
-        assert not need.get_conflicting_needs(self.needs)
+    def test_conflict_around(self):
+        need = create_need(8, 13)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 1
 
-    def test_conflict_0h_grace(self):
-        need = create_need(11, 14)
-        assert need.get_conflicting_needs(self.needs,
-                                          grace=datetime.timedelta(hours=0))
+    def test_conflict_around_no_grace(self):
+        need = create_need(8, 13)
+        assert ShiftHelper.objects.conflicting(need=need).count() == 1
 
 
 class LocationTestCase(TestCase):
     def test_need_manager_for_location(self):
         """
             checks that get_days_with_needs() returns only dates later than datetime.now()
         """
-        now = datetime.datetime.now()
-        yesterday_start = now - datetime.timedelta(1)
-        yesterday_end = yesterday_start + datetime.timedelta(hours=1)
-        tomorrow_start = now + datetime.timedelta(1)
-        tomorrow_end = tomorrow_start + datetime.timedelta(hours=1)
+        now = datetime.now()
+        yesterday_start = now - timedelta(1)
+        yesterday_end = yesterday_start + timedelta(hours=1)
+        tomorrow_start = now + timedelta(1)
+        tomorrow_end = tomorrow_start + timedelta(hours=1)
 
         location = LocationFactory.create()