visit order should be computed on ids if the begin_date is the same

CHANGELOG

+smasch (1.0.0~beta.6-1) unstable; urgency=low
+
+  * bug fix: when two visits with the same start date are created compute visit
+    number on the visit creation order basis (#369)
+
+ -- Piotr Gawron <piotr.gawron@uni.lu>  Wed, 24 Feb 2021 12:00:00 +0200
+
 smasch (1.0.0~beta.5-1) unstable; urgency=low
 
   * bug fix: login page did not contain valid link to help with account

smash/web/models/visit.py

@@ -126,16 +126,17 @@ class Visit(models.Model):
         self.save()
         return
 
+
 @receiver(post_save, sender=Visit)
 def check_visit_number(sender, instance, created, **kwargs):
-    # no other solution to ensure the visit_number is in cronological order than to sort the whole list if there are future visits
+    # no other solution to ensure the visit_number is in chronological order than to sort the whole list if there are future visits
     visit = instance
     if visit.subject is not None: #not sure if select_for_update has an effect, the tests work as well without it
         #new visit, sort only future visit respect to the new one
         if created:
             visits_before = Visit.objects.select_for_update().filter(subject=visit.subject).filter(datetime_begin__lt=visit.datetime_begin).count()
             # we need to sort the future visits respect to the new one, if any
-            visits = Visit.objects.select_for_update().filter(subject=visit.subject).filter(datetime_begin__gte=visit.datetime_begin).order_by('datetime_begin','datetime_end')
+            visits = Visit.objects.select_for_update().filter(subject=visit.subject).filter(datetime_begin__gte=visit.datetime_begin).order_by('datetime_begin','id')
             with transaction.atomic(): #not sure if it has an effect, the tests work as well without it
                 for i, v in enumerate(visits):
                     expected_visit_number = (visits_before + i + 1)
@@ -143,10 +144,10 @@ def check_visit_number(sender, instance, created, **kwargs):
                         Visit.objects.filter(id=v.id).update(visit_number=expected_visit_number) # does not rise post_save, we avoid recursion
                         if v.id == visit.id: #if the iteration visit is the same that the instance that produced the signal call
                             #this ensures that the upper saved object is also updated, otherwise, refresh_from_db should be called
-                            visit.visit_number = v.visit_number 
+                            visit.visit_number = v.visit_number
         else:
             #if visits are modified, then, check everything
-            visits = Visit.objects.select_for_update().filter(subject=visit.subject).order_by('datetime_begin','datetime_end')
+            visits = Visit.objects.select_for_update().filter(subject=visit.subject).order_by('datetime_begin','id')
             with transaction.atomic():
                 for i, v in enumerate(visits):
                     expected_visit_number = (i+1)

smash/web/tests/models/test_visit.py

@@ -229,3 +229,32 @@ class VisitModelTests(TestCase):
     def test_visit_to_unicode(self):
         visit = create_visit(create_study_subject())
         self.assertIsNotNone(str(visit))
+
+    def test_visit_numbers_with_the_same_dates(self):
+        subject = create_study_subject()
+        visit1 = create_visit(subject=subject,
+                              datetime_begin=get_today_midnight_date() - datetime.timedelta(days=1),
+                              datetime_end=get_today_midnight_date() + datetime.timedelta(days=4))
+        visit2 = create_visit(subject=subject,
+                              datetime_begin=get_today_midnight_date() - datetime.timedelta(days=1),
+                              datetime_end=get_today_midnight_date() + datetime.timedelta(days=3))
+        visit3 = create_visit(subject=subject,
+                              datetime_begin=get_today_midnight_date() + datetime.timedelta(days=1),
+                              datetime_end=get_today_midnight_date() + datetime.timedelta(days=2))
+
+        visit3.is_finished = True
+        visit3.save()
+
+        visit2.mark_as_finished()
+
+        visit1.is_finished = True
+        visit1.save()
+
+        self.assertEqual(4, Visit.objects.filter(subject=subject).count())
+        # order should be [V1, V2, V3]
+        visit1.refresh_from_db()
+        visit2.refresh_from_db()
+        visit3.refresh_from_db()
+        self.assertEqual(1, visit1.visit_number)
+        self.assertEqual(2, visit2.visit_number)
+        self.assertEqual(3, visit3.visit_number)