- 07 Sep, 2026 5 commits
-
-
Mahmoud Aglan authored
EnrollmentCancelled takes (enrolment, reason, actor). The service dispatched (enrolment, actor), so PHP put the User where the string reason was expected and found no third argument. dispatch() constructs the event before the dispatcher ever sees it, so this threw an ArgumentCountError on the line itself — inside the service's own DB::transaction, which then rolled the whole cancellation back. Every cancellation on every tenant failed this way. The row stayed active, the group count stayed high, and the nightly renewal run kept billing an enrolment the desk believed it had ended. The only symptom was an error page, and being ShouldDispatchAfterCommit made no difference — the object is built at dispatch, not at commit. Found while removing three test participants from a live tenant: the cleanup could not cancel their enrolments. Pinned by a test that cancels a real enrolment against a restored tenant and asserts it reaches 'cancelled' — which is the whole proof, since a wrong argument list cannot get that far. Event::fake is deliberately not used: it would assert nothing here, because the event is correctly withheld until a commit the test rolls back. A second test pins the event's parameter names so the contract is caught even with no tenant to run against. Co-Authored-By:Claude Opus 5 (1M context) <noreply@anthropic.com>
-
Mahmoud Aglan authored
Three things a football academy keeps about a player that had nowhere to live, found while mapping a 495-row roster exported from a club's previous system. jersey_name is not the player's name. "Mohamed Sherif Mikkawi" wears "Mikkawi", and across 495 players not one shirt name equalled the full name — so it cannot be derived and has to be stored. product_customizations can already ask for it at the till, but that answer belongs to one sale and freezes onto that invoice; the club still needs to know what this player's shirt says, before and after any particular kit is bought. school_name is asked at registration everywhere and is what an academy plans term times and school fixtures around. previous_clubs is free text on purpose. A transfer history is a sentence — "Wadi Degla then Zed" — not a foreign key, and a clubs table would force the desk to resolve every spelling before it could record anything at all. The same roster spells one club four ways. Full slice: migration, model, service, form, show view. All three nullable and additive, each guarded by hasColumn, so the migration is safe against every populated tenant and nothing reads them until something writes them. Co-Authored-By: -
Mahmoud Aglan authored
facility_activities.academy_id is NOT NULL and the pivot has no model to stamp it, so $facility->activities()->sync([...]) threw a not-null violation. Both write paths use exactly that call — FacilityService::create() and FacilityForm::save() — so ticking a sport on a facility has never once worked on any tenant. It surfaces only on Postgres, and the default sqlite suite has no such table, which is why nothing caught it. The consequence was quiet rather than loud: 2026_08_30_000003 treats a facility with no rows here as one that hosts anything, so instead of an error the schedule builder just kept offering every group in the academy for every pitch — the thing that migration was written to stop. A football pitch still lists swimming groups. Fixed on the relation with withPivotValue(), which both fills the column on write and scopes it on read, so the edit path is covered too without either caller having to remember. Also null-coalesces the two optional keys in validateNoOverlap(). Neither is passed by createDefaultLayout(), so every facility ever created raised "Undefined array key" — a warning in the app, a hard error under PHPUnit. The deeper issue is left alone on purpose and noted in place: a null day compiles to `effective_day_of_week = NULL`, which SQL never satisfies, so an all-days layout is currently exempt from overlap detection. Every tenant already has an auto-created all-days default layout, so making it collide would start refusing temporal layouts that can be added today — a behaviour change that wants its own decision, not a drive-by. Co-Authored-By: -
Mahmoud Aglan authored
A compound charges a resident 2,200 and a non-resident 2,800, and the second child of a non-resident 2,400 — while a resident's second child stays at 2,200, because the resident price is already the discounted one. A sibling_order rule could say "second child" but not "and only for non-residents", and the arithmetic has no shortcut: a flat 400 off takes the resident to 1,800, and a percentage landing on 2,400 from 2,800 lands on 1,886 from 2,200. The config-only alternative was a second stackable rule holding fixed_price 220000 to push residents back up. That works arithmetically and is the wrong answer: it stores the tier's base price in a second table. The day fees rise and only base_prices is edited — the obvious place — that rule silently forces every resident back to the old number, with no error and no missing-price failure to notice. It also prints a +200 EGP "discount" line on a resident sibling's invoice, because appliedRules is stored verbatim on it. So sibling_order and family_size gain an optional membership_type list in their conditions, evaluated as an AND alongside the existing range. conditions is jsonb with no CHECK constraint, so no migration is needed. The key is absent from every rule authored before now, isset() is false, and evaluation is bit-identical for every existing rule on every tenant — additive in the same sense a nullable column is. It is accepted only where the schema declares it; normalize() strips it from an age or loyalty rule rather than storing a condition the engine will never consult. describe() appends the tier to the existing clause so the picker's rejection reason stays a sentence. Pinned by 13 tests, the first of which is the one that matters to the other tenants: absent key, unchanged behaviour. Co-Authored-By: -
Mahmoud Aglan authored
fix(training): give generated sessions a branch, and reserve the pitch they were actually scheduled on Three defects on one path, all silent, all found while auditing a tenant whose sessions were invisible on every branch screen. BranchContext::branchIdForStamping() returns null in console and queue context — deliberately, because a cron that silently billed one branch would be worse than an unfiltered read. But that is exactly where sessions are born: the 02:00 sessions:generate-upcoming run, and the TrainingSchedule::saved() hook. So SessionGeneratorService and AttendanceGenerationService, which both relied on the BelongsToBranch creating hook, have been writing branch_id NULL since they were written. On a strictly-scoped table that is not a leak but a disappearance: the row is in the database, counted by SQL, and on no screen in the product. It is also why attendance:backfill exists at 02:30 — it repairs the attendance half nightly, with the same session -> group attribution now applied at the source. CreateAutoReservation resolved the schedule by (training_group_id, day_of_week) and took ->first(): no start_time match, no is_active filter, no ordering. A group that trains twice on one weekday, or that has a deactivated row sitting beside a live one, booked whichever row sorted first — the wrong hour, or another branch's pitch entirely. The session already records which schedule row produced it, so use it; the (group, weekday) lookup survives only as a fallback for a session with no schedule_id, and now matches on time and is_active. The listener swallows its own exceptions into a log line, so none of this ever surfaced as an error. Co-Authored-By:
-
- 06 Sep, 2026 2 commits
-
-
DevPilot authored
A cashier could hold only one open drawer across the whole academy, and the three screens that ask about it did not agree on which one. Opening a shift checked every branch (withoutBranchScope), while the POS terminal and the manage screen looked only at the branch being worked in. So a drawer left open at one branch locked every other branch out of selling: the terminal said "open a shift first", the open form answered "there is already one open", and the manage screen offered nothing to close, because the shift it was refusing over was in a branch that screen will not show. On OC-Sport three accounts had drawers open at ZSC since July, which is every account the other seven branches sell through. A drawer is a physical box standing in one branch — its float, its cash in and its variance at close all belong to that branch's reconciliation, and POSService already refuses to ring a sale against another branch's session. So the invariant it can actually carry is one open drawer per cashier per branch, which is also what the desks need: each branch opens its own shift and collects normally, and closing one is never a precondition for another. Also fixed, because more than one session per user can now be open at once: - getOpenSession() resolves a named branch through forBranch() rather than filtering on top of the request scope. RefundService asks it for the drawer of the branch whose money is going back out; from any other branch that returned null and the refund silently skipped the cash count. - UpdateCashSessionTotals prefers the drawer the payment names, and narrows its fallback to the payment's own branch. It runs on a queue where the branch scope is off, so an unqualified first() would have counted one branch's cash into another branch's box. - Both screens now name the branches where the cashier still has a drawer open, so "there is already an open shift" is something the desk can act on. Co-Authored-By: -
DevPilot authored
The transfer wizard died on the return leg of every shuffle. transfer() always INSERTs the replacement enrolment, but nothing ever deletes the row a player leaves — it stays, cancelled — and `enrollments` is unique on (participant_id, training_group_id). So moving a child from 2015 A to 2015 B worked, and moving them back a month later raised a raw UniqueConstraintViolationException that the wizard's catch-all reported to the desk as "خطأ غير متوقع", with nothing to say what had gone wrong. On the OC-Sport tenant 94 active players already have a former group that is still open and still listed as a destination, so this was one click away on any of them. Revive the existing row rather than loosening the constraint: one player is in one group once, which is what the unique key says and what the history should read like. The lookup drops global scopes deliberately — the constraint is academy-wide and knows nothing about the branch scope, so a row this request could not see would still collide — and clears the withdrawal that ended the previous spell, or the revived enrolment would read as active and withdrawn at once and the renewal command would skip it. Two neighbours fixed while here, both latent rather than reported: - transfer() had no "one group per programme" guard, though enroll() has one. A transfer into a programme the player was already enrolled in left two active enrolments, which GenerateRenewalInvoices bills twice. - transfer() left academy_id to BelongsToAcademy, which fills it from a container binding that only exists in a web request. Same NOT NULL failure enroll() was already fixed for; states it explicitly now. Transferring into the group the player is already in is now refused instead of cancelling and reviving the same row and reporting success. Verified against a restored oc_sport tenant (the suite's own convention): the new test reproduces the violation without the fix and passes with it, and the full suite shows the same 6 pre-existing failures before and after — no regressions. Co-Authored-By:
-
- 04 Sep, 2026 1 commit
-
-
Mahmoud Aglan authored
Where a collection went in the ledger was a `match` statement: five itemable classes hardcoded to five revenue codes, identical for every client, and no way to express tax at all. An accountant who said "no, five pounds of that is a registration fee and the rest is training, and the kit carries 14% VAT" had no answer but a deploy. Revenue routing makes that answer data. Every kind of money the ERP can take is a source with one rule: a tax treatment, and destination lines that take a flat amount, a percentage, or the remainder. A rule can be narrowed to a single product, programme or event, which beats the academy default for that item alone. financial.revenue-routing edits them, with a preview that runs the unsaved rule through the real allocation engine rather than a second implementation that agrees with it until it doesn't. Three things it will not do: - Book tax as revenue. VAT collected is owed to the Authority, so it comes out first and credits a liability; the service refuses a tax account that is not one. Egypt's 14% is a per-source setting because some services are 10% and some are exempt. - Write an entry that does not balance. Integer piasters throughout, floors everywhere, one line closes the rounding, and assertBalanced() throws before a single row is written — a transaction is immutable, so a wrong one can only be reversed, never corrected. - Change anything on the day it ships. The migration seeds each academy the rule that reproduces its current behaviour exactly, so nothing moves until someone asks it to. The platform fee ships switched off for the same reason. Refunds now reverse the collection's own ledger rows in proportion rather than re-running today's rules — a rule edited last week would otherwise unwind money into accounts it never touched. LedgerAccountResolver::splitRevenue() and revenueCodeForItemable() are deleted rather than deprecated: a second implementation of "where does this money go" is one a caller would eventually reach for, and it would bypass every rule the academy wrote. Verified on a restored oc_sport tenant copy and on a database built from zero: 521 tests, 0 failures on both the Postgres and SQLite suites. PaymentLedgerTest builds a schema without the routing tables, which pins the other half of the safety property — an installation that has not received the migration keeps taking money on the built-in behaviour instead of failing at the till. Co-Authored-By:
-
- 03 Sep, 2026 16 commits
-
-
Mahmoud Aglan authored
The console could tell an operator that a player had no enrolment and nothing in the system was billing him. It could not do anything about it: all ten settlement actions move money on an account that already exists, so the one account that had stopped existing was the one the wizard could not fix. The operator read the diagnosis and had no button. `restore_enrollment` is that button. It delegates to EnrollmentService rather than inserting a row, so a restored enrolment passes the same checks a new one does — capacity, group status, one-group-per-programme, the branch rule — and lands identical to every other row in the table. A repair that produces a slightly different shape of row is a second bug waiting. It raises no invoice. `next_billing_date` comes out as the 1st of next month, so the cycle resumes on its own and the current month stays a deliberate act: the operator adds `bill_month` for it if it is owed, with the amount in front of them. Restoring a subscription and charging for it are two decisions and the second is not ours to assume. The group is suggested, not chosen. The enrolment that knew the programme is gone; the only surviving record is the name on the player's last subscription invoice, and that programme was deleted and recreated under a new one — so it cannot be looked up, only matched. Matching is on the years in the name, because these programmes are birth-year cohorts: «اكاديمية 2017 -2018» and «أكاديمية (2017-2018)» share nothing as strings and are obviously one cohort to a human. Cohorts written in two digits match nothing and a retired squad has no successor, which is exactly where the person at the desk knows and this screen does not — so the suggestion is labelled as a guess and the full list is always there. Two bugs found while proving it works, both real rather than test-only: - EnrollmentService left `academy_id` to BelongsToAcademy, which fills it from the `current_academy` container binding — bound only inside a web request. An enrolment created from a command, a queued job or a service died on a NOT NULL violation. It is now stated from the participant, which is true under every caller and which the trait leaves alone. - The group list sorted with `sortBy([fn, fn])`. sortBy reads an array as [column, direction] pairs, so it sorted by neither and buried the suggestion mid-list, where an operator in a hurry never sees it. Verified end to end against the restored tenant: stage the restore, apply it, and the player is enrolled in the suggested group with next month's billing date and not one new invoice. 498 tests, 389 pass / 109 skip on Postgres and 313 / 185 on SQLite. Co-Authored-By: -
Mahmoud Aglan authored
The screen listed what was wrong with an account and stopped there. The person reading it is a receptionist mid-shift, not an accountant with time to work out what "مستلزم محاسَب بأكثر من سعره" means or which of nine tools fixes it — and a case nobody understands is a case nobody touches, which is money. So the catalogue now carries the explanation with the label: what the case means, why it happens, the numbered handling, and which tool does it. Selecting a case in the rail shows all of that beside the accounts it filters to. Every row opens its own evidence in place — the months nobody billed, the free-text lines that are really product sales, where the player stands on his kit — so the operator reads the account before deciding, without opening the wizard to find out whether it is worth opening. New case, highest severity: **لا يوجد له اشتراك مسجَّل**. An active paying player with invoice history and no enrolment row at all. Renewal billing reads `enrollments` and nothing else, so these players are not billed late — they are not billed at all, and no other screen in the system lists them. That is what made OC-Sport's 95 lost accounts invisible for a month after a programme delete took their enrolments. The row reads the programme off their last invoice line, because the enrolment that knew it is gone and that name is what an operator needs to put them back. Two things the rebuild fixes on the way past: - The scan no longer passes `only` to the scanner. Filtering there dropped the other cases from the result, so the rail beside a selected case read zero everywhere — which an operator reads as "those are fixed", not "those are hidden". - `expanded` is `#[Locked]`. It names a row whose detail is that participant's own money; settable from the browser it is an id to aim somewhere else. Verified against the restored tenant: 496 tests, 387 pass / 109 skip on Postgres and 313 / 183 on SQLite, and every one of the first 40 flagged accounts renders its detail block. Rendered and inspected at 390px, 820px and 1360px. Co-Authored-By: -
Mahmoud Aglan authored
Deleting a programme or a group called `$group->enrollments()->forceDelete()` on both paths — hard delete, no soft delete, no audit row, nothing to restore from. `enrollments` is the only table GenerateRenewalInvoices bills from, so the players were not merely losing history: they stopped existing as far as billing was concerned while still training, still members, still owing money. OC-Sport reorganised its season on 28 August 2026 by deleting the programmes and recreating them under new names. 181 enrolments went with them. On 2 September the renewal run raised 229 invoices, reported a clean success, and 95 paying players were not among them. Nobody found out until a parent asked why no bill had come for his son. Two halves, because neither alone is enough: - A destructive delete is refused while anything is enrolled — active or cancelled, since a cancelled enrolment is still the only record of what an issued invoice bought. Archive the programme, or transfer the players and close the group. The count drops the branch scope: an enrolment hidden by the active branch is still an enrolment, and reading zero because of it is how a guard like this fails open. - The renewal command now names every paying player who has invoice history and no enrolment at all, and logs them. No guard recovers the rows already lost, or catches the next way somebody finds to lose them. An empty enrolment set with such players left over is a FAILURE exit — "nothing to bill" and "every enrolment was destroyed" produce the same empty set, and the players left over are the only thing that tells them apart. Group deletion moves into TrainingGroupService so the guard sits on every path to it; GroupList duplicating the cleanup inline is how one path ended up without it. Co-Authored-By: -
Mahmoud Aglan authored
Two things the desk could not previously be made to capture. A product now carries the details it cannot be sold without — a size, a colour, the name printed on the back. The admin declares them per product as either free text or a list of predefined answers; the registration wizard renders them inline on the cart row and refuses to move to payment while a required one is blank. None of this is a separate product, and none of it is reconstructable after the fact from an invoice line that says "قميص تدريب" — which is why the answers freeze onto invoice_items.metadata and into the line description at sale time. Renaming "لارج" to "L" next season must not rewrite what a player ordered last season, exactly as prices freeze. hotbuyCustomizations is browser-writable by necessity — it is what the receptionist is typing — so nothing downstream trusts it. Both the step guard and confirm() re-read the questions through Eloquent (Product carries BranchScope, so another branch's product resolves to nothing) and check every answer against ProductCustomization::accepts(). A select must match its own list: the dropdown is a convenience, not the check. Separately, a person entered into the system must now carry a four-part name. Egyptian records are keyed on it — national ID, birth certificate, federation card, school file — and a player registered as "محمد أحمد" matches none of them; two players sharing a first and father's name are common enough that parts three and four are what tell them apart. Applied where people are created at the desk and in the portal: the registration wizard (player and guardian), retroactive enrolment, the participant form, and portal sign-up. Four is a floor, not a target. FullName splits on Unicode whitespace explicitly. \s under /u still only means ASCII whitespace, and an Arabic keyboard produces U+00A0 — glued together, a perfectly valid four-part name would have been rejected. Deliberately left alone: - ParticipantImport is exempt. Enforcing the rule on a bulk import would block loading an existing roster, which is the one case where the short names are already a fact. - Records already in the database are untouched. This validates at entry. - The POS terminal, settlement wizard, group screen and essential- deliveries screen sell these same products and do not yet ask for the customizations. - Kits carry no customizations, only products. Migration is additive and guarded; CHECK constraint matches the enum character for character. 382 passed / 99 skipped on the restored Postgres tenant, 303 passed / 178 skipped on SQLite. Co-Authored-By: -
Mahmoud Aglan authored
Every dues report was per invoice, so answering 'how much is on this boy' meant adding rows up by eye, and there was no way to ask it of a cohort at all — no report anywhere could filter on a date of birth. Two new ones: participant_dues totals each player, participant_invoice_history is the same money invoice by invoice. Both take a birth-year box, because that is how a club talks about its players: مواليد 2015 is a cohort, not a search. What is owed sums each invoice's stored due_amount floored at zero rather than subtracting paid from billed. An overpaid invoice carries a negative due_amount, and summing the subtraction would let it cancel another invoice's real debt — the report would quietly forgive money. The birth-year filter reaches invoices → participants → people through whereHas rather than a join, so Participant's BranchScope survives the subquery and another branch's player cannot be reached by typing their membership number into the search box. Filters are declared in the report config instead of special-cased, and the CSV export builds its arguments from the same list. That closes an existing divergence: the minimum-absences filter was wired into the page and not the export, so filtering the absentees report and downloading it gave you the unfiltered one. Co-Authored-By: -
Mahmoud Aglan authored
feat(pricing): scope a discount to members or to non-members, and let a branch-targeted one save at all Membership tier decided only which base price was read; it was invisible to every rule. A club wanting '10% off, members only' had no way to say it. The nearest rule type, membership_duration, is tenure in months, which is a different question and happily matches a non-member who has been around a while. New membership_type rule type, with the CHECK constraint widened to admit it — no existing row changes value, and the new one is unreachable until a rule is authored with it. The tier the rule matches is the same one that chose the base price, so a discount and the price it discounts cannot disagree about who this is. An unset tier reads as non_member, matching step 1, so a 'members only' rule cannot quietly reach someone nobody ever classified. And the bug the test for it found: NO branch-targeted discount could be saved. pricing_rule_branches.academy_id is NOT NULL and the pivot has no model, so no BelongsToAcademy hook filled it and a bare sync() died on a not-null violation — every rule authored in the wizard with a branch ticked. Stamped by hand, the way ProgramForm already does for program_products. Co-Authored-By: -
Mahmoud Aglan authored
Proration counted remaining days on a hardcoded 30-day month, a unit the academy never sold. A programme meeting Sunday and Tuesday holds no sessions over a long weekend, so someone joining on the 22nd was billed a third of a month for perhaps two trainings — and the calendar never noticed. September 2026, Sun+Tue: three of the month's nine sessions remain on the 22nd, not nine of thirty days. SessionCountService counts from the timetable rather than from training_sessions, because the generator only materialises rows about a week ahead and counting rows would under-report the rest of the month — exactly the question proration asks. Its rules are the generator's, deliberately identical: an active schedule row naming the weekday, effective that day, with no training-affecting holiday on it. The desk now chooses per registration: شهر كامل, نص شهر, or باقي تمرينات الشهر. The mode is settable from the browser by design, and safe to be — the academy setting remains the gate, and an unrecognised value falls back to the default rather than being honoured. Joining after the month's last session owes nothing for that month, so no invoice is raised at all; a zero-total one is what AccountAnomalyScanner reports as corruption. Also: a branch whose takings never pass through the system. A partner-run site bills nobody — participants enrol unbilled, enrolments are marked waived, and the branch's income is entered afterwards on the external-revenue screen. Skipping the invoice rather than writing a zero one, for the same reason as above. The guard sits before the renewal command's adoption step, not after: those enrolments carry no billing date precisely because they are off the cycle, and adoption would read that as an oversight and put every one of them onto it. Fixes a latent crash on the way: BranchSettingsService called app('current_academy') unguarded, which throws rather than returning null outside a request. The renewal command binds no academy, so asking it the billing question from the console would have taken the nightly run down. Co-Authored-By: -
Mahmoud Aglan authored
Four things the programme form could not say, all of which sent the desk somewhere else or nowhere at all. A timetable. Recording that a group trains Sunday and Tuesday from four to half five required the visual grid, which demands a facility and a space — so clubs that place nobody on a grid had no timetable at all, and the proration and attendance engines had nothing to read. The form now writes plain TrainingSchedule rows on the programme's default group with facility_id null. Rows the grid HAS placed are left alone: clearing a checkbox here must not strip a space reservation off a scheduled session. A day dropped from the selection is deactivated rather than deleted, because generated sessions hold a hard FK to the row and attendance hangs off those sessions. Renewal defaults. A new programme opened as manual_renew, so every one had to be corrected by hand or quietly stopped billing. It now opens auto_renew, monthly, on the first. Kits. program_products already records 'this player must buy that thing', but only for a single product; a kit sold as one thing could not be required. program_kits is a separate pivot rather than a nullable kit_id on program_products, whose product_id is NOT NULL and whose uniqueness is (academy, programme, product). Nor is a kit expanded into its components: the POS writes an invoice line carrying itemable_type = Kit, so a programme requiring the parts would report every buyer as missing all of them. The group roster flags a missing kit the way it flags a missing product. Prices. The programmes list showed no price, so comparing what two programmes cost meant opening both. Both tiers now show, in one query for the page, and a tier with no active base price reads 'غير محدد' rather than 0 — the engine hard-fails there, so a zero would be a figure nobody will ever be charged. Co-Authored-By: -
Mahmoud Aglan authored
The product calls a person who belongs to the organisation عضو and everyone else غير عضو. That is only true for a club. The same software runs inside residential compounds (مقيم / غير مقيم), on beaches and in resorts (مشترك / زائر), and in hotels (نزيل / زائر), where every screen read as though it had been written for somebody else. The data model does not move: participants.membership_type still holds 'member' and 'non_member', every query and every enum is untouched, and nothing about pricing or membership logic changes. Only the words shown to a human do, and they are chosen per branch — one academy can run a club and a compound at the same time. Eight Arabic forms are stored per branch rather than derived, because Arabic will not let you derive them: ال prefixes the noun in العضو but the second word in غير العضو, and the plural of عضو is أعضاء while the plural of مقيم is مقيمين. Everything a screen needs beyond those eight — نوع العضوية, رقم العضوية, سعر العضو — composes from them in Identity\Support\Terminology. Plurals are stored in the ـين form because almost every site prints them after a preposition or in an idafa. TerminologyService takes an explicit ?int $branchId like every other domain service, so it stays callable from a queued notification or an artisan report; terms()/term()/membership_label() in app/Helpers are the adapter that reads BranchContext for a Blade file. Reads go through the query builder rather than the model, because the sidebar consults this on every page and BranchSetting's branch scope would filter a settings screen editing branch B, viewed from branch A, down to nothing. The service is bound scoped, so its memo lives exactly one request. Presets (club, compound, resort, hotel, gym) fill the settings screen and stay editable afterwards; the fields are stored in the existing branch_settings table, so there is no schema change. An unconfigured branch — and the public website, and a user in all-branches mode — gets the default preset rather than blank labels. term() throws on an unknown key rather than echoing it back, because a typo that printed "membershipp_type" onto a receipt would survive review. 62 call sites converted across the sidebar, participant screens, the registration wizard, the portal, reports, messaging, imports and the printed card. New screen at /settings/terminology behind permission:settings.manage. Verified: 413 tests green, no failures. Co-Authored-By: -
Mahmoud Aglan authored
This is the actual reason no receipt could be attached — 1b62a44f-… and every other support code from the expense pages: UnableToRetrieveMetadata: Unable to retrieve the file_size for file at location: livewire-tmp/adbO73…jpg ExpenseShow.php(93): TemporaryUploadedFile->getSize() store() moves the file out of livewire-tmp. The attachment array put 'path' => $file->store(…) on its first line, and PHP evaluates array literals in order, so getMimeType() and getSize() on the lines below ran against a path that had just stopped existing. Validation passed, the vanished-upload guard passed, the file was even written to its final home — and then the request died with a 500 on the way to the row. Nothing was ever saved. Both handlers now read name, mime and size into locals first and store last. Reproduced against a real local disk before and after; Livewire's test harness swaps in a temp disk that does not move the file, which is why a component test would have gone green on the broken code, so the regression guard asserts the source ordering instead. Co-Authored-By: -
Mahmoud Aglan authored
Three things stood between the desk and an attached receipt: - The picker on the create form listed extensions (.jpg,.png,…), which on iOS greys out the camera roll; and both components validated mimes:jpg,jpeg,png,pdf,webp, so a HEIC photo — the iPhone default — was refused after the picker had accepted it. Both now accept image/* plus heic/heif, and validate the same set. - The attachment stream built Content-Disposition by hand with addslashes(). Receipt names here are Arabic, and an Arabic filename is not a legal header value: the inline preview came back broken and the download came back mangled. Symfony builds the header now, via Storage::response() for inline and download() for the save. - HEIC is an image no browser but Safari paints, so it falls back to the file card instead of a broken <img> (attachmentIsViewableImage()). Also: the create form advertised a 5MB ceiling while the real one is config('uploads.max_kb') (150MB), and its uploaded-file chip called getClientOriginalName() on whatever sat in the property. Co-Authored-By: -
Mahmoud Aglan authored
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: -
Mahmoud Aglan authored
The last commit taught the settlement screen to SEE money billed twice. It still could not do anything about it: the panel described the problem and then offered "تسجيل باقي القيمة", which bills MORE. The only way to actually fix the reported account was an admin editing the invoice by hand, which is how it broke in the first place. So the screen gets the missing action. `void_duplicate` takes a demand off the books — reduce an invoice, or cancel it outright — and it is deliberately not a waiver. A waiver forgives a debt that was real and is revenue given away; this removes a demand that should never have existed, so the club is not out of pocket by a piaster and it reports as negative billing. The cart's third tile flips to "سيُحذف من المطلوب" rather than showing a minus sign under "سيُفوتَر". Detecting is only half a tool. An operator standing in front of a parent needs to know WHICH of six invoices carries the duplicate, and the second spent working that out is the second the wrong one gets cancelled. So the wizard matches the excess back to the invoices that could be carrying it, newest first, and pre-fills the amount: on the reported account it offers "INV-000593: 5,500 -> 3,000" as one button. The worklist states the figure on the row, so the excess is visible before anything is opened, and severity now colours the chips — a double-billing and a missing month read identically before, which is why the list got worked top-to-bottom. The guard is the whole value of the thing: an invoice with money collected against it is never touched. Reducing one below what was paid strands real money — the payment row saying one thing, its immutable ledger entry another, which is exactly the damage the old correction wizard used to do. That money has somewhere to go (move_payment, or credit_wallet) and which is right is a person's decision, so the action refuses and the panel says so instead of offering a button. Same refusal for an invoice belonging to another participant, for raising an amount through a tool named for lowering it, and for an invoice with more than one line, where reducing the header cannot say which of two lines was the duplicate. Verified end to end against the restored tenant, which still carries the account in its broken state: scanner flags one, screen proposes INV-000593 5,500 -> 3,000, applying it leaves billed 8,000 of an 8,000 card with 3,000 still owed, and the re-scan comes back clean — the same correction that had to be made by hand on production, now one click. 364 tests green on SQLite and Postgres. Co-Authored-By: -
Mahmoud Aglan authored
Participant 128 on OC-Sport: an 8,000 registration card, 2,500 paid in July and 2,500 in August. The settlement screen told the operator he had paid 5,000 of 10,500 and still owed 5,500, and offered a button to bill him that 5,500 on top. The 10,500 is real, and that is the problem. INV-000310 carried the first instalment on 2 July. On 5 August the full 8,000 card was invoiced again as INV-000588, 2,500 was collected against it, and the next day it was split — reduced to 2,500 with a 5,500 remainder as INV-000593 — by someone who never saw the July instalment. Three lines, 10,500, for a card that costs 8,000. bundleStatus() then read `max(billed, price)`. That reading treats hand-typed lines as if they defined the obligation, so every duplicate and every correction raised the debt, and the excess disappeared into a larger number instead of being noticed. But a hand-typed line is an instalment TOWARD a card whose price the product record still holds: the card is what is owed. A real product sale is different — its price froze at the till — so that keeps billing as the obligation. So: expected is the frozen sale price for a real sale, and the card's price otherwise. Anything typed past it is the same money entered twice and is reported as `over_billed_bundle` rather than absorbed. #128 now reads 5,000 of 8,000 with 3,000 left, flagged for a 2,500 double entry. A scan of the restored tenant finds exactly one such account: his. Two guards on the tool that produced it. The correction wizard clamped paid_amount down to the new total and rewrote the payment rows themselves, so cutting an invoice below what had been collected against it destroyed real money — the payment row said one thing and its double-entry transaction still said another, and nobody was told. It now refuses and names the settlement wizard, which can move the payment or credit it to a wallet. And the split step lists what the account already carries, so a second "first instalment" is visible before it is created rather than three weeks after. Co-Authored-By: -
Mahmoud Aglan authored
A product carries a member rate, a non-member rate, and instalment plans open to one tier or the other. Only the registration wizard ever read any of it. The till resolved prices through the pricing engine, the engine knew only about base_prices, and nothing writes a product's member rate there — on OC-Sport there is not one base_prices row for any product — so every calculate() threw, the terminal fell back to selling_price, and every member buying at reception paid the walk-in price with nothing on the receipt to say so. The 8,000 card costs members 6,000; they were charged 8,000. The plans were invisible too: the only partial payment the terminal offered was a free-typed عربون, so a receptionist taking the first instalment of an agreed schedule typed it into a manual line, and the sale left no plan behind for anything to track. The precedence now lives in the engine, once, so every caller gets it: a base price tagged with this membership type wins, then the product's own column for this tier, then any other base price. selling_price stays out of it — it is the catalogue's advertised number, not a configured price, and admitting it would make "nobody set a price" undetectable, which is the hard fail the pricing rules require. The terminal falls back to priceForTier() only when the engine has nothing at all, and says on screen which price list is in force. Plans reach the terminal as a per-line picker: pick a schedule, choose how many instalments are being paid today, see the rest. Everything is re-resolved in POSService from the database — the cart is a public property, so a plan id in it is a number the browser chose, and a plan belonging to another product or to the other tier is refused rather than ignored. participantId is #[Locked] for the same reason: it now decides which price list the sale is quoted from. buildSchedule() rounded every auto slot up, so three instalments of an 8,000.00 card came to 8,000.01 — a plan asking for a piaster the invoice never billed, which could never reach `completed`. Piasters split the way they do everywhere else here: floor each share, last slot takes the remainder. Verified against the restored OC-Sport tenant: member #18 prices at 6,000.00 on the 2,000x3 members' plan, non-member #257 at 8,000.00 on the 2,500/2,500/3,000 plan; both schedules sum exactly. Full suite green on SQLite and on Postgres, POS terminal renders. Co-Authored-By: -
Mahmoud Aglan authored
portal.css is built with `source(none)`, so a template not named in an @source line contributes nothing. layouts/portal.blade.php was not named. Every utility the shell alone used — h-8 w-8 on the logo, max-w-2xl on the column, pb-28 above the tab bar — was absent from the bundle, so a tenant logo rendered at its natural size, the page went wider than the phone, and the first tab sat off-screen. The layouts are scanned now, and a test asserts they stay scanned. The rail made it worse: .rail padded itself 1rem and pulled back -1rem, the bleed trick for a rail inside a padded column. Its one caller sits in an unpadded header and supplies its own padding, so the negative margins had nothing to cancel and made the element 2rem wider than the viewport. Colour. OC Sport themed their website navy and gold and never opened the branding screen, so the portal, the PWA theme colour and the admin were all still on our shipped blue. Branding colours still sitting on the default are now inherited from the academy's own website palette; picking any colour settles it. Editing the site bumps the brand cache, or the rest of the product keeps yesterday's palette indefinitely. That exposed what shade 600 was doing as a link colour: anchored on the tenant's own colour, it lands within 0.02 of a dark navy, so links rendered as body text. ColorRamp now derives an interactive colour placed at a lightness that reads as a colour and clears 4.5:1 on the surface it is drawn on — one for light, one for dark — and an ink() for the semantic palette, which is chosen to be seen as a fill: amber is about 2:1 as 10px text on white, and that is what every error message and status chip was using. Filled buttons get a computed foreground instead of a hardcoded #fff. Screens: the outstanding balance was a third of a three-across statistics row, wrapping onto two lines, and repeated verbatim in the action list above it. It is one fact and it is the fact members open the app for, so it is the headline, figure large and currency small, and the row is two tiles. Icon path data was retyped in three places and the home screen's copies were truncated mid-curve — a member saw a tick where a wallet should be — so there is one icon component. The status chip five screens built by hand is one component too. Co-Authored-By:
-
- 02 Sep, 2026 11 commits
-
-
Mahmoud Aglan authored
A file crossing this system passes four size checks and they disagreed: the component's rules said 5 MB, Livewire's undeclared default said 12, PHP said 20, nginx said 25. Whichever was smallest won, with a message written by whoever owned that layer — and nginx's refusal is a 413 error page, not something a receptionist can act on. config/uploads.php holds the number now, and the four layers are set from it in the right order: nginx (160M) is the most generous so it never refuses first, then post_max_size (160M) above upload_max_filesize (150M) so a file at the limit is rejected as a file rather than as a malformed request, then Livewire's temporary-upload rule, then the component. The size named in each Arabic error message is interpolated from the same config instead of retyped, because the old messages said "5 ميجابايت" while the rule said something else. Time limits went with it: max_input_time is what cuts off a body still arriving, and 150 MB over Egyptian mobile data is minutes, so it and nginx's client_body_timeout go to 300s and Livewire's max_upload_time to 30 minutes. Pictures keep their own small ceilings — a logo is carried on every page load. And the settlement worklist stops treating the running month as a problem. Late now means a month that has ENDED and was not collected — read from the month the invoice names, not from its due date — because a club collects all month and does not consider a player a problem on the 9th. A card being paid on an agreed plan whose next instalment has not come due is not an anomaly either. Together: a player who owes only this month and whose bundled product is bought or paid up to date does not appear at all, which is the whole point of the screen. An unpaid bundled product goes back to standing on its own, since that is exactly what it exists to find. Verified on the restored tenant: 24 settlement cases including the new month rule (last month unpaid flags, this month never does, whatever the due date), full suite 334 tests on both connections. Co-Authored-By: -
Mahmoud Aglan authored
Error eab81218-6482-47eb-9d09-aa54259069aa: an expense receipt, from an iPhone, at 16:22 today. League\Flysystem\UnableToRetrieveMetadata — "Unable to retrieve the file_size for livewire-tmp/Vy05fpe….jpg". Livewire uploads in two steps: the file lands in livewire-tmp on its own request, and the component reads it on a later one. Between those two the file can be gone — every push redeploys the container and livewire-tmp is not persistent, Livewire's own cleanup removes stale files, and a phone happily resends a form after the app has restarted. The first thing to touch the file is validation, because `max:5120` calls getSize(), so the receptionist standing at the desk with a receipt got an error page and a support code instead of a form. The file being gone is not exceptional, it is Tuesday. ChecksTemporaryUploads asks whether the pending upload still exists (treating an unreachable disk as gone rather than letting a storage exception reach the browser), clears the dead handle so the next attempt starts clean, and puts one Arabic sentence on the field: اختر الملف مرة أخرى وأعد الرفع. Applied to every component that reads an upload, not just the one that was reported — expense receipt and expense form, the three portal uploads (payment proof, documents, requests), branding images, the page builder, the gallery, the document wizard, the event wizard's cover and gallery photos, and the participant import. Also pins what the group roster already does with combined invoices, since it was worth proving rather than assuming: participants 219 and 97 each paid part of the federation card on an invoice shared with a kit, typed as free text, and the roster allocates the payment across the lines and shows the card's share against the price for that member's tier — 2,500 of 8,000 and 2,000 of 6,000, both labelled أقساط. Full suite 325 tests on SQLite and on the restored tenant, no failures. Co-Authored-By: -
Mahmoud Aglan authored
Three things the screen got wrong on the first day it was used. Search could not find people. `like '%term%'` over name_ar fails on this data for two reasons that have nothing to do with the searcher being careless: nobody agrees about hamza (عبدالله أحمد / عبدالله احمد is the same child, so is يحيى/يحيي and حمزة/حمزه), and a name on file is four or five words while the person searching types the two they remember — "عبدالله صلاح" against "عبدالله أحمد صلاح سيد" matches nothing because those words are not adjacent. ArabicSearch folds both sides to one spelling and requires each word of the term to appear somewhere in the name, folding in SQL (Postgres translate) so the database does the work. The bundle probe asked whether a product line existed, so a boy who paid 2,500 toward his federation card — typed as "القسط الاول" on the same invoice as his kit — was reported as never having bought one. That is the exact reading the group roster stopped doing last week, and two screens answering the same question differently is worse than either answer. It now reads the money the way BundledProductLine does, bare instalments included where the programme requires exactly one product, and reports a position rather than a yes/no: paid, part paid with the remainder and a progress bar, or nothing at all. And the worklist was flagging ordinary business. A renewal issued on the 1st and due on the 8th is not an anomaly, it is Tuesday — so the flags now fire on invoices past their due date, not merely unpaid. A card being paid off on an agreed plan through the till is not an anomaly either; only money recorded outside the product is. "Requires a card and has not bought one" is a sales fact, not a payment anomaly, so it annotates an account without summoning it. And enrolment start_date is copied from the GROUP's season start, so a player entered in August carried a 16 July start and was reported as owing months of a season he was not in — the month he joined is the later of start_date and enrollment_date. On the restored tenant this takes the worklist from 165 accounts to 77, and unbilled-month flags from 35 to 1. Participant 219 now reads "سدد 2,500 من 8,000" instead of "لم يُحاسَب على مستلزم البرنامج". Verified: 46 settlement/search/render cases pass, full suite 318 tests on both SQLite and the restored tenant, no failures. Co-Authored-By: -
Mahmoud Aglan authored
Reported: the weekly schedule shows a schedule on a branch that was never scheduled. Investigated against a restored tenant and the component is correct — branch 2 gets only branch 2's sessions, and a branch with nothing gets an empty grid. But chasing it found a real hole in the suite. BranchScopedScreensTest searches rendered HTML for another branch's uuids. The weekly schedule grid prints group names and times and no uuid at all, and training_sessions was not even among the tables it collected uuids for. A foreign session sitting in that grid would have left nothing in the markup to search for, and the suite would have stayed green while the screen was wrong. Several other grids and calendars are the same shape. So this asserts one level earlier, on the objects rather than the markup. Every component reachable at a parameterless URL is mounted under every branch, the data handed to its views is captured through a view composer, and every model in it that carries a branch_id must belong to the active branch — or be null only where null still means "every branch" (people and the academy calendar), or belong to a model that declares BRANCH_SCOPE_EXEMPT. It reads that declaration rather than keeping a second list that would drift away from it. Components come from the router rather than a hand-written list, so a screen added next month is covered without anyone remembering. 1,188 component mounts across nine branches, 9.5M records inspected, no screen handing its view another branch's record. Co-Authored-By: -
Mahmoud Aglan authored
A club that ran on paper for years does not arrive in the system as a clean ledger. On the first academy live, 33 players were registered with an invoice raised and the "pay now" toggle left off — 25 of them in two data-entry evenings — and 27 of those are now carrying an unpaid registration month plus an unpaid September renewal. Nine paid for the federation card in instalments typed into free-text lines. Eight invoices were issued at zero because no price existed yet. Three people exist twice. None of that is a bug in one screen; it is a whole class of file that reality got ahead of. The desk had four tools that each did a slice: collect a payment, correct one invoice's amount, back-fill missing invoices, register someone who started months ago. None of them answers the question an operator has in front of a parent — this file is wrong in several ways at once, what do we do about all of it — so corrections were made wherever a screen allowed them and the ledger drifted further. SettlementService applies a reviewed set of corrections as one transaction and one record: money taken and never entered (on the day it was actually taken), a month closed for less than it was billed because the player joined halfway through, a month dropped entirely, a month nobody billed, a card or kit sold outside the system, a free-text line linked to the product it was really paying for, an agreed instalment plan, a payment sitting on the wrong month, and an overpayment held as wallet credit. Money moves through PaymentService so the ledger, the balance and the receipt all happen; stock through InventoryService; a waiver is written as the admin_override the roster already knows how to explain, leaving subtotal_amount alone so "650 of 900, discounted" still reads. Nothing calls auth() or session(): actor, branch and amounts are parameters. AccountAnomalyScanner finds the files rather than waiting for an argument at the desk — seven cases, worst first, each with the sentence that says what to check. SettlementWorklist lists them with a CSV export; AccountSettlementWizard puts one account on a page, proposes the corrections that fit what it found, shows exactly what will be collected, waived and billed, and demands a written reason before it writes anything. Both screens are gated on a new settlements.manage permission — waiving a month is the academy's call, and an owner should not need a platform administrator to make it — delivered by migration as well as seeder, since db:seed only runs on a first deploy. Two things the tests caught rather than production: Postgres refuses FOR UPDATE on an aggregate, so numbering settlements from max(id) would have rolled back a whole settlement the operator had already confirmed; and payment_plans_status_check has no 'partial', so a part-paid plan is active with the count saying how far along it is. Verified against a restored oc-sport tenant: 20 settlement cases and 7 render/permission cases pass, including cross-participant access, a future date, an oversized payment, and a failing second action rolling the first one back. Full suite 298 tests, no failures. Co-Authored-By: -
Mahmoud Aglan authored
The قيد column asked the database one question — is there an invoice line carrying this product's itemable morph? That is how the POS writes a sale and not how most of this money was collected. A receptionist taking the first instalment types "القسط الاول من القيد" into a free-text line; the player has paid, and the roster called him a non-buyer. On OC-Sport that is fourteen lines across nine players, every one of whom had paid. BundledProductLine reads those lines the way SubscriptionLine reads subscription ones: over whole normalised words, matched against the words that identify the product and nothing else. "قيد اشتراك فريق اتحاد الكرة" is identified by قيد and اتحاد — اشتراك heads half the subscription lines in the same ledger and فريق is how the programmes are named, so any word appearing in a programme name is dropped as unable to tell the two apart. A line that names nothing at all ("القسط الاول") is attributed only where it can be: the programme requires exactly one product and the invoice pays for no training, so there is one thing here paid in instalments and that is what it is paying off. The column now shows the money rather than a yes/no: paid so far against what is owed, "أقساط" while it is being paid off, "مدفوع بالكامل" once it is settled. What is owed comes from the product line when there is one — that price was agreed and frozen at the sale. Money typed by hand is only the instalments taken so far, so 2,500 of 2,500 would call a third of a card paid in full; there the total is the product's price for this member's tier, marked ≈ and explained in the cell's title. Two matching consequences: instalment wording no longer counts as subscription (training is billed by the month here, so a bare "القسط الاول" made a player who had paid 2,500 toward his card read as having paid for July's training), and free players no longer inflate the header's "بدون" count — they are exempt from the bundle, and the cell already says so with a dash. Verified against a restored OC-Sport tenant: all six فريق rosters render, the reported player (عبدالله أحمد صلاح سيد) now reads 2,500 / 8,000 ≈ (31%) أقساط instead of "لم يشترِ", and every hand-typed payer is counted. 271 tests pass. Co-Authored-By: -
Mahmoud Aglan authored
Everything so far was validated against one client database. The tenants are not all shaped like OC-Sport, every one of them runs `migrate --force` on its next container start, and there is no staging in between — so the risk worth chasing was never OC-Sport, it was the tenant I cannot see. Reproduced by building one. On a database where a programme's groups run at two branches, 2026_09_13_000002 declines to guess and falls through to the main-branch fallback. The group at the other branch keeps pointing at a programme that branch can no longer see: its name renders blank, and PricingService cannot find a base price for it, so the enrolment cannot be billed at all. `لا يوجد سعر محدد`, on a group that worked the day before. Silent, and caused by the migration rather than found by it. - 2026_09_13_000003 replicates instead of picking a winner. The programme keeps its identity where it was pinned, every other branch using it gets a copy of its own, and that branch's groups, enrolments, active prices and product bundles are repointed at the copy. Nothing is deleted and nothing changes branch. Two branches running "فريق 2018" now have two rows that can diverge, which is the point — the same answer the product already gives for groups. Verified on a constructed tenant carrying the fault, and a no-op on OC-Sport. - `php artisan branch:audit` reports what is silent in the UI: strictly-scoped rows with no branch (not a leak — a disappearance, present in SQL and on no screen), children in a different branch from their parent, and programmes with live enrolments and no active price. Exits non-zero so it can gate a deploy check. On OC-Sport it finds one genuine pre-existing problem — programme #32 has six active enrolments and no price at all — and no branch integrity faults. - BranchValidationRulesTest closes a gap in the suite itself: every other branch test needs a restored Postgres tenant and skips without one, so on an ordinary `php artisan test` none of them run. This one reads source, so it runs everywhere — banning a raw `exists:` rule on a branch-owned table (they compile to a raw query that accepts any id in the academy, and the property feeding one is usually browser-settable), and failing when a model carries branch_id in $fillable without declaring how it is scoped. - Event now declares BRANCH_SCOPE_EXEMPT with its reasoning, so that being academy-wide reads as a decision rather than as a model somebody forgot. Standard suite: 231 tests, no failures. Against a restored tenant: 28 branch tests, 23,093 assertions — and the same suite passes against the constructed tenant that carried the split fault. Co-Authored-By: -
Mahmoud Aglan authored
The previous check asked whether a dashboard query mentions branch_id. That is the weaker half of the question. A widget that hardcoded the main branch, or read a stale id off the URL, or took auth()->user()->branch_id instead of the session, would mention branch_id on every query and still show the wrong branch's numbers — and it would look perfectly correct in testing, because the main branch is the one usually selected. So the assertion is now on the bound value, not the SQL text. Laravel's bindings are positional, so the value belonging to a `branch_id = ?` predicate is found by counting the placeholders before it. Every dashboard is rendered under each branch that carries data, and every branch id bound into a branch_id comparison must equal the branch the session selected. The seven widget components are also mounted directly, rather than only through the pages that embed them. Two of them — EnrollmentTrends and RevenueWidget — are written but on no view today, so page-level coverage alone would have said nothing about either. Measured on the live tenant: with branch 1 selected, 536 branch bindings, all of them 1. With branch 2, 1,182 bindings, all of them 2. No dashboard binds a branch other than the selected one. Co-Authored-By: -
Mahmoud Aglan authored
The two existing suites cannot catch a leaking dashboard. BranchIsolationTest proves the scope narrows a model; BranchScopedScreensTest looks for another branch's records in the rendered HTML. But a dashboard renders totals, not records — a revenue widget quietly summing every branch shows a number that is simply wrong, with no uuid anywhere to give it away, and both suites pass. Dashboards are also where the raw query builder lives, because that is what aggregates are written in, and a raw DB::table() goes straight past every global scope. So this asserts at the only layer that sees both: the wire. Two checks: - Every SQL statement each of sixteen dashboards runs is captured with DB::listen, and any query reading a branch-owned table without mentioning `branch_id` anywhere — its own WHERE, a join, a subquery the scope added — is a failure. Deliberately crude, because a strict SQL parse would be worse than useless here: a query that never says the word never asked. - The per-branch figures for participants, enrolments, invoices, payment totals, attendance and groups must add up to the academy-wide figure. A widget ignoring the branch returns the whole academy for every branch, so the sum comes out a multiple of the truth. Current state: all sixteen render, 1,354 queries captured, 902 of them touching branch-owned tables, and every one names a branch. Co-Authored-By: -
Mahmoud Aglan authored
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: -
Mahmoud Aglan authored
Branch was built as a reporting lens. Nine tables carried `branch_id`, the trait that stamped it deliberately added no scope, and each of 212 Livewire components was individually responsible for remembering to filter. Most remembered for their main list query. Almost none remembered for the <select> rendered beside it — so picking a branch narrowed the table and left every dropdown, typeahead, count badge and print template still offering the other branches' groups, players, facilities and trainers. On OC-Sport, where only ZSC is live and six other branches sit half-configured, that surfaced as stray rows turning up in pickers all over the app. An audit of the whole surface found 434 such leaks across 165 files. Per-screen patching would not have held: the next component added would forget again. So branch now works the way academy already does — a global scope on the model, which no screen can route around. - BranchScopeState owns the on/off switch, BranchScope the filter itself. ResolveBranchContext activates enforcement; it stays off for console and queues, guests, member-portal accounts and the routes in config/branch_scope.php. A cron that silently billed one branch, or a parent hidden from their own child, would each be worse than the leak being fixed. - Models fall into four buckets. STRICT (operational records — players, groups, facilities, invoices, payments, attendance) filter `branch_id = :active`. SHARED (catalogue and configuration — products, base prices, pricing rules, programmes, trainers) filter `branch_id = :active OR branch_id IS NULL`, because null there means "every branch uses this row" and hard-filtering it would leave other branches with no active base price — a hard failure that stops a sale, not a tightening. CHILD line items scope through their parent relation rather than growing a denormalised branch_id that drifts. The rest are academy-level and get no trait. - The migration adds branch_id to 22 more tables and backfills every existing row parent-first (a session from its group, an attendance record from the session), falling back to the academy's main branch. Every step is guarded and additive, and it is safe to re-run: verified by rollback and re-migrate against a populated tenant copy. - The residue a scope cannot catch is fixed by hand: `exists:` validation rules run raw SQL and accepted any id in the academy, browser-settable `#[Url] public $branch_id` properties were an authorisation bypass rather than a filter, raw DB::table aggregates bypassed Eloquent entirely, and several figures attributed a row through the wrong table (an invoice's branch read from its payments'). Screens that are cross-branch by design — transfers, executive roll-ups, branch administration — now opt out explicitly and narrowly. Verified against a restored OC-Sport copy: BranchIsolationTest and BranchScopedScreensTest, 13 tests / 22,897 assertions, comparing what Eloquent returns under each branch against what raw SQL says is in it, rendering every staff screen, and proving a branch id in the URL cannot override the selected one and a detail route cannot open another branch's record. Standard suite: 223 tests, no failures. docs/agent-rules/19-branch-isolation.md records the rule and how to add a new branch-owned table. Co-Authored-By:
-
- 01 Sep, 2026 5 commits
-
-
Mahmoud Aglan authored
On 1 September OC-Sport had 230 active players due for renewal and the nightly command raised zero invoices. It resolved the invoice actor with User::find($enrollment->created_by); `enrollments` has no created_by column — it is enrolled_by — and Eloquent answers null for a missing attribute, so User::find(null) returned null and every enrolment took the "no valid user" branch. Introduced by da344cda on 9 August, which is why August still billed (2 and 5 August) and September did not. The command printed a summary and exited SUCCESS the whole time, so nothing in the system disagreed for four weeks. The column name was the trigger; the silence was the bug. Four things change so this class of failure cannot repeat: * The actor comes from enrolled_by, and falls back (programme creator, then an academy admin) rather than skipping. A receptionist leaving the academy must never be the reason a paying member goes unbilled. * A run that finds players and bills none of them exits FAILURE. The scheduler now reports a broken run instead of a tidy one. * Cycles are caught up. The old code advanced next_billing_date by addMonth() and raised one invoice, so a run lost to a container restart at 07:00 skipped that month permanently. It now bills every cycle between next_billing_date and today. * Invoices are dated the cycle they buy, never the day the job ran, and carry metadata.month — the signal SubscriptionLine trusts above Arabic month names and above issue_date. A September renewal raised on the 20th is still September money. The "renew on the 1st" rule was hand-written in five places and three disagreed: the command drifted the anchor a day every time a run was late, ReconciliationWizard advanced from now() instead of from the cycle it was closing (skipping one), and EnrollmentService ignored billing cycles longer than a month. They now share App\Domain\Training\Support\ BillingCycle, which is the only definition of the rule. Also fixed along the way: * Discounts were put on the invoice header AND the line, and recalculateTotals() computes total = sum(line totals) - header discount, so every discounted renewal charged the discount twice. OC-Sport has an active sibling-discount rule, so this was live money. Lines now carry the undiscounted price, which is the convention ParticipantBillingService already documents. * CollectPaymentWizard deduplicated renewals with notes LIKE %{programme name}% over unpaid statuses only. OC-Sport has three programmes called "فريق 2018", so one player suppressed another's; and once a renewal was paid the guard stopped seeing it and the next visit billed the month again. It now matches on the cycle and the enrolment, and invoice creation shares a transaction with the next_billing_date advance. * An active paying enrolment with a null next_billing_date was invisible to every renewal query in the system, permanently. Such rows are now adopted onto the current cycle — never retroactively. Verified against a copy of the OC-Sport tenant: 230 invoices dated 2026-09-01, 65 members at 650 EGP and 162 non-members at 900 EGP, the sibling discount applied once, re-running adds nothing. Six players on عبدالعال 2012 fail loudly because that programme has no base price — a hard fail by design, and now visible instead of silent. Co-Authored-By:
-
Mahmoud Aglan authored
Every branded page on the live tenant answered 500 with "BrandingService::for(): Return value must be of type BrandProfile, __PHP_Incomplete_Class returned". config/cache.php shipped Laravel's default serializable_classes => false, which unserializes cache values with allowed_classes: false. That is safe for an app that caches only scalars and arrays, and fatal for one that does not. We cache whole value objects deliberately: BrandProfile is the entire tenant brand, resolved once and held until branding changes, and read by every admin, portal and print layout. With classes refused it came back as __PHP_Incomplete_Class, the return type threw, and the admin went dark. The setting exists to stop a gadget chain in a cache an attacker can already write to. Ours is the tenant's own Postgres, reachable only by the app; anyone who can write there can do worse directly. CACHE_SERIALIZABLE_CLASSES lets a deployment pass its own allowlist without a code change. BrandingService now also checks what the cache handed back before trusting it, and rebuilds when it is not a profile. A cache that cannot return this class should cost a rebuild per request, never a 500 — the failure has to degrade, not detonate. Verified: every admin screen and the group roster render against a restored copy of the live tenant with CACHE_STORE=database. Co-Authored-By: -
Mahmoud Aglan authored
The terminal read products.selling_price and never called the pricing engine, so a product priced per membership type — a member rate and a walk-in rate, which is how base_prices has always modelled it — sold at whichever single number the catalogue carried. Members were charged the non-member price and nothing on the receipt showed it had happened. Every product line now comes from PricingService, which resolves the base price for THIS buyer and then applies the academy's rules to it. selling_price stays the fallback for a product nobody has priced through the engine: it is the price the catalogue advertises, and refusing the sale outright would close the shop over a configuration gap. Cashiers scan first and identify the customer afterwards at least as often as the other way round, so selecting or clearing a participant re-prices what is already in the cart, and the product grid shows that buyer's price with the list price struck through beside it — a rate the cashier cannot quote out loud is a rate that gets argued about at the counter. Checkout prices everything again before it believes any of it. The cart is a public Livewire property, so the unit prices arriving at checkout are whatever the browser last sent; if the engine's answer differs from what the cashier is looking at, the sale stops rather than charging a total nobody saw. Verified against a restored copy of the live tenant: with member/non-member base prices on a real product, member 900, non-member 1,200, walk-in 1,200, and a product with no base price falls back to its catalogue 8,000. Co-Authored-By: -
Mahmoud Aglan authored
The roster column called itself الدفع and answered two questions wrongly. WHAT COUNTS. "Subscription" was every invoice line with no itemable — which is every line a receptionist typed by hand. On the live tenant that swept in 42,800 EGP of federation registration fees ("قسط القيد", "قيد اشتراك", "أقساط متبقية من قيد اتحاد الكرة") and kit ("الزي", "شنطة لبس") and reported it as training money players had paid. SubscriptionLine now reads the text: the academy's own product names first, then its programme names, then the words for registration and kit — matched on whole normalised WORDS, never substrings, because "تجهيزي" contains the letters of "زي" and a substring match turns a subscription into merchandise. Anything still unrecognised keeps counting as subscription, so no line disappears unannounced. WHICH MONTH. The month was the invoice's issue_date, which is only the covered month when the invoice was raised inside it. August's subscription typed up in September belonged to no cycle at all, and one invoice covering July and August counted twice over. Months now come from the invoice's own metadata, then any month named in the line or the notes ("اشتراك يوليو 2026", "اشتراك شهر 8", ranges), then the issue date — and a line covering several months is split evenly across them, remainder on the last, so the parts can never exceed what was billed. A line that names no month is still judged by its date, which is the only evidence there is. A year is only read as a date when it sits near the invoice, so the age group in "فريق 2011/2012" cannot date a 2026 subscription to 2011. Verified against a restored copy of the live tenant: subscription billed for July 123,173 -> 101,073 EGP and August 200,538 -> 187,038 EGP, and every excluded line was checked one by one — all 14 are registration or kit, none is a programme. No line currently moves month; that half is future-proofing plus the combined-invoice case the old data is full of. Column renamed الدفع -> الاشتراك and the header now carries the year as well as the month, because "سبتمبر" alone does not say which September. Co-Authored-By: -
Mahmoud Aglan authored
A pointer-level test and a page-by-page diff against the reference site, rather than reading the markup. Dropdowns could be opened but not used. The panel sat 4px below its trigger with nothing in the gap, and the menu closed the instant the pointer left the trigger's box — so travelling to an item crossed dead space and the menu shut before the click landed. The panel now starts flush against the trigger (`top-full`, padding inside the panel rather than a margin outside it), and the close is delayed and cancellable. Same contract applied to the language switcher, which had the identical gap. Verified by dispatching real mouse moves along the path a hand takes: the menu survives the trip and the click lands on /en/football. `columns` silently dropped its children. It reads them from slots col1..colN, so a child in the generic `default` slot — what a blueprint import or a hand-built tree naturally produces — matched no column and vanished with no error and no empty box. That is how the about-us video disappeared. Loose children are now dealt out across the columns. Nine of eleven pages had no h1 at all. Blocks hardcoded their heading level, so whichever section carried a page's title rendered as h2 and the document had no top-level heading. `heading_level` is now part of the section-header contract (and of text_image, video, profile_card, contact_form, app_download, cta, rich_text), so the section carrying the page title is an authoring decision rather than an accident of which block was used. Every page now has exactly one. Also from the sweep: - El3ab's entire privacy policy — nine sections, both languages — was absent from our page. Restored from the reference render. - The line the reference shows above its contact form was missing; contact_form gains `form_intro`. - The chairman's name was a <p> on top of the portrait; it is a heading. - Footer column titles were <h4> with no <h3> above them, a skipped level in the outline screen readers navigate by. Promoted to <h3>. - `heading_rule` had been added to app_download *inside* its features repeater, so every feature row offered a meaningless "underline" control and the section heading offered none. Moved to where the heading actually is. Verified: 22 page/locale combinations answer 200 with no logged block failures, 137 block/variant combinations render, every page has exactly one h1, the carousel still does not scroll the page, and the suite passes. Co-Authored-By:
-
