From 800d9f6c656006bb74a15b38efde9923149c0e63 Mon Sep 17 00:00:00 2001 From: Valentin Deniaud Date: Thu, 21 Apr 2022 11:48:57 +0200 Subject: [PATCH 1/2] api: forbid overlapping events booking (#64383) --- chrono/agendas/models.py | 29 ++++- chrono/api/serializers.py | 2 + chrono/api/views.py | 15 +++ chrono/utils/db.py | 11 +- .../datetimes/test_events_multiple_agendas.py | 86 ++++++++++++- tests/api/fillslot/test_events.py | 118 +++++++++++++++++- .../fillslot/test_events_multiple_agendas.py | 50 +++++++- 7 files changed, 300 insertions(+), 11 deletions(-) diff --git a/chrono/agendas/models.py b/chrono/agendas/models.py index 7bb3e658..6692e6e7 100644 --- a/chrono/agendas/models.py +++ b/chrono/agendas/models.py @@ -36,8 +36,8 @@ from django.contrib.postgres.fields import ArrayField, JSONField from django.core.exceptions import FieldDoesNotExist, ValidationError from django.core.validators import MaxValueValidator, MinValueValidator from django.db import connection, models, transaction -from django.db.models import Count, Exists, F, Func, Max, OuterRef, Prefetch, Q -from django.db.models.functions import Cast, Coalesce, ExtractWeek +from django.db.models import Count, Exists, ExpressionWrapper, F, Func, Max, OuterRef, Prefetch, Q, Value +from django.db.models.functions import Cast, Coalesce, Concat, ExtractWeek from django.template import Context, Template, TemplateSyntaxError, VariableDoesNotExist, engines from django.urls import reverse from django.utils import functional @@ -55,7 +55,7 @@ from django.utils.translation import ungettext from chrono.interval import Interval, IntervalSet from chrono.utils.date import get_weekday_index -from chrono.utils.db import SumCardinality +from chrono.utils.db import ArraySubquery, SumCardinality from chrono.utils.publik_urls import translate_from_publik_url from chrono.utils.requests_wrapper import requests as requests_wrapper @@ -1562,6 +1562,29 @@ class Event(models.Model): ) return qs + @staticmethod + def annotate_queryset_with_overlaps(qs): + qs = qs.annotate( + computed_end_datetime=ExpressionWrapper( + F('start_datetime') + datetime.timedelta(minutes=1) * F('duration'), + output_field=models.DateTimeField(), + ), + computed_slug=Concat('agenda__slug', Value('@'), 'slug'), + ) + + overlapping_events = qs.filter( + start_datetime__lt=OuterRef('computed_end_datetime'), + computed_end_datetime__gt=OuterRef('start_datetime'), + ).exclude(pk=OuterRef('pk')) + + return qs.annotate( + overlaps=ArraySubquery( + overlapping_events.values('computed_slug'), + output_field=ArrayField(models.CharField()), + ), + has_overlap=Exists(overlapping_events), + ) + @property def remaining_places(self): return max(0, self.places - self.booked_places) diff --git a/chrono/api/serializers.py b/chrono/api/serializers.py index 3f7606a9..84c53baf 100644 --- a/chrono/api/serializers.py +++ b/chrono/api/serializers.py @@ -75,6 +75,7 @@ class SlotSerializer(serializers.Serializer): extra_phone_numbers = StringOrListField( required=False, child=serializers.CharField(max_length=16, allow_blank=False) ) + check_overlaps = serializers.BooleanField(default=False) class SlotsSerializer(SlotSerializer): @@ -325,6 +326,7 @@ class MultipleAgendasDatetimesSerializer(AgendaOrSubscribedSlugsMixin, Datetimes guardian_external_id = serializers.CharField(max_length=250, required=False) with_status = serializers.BooleanField(default=False) guardian_external_id = serializers.CharField(max_length=250, required=False) + check_overlaps = serializers.BooleanField(default=False) def validate(self, attrs): super().validate(attrs) diff --git a/chrono/api/views.py b/chrono/api/views.py index 83a8e15d..d944fb87 100644 --- a/chrono/api/views.py +++ b/chrono/api/views.py @@ -596,6 +596,8 @@ def get_event_detail( details['status'] = 'cancelled' else: details['status'] = 'free' + if hasattr(event, 'overlaps'): + details['overlaps'] = event.overlaps return details @@ -911,6 +913,7 @@ class MultipleAgendasDatetimes(APIView): show_past_events = bool(payload.get('show_past_events')) show_only_subscribed = bool('subscribed' in payload) with_status = bool(payload.get('with_status')) + check_overlaps = bool(payload.get('check_overlaps')) entries = Event.objects.none() for agenda in agendas: @@ -926,6 +929,8 @@ class MultipleAgendasDatetimes(APIView): show_out_of_minimal_delay=show_past_events, ) entries = Event.annotate_queryset_for_user(entries, user_external_id, with_status=with_status) + if check_overlaps: + entries = Event.annotate_queryset_with_overlaps(entries) if show_only_subscribed: entries = entries.filter( agenda__subscriptions__user_external_id=user_external_id, @@ -1843,9 +1848,18 @@ class EventsFillslots(APIView): payload = serializer.validated_data user_external_id = payload['user_external_id'] bypass_delays = payload.get('bypass_delays') + check_overlaps = payload.get('check_overlaps') events = self.get_events(request, payload) + if check_overlaps: + overlapping_events = Event.annotate_queryset_with_overlaps(events).filter(has_overlap=True) + if overlapping_events: + raise APIError( + N_('Some events occur at the same time: %s'), + ', '.join(sorted(str(x) for x in overlapping_events)), + ) + already_booked_events = self.get_already_booked_events(user_external_id) already_booked_events = already_booked_events.filter(start_datetime__gt=now()) if start_datetime: @@ -1877,6 +1891,7 @@ class EventsFillslots(APIView): booking__user_external_id=user_external_id, booking__cancellation_datetime__isnull=False, ) + # book only events without active booking for the user events = events.exclude( pk__in=Booking.objects.filter( diff --git a/chrono/utils/db.py b/chrono/utils/db.py index 2f202643..bbf3c83a 100644 --- a/chrono/utils/db.py +++ b/chrono/utils/db.py @@ -15,13 +15,22 @@ # along with this program. If not, see . from django.db.migrations.operations.base import Operation -from django.db.models import Aggregate +from django.db.models import Aggregate, Subquery class SumCardinality(Aggregate): template = 'SUM(CARDINALITY(%(expressions)s))' +class ArraySubquery(Subquery): + ''' + Available in Django 4.0 + https://docs.djangoproject.com/en/4.0/ref/contrib/postgres/expressions/#arraysubquery-expressions + ''' + + template = 'ARRAY(%(subquery)s)' + + class EnsureJsonbType(Operation): reversible = True diff --git a/tests/api/datetimes/test_events_multiple_agendas.py b/tests/api/datetimes/test_events_multiple_agendas.py index c932b4c2..6c12e50f 100644 --- a/tests/api/datetimes/test_events_multiple_agendas.py +++ b/tests/api/datetimes/test_events_multiple_agendas.py @@ -382,7 +382,11 @@ def test_datetimes_multiple_agendas_queries(app): with CaptureQueriesContext(connection) as ctx: resp = app.get( '/api/agendas/datetimes/', - params={'agendas': ','.join(str(i) for i in range(10)), 'show_past_events': True}, + params={ + 'agendas': ','.join(str(i) for i in range(10)), + 'show_past_events': True, + 'check_overlaps': True, + }, ) assert len(resp.json['data']) == 30 assert len(ctx.captured_queries) == 2 @@ -390,7 +394,12 @@ def test_datetimes_multiple_agendas_queries(app): with CaptureQueriesContext(connection) as ctx: resp = app.get( '/api/agendas/datetimes/', - params={'subscribed': 'all', 'user_external_id': 'xxx', 'show_past_events': True}, + params={ + 'subscribed': 'all', + 'user_external_id': 'xxx', + 'show_past_events': True, + 'check_overlaps': True, + }, ) assert len(resp.json['data']) == 30 assert len(ctx.captured_queries) == 2 @@ -398,7 +407,12 @@ def test_datetimes_multiple_agendas_queries(app): with CaptureQueriesContext(connection) as ctx: resp = app.get( '/api/agendas/datetimes/', - params={'subscribed': 'category-a', 'user_external_id': 'xxx', 'show_past_events': True}, + params={ + 'subscribed': 'category-a', + 'user_external_id': 'xxx', + 'show_past_events': True, + 'check_overlaps': True, + }, ) assert len(resp.json['data']) == 30 assert len(ctx.captured_queries) == 2 @@ -420,6 +434,7 @@ def test_datetimes_multiple_agendas_queries(app): 'user_external_id': 'xxx', 'guardian_external_id': 'mother_id', 'show_past_events': True, + 'check_overlaps': True, }, ) assert len(resp.json['data']) == 30 @@ -1352,3 +1367,68 @@ def test_datetimes_multiple_agendas_with_status(app): status=400, ) assert 'required' in resp.json['errors']['user_external_id'][0] + + +@pytest.mark.freeze_time('2021-09-06 12:00') +def test_datetimes_multiple_agendas_overlapping_events(app): + agenda = Agenda.objects.create(label='Foo bar', kind='events') + Event.objects.create( + label='Event 12-14', + start_datetime=now() + datetime.timedelta(days=5), + duration=120, + places=5, + agenda=agenda, + ) + Event.objects.create( + label='Event containing all events', + start_datetime=now() + datetime.timedelta(days=4, hours=23), + duration=440, + places=5, + agenda=agenda, + ) + second_agenda = Agenda.objects.create(label='Foo bar 2', kind='events') + Event.objects.create( + label='Event 13-15', + start_datetime=now() + datetime.timedelta(days=5, hours=1), + duration=120, + places=5, + agenda=second_agenda, + ) + Event.objects.create( + label='Event 14-16', + start_datetime=now() + datetime.timedelta(days=5, hours=2), + duration=120, + places=5, + agenda=second_agenda, + ) + Event.objects.create( + label='Event no duration', + start_datetime=now() + datetime.timedelta(days=5, hours=1), + places=5, + agenda=second_agenda, + ) + Event.objects.create( + label='Event other day', + start_datetime=now() + datetime.timedelta(days=6), + places=5, + agenda=second_agenda, + ) + + resp = app.get('/api/agendas/datetimes/', params={'agendas': 'foo-bar,foo-bar-2', 'check_overlaps': True}) + assert [(x['id'], x['overlaps']) for x in resp.json['data']] == [ + ( + 'foo-bar@event-containing-all-events', + ['foo-bar@event-12-14', 'foo-bar-2@event-13-15', 'foo-bar-2@event-14-16'], + ), + ('foo-bar@event-12-14', ['foo-bar@event-containing-all-events', 'foo-bar-2@event-13-15']), + ( + 'foo-bar-2@event-13-15', + ['foo-bar@event-containing-all-events', 'foo-bar@event-12-14', 'foo-bar-2@event-14-16'], + ), + ('foo-bar-2@event-no-duration', []), + ('foo-bar-2@event-14-16', ['foo-bar@event-containing-all-events', 'foo-bar-2@event-13-15']), + ('foo-bar-2@event-other-day', []), + ] + + resp = app.get('/api/agendas/datetimes/', params={'agendas': 'foo-bar,foo-bar-2'}) + assert ['overlaps' not in x for x in resp.json['data']] diff --git a/tests/api/fillslot/test_events.py b/tests/api/fillslot/test_events.py index 1c9cf504..d6122422 100644 --- a/tests/api/fillslot/test_events.py +++ b/tests/api/fillslot/test_events.py @@ -30,10 +30,10 @@ def test_api_events_fillslots(app, user): app.authorization = ('Basic', ('john.doe', 'password')) fillslots_url = '/api/agenda/%s/events/fillslots/' % agenda.slug - params = {'user_external_id': 'user_id', 'slots': 'event,event-2'} + params = {'user_external_id': 'user_id', 'check_overlaps': True, 'slots': 'event,event-2'} with CaptureQueriesContext(connection) as ctx: resp = app.post_json(fillslots_url, params=params) - assert len(ctx.captured_queries) == 11 + assert len(ctx.captured_queries) == 12 assert resp.json['booking_count'] == 2 assert len(resp.json['booked_events']) == 2 assert resp.json['booked_events'][0]['id'] == 'event' @@ -402,3 +402,117 @@ def test_api_events_fillslots_preserve_out_of_delays_bookings(app, user, freezer assert resp.json['cancelled_booking_count'] == 0 assert event.booking_set.filter(cancellation_datetime__isnull=True).count() == 1 assert second_event.booking_set.filter(cancellation_datetime__isnull=True).count() == 0 + + +@pytest.mark.freeze_time('2021-09-06 12:00') +def test_api_events_fillslots_overlapping_events(app, user, freezer): + agenda = Agenda.objects.create(label='Foo bar', kind='events') + first_event = Event.objects.create( + label='Event 12-14', + start_datetime=now() + datetime.timedelta(days=5), + duration=120, + places=5, + agenda=agenda, + ) + second_event = Event.objects.create( + label='Event 13-15', + start_datetime=now() + datetime.timedelta(days=5, hours=1), + duration=120, + places=5, + agenda=agenda, + ) + Event.objects.create( + label='Event 14-16', + start_datetime=now() + datetime.timedelta(days=5, hours=2), + duration=120, + places=5, + agenda=agenda, + ) + Event.objects.create( + label='Event no duration', + start_datetime=now() + datetime.timedelta(days=5, hours=1), + places=5, + agenda=agenda, + ) + + app.authorization = ('Basic', ('john.doe', 'password')) + fillslots_url = '/api/agenda/foo-bar/events/fillslots/' + params = {'user_external_id': 'user_id', 'check_overlaps': True} + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14'}) + assert resp.json['booking_count'] == 1 + + # booking the same event is still allowed + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14'}) + assert resp.json['err'] == 0 + assert resp.json['booking_count'] == 0 + + # changing booking to second event is allowed + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-13-15'}) + assert resp.json['err'] == 0 + assert resp.json['booking_count'] == 1 + assert resp.json['cancelled_booking_count'] == 1 + + # events are not overlapping if one ends when the other starts + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-14-16'}) + assert resp.json['booking_count'] == 2 + assert resp.json['cancelled_booking_count'] == 1 + + # booking overlapping events is allowed if one has no duration + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-no-duration'}) + assert resp.json['err'] == 0 + assert resp.json['booking_count'] == 1 + assert resp.json['cancelled_booking_count'] == 1 + + # default behavior does not check for overlaps + resp = app.post_json( + fillslots_url, params={'user_external_id': 'user_id', 'slots': 'event-12-14,event-13-15'} + ) + assert resp.json['err'] == 0 + assert resp.json['booking_count'] == 1 + assert resp.json['cancelled_booking_count'] == 1 + + # clearing overlapping bookings is allowed + resp = app.post_json(fillslots_url, params={**params, 'slots': ''}) + assert resp.json['err'] == 0 + assert resp.json['booking_count'] == 0 + assert resp.json['cancelled_booking_count'] == 2 + + # booking overlapping events with durations is forbidden + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-13-15'}) + assert resp.json['err'] == 1 + assert resp.json['err_desc'] == 'Some events occur at the same time: Event 12-14, Event 13-15' + + # still overlaps but start before + second_event.start_datetime -= datetime.timedelta(hours=2) + second_event.save() + + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-13-15'}) + assert resp.json['err'] == 1 + assert resp.json['err_desc'] == 'Some events occur at the same time: Event 12-14, Event 13-15' + + # still overlaps but contains first event + second_event.start_datetime = first_event.start_datetime - datetime.timedelta(minutes=10) + second_event.save() + second_event.duration = first_event.duration + 10 + second_event.save() + + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-13-15'}) + assert resp.json['err'] == 1 + assert resp.json['err_desc'] == 'Some events occur at the same time: Event 12-14, Event 13-15' + + # still overlaps but contained by first event + second_event.start_datetime = first_event.start_datetime + datetime.timedelta(minutes=10) + second_event.save() + second_event.duration = first_event.duration - 10 + second_event.save() + + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-13-15'}) + assert resp.json['err'] == 1 + assert resp.json['err_desc'] == 'Some events occur at the same time: Event 12-14, Event 13-15' + + # no more overlap + second_event.start_datetime -= datetime.timedelta(hours=5) + second_event.save() + + resp = app.post_json(fillslots_url, params={**params, 'slots': 'event-12-14,event-13-15'}) + assert resp.json['booking_count'] == 2 diff --git a/tests/api/fillslot/test_events_multiple_agendas.py b/tests/api/fillslot/test_events_multiple_agendas.py index 4d68e67c..65df798e 100644 --- a/tests/api/fillslot/test_events_multiple_agendas.py +++ b/tests/api/fillslot/test_events_multiple_agendas.py @@ -160,10 +160,10 @@ def test_api_events_fillslots_multiple_agendas(app, user): assert event_slugs == 'first-agenda@event,second-agenda@event' app.authorization = ('Basic', ('john.doe', 'password')) - params = {'user_external_id': 'user_id', 'slots': event_slugs} + params = {'user_external_id': 'user_id', 'check_overlaps': True, 'slots': event_slugs} with CaptureQueriesContext(connection) as ctx: resp = app.post_json('/api/agendas/events/fillslots/?agendas=%s' % agenda_slugs, params=params) - assert len(ctx.captured_queries) == 16 + assert len(ctx.captured_queries) == 17 assert resp.json['booking_count'] == 2 assert len(resp.json['booked_events']) == 2 assert resp.json['booked_events'][0]['id'] == 'first-agenda@event' @@ -648,3 +648,49 @@ def test_api_events_fillslots_multiple_agendas_shared_custody(app, user): ) assert resp.json['err'] == 1 assert resp.json['err_desc'] == 'Some events are outside guardian custody: first-agenda@event-thursday' + + +@pytest.mark.freeze_time('2021-09-06 12:00') +def test_api_events_fillslots_multiple_agendas_overlapping_events(app, user, freezer): + agenda = Agenda.objects.create(label='Foo bar', kind='events') + Event.objects.create( + label='Event', + start_datetime=now() + datetime.timedelta(days=5), + duration=120, + places=5, + agenda=agenda, + ) + second_agenda = Agenda.objects.create(label='Foo bar 2', kind='events') + Event.objects.create( + label='Event 2', + start_datetime=now() + datetime.timedelta(days=5, hours=1), + duration=120, + places=5, + agenda=second_agenda, + ) + + app.authorization = ('Basic', ('john.doe', 'password')) + fillslots_url = '/api/agendas/events/fillslots/?agendas=%s' + resp = app.post_json( + fillslots_url % ','.join((agenda.slug, second_agenda.slug)), + params={ + 'user_external_id': 'user_id', + 'check_overlaps': True, + 'slots': 'foo-bar@event,foo-bar-2@event-2', + }, + ) + assert resp.json['err'] == 1 + assert resp.json['err_desc'] == 'Some events occur at the same time: Event, Event 2' + + # events can be booked separately + resp = app.post_json( + fillslots_url % agenda.slug, + params={'user_external_id': 'user_id', 'check_overlaps': True, 'slots': 'foo-bar@event'}, + ) + assert resp.json['booking_count'] == 1 + + resp = app.post_json( + fillslots_url % second_agenda.slug, + params={'user_external_id': 'user_id', 'check_overlaps': True, 'slots': 'foo-bar-2@event-2'}, + ) + assert resp.json['booking_count'] == 1 -- 2.30.2