Merge pull request #375 from CTPUG/bugs/clashes_not_shown_correctly

Tweak logic around clashes and invalid venues, so errors show correctly in the admin interface

wafer/schedule/admin.py

@@ -112,7 +112,8 @@ def find_clashes(all_items):
                 clashes[pos].append(item)
             else:
                 seen_venue_slots[pos] = item
-    return clashes
+    # We return a list, to match other validators
+    return clashes.items()
 
 
 def find_invalid_venues(all_items):
@@ -130,7 +131,7 @@ def find_invalid_venues(all_items):
         if not valid:
             venues.setdefault(item.venue, [])
             venues[item.venue].append(item)
-    return venues
+    return venues.items()
 
 
 # Helper methods for calling the validators

wafer/schedule/templates/admin/scheduleitem_list.html

@@ -12,10 +12,10 @@ <h2>{% trans "Errors in the schedule" %}</h2>
           {% if errors.clashes %}
           <h3>{% trans "Clashes" %}</h3>
           <ul>
-             {% for pos, items in errors.clashes.items %}
-             <li>{{ pos.0 }} at {{ pos.1.get_start_time|time:"H:i" }} --
+             {% for pos, items in errors.clashes %}
+             <li>{{ pos.0 }} at {{ items.0.get_start_time }} --
                  {% for item in items %}
-                     {{ item }},
+                 <b>{{ item.get_desc|escape }}</b>,
                  {% endfor %}
              </li>
              {% endfor %}
@@ -48,10 +48,10 @@ <h3>{% trans "Duplicates in the schedule" %}</h3>
           {% if errors.venues %}
           <h3>{% trans "Venues assigned on days they are not available" %}</h3>
           <ul>
-             {% for venue, items in errors.venues.items %}
-             <li>{{ venue }} --
+             {% for venue, items in errors.venues %}
+             <li>{{ venue }} at {{ items.0.get_start_time }} --
                  {% for item in items %}
-                    {{ item }},
+                 <b>{{ item.get_desc|escape }}</b>,
                  {% endfor %}
              </li>
              {% endfor %}

wafer/schedule/tests/test_admin.py

@@ -407,35 +407,40 @@ def test_clashes(self):
         item1.slots.add(slot1)
         item2.slots.add(slot1)
         all_items = prefetch_schedule_items()
-        clashes = find_clashes(all_items)
+        # Needs to be an explicit list, not a generator on python 3 here
+        clashes = list(find_clashes(all_items))
         assert len(clashes) == 1
         pos = (venue1, slot1)
-        assert pos in clashes
-        assert item1 in clashes[pos]
-        assert item2 in clashes[pos]
+        assert pos == clashes[0][0]
+        assert item1 in clashes[0][1]
+        assert item2 in clashes[0][1]
         # Create a overlapping clashes
         item2.slots.remove(slot1)
         item1.slots.add(slot2)
         item2.slots.add(slot2)
         all_items = prefetch_schedule_items()
-        clashes = find_clashes(all_items)
+        clashes = list(find_clashes(all_items))
         assert len(clashes) == 1
         pos = (venue1, slot2)
-        assert pos in clashes
-        assert item1 in clashes[pos]
-        assert item2 in clashes[pos]
+        assert pos == clashes[0][0]
+        assert item1 in clashes[0][1]
+        assert item2 in clashes[0][1]
         # Add a clash in a second venue
         item3 = ScheduleItem.objects.create(venue=venue2, details="Item 3")
         item4 = ScheduleItem.objects.create(venue=venue2, details="Item 4")
         item3.slots.add(slot2)
         item4.slots.add(slot2)
         all_items = prefetch_schedule_items()
-        clashes = find_clashes(all_items)
+        clashes = list(find_clashes(all_items))
         assert len(clashes) == 2
         pos = (venue2, slot2)
-        assert pos in clashes
-        assert item3 in clashes[pos]
-        assert item4 in clashes[pos]
+        assert pos in [x[0] for x in clashes]
+        clash_items = []
+        for sublist in clashes:
+            if sublist[0] == pos:
+                clash_items.extend(sublist[1])
+        assert item3 in clash_items
+        assert item4 in clash_items
         # Fix clashes
         item1.slots.remove(slot2)
         item3.slots.remove(slot2)
@@ -652,30 +657,38 @@ def test_venues(self):
         item2.slots.add(slot1)
 
         all_items = prefetch_schedule_items()
-        venues = find_invalid_venues(all_items)
-        assert set(venues) == set([venue2])
-        assert set(venues[venue2]) == set([item2])
+        # Needs to be an explicit list, as in the find_clashes test
+        venues = list(find_invalid_venues(all_items))
+        assert venues[0][0] == venue2
+        assert set(venues[0][1]) == set([item2])
 
         slot2 = Slot.objects.create(start_time=start1, end_time=start2,
                                     day=day2)
         item3 = ScheduleItem.objects.create(venue=venue2, page_id=page.pk)
 
         item3.slots.add(slot2)
         all_items = prefetch_schedule_items()
-        venues = find_invalid_venues(all_items)
-        assert set(venues) == set([venue2])
-        assert set(venues[venue2]) == set([item2])
+        venues = list(find_invalid_venues(all_items))
+        assert venues[0][0] == venue2
+        assert set(venues[0][1]) == set([item2])
 
         item4 = ScheduleItem.objects.create(venue=venue1, page_id=page.pk)
         item5 = ScheduleItem.objects.create(venue=venue2, page_id=page.pk)
 
         item4.slots.add(slot2)
         item5.slots.add(slot1)
         all_items = prefetch_schedule_items()
-        venues = find_invalid_venues(all_items)
-        assert set(venues) == set([venue1, venue2])
-        assert set(venues[venue1]) == set([item4])
-        assert set(venues[venue2]) == set([item2, item5])
+        venues = list(find_invalid_venues(all_items))
+        assert set([x[0] for x in venues]) == set([venue1, venue2])
+        venue_1_items = []
+        venue_2_items = []
+        for sublist in venues:
+            if sublist[0] == venue1:
+                venue_1_items.extend(sublist[1])
+            elif sublist[0] == venue2:
+                venue_2_items.extend(sublist[1])
+        assert set(venue_1_items) == set([item4])
+        assert set(venue_2_items) == set([item2, item5])
 
     def test_validate_schedule(self):
         """Check the behaviour of validate schedule