From ce88b58003a983dc2487378128212e8ad6094bc8 Mon Sep 17 00:00:00 2001 From: Antoine Pelletier Date: Mon, 24 Aug 2026 17:23:10 +0200 Subject: [PATCH] wip --- .env | 13 + ...0260824180000_reservation_bike_periods.sql | 25 ++ db/schema.sql | 8 +- db/seed.sql | 31 +- frontend/src/components/admin/BikeFleet.vue | 8 +- .../src/components/admin/ReservationCard.vue | 100 +++++- .../components/reservation/BikeCalendar.vue | 26 +- .../reservation/ReservationDetails.vue | 34 ++- .../reservation/ReservationEditDialog.vue | 284 +++++++++++++++++- frontend/src/lib/api.d.ts | 144 ++++++++- frontend/src/locales/en.yml | 26 ++ frontend/src/locales/fr.yml | 26 ++ frontend/src/services/api/client.ts | 15 +- frontend/src/services/api/reservations.ts | 51 +++- frontend/src/utils/types.ts | 20 ++ frontend/src/views/ReservationView.vue | 42 ++- src/api/reservations.rs | 94 ++++-- src/core/controller/reservations.rs | 282 +++++++++++++++-- src/core/models/reservation.rs | 89 +++++- .../repositories/reservations_repository.rs | 9 +- src/services/database/reservations.rs | 162 ++++++++-- src/services/mod.rs | 1 + src/services/telegram.rs | 233 ++++++++++++++ src/utils/config.rs | 54 ++++ 24 files changed, 1645 insertions(+), 132 deletions(-) create mode 100644 db/migrations/20260824180000_reservation_bike_periods.sql create mode 100644 src/services/telegram.rs diff --git a/.env b/.env index d5cf572..7c01ab6 100644 --- a/.env +++ b/.env @@ -1,2 +1,15 @@ # This file is used by dbmate, and by the sqlx macros at compile time. DATABASE_URL=postgres://postgres:postgres@localhost:5432/cargagep?sslmode=disable + +# Read by the backend too: anything named APP__* here reaches the configuration +# exactly like an exported variable would, and a real environment variable still +# wins over this file. This is where the secrets live in development. +# +# The bot that posts to the cargobikes group. Leave both empty and nothing is +# sent — which is what a machine without the bot wants. +# * BOT_TOKEN comes from @BotFather +# * CHAT_ID is the group's id: add the bot to the group, post a message, then +# read it from https://api.telegram.org/bot/getUpdates (a group id is +# negative, e.g. -1001234567890) +APP__TELEGRAM__BOT_TOKEN= +APP__TELEGRAM__CHAT_ID= diff --git a/db/migrations/20260824180000_reservation_bike_periods.sql b/db/migrations/20260824180000_reservation_bike_periods.sql new file mode 100644 index 0000000..f6a96c8 --- /dev/null +++ b/db/migrations/20260824180000_reservation_bike_periods.sql @@ -0,0 +1,25 @@ +-- migrate:up + +-- A bike is normally held for its reservation's own period. These two columns +-- override it for one bike, which is how a conflict is resolved without moving +-- the whole booking: the reservation keeps its hours, and only the bike both +-- bookings want changes hands earlier. +-- +-- NULL means "the reservation's period", so every existing row keeps behaving +-- exactly as before. +ALTER TABLE reservations_bikes + ADD COLUMN start_time timestamptz, + ADD COLUMN end_time timestamptz; + +-- Both or neither, and the right way round +ALTER TABLE reservations_bikes ADD CONSTRAINT reservations_bikes_period CHECK ( + (start_time IS NULL AND end_time IS NULL) + OR (start_time IS NOT NULL AND end_time IS NOT NULL AND start_time < end_time)); + +-- Conflicts are looked up by bike — "who else holds this one, and when" — which +-- `reservations_bikes_bike_id_idx` already serves: it was created with the +-- table, and the primary key, starting with reservation_id, could not. + +-- migrate:down +ALTER TABLE reservations_bikes DROP CONSTRAINT reservations_bikes_period; +ALTER TABLE reservations_bikes DROP COLUMN start_time, DROP COLUMN end_time; diff --git a/db/schema.sql b/db/schema.sql index c182d8d..8f44085 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -130,7 +130,10 @@ CREATE TABLE public.reservations ( CREATE TABLE public.reservations_bikes ( reservation_id integer NOT NULL, - bike_id integer NOT NULL + bike_id integer NOT NULL, + start_time timestamp with time zone, + end_time timestamp with time zone, + CONSTRAINT reservations_bikes_period CHECK ((((start_time IS NULL) AND (end_time IS NULL)) OR ((start_time IS NOT NULL) AND (end_time IS NOT NULL) AND (start_time < end_time)))) ); @@ -505,4 +508,5 @@ INSERT INTO public.schema_migrations (version) VALUES ('20260823210000'), ('20260823230000'), ('20260824120000'), - ('20260824140000'); + ('20260824140000'), + ('20260824180000'); diff --git a/db/seed.sql b/db/seed.sql index 4d537c0..6e07733 100644 --- a/db/seed.sql +++ b/db/seed.sql @@ -81,18 +81,41 @@ FROM (VALUES date_trunc('day', now()) + interval '6 days' + interval '18 hours', 2, '@bob_dupont', 'Annulée faute de conducteur', ARRAY['bob.dupont@epfl.ch'], - 'cancelled'::reservation_status) + 'cancelled'::reservation_status), + -- Wants the 5000 while reservation 2 still holds it, so the admin page has + -- a conflict to warn about without anybody having to build one by hand + (7, 'S4S', + date_trunc('day', now()) + interval '1 day' + interval '14 hours', + date_trunc('day', now()) + interval '2 days' + interval '10 hours', + 1, '@alice_martin', 'Stand de la journée portes ouvertes', + ARRAY['alice.martin@epfl.ch'], + 'requested'::reservation_status) ) AS r (id, unit_name, start_time, end_time, requester_id, telegram, "description", linka_emails, status) JOIN public.units u ON u."name" = r.unit_name ON CONFLICT DO NOTHING; INSERT INTO public.reservations_users (reservation_id, user_id) VALUES - (1, 1), (1, 2), (2, 3), (3, 2), (3, 1), (4, 3), (5, 1), (6, 2) + (1, 1), (1, 2), (2, 3), (3, 2), (3, 1), (4, 3), (5, 1), (6, 2), (7, 1) ON CONFLICT DO NOTHING; -INSERT INTO public.reservations_bikes (reservation_id, bike_id) VALUES - (1, 1), (1, 2), (2, 3), (3, 1), (3, 4), (4, 2), (5, 5), (6, 4) +-- The bikes each reservation holds. The two period columns are normally NULL, +-- which means "the reservation's own period"; they are only filled when a bike +-- is handed over early to settle a conflict, as reservation 1 does with the +-- 2000 — that one is given back at noon today rather than tomorrow evening. +INSERT INTO public.reservations_bikes (reservation_id, bike_id, start_time, end_time) VALUES + (1, 1, NULL::timestamptz, NULL::timestamptz), + (1, 2, date_trunc('day', now()) - interval '1 day' + interval '8 hours', + date_trunc('day', now()) + interval '12 hours'), + (2, 3, NULL, NULL), + -- Held by reservation 2, and wanted by reservation 7 at the same time + (2, 5, NULL, NULL), + (3, 1, NULL, NULL), + (3, 4, NULL, NULL), + (4, 2, NULL, NULL), + (5, 5, NULL, NULL), + (6, 4, NULL, NULL), + (7, 5, NULL, NULL) ON CONFLICT DO NOTHING; -- Keep the sequences in sync with the explicit ids inserted above diff --git a/frontend/src/components/admin/BikeFleet.vue b/frontend/src/components/admin/BikeFleet.vue index 95d9255..075e262 100644 --- a/frontend/src/components/admin/BikeFleet.vue +++ b/frontend/src/components/admin/BikeFleet.vue @@ -26,8 +26,14 @@ const setStatus = useSetBikeStatus() /** Bikes held by a reservation that is under way right now */ const inUse = computed(() => { const ids = new Set() + const now = Date.now() for (const reservation of props.reservations) { - if (reservation.status === 'ongoing') reservation.bikes.forEach((id) => ids.add(id)) + if (reservation.status !== 'ongoing') continue + // A bike handed over early is no longer in use, even though the + // reservation it belonged to is still running + for (const bike of reservation.bikes) { + if (new Date(bike.end_time).getTime() > now) ids.add(bike.id) + } } return ids }) diff --git a/frontend/src/components/admin/ReservationCard.vue b/frontend/src/components/admin/ReservationCard.vue index 08ba5e9..e5ecdab 100644 --- a/frontend/src/components/admin/ReservationCard.vue +++ b/frontend/src/components/admin/ReservationCard.vue @@ -6,15 +6,23 @@ */ import { computed, ref } from 'vue' import { useI18n } from 'vue-i18n' -import { Pencil } from '@lucide/vue' +import { Pencil, TriangleAlert } from '@lucide/vue' import { toast } from 'vue-sonner' import ReservationDetails from '@/components/reservation/ReservationDetails.vue' import ReservationEditDialog from '@/components/reservation/ReservationEditDialog.vue' import { Badge } from '@/components/ui/badge' import { Button } from '@/components/ui/button' -import { useSetReservationStatus } from '@/services/api/reservations' -import { unitLabel, type Bike, type Reservation, type ReservationStatus } from '@/utils/types' +import { useConflicts, useSetReservationStatus } from '@/services/api/reservations' +import { HttpStatus } from 'http-status-ts' + +import { + ApiError, + unitLabel, + type Bike, + type Reservation, + type ReservationStatus, +} from '@/utils/types' const props = defineProps<{ reservation: Reservation; bikes: Bike[] }>() @@ -44,6 +52,47 @@ const BADGE_CLASS: Record = { const transitions = computed(() => TRANSITIONS[props.reservation.status]) +/** + * What approving this reservation would clash with. + * + * Only asked while approving is on the table: an already approved one has + * taken its bikes, and a final one has given them back. The backend refuses + * the transition too — this is what says why, before the click. + */ +const canApprove = computed(() => transitions.value.includes('approved')) +const probe = computed(() => + canApprove.value + ? { + reservation: props.reservation.id, + start_time: props.reservation.start_time, + end_time: props.reservation.end_time, + bikes: props.reservation.bikes.map((held) => ({ + id: held.id, + start_time: held.start_time, + end_time: held.end_time, + })), + } + : null, +) +const { data: conflicts } = useConflicts(probe, { enabled: canApprove }) + +const blocking = computed(() => + (conflicts.value ?? []).map((conflict) => ({ + ...conflict, + name: props.bikes.find((bike) => bike.id === conflict.bike)?.name ?? `#${conflict.bike}`, + })), +) + +const { locale } = useI18n() +const formatter = computed( + () => + new Intl.DateTimeFormat(locale.value === 'fr' ? 'fr-CH' : 'en-GB', { + dateStyle: 'medium', + timeStyle: 'short', + }), +) +const format = (iso: string) => formatter.value.format(new Date(iso)) + /** A reservation nobody can act on any more is not worth an edit button */ const editable = computed( () => !['refused', 'cancelled', 'archived'].includes(props.reservation.status), @@ -51,9 +100,18 @@ const editable = computed( const editing = ref(false) function move(status: ReservationStatus) { + // The button is disabled while a conflict stands, but the list it was drawn + // from may be a few seconds old: the backend has the last word. setStatus.mutate( { id: props.reservation.id, status }, - { onError: () => toast.error(t('admin.reservations.error')) }, + { + onError: (error) => + toast.error( + error instanceof ApiError && error.status === HttpStatus.CONFLICT + ? t('admin.reservations.conflict.error') + : t('admin.reservations.error'), + ), + }, ) } @@ -79,7 +137,12 @@ function move(status: ReservationStatus) { :key="status" size="sm" :variant="status === 'approved' ? 'default' : 'outline'" - :disabled="setStatus.isPending.value" + :disabled="setStatus.isPending.value || (status === 'approved' && blocking.length > 0)" + :title=" + status === 'approved' && blocking.length + ? $t('admin.reservations.conflict.title') + : undefined + " @click="move(status)" > {{ $t(`admin.reservations.action.${status}`) }} @@ -87,6 +150,33 @@ function move(status: ReservationStatus) { + +
+

+ + {{ $t('admin.reservations.conflict.title') }} +

+

+ {{ + $t('admin.reservations.conflict.line', { + bike: conflict.name, + id: conflict.reservation, + unit: conflict.unit, + from: format(conflict.start_time), + to: format(conflict.end_time), + }) + }} +

+

{{ $t('admin.reservations.conflict.hint') }}

+
+ reservation.bikes.includes(bike.id)) - .flatMap((reservation) => { - const start = new Date(reservation.start_time) - const end = new Date(reservation.end_time) - const from = start > dayStart ? start : dayStart - const to = end < dayEnd ? end : dayEnd - if (to <= from) return [] + return booked.value.flatMap((reservation) => { + // The bike's own period, which is not always the reservation's: a + // conflict may have been settled by handing this one over early + const held = reservation.bikes.find((held) => held.id === bike.id) + if (!held) return [] + const start = new Date(held.start_time) + const end = new Date(held.end_time) + const from = start > dayStart ? start : dayStart + const to = end < dayEnd ? end : dayEnd + if (to <= from) return [] - const top = ((from.getTime() - dayStart.getTime()) / 3_600_000) * HOUR_HEIGHT - const height = Math.max(((to.getTime() - from.getTime()) / 3_600_000) * HOUR_HEIGHT, 6) - return [{ id: reservation.id, top, height }] - }) + const top = ((from.getTime() - dayStart.getTime()) / 3_600_000) * HOUR_HEIGHT + const height = Math.max(((to.getTime() - from.getTime()) / 3_600_000) * HOUR_HEIGHT, 6) + return [{ id: reservation.id, top, height }] + }) } diff --git a/frontend/src/components/reservation/ReservationDetails.vue b/frontend/src/components/reservation/ReservationDetails.vue index 3337da9..d3ffb78 100644 --- a/frontend/src/components/reservation/ReservationDetails.vue +++ b/frontend/src/components/reservation/ReservationDetails.vue @@ -34,10 +34,19 @@ function shows(field: Field) { const { locale } = useI18n() /** Names when we know the bike, `#id` when the list has not loaded yet */ -const bikeNames = computed(() => - props.reservation.bikes.map((id) => props.bikes.find((bike) => bike.id === id)?.name ?? `#${id}`), +function bikeName(id: number) { + return props.bikes.find((bike) => bike.id === id)?.name ?? `#${id}` +} + +const heldBikes = computed(() => + props.reservation.bikes.map((held) => ({ ...held, name: bikeName(held.id) })), ) +// A bike whose period is not the reservation's gets a line of its own: it is +// the whole point of the override, and hiding it would make the reservation +// look like it still holds the bike to the end. +const rescheduled = computed(() => heldBikes.value.filter((held) => held.custom)) + // The e-mail address is already on the reservation as a Linka Go account or in // the admin's own directory: the name is what tells the people apart here. const peopleNames = computed(() => @@ -77,17 +86,28 @@ function format(iso: string) { -
+
{{ $t('reservation.details.bikes') }}
- {{ name }} + {{ held.name }} + +
+
+ + {{ held.name }} · {{ format(held.start_time) }} → {{ format(held.end_time) }}
diff --git a/frontend/src/components/reservation/ReservationEditDialog.vue b/frontend/src/components/reservation/ReservationEditDialog.vue index 76c001b..59264ba 100644 --- a/frontend/src/components/reservation/ReservationEditDialog.vue +++ b/frontend/src/components/reservation/ReservationEditDialog.vue @@ -11,9 +11,11 @@ */ import { computed, reactive, ref, shallowRef, watch } from 'vue' import { useI18n } from 'vue-i18n' -import { Plus, X } from '@lucide/vue' +import { refDebounced } from '@vueuse/core' +import { CalendarClock, Plus, TriangleAlert, Undo2, X } from '@lucide/vue' import { CalendarDate, getLocalTimeZone, type DateValue } from '@internationalized/date' import { toast } from 'vue-sonner' +import { HttpStatus } from 'http-status-ts' import DatePicker from '@/components/DatePicker.vue' import TimePicker from '@/components/TimePicker.vue' @@ -30,8 +32,15 @@ import { } from '@/components/ui/dialog' import { Input } from '@/components/ui/input' import { Label } from '@/components/ui/label' -import { useUpdateReservation } from '@/services/api/reservations' -import { unitLabel, type Bike, type Reservation } from '@/utils/types' +import { useConflicts, useUpdateReservation } from '@/services/api/reservations' +import { + ApiError, + unitLabel, + type Bike, + type Conflict, + type NewReservationBike, + type Reservation, +} from '@/utils/types' const props = withDefaults( defineProps<{ reservation: Reservation; bikes: Bike[]; emailsOnly?: boolean }>(), @@ -39,7 +48,7 @@ const props = withDefaults( ) const open = defineModel('open', { default: false }) -const { t } = useI18n() +const { t, locale } = useI18n() const update = useUpdateReservation() const EMAIL_RE = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ @@ -56,6 +65,15 @@ const form = reactive({ const errors = reactive>({}) const submitted = ref(false) +/** + * A period one bike is held for, when it is not the reservation's own. This is + * how a conflict is settled: the booking keeps its hours and the disputed bike + * changes hands earlier. A bike absent from here simply follows the + * reservation. + */ +type Period = { startDate: DateValue; startTime: string; endDate: DateValue; endTime: string } +const overrides = reactive>({}) + function toCalendarDate(date: Date): DateValue { return new CalendarDate(date.getFullYear(), date.getMonth() + 1, date.getDate()) } @@ -67,6 +85,17 @@ function toSlot(date: Date): string { return `${String(Math.floor(total / 60)).padStart(2, '0')}:${String(total % 60).padStart(2, '0')}` } +function toPeriod(from: string, to: string): Period { + const start = new Date(from) + const end = new Date(to) + return { + startDate: toCalendarDate(start), + startTime: toSlot(start), + endDate: toCalendarDate(end), + endTime: toSlot(end), + } +} + /** Reloads the fields from the reservation, so cancelling really cancels */ function load() { const start = new Date(props.reservation.start_time) @@ -75,7 +104,11 @@ function load() { endDate.value = toCalendarDate(end) form.startTime = toSlot(start) form.endTime = toSlot(end) - form.bikes = [...props.reservation.bikes] + form.bikes = props.reservation.bikes.map((held) => held.id) + Object.keys(overrides).forEach((key) => delete overrides[Number(key)]) + for (const held of props.reservation.bikes) { + if (held.custom) overrides[held.id] = toPeriod(held.start_time, held.end_time) + } form.emails = props.reservation.linka_emails.length ? [...props.reservation.linka_emails] : [''] Object.keys(errors).forEach((key) => delete errors[key]) submitted.value = false @@ -104,16 +137,118 @@ const end = computed(() => toDate(endDate.value, form.endTime)) * of service — the same rule the backend applies. */ function isBlocked(bike: Bike) { - return bike.status === 'out_of_service' && !props.reservation.bikes.includes(bike.id) + return ( + bike.status === 'out_of_service' && !props.reservation.bikes.some((held) => held.id === bike.id) + ) } function toggleBike(bike: Bike) { if (isBlocked(bike)) return const index = form.bikes.indexOf(bike.id) - if (index >= 0) form.bikes.splice(index, 1) - else form.bikes.push(bike.id) + if (index >= 0) { + form.bikes.splice(index, 1) + delete overrides[bike.id] + } else form.bikes.push(bike.id) } +function bikeName(id: number) { + return props.bikes.find((bike) => bike.id === id)?.name ?? `#${id}` +} + +/** The period a bike would be held for once saved, reservation's or its own */ +function heldPeriod(id: number): [Date, Date] | null { + const own = overrides[id] + const from = own ? toDate(own.startDate, own.startTime) : start.value + const to = own ? toDate(own.endDate, own.endTime) : end.value + return from && to ? [from, to] : null +} + +function useOwnPeriod(id: number) { + if (!start.value || !end.value) return + overrides[id] = toPeriod(start.value.toISOString(), end.value.toISOString()) +} + +function followReservation(id: number) { + delete overrides[id] +} + +/** The fleet in the shape the backend takes: no period means "the booking's" */ +const editedBikes = computed(() => + form.bikes.map((id) => { + const period = heldPeriod(id) + if (!overrides[id] || !period) return { id } + return { id, start_time: period[0].toISOString(), end_time: period[1].toISOString() } + }), +) + +/** + * What the reservation would clash with once saved. Answered by the backend + * against the whole table, and only a warning: whoever manages the reservation + * is allowed to double-book knowingly, and is the only one who can sort it out. + */ +const probe = computed(() => { + // Emails-only mode changes neither the period nor the fleet: nothing to check + if (props.emailsOnly || !start.value || !end.value || !form.bikes.length) return null + if (end.value <= start.value) return null + return { + reservation: props.reservation.id, + start_time: start.value.toISOString(), + end_time: end.value.toISOString(), + bikes: editedBikes.value, + } +}) +// Every picker change would otherwise be its own request +const debouncedProbe = refDebounced(probe, 300) +const { data: conflicts } = useConflicts(debouncedProbe, { enabled: open }) + +/** One block per bike, since the same one can be held by two reservations */ +const conflictsByBike = computed(() => { + const grouped = new Map() + for (const conflict of conflicts.value ?? []) { + const list = grouped.get(conflict.bike) + if (list) list.push(conflict) + else grouped.set(conflict.bike, [conflict]) + } + return [...grouped].map(([id, list]) => ({ id, name: bikeName(id), conflicts: list })) +}) + +/** + * The one-click way out, when there is one: hand the bike back before the other + * booking takes it, or take it once that one is done. It only ever shortens our + * own hold, so it is always safe — and it is exactly what settles the case + * where two bookings want the same bike on either side of an hour. + */ +function resolution(id: number, conflict: Conflict): { at: Date; kind: 'end' | 'start' } | null { + const period = heldPeriod(id) + if (!period) return null + const [from, to] = period + const otherFrom = new Date(conflict.start_time) + const otherTo = new Date(conflict.end_time) + if (otherFrom > from) return { at: otherFrom, kind: 'end' } + if (otherTo < to) return { at: otherTo, kind: 'start' } + // The other booking covers ours whole: only moving the reservation, or the + // other one, can settle this + return null +} + +function applyResolution(id: number, conflict: Conflict) { + const fix = resolution(id, conflict) + const period = heldPeriod(id) + if (!fix || !period) return + const [from, to] = period + const next = fix.kind === 'end' ? [from, fix.at] : [fix.at, to] + overrides[id] = toPeriod(next[0].toISOString(), next[1].toISOString()) +} + +const timeFormatter = computed( + () => + new Intl.DateTimeFormat(locale.value === 'fr' ? 'fr-CH' : 'en-GB', { + dateStyle: 'medium', + timeStyle: 'short', + }), +) +const formatMoment = (value: Date | string) => timeFormatter.value.format(new Date(value)) + function addEmail() { form.emails.push('') } @@ -133,6 +268,20 @@ function validate(): boolean { errors.end = t('reservation.error-end-before-start') } if (form.bikes.length === 0) errors.bikes = t('reservation.error-no-bike') + + // The backend refuses these too; saying so here saves a round trip and + // names the bike rather than the whole reservation + for (const id of form.bikes) { + const own = overrides[id] + if (!own) continue + const from = toDate(own.startDate, own.startTime) + const to = toDate(own.endDate, own.endTime) + if (!from || !to || to <= from) { + errors.periods = t('reservation.edit.error-period', { bike: bikeName(id) }) + } else if (start.value && end.value && (from < start.value || to > end.value)) { + errors.periods = t('reservation.edit.error-period-outside', { bike: bikeName(id) }) + } + } } const emails = form.emails.map((email) => email.trim()).filter(Boolean) @@ -158,7 +307,12 @@ function save() { // the backend checks before accepting the change start_time: props.emailsOnly ? props.reservation.start_time : start.value!.toISOString(), end_time: props.emailsOnly ? props.reservation.end_time : end.value!.toISOString(), - bikes: props.emailsOnly ? [...props.reservation.bikes] : [...form.bikes], + bikes: props.emailsOnly + ? props.reservation.bikes.map((held) => ({ + id: held.id, + ...(held.custom ? { start_time: held.start_time, end_time: held.end_time } : {}), + })) + : editedBikes.value, linka_emails: form.emails.map((email) => email.trim()).filter(Boolean), }, { @@ -166,7 +320,12 @@ function save() { toast.success(t('reservation.edit.saved')) open.value = false }, - onError: (error) => toast.error(error.message || t('reservation.edit.error')), + onError: (error) => + toast.error( + error instanceof ApiError && error.status === HttpStatus.CONFLICT + ? t('reservation.conflicts.refused') + : error.message || t('reservation.edit.error'), + ), }, ) } @@ -254,6 +413,111 @@ function save() {

{{ errors.bikes }}

+ +
+ {{ $t('reservation.edit.periods') }} +
+
+ + {{ bikeName(id) }} + + {{ $t('reservation.edit.follows') }} + + + + +
+ +
+ + {{ $t('reservation.edit.period-from') }} + +
+ + +
+ + {{ $t('reservation.edit.period-to') }} + +
+ + +
+
+
+

{{ errors.periods }}

+
+ + +
+

+ + {{ $t('reservation.conflicts.title') }} +

+

{{ $t('reservation.conflicts.intro') }}

+
+
+

+ {{ group.name }} + — + {{ + $t('reservation.conflicts.held', { + unit: conflict.unit, + id: conflict.reservation, + from: formatMoment(conflict.start_time), + to: formatMoment(conflict.end_time), + }) + }} +

+ +
+
+
+
diff --git a/frontend/src/lib/api.d.ts b/frontend/src/lib/api.d.ts index 1811d44..66badbd 100644 --- a/frontend/src/lib/api.d.ts +++ b/frontend/src/lib/api.d.ts @@ -512,7 +512,7 @@ export interface paths { } content?: never } - /** @description One of the bikes is out of service */ + /** @description One of the bikes is out of service, or is already held over that period */ 409: { headers: { [name: string]: unknown @@ -644,6 +644,66 @@ export interface paths { patch?: never trace?: never } + '/api/reservations/conflicts': { + parameters: { + query?: never + header?: never + path?: never + cookie?: never + } + get?: never + put?: never + /** + * Which of the bikes wanted are already held over the same period + * @description Only approved and ongoing reservations hold a bike, so only they appear here. The booking form uses it to grey out what is taken, and the admin page to warn before double-booking on purpose. + */ + post: { + parameters: { + query?: never + header?: never + path?: never + cookie?: never + } + /** + * @description What a would-be booking asks about: a period, the bikes wanted, and the + * reservation to leave out of the answer when one is being edited. + */ + requestBody: { + content: { + 'application/json': components['schemas']['ConflictProbeForm'] + } + } + responses: { + 200: { + headers: { + [name: string]: unknown + } + content: { + 'application/json': components['schemas']['Conflict'][] + } + } + /** @description The period is empty or the wrong way round */ + 400: { + headers: { + [name: string]: unknown + } + content?: never + } + /** @description Unauthenticated - a session is required */ + 401: { + headers: { + [name: string]: unknown + } + content?: never + } + } + } + delete?: never + options?: never + head?: never + patch?: never + trace?: never + } '/api/reservations/{id}': { parameters: { query?: never @@ -712,7 +772,7 @@ export interface paths { } content?: never } - /** @description A newly added bike is out of service, or the reservation is final */ + /** @description A newly added bike is out of service or already held over that period (managers may double-book on purpose, so this only reaches anybody else), or the reservation is final */ 409: { headers: { [name: string]: unknown @@ -837,7 +897,8 @@ export interface components { * travels to anybody, signed in or not. */ CalendarReservation: { - bikes: number[] + /** @description With their own periods: a bike handed over early is drawn as such */ + bikes: components['schemas']['ReservationBike'][] /** Format: date-time */ end_time: string /** Format: int32 */ @@ -858,6 +919,43 @@ export interface components { /** Format: date-time */ to?: string | null } + /** + * @description A bike the probe wants that somebody else already holds over part of the + * same period. Only the reservations that actually hold a bike — approved and + * ongoing — can produce one. + */ + Conflict: { + /** Format: int32 */ + bike: number + /** Format: date-time */ + end_time: string + /** Format: int32 */ + reservation: number + /** + * Format: date-time + * @description When the bike is held, which is what a resolution has to step around + */ + start_time: string + status: components['schemas']['ReservationStatus'] + /** @description Who holds it, to be named in the warning */ + unit: string + } + /** + * @description What a would-be booking asks about: a period, the bikes wanted, and the + * reservation to leave out of the answer when one is being edited. + */ + ConflictProbeForm: { + bikes: components['schemas']['NewReservationBike'][] + /** Format: date-time */ + end_time: string + /** + * Format: int32 + * @description The reservation being edited, so it does not conflict with itself + */ + reservation?: number | null + /** Format: date-time */ + start_time: string + } DevUser: { admin: boolean email: string @@ -887,6 +985,18 @@ export interface components { unit: components['schemas']['NewReservationUnit'] users: number[] } + /** + * @description A bike as an edit carries it. `None` means "the reservation's period", which + * is what a bike nobody had to arbitrate over keeps. + */ + NewReservationBike: { + /** Format: date-time */ + end_time?: string | null + /** Format: int32 */ + id: number + /** Format: date-time */ + start_time?: string | null + } /** @description The same choice, as the client sends it: only the id travels for a known unit. */ NewReservationUnit: | { @@ -906,7 +1016,7 @@ export interface components { } Reservation: { description: string - bikes: number[] + bikes: components['schemas']['ReservationBike'][] /** Format: date-time */ end_time: string /** Format: int32 */ @@ -926,6 +1036,26 @@ export interface components { unit: components['schemas']['ReservationUnit'] users: components['schemas']['UserSummary'][] } + /** + * @description One bike on a reservation, with the period it is actually held for. + * + * Normally the reservation's own period. An override is how a conflict is + * settled without moving the whole booking: the reservation keeps its hours + * and only the disputed bike changes hands earlier. + */ + ReservationBike: { + /** @description Whether that period is the bike's own rather than the reservation's */ + custom: boolean + /** Format: date-time */ + end_time: string + /** Format: int32 */ + id: number + /** + * Format: date-time + * @description Already resolved: the bike's own period, or the reservation's + */ + start_time: string + } /** * @description Only what the admin page lets somebody change. The unit, the requester, the * telegram handle and the reason are shown but not editable, so they are not @@ -933,7 +1063,11 @@ export interface components { * rather than trusting a client to send them unchanged. */ ReservationEditForm: { - bikes: number[] + /** + * @description Each with its own period when a conflict was settled by handing it over + * early, and without one when it simply follows the reservation + */ + bikes: components['schemas']['NewReservationBike'][] /** Format: date-time */ end_time: string linka_emails: string[] diff --git a/frontend/src/locales/en.yml b/frontend/src/locales/en.yml index dfa5644..0b2def6 100644 --- a/frontend/src/locales/en.yml +++ b/frontend/src/locales/en.yml @@ -31,6 +31,7 @@ reservation: bikes-empty: No cargobike available for this period. bikes-error: Unable to load the cargobikes. bike-out-of-service: Out of service + bike-taken: Booked bike-size-large: Large cargo bikes bike-size-small: Small cargo bikes bike-size-empty: No bike of this size. @@ -72,8 +73,26 @@ reservation: reason: Reason show-more: 'Show {n} more' show-less: Show less + own-period: This cargobike's own period + conflicts: + title: Cargobike already booked + intro: >- + These cargobikes are already held by another reservation over the same period. + You can save anyway — the call is yours. + held: 'held by {unit} (#{id}) from {from} to {to}' + give-back: 'Give the {bike} back on {at}' + take-later: 'Take the {bike} from {at}' + refused: One of the cargobikes is already booked over that period. edit: action: Edit + periods: Period per cargobike + follows: follows the reservation + own-period: Different period + follow-again: Follow the reservation + period-from: Taken from + period-to: Given back on + error-period: 'The period for the {bike} is empty or the wrong way round.' + error-period-outside: "The period for the {bike} must stay inside the reservation's." title: 'Reservation #{id}' intro: >- Only the period, the cargobikes and the Linka Go accounts can be changed. @@ -162,6 +181,13 @@ admin: all-status: Any status count: No result | 1 reservation | {count} reservations empty: No reservation matches this search. + conflict: + title: 'Cannot approve: cargobike already booked' + line: 'The {bike} is already held by {unit} (#{id}), from {from} to {to}.' + hint: >- + Change that cargobike's period on either reservation, or drop it from this + one, then approve. + error: 'Approval refused: a cargobike is already booked over that period.' load-error: Unable to load the reservations. error: The status change failed. action: diff --git a/frontend/src/locales/fr.yml b/frontend/src/locales/fr.yml index fbb91b7..97c862c 100644 --- a/frontend/src/locales/fr.yml +++ b/frontend/src/locales/fr.yml @@ -32,6 +32,7 @@ reservation: bikes-empty: Aucun cargobike disponible pour ce créneau. bikes-error: Impossible de charger les cargobikes. bike-out-of-service: Hors service + bike-taken: Réservé bike-size-large: Grands cargos bike-size-small: Petits cargos bike-size-empty: Aucun vélo de cette taille. @@ -73,8 +74,26 @@ reservation: reason: Raison show-more: 'Afficher {n} de plus' show-less: Réduire + own-period: Période propre à ce cargobike + conflicts: + title: Cargobike déjà réservé + intro: >- + Ces cargobikes sont déjà tenus par une autre réservation sur la même plage. + Vous pouvez enregistrer quand même : à vous de trancher. + held: 'pris par {unit} (#{id}) du {from} au {to}' + give-back: 'Rendre le {bike} le {at}' + take-later: 'Prendre le {bike} à partir du {at}' + refused: Un des cargobikes est déjà réservé sur cette plage horaire. edit: action: Modifier + periods: Périodes par cargobike + follows: suit la réservation + own-period: Période différente + follow-again: Suivre la réservation + period-from: Pris à partir de + period-to: Rendu le + error-period: "La période du {bike} est vide ou à l'envers." + error-period-outside: 'La période du {bike} doit rester dans celle de la réservation.' title: 'Réservation #{id}' intro: >- Seuls la période, les cargobikes et les comptes Linka Go peuvent être modifiés. @@ -163,6 +182,13 @@ admin: all-status: Tous les statuts count: Aucun résultat | 1 réservation | {count} réservations empty: Aucune réservation ne correspond à cette recherche. + conflict: + title: 'Validation impossible : cargobike déjà réservé' + line: 'Le {bike} est déjà pris par {unit} (#{id}), du {from} au {to}.' + hint: >- + Modifiez la période de ce cargobike sur l'une des deux réservations, ou + retirez-le de celle-ci, puis validez. + error: 'Validation refusée : un cargobike est déjà réservé sur cette plage.' load-error: Impossible de charger les réservations. error: Le changement de statut a échoué. action: diff --git a/frontend/src/services/api/client.ts b/frontend/src/services/api/client.ts index df72c68..911f834 100644 --- a/frontend/src/services/api/client.ts +++ b/frontend/src/services/api/client.ts @@ -3,7 +3,7 @@ import { HttpStatus } from 'http-status-ts' import type { QueryClient } from '@tanstack/vue-query' import type { paths } from '@/lib/api' -import { Forbidden, Unauthorized } from '@/utils/types' +import { ApiError, Forbidden, Unauthorized } from '@/utils/types' import { SESSION_KEY } from './keys' // Registered by main.ts. The interceptor below needs it to drop the session @@ -41,8 +41,17 @@ client.use({ } else if (response.status === HttpStatus.NOT_FOUND) { throw new Error(`Not found: ${response.url}`) } - throw new Error( - `Unexpected error from ${response.url}: ${response.status} ${response.statusText}`, + // The handlers answer with a plain string; keeping it lets a view show + // what actually went wrong instead of a generic failure. Cloned, so + // nothing downstream finds the body already read. + const detail = await response + .clone() + .text() + .catch(() => '') + throw new ApiError( + response.status, + detail.trim() || + `Unexpected error from ${response.url}: ${response.status} ${response.statusText}`, ) } }, diff --git a/frontend/src/services/api/reservations.ts b/frontend/src/services/api/reservations.ts index 0586a8e..cfa046d 100644 --- a/frontend/src/services/api/reservations.ts +++ b/frontend/src/services/api/reservations.ts @@ -11,7 +11,14 @@ import { computed, toValue, type MaybeRefOrGetter } from 'vue' import { keepPreviousData, useMutation, useQuery, useQueryClient } from '@tanstack/vue-query' import { HttpStatus } from 'http-status-ts' -import type { NewReservation, Reservation, ReservationEdit, ReservationStatus } from '@/utils/types' +import type { + Conflict, + NewReservation, + NewReservationBike, + Reservation, + ReservationEdit, + ReservationStatus, +} from '@/utils/types' import { getClient } from './client' export const RESERVATIONS_KEY = ['reservations'] @@ -70,6 +77,48 @@ export function useReservations( }) } +export const CONFLICTS_KEY = ['reservations', 'conflicts'] + +/** The question a conflict check asks: a period, and the bikes wanted over it */ +export type ConflictProbe = { + /** The reservation being edited, so it does not conflict with itself */ + reservation?: number + start_time: string + end_time: string + bikes: NewReservationBike[] +} + +/** + * Which of the bikes wanted are already held by an approved or ongoing + * reservation over the same period. + * + * Overlap is a question about the whole table, so the database answers it: the + * browser never sees the bookings it is being compared against. A read behind a + * POST, because each bike may carry a period of its own — more than a query + * string can say. + */ +export function useConflicts( + probe: MaybeRefOrGetter, + options: { enabled?: MaybeRefOrGetter } = {}, +) { + const body = computed(() => toValue(probe)) + return useQuery({ + queryKey: computed(() => [...CONFLICTS_KEY, body.value]), + enabled: computed(() => (toValue(options.enabled) ?? true) && body.value !== null), + staleTime: 30 * 1000, + placeholderData: keepPreviousData, + queryFn: async () => { + const { data, response } = await getClient().POST('/api/reservations/conflicts', { + body: body.value!, + }) + if (response.status !== HttpStatus.OK) { + throw new Error(`Unexpected status code received: ${response.status}`) + } + return (data ?? []) as Conflict[] + }, + }) +} + /** * The backend re-checks the state machine and answers 409 on a transition it * does not allow, so the buttons only have to offer the plausible ones. diff --git a/frontend/src/utils/types.ts b/frontend/src/utils/types.ts index 62d65e4..ea7305d 100644 --- a/frontend/src/utils/types.ts +++ b/frontend/src/utils/types.ts @@ -18,6 +18,23 @@ export class Forbidden extends Error { } } +/** + * Any other refusal, with the status and whatever the handler wrote. The + * handlers answer with a plain string, and some of them — a bike taken in the + * meantime, a period the backend rejects — are worth showing rather than + * flattening into "unexpected error". + */ +export class ApiError extends Error { + constructor( + readonly status: number, + message: string, + ) { + super(message) + this.name = 'ApiError' + Object.setPrototypeOf(this, ApiError.prototype) + } +} + // Shorthands over the generated schemas, so views never import `@/lib/api` directly export type Bike = components['schemas']['Bike'] export type BikeStatus = components['schemas']['BikeStatus'] @@ -30,6 +47,9 @@ export type ReservationUnit = components['schemas']['ReservationUnit'] export type NewReservation = components['schemas']['NewReservation'] export type ReservationEdit = components['schemas']['ReservationEditForm'] export type CalendarReservation = components['schemas']['CalendarReservation'] +export type ReservationBike = components['schemas']['ReservationBike'] +export type NewReservationBike = components['schemas']['NewReservationBike'] +export type Conflict = components['schemas']['Conflict'] /** * What the details block can render: the fields the public calendar carries, diff --git a/frontend/src/views/ReservationView.vue b/frontend/src/views/ReservationView.vue index 1a92c1c..00125f0 100644 --- a/frontend/src/views/ReservationView.vue +++ b/frontend/src/views/ReservationView.vue @@ -4,6 +4,7 @@ import { useI18n } from 'vue-i18n' import { Plus, TriangleAlert, X } from '@lucide/vue' import { getLocalTimeZone, today, type DateValue } from '@internationalized/date' import { toast } from 'vue-sonner' +import { HttpStatus } from 'http-status-ts' import DatePicker from '@/components/DatePicker.vue' import TelegramInput from '@/components/TelegramInput.vue' @@ -16,9 +17,9 @@ import { Label } from '@/components/ui/label' import { Skeleton } from '@/components/ui/skeleton' import { Textarea } from '@/components/ui/textarea' import { useBikes } from '@/services/api/bikes' -import { useCreateReservation } from '@/services/api/reservations' +import { useConflicts, useCreateReservation } from '@/services/api/reservations' import { useSession } from '@/services/api/auth' -import type { Bike, NewReservation } from '@/utils/types' +import { ApiError, type Bike, type NewReservation } from '@/utils/types' const { t } = useI18n() @@ -76,8 +77,30 @@ const periodPicked = computed(() => start.value !== null && end.value !== null) const { data: bikes, isPending: bikesPending, isError: bikesError } = useBikes() +/** + * Which bikes an approved or ongoing reservation already holds over the period + * asked for. The overlap is worked out by the backend against the whole table: + * the browser is told "these are taken", not handed everybody's bookings to + * work it out itself. + */ +const probe = computed(() => { + if (!start.value || !end.value || end.value <= start.value) return null + return { + start_time: start.value.toISOString(), + end_time: end.value.toISOString(), + bikes: (bikes.value ?? []).map((bike) => ({ id: bike.id })), + } +}) +const { data: conflicts } = useConflicts(probe) +const taken = computed(() => new Set((conflicts.value ?? []).map((conflict) => conflict.bike))) + +function isTaken(bike: Bike) { + return taken.value.has(bike.id) +} + +/** Out of service or already booked: either way it cannot be picked */ function isUnavailable(bike: Bike) { - return bike.status === 'out_of_service' + return bike.status === 'out_of_service' || isTaken(bike) } const availableBikes = computed(() => (bikes.value ?? []).filter((bike) => !isUnavailable(bike))) @@ -192,7 +215,12 @@ function submit() { }, // The message is the backend's own: "bike 3 is out of service" is worth // reading, and a generic failure would hide it. - onError: (error) => toast.error(error.message || t('reservation.submit-error')), + onError: (error) => + toast.error( + error instanceof ApiError && error.status === HttpStatus.CONFLICT + ? t('reservation.conflicts.refused') + : error.message || t('reservation.submit-error'), + ), }) } @@ -348,7 +376,11 @@ function submit() { v-if="isUnavailable(bike)" class="text-destructive shrink-0 rounded px-1.5 py-0.5 text-[0.65rem] font-bold uppercase" > - {{ $t('reservation.bike-out-of-service') }} + {{ + bike.status === 'out_of_service' + ? $t('reservation.bike-out-of-service') + : $t('reservation.bike-taken') + }}
diff --git a/src/api/reservations.rs b/src/api/reservations.rs index aa48163..4e4c9d6 100644 --- a/src/api/reservations.rs +++ b/src/api/reservations.rs @@ -9,7 +9,7 @@ use aide::{ axum::{ ApiRouter, - routing::{get_with, put_with}, + routing::{get_with, post_with, put_with}, }, transform::TransformOperation, }; @@ -29,12 +29,10 @@ use crate::{ AnonAppController, AppController, ControllerError, reservations::ReservationsControllerError, }, - models::{ - bike::BikeId, - reservation::{ - CalendarReservation, NewReservation, Reservation, ReservationEdit, ReservationPage, - ReservationQuery, ReservationStatus, - }, + models::reservation::{ + CalendarReservation, Conflict, ConflictProbe, NewReservation, NewReservationBike, + Reservation, ReservationEdit, ReservationId, ReservationPage, ReservationQuery, + ReservationStatus, }, }, }; @@ -55,6 +53,7 @@ pub fn routes() -> ApiRouter { "/calendar", get_with(get_calendar_reservations, get_calendar_reservations_docs), ) + .api_route("/conflicts", post_with(find_conflicts, find_conflicts_docs)) .api_route( "/{id}", put_with(update_reservation, update_reservation_docs), @@ -145,7 +144,8 @@ async fn create_reservation( err @ ReservationsControllerError::ReservationInvalid, )) => Err((StatusCode::BAD_REQUEST, err.to_string())), Err(ControllerError::Reservation( - err @ ReservationsControllerError::BikeOutOfService(_), + err @ (ReservationsControllerError::BikeOutOfService(_) + | ReservationsControllerError::BikesTaken(_)), )) => Err((StatusCode::CONFLICT, err.to_string())), Err(ControllerError::Reservation( err @ ReservationsControllerError::NotAMemberOfUnit(_), @@ -169,7 +169,9 @@ fn create_reservation_docs(op: TransformOperation) -> TransformOperation { .response_with::<201, Json, _>(desc("The reservation, as stored")) .response_with::<400, (), _>(desc("The reservation is malformed")) .response_with::<403, (), _>(desc("The requester does not belong to the unit named")) - .response_with::<409, (), _>(desc("One of the bikes is out of service")) + .response_with::<409, (), _>(desc( + "One of the bikes is out of service, or is already held over that period", + )) .response_with::<422, (), _>(desc("The unit or one of the bikes does not exist")) } @@ -229,15 +231,64 @@ fn get_my_reservations_docs(op: TransformOperation) -> TransformOperation { .response_with::<400, (), _>(desc("A status in the filter is not a known one")) } -/// Only what the admin page lets somebody change. The unit, the requester, the -/// telegram handle and the reason are shown but not editable, so they are not -/// in the body at all: the handler reads them back from the stored reservation -/// rather than trusting a client to send them unchanged. +/// What a would-be booking asks about: a period, the bikes wanted, and the +/// reservation to leave out of the answer when one is being edited. +#[derive(Debug, Deserialize, JsonSchema)] +struct ConflictProbeForm { + /// The reservation being edited, so it does not conflict with itself + reservation: Option, + start_time: DateTime, + end_time: DateTime, + bikes: Vec, +} + +/// A read, not a write, but the question does not fit in a query string: each +/// bike may carry a period of its own. +#[axum::debug_handler] +async fn find_conflicts( + ac: AppController, + Json(form): Json, +) -> Result>, (StatusCode, String)> { + if form.end_time <= form.start_time { + return Err((StatusCode::BAD_REQUEST, "Empty period".to_owned())); + } + let probe = ConflictProbe { + reservation: form.reservation, + start_time: form.start_time, + end_time: form.end_time, + bikes: form.bikes, + }; + match ac.find_conflicts(probe).await { + Ok(conflicts) => Ok(Json(conflicts)), + Err(err) => unexpected_error("find_conflicts", err), + } +} + +fn find_conflicts_docs(op: TransformOperation) -> TransformOperation { + op.tag("Reservations") + .summary("Which of the bikes wanted are already held over the same period") + .description( + "Only approved and ongoing reservations hold a bike, so only they appear \ + here. The booking form uses it to grey out what is taken, and the admin \ + page to warn before double-booking on purpose.", + ) + .response_with::<400, (), _>(desc("The period is empty or the wrong way round")) +} + +/// Only what the admin page lets somebody change. The unit, the requester and +/// the reason are shown but not editable, so they are not in the body at all: +/// the handler reads them back from the stored reservation rather than trusting +/// a client to send them unchanged. #[derive(Debug, Deserialize, JsonSchema)] struct ReservationEditForm { start_time: DateTime, end_time: DateTime, - bikes: Vec, + /// Whoever picks the bikes up may change, and with them the handle to + /// reach on the day + telegram: String, + /// Each with its own period when a conflict was settled by handing it over + /// early, and without one when it simply follows the reservation + bikes: Vec, linka_emails: Vec, } @@ -261,7 +312,7 @@ async fn update_reservation( start_time: form.start_time, end_time: form.end_time, users: current.users.iter().map(|user| user.id).collect(), - telegram: current.telegram, + telegram: form.telegram, description: current.description, bikes: form.bikes, linka_emails: form.linka_emails, @@ -302,7 +353,9 @@ fn update_reservation_docs(op: TransformOperation) -> TransformOperation { .response::<404, ()>() .response_with::<400, (), _>(desc("The reservation would become malformed")) .response_with::<409, (), _>(desc( - "A newly added bike is out of service, or the reservation is final", + "A newly added bike is out of service or already held over that period \ + (managers may double-book on purpose, so this only reaches anybody else), \ + or the reservation is final", )) .response_with::<422, (), _>(desc("One of the bikes does not exist")) } @@ -333,7 +386,8 @@ async fn set_status( { Ok(()) => Ok(()), Err(ControllerError::Reservation( - err @ ReservationsControllerError::InvalidTransition(..), + err @ (ReservationsControllerError::InvalidTransition(..) + | ReservationsControllerError::BikesTaken(_)), )) => Err((StatusCode::CONFLICT, err.to_string())), Err(err) => unexpected_error("set_status", err), } @@ -342,7 +396,11 @@ async fn set_status( fn set_status_docs(op: TransformOperation) -> TransformOperation { op.tag("Reservations") .summary("Move a reservation through its state machine") - .description("Refuses a transition the state machine does not allow, with a 409.") + .description( + "Refuses a transition the state machine does not allow, with a 409 — and, \ + with the same status, approving a reservation whose bikes an approved \ + one already holds over the same period.", + ) .response_with::<403, (), _>(manager_desc) .response::<404, ()>() .response::<409, ()>() diff --git a/src/core/controller/reservations.rs b/src/core/controller/reservations.rs index fa42189..fc5b76e 100644 --- a/src/core/controller/reservations.rs +++ b/src/core/controller/reservations.rs @@ -1,17 +1,22 @@ use chrono::{DateTime, Utc}; use thiserror::Error; -use crate::core::{ - controller::{AnonAppController, AppController, ControllerError, ManagerAppController}, - models::{ - bike::BikeStatus, - reservation::{ - CalendarReservation, Involvement, NewReservation, NewReservationUnit, Reservation, - ReservationEdit, ReservationId, ReservationPage, ReservationQuery, ReservationStatus, +use crate::{ + core::{ + controller::{AnonAppController, AppController, ControllerError, ManagerAppController}, + models::{ + bike::{BikeId, BikeStatus}, + reservation::{ + CalendarReservation, Conflict, ConflictProbe, Involvement, NewReservation, + NewReservationBike, NewReservationUnit, Reservation, ReservationBike, + ReservationEdit, ReservationId, ReservationPage, ReservationQuery, + ReservationStatus, + }, + unit::UnitId, }, - unit::UnitId, + repositories::RepositoryError, }, - repositories::RepositoryError, + services::telegram::{self, Notification}, }; /// Reading the reservations needs no session: the calendar is public. @@ -114,10 +119,59 @@ impl AppController { return Err(ReservationsControllerError::BikeOutOfService(*id).into()); } } - self.db + // A bike held by an approved or ongoing reservation is not on offer. + // An admin is left free to double-book knowingly; the form they use + // warns them, and only they can sort the conflict out afterwards. + if !self.user().admin { + let conflicts = self + .db + .find_conflicts(ConflictProbe { + reservation: None, + start_time: reservation.start_time, + end_time: reservation.end_time, + bikes: reservation + .bikes + .iter() + .map(|&id| NewReservationBike { + id, + start_time: None, + end_time: None, + }) + .collect(), + }) + .await?; + if !conflicts.is_empty() { + return Err(ReservationsControllerError::BikesTaken(taken(&conflicts)).into()); + } + } + + let created = self + .db .create_reservation(reservation, self.user().id) - .await - .map_err(Into::into) + .await?; + + // The group is told, in the background: a reservation is filed whether + // or not Telegram answered. + let mut names = Vec::with_capacity(created.bikes.len()); + for held in &created.bikes { + names.push(match self.get_bike(held.id).await { + Ok(bike) => bike.name, + Err(_) => format!("#{}", held.id), + }); + } + telegram::notify(Notification::reservation_requested(&created, names)); + + Ok(created) + } + + /// Which of the bikes wanted are already held over the same period, and by + /// whom. Answering this needs a session but nothing more: it is what the + /// booking form greys out and what the admin page warns about. + pub async fn find_conflicts( + &self, + probe: ConflictProbe, + ) -> Result, ControllerError> { + self.db.find_conflicts(probe).await.map_err(Into::into) } } @@ -159,7 +213,13 @@ impl AppController { if current.status != ReservationStatus::Requested && (reservation.start_time != current.start_time || reservation.end_time != current.end_time - || !same_bikes(&reservation.bikes, ¤t.bikes)) + || reservation.telegram != current.telegram + || !same_fleet( + &reservation.bikes, + ¤t.bikes, + reservation.start_time, + reservation.end_time, + )) { return Err(ReservationsControllerError::OnlyEmailsEditable(current.status).into()); } @@ -167,12 +227,31 @@ impl AppController { // A bike already on the reservation may well have broken down since: // only a newly added one has to be in service. - for id in &reservation.bikes { - if current.bikes.contains(id) { + for bike in &reservation.bikes { + if current.bikes.iter().any(|held| held.id == bike.id) { continue; } - if self.get_bike(*id).await?.status == BikeStatus::OutOfService { - return Err(ReservationsControllerError::BikeOutOfService(*id).into()); + if self.get_bike(bike.id).await?.status == BikeStatus::OutOfService { + return Err(ReservationsControllerError::BikeOutOfService(bike.id).into()); + } + } + + // Whoever manages the reservation may double-book on purpose — the + // admin page warns them and lets them decide. Anybody else may not: + // otherwise the rule the booking form applies would be one page refresh + // away from being bypassed. + if !manages { + let conflicts = self + .db + .find_conflicts(ConflictProbe { + reservation: Some(reservation.id), + start_time: reservation.start_time, + end_time: reservation.end_time, + bikes: reservation.bikes.clone(), + }) + .await?; + if !conflicts.is_empty() { + return Err(ReservationsControllerError::BikesTaken(taken(&conflicts)).into()); } } @@ -195,10 +274,35 @@ impl AppController { } } -/// Order carries no meaning here, so the two lists are compared as sets -fn same_bikes(left: &[i32], right: &[i32]) -> bool { - let mut left = left.to_vec(); - let mut right = right.to_vec(); +/// The bikes a set of conflicts is about, once each +fn taken(conflicts: &[Conflict]) -> Vec { + let mut bikes: Vec = conflicts.iter().map(|conflict| conflict.bike).collect(); + bikes.sort_unstable(); + bikes.dedup(); + bikes +} + +/// Whether an edit leaves the fleet exactly as it is — the same bikes, each +/// held over the same period. Order carries no meaning, so both are compared as +/// sets, and the edit's periods are resolved first: leaving one out means "the +/// reservation's own", which is what a bike nobody arbitrated over already has. +fn same_fleet( + edit: &[NewReservationBike], + current: &[ReservationBike], + start: DateTime, + end: DateTime, +) -> bool { + let mut left: Vec<_> = edit + .iter() + .map(|bike| { + let (from, to) = bike.period(start, end); + (bike.id, from, to) + }) + .collect(); + let mut right: Vec<_> = current + .iter() + .map(|bike| (bike.id, bike.start_time, bike.end_time)) + .collect(); left.sort_unstable(); left.dedup(); right.sort_unstable(); @@ -213,17 +317,46 @@ impl ManagerAppController { id: ReservationId, status: ReservationStatus, ) -> Result<(), ControllerError> { - let current = self.db.get_reservation(id).await?; - if current.unit.scope() != self.unit { + let reservation = self.db.get_reservation(id).await?; + if reservation.unit.scope() != self.unit { return Err(ControllerError::ImmutableUnitModificationError); } - let current = current.status; + let current = reservation.status; if current == status { return Ok(()); } if !current.can_transition_to(status) { return Err(ReservationsControllerError::InvalidTransition(current, status).into()); } + + // Approving is the moment a reservation really takes its bikes, so it is + // the moment a double booking stops being a warning and becomes a + // refusal: two approved reservations over one bike means somebody turns + // up to an empty rack. Only this transition is guarded — a reservation + // already approved is allowed to start, whatever was overridden earlier. + if status == ReservationStatus::Approved { + let conflicts = self + .db + .find_conflicts(ConflictProbe { + reservation: Some(id), + start_time: reservation.start_time, + end_time: reservation.end_time, + bikes: reservation + .bikes + .iter() + .map(|held| NewReservationBike { + id: held.id, + start_time: Some(held.start_time), + end_time: Some(held.end_time), + }) + .collect(), + }) + .await?; + if !conflicts.is_empty() { + return Err(ReservationsControllerError::BikesTaken(taken(&conflicts)).into()); + } + } + self.db .set_reservation_status(id, status) .await @@ -254,20 +387,105 @@ pub enum ReservationsControllerError { NotOnTheReservation, #[error("A reservation that is {0:?} only accepts a change of Linka Go accounts")] OnlyEmailsEditable(ReservationStatus), + #[error("Cargobike(s) {0:?} are already booked over that period")] + BikesTaken(Vec), } #[cfg(test)] mod tests { - use super::same_bikes; + use chrono::{TimeZone, Utc}; + + use super::{same_fleet, taken}; + use crate::core::models::reservation::{ + Conflict, NewReservationBike, ReservationBike, ReservationStatus, + }; + + fn at(hour: u32) -> chrono::DateTime { + Utc.with_ymd_and_hms(2026, 8, 25, hour, 0, 0).unwrap() + } + + /// A bike on the edit, following the reservation unless told otherwise + fn wanted(id: i32, period: Option<(u32, u32)>) -> NewReservationBike { + NewReservationBike { + id, + start_time: period.map(|(from, _)| at(from)), + end_time: period.map(|(_, to)| at(to)), + } + } + + fn held(id: i32, from: u32, to: u32) -> ReservationBike { + ReservationBike { + id, + start_time: at(from), + end_time: at(to), + custom: false, + } + } #[test] - fn bike_lists_compare_as_sets() { - assert!(same_bikes(&[1, 2], &[2, 1]), "order carries no meaning"); - assert!(same_bikes(&[1, 1, 2], &[2, 1]), "nor do repeats"); - assert!(same_bikes(&[], &[])); + fn fleets_compare_as_sets_of_bikes_and_periods() { + let (start, end) = (at(12), at(20)); + let current = [held(1, 12, 20), held(2, 12, 20)]; - assert!(!same_bikes(&[1, 2], &[1]), "one was removed"); - assert!(!same_bikes(&[1], &[1, 2]), "one was added"); - assert!(!same_bikes(&[1], &[2])); + assert!( + same_fleet(&[wanted(2, None), wanted(1, None)], ¤t, start, end), + "order carries no meaning" + ); + assert!( + same_fleet( + &[wanted(1, None), wanted(1, None), wanted(2, None)], + ¤t, + start, + end + ), + "nor do repeats" + ); + assert!( + same_fleet( + &[wanted(1, Some((12, 20))), wanted(2, None)], + ¤t, + start, + end + ), + "spelling out the reservation's own period changes nothing" + ); + + assert!( + !same_fleet(&[wanted(1, None)], ¤t, start, end), + "one was removed" + ); + assert!( + !same_fleet( + &[wanted(1, None), wanted(2, None), wanted(3, None)], + ¤t, + start, + end + ), + "one was added" + ); + assert!( + !same_fleet( + &[wanted(1, Some((12, 15))), wanted(2, None)], + ¤t, + start, + end + ), + "one is handed over early" + ); + } + + #[test] + fn conflicting_bikes_are_named_once() { + let conflict = |bike| Conflict { + bike, + reservation: 1, + unit: "PolyNite".to_owned(), + status: ReservationStatus::Approved, + start_time: at(12), + end_time: at(20), + }; + // The same bike can be held by two reservations at once + assert_eq!(taken(&[conflict(2), conflict(1), conflict(2)]), vec![1, 2]); + assert!(taken(&[]).is_empty()); } } diff --git a/src/core/models/reservation.rs b/src/core/models/reservation.rs index ee77935..334454d 100644 --- a/src/core/models/reservation.rs +++ b/src/core/models/reservation.rs @@ -30,6 +30,9 @@ impl ReservationStatus { (self, next), (Requested, Refused) | (Requested, Approved) + // Back to the queue: an admin who approved too quickly, or who + // needs the bikes freed while the booking is discussed + | (Approved, Requested) | (Approved, Cancelled) | (Approved, Ongoing) | (Ongoing, Cancelled) @@ -139,6 +142,79 @@ impl NewReservationUnit { } } +/// One bike on a reservation, with the period it is actually held for. +/// +/// Normally the reservation's own period. An override is how a conflict is +/// settled without moving the whole booking: the reservation keeps its hours +/// and only the disputed bike changes hands earlier. +#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema, PartialEq, Eq)] +pub struct ReservationBike { + pub id: BikeId, + /// Already resolved: the bike's own period, or the reservation's + pub start_time: DateTime, + pub end_time: DateTime, + /// Whether that period is the bike's own rather than the reservation's + pub custom: bool, +} + +/// A bike as an edit carries it. `None` means "the reservation's period", which +/// is what a bike nobody had to arbitrate over keeps. +#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema, PartialEq, Eq)] +pub struct NewReservationBike { + pub id: BikeId, + pub start_time: Option>, + pub end_time: Option>, +} + +impl NewReservationBike { + /// The period this bike is held for, given the reservation's own + pub fn period( + &self, + start: DateTime, + end: DateTime, + ) -> (DateTime, DateTime) { + ( + self.start_time.unwrap_or(start), + self.end_time.unwrap_or(end), + ) + } + + /// Both bounds or neither, the right way round, and inside the + /// reservation's own period: a bike cannot be held when nothing is booked. + fn is_valid(&self, start: DateTime, end: DateTime) -> bool { + match (self.start_time, self.end_time) { + (None, None) => true, + (Some(from), Some(to)) => from < to && from >= start && to <= end, + _ => false, + } + } +} + +/// A bike wanted over a period, as a conflict check asks about it. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ConflictProbe { + /// The reservation being edited, left out of its own check + pub reservation: Option, + pub start_time: DateTime, + pub end_time: DateTime, + pub bikes: Vec, +} + +/// A bike the probe wants that somebody else already holds over part of the +/// same period. Only the reservations that actually hold a bike — approved and +/// ongoing — can produce one. +#[derive(Debug, Serialize, Deserialize, Clone, JsonSchema, PartialEq, Eq)] +pub struct Conflict { + pub bike: BikeId, + pub reservation: ReservationId, + /// Who holds it, to be named in the warning + pub unit: String, + pub status: ReservationStatus, + /// When the bike is held, which is what a resolution has to step around + pub start_time: DateTime, + pub end_time: DateTime, +} + #[derive(Debug, Serialize, Deserialize, Clone, JsonSchema, PartialEq, Eq)] pub struct Reservation { pub id: ReservationId, @@ -150,7 +226,7 @@ pub struct Reservation { pub users: Vec, pub telegram: String, pub description: String, - pub bikes: Vec, + pub bikes: Vec, /// Linka Go accounts allowed to unlock the bikes. Plain addresses: they /// need not belong to anybody who ever logs into this app. pub linka_emails: Vec, @@ -167,7 +243,8 @@ pub struct CalendarReservation { pub unit: ReservationUnit, pub start_time: DateTime, pub end_time: DateTime, - pub bikes: Vec, + /// With their own periods: a bike handed over early is drawn as such + pub bikes: Vec, pub status: ReservationStatus, } @@ -282,7 +359,9 @@ pub struct ReservationEdit { pub users: Vec, pub telegram: String, pub description: String, - pub bikes: Vec, + /// Each with the period it is held for, or nothing to follow the + /// reservation's own + pub bikes: Vec, pub linka_emails: Vec, } @@ -322,6 +401,10 @@ impl ReservationEdit { && self.end_time > self.start_time && !self.bikes.is_empty() && !self.users.is_empty() + && self + .bikes + .iter() + .all(|bike| bike.is_valid(self.start_time, self.end_time)) && telegram_valid(&self.telegram) && linka_emails_valid(&self.linka_emails) } diff --git a/src/core/repositories/reservations_repository.rs b/src/core/repositories/reservations_repository.rs index b8e3641..4845abc 100644 --- a/src/core/repositories/reservations_repository.rs +++ b/src/core/repositories/reservations_repository.rs @@ -3,8 +3,8 @@ use async_trait::async_trait; use crate::core::{ models::{ reservation::{ - NewReservation, Reservation, ReservationEdit, ReservationId, ReservationPage, - ReservationQuery, ReservationStatus, + Conflict, ConflictProbe, NewReservation, Reservation, ReservationEdit, ReservationId, + ReservationPage, ReservationQuery, ReservationStatus, }, unit::UnitId, user::UserId, @@ -30,6 +30,11 @@ pub trait ReservationsRepository { ) -> Result, RepositoryError>; async fn get_reservation(&self, id: ReservationId) -> Result; + /// The bikes in `probe` that somebody else already holds over part of the + /// same period, one row per overlap. Only approved and ongoing reservations + /// hold a bike, and a reservation is never in conflict with itself. + async fn find_conflicts(&self, probe: ConflictProbe) -> Result, RepositoryError>; + /// Always stored as `Requested`: the state machine starts here. The /// requester comes from the session, not from the request body. async fn create_reservation( diff --git a/src/services/database/reservations.rs b/src/services/database/reservations.rs index e121c47..bb9b0db 100644 --- a/src/services/database/reservations.rs +++ b/src/services/database/reservations.rs @@ -12,8 +12,9 @@ use crate::{ core::{ models::{ reservation::{ - NewReservation, NewReservationUnit, Reservation, ReservationEdit, ReservationId, - ReservationPage, ReservationQuery, ReservationStatus, ReservationUnit, + Conflict, ConflictProbe, NewReservation, NewReservationBike, NewReservationUnit, + Reservation, ReservationEdit, ReservationId, ReservationPage, ReservationQuery, + ReservationStatus, ReservationUnit, }, unit::{Unit, UnitId}, user::UserId, @@ -73,7 +74,7 @@ struct ReservationDB { pub status: ReservationStatusDB, pub linka_emails: Vec, pub users: Value, - pub bikes: Vec, + pub bikes: Value, } impl TryFrom for Reservation { @@ -100,13 +101,35 @@ impl TryFrom for Reservation { users: serde_json::from_value(value.users)?, telegram: value.telegram, description: value.description, - bikes: value.bikes, + bikes: serde_json::from_value(value.bikes)?, linka_emails: value.linka_emails, status: value.status.into(), }) } } +struct ConflictDB { + pub bike: i32, + pub reservation: i32, + pub unit: String, + pub status: ReservationStatusDB, + pub start_time: DateTime, + pub end_time: DateTime, +} + +impl From for Conflict { + fn from(value: ConflictDB) -> Self { + Conflict { + bike: value.bike, + reservation: value.reservation, + unit: value.unit, + status: value.status.into(), + start_time: value.start_time, + end_time: value.end_time, + } + } +} + /// The page query carries one extra column — how many rows the filter matches /// in all, from a window function — so it needs its own row type. Everything /// else is the shared one, which the conversion below hands back. @@ -123,7 +146,7 @@ struct ReservationPageRowDB { pub status: ReservationStatusDB, pub linka_emails: Vec, pub users: Value, - pub bikes: Vec, + pub bikes: Value, pub total: i64, } @@ -152,7 +175,7 @@ impl SqlxDatabase { tx: &mut sqlx::Transaction<'_, sqlx::Postgres>, id: ReservationId, users: &[i32], - bikes: &[i32], + bikes: &[NewReservationBike], ) -> Result<(), RepositoryError> { query!( r#"DELETE FROM reservations_users WHERE reservation_id = $1"#, @@ -175,11 +198,18 @@ impl SqlxDatabase { ) .execute(&mut **tx) .await?; + // Three parallel arrays rather than a row constructor: it keeps the + // insert to one statement, and NULL still means "the reservation's period" + let bike_ids: Vec = bikes.iter().map(|bike| bike.id).collect(); + let starts: Vec>> = bikes.iter().map(|bike| bike.start_time).collect(); + let ends: Vec>> = bikes.iter().map(|bike| bike.end_time).collect(); query!( - r#"INSERT INTO reservations_bikes (reservation_id, bike_id) - SELECT $1, UNNEST($2::integer[])"#, + r#"INSERT INTO reservations_bikes (reservation_id, bike_id, start_time, end_time) + SELECT $1, * FROM UNNEST($2::integer[], $3::timestamptz[], $4::timestamptz[])"#, id, - bikes + &bike_ids, + &starts as &[Option>], + &ends as &[Option>] ) .execute(&mut **tx) .await?; @@ -266,10 +296,19 @@ impl ReservationsRepository for SqlxDatabase { JOIN users u ON u.id = ru.user_id WHERE ru.reservation_id = r.id ), '[]'::json) AS "users!", - ARRAY( - SELECT bike_id FROM reservations_bikes - WHERE reservation_id = r.id ORDER BY bike_id - ) AS "bikes!", + -- Each bike with the period it is actually held for: its own + -- when a conflict was settled by handing it over early, the + -- reservation's otherwise + COALESCE(( + SELECT json_agg(json_build_object( + 'id', rb.bike_id, + 'start_time', COALESCE(rb.start_time, r.start_time), + 'end_time', COALESCE(rb.end_time, r.end_time), + 'custom', rb.start_time IS NOT NULL + ) ORDER BY rb.bike_id) + FROM reservations_bikes rb + WHERE rb.reservation_id = r.id + ), '[]'::json) AS "bikes!", -- The size of the whole match, alongside the page itself: the -- caller can show "42 results" without a second round trip COUNT(*) OVER () AS "total!" @@ -365,10 +404,19 @@ impl ReservationsRepository for SqlxDatabase { JOIN users u ON u.id = ru.user_id WHERE ru.reservation_id = r.id ), '[]'::json) AS "users!", - ARRAY( - SELECT bike_id FROM reservations_bikes - WHERE reservation_id = r.id ORDER BY bike_id - ) AS "bikes!" + -- Each bike with the period it is actually held for: its own + -- when a conflict was settled by handing it over early, the + -- reservation's otherwise + COALESCE(( + SELECT json_agg(json_build_object( + 'id', rb.bike_id, + 'start_time', COALESCE(rb.start_time, r.start_time), + 'end_time', COALESCE(rb.end_time, r.end_time), + 'custom', rb.start_time IS NOT NULL + ) ORDER BY rb.bike_id) + FROM reservations_bikes rb + WHERE rb.reservation_id = r.id + ), '[]'::json) AS "bikes!" FROM reservations r LEFT JOIN units un ON un.id = r.unit_id WHERE r.unit_id = $1 @@ -382,6 +430,56 @@ impl ReservationsRepository for SqlxDatabase { .collect::, _>>()?) } + async fn find_conflicts(&self, probe: ConflictProbe) -> Result, RepositoryError> { + // The probe resolved: every bike with the period it would be held for + let mut bikes = Vec::with_capacity(probe.bikes.len()); + let mut starts = Vec::with_capacity(probe.bikes.len()); + let mut ends = Vec::with_capacity(probe.bikes.len()); + for bike in &probe.bikes { + let (from, to) = bike.period(probe.start_time, probe.end_time); + bikes.push(bike.id); + starts.push(from); + ends.push(to); + } + + Ok(query_as!( + ConflictDB, + r#"WITH wanted(bike_id, start_time, end_time) AS ( + SELECT * FROM UNNEST($1::integer[], $2::timestamptz[], $3::timestamptz[]) + ) + SELECT + w.bike_id AS "bike!", + r.id AS "reservation!", + COALESCE(un."name", r.unit_label, '') AS "unit!", + r.status AS "status: ReservationStatusDB", + COALESCE(rb.start_time, r.start_time) AS "start_time!", + COALESCE(rb.end_time, r.end_time) AS "end_time!" + FROM wanted w + JOIN reservations_bikes rb ON rb.bike_id = w.bike_id + JOIN reservations r ON r.id = rb.reservation_id + LEFT JOIN units un ON un.id = r.unit_id + -- Only a reservation that actually holds the bike can conflict: a + -- request holds nothing yet, and a final one holds nothing any more + WHERE r.status IN ('approved', 'ongoing') + -- A reservation never conflicts with itself + AND ($4::integer IS NULL OR r.id <> $4) + -- Half-open overlap: ending exactly when the other starts is fine, + -- which is what makes handing a bike over at 15:00 a resolution + AND COALESCE(rb.start_time, r.start_time) < w.end_time + AND COALESCE(rb.end_time, r.end_time) > w.start_time + ORDER BY w.bike_id, "start_time!""#, + &bikes, + &starts, + &ends, + probe.reservation, + ) + .fetch_all(&self.pool) + .await? + .into_iter() + .map(Into::into) + .collect()) + } + async fn get_reservation(&self, id: ReservationId) -> Result { Ok(query_as!( ReservationDB, @@ -410,10 +508,19 @@ impl ReservationsRepository for SqlxDatabase { JOIN users u ON u.id = ru.user_id WHERE ru.reservation_id = r.id ), '[]'::json) AS "users!", - ARRAY( - SELECT bike_id FROM reservations_bikes - WHERE reservation_id = r.id ORDER BY bike_id - ) AS "bikes!" + -- Each bike with the period it is actually held for: its own + -- when a conflict was settled by handing it over early, the + -- reservation's otherwise + COALESCE(( + SELECT json_agg(json_build_object( + 'id', rb.bike_id, + 'start_time', COALESCE(rb.start_time, r.start_time), + 'end_time', COALESCE(rb.end_time, r.end_time), + 'custom', rb.start_time IS NOT NULL + ) ORDER BY rb.bike_id) + FROM reservations_bikes rb + WHERE rb.reservation_id = r.id + ), '[]'::json) AS "bikes!" FROM reservations r LEFT JOIN units un ON un.id = r.unit_id WHERE r.id = $1"#, @@ -459,7 +566,18 @@ impl ReservationsRepository for SqlxDatabase { if !users.contains(&requester) { users.push(requester); } - Self::set_reservation_links(&mut tx, id, &users, &reservation.bikes).await?; + // A new reservation holds every bike for its own period; only an edit + // can hand one over early. + let bikes: Vec = reservation + .bikes + .iter() + .map(|&id| NewReservationBike { + id, + start_time: None, + end_time: None, + }) + .collect(); + Self::set_reservation_links(&mut tx, id, &users, &bikes).await?; tx.commit().await?; diff --git a/src/services/mod.rs b/src/services/mod.rs index 6138efe..9ebf600 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -1,2 +1,3 @@ //! External services used by the core: database, and any third party api you add. pub mod database; +pub mod telegram; diff --git a/src/services/telegram.rs b/src/services/telegram.rs new file mode 100644 index 0000000..6af0414 --- /dev/null +++ b/src/services/telegram.rs @@ -0,0 +1,233 @@ +//! Telegram notifications to the group that runs the cargobikes. +//! +//! The point of this module is that adding a message is a two-line change: +//! a variant on [`Notification`] and an arm in [`Notification::render`]. +//! Nothing else in the app has to know that Telegram exists, nor how a message +//! is worded — a caller says *what happened*, not *what to write*. +//! +//! Sending never blocks the request that triggered it and never fails it: a +//! reservation is filed whether or not the group was told about it. When the +//! bot is not configured, [`notify`] is a no-op, which is what a development +//! machine and the test suite want. + +use std::sync::OnceLock; + +use chrono::{DateTime, Utc}; +use openidconnect::reqwest; +use serde::Serialize; +use tracing::{debug, error, warn}; + +use crate::{core::models::reservation::Reservation, utils::config}; + +/// Something worth telling the group about. +/// +/// Each variant carries what the message needs, already resolved: the renderer +/// reads no database and can therefore never fail nor be slow. +#[derive(Debug, Clone)] +pub enum Notification { + /// A reservation request has just been filed + ReservationRequested { + id: i32, + unit: String, + requester: String, + start_time: DateTime, + end_time: DateTime, + bikes: Vec, + description: String, + }, +} + +impl Notification { + /// The message body, in Telegram's HTML parse mode. Only `&`, `<` and `>` + /// need escaping there, which [`escape`] does for every value that comes + /// from a user. + fn render(&self) -> String { + match self { + Notification::ReservationRequested { + id, + unit, + requester, + start_time, + end_time, + bikes, + description, + } => { + let bikes = if bikes.is_empty() { + "—".to_owned() + } else { + escape(&bikes.join(", ")) + }; + format!( + "🚲 Nouvelle demande de réservation #{id}\n\ + Association : {unit}\n\ + Demandée par : {requester}\n\ + Période : {start} → {end}\n\ + Cargobike(s) : {bikes}\n\ + Raison : {description}", + unit = escape(unit), + requester = escape(requester), + start = format_moment(*start_time), + end = format_moment(*end_time), + description = escape(description), + ) + } + } + } +} + +impl Notification { + /// The notification a freshly filed request produces, given the reservation + /// as stored and the names of the bikes it holds. + pub fn reservation_requested(reservation: &Reservation, bikes: Vec) -> Self { + Notification::ReservationRequested { + id: reservation.id, + unit: reservation.unit.label().to_owned(), + requester: reservation + .users + .iter() + .find(|user| user.id == reservation.requester) + .map_or_else( + || format!("#{}", reservation.requester), + |user| format!("{} {}", user.firstname, user.name), + ), + start_time: reservation.start_time, + end_time: reservation.end_time, + bikes, + description: reservation.description.clone(), + } + } +} + +/// Sends `notification` to the configured group, in the background. +/// +/// Returns immediately. A failure is logged and goes no further: the caller's +/// own work has already succeeded, and undoing it because Telegram is down +/// would be worse than a missing message. +pub fn notify(notification: Notification) { + let Some(telegram) = config::get().telegram.as_ref() else { + debug!("[TELEGRAM] not configured, dropping {notification:?}"); + return; + }; + let token = telegram.bot_token.clone(); + let chat_id = telegram.chat_id.clone(); + let api_url = telegram.get_api_url().to_owned(); + + tokio::spawn(async move { + if let Err(err) = send(&api_url, &token, &chat_id, ¬ification.render()).await { + error!("[TELEGRAM] could not send the notification: {err}"); + } + }); +} + +#[derive(Serialize)] +struct SendMessage<'a> { + chat_id: &'a str, + text: &'a str, + parse_mode: &'a str, + /// Link previews turn a reservation into a wall of nothing + disable_web_page_preview: bool, +} + +async fn send(api_url: &str, token: &str, chat_id: &str, text: &str) -> Result<(), String> { + // Serialised by hand: the http client is the one `openidconnect` brings, and + // it is built without its `json` feature. + let body = serde_json::to_string(&SendMessage { + chat_id, + text, + parse_mode: "HTML", + disable_web_page_preview: true, + }) + .map_err(|err| err.to_string())?; + + let response = http_client() + .post(format!("{api_url}/bot{token}/sendMessage")) + .header("content-type", "application/json") + .body(body) + .send() + .await + .map_err(|err| err.to_string())?; + + if !response.status().is_success() { + // Telegram explains itself in the body, and that is the only way to + // tell "wrong token" from "the bot is not in that group" + let status = response.status(); + let body = response.text().await.unwrap_or_default(); + warn!("[TELEGRAM] {status}: {body}"); + return Err(format!("{status}: {body}")); + } + Ok(()) +} + +fn http_client() -> &'static reqwest::Client { + static HTTP_CLIENT: OnceLock = OnceLock::new(); + HTTP_CLIENT.get_or_init(|| { + reqwest::ClientBuilder::new() + .timeout(std::time::Duration::from_secs(10)) + .build() + .expect("Unable to build the telegram http client") + }) +} + +/// The three characters Telegram's HTML mode reads as markup +fn escape(value: &str) -> String { + value + .replace('&', "&") + .replace('<', "<") + .replace('>', ">") +} + +/// Swiss local time, which is the only one the group cares about +fn format_moment(moment: DateTime) -> String { + moment + .with_timezone(&chrono::FixedOffset::east_opt(2 * 3600).expect("valid offset")) + .format("%d.%m.%Y %H:%M") + .to_string() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn moment(day: u32, hour: u32) -> DateTime { + use chrono::TimeZone; + Utc.with_ymd_and_hms(2026, 8, day, hour, 0, 0).unwrap() + } + + #[test] + fn a_request_reads_as_a_message() { + let message = Notification::ReservationRequested { + id: 12, + unit: "PolyNite".to_owned(), + requester: "Milan Hyenne".to_owned(), + start_time: moment(25, 10), + end_time: moment(26, 16), + bikes: vec!["1000".to_owned(), "2000".to_owned()], + description: "Transport du matériel".to_owned(), + } + .render(); + + assert!(message.contains("#12")); + assert!(message.contains("PolyNite")); + assert!(message.contains("1000, 2000")); + // Rendered in local time: 10:00 UTC is noon in Lausanne + assert!(message.contains("25.08.2026 12:00"), "{message}"); + } + + #[test] + fn markup_in_a_user_value_is_escaped() { + let message = Notification::ReservationRequested { + id: 1, + unit: "Fake & Co".to_owned(), + requester: "A".to_owned(), + start_time: moment(25, 10), + end_time: moment(25, 12), + bikes: vec![], + description: String::new(), + } + .render(); + + assert!(message.contains("<b>Fake</b> & Co")); + // The heading is ours, and stays markup + assert!(message.contains("Nouvelle demande")); + } +} diff --git a/src/utils/config.rs b/src/utils/config.rs index 3d04377..04ab400 100644 --- a/src/utils/config.rs +++ b/src/utils/config.rs @@ -31,6 +31,26 @@ pub struct OidcConfig { pub session_lifetime: Option, } +/// The bot that posts to the group. Absent, nothing is sent — which is what a +/// development machine wants. Both values are secrets: keep them in `.env` or +/// in the environment, not in a committed file. +#[derive(Deserialize, Clone)] +pub struct TelegramConfig { + pub bot_token: String, + pub chat_id: String, + /// Where the bot api lives. Only ever set to something else to point the + /// sender at a stub, or at a proxy in a network that cannot reach Telegram. + pub api_url: Option, +} + +impl TelegramConfig { + pub fn get_api_url(&self) -> &str { + self.api_url + .as_deref() + .unwrap_or("https://api.telegram.org") + } +} + /// A user that can be logged in without going through the provider. /// Only usable in debug builds, see `POST /api/login`. #[derive(Deserialize, Clone)] @@ -50,6 +70,9 @@ pub struct AppConfig { pub server: ServerConfig, pub postgres: PostgresConfig, pub oidc: OidcConfig, + /// Absent when the bot is not set up: notifications are then dropped + #[serde(default)] + pub telegram: Option, #[serde(default)] pub dev_users: Vec, /// Directory containing the built frontend @@ -89,11 +112,18 @@ impl AppConfig { /// Loads the configuration, by decreasing priority: /// - `APP__SERVER__PORT=3000` style environment variables +/// - the same variables read from `./.env`, which is where the secrets live in +/// development (a real environment variable still wins over the file) /// - `./config.yml` /// - the file pointed by `$APP_CONFIG` (`/etc/cargagep/config.yml` by default) pub fn get() -> &'static AppConfig { static CONFIG: OnceLock = OnceLock::new(); CONFIG.get_or_init(|| { + // `.env` is read into the environment first, so `APP__*` written there + // behaves exactly like a variable exported by hand. Anything already in + // the environment is left alone. + load_dotenv(); + let config_path = std::env::var("APP_CONFIG").unwrap_or("/etc/cargagep/config.yml".to_owned()); let config_path = Path::new(&config_path); @@ -112,3 +142,27 @@ pub fn get() -> &'static AppConfig { .expect("Invalid config format") }) } + +/// A minimal `.env` reader: `KEY=value` per line, `#` comments, optional +/// surrounding quotes. Deliberately not a dependency — the file is ours, and +/// dbmate already reads it with the same rules. +fn load_dotenv() { + let Ok(contents) = std::fs::read_to_string(".env") else { + return; + }; + for line in contents.lines() { + let line = line.trim(); + if line.is_empty() || line.starts_with('#') { + continue; + } + let Some((key, value)) = line.split_once('=') else { + continue; + }; + let key = key.trim().trim_start_matches("export ").trim(); + let value = value.trim().trim_matches('"').trim_matches('\''); + // An exported variable wins: that is how a deployment overrides the file + if std::env::var_os(key).is_none() { + unsafe { std::env::set_var(key, value) }; + } + } +}