From f8ff4b82a3c61c3d90c182fc4a2aff327bfa8b67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Fromme?= Date: Tue, 8 Sep 2026 17:11:00 +0200 Subject: [PATCH] feat: replace custom discount with absolute value in favor of percentage --- docs/api-consumer-guide.md | 10 ++--- migrations/Version20260908161500.php | 30 +++++++++++++++ .../AccommodationBooking/EditController.php | 1 + src/Entity/Groups/AccommodationBooking.php | 15 ++++---- .../Admin/Groups/AccommodationBookingType.php | 19 +++++++--- src/Model/AccommodationBookingApiResponse.php | 6 +-- ...ccommodationBookingBreakdownCalculator.php | 11 +++--- .../accommodation_booking/edit.html.twig | 2 +- .../accommodation_booking/index.html.twig | 4 +- .../accommodation_booking/show.html.twig | 2 +- templates/email/_price_breakdown.html.twig | 2 +- templates/groups/offer/_summary.html.twig | 2 +- .../AccommodationBookingControllerTest.php | 4 +- .../Groups/AccommodationBookingTypeTest.php | 10 ++--- ...modationBookingBreakdownCalculatorTest.php | 37 +++++++++++++------ 15 files changed, 105 insertions(+), 50 deletions(-) create mode 100644 migrations/Version20260908161500.php diff --git a/docs/api-consumer-guide.md b/docs/api-consumer-guide.md index 2ad7352..7df06f6 100644 --- a/docs/api-consumer-guide.md +++ b/docs/api-consumer-guide.md @@ -276,7 +276,7 @@ Single booking by UUID. `404 {"message":"Not found"}` if unknown. "accommodationDiscount": null, "boardServiceDiscount": null, "additionalServicesDiscount": null, - "totalDiscount": null, + "totalDiscountAmount": null, "totalDiscountLabel": null, "totalPrice": 123456, "pricingCurrency": "EUR", @@ -285,19 +285,19 @@ Single booking by UUID. `404 {"message":"Not found"}` if unknown. } ``` -`totalPrice` is in integer **minor units** (cents) — unlike the contingent endpoints. The `*Discount` fields are whole **percentages** (1–100) or `null`, not amounts. `dateFrom`/`dateTo` are `Y-m-d`; `acceptedAt` is a full ISO-8601 datetime and is `null` until accepted. `priceBreakdown` is a computed nested structure. +`totalPrice` and `totalDiscountAmount` are integer **minor units** (cents) — unlike the contingent endpoints. The three `*Discount` fields are whole **percentages** (1–100) or `null`, not amounts. `dateFrom`/`dateTo` are `Y-m-d`; `acceptedAt` is a full ISO-8601 datetime and is `null` until accepted. `priceBreakdown` is a computed nested structure. The first three discounts each apply to one section of the breakdown: `accommodationDiscount` to `basePrice + additionalPersonsPrice`, `boardServiceDiscount` to `boardPrice`, `additionalServicesDiscount` to `servicesPrice`. Surcharges and running costs sit in `total` but in no section, so they are never discounted. -`totalDiscount` is a freely named discount on the whole price, labelled by `totalDiscountLabel`. **It compounds**: the three section discounts reduce `total` first, and `totalDiscount` then applies to that already-reduced subtotal. Recomputing it against the gross `total` yields a different figure. Each amount is rounded to the nearest cent at the step it is applied. +`totalDiscountAmount` is a freely named discount on the whole price, labelled by `totalDiscountLabel`. It is a fixed sum, not a percentage, and is subtracted **after** the three section discounts have reduced `total`. It is capped at what is left of that subtotal, so `discountedTotal` is never negative and the amount shown in the breakdown may be smaller than `totalDiscountAmount` itself. The section discounts are each rounded to the nearest cent as they are applied. `priceBreakdown` carries the resulting rows, all in minor units: | field | meaning | | --- | --- | | `discounts` | list of `{label, percent, amount}` for the section discounts; entries rounding to `0` are omitted, and `label` already includes its `Rabatt ` prefix | -| `discountSubtotal` | `total` minus the section discounts — present only when `totalDiscount` applies *and* at least one section discount did, otherwise `null` | -| `totalDiscountDetails` | `{label, percent, amount}` for the total discount, or `null` | +| `discountSubtotal` | `total` minus the section discounts — present only when `totalDiscountAmount` applies *and* at least one section discount did, otherwise `null` | +| `totalDiscountDetails` | `{label, amount}` for the total discount, or `null`; `amount` is the capped figure actually subtracted | | `discountedTotal` | the final price, equal to the booking's `totalPrice` | #### `POST /api/accommodation-bookings/{uuid}/accept` — scope `api` diff --git a/migrations/Version20260908161500.php b/migrations/Version20260908161500.php new file mode 100644 index 0000000..964394e --- /dev/null +++ b/migrations/Version20260908161500.php @@ -0,0 +1,30 @@ +addSql('ALTER TABLE accommodation_booking DROP total_discount, ADD total_discount_amount INT DEFAULT NULL'); + } + + public function down(Schema $schema): void + { + $this->addSql('ALTER TABLE accommodation_booking DROP total_discount_amount, ADD total_discount INT DEFAULT NULL'); + } +} diff --git a/src/Controller/Admin/AccommodationBooking/EditController.php b/src/Controller/Admin/AccommodationBooking/EditController.php index f4c928f..85a495a 100644 --- a/src/Controller/Admin/AccommodationBooking/EditController.php +++ b/src/Controller/Admin/AccommodationBooking/EditController.php @@ -91,6 +91,7 @@ class EditController extends AbstractController 'current_board_service' => $currentBoardService, 'current_additional_services' => $currentAdditionalServices, 'assignable_managers' => $assignableManagers, + 'currency' => $booking->getPricingCurrency() ?? $accommodation?->getCurrency() ?? 'EUR', ]); $form->handleRequest($request); diff --git a/src/Entity/Groups/AccommodationBooking.php b/src/Entity/Groups/AccommodationBooking.php index e8e54e3..87bd340 100644 --- a/src/Entity/Groups/AccommodationBooking.php +++ b/src/Entity/Groups/AccommodationBooking.php @@ -156,13 +156,14 @@ class AccommodationBooking implements BlameableEntityInterface, TimestampableEnt #[Assert\Range(min: 1, max: 100)] private ?int $additionalServicesDiscount = null; + /** Absolute discount on the total, in minor units. Applied after the three section discounts. */ #[ORM\Column(nullable: true)] - #[Assert\Range(min: 1, max: 100)] - private ?int $totalDiscount = null; + #[Assert\Positive] + private ?int $totalDiscountAmount = null; #[ORM\Column(length: 255, nullable: true)] #[Assert\Expression( - 'null === this.getTotalDiscount() or (null !== value and "" !== value)', + 'null === this.getTotalDiscountAmount() or (null !== value and "" !== value)', message: 'Bitte eine Bezeichnung für den Rabatt angeben.', )] private ?string $totalDiscountLabel = null; @@ -649,14 +650,14 @@ class AccommodationBooking implements BlameableEntityInterface, TimestampableEnt return $this; } - public function getTotalDiscount(): ?int + public function getTotalDiscountAmount(): ?int { - return $this->totalDiscount; + return $this->totalDiscountAmount; } - public function setTotalDiscount(?int $totalDiscount): self + public function setTotalDiscountAmount(?int $totalDiscountAmount): self { - $this->totalDiscount = $totalDiscount; + $this->totalDiscountAmount = $totalDiscountAmount; return $this; } diff --git a/src/Form/Admin/Groups/AccommodationBookingType.php b/src/Form/Admin/Groups/AccommodationBookingType.php index bb1caa6..8617907 100644 --- a/src/Form/Admin/Groups/AccommodationBookingType.php +++ b/src/Form/Admin/Groups/AccommodationBookingType.php @@ -14,11 +14,13 @@ use Symfony\Component\Form\Extension\Core\Type\ChoiceType; use Symfony\Component\Form\Extension\Core\Type\DateType; use Symfony\Component\Form\Extension\Core\Type\EmailType; use Symfony\Component\Form\Extension\Core\Type\IntegerType; +use Symfony\Component\Form\Extension\Core\Type\MoneyType; use Symfony\Component\Form\Extension\Core\Type\TextareaType; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\Form\FormInterface; use Symfony\Component\OptionsResolver\OptionsResolver; +use Symfony\Component\Validator\Constraints\Positive; use Symfony\Component\Validator\Constraints\Range; /** @extends AbstractType */ @@ -158,14 +160,17 @@ class AccommodationBookingType extends AbstractType new Range(min: 1, max: 100), ], ]) - // Applies to the total after the three section discounts. The label is not marked - // required here — a percentage may legitimately be absent — but the entity refuses - // a percentage without one, so no unnamed discount can reach the customer. - ->add('totalDiscount', IntegerType::class, [ - 'label' => 'Rabatt Gesamtpreis (%)', + // A fixed sum, not a percentage, applied to the total after the three section + // discounts. The label is not marked required here — an amount may legitimately be + // absent — but the entity refuses an amount without one, so no unnamed discount can + // reach the customer. + ->add('totalDiscountAmount', MoneyType::class, [ + 'label' => 'Rabatt Gesamtpreis', + 'currency' => $options['currency'], + 'divisor' => 100, 'required' => false, 'constraints' => [ - new Range(min: 1, max: 100), + new Positive(), ], ]) ->add('totalDiscountLabel', TextType::class, [ @@ -201,6 +206,7 @@ class AccommodationBookingType extends AbstractType 'current_board_service' => null, 'current_additional_services' => [], 'assignable_managers' => [], + 'currency' => 'EUR', ]); $resolver->setAllowedTypes('max_adolescent_age', 'int'); $resolver->setAllowedTypes('board_services', 'array'); @@ -208,5 +214,6 @@ class AccommodationBookingType extends AbstractType $resolver->setAllowedTypes('current_board_service', ['null', BoardService::class]); $resolver->setAllowedTypes('current_additional_services', 'array'); $resolver->setAllowedTypes('assignable_managers', 'array'); + $resolver->setAllowedTypes('currency', 'string'); } } diff --git a/src/Model/AccommodationBookingApiResponse.php b/src/Model/AccommodationBookingApiResponse.php index 4c1d2f4..8e9c219 100644 --- a/src/Model/AccommodationBookingApiResponse.php +++ b/src/Model/AccommodationBookingApiResponse.php @@ -73,9 +73,9 @@ final readonly class AccommodationBookingApiResponse #[Groups(['api:single'])] public ?int $additionalServicesDiscount; - /** Applies to the total after the three section discounts above, not to the gross total. */ + /** An absolute amount in minor units, subtracted after the three section discounts above. */ #[Groups(['api:single'])] - public ?int $totalDiscount; + public ?int $totalDiscountAmount; #[Groups(['api:single'])] public ?string $totalDiscountLabel; @@ -122,7 +122,7 @@ final readonly class AccommodationBookingApiResponse $this->accommodationDiscount = $booking->getAccommodationDiscount(); $this->boardServiceDiscount = $booking->getBoardServiceDiscount(); $this->additionalServicesDiscount = $booking->getAdditionalServicesDiscount(); - $this->totalDiscount = $booking->getTotalDiscount(); + $this->totalDiscountAmount = $booking->getTotalDiscountAmount(); $this->totalDiscountLabel = $booking->getTotalDiscountLabel(); $this->totalPrice = $booking->getTotalPrice(); $this->pricingCurrency = $booking->getPricingCurrency(); diff --git a/src/Service/AccommodationBookingBreakdownCalculator.php b/src/Service/AccommodationBookingBreakdownCalculator.php index 657f21a..a0d9a7d 100644 --- a/src/Service/AccommodationBookingBreakdownCalculator.php +++ b/src/Service/AccommodationBookingBreakdownCalculator.php @@ -43,7 +43,8 @@ class AccommodationBookingBreakdownCalculator public function withDiscounts(AccommodationBooking $booking, array $breakdown): array { // The labels carry their own "Rabatt" prefix so that every row renders as plain - // {label} {percent} %, including the freely named discount on the total below. + // {label} {percent} %, while the freely named discount on the total below is a fixed + // amount and renders as its label alone. $buckets = [ ['Rabatt Unterkunft', $booking->getAccommodationDiscount(), (int) ($breakdown['basePrice'] ?? 0) + (int) ($breakdown['additionalPersonsPrice'] ?? 0)], ['Rabatt Verpflegung', $booking->getBoardServiceDiscount(), (int) ($breakdown['boardPrice'] ?? 0)], @@ -70,15 +71,15 @@ class AccommodationBookingBreakdownCalculator // computing the new total, where the stored one is still the outdated value. $subtotal = (int) ($breakdown['total'] ?? 0) - $discountSum; - // Compounds on the already-reduced subtotal rather than on the gross total: the discount - // is granted on what the customer would otherwise pay, not on a figure no longer asked. - $totalDiscountAmount = self::discountAmount($subtotal, $booking->getTotalDiscount()); + // A fixed sum rather than a percentage, subtracted after the section discounts. Clamped to + // what is left of the subtotal so that a discount larger than the price cannot produce a + // negative total, the same guard BookingSummaryAssembler applies to vouchers. + $totalDiscountAmount = min($booking->getTotalDiscountAmount() ?? 0, max(0, $subtotal)); $breakdown['discounts'] = $discounts; $breakdown['totalDiscountDetails'] = 0 !== $totalDiscountAmount ? [ 'label' => (string) $booking->getTotalDiscountLabel(), - 'percent' => $booking->getTotalDiscount(), 'amount' => $totalDiscountAmount, ] : null; diff --git a/templates/admin/accommodation_booking/edit.html.twig b/templates/admin/accommodation_booking/edit.html.twig index 9af75ee..e0a29d5 100644 --- a/templates/admin/accommodation_booking/edit.html.twig +++ b/templates/admin/accommodation_booking/edit.html.twig @@ -81,7 +81,7 @@
{{ form_row(form.totalDiscountLabel) }} - {{ form_row(form.totalDiscount) }} + {{ form_row(form.totalDiscountAmount) }}
{{ form_row(form.remarks) }} diff --git a/templates/admin/accommodation_booking/index.html.twig b/templates/admin/accommodation_booking/index.html.twig index d259361..88dd9ed 100644 --- a/templates/admin/accommodation_booking/index.html.twig +++ b/templates/admin/accommodation_booking/index.html.twig @@ -84,8 +84,8 @@ {% if booking.additionalServicesDiscount is not null %} {% set discounts = discounts | merge(['Zusatzleistungen ' ~ booking.additionalServicesDiscount ~ '%']) %} {% endif %} - {% if booking.totalDiscount is not null %} - {% set discounts = discounts | merge([(booking.totalDiscountLabel ?: 'Gesamtpreis') ~ ' ' ~ booking.totalDiscount ~ '%']) %} + {% if booking.totalDiscountAmount is not null %} + {% set discounts = discounts | merge([(booking.totalDiscountLabel ?: 'Gesamtpreis') ~ ' ' ~ ((booking.totalDiscountAmount / 100) | format_currency(booking.pricingCurrency ?: 'EUR'))]) %} {% endif %} {{ discounts | length > 0 ? (discounts | join(', ')) : '–' }} diff --git a/templates/admin/accommodation_booking/show.html.twig b/templates/admin/accommodation_booking/show.html.twig index cb7d6c3..d297af5 100644 --- a/templates/admin/accommodation_booking/show.html.twig +++ b/templates/admin/accommodation_booking/show.html.twig @@ -160,7 +160,7 @@ {% endif %} {% if priceBreakdown.totalDiscountDetails is not null %} - {{ priceBreakdown.totalDiscountDetails.label }} {{ priceBreakdown.totalDiscountDetails.percent }} % + {{ priceBreakdown.totalDiscountDetails.label }} – {{ (priceBreakdown.totalDiscountDetails.amount / 100) | format_currency(currency) }} {% endif %} diff --git a/templates/email/_price_breakdown.html.twig b/templates/email/_price_breakdown.html.twig index 2acac1a..750c76c 100644 --- a/templates/email/_price_breakdown.html.twig +++ b/templates/email/_price_breakdown.html.twig @@ -89,7 +89,7 @@ {% endif %} {% if priceBreakdown.totalDiscountDetails is not null %} - {{ priceBreakdown.totalDiscountDetails.label }} {{ priceBreakdown.totalDiscountDetails.percent }} % + {{ priceBreakdown.totalDiscountDetails.label }} – {{ (priceBreakdown.totalDiscountDetails.amount / 100)|format_currency(currency) }} {% endif %} diff --git a/templates/groups/offer/_summary.html.twig b/templates/groups/offer/_summary.html.twig index e363ad8..cf025b8 100644 --- a/templates/groups/offer/_summary.html.twig +++ b/templates/groups/offer/_summary.html.twig @@ -164,7 +164,7 @@ {% endif %} {% if ctx.priceBreakdown.totalDiscountDetails is not null %} - {{ ctx.priceBreakdown.totalDiscountDetails.label }} {{ ctx.priceBreakdown.totalDiscountDetails.percent }} % + {{ ctx.priceBreakdown.totalDiscountDetails.label }} – {{ (ctx.priceBreakdown.totalDiscountDetails.amount / 100)|format_currency(currency) }} {% endif %} diff --git a/tests/Controller/Api/AccommodationBookingControllerTest.php b/tests/Controller/Api/AccommodationBookingControllerTest.php index 978b732..8184384 100644 --- a/tests/Controller/Api/AccommodationBookingControllerTest.php +++ b/tests/Controller/Api/AccommodationBookingControllerTest.php @@ -56,7 +56,7 @@ class AccommodationBookingControllerTest extends TestCase $booking->setAccommodationDiscount(10); $booking->setBoardServiceDiscount(20); $booking->setAdditionalServicesDiscount(30); - $booking->setTotalDiscount(5); + $booking->setTotalDiscountAmount(5000); $booking->setTotalDiscountLabel('Treuerabatt'); $booking->setStatus(AccommodationBookingStatus::Confirmed); $booking->setOrigin(AccommodationBookingOrigin::Direct); @@ -129,7 +129,7 @@ class AccommodationBookingControllerTest extends TestCase 'accommodationDiscount' => 10, 'boardServiceDiscount' => 20, 'additionalServicesDiscount' => 30, - 'totalDiscount' => 5, + 'totalDiscountAmount' => 5000, 'totalDiscountLabel' => 'Treuerabatt', 'totalPrice' => 11111, 'pricingCurrency' => 'EUR', diff --git a/tests/Form/Admin/Groups/AccommodationBookingTypeTest.php b/tests/Form/Admin/Groups/AccommodationBookingTypeTest.php index 7196cf7..72bf1df 100644 --- a/tests/Form/Admin/Groups/AccommodationBookingTypeTest.php +++ b/tests/Form/Admin/Groups/AccommodationBookingTypeTest.php @@ -74,13 +74,13 @@ class AccommodationBookingTypeTest extends TestCase } /** - * The label is printed verbatim on the customer's PDF, so a percentage without one is + * The label is printed verbatim on the customer's PDF, so an amount without one is * refused — and in the Default group, so that even a draft cannot store the pair half-set. */ public function testTotalDiscountRequiresALabel(): void { $booking = new AccommodationBooking(); - $booking->setTotalDiscount(5); + $booking->setTotalDiscountAmount(5000); $violations = $this->validate($booking, ['Default']); @@ -91,17 +91,17 @@ class AccommodationBookingTypeTest extends TestCase public function testALabelledTotalDiscountPassesValidation(): void { $booking = new AccommodationBooking(); - $booking->setTotalDiscount(5); + $booking->setTotalDiscountAmount(5000); $booking->setTotalDiscountLabel('Treuerabatt'); self::assertCount(0, $this->validate($booking, ['Default'])); } /** - * The reverse is harmless — without a percentage the label never reaches a breakdown row — + * The reverse is harmless — without an amount the label never reaches a breakdown row — * so it is deliberately left valid rather than blocking a half-typed edit. */ - public function testALabelWithoutAPercentageIsAccepted(): void + public function testALabelWithoutAnAmountIsAccepted(): void { $booking = new AccommodationBooking(); $booking->setTotalDiscountLabel('Treuerabatt'); diff --git a/tests/Service/AccommodationBookingBreakdownCalculatorTest.php b/tests/Service/AccommodationBookingBreakdownCalculatorTest.php index 593cdad..358920d 100644 --- a/tests/Service/AccommodationBookingBreakdownCalculatorTest.php +++ b/tests/Service/AccommodationBookingBreakdownCalculatorTest.php @@ -101,33 +101,32 @@ class AccommodationBookingBreakdownCalculatorTest extends TestCase /** * On its own the total discount has nothing to sit below, so no subtotal row is offered and - * the percentage applies to the plain total. + * the amount comes straight off the plain total. */ public function testTotalDiscountAloneAppliesToTheTotalWithoutASubtotal(): void { $booking = new AccommodationBooking(); - $booking->setTotalDiscount(10); + $booking->setTotalDiscountAmount(1235); $booking->setTotalDiscountLabel('Treuerabatt'); $breakdown = $this->createCalculator()->withDiscounts($booking, $this->breakdown()); self::assertSame([], $breakdown['discounts']); - self::assertSame(['label' => 'Treuerabatt', 'percent' => 10, 'amount' => 1235], $breakdown['totalDiscountDetails']); + self::assertSame(['label' => 'Treuerabatt', 'amount' => 1235], $breakdown['totalDiscountDetails']); self::assertNull($breakdown['discountSubtotal']); self::assertSame(12345 - 1235, $breakdown['discountedTotal']); } /** - * The worked example from the specification: the total discount compounds on the subtotal - * left by the section discounts, not on the gross total — 5 % of 930,00 € is 46,50 €, where - * 5 % of the gross 1.000,00 € would have been 50,00 €. + * The worked example from the specification: a fixed 50,00 € off the 930,00 € left by the + * section discounts, which is where the Zwischensumme row comes from. */ - public function testTotalDiscountCompoundsOnTheSubtotalAfterSectionDiscounts(): void + public function testTotalDiscountIsSubtractedFromTheSubtotalAfterSectionDiscounts(): void { $booking = new AccommodationBooking(); $booking->setAccommodationDiscount(10); $booking->setBoardServiceDiscount(5); - $booking->setTotalDiscount(5); + $booking->setTotalDiscountAmount(5000); $booking->setTotalDiscountLabel('Treuerabatt'); $breakdown = $this->createCalculator()->withDiscounts($booking, [ @@ -138,14 +137,30 @@ class AccommodationBookingBreakdownCalculatorTest extends TestCase // accommodation: 60000*10% = 6000, board: 20000*5% = 1000 → subtotal 93000 self::assertSame(93000, $breakdown['discountSubtotal']); - self::assertSame(['label' => 'Treuerabatt', 'percent' => 5, 'amount' => 4650], $breakdown['totalDiscountDetails']); - self::assertSame(88350, $breakdown['discountedTotal']); + self::assertSame(['label' => 'Treuerabatt', 'amount' => 5000], $breakdown['totalDiscountDetails']); + self::assertSame(88000, $breakdown['discountedTotal']); + } + + /** + * A sum larger than the price is clamped rather than refused: prices move after the discount + * was agreed, and a negative total would be worse than a free stay. + */ + public function testTotalDiscountLargerThanTheSubtotalIsClampedToIt(): void + { + $booking = new AccommodationBooking(); + $booking->setTotalDiscountAmount(20000); + $booking->setTotalDiscountLabel('Treuerabatt'); + + $breakdown = $this->createCalculator()->withDiscounts($booking, $this->breakdown()); + + self::assertSame(['label' => 'Treuerabatt', 'amount' => 12345], $breakdown['totalDiscountDetails']); + self::assertSame(0, $breakdown['discountedTotal']); } public function testTotalDiscountThatAmountsToNothingIsSkipped(): void { $booking = new AccommodationBooking(); - $booking->setTotalDiscount(20); + $booking->setTotalDiscountAmount(2000); $booking->setTotalDiscountLabel('Treuerabatt'); $breakdown = $this->createCalculator()->withDiscounts($booking, ['total' => 0]);