Commit 54df3e88 authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix(pricing): count a player's age forwards, and hold the picker to the same ceiling as the engine

Carbon 3 signs its differences: `$a->diffIn*($b)` answers `$b − $a`. The
pricing engine asked it the other way round —
`now()->diffInYears($birthday)` — so every participant it priced arrived
at the rules with a negative age and a negative membership duration.

The damage ran in both directions at once. A rule with a `min` never
matched anyone again: the loyalty and annual recipes both ask for twelve
months, and −25 is not twelve. A rule with a `max` matched the entire
academy: the juniors recipe is `max: 6`, and −34 is comfortably under
six, so one click in the rule builder would have taken 10% off every
price in the club. The registration wizard computed age correctly in its
own provisional context, which is why the desk saw one price at
registration and another at renewal.

Age and tenure now read from the older moment forward, through two named
helpers that say why, and a date in the future is no age rather than a
negative one.

Alongside it, the discount picker: `selectedDiscountIds` is a public
Livewire property, so it is a list the browser sends, and the total was
summed from whatever ids arrived. applyDiscount() refuses a blocked rule
and diverts an above-ceiling one into an approval request; neither guard
survived to where the money was worked out. The engine's verdict is
re-read there now, the academy's global discount ceiling applies to a
hand-assembled total exactly as it does at step 8, a manual discount
above the actor's cap reaches neither the total nor the invoice
snapshot, and the picker's state is #[Locked] — it is driven entirely
by wire:click, so nothing needed to arrive from the browser at all.

Also here, found while reading for the above:

- POSTerminal::updateQuantity() did not check the index exists, so an
  invented one wrote a cart line made of a quantity and nothing else.
- The same reversed diff in four other places: overdue invoices and
  renewals reported negative days on the dashboard and in reminder
  messages, expiring memberships reported negative days remaining, and
  a product's months-active pinned to 1, inflating its average monthly
  movement to its entire lifetime sales.
- validateCoupon() still carried a comment promising academy-wide
  coupons, three commits after branch_owns_the_catalogue removed them.

Verified: 379 tests green on SQLite and against the restored OC-Sport
tenant. That tenant carries one pricing rule (sibling_order, 4 EGP) and
no invoice with a discount snapshot, so there is no historical billing
to correct — the bug was waiting on the first age or loyalty rule.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent 579a3a8b
......@@ -28,7 +28,10 @@ public function handle(NotificationService $notificationService): int
continue;
}
$daysOverdue = now()->diffInDays($invoice->due_date);
// From the due date forward: Carbon 3's diff is signed, and asking
// now() for the distance back to a past due date sent every
// reminder out saying the invoice was "-9 days" overdue.
$daysOverdue = (int) $invoice->due_date->diffInDays(now());
$balanceDue = $invoice->total_amount - $invoice->paid_amount;
try {
......
......@@ -8,6 +8,7 @@
use App\Domain\Pricing\Services\DiscountCandidate;
use App\Domain\Pricing\Services\PricingService;
use Illuminate\Database\Eloquent\Model;
use Livewire\Attributes\Locked;
/**
* Drop-in discount picker state for any checkout Livewire component.
......@@ -15,16 +16,24 @@
* The engine decides; this trait only carries the receptionist's choices.
* Nothing here re-derives eligibility — that would let the till and the
* invoice disagree about the price.
*
* Everything except the three manual-discount inputs is #[Locked]. The picker
* is driven entirely by wire:click on applyDiscount()/removeDiscount(), so
* nothing needs to arrive from the browser; left unlocked, the candidate list
* and the selection were both a price the browser could name.
*/
trait ManagesDiscounts
{
/** @var array<int, array> Rendered candidates from PricingService::explain(). */
#[Locked]
public array $discountCandidates = [];
/** @var array<int> Rule ids the operator explicitly turned on. */
#[Locked]
public array $selectedDiscountIds = [];
/** @var array<int> Rule ids the engine applied that the operator turned off. */
#[Locked]
public array $declinedDiscountIds = [];
public string $manualDiscountAmount = '';
......@@ -32,7 +41,10 @@
public string $manualDiscountNote = '';
public ?string $manualDiscountError = null;
#[Locked]
public int $discountBaseAmount = 0;
#[Locked]
public int $discountFinalAmount = 0;
/**
......@@ -166,20 +178,29 @@ public function clearManualDiscount(): void
$this->manualDiscountError = null;
}
/** Total discount in piasters from the operator's current selection. */
/**
* Total discount in piasters from the operator's current selection.
*
* The academy's global discount ceiling applies here exactly as it does at
* step 8 of the engine. Without it the setting only governed the discounts
* nobody chose: a receptionist stacking three rules by hand walked straight
* past a limit the same academy had set to fifty percent.
*/
public function selectedDiscountTotal(): int
{
$total = 0;
foreach ($this->selectedDiscountIds as $id) {
$total += (int) ($this->findCandidate($id)['discount'] ?? 0);
foreach ($this->honouredSelection() as $candidate) {
$total += (int) ($candidate['discount'] ?? 0);
}
if ($this->manualDiscountAmount !== '') {
$total += (int) round((float) $this->manualDiscountAmount * 100);
}
$total += $this->manualDiscountPiasters();
return min($total, $this->discountBaseAmount);
return max(0, min(
$total,
$this->discountBaseAmount,
app(PricingService::class)->maxDiscountFor($this->discountBaseAmount),
));
}
/** Snapshot for the invoice — frozen names, not ids, so receipts stay readable. */
......@@ -187,21 +208,19 @@ public function discountSnapshot(): array
{
$lines = [];
foreach ($this->selectedDiscountIds as $id) {
if ($c = $this->findCandidate($id)) {
$lines[] = [
'rule_id' => $c['rule_id'],
'name' => $c['name'],
'discount' => (int) $c['discount'],
];
}
foreach ($this->honouredSelection() as $candidate) {
$lines[] = [
'rule_id' => $candidate['rule_id'],
'name' => $candidate['name'],
'discount' => (int) $candidate['discount'],
];
}
if ($this->manualDiscountAmount !== '' && $this->manualDiscountReason !== '') {
if ($this->manualDiscountPiasters() > 0) {
$lines[] = [
'rule_id' => null,
'name' => ManualDiscountReason::from($this->manualDiscountReason)->label(),
'discount' => (int) round((float) $this->manualDiscountAmount * 100),
'discount' => $this->manualDiscountPiasters(),
'note' => $this->manualDiscountNote ?: null,
'by' => auth()->id(),
];
......@@ -210,6 +229,65 @@ public function discountSnapshot(): array
return $lines;
}
/**
* The selected candidates the engine is willing to stand behind.
*
* applyDiscount() already refuses a blocked rule and turns one above the
* operator's ceiling into an approval request. Those guards only hold while
* the money is computed from the same verdict they were given, so it is
* re-read here rather than trusted from the selection list.
*/
protected function honouredSelection(): array
{
$honoured = [];
foreach ($this->selectedDiscountIds as $id) {
$candidate = $this->findCandidate($id);
if ($candidate === null) {
continue;
}
if (! in_array($candidate['state'], [
DiscountCandidate::APPLIED,
DiscountCandidate::AVAILABLE,
DiscountCandidate::STACKS,
], true)) {
continue;
}
$honoured[] = $candidate;
}
return $honoured;
}
/**
* The manual discount, in piasters, or nothing when it is not one this
* actor may give. validateManualDiscount() reports the refusal to the
* screen; this keeps the refused amount out of the arithmetic either way.
*/
protected function manualDiscountPiasters(): int
{
if ($this->manualDiscountAmount === '' || $this->manualDiscountReason === '') {
return 0;
}
if (! ManualDiscountReason::tryFrom($this->manualDiscountReason)) {
return 0;
}
$piasters = (int) round((float) $this->manualDiscountAmount * 100);
$check = app(PricingService::class)->checkManualDiscount(
$this->discountBaseAmount,
$piasters,
$this->actorRoleLevel(),
);
return $check['allowed'] ? $piasters : 0;
}
public function getManualReasonOptionsProperty(): array
{
return ManualDiscountReason::options();
......
......@@ -149,8 +149,7 @@ public function calculate(
}
// ─── Step 8: Apply global maximum discount ───────────────────
$maxDiscountPercent = $this->getMaxDiscountPercent();
$maxAllowedTotalDiscount = (int) floor($originalAmount * $maxDiscountPercent / 100);
$maxAllowedTotalDiscount = $this->maxDiscountFor($originalAmount);
if ($totalDiscount > $maxAllowedTotalDiscount) {
$currentAmount = $originalAmount - $maxAllowedTotalDiscount;
$totalDiscount = $maxAllowedTotalDiscount;
......@@ -335,9 +334,7 @@ private function gatherApplicableRules(Model $priceable, ?int $branchId, string
private function buildParticipantContext(Participant $participant): array
{
$person = $participant->person;
$age = $person?->date_of_birth
? (int) now()->diffInYears($person->date_of_birth)
: null;
$age = $this->completedYearsSince($person?->date_of_birth);
// Family size: count participants sharing a guardian
$familySize = 1;
......@@ -358,9 +355,7 @@ private function buildParticipantContext(Participant $participant): array
}
// Membership duration in months
$membershipMonths = $participant->created_at
? (int) now()->diffInMonths($participant->created_at)
: 0;
$membershipMonths = $this->completedMonthsSince($participant->created_at) ?? 0;
// Active enrollment count
$enrollmentCount = $participant->activeEnrollments()->count();
......@@ -377,6 +372,42 @@ private function buildParticipantContext(Participant $participant): array
];
}
/**
* Whole years from a past moment until now — an age, a tenure.
*
* Carbon 3 signs its differences: `$a->diffIn*($b)` answers `$b − $a`, so
* `now()->diffInYears($birthday)` returns a *negative* number of years for
* anyone already born. Cast to int that made every participant's age
* negative, and the damage ran both ways: a rule with `min` (teens, twelve
* months' loyalty) stopped matching anyone, while a rule with `max` — the
* juniors discount is `max: 6` — matched every member of the academy,
* because −34 is comfortably under six. Read from the older moment forward.
*/
private function completedYearsSince(mixed $moment): ?int
{
return $this->completedUnitsSince($moment, 'diffInYears');
}
/** Whole months from a past moment until now. See completedYearsSince(). */
private function completedMonthsSince(mixed $moment): ?int
{
return $this->completedUnitsSince($moment, 'diffInMonths');
}
private function completedUnitsSince(mixed $moment, string $method): ?int
{
if ($moment === null || $moment === '') {
return null;
}
$moment = $moment instanceof \DateTimeInterface
? \Carbon\Carbon::instance($moment)
: \Carbon\Carbon::parse($moment);
// A date in the future is not a negative age; it is no age at all.
return max(0, (int) $moment->{$method}(now()));
}
// ─── Step 3: Condition Evaluation ─────────────────────────────────────
private function evaluateConditions(PricingRule $rule, array $context): bool
......@@ -545,9 +576,14 @@ public function validateCoupon(
// The branch is named explicitly rather than left to the Promotion
// scope: this method runs from queues and console commands too, where
// enforcement is off, and a coupon printed for one branch being honoured
// at another branch's till is money. Promotions are per branch, so
// branch_id NULL is the academy-wide coupon and stays valid everywhere —
// grouped, so the OR cannot be broken apart by a later condition.
// at another branch's till is money.
//
// There is no academy-wide coupon any more: `branch_owns_the_catalogue`
// moved `promotions` into the strict bucket and gave every existing row
// a branch, so a null here is an unfiled row rather than a coupon good
// everywhere, and honouring it at every till would be the leak this
// check exists to close. Grouped so the branch test cannot be broken
// apart by a later condition.
$promotion = Promotion::where('code', $code)
->where('is_active', true)
->when($branchId, fn ($q) => $q->where(
......@@ -649,6 +685,19 @@ private function getMaxDiscountPercent(): int
return ($value > 0 && $value <= 100) ? $value : self::DEFAULT_MAX_DISCOUNT_PERCENT;
}
/**
* The most that may ever come off this amount, whoever assembled the total.
*
* Step 8 caps what the engine works out for itself. The discount picker
* assembles a total of its own — the operator's selections, plus a manual
* line — and it has to answer to the same academy ceiling, or the setting
* governs only the discounts nobody chose. Public so the picker can ask.
*/
public function maxDiscountFor(int $baseAmount): int
{
return (int) floor(max(0, $baseAmount) * $this->getMaxDiscountPercent() / 100);
}
// ─── Picker & Simulator ───────────────────────────────────────────────
/**
......
......@@ -219,7 +219,12 @@ public function expiredMemberships(string $from, string $to, ?int $branchId = nu
'expires_at' => $p->membership_expires_at?->format('Y-m-d'),
'status' => $p->status?->value ?? $p->status,
'branch' => $p->branch?->name_ar ?? '',
'days_until' => $p->membership_expires_at?->diffInDays(now(), false),
// Now forward to the expiry, so a membership with a fortnight
// left reads +14 and an expired one reads negative. Reversed,
// the signed diff put a minus in front of every future date.
'days_until' => $p->membership_expires_at
? (int) now()->diffInDays($p->membership_expires_at, false)
: null,
]);
}
......
......@@ -76,7 +76,10 @@ public function render()
'participant_uuid' => $participant->uuid,
'amount' => $invoice->due_amount,
'program_name' => $this->extractProgramName($invoice->notes),
'days_overdue' => $invoice->due_date?->isPast() ? (int) now()->diffInDays($invoice->due_date) : 0,
// Carbon 3 signs its diffs — $a->diffInDays($b) is $b − $a.
// Asked the other way round, every overdue invoice reported
// a negative number of days and the list sorted backwards.
'days_overdue' => $invoice->due_date?->isPast() ? (int) $invoice->due_date->diffInDays(now()) : 0,
'type' => 'invoice',
]);
}
......@@ -88,7 +91,7 @@ public function render()
'participant_uuid' => $enrollment->participant?->uuid,
'amount' => null,
'program_name' => $enrollment->program?->name_ar ?? '-',
'days_overdue' => (int) now()->diffInDays($enrollment->next_billing_date),
'days_overdue' => (int) ($enrollment->next_billing_date?->diffInDays(now()) ?? 0),
'type' => 'no_invoice',
]);
}
......
......@@ -76,7 +76,10 @@ public function render()
$monthsActive = 1;
if ($firstSale) {
$monthsActive = max(1, (int) now()->diffInMonths($firstSale));
// From the first sale forward. Reversed, the signed diff was always
// negative, max(1, …) pinned it to one month, and every product's
// average monthly movement read as its entire lifetime sales.
$monthsActive = max(1, (int) $firstSale->diffInMonths(now()));
}
$avgMonthlyMovement = round($totalUnitsSold / $monthsActive, 1);
......
......@@ -544,6 +544,14 @@ public function removeItem(int $index): void
public function updateQuantity(int $index, int $quantity): void
{
// setItemPlan() and setPayInstallments() both check this; without it
// here, an index the browser invented wrote a new cart line made of a
// quantity and nothing else — no product, no name, no price — which
// checkout then tried to sell.
if (! isset($this->cart[$index])) {
return;
}
if ($quantity <= 0) {
$this->removeItem($index);
return;
......
<?php
namespace Tests\Unit;
use App\Domain\Pricing\Concerns\ManagesDiscounts;
use App\Domain\Pricing\Services\DiscountCandidate;
use Tests\TestCase;
/**
* What the discount picker is allowed to take off a price.
*
* The picker is authoritative once the receptionist has touched it — the engine
* proposes, the desk disposes — and the total was summed straight from
* `selectedDiscountIds`. That property is public, so it is a list the browser
* sends: applyDiscount()'s refusal of a blocked rule, and its diversion of an
* above-ceiling one into an approval request, were both worth nothing by the
* time the money was worked out. The academy's global discount ceiling was
* missing from that path too, so it governed only the discounts nobody chose.
*
* With no academy bound, SystemSetting::get() answers with its defaults: a 50%
* global ceiling and a 10% manual ceiling for a receptionist.
*/
class DiscountPickerCeilingTest extends TestCase
{
public function test_a_blocked_rule_in_the_selection_is_not_paid_for(): void
{
$till = $this->till([
$this->candidate(1, DiscountCandidate::APPLIED, 10000),
$this->candidate(2, DiscountCandidate::BLOCKED, 40000),
]);
$till->selectedDiscountIds = [1, 2];
$this->assertSame(10000, $till->selectedDiscountTotal());
}
public function test_a_rule_awaiting_approval_is_not_paid_for_either(): void
{
$till = $this->till([$this->candidate(1, DiscountCandidate::NEEDS_APPROVAL, 30000)]);
$till->selectedDiscountIds = [1];
$this->assertSame(0, $till->selectedDiscountTotal());
}
public function test_an_id_that_was_never_offered_is_ignored(): void
{
$till = $this->till([$this->candidate(1, DiscountCandidate::APPLIED, 10000)]);
$till->selectedDiscountIds = [1, 99];
$this->assertSame(10000, $till->selectedDiscountTotal());
}
public function test_stacked_discounts_stop_at_the_academys_ceiling(): void
{
// Three genuine 25% rules on a 1,000 EGP price is 75% — more than the
// academy allows anyone to give, however it was assembled.
$till = $this->till([
$this->candidate(1, DiscountCandidate::APPLIED, 25000),
$this->candidate(2, DiscountCandidate::STACKS, 25000),
$this->candidate(3, DiscountCandidate::STACKS, 25000),
]);
$till->selectedDiscountIds = [1, 2, 3];
$this->assertSame(50000, $till->selectedDiscountTotal());
}
public function test_a_manual_discount_within_the_actors_ceiling_counts(): void
{
$till = $this->till([]);
$till->manualDiscountAmount = '100'; // 100 EGP of 1,000 = 10%
$till->manualDiscountReason = 'hardship';
$this->assertSame(10000, $till->selectedDiscountTotal());
$this->assertCount(1, $till->discountSnapshot());
}
public function test_a_manual_discount_above_the_actors_ceiling_reaches_neither_the_total_nor_the_invoice(): void
{
$till = $this->till([]);
$till->manualDiscountAmount = '200'; // 200 EGP of 1,000 = 20%
$till->manualDiscountReason = 'hardship';
$this->assertFalse($till->validateManualDiscount());
$this->assertSame(0, $till->selectedDiscountTotal());
$this->assertSame([], $till->discountSnapshot());
}
public function test_the_snapshot_records_exactly_what_was_charged(): void
{
$till = $this->till([
$this->candidate(1, DiscountCandidate::APPLIED, 10000, 'خصم الإخوة'),
$this->candidate(2, DiscountCandidate::BLOCKED, 40000, 'منحة دراسية'),
]);
$till->selectedDiscountIds = [1, 2];
$this->assertSame(
[['rule_id' => 1, 'name' => 'خصم الإخوة', 'discount' => 10000]],
$till->discountSnapshot()
);
}
// ---- helpers ----------------------------------------------------------
/** A checkout screen with a 1,000 EGP item on it, operated by a receptionist. */
private function till(array $candidates): object
{
$till = new class
{
use ManagesDiscounts;
public int $roleLevel = 0;
protected function actorRoleLevel(): int
{
return $this->roleLevel;
}
};
$till->discountCandidates = $candidates;
$till->discountBaseAmount = 100000;
$till->discountFinalAmount = 100000;
return $till;
}
private function candidate(int $id, string $state, int $discount, string $name = 'خصم'): array
{
return (new DiscountCandidate(
ruleId: $id,
name: $name,
state: $state,
discountAmount: $discount,
))->toArray();
}
}
<?php
namespace Tests\Unit;
use App\Domain\Pricing\Models\PricingRule;
use App\Domain\Pricing\Services\PricingService;
use Carbon\Carbon;
use Tests\TestCase;
/**
* The participant context the discount rules are judged against.
*
* Carbon 3 signs its differences: `$a->diffIn*($b)` answers `$b − $a`. The
* engine asked `now()->diffInYears($birthday)`, which for anyone already born
* is negative, and the damage ran in both directions at once — a rule with a
* `min` (the twelve-month loyalty discount) stopped matching a single member,
* while a rule with a `max` (the juniors discount, `max: 6`) matched the whole
* academy, because −34 is comfortably under six.
*
* These pin the arithmetic and the two rules it broke.
*/
class PricingContextAgeTest extends TestCase
{
protected function setUp(): void
{
parent::setUp();
Carbon::setTestNow('2026-09-03');
}
protected function tearDown(): void
{
Carbon::setTestNow();
parent::tearDown();
}
// ---- the arithmetic ---------------------------------------------------
public function test_age_is_counted_forward_from_the_birthday(): void
{
$this->assertSame(12, $this->years(Carbon::parse('2014-05-01')));
$this->assertSame(36, $this->years(Carbon::parse('1990-01-01')));
}
public function test_a_birthday_that_has_not_arrived_this_year_is_not_rounded_up(): void
{
// Born 2014-12-31: still 11 on 2026-09-03, not 12.
$this->assertSame(11, $this->years(Carbon::parse('2014-12-31')));
}
public function test_membership_months_are_counted_forward_from_joining(): void
{
$this->assertSame(20, $this->months(Carbon::parse('2025-01-01')));
$this->assertSame(0, $this->months(Carbon::parse('2026-09-01')));
}
public function test_a_missing_date_has_no_age_rather_than_a_wrong_one(): void
{
$this->assertNull($this->years(null));
$this->assertNull($this->years(''));
}
public function test_a_future_date_is_no_age_rather_than_a_negative_one(): void
{
$this->assertSame(0, $this->years(Carbon::parse('2030-01-01')));
$this->assertSame(0, $this->months(Carbon::parse('2030-01-01')));
}
public function test_a_date_string_is_read_as_well_as_a_carbon(): void
{
$this->assertSame(12, $this->years('2014-05-01'));
}
// ---- the rules it broke -----------------------------------------------
public function test_an_adult_does_not_qualify_for_the_juniors_discount(): void
{
$rule = new PricingRule(['rule_type' => 'age', 'conditions' => ['max' => 6]]);
$this->assertFalse($this->evaluate($rule, ['age' => $this->years(Carbon::parse('1990-01-01'))]));
$this->assertTrue($this->evaluate($rule, ['age' => $this->years(Carbon::parse('2021-01-01'))]));
}
public function test_a_member_of_two_years_qualifies_for_the_loyalty_discount(): void
{
$rule = new PricingRule(['rule_type' => 'loyalty', 'conditions' => ['min' => 12]]);
$joined = ['membership_duration_months' => $this->months(Carbon::parse('2024-08-01'))];
$this->assertTrue($this->evaluate($rule, $joined));
$newcomer = ['membership_duration_months' => $this->months(Carbon::parse('2026-08-01'))];
$this->assertFalse($this->evaluate($rule, $newcomer));
}
// ---- helpers ----------------------------------------------------------
private function years(mixed $moment): ?int
{
return $this->invokePrivate('completedYearsSince', $moment);
}
private function months(mixed $moment): ?int
{
return $this->invokePrivate('completedMonthsSince', $moment);
}
private function invokePrivate(string $method, mixed ...$args): mixed
{
$reflected = new \ReflectionMethod(PricingService::class, $method);
$reflected->setAccessible(true);
return $reflected->invoke(app(PricingService::class), ...$args);
}
private function evaluate(PricingRule $rule, array $context): bool
{
return $this->invokePrivate('evaluateConditions', $rule, $context);
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment