diff --git a/NOTES.md b/NOTES.md new file mode 100644 index 00000000..702e5477 --- /dev/null +++ b/NOTES.md @@ -0,0 +1,51 @@ +# Some notes that gather my thinking + +## Foreword + +Please, note that my main experience is with using Ruby on Rails so I used this exercise to learn +and explore Django along the way. Consider my small experience with the framework when reviewing +this code. In any case, whatever the framework, web dev is still web dev, and I found very similar +fundamental issues and patterns. + +## Actual notes + +App name : operations + +A BusStop is a place. A Place can be a BusStop, but also eventually other types such as TrainStop +for example. Having BusStop, and TrainStop as separate tables enable this multiplicity of function +for the same place. + +BusStop order is determined by arrival time, no need for an additional order column. + +One can be tempted to add departure_bus_shift_entry and arrival_bus_shift_entry to BusShift to +enforce it having at least two stops but it de-normalizes the data and create duplication with what +can already be determined with the set en entries. I think adding a validation only at the database +level is sane as it also enables to have a "WIP" shift. + +We might want to denormalize (arrival_time/duration) instead of recomputing things on the go, but +only if we actually hit performance issues. Correctness is more important than speed. + +To add proper database constraints, one could use PostgreSQL with the `tstzrange` type +(https://www.postgresql.org/docs/current/rangetypes.html) or compile SQLite with the R*Tree module +(https://sqlite.org/draft/rtree.html). Speaking with some experience in compiling and deploying +custom SQLite distributions for [MTG Translator](https://mtg-translator.net), I don't really want to +go into this trouble for the exercise, but it's a good thing to know. + +Simlpified `__str__` representation so that it's more natural to read about on the admin interface +and it avoids some N+1 queries. + +I'm fighting a bit with N+1 queries in the admin that are _everywhere_. Having to use custom +querysets to preload data and fix the issue kinda defeat the purpose of an easy to use admin +interface builder and I feel like Django could improve a lot on that. Coming from Rails, I didn't +expected this to be so convoluted. I like the concept of query Managers though. +EDIT: I think I've come to a satisfying solution with using different managers in different contexts +and letting the caller (that knows what objects and attributes thet want) to use the appropriate +context manager + +I'm also having some troubles with the validation. There are multiple ways of validating in Django. +Which is the right one? I've decided on considering the shift overlap and minimum number of stops as +business specific validation that do not pose a risk on data integrity and as such I don't want to +force a complex validation at the database or model level and it's the caller's responsibility to +validate business rules. In our case, it's the Admin interface using a custom FormSet with a clean() +method that does the heavy lifting. It could be refactored into a dedicated Validator to use in +other places. diff --git a/padam_django/apps/common/management/commands/create_data.py b/padam_django/apps/common/management/commands/create_data.py index a149a937..929d96bd 100644 --- a/padam_django/apps/common/management/commands/create_data.py +++ b/padam_django/apps/common/management/commands/create_data.py @@ -12,3 +12,5 @@ def handle(self, *args, **options): management.call_command('create_drivers', number=5) management.call_command('create_buses', number=10) management.call_command('create_places', number=30) + management.call_command('create_bus_stops', number=30) + management.call_command('create_bus_shifts', number=10) diff --git a/padam_django/apps/fleet/admin.py b/padam_django/apps/fleet/admin.py index 3fba5023..248e48a8 100644 --- a/padam_django/apps/fleet/admin.py +++ b/padam_django/apps/fleet/admin.py @@ -1,13 +1,37 @@ from django.contrib import admin +from padam_django.apps.operations.models import BusShift + from . import models +class BusShiftInline(admin.TabularInline): + model = BusShift + extra = 0 + show_change_link = True + + def get_queryset(self, _request): + return BusShift.with_buses_and_drivers + + # It doesn't make sense to enable adding a new shift to a Driver/Bus without the appropriate + # form so better disable that + def has_add_permission(self, _request, _obj): + return False + + # Same + def has_change_permission(self, _request, _obj): + return False + + @admin.register(models.Bus) class BusAdmin(admin.ModelAdmin): - pass + list_display = ["licence_plate"] + search_fields = ["licence_plate"] + inlines = [BusShiftInline] @admin.register(models.Driver) class DriverAdmin(admin.ModelAdmin): - pass + list_display = ["user"] + search_fields = ["user__username", "user__first_name", "user__last_name"] + inlines = [BusShiftInline] diff --git a/padam_django/apps/fleet/factories.py b/padam_django/apps/fleet/factories.py index c78c832e..9904bcbd 100644 --- a/padam_django/apps/fleet/factories.py +++ b/padam_django/apps/fleet/factories.py @@ -3,7 +3,6 @@ from . import models - fake = Faker(['fr']) diff --git a/padam_django/apps/fleet/models.py b/padam_django/apps/fleet/models.py index 4cd3f19d..5022d01d 100644 --- a/padam_django/apps/fleet/models.py +++ b/padam_django/apps/fleet/models.py @@ -2,10 +2,14 @@ class Driver(models.Model): - user = models.OneToOneField('users.User', on_delete=models.CASCADE, related_name='driver') + user = models.OneToOneField( + "users.User", on_delete=models.CASCADE, related_name="driver" + ) def __str__(self): - return f"Driver: {self.user.username} (id: {self.pk})" + # This triggers N+1 by default, but let's assume that loading Driver objects without loading + # their related User is useless anyway so it's a good compromise for readability + return f"{self.user.first_name} {self.user.last_name}" class Bus(models.Model): @@ -15,4 +19,4 @@ class Meta: verbose_name_plural = "Buses" def __str__(self): - return f"Bus: {self.licence_plate} (id: {self.pk})" + return self.licence_plate diff --git a/padam_django/apps/geography/admin.py b/padam_django/apps/geography/admin.py index e0334458..0fa78d18 100644 --- a/padam_django/apps/geography/admin.py +++ b/padam_django/apps/geography/admin.py @@ -5,4 +5,11 @@ @admin.register(models.Place) class PlaceAdmin(admin.ModelAdmin): - pass + list_display = ["name", "longitude", "latitude"] + search_fields = ["name"] + + +@admin.register(models.BusStop) +class BusStopAdmin(admin.ModelAdmin): + list_display = ["place"] + search_fields = ["place__name"] diff --git a/padam_django/apps/geography/factories.py b/padam_django/apps/geography/factories.py index b134a30c..6ad45df2 100644 --- a/padam_django/apps/geography/factories.py +++ b/padam_django/apps/geography/factories.py @@ -15,3 +15,10 @@ class PlaceFactory(factory.django.DjangoModelFactory): class Meta: model = models.Place + + +class BusStopFactory(factory.django.DjangoModelFactory): + place = factory.SubFactory(PlaceFactory) + + class Meta: + model = models.BusStop diff --git a/padam_django/apps/geography/management/commands/create_bus_stops.py b/padam_django/apps/geography/management/commands/create_bus_stops.py new file mode 100644 index 00000000..c9cc2313 --- /dev/null +++ b/padam_django/apps/geography/management/commands/create_bus_stops.py @@ -0,0 +1,12 @@ +from padam_django.apps.common.management.base import CreateDataBaseCommand + +from padam_django.apps.geography.factories import BusStopFactory + + +class Command(CreateDataBaseCommand): + help = "Create few bus stops" + + def handle(self, *args, **options): + super().handle(*args, **options) + self.stdout.write(f"Creating {self.number} bus stops ...") + BusStopFactory.create_batch(size=self.number) diff --git a/padam_django/apps/geography/migrations/0002_busstop.py b/padam_django/apps/geography/migrations/0002_busstop.py new file mode 100644 index 00000000..98bad532 --- /dev/null +++ b/padam_django/apps/geography/migrations/0002_busstop.py @@ -0,0 +1,27 @@ +# Generated by Django 4.2.16 on 2026-07-10 14:43 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + dependencies = [ + ("geography", "0001_initial"), + ] + + operations = [ + migrations.CreateModel( + name="BusStop", + fields=[ + ( + "place", + models.OneToOneField( + on_delete=django.db.models.deletion.PROTECT, + primary_key=True, + serialize=False, + to="geography.place", + ), + ), + ], + ), + ] diff --git a/padam_django/apps/geography/models.py b/padam_django/apps/geography/models.py index e566ee2b..b151db1a 100644 --- a/padam_django/apps/geography/models.py +++ b/padam_django/apps/geography/models.py @@ -12,4 +12,13 @@ class Meta: unique_together = (("longitude", "latitude"), ) def __str__(self): - return f"Place: {self.name} (id: {self.pk})" + return self.name + + +class BusStop(models.Model): + place = models.OneToOneField(Place, models.PROTECT, primary_key=True) + + def __str__(self): + # This triggers N+1 by default, but let's assume that loading BusStop objects without loading + # their related Place is useless anyway so it's a good compromise for readability + return self.place.name diff --git a/padam_django/apps/operations/__init__.py b/padam_django/apps/operations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/padam_django/apps/operations/admin.py b/padam_django/apps/operations/admin.py new file mode 100644 index 00000000..cc231954 --- /dev/null +++ b/padam_django/apps/operations/admin.py @@ -0,0 +1,127 @@ +from django.contrib import admin +from django.db.models import Max, Min +from django.forms import ValidationError, models + +from padam_django.apps.operations import bus_shift + +from .models import BusShift, BusShiftEntry + + +class BusShiftEntryInlineFormSet(models.BaseInlineFormSet): + # TODO: this is kind of messy and shoud be refactored AND extracted into a proper validator + def clean(self): + super().clean() + + shift = self.instance + + shift_entries = [ + BusShiftEntry( + bus_shift_id=form.cleaned_data["bus_shift"], + bus_stop_id=form.cleaned_data["bus_stop"], + arrival_time=form.cleaned_data["arrival_time"], + ) + for form in self.forms + if form.is_valid() + and form.cleaned_data + and not form.cleaned_data.get("DELETE") + ] + + if len(shift_entries) < BusShift.MIN_ENTRIES_COUNT: + raise ValidationError("A BusShift must have at least 2 entries") + + # NOTE: we can't use the model properties here since the association isn't persisted yet + sorted_shift_entries = sorted( + shift_entries, key=lambda entry: entry.arrival_time + ) + departure_time = sorted_shift_entries[0].arrival_time + arrival_time = sorted_shift_entries[-1].arrival_time + + errors = [] + + # TODO: better BusShift manager that supports more granular control about loading patterns + # that could be used here + is_driver_already_on_shift = ( + BusShift.objects.annotate( + dep_time=Min("entries__arrival_time"), + arr_time=Max("entries__arrival_time"), + ) + .filter( + driver=shift.driver, + arr_time__gt=departure_time, + dep_time__lt=arrival_time, + ) + .exclude(pk=shift.pk) + .exists() + ) + + is_bus_already_on_shift = ( + BusShift.objects.annotate( + dep_time=Min("entries__arrival_time"), + arr_time=Max("entries__arrival_time"), + ) + .filter( + bus=shift.bus, + arr_time__gt=departure_time, + dep_time__lt=arrival_time, + ) + .exclude(pk=shift.pk) + .exists() + ) + + if is_driver_already_on_shift: + errors.append(ValidationError("Driver already on shift")) + if is_bus_already_on_shift: + errors.append(ValidationError("Bus already on shift")) + if len(errors) > 0: + raise ValidationError(errors) + + +class BusShiftEntryInline(admin.TabularInline): + model = BusShiftEntry + formset = BusShiftEntryInlineFormSet + extra = 0 + ordering = ["arrival_time"] + + autocomplete_fields = ["bus_stop"] + + +@admin.register(BusShift) +class BusShiftAdmin(admin.ModelAdmin): + list_display = [ + "__str__", + "bus", + "driver", + bus_shift.departure_stop, + bus_shift.departure_time, + bus_shift.arrival_stop, + bus_shift.arrival_time, + bus_shift.duration, + ] + list_display_links = [ + "__str__", + "bus", + "driver", + ] + + search_fields = [ + "bus__licence_plate", + "driver__user__username", + "driver__user__first_name", + "driver__user__last_name", + ] + list_per_page = 15 + + def get_queryset(self, request): + return BusShift.with_loaded_entries + + autocomplete_fields = ["bus", "driver"] + readonly_fields = [ + bus_shift.departure_stop, + bus_shift.departure_stop, + bus_shift.departure_time, + bus_shift.arrival_stop, + bus_shift.arrival_time, + bus_shift.duration, + ] + + inlines = [BusShiftEntryInline] diff --git a/padam_django/apps/operations/apps.py b/padam_django/apps/operations/apps.py new file mode 100644 index 00000000..0add8490 --- /dev/null +++ b/padam_django/apps/operations/apps.py @@ -0,0 +1,6 @@ +from django.apps import AppConfig + + +class OperationsConfig(AppConfig): + default_auto_field = "django.db.models.BigAutoField" + name = "padam_django.apps.operations" diff --git a/padam_django/apps/operations/bus_shift.py b/padam_django/apps/operations/bus_shift.py new file mode 100644 index 00000000..025db1b3 --- /dev/null +++ b/padam_django/apps/operations/bus_shift.py @@ -0,0 +1,35 @@ +""" +Collection of domain-related functions to work with BusShift +""" + + +def departure(shift): + return min(shift.entries.all(), key=__sort_key) + + +def arrival(shift): + return max(shift.entries.all(), key=__sort_key) + + +def departure_stop(shift): + return departure(shift).bus_stop + + +def arrival_stop(shift): + return arrival(shift).bus_stop + + +def departure_time(shift): + return departure(shift).arrival_time + + +def arrival_time(shift): + return arrival(shift).arrival_time + + +def duration(shift): + return arrival_time(shift) - departure_time(shift) + + +def __sort_key(entry): + return entry.arrival_time diff --git a/padam_django/apps/operations/factories.py b/padam_django/apps/operations/factories.py new file mode 100644 index 00000000..38991c44 --- /dev/null +++ b/padam_django/apps/operations/factories.py @@ -0,0 +1,30 @@ +import factory +from faker import Faker +from datetime import timedelta + +from django.utils import timezone + +from . import models + +fake = Faker(["fr"]) + + +class BusShiftFactory(factory.django.DjangoModelFactory): + bus = factory.SubFactory("padam_django.apps.fleet.factories.BusFactory") + driver = factory.SubFactory("padam_django.apps.fleet.factories.DriverFactory") + + class Meta: + model = models.BusShift + + +class BusShiftEntryFactory(factory.django.DjangoModelFactory): + bus_shift = factory.SubFactory(BusShiftFactory) + bus_stop = factory.SubFactory( + "padam_django.apps.geography.factories.BusStopFactory" + ) + arrival_time = factory.LazyFunction( + lambda: timezone.now() + timedelta(hours=fake.random_int(min=1, max=24)) + ) + + class Meta: + model = models.BusShiftEntry diff --git a/padam_django/apps/operations/management/__init__.py b/padam_django/apps/operations/management/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/padam_django/apps/operations/management/commands/__init__.py b/padam_django/apps/operations/management/commands/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/padam_django/apps/operations/management/commands/create_bus_shifts.py b/padam_django/apps/operations/management/commands/create_bus_shifts.py new file mode 100644 index 00000000..3b728301 --- /dev/null +++ b/padam_django/apps/operations/management/commands/create_bus_shifts.py @@ -0,0 +1,18 @@ +from padam_django.apps.common.management.base import CreateDataBaseCommand + +from padam_django.apps.operations.factories import BusShiftFactory, BusShiftEntryFactory + + +class Command(CreateDataBaseCommand): + help = "Create few bus shifts" + + def handle(self, *args, **options): + super().handle(*args, **options) + self.stdout.write(f"Creating {self.number} bus shifts ...") + + # Create bus shifts with entries + bus_shifts = BusShiftFactory.create_batch(size=self.number) + + # Add entries to each shift (minimum 2 per shift) + for shift in bus_shifts: + BusShiftEntryFactory.create_batch(size=2, bus_shift=shift) diff --git a/padam_django/apps/operations/migrations/0001_initial.py b/padam_django/apps/operations/migrations/0001_initial.py new file mode 100644 index 00000000..b36d0608 --- /dev/null +++ b/padam_django/apps/operations/migrations/0001_initial.py @@ -0,0 +1,83 @@ +# Generated by Django 4.2.16 on 2026-07-10 14:43 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + initial = True + + dependencies = [ + ("fleet", "0002_auto_20211109_1456"), + ("geography", "0002_busstop"), + ] + + operations = [ + migrations.CreateModel( + name="BusShift", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "bus", + models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, + related_name="shifts", + to="fleet.bus", + ), + ), + ( + "driver", + models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, + related_name="shifts", + to="fleet.driver", + ), + ), + ], + ), + migrations.CreateModel( + name="BusShiftEntry", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("arrival_time", models.DateTimeField()), + ( + "bus_shift", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="entries", + to="operations.busshift", + ), + ), + ( + "bus_stop", + models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, + related_name="shift_entries", + to="geography.busstop", + ), + ), + ], + ), + migrations.AddConstraint( + model_name="busshiftentry", + constraint=models.UniqueConstraint( + fields=("bus_shift", "bus_stop"), name="unique_bus_stop_per_shift" + ), + ), + ] diff --git a/padam_django/apps/operations/migrations/__init__.py b/padam_django/apps/operations/migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/padam_django/apps/operations/models.py b/padam_django/apps/operations/models.py new file mode 100644 index 00000000..09550018 --- /dev/null +++ b/padam_django/apps/operations/models.py @@ -0,0 +1,78 @@ +from django.db import models + + +class BusShiftWithBusesAndDrivers(models.Manager): + def get_queryset(self): + return super().get_queryset().select_related("bus", "driver") + + +class BusShiftWithLoadedEntriesManager(models.Manager): + """ + BusShiftWithLoadedEntriesManager loads a collection of BusShift and associated BusShiftEntry + objects so that useful work can be done with them without trigerring N+1 queries. + + It traverses the association hierarchy and efficiently retrieves the following: + BusShift -> BusShiftEntry -> BusStop -> Place + -> Driver -> User + + Note that by default BusShift objects are ordered by departure time and BusShiftEntry objects + are ordered by arrival time + """ + + def get_queryset(self): + return ( + super() + .get_queryset() + .prefetch_related( + models.Prefetch( + "entries", + queryset=BusShiftEntry.objects.select_related( + "bus_stop", + "bus_stop__place", + "bus_shift", + "bus_shift__driver", + "bus_shift__driver__user", + ).order_by("arrival_time"), + ) + ) + .annotate( + dep_time=models.Min("entries__arrival_time"), + arr_time=models.Max("entries__arrival_time"), + ) + .order_by("-dep_time", "-arr_time") + ) + + +class BusShift(models.Model): + MIN_ENTRIES_COUNT = 2 + + bus = models.ForeignKey("fleet.Bus", models.PROTECT, related_name="shifts") + driver = models.ForeignKey("fleet.Driver", models.PROTECT, related_name="shifts") + + objects = models.Manager() + with_buses_and_drivers = BusShiftWithBusesAndDrivers() + with_loaded_entries = BusShiftWithLoadedEntriesManager() + + def __str__(self): + return f"BusShift #{self.pk}" + + +class BusShiftEntry(models.Model): + bus_shift = models.ForeignKey(BusShift, models.CASCADE, related_name="entries") + bus_stop = models.ForeignKey( + "geography.BusStop", models.PROTECT, related_name="shift_entries" + ) + arrival_time = models.DateTimeField() + + class Meta: + verbose_name = "Bus Shift Entry" + verbose_name_plural = "Bus Shift Entries" + + constraints = [ + models.UniqueConstraint( + fields=["bus_shift", "bus_stop"], name="unique_bus_stop_per_shift" + ) + ] + + def __str__(self): + return f"BusShiftEntry #{self.pk} ({self.arrival_time})" diff --git a/padam_django/settings.py b/padam_django/settings.py index 129e922c..d4a6979c 100644 --- a/padam_django/settings.py +++ b/padam_django/settings.py @@ -12,6 +12,9 @@ from pathlib import Path + +from django.utils.log import DEFAULT_LOGGING + # Build paths inside the project like this: BASE_DIR / 'subdir'. BASE_DIR = Path(__file__).resolve().parent.parent @@ -45,6 +48,7 @@ 'padam_django.apps.fleet', 'padam_django.apps.geography', 'padam_django.apps.users', + 'padam_django.apps.operations', ] MIDDLEWARE = [ @@ -136,3 +140,16 @@ # https://docs.djangoproject.com/en/3.2/ref/settings/#default-auto-field DEFAULT_AUTO_FIELD = 'django.db.models.BigAutoField' + +# Enable SQL requests logging to hunt N+1 queries +LOGGING = DEFAULT_LOGGING | { + 'handlers': { + 'console': DEFAULT_LOGGING['handlers']['console'] | {'level': 'DEBUG'} + }, + 'loggers': { + 'django.db.backends': { + 'handlers': ['console'], + 'level': 'DEBUG', + } + }, +}