# UserProgramService Decomposition — Follow-Up Work Produced by the `refactor/userprogramservice-decomposition` branch (2026-08-25). That branch was **behaviour-neutral by design**: it moved code and changed nothing else. Everything below was found *while* refactoring, deliberately left alone, and pinned by a test so it cannot drift silently. Each defect is real, pre-existing, and confirmed twice — once by the engineer who found it, once by an independent reviewer. --- ## Production defects, in priority order ### Tier 1 — transaction hygiene (one loses a write) **F1. `updateLanguage` returns a failure after a successful commit.** `app/Services/User/Program/ProgramAssignmentService.php`, Branch B. If `getOnboardCompleteData()` returns null, `$onboardData->bOnBoardComplete` is read *after* `DB::commit()` has already run. `HandleExceptions` turns that property-read warning into a thrown `ErrorException`, the catch returns HTTP 406 `"Something went wrong. Try again."` — **and the language change is already persisted.** The client is told it failed; it did not. Branch A guards this identically-shaped read; Branch B does not. *Fix:* guard the read the way Branch A does, or move it before the commit. **F2. `skipIntroSubScreen` has five early returns inside an open transaction.** `app/Services/User/Program/SubscriptionDetailService.php:103-132`. `DB::beginTransaction()` runs at :103; the returns at :106, :109, :112, :115 and :127 reach neither `DB::commit()` (:132) nor a `rollBack()`. Worse, the `vExtendedUrl` write at :118 executes *before* the :127 return — so that write is performed and then silently discarded at connection teardown. *Fix:* roll back explicitly on each early return, or restructure so the transaction opens after the guards. **F3. `updateSubscription`'s `Invalid Post Data` early return leaks an open transaction.** `app/Services/User/Program/SubscriptionUpdateService.php`. `DB::beginTransaction()` has already run; the early return reaches neither `DB::commit()` nor the catch's `DB::rollBack()`. *Fix:* as F2. ### Tier 2 — unguarded reads that surface as generic 500s **F4. `updateSubscription` has no user-not-found guard.** `$userData = $authenticatedUser ?? $this->userQuery->checkValidUser($userId);` then `$userData->iProgramID` is read **before** `try { DB::beginTransaction(); }`. `checkValidUser()` is genuinely `?User`, so a null user throws outside any catch and propagates to the caller. *Fix:* guard, or move the read inside the try. **F5. `getSubDetails` reads `$userSubscriptionData['plan_id']` unguarded.** `app/Services/User/Program/SubscriptionDetailService.php`. `UserProgramQuery::getSubscriptionDetail()` catches its own DB errors and returns `[]`. The unguarded array read then throws, is caught by the generic handler, and a genuine query failure is reported to the client as "Something went wrong" — indistinguishable from a validation problem. *Fix:* guard the read, or have the query surface its failure. ### Tier 3 — divergences between methods that should agree **F6. `bUnlocked` is `1` in `updateProgram` but `0` in `updateLanguage`** for the same conceptual operation (`ProgramAssignmentService.php:184` vs `:373`). One of them is wrong; product needs to say which. **F7. `sendRedditEvent` and `sendFBEvent` disagree on currency handling.** `app/Services/User/Program/ProgramEventDispatcher.php:111-112`. Meta lower-cases `vCurrency` and defaults to `'inr'`; Reddit does neither and defaults to `'INR'`. Two conversion APIs receive different values from the same input. **F8. `updateLanguage` Branch B calls `activeDayService` unconditionally** before the existing-row check and discards the result when a row is found — a wasted query on every call that finds a row. --- ## Non-defect follow-ups **F9. Three timing-flaky tests will destabilise CI.** Each asserts two IDs generated across a `usleep(1000)` differ, against millisecond wall-clock resolution: - `tests/Unit/Helpers/CommonHelperTest.php:233` - `tests/Unit/Services/MetaCapiServiceTest.php:155` - `tests/Unit/Services/User/UserUtilityServiceTest.php:170` Measured at roughly a 2-in-7 chance of at least one failing per full-suite run under load. With `.github/workflows/ci.yml` about to start enforcing gates, this is a ~25% red rate on unrelated changes. *Fix:* give `generateEventId()` / `generateVEUserID()` a counter or random component — two calls in the same millisecond genuinely collide in production too — or relax the three tests. **F10. ~119 `->ordered()` calls remain inert.** Mockery allocates order numbers per-mock unless `->globally()` precedes `->ordered()`, so cross-collaborator sequences are unenforced. The three tests that explicitly claim a cross-collaborator ordering were fixed on this branch; the rest were left deliberately, because making them enforceable is a per-test judgement (e.g. `UpdateLanguageCharacterizationTest:574` declares its `DB::` expectations out of call order and would go red). *Fix:* per-test, opportunistically. **F11. The six services are bound with `bind()`, not `singleton()`** (`AppServiceProvider.php:94-105`). The facade now constructs `UserUtilityService` 5×, and `UserQuery`/`UserConfigQuery`/`UserProgramQuery` 4× each, on every one of the 11 routes. All six are stateless, so `singleton()` is safe and removes ~40 allocations per request. **F12. Cosmetic inconsistency across the six services** — five are `…Service`, one is `ProgramEventDispatcher`; interface docblocks mix precise `array{...}` shapes with `array` and bare `array`; one constructor does not lead with `UserUtilityService`. Worth one normalising pass. **F13. Dead public surface.** `sendFBEvent`/`sendRedditEvent` remain public on the facade with no production caller, retained only because `tests/Unit/Services/User/UserProgramServiceRedditEventTest.php` is the sole test proving facade→dispatcher delegation. Retarget that test at the interface, then drop the two methods. --- ## Known, accepted behaviour change Preserved everywhere except here. On malformed input, several endpoints moved from **fail-closed to fail-open** because `declare(strict_types=1)` plus an explicit cast now coerces where a `TypeError` previously escaped `catch (Exception)` as a 500: | Endpoint | Input | Before | After | |---|---|---|---| | `api/User/getProgramList` (unauthenticated) | `userid: abc` | 500 | 200, unfiltered program list | | `api/User/UpdateSubscription` | `iCouponId: "abc"` | 500 + dangling transaction | coupon ignored, purchase still recorded | | `api/User/updateProgram` | `iProgramId: "3abc"` | 406 | 200, enrolled in program 3 | Each was checked for data exposure and none creates one — user `0` simply applies no partner filtering. The second is materially better: a user who had already paid Apple/Google previously got a 500 and no recorded subscription on every retry. The first is pinned by a regression test; the other two are not. --- ## Schema dependency worth knowing about Two methods return an untyped collaborator result through a native return type, which is only safe because the columns are integers: - `SubscriptionUpdateService::getDiscount(): ?int` ← `tbl_ProductCodes.iDiscount` is `int unsigned` - `SubscriptionUpdateService::updateUserProgramData(): int` ← `tbl_UserPrograms.iUserProgramID` is `int unsigned` If either column is ever migrated to `DECIMAL` or `VARCHAR`, mysqlnd returns a **string**, the return raises a `TypeError` — an `Error`, not an `Exception` — and it escapes `catch (Exception)` as a 500 with an open transaction on the subscription-purchase endpoint. Comments record this at each return site.