Commit d3cbd5f1 authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix(branch): the catalogue belongs to a branch too, and events belong to none

The first pass put programmes, products, prices, promotions, warehouses and
receipt templates in the *shared* bucket, where `branch_id IS NULL` means "every
branch uses this row". The reasoning was that a missing base price is a hard
failure that stops a sale, so hard-filtering the catalogue risked leaving a
branch unable to sell anything.

That bought safety with the wrong currency. A branch is meant to read as its own
installation — its own programmes at its own prices, its own products, its own
stores — and a programme offered at one branch turning up in another branch's
dropdown is the same bug as a group doing it. The shared bucket just hid it
behind a plausible-sounding rule.

The data said the caution was unnecessary. Across the live tenant no group
points at a programme in another branch, no base price prices a programme in
another branch, and exactly three catalogue rows had no branch at all. The
catalogue was already per-branch in practice and merely unlabelled.

  - Programmes, base prices, pricing rules, promotions, products, product
    categories, kits, warehouses, receipt templates and wallets move to the
    strict bucket. `kits` and `product_categories` gain the column; the rest
    only needed their nulls resolved, from actual usage where a link existed
    and from the main branch otherwise.

  - Events go the other way and lose the trait entirely, with their
    registrations. An event is an academy-wide occasion and is genuinely not
    per branch. The column stays — dropping it would be destructive — but
    nothing reads it.

  - Only people and the academy calendar stay shared: employees, trainers,
    guardians, holidays. A coach who works two pitches needs one record visible
    from both, not two that drift. Even there the nulls are narrowed — anyone
    who demonstrably belongs to one branch is pinned to it, which in this tenant
    leaves none shared at all.

  - Pricing rules are the one model whose branch is genuinely many-valued: the
    wizard targets a list through `pricing_rule_branches` and deliberately
    leaves the legacy column null. A column scope would have hidden every rule
    it has ever created, so PricingRule supplies its own scope reading the pivot
    — and BelongsToBranch now lets a model do that.

  - Forms that let a user save a catalogue row with no branch now require one,
    and the "كل الفروع" option is gone from those pickers: on a strictly scoped
    table that choice does not mean every branch, it means none. The setup
    wizard's seeded prices are filed against their programme's branch rather
    than null, so a new academy does not finish setup unable to sell.

  - Comments throughout said "SHARED — branch_id NULL means every branch uses
    this row". They now say what is true.

tests/Feature/PricingSurvivesBranchScopeTest.php is the guard on the original
worry: it prices every live enrolment inside its own branch on each run. It
passes, and reports the one programme that has no active base price at all — a
pre-existing gap, unrelated to scoping. Branch suites: 14 tests / 23,060
assertions against a restored tenant. Standard suite: 224 tests, no failures.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent d714d239
......@@ -16,17 +16,10 @@
use Illuminate\Database\Eloquent\Relations\MorphMany;
use Illuminate\Database\Eloquent\Relations\MorphOne;
use Illuminate\Database\Eloquent\SoftDeletes;
use App\Domain\Shared\Traits\BelongsToBranch;
class Event extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
use BelongsToAcademy, HasUuid, SoftDeletes;
protected $fillable = [
'academy_id', 'branch_id',
......
......@@ -9,19 +9,10 @@
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\SoftDeletes;
use App\Domain\Shared\Traits\ScopedThroughBranch;
class EventRegistration extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, ScopedThroughBranch;
/**
* The parent whose branch this row inherits. A line item has no
* branch of its own and must not grow one — a denormalised copy
* here is a second source of truth that drifts the first time the
* parent moves.
*/
protected static string $branchScopeRelation = 'event';
use BelongsToAcademy, HasUuid, SoftDeletes;
protected $fillable = [
'academy_id',
......
......@@ -14,12 +14,6 @@ class Wallet extends Model
{
use HasUuid, BelongsToAcademy, Auditable, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected $fillable = [
'academy_id', 'branch_id',
'owner_type',
......
......@@ -10,15 +10,16 @@
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\SoftDeletes;
use App\Domain\Shared\Traits\BelongsToBranch;
class Kit extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, Auditable, ManglesUniqueOnDelete;
use BelongsToAcademy, HasUuid, SoftDeletes, Auditable, ManglesUniqueOnDelete, BelongsToBranch;
protected array $uniqueFieldsToMangle = ['sku'];
protected $fillable = [
'academy_id',
'academy_id', 'branch_id',
'name_ar',
'name',
'sku',
......
......@@ -19,12 +19,6 @@ class Product extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, Auditable, ManglesUniqueOnDelete, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected array $uniqueFieldsToMangle = ['sku'];
protected $fillable = [
......
......@@ -9,15 +9,16 @@
use Illuminate\Database\Eloquent\Relations\BelongsTo;
use Illuminate\Database\Eloquent\Relations\HasMany;
use Illuminate\Database\Eloquent\SoftDeletes;
use App\Domain\Shared\Traits\BelongsToBranch;
class ProductCategory extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes;
use BelongsToAcademy, HasUuid, SoftDeletes, BelongsToBranch;
protected $table = 'product_categories';
protected $fillable = [
'academy_id',
'academy_id', 'branch_id',
'name_ar',
'name',
'parent_id',
......
......@@ -18,12 +18,6 @@ class Warehouse extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, ManglesUniqueOnDelete, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected array $uniqueFieldsToMangle = ['code'];
protected $fillable = [
......
......@@ -49,15 +49,12 @@ public function createMovement(
return DB::transaction(function () use ($product, $warehouse, $type, $quantity, $actor, $unitCost, $reference, $reason, $batchNumber, $expiryDate, $expectedDirection) {
// 1. Lock the level row.
//
// Read through the warehouse, not through the branch scope: a level
// row is one warehouse's count of one product, and `warehouses` is
// a shared table where branch_id NULL means every branch uses it.
// Scoped, an academy-wide warehouse's level row (backfilled onto the
// main branch) is invisible from every other branch, and this method
// would then insert a second row for the same product/warehouse pair
// — a duplicate count, or a unique-key failure that stops the sale.
// The caller already holds the Warehouse, which is where the branch
// check belongs.
// Read through the warehouse, not through the branch scope. A
// level row is one warehouse's count of one product; if the scope
// hid an existing row this method would insert a second one for the
// same product/warehouse pair — a duplicate count, or a unique-key
// failure that stops the sale. The caller already holds the
// Warehouse, which is where the branch check belongs.
$level = InventoryLevel::withoutBranchScope()
->where('product_id', $product->id)
->where('warehouse_id', $warehouse->id)
......
......@@ -93,11 +93,10 @@ public function update(Kit $kit, array $data, array $components): Kit
* Every component must be a product this branch may actually sell.
*
* The ids arrive from the kit form and were written straight into
* kit_components. `products` is shared — branch_id NULL means every branch
* — so the check is the shared reading, which the Product scope already
* applies: a product belonging to another branch alone resolves to nothing
* and is refused here rather than becoming a component of a kit every
* branch can sell but only one branch can assemble.
* kit_components. A kit may only bundle products this branch stocks, which
* the Product scope already enforces: another branch's product resolves to
* nothing and is refused here, rather than becoming a component of a kit
* this branch can sell but cannot assemble.
*
* @param array<int, array{product_id?: int|string}> $components
*/
......
......@@ -91,8 +91,7 @@ public function create(array $data, array $items, User $creator): PurchaseOrder
/**
* The warehouse an order's goods may be received into.
*
* `warehouses` is a shared table — branch_id NULL means every branch uses
* it — so the rule is not "same branch" but "not some other branch's". Read
* An order may only be received into a store this branch keeps. Read
* without the branch scope deliberately: this has to be an explicit refusal
* with a message a receptionist can act on, not a ModelNotFound thrown out
* of a scoped findOrFail.
......
......@@ -13,12 +13,6 @@ class ReceiptTemplate extends Model
{
use BelongsToAcademy, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected $fillable = [
'academy_id',
'branch_id',
......@@ -54,12 +48,16 @@ public function creator(): BelongsTo
return $this->belongsTo(User::class, 'created_by');
}
/**
* Kept for callers that resolve a template for a branch other than the
* active one — printing a receipt from a queued job, say, where the global
* scope is not switched on at all.
*/
public function scopeForBranch($query, int $branchId)
{
return $query->where(function ($q) use ($branchId) {
$q->where('branch_id', $branchId)
->orWhereNull('branch_id');
});
return $query
->withoutGlobalScope(\App\Domain\Shared\Scopes\BranchScope::class)
->where('branch_id', $branchId);
}
public function scopeOfType($query, string $type)
......
......@@ -344,15 +344,13 @@ private function buildInvoiceItems(array $cartItems): array
private function createEnrollmentIfNeeded(Participant $participant, int $programId, ?int $groupId, int $branchId): void
{
// The programme and group ids ride in on the cart array from the
// browser. `training_programs` is shared, so branch_id NULL is the
// academy-wide catalogue and must stay sellable; `training_groups` is
// not — a group belongs to exactly one branch, and a till in one branch
// enrolling a player into another branch's group puts that player on a
// roster, an attendance sheet and a session list they will never attend.
// Stated explicitly rather than relying on the request scope, so a
// queued or console sale is refused the same way.
// browser, and a till in one branch enrolling a player into another
// branch's programme puts them on a roster, an attendance sheet and a
// session list they will never attend. Stated explicitly rather than
// relying on the request scope, so a queued or console sale is refused
// the same way.
$program = TrainingProgram::where('id', $programId)
->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'))
->where(fn ($q) => $q->where('branch_id', $branchId))
->first();
if (! $program) {
......@@ -401,12 +399,11 @@ private function deductInventoryIfTracked(int $productId, int $quantity, int $br
return;
}
// Products are shared: branch_id NULL is the academy-wide catalogue and
// every branch may sell it. A product belonging to another branch alone
// is not sellable here — its stock sits in that branch's warehouse — so
// A branch sells its own products. One belonging to another branch is
// not sellable here — its stock sits in that branch's warehouse — so
// refuse rather than sell it and deduct from the wrong shelf.
$product = Product::where('id', $productId)
->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'))
->where(fn ($q) => $q->where('branch_id', $branchId))
->first();
if (! $product) {
......@@ -426,7 +423,7 @@ private function deductInventoryIfTracked(int $productId, int $quantity, int $br
// every branch uses it — skipping those left shared stock never
// deducted).
$warehouse = Warehouse::where('is_active', true)
->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'))
->where(fn ($q) => $q->where('branch_id', $branchId))
->orderByRaw('CASE WHEN branch_id = ? THEN 0 ELSE 1 END', [$branchId])
->first();
......
......@@ -15,12 +15,6 @@ class BasePrice extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected $fillable = [
'academy_id',
'priceable_type',
......
......@@ -16,12 +16,6 @@ class PricingRule extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected $fillable = [
'academy_id',
'name_ar',
......@@ -157,9 +151,25 @@ public function scopeEffectiveOn(Builder $query, $date): Builder
});
}
/**
* The branch this model is isolated by lives in a pivot, not a column.
*/
public static function branchScope(): \Illuminate\Database\Eloquent\Scope
{
return new \App\Domain\Shared\Scopes\PricingRuleBranchScope();
}
/**
* Read the rules targeting one specific branch, whichever branch is active.
*
* Drops the global scope first — without that this would AND the two and
* silently return only the rules both branches share.
*/
public function scopeForBranch(Builder $query, ?int $branchId): Builder
{
return $query->where(function (Builder $q) use ($branchId) {
return $query
->withoutGlobalScope(\App\Domain\Shared\Scopes\PricingRuleBranchScope::class)
->where(function (Builder $q) use ($branchId) {
// Academy-wide = no legacy FK AND no pivot targeting.
$q->where(function (Builder $sq) {
$sq->whereNull('branch_id')->whereNotExists(function ($e) {
......
......@@ -17,12 +17,6 @@ class Promotion extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected $fillable = [
'academy_id',
'name_ar',
......
......@@ -496,13 +496,13 @@ 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` is shared, so
// 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.
$promotion = Promotion::where('code', $code)
->where('is_active', true)
->when($branchId, fn ($q) => $q->where(
fn ($w) => $w->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($w) => $w->where('branch_id', $branchId)
))
->first();
......
<?php
namespace App\Domain\Shared\Scopes;
use App\Domain\Shared\Context\BranchScopeState;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\Scope;
/**
* Branch isolation for the one model whose branch is genuinely many-valued.
*
* Every other branch-owned table answers "which branch?" with a column. A
* pricing rule answers it with a list: `pricing_rule_branches` is how the
* wizard targets "this discount applies at ZSC and Tara", and
* `pricing_rules.branch_id` is the older single-branch column that predates it.
* A plain BranchScope on the column would hide every rule the wizard has ever
* created, because the wizard leaves that column null on purpose and puts the
* targeting in the pivot.
*
* So this mirrors the model's own scopeForBranch(): a rule is visible here if
* it names this branch in either place, or if it names nowhere at all — a rule
* with no column and no pivot rows targets the whole academy, and an
* academy-wide discount is not another branch's data.
*/
class PricingRuleBranchScope implements Scope
{
public function apply(Builder $builder, Model $model): void
{
$state = app(BranchScopeState::class);
if (! $state->shouldFilter()) {
return;
}
$branchId = $state->branchId();
$table = $model->getTable();
// Grouped, so a later orWhere() on the same builder — a search filter,
// say — cannot break the OR apart and leak every branch's rules.
$builder->where(function (Builder $query) use ($branchId, $table) {
$query->where(function (Builder $untargeted) use ($table) {
$untargeted->whereNull($table.'.branch_id')
->whereNotExists(function ($e) use ($table) {
$e->selectRaw('1')->from('pricing_rule_branches')
->whereColumn('pricing_rule_branches.pricing_rule_id', $table.'.id');
});
});
if ($branchId) {
$query->orWhere($table.'.branch_id', $branchId)
->orWhereExists(function ($e) use ($branchId, $table) {
$e->selectRaw('1')->from('pricing_rule_branches')
->whereColumn('pricing_rule_branches.pricing_rule_id', $table.'.id')
->where('pricing_rule_branches.branch_id', $branchId);
});
}
});
}
}
......@@ -507,10 +507,9 @@ public function trainerWorkload(string $from, string $to, ?int $branchId = null)
* an arbitrary warehouse row that could belong to another branch entirely.
* The report then decided 'نفد' / 'منخفض' from someone else's shelf.
*
* Products are shared — branch_id NULL is the academy-wide catalogue and
* must stay visible, or every branch's low-stock list would empty out — so
* the branch filter on them is the shared reading. The levels are summed
* across this branch's warehouses instead of picking one.
* Both halves are branch-filtered now: the products by their own scope,
* and the levels by the warehouses they sit in. The on-hand figure is
* summed across this branch's warehouses instead of picking one.
*/
public function lowStockReport(?int $branchId = null): Collection
{
......@@ -519,14 +518,13 @@ public function lowStockReport(?int $branchId = null): Collection
'inventoryLevels' => fn ($q) => $q->when(
$branchId,
fn ($l) => $l->whereHas('warehouse', fn ($w) => $w
->where('branch_id', $branchId)
->orWhereNull('branch_id'))
->where('branch_id', $branchId))
),
])
->where('track_inventory', true)
->where('is_active', true)
->when($branchId, fn ($q) => $q->where(
fn ($w) => $w->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($w) => $w->where('branch_id', $branchId)
))
->get()
->map(function ($product) {
......
......@@ -43,7 +43,19 @@ public static function bootBelongsToBranch(): void
}
});
static::addGlobalScope(new BranchScope(static::branchScopeAllowsShared()));
static::addGlobalScope(static::branchScope());
}
/**
* The scope this model is isolated by.
*
* Overridable for the one model whose branch is many-valued: a pricing rule
* targets a list of branches through a pivot, not a column, so it supplies
* its own. Everything else wants the column.
*/
public static function branchScope(): \Illuminate\Database\Eloquent\Scope
{
return new BranchScope(static::branchScopeAllowsShared());
}
/**
......@@ -91,7 +103,7 @@ public static function resolveActiveBranchId(): ?int
*/
public function scopeWithoutBranchScope(Builder $query): Builder
{
return $query->withoutGlobalScope(BranchScope::class);
return $query->withoutGlobalScope(static::branchScope()::class);
}
/**
......@@ -99,7 +111,7 @@ public function scopeWithoutBranchScope(Builder $query): Builder
*/
public function scopeForBranch(Builder $query, ?int $branchId): Builder
{
$query = $query->withoutGlobalScope(BranchScope::class);
$query = $query->withoutGlobalScope(static::branchScope()::class);
return $branchId === null
? $query
......
......@@ -20,12 +20,6 @@ class TrainingProgram extends Model
{
use BelongsToAcademy, HasUuid, SoftDeletes, Auditable, ManglesUniqueOnDelete, BelongsToBranch;
/**
* branch_id NULL means every branch uses this row — catalogue and
* configuration, not an operational record owned by one branch.
*/
protected static bool $branchScopeAllowsShared = true;
protected array $uniqueFieldsToMangle = ['slug'];
protected $fillable = [
......
......@@ -479,10 +479,9 @@ private function getRevenueBreakdown($from, $to): array
->where('invoices.subtotal_amount', '>', 0)
->whereBetween('payments.payment_date', [$from, $to])
->when($branchId, fn ($q) => $q->where('payments.branch_id', $branchId))
// products is SHARED — branch_id NULL means every branch sells it —
// so the joined catalogue gets the same OR-null clause BranchScope
// would apply to an Eloquent read, rather than naming another
// branch's products in this branch's revenue table.
// The joined catalogue needs the same clause BranchScope would
// apply to an Eloquent read: a join goes around the scope, and
// without this the revenue table names another branch's products.
->when($branchId, fn ($q) => $q->where(
fn ($p) => $p->where('products.branch_id', $branchId)->orWhereNull('products.branch_id')
))
......@@ -546,12 +545,11 @@ private function buildTopProgramsQuery($from, $to, ?int $academyId, ?int $branch
if ($branchId) {
$filters .= "\n AND p.branch_id = :branch_id";
// training_programs is SHARED: branch_id NULL means every branch
// runs it. Strict equality here would drop the academy-wide
// programmes out of the chart along with their revenue, so the join
// gets the OR-null form BranchScope uses. Bound under its own name
// because a repeated named placeholder is not worth finding out
// about in production.
// The programmes join is filtered too, not just the payments: a
// raw join sees no global scope, and an unfiltered one would name
// another branch's programmes in this branch's chart. Bound under
// its own placeholder name because a repeated named placeholder is
// not worth finding out about in production.
$filters .= "\n AND (tp.branch_id = :branch_id_tp OR tp.branch_id IS NULL)";
$bindings['branch_id'] = $branchId;
$bindings['branch_id_tp'] = $branchId;
......
......@@ -97,9 +97,9 @@ private function branchTrainerUserIds(): array
* happily accept another branch's programme id posted straight into the
* public $programId. The branch clause has to be written out here.
*
* training_programs is a shared table — branch_id NULL means every branch
* uses that programme — so the null is allowed too. In all-branches mode
* there is no branch to compare against and the rule stays open.
* A group may only be opened on a programme this branch actually runs.
* In all-branches mode there is no branch to compare against and the rule
* stays open.
*/
private function programExistsRule(): \Illuminate\Validation\Rules\Exists
{
......@@ -107,7 +107,7 @@ private function programExistsRule(): \Illuminate\Validation\Rules\Exists
$branchId = $this->getActiveBranchId();
if ($branchId !== null) {
$rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
$rule->where(fn ($q) => $q->where('branch_id', $branchId));
}
return $rule;
......
......@@ -112,8 +112,8 @@ private function branchOptions(): Collection
/**
* exists: runs a raw table query and never sees BranchScope, so it would
* accept any programme in the academy posted straight into the public
* $program_id. training_programs is shared — branch_id NULL means every
* branch uses that row — so the null is allowed alongside this branch.
* $program_id. A group may only be opened on a programme this branch
* actually runs.
*/
private function programExistsRule(): \Illuminate\Validation\Rules\Exists
{
......@@ -121,7 +121,7 @@ private function programExistsRule(): \Illuminate\Validation\Rules\Exists
$branchId = $this->getActiveBranchId();
if ($branchId !== null) {
$rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
$rule->where(fn ($q) => $q->where('branch_id', $branchId));
}
return $rule;
......
......@@ -193,14 +193,13 @@ public function confirm(): void
private function rulesForStep(int $step): array
{
// exists: is a raw table query — no academy scope, no branch scope — and
// $items is a plain public array. Both tables are shared catalogue, so
// branch_id NULL ("every branch uses this") stays allowed.
// $items is a plain public array, so both predicates are written out.
$academyId = app('current_academy')->id;
$branchId = $this->getActiveBranchId();
$inBranch = fn ($rule) => $branchId === null
? $rule
: $rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
: $rule->where(fn ($q) => $q->where('branch_id', $branchId));
return match ($step) {
1 => [
......@@ -247,9 +246,8 @@ public function messages(): array
public function render()
{
// Both pickers are branch-filtered by the models' own BranchScope. The
// flat ->where('branch_id', …) that used to narrow the warehouses was a
// bug on a shared-scoped model: it hid every academy-wide warehouse.
// Both pickers are branch-filtered by the models' own BranchScope, so
// neither carries a hand-written branch clause.
return view('livewire.inventory.create-purchase-order-wizard', [
'warehouses' => Warehouse::query()
->where('is_active', true)
......
......@@ -83,8 +83,8 @@ public function rules(): array
{
// unique:/exists: run raw table queries — they see neither the academy
// scope nor the branch scope. kits.sku is unique(academy_id, sku), so
// that rule is academy-scoped; the component products are a shared
// catalogue, so theirs is branch-or-NULL.
// that rule is academy-scoped; the component products are this
// branch's own, so theirs is branch-exact.
$academyId = app('current_academy')->id;
$branchId = $this->getActiveBranchId();
......@@ -112,7 +112,7 @@ public function rules(): array
->where('academy_id', $academyId)
->whereNull('deleted_at')
->when($branchId !== null, fn ($rule) => $rule->where(
fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($q) => $q->where('branch_id', $branchId)
)),
],
'components.*.quantity' => 'required|integer|min:1',
......
......@@ -89,7 +89,7 @@ private function warehouseRule(): \Illuminate\Validation\Rules\Exists
->where('academy_id', app('current_academy')->id)
->whereNull('deleted_at')
->when($branchId !== null, fn ($rule) => $rule->where(
fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($q) => $q->where('branch_id', $branchId)
));
}
......
......@@ -80,9 +80,8 @@ public function render()
// every joined table — filtering only the driving one would still print
// another branch's product and warehouse names into the rows.
//
// inventory_movements and its academy are strict; products and
// warehouses are shared catalogue, where branch_id NULL means "every
// branch uses this", so those two are branch-or-NULL.
// A raw join sees no global scope, so every joined table is filtered
// by hand — the movements, and the products and warehouses they name.
$query = DB::table('inventory_movements')
->join('products', 'inventory_movements.product_id', '=', 'products.id')
->join('warehouses', 'inventory_movements.warehouse_id', '=', 'warehouses.id')
......
......@@ -66,7 +66,7 @@ public function render()
$query = Product::query()
->with(['category', 'inventoryLevels'])
->when($branchId, fn ($q) => $q->where(function ($sub) use ($branchId) {
$sub->where('branch_id', $branchId)->orWhereNull('branch_id');
$sub->where('branch_id', $branchId);
}))
->when($this->search, function ($q) {
$search = $this->search;
......
......@@ -89,14 +89,13 @@ public function rules(): array
// branch scope reaches it — and $warehouse_id and $items are plain
// public properties. Without these predicates the form would file this
// branch's order against another branch's store, or order a product
// that branch alone stocks. Both tables are shared catalogue, so
// branch_id NULL ("every branch uses this") has to stay allowed.
// that branch alone stocks.
$academyId = app('current_academy')->id;
$branchId = $this->getActiveBranchId();
$inBranch = fn ($rule) => $branchId === null
? $rule
: $rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
: $rule->where(fn ($q) => $q->where('branch_id', $branchId));
return [
'supplier_name' => 'required|string|max:255',
......@@ -189,9 +188,8 @@ public function save(): void
public function render()
{
// Both pickers are branch-filtered by the models' own BranchScope. The
// flat ->where('branch_id', …) that used to narrow the warehouses was a
// bug on a shared-scoped model: it hid every academy-wide warehouse.
// Both pickers are branch-filtered by the models' own BranchScope, so
// neither carries a hand-written branch clause.
return view('livewire.inventory.purchase-order-form', [
'warehouses' => Warehouse::query()
->where('is_active', true)
......
......@@ -36,15 +36,14 @@ public function rules(): array
// exists: is a raw table query — neither the academy scope nor the
// branch scope reaches it — and both ids are plain public properties
// bound with wire:model, so the browser can send whatever it likes.
// Product and Warehouse are both shared catalogue, where branch_id NULL
// means "every branch uses this row", hence branch-or-NULL rather than
// a flat equality that would hide the academy-wide rows.
// The adjustment has to name this branch's own product and its own
// store.
$academyId = app('current_academy')->id;
$branchId = $this->getActiveBranchId();
$inBranch = fn ($rule) => $branchId === null
? $rule
: $rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
: $rule->where(fn ($q) => $q->where('branch_id', $branchId));
return [
'product_id' => ['required', $inBranch(
......
......@@ -185,14 +185,13 @@ private function rulesForStep(int $step): array
{
// exists: is a raw table query and sees neither the academy nor the
// branch scope, so the step gates have to carry the predicate
// themselves. Product and Warehouse are shared catalogue — branch_id
// NULL means every branch uses the row — hence branch-or-NULL.
// themselves: this branch's own products, and its own stores.
$academyId = app('current_academy')->id;
$branchId = $this->getActiveBranchId();
$inBranch = fn ($rule) => $branchId === null
? $rule
: $rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
: $rule->where(fn ($q) => $q->where('branch_id', $branchId));
return match ($step) {
1 => [
......@@ -217,10 +216,8 @@ private function rulesForStep(int $step): array
public function render()
{
// Both lists are branch-filtered by the models' own BranchScope. The
// flat ->where('branch_id', $branchId) that used to sit on the warehouse
// query was worse than redundant: Warehouse is shared-scoped, so it hid
// every academy-wide warehouse from every branch.
// Both lists are branch-filtered by the models' own BranchScope, so
// neither carries a hand-written branch clause.
$warehouses = Warehouse::active()
->orderBy('name_ar')->get(['id', 'name_ar', 'name', 'code']);
......
......@@ -50,7 +50,7 @@ public function rules(): array
{
// exists: is a raw table query — no academy scope, no branch scope — and
// $warehouse_id is a plain public property driving updatedWarehouseId().
// Warehouse is shared catalogue, so branch_id NULL stays allowed.
// Only this branch's stores: a branch counts its own stock.
$academyId = app('current_academy')->id;
$branchId = $this->getActiveBranchId();
......@@ -60,7 +60,7 @@ public function rules(): array
if ($branchId !== null) {
$warehouseExists->where(
fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($q) => $q->where('branch_id', $branchId)
);
}
......@@ -205,9 +205,8 @@ public function save(): void
public function render()
{
return view('livewire.inventory.stock-count-form', [
// Branch comes from Warehouse's own scope. The flat
// ->where('branch_id', …) that stood here hid every academy-wide
// warehouse, because Warehouse is shared-scoped.
// Branch comes from Warehouse's own scope — no hand-written
// clause here, and none needed.
'warehouses' => Warehouse::active()
->orderBy('name_ar')->get(['id', 'name_ar', 'code']),
]);
......
......@@ -79,10 +79,9 @@ public function render()
{
// StockCount carries its own branch_id under BelongsToBranch, so the
// list is already narrowed. The whereHas('warehouse', branch_id = …)
// that stood here was both redundant and wrong: Warehouse is
// shared-scoped, so it dropped every count taken in an academy-wide
// store — and finalize()/cancel() below never had it, so the actions
// and the list they act on disagreed about what exists.
// that stood here was redundant, and finalize()/cancel() below never
// had it — so the actions and the list they act on disagreed about what
// exists.
$query = StockCount::query()
->with(['warehouse', 'creator'])
->withCount('items')
......
......@@ -72,9 +72,11 @@ public function rules(): array
// and $branch_id is a plain public property, so a warehouse (and
// every level and movement hanging off it) could be moved into
// another branch from the browser. selectableBranches() is one entry
// unless the user is genuinely in all-branches mode. null stays
// allowed: on a shared-scoped model it means "every branch".
'branch_id' => ['nullable', Rule::in($this->selectableBranches()->pluck('id')->all())],
// unless the user is genuinely in all-branches mode. Required
// rather than nullable: the row is strictly branch-scoped, so an
// empty picker would file the store against no branch and hide it
// from every one of them.
'branch_id' => ['required', Rule::in($this->selectableBranches()->pluck('id')->all())],
'manager_id' => ['nullable', Rule::in($this->selectableManagerIds())],
'address' => 'nullable|string|max:500',
'is_active' => 'boolean',
......@@ -132,6 +134,7 @@ public function messages(): array
'code.max' => 'كود المستودع يجب ألا يتجاوز 20 حرف',
'type.required' => 'نوع المستودع مطلوب',
'type.in' => 'نوع المستودع غير صالح',
'branch_id.required' => 'يجب اختيار الفرع',
'branch_id.in' => 'الفرع غير متاح',
'manager_id.in' => 'المدير غير متاح',
];
......
......@@ -37,11 +37,9 @@ public function updatedSearch(): void
public function render()
{
// Warehouse is shared-scoped: BranchScope already reads this branch's
// stores plus the academy-wide ones. The flat ->where('branch_id', …)
// that stood here hid every academy-wide warehouse from every branch —
// the same rows the pickers on the count, adjustment and purchase-order
// screens legitimately offer.
// No hand-written branch clause: BranchScope already narrows this to
// the branch's own stores, and a second one here would only be a copy
// that drifts.
$query = Warehouse::query()
->with(['branch', 'inventoryLevels'])
->withCount('inventoryLevels')
......
......@@ -549,7 +549,7 @@ public function render()
$products = Product::where('is_active', true)
->when($branchId, fn ($q) => $q->where(function ($sub) use ($branchId) {
$sub->where('branch_id', $branchId)->orWhereNull('branch_id');
$sub->where('branch_id', $branchId);
}))
->orderBy('name_ar')
->get();
......
......@@ -90,18 +90,18 @@ public function mount(?BasePrice $basePrice = null): void
* BranchScope is not yet enforcing when the router resolves {basePrice}.
* The record has to be checked here by hand.
*
* base_prices is a SHARED branch table: branch_id NULL is the academy-wide
* price every branch falls back to, and it stays editable from anywhere.
* A null active branch is a user genuinely in all-branches mode.
* A price belongs to the branch that charges it, so only that branch may
* open it.
*/
private function assertBranchVisible(?int $branchId): void
{
$active = $this->getActiveBranchId();
abort_if(
$active !== null && $branchId !== null && $branchId !== $active,
404
);
// A null active branch is a user genuinely in all-branches mode, who
// may reach anything. A null on the record itself is a row the backfill
// missed: strictly scoped, it belongs to no branch and is editable from
// none, so refuse rather than let one branch quietly adopt it.
abort_if($active !== null && $branchId !== $active, 404);
}
/**
......@@ -121,14 +121,11 @@ private function priceableExistsRule(): mixed
$rule->where('academy_id', $academyId);
}
// training_programs is SHARED too — branch_id NULL is the
// academy-wide programme every branch prices, so it must stay
// selectable rather than being filtered out with the other
// branches' programmes.
// exists: is a raw query, so the global scope on TrainingProgram
// does not reach it. A price may only be attached to a programme
// this branch actually runs.
if ($branchId !== null) {
$rule->where(fn ($q) => $q
->where('branch_id', $branchId)
->orWhereNull('branch_id'));
$rule->where('branch_id', $branchId);
}
return $rule;
......@@ -159,7 +156,10 @@ public function rules(): array
// Not exists:branches,id — that accepts every branch in the academy
// from an input the browser controls. Only what the picker may
// legitimately offer, plus the empty "all branches" value.
'branch_id' => ['nullable', Rule::in($this->selectableBranches()->pluck('id')->all())],
// Required, not nullable: the row is strictly branch-scoped, so an
// empty picker would file it against no branch and hide it from
// every one of them.
'branch_id' => ['required', Rule::in($this->selectableBranches()->pluck('id')->all())],
'amount' => 'required|numeric|min:0.01',
'effective_from' => 'required|date',
'effective_to' => 'nullable|date|after:effective_from',
......@@ -180,6 +180,7 @@ public function messages(): array
'effective_from.required' => __('تاريخ البداية مطلوب'),
'effective_to.after' => __('تاريخ النهاية يجب أن يكون بعد تاريخ البداية'),
'priceable_id.exists' => __('العنصر المحدد غير صالح'),
'branch_id.required' => 'يجب اختيار الفرع',
'branch_id.in' => __('الفرع المحدد غير صالح'),
];
}
......@@ -209,16 +210,6 @@ public function save(): void
$data['created_by'] = auth()->id();
$created = BasePrice::create($data);
// BelongsToBranch stamps the active branch onto any create that
// leaves branch_id empty. That is right for an operational record
// and wrong here: an empty picker means "every branch uses this",
// and the stamp would pin an academy-wide row to whichever branch
// happened to author it. Rule 19 spells out what that costs —
// PricingService::findBasePrice() falls back to the branch_id NULL
// row, and no active base price is a hard failure that stops a sale.
if ($data['branch_id'] === null) {
$created->update(['branch_id' => null]);
}
session()->flash('success', __('تم إنشاء السعر بنجاح'));
}
......
......@@ -390,12 +390,13 @@ public function save(): void
'min_role_level' => $this->minRoleLevel,
]);
// BelongsToBranch stamps the active branch onto any create
// that leaves branch_id empty, so passing null above is not
// enough to keep the legacy column out of the way. Unpinned, a
// rule the author meant for every branch would apply in theirs
// alone — and PricingRule::scopeForBranch() reads this column
// as one more target beside the pivot.
// The pivot below is where this wizard states its targeting,
// so the legacy single-branch column has to stay empty or it
// would silently become a second, contradictory target.
// BelongsToBranch stamps the active branch onto any create that
// leaves it null, hence undoing it here. PricingRuleBranchScope
// reads the pivot, so a rule targeted this way is still visible
// only at the branches it names.
if ($rule->branch_id !== null) {
$rule->update(['branch_id' => null]);
}
......
......@@ -137,7 +137,10 @@ public function rules(): array
// Not exists:branches,id — a raw table query accepts every branch
// in the academy. Only what the picker may legitimately offer, plus
// the empty "all branches" value.
'branch_id' => ['nullable', Rule::in($this->selectableBranches()->pluck('id')->all())],
// Required, not nullable: the row is strictly branch-scoped, so an
// empty picker would file it against no branch and hide it from
// every one of them.
'branch_id' => ['required', Rule::in($this->selectableBranches()->pluck('id')->all())],
'priority' => 'integer|min:0',
'is_stackable' => 'boolean',
'max_discount_percent' => 'nullable|integer|min:1|max:100',
......@@ -176,6 +179,7 @@ public function messages(): array
'effective_to.after' => __('تاريخ النهاية يجب أن يكون بعد تاريخ البداية'),
'max_discount_percent.min' => __('الحد الأقصى للخصم يجب أن يكون بين 1 و 100'),
'max_discount_percent.max' => __('الحد الأقصى للخصم يجب أن يكون بين 1 و 100'),
'branch_id.required' => 'يجب اختيار الفرع',
'branch_id.in' => __('الفرع المحدد غير صالح'),
'conditions.values.*.in' => __('الفرع المحدد غير صالح'),
];
......@@ -229,16 +233,6 @@ public function save(): void
$data['usage_count'] = 0;
$created = PricingRule::create($data);
// BelongsToBranch stamps the active branch onto any create that
// leaves branch_id empty. That is right for an operational record
// and wrong here: an empty picker means "every branch uses this
// rule", and the stamp would pin an academy-wide discount to
// whichever branch happened to author it —
// PricingRule::scopeForBranch() reads this column as a target, so
// every other branch would silently stop seeing the discount.
if ($data['branch_id'] === null) {
$created->update(['branch_id' => null]);
}
session()->flash('success', __('تم إنشاء القاعدة بنجاح'));
}
......
......@@ -96,18 +96,18 @@ public function mount(?Promotion $promotion = null): void
* The record — including its coupon code and usage counters — has to be
* checked here by hand.
*
* promotions is a SHARED branch table: branch_id NULL is the academy-wide
* offer every branch honours, and it stays editable from anywhere. A null
* active branch is a user genuinely in all-branches mode.
* An offer belongs to the branch that runs it, so only that branch may
* open it.
*/
private function assertBranchVisible(?int $branchId): void
{
$active = $this->getActiveBranchId();
abort_if(
$active !== null && $branchId !== null && $branchId !== $active,
404
);
// A null active branch is a user genuinely in all-branches mode, who
// may reach anything. A null on the record itself is a row the backfill
// missed: strictly scoped, it belongs to no branch and is editable from
// none, so refuse rather than let one branch quietly adopt it.
abort_if($active !== null && $branchId !== $active, 404);
}
private function formatAdjustmentForDisplay(Promotion $promotion): string
......@@ -133,7 +133,10 @@ public function rules(): array
// Not exists:branches,id — a raw table query accepts every branch
// in the academy. Only what the picker may legitimately offer, plus
// the empty "all branches" value.
'branch_id' => ['nullable', Rule::in($this->selectableBranches()->pluck('id')->all())],
// Required, not nullable: the row is strictly branch-scoped, so an
// empty picker would file it against no branch and hide it from
// every one of them.
'branch_id' => ['required', Rule::in($this->selectableBranches()->pluck('id')->all())],
'min_purchase_amount' => 'nullable|numeric|min:0',
'max_discount_amount' => 'nullable|numeric|min:0',
'start_date' => 'required|date',
......@@ -158,6 +161,7 @@ public function messages(): array
'adjustment_value.min' => __('قيمة التعديل يجب أن تكون أكبر من صفر'),
'start_date.required' => __('تاريخ البداية مطلوب'),
'end_date.after' => __('تاريخ النهاية يجب أن يكون بعد تاريخ البداية'),
'branch_id.required' => 'يجب اختيار الفرع',
'branch_id.in' => __('الفرع المحدد غير صالح'),
];
}
......@@ -196,14 +200,6 @@ public function save(): void
$data['usage_count'] = 0;
$created = Promotion::create($data);
// BelongsToBranch stamps the active branch onto any create that
// leaves branch_id empty. That is right for an operational record
// and wrong here: an empty picker means "every branch honours this
// offer", and the stamp would pin an academy-wide promotion — and
// its coupon code — to whichever branch happened to author it.
if ($data['branch_id'] === null) {
$created->update(['branch_id' => null]);
}
session()->flash('success', __('تم إنشاء العرض بنجاح'));
}
......
......@@ -145,11 +145,11 @@ public function mount(?TrainingProgram $program = null): void
} else {
$this->authorize('programs.create');
// A new programme belongs to the branch being worked in. In
// all-branches mode this stays null — the picker appears there and
// asks, and null is a legitimate answer for a programme: the
// training_programs row is shared, so NULL means every branch uses
// it (which is why ProgramList shows it under 'كل الفروع').
// A programme belongs to exactly one branch — a branch runs its
// own programmes at its own prices. In all-branches mode there is
// no active branch to inherit, so the picker asks and the rule
// below refuses to save without an answer: a programme with no
// branch is not academy-wide, it is invisible from every branch.
$this->branch_id = $this->getActiveBranchId();
}
}
......@@ -165,7 +165,9 @@ public function rules(): array
// in the academy, and this property is settable from the browser.
// selectableBranches() is one entry unless the user is genuinely
// in all-branches mode.
'branch_id' => ['nullable', Rule::in($this->selectableBranches()->pluck('id')->all())],
// Required, not nullable: TrainingProgram is strictly scoped, so a
// null branch would hide the programme from every branch at once.
'branch_id' => ['required', Rule::in($this->selectableBranches()->pluck('id')->all())],
'default_trainer_id' => ['nullable', Rule::in($this->trainerOptions()->pluck('id')->all())],
'description' => 'nullable|string',
'description_ar' => 'nullable|string',
......@@ -196,8 +198,7 @@ public function rules(): array
'non_member_price' => 'nullable|numeric|min:0',
// The checkbox list is filtered by Product's branch scope, but the
// array itself is posted from the browser, so the ids are checked
// again here. products is shared: NULL branch_id is the
// academy-wide catalogue every branch sells from.
// again here, against the products this branch actually sells.
'bundled_product_ids' => ['array'],
'bundled_product_ids.*' => ['integer', $this->productExistsRule()],
];
......@@ -213,7 +214,7 @@ private function productExistsRule(): \Illuminate\Validation\Rules\Exists
$branchId = $this->getActiveBranchId();
if ($branchId !== null) {
$rule->where(fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id'));
$rule->where(fn ($q) => $q->where('branch_id', $branchId));
}
return $rule;
......@@ -282,6 +283,7 @@ public function messages(): array
'name_ar.max' => 'اسم البرنامج بالعربية يجب ألا يتجاوز 255 حرف',
'activity_id.required' => 'النشاط مطلوب',
'activity_id.exists' => 'النشاط المحدد غير موجود',
'branch_id.required' => 'يجب اختيار الفرع',
'branch_id.in' => 'الفرع المحدد غير متاح',
'default_trainer_id.in' => 'المدرب المحدد غير متاح في هذا الفرع',
'bundled_product_ids.*.exists' => 'أحد المنتجات المحددة غير متاح في هذا الفرع',
......@@ -376,14 +378,6 @@ public function save(TrainingProgramService $service): void
} else {
$program = $service->create($data, auth()->user());
// A user genuinely in all-branches mode who chose 'كل الفروع'
// means it: the programme is shared, and every branch uses it.
// BelongsToBranch stamps the actor's own branch onto a row
// created with a null branch_id, which would pin the programme
// to that one branch and hide it from all the others.
if ($this->branch_id === null && $program->branch_id !== null) {
$program->forceFill(['branch_id' => null])->save();
}
$this->savePrices($program);
$this->saveBundledProducts($program);
......@@ -471,16 +465,12 @@ private function savePrices(TrainingProgram $program): void
'metadata' => ['membership_type' => $membershipType],
]);
// BelongsToBranch stamps the active branch onto any row
// created with a null branch_id. For an academy-wide
// programme (branch_id NULL, which is what shared mode
// means) that would pin its only price to whichever branch
// happened to save the form, and every other branch would
// be left with no active base price — a hard failure that
// stops the sale. The programme's own branch wins.
if ($program->branch_id === null && $price->branch_id !== null) {
$price->forceFill(['branch_id' => null])->save();
}
// The price is filed against the programme's branch, passed
// explicitly above rather than left to BelongsToBranch's
// stamp — the actor's active branch and the programme's are
// the same in practice, but a price that disagreed with the
// thing it prices would be invisible exactly where it is
// needed.
}
} elseif ($existing) {
$existing->update(['is_active' => false]);
......
......@@ -77,13 +77,12 @@ public function rules(): array
2 => [
// activities are academy-level: they carry no branch.
'selected_activity_id' => 'required|exists:activities,id',
// training_programs is a shared table — a null branch means
// every branch uses that programme, so the null must pass too
// or the academy-wide catalogue becomes unenrollable.
// A branch runs its own programmes, so only this branch's
// pass. exists: is raw SQL and never sees BranchScope.
'selected_program_id' => [
'required',
Rule::exists('training_programs', 'id')->where(
fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($q) => $q->where('branch_id', $branchId)
),
],
],
......
......@@ -371,13 +371,13 @@ private function rulesForStep(int $step): array
// reaches, and selected_program_id is a public property. The
// dropdown is filtered; without this rule the dropdown was the
// only thing standing between a foreign programme and an
// enrolment. training_programs is shared, so a null branch —
// "every branch uses this programme" — has to pass too.
// enrolment. A branch runs its own programmes, so only this
// branch's pass.
'selected_program_id' => [
'required',
Rule::exists('training_programs', 'id')->where(
fn ($q) => $q->where('branch_id', $this->getActiveBranchIdOrFail())
->orWhereNull('branch_id')
),
],
],
......
......@@ -93,15 +93,14 @@ public function mount(): void
/**
* `exists:` runs a raw table query, so no Eloquent global scope reaches it,
* and selected_program_id is a public property the browser can set.
* training_programs is a shared table — a null branch means every branch
* uses that programme, so the null has to pass too.
* A branch runs its own programmes, so only this branch's pass.
*/
private function programExistsRule(): \Illuminate\Validation\Rules\Exists
{
$branchId = $this->getActiveBranchIdOrFail();
return Rule::exists('training_programs', 'id')->where(
fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id')
fn ($q) => $q->where('branch_id', $branchId)
);
}
......
......@@ -723,7 +723,11 @@ public function completeSetup(): void
'academy_id' => $academyId,
'priceable_type' => TrainingProgram::class,
'priceable_id' => $program->id,
'branch_id' => null,
// The price is filed where the programme is. A null here
// used to mean "every branch"; base_prices is strictly
// scoped now, so it would mean "no branch at all" and the
// academy would finish its own setup wizard unable to sell.
'branch_id' => $program->branch_id,
'name_ar' => 'الاشتراك الشهري — ' . $programData['name_ar'],
'name' => 'Monthly — ' . $programData['name_ar'],
'amount' => $monthlyFeePiasters,
......@@ -741,7 +745,7 @@ public function completeSetup(): void
'academy_id' => $academyId,
'priceable_type' => TrainingProgram::class,
'priceable_id' => $program->id,
'branch_id' => null,
'branch_id' => $program->branch_id,
'name_ar' => 'رسوم التسجيل — ' . $programData['name_ar'],
'name' => 'Registration Fee — ' . $programData['name_ar'],
'amount' => $registrationFeePiasters,
......
......@@ -47,22 +47,47 @@ bucket **before** writing the migration.
### STRICT — `use BelongsToBranch;`
An operational record owned by exactly one branch: a player, a group, a
facility, an invoice, a payment, an attendance record, a cash session. Filtered
`WHERE branch_id = :active`. **A null branch here is a bug**, not a value — the
row would be invisible from every branch at once. Backfill it in the migration
and give it a fallback.
The default, and where nearly everything belongs. Filtered
`WHERE branch_id = :active`.
Two groups sit here:
- **Operational records** owned by exactly one branch — a player, a group, an
enrolment, a session, an attendance record, a facility, an invoice, a payment,
a cash session, a stock movement.
- **The catalogue** — programmes, base prices, pricing rules, promotions,
products, product categories, kits, warehouses, receipt templates, wallets.
A branch runs its own programmes at its own prices and sells its own stock.
A programme offered at one branch appearing in another branch's dropdown is
the same bug as a group doing it.
**A null branch here is a bug**, not a value — the row would be invisible from
every branch at once. Backfill it in the migration and give it a fallback.
The catalogue was briefly modelled as shared, on the reasoning that a missing
base price is a hard failure that stops a sale. That was the wrong trade: it
bought safety by making every branch's price list visible everywhere. The right
fix is to make sure every branch has its own prices — which the backfill in
`2026_09_13_000002` does, and which
`tests/Feature/PricingSurvivesBranchScopeTest.php` keeps honest by pricing every
live enrolment inside its own branch on each run.
### SHARED — `use BelongsToBranch;` + `protected static bool $branchScopeAllowsShared = true;`
Catalogue and configuration, where `branch_id IS NULL` means *every branch uses
this row*: products, base prices, pricing rules, promotions, receipt templates,
programmes, holidays, warehouses, employees, trainers. Filtered
`WHERE (branch_id = :active OR branch_id IS NULL)`.
Filtered `WHERE (branch_id = :active OR branch_id IS NULL)`, where null means
*every branch uses this row*.
Hard-filtering these would be a production outage, not a tightening: pinning the
academy-wide base price to one branch leaves every other branch with no active
price, and *no active base price is a hard failure that stops a sale*.
Reserved for the few things that genuinely belong to more than one branch at
once: **people** (employees, trainers, guardians) and **the academy calendar**
(holidays). A coach who works Monday at one pitch and Wednesday at another needs
one record visible from both, not two records that drift apart.
Even here, null is for real sharing only. Anyone who demonstrably belongs to a
single branch is pinned to it by the migration; only a person actually spanning
branches keeps the null.
Do not reach for this bucket to avoid a backfill. If a table is shared because
its rows have no branch yet, the fix is the backfill.
### CHILD — `use ScopedThroughBranch;` + `protected static string $branchScopeRelation = 'parent';`
......@@ -78,9 +103,20 @@ missing, which is correct for a line item and wrong for an optional link.
### GLOBAL — no trait
Genuinely academy-level: the website CMS, roles, permissions, the chart of
accounts, audit logs, activities, evaluation criteria, system settings, people.
Branch has no meaning here.
Genuinely academy-level: the website CMS, the member portal's own tables, roles,
permissions, the chart of accounts, audit logs, activities, evaluation criteria,
system settings, people.
**Events** are here too, with their registrations. An event is an academy-wide
occasion — a tournament, an open day — and is one of the few operational things
that is deliberately not per branch. `events.branch_id` still exists (dropping a
column is destructive, and a per-branch event is plausible later) but nothing
reads it.
The **member portal** is global in a different sense: it is scoped by
membership, not by branch, so enforcement is switched off for member accounts
entirely. A signed-in member sees their own branch's world because their own
records live there — not because a filter put them there.
---
......@@ -108,7 +144,7 @@ that is a cross-branch write, not just a read.
For a SHARED table, allow the null too:
```php
Rule::exists('products', 'id')->where(
Rule::exists('trainers', 'id')->where(
fn ($q) => $q->where('branch_id', $branchId)->orWhereNull('branch_id')
)
```
......
......@@ -59,7 +59,7 @@ class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2
<label for="branch_id" class="block text-sm font-medium text-gray-700 mb-1">{{ __('الفرع') }}</label>
<select id="branch_id" wire:model="branch_id"
class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2 focus:ring-blue-500 focus:border-blue-500 @error('branch_id') border-red-500 @enderror">
<option value="">{{ __('-- كل الفروع --') }}</option>
<option value="">{{ __('اختر الفرع') }}</option>
@foreach($branches as $branch)
<option value="{{ $branch->id }}">{{ $branch->name_ar }}</option>
@endforeach
......
......@@ -63,7 +63,7 @@ class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2
<label for="branch_id" class="block text-sm font-medium text-gray-700 mb-1">{{ __('الفرع') }}</label>
<select id="branch_id" wire:model="branch_id"
class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2 focus:ring-blue-500 focus:border-blue-500">
<option value="">{{ __('كل الفروع') }}</option>
<option value="">{{ __('اختر الفرع') }}</option>
@foreach($branches as $branch)
<option value="{{ $branch->id }}">{{ $branch->name_ar }}</option>
@endforeach
......
......@@ -78,7 +78,7 @@ class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2
<label for="branch_id" class="block text-sm font-medium text-gray-700 mb-1">{{ __('الفرع') }}</label>
<select id="branch_id" wire:model="branch_id"
class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2 focus:ring-blue-500 focus:border-blue-500">
<option value="">{{ __('كل الفروع') }}</option>
<option value="">{{ __('اختر الفرع') }}</option>
@foreach($branches as $branch)
<option value="{{ $branch->id }}">{{ $branch->name_ar }}</option>
@endforeach
......
......@@ -87,7 +87,7 @@ class="w-full px-4 py-2 border border-gray-300 rounded-lg focus:ring-2 focus:rin
<label for="branch_id" class="block text-sm font-medium text-gray-700 mb-1">{{ __('الفرع') }}</label>
<select id="branch_id" wire:model="branch_id"
class="w-full px-4 py-2 border border-gray-300 rounded-lg focus:ring-2 focus:ring-blue-500 focus:border-blue-500">
<option value="">{{ __('كل الفروع') }}</option>
<option value="">{{ __('اختر الفرع') }}</option>
@foreach($branches as $branch)
<option value="{{ $branch->id }}">{{ $branch->name_ar }}</option>
@endforeach
......
......@@ -36,14 +36,15 @@ class="text-sm text-gray-600 hover:text-gray-800">{{ __('← العودة للق
</select>
@error('activity_id') <p class="mt-1 text-xs text-red-600">{{ $message }}</p> @enderror
</div>
{{-- Branch is the topbar switcher's job. This picker appears only
for a user genuinely viewing every branch, who has to say
whether the programme belongs to one branch or to all. --}}
{{-- Branch is the topbar switcher's job. This picker appears
only for a user genuinely viewing every branch, who has to
name the one branch the programme runs at — a programme
belongs to exactly one, the same as a group does. --}}
@if($branches->count() > 1)
<div>
<label class="block text-sm font-medium text-gray-700 mb-1">{{ __('الفرع') }}</label>
<select wire:model="branch_id" class="w-full px-4 py-2.5 text-sm border border-gray-300 rounded-lg focus:ring-2 focus:ring-blue-500 focus:border-blue-500">
<option value="">{{ __('كل الفروع') }}</option>
<option value="">{{ __('اختر الفرع') }}</option>
@foreach($branches as $branch)
<option value="{{ $branch->id }}">{{ $branch->name_ar }}</option>
@endforeach
......
......@@ -45,17 +45,34 @@ class BranchIsolationTest extends TestCase
\App\Domain\Scheduling\Models\Assignment::class => 'assignments',
\App\Domain\POS\Models\POSTransaction::class => 'pos_transactions',
\App\Domain\Financial\Models\CashSession::class => 'cash_sessions',
];
/** Models where branch_id null means "every branch uses this row". */
private const SHARED = [
// The catalogue. A branch runs its own programmes at its own prices and
// sells its own products; a programme offered at one branch showing up
// in another's dropdown is the same bug as a group doing it.
\App\Domain\Training\Models\TrainingProgram::class => 'training_programs',
\App\Domain\Pricing\Models\BasePrice::class => 'base_prices',
\App\Domain\Pricing\Models\PricingRule::class => 'pricing_rules',
\App\Domain\Pricing\Models\Promotion::class => 'promotions',
\App\Domain\Inventory\Models\Product::class => 'products',
\App\Domain\Inventory\Models\Warehouse::class => 'warehouses',
\App\Domain\Inventory\Models\Kit::class => 'kits',
\App\Domain\Inventory\Models\ProductCategory::class => 'product_categories',
\App\Domain\POS\Models\ReceiptTemplate::class => 'receipt_templates',
\App\Domain\Financial\Models\Wallet::class => 'wallets',
];
/**
* Models where branch_id null still means "every branch uses this row".
*
* Only people and academy-wide calendar entries remain. A person is the one
* thing that legitimately belongs to two branches at once — a coach working
* two pitches needs one record visible from both, not two that drift — and a
* public holiday is not a branch's property either.
*/
private const SHARED = [
\App\Domain\HR\Models\Employee::class => 'employees',
\App\Domain\HR\Models\Trainer::class => 'trainers',
\App\Domain\POS\Models\ReceiptTemplate::class => 'receipt_templates',
\App\Domain\Identity\Models\Guardian::class => 'guardians',
\App\Domain\Training\Models\Holiday::class => 'holidays',
];
protected function setUp(): void
......
<?php
namespace Tests\Feature;
use App\Domain\Pricing\Services\PricingService;
use App\Domain\Shared\Exceptions\DomainException;
use App\Domain\Shared\Context\BranchScopeState;
use App\Domain\Shared\Models\Academy;
use App\Domain\Training\Models\Enrollment;
use Illuminate\Support\Facades\DB;
use Tests\TestCase;
/**
* The catalogue is strictly branch-scoped, and a missing base price is a hard
* failure that stops a sale. So the one thing worth pinning is that every live
* enrolment can still be priced with the scope switched on.
*
* DB_CONNECTION=pgsql DB_DATABASE=oc_sport_test ./vendor/bin/phpunit --filter PricingSurvivesBranchScopeTest
*/
class PricingSurvivesBranchScopeTest extends TestCase
{
private ?BranchScopeState $state = null;
protected function setUp(): void
{
parent::setUp();
if (config('database.default') !== 'pgsql') {
$this->markTestSkipped('Needs a restored Postgres tenant.');
}
if (! $academy = Academy::first()) {
$this->markTestSkipped('No academy in the restored tenant.');
}
app()->instance('current_academy', $academy);
$this->state = app(BranchScopeState::class);
}
protected function tearDown(): void
{
$this->state?->deactivate();
parent::tearDown();
}
public function test_every_active_enrolment_still_prices_inside_its_own_branch(): void
{
$pricing = app(PricingService::class);
// Which branches actually have live enrolments to price.
$branches = DB::table('enrollments')
->join('training_groups', 'training_groups.id', '=', 'enrollments.training_group_id')
->where('enrollments.status', 'active')
->whereNotNull('training_groups.branch_id')
->distinct()
->pluck('training_groups.branch_id');
$failures = [];
$priced = 0;
foreach ($branches as $branchId) {
$this->state->activate((int) $branchId);
$enrolments = Enrollment::with('group.program', 'participant')
->where('status', 'active')
->get();
foreach ($enrolments as $enrolment) {
$programme = $enrolment->group?->program;
if (! $programme) {
// The group or its programme is not visible from this
// branch — that is a data problem this test should surface,
// not swallow.
$failures["enrolment #{$enrolment->id}"] = 'group or programme not visible in branch '.$branchId;
continue;
}
try {
$pricing->calculate($programme, $enrolment->participant, (int) $branchId);
$priced++;
} catch (DomainException $e) {
$key = "programme #{$programme->id} in branch {$branchId}";
$failures[$key] = $e->getMessage();
}
}
}
// A programme that had no price before this change still has none — that
// is pre-existing and not what this test is guarding. What it guards is
// that scoping did not take a price away, so compare against the same
// question asked with the scope off.
$this->state->deactivate();
$preExisting = [];
foreach (array_keys($failures) as $key) {
if (! preg_match('/programme #(\d+) in branch (\d+)/', $key, $m)) {
continue;
}
$hasAnyPrice = DB::table('base_prices')
->where('priceable_type', 'like', '%TrainingProgram')
->where('priceable_id', $m[1])
->where('is_active', true)
->exists();
if (! $hasAnyPrice) {
$preExisting[] = $key;
}
}
$caused = array_diff_key($failures, array_flip($preExisting));
$this->assertSame(
[],
$caused,
"Branch scoping took a price away from a live enrolment:\n"
.json_encode($caused, JSON_PRETTY_PRINT | JSON_UNESCAPED_UNICODE)
);
$this->assertGreaterThan(0, $priced, 'Nothing priced at all — the test proved nothing.');
if ($preExisting !== []) {
fwrite(STDERR, "\nPre-existing (no active base price at all, unrelated to branch scoping):\n "
.implode("\n ", $preExisting)."\n");
}
}
}
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