Add BusShift feature

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.

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)

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]

padam_django/apps/fleet/factories.py

@@ -3,7 +3,6 @@
 
 from . import models
 
-
 fake = Faker(['fr'])
 
 

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

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"]

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

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)

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",
+                    ),
+                ),
+            ],
+        ),
+    ]

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

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]

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"

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