diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 51960448f..8cc45a2a7 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -58,6 +58,17 @@ jobs: - { name: "SummitOrderServiceTest", filter: "--filter SummitOrderServiceTest" } - { name: "SummitRSVPServiceTest", filter: "--filter SummitRSVPServiceTest" } - { name: "SummitRSVPInvitationServiceTest", filter: "--filter SummitRSVPInvitationServiceTest" } + - { name: "SummitServiceTest", filter: "--filter SummitServiceTest" } + - { name: "SpeakerServiceRegistrationTest", filter: "--filter SpeakerServiceRegistrationTest" } + - { name: "PresentationServiceTest", filter: "--filter PresentationServiceTest" } + - { name: "SummitPromoCodeServiceTest", filter: "--filter SummitPromoCodeServiceTest" } + - { name: "SummitScheduleSettingsServiceTest", filter: "--filter SummitScheduleSettingsServiceTest" } + - { name: "SummitSelectedPresentationListServiceTest", filter: "--filter SummitSelectedPresentationListServiceTest" } + - { name: "SelectionPlanOrderExtraQuestionTypeServiceTest", filter: "--filter SelectionPlanOrderExtraQuestionTypeServiceTest" } + - { name: "SummitSelectionPlanServiceTest", filter: "--filter SummitSelectionPlanServiceTest" } + - { name: "SummitRegistrationInvitationServiceTest", filter: "--filter SummitRegistrationInvitationServiceTest" } + - { name: "SummitSubmissionInvitationServiceTest", filter: "--filter SummitSubmissionInvitationServiceTest" } + - { name: "MemberServiceTest", filter: "--filter MemberServiceTest" } - { name: "EntityModelUnitTests", filter: "tests/Unit/Entities/" } - { name: "AuditUnitTests", filter: "tests/Unit/Audit/" } - { name: "AuditOtlpStrategyTest", filter: "--filter AuditOtlpStrategyTest" } @@ -101,9 +112,11 @@ jobs: REGISTRATION_DEFAULT_LIVE_STRIPE_PRIVATE_KEY: REGISTRATION_DEFAULT_LIVE_STRIPE_PUBLISHABLE_KEY: REGISTRATION_DEFAULT_LIVE_WEBHOOK_SECRET: - REGISTRATION_DEFAULT_TEST_STRIPE_PRIVATE_KEY: sk_test_12345 - REGISTRATION_DEFAULT_TEST_STRIPE_PUBLISHABLE_KEY: pk_12345 + REGISTRATION_DEFAULT_TEST_STRIPE_PRIVATE_KEY: ${{ secrets.TEST_STRIPE_SECRET_KEY || 'sk_test_12345' }} + REGISTRATION_DEFAULT_TEST_STRIPE_PUBLISHABLE_KEY: ${{ secrets.TEST_STRIPE_PUBLISHABLE_KEY || 'pk_12345' }} REGISTRATION_DEFAULT_TEST_WEBHOOK_SECRET: whsec_12345 + TEST_STRIPE_SECRET_KEY: ${{ secrets.TEST_STRIPE_SECRET_KEY || 'sk_test_dummy_key' }} + TEST_STRIPE_PUBLISHABLE_KEY: ${{ secrets.TEST_STRIPE_PUBLISHABLE_KEY || 'pk_test_dummy_key' }} BOOKABLE_ROOMS_DEFAULT_PAYMENT_PROVIDER: Stripe BOOKABLE_ROOMS_DEFAULT_STRIPE_TEST_MODE: true BOOKABLE_ROOMS_DEFAULT_LIVE_STRIPE_PRIVATE_KEY: diff --git a/adr/003-nested-transaction-rollback-safety.md b/adr/003-nested-transaction-rollback-safety.md new file mode 100644 index 000000000..6361b883a --- /dev/null +++ b/adr/003-nested-transaction-rollback-safety.md @@ -0,0 +1,392 @@ +# ADR-003: Nested Transaction Rollback Safety — Remove DBAL Savepoints, Rely on Native `isRollbackOnly` + +- **Status:** Accepted +- **Date:** 2026-07-10 +- **Branch:** `hotfix/doctrine-tx-manager` +- **Component:** `app/Services/Utils/DoctrineTransactionService.php` + +## Context + +`DoctrineTransactionService::transaction()` is the single choke point every service in this +codebase uses to wrap business logic in a DB transaction. It detects whether a transaction is +already active on the connection and routes to one of two paths: + +- **Root** (`runRootTransaction`) — owns the connection lifecycle, sets the isolation level, + retries on transient connection errors, flushes the `EntityManager`, and issues the real + `COMMIT`. +- **Nested** (`runNestedTransaction`) — used when a service method wrapped in its own + `tx_service->transaction()` call is invoked from inside another service method that is + *also* wrapped in its own `tx_service->transaction()` call (an "outer/inner" pair). It + flushes so auto-generated IDs are available to the caller, but does not retry, reset the + `EntityManager`, or commit at the DB level — that's the root's job. + +The question this ADR answers: **when the inner (nested) transaction fails, what happens to +the outer (root) transaction's eventual commit?** Three iterations were needed to get this +right. + +## Decision Timeline + +### Baseline (`origin/main`) — no nested-transaction awareness at all + +Before this branch, `DoctrineTransactionService::transaction()` (`origin/main`, verified at +`988a6d3e6`) had **no root/nested distinction whatsoever** — every call, whether the outermost +one or one invoked from inside another already-open `transaction()` call, ran the identical +retry loop: + +```php +$em->getConnection()->beginTransaction(); +$result = $callback($this); +$em->flush(); +$em->getConnection()->commit(); +``` + +And on **any** exception at all — a connection drop, or an ordinary business-rule +`ValidationException` with no connection problem whatsoever — the catch block did this +**unconditionally**, regardless of nesting depth or exception type: + +```php +$em->getConnection()->close(); +$em->close(); +if ($em->getConnection()->isTransactionActive()) $em->getConnection()->rollBack(); +Registry::resetManager($this->manager_name); +``` + +No `setNestTransactionsWithSavepoints` call exists anywhere in this baseline — savepoints were +never part of `main`'s design; they were introduced (and later discarded) entirely within this +branch, see Iteration 1 below. + +**Why this is unsafe for nested calls.** Because a nested `transaction()` call is just a normal +recursive call to this same method, a failure at the INNER level closes the entire physical DB +connection and the shared `EntityManager` before the exception even propagates back to the +OUTER call. The outer call's own local `$em` reference now points at an already-closed manager +— its next `flush()` throws `EntityManagerClosed`, which its own catch block then tears down +*again* (a second `close()+close()+resetManager()`), on top of whatever work the outer callback +had pending. This is the mechanism the empirical A/B test later in this document refers to as +"`main` (old code): ... calls `Registry::resetManager()` **twice**, loses everything." The +blast radius is total and applies to every nested failure uniformly, not just connection-level +ones — there is no scoped, partial-rollback concept here at all. + +### Iteration 1 — DBAL savepoints, discarded + +Commit `d88de2ce3` introduced the root/nested split above (specifically to stop a nested +failure from destroying the outer `EntityManager`, per that commit's own message) and, alongside +it, called `$conn->setNestTransactionsWithSavepoints(true)` on the root transaction. The intent: when a +nested transaction's `rollBack()` runs on a savepoints-enabled connection, DBAL issues +`ROLLBACK TO SAVEPOINT` instead of rolling back the whole DB transaction — undoing only the +inner work while leaving the outer transaction free to continue and commit its own writes. +This was meant to make a "catch the inner failure, log it, and continue" pattern safe. + +**Why it was discarded.** `ROLLBACK TO SAVEPOINT` only undoes the *database's* row-level +changes for that inner transaction. It does nothing to Doctrine ORM's `UnitOfWork` — the +in-memory identity map of managed entities and pending changesets. Doctrine's `UnitOfWork` has +**no concept of savepoints at all**: it does not know a savepoint rollback happened, does not +discard the entities the inner transaction created/mutated, and does not roll back their +in-memory state. After a savepoint rollback, the outer transaction's `EntityManager` can still +hold references to entities that: + +- Doctrine's identity map believes are managed/persisted, when the DB has actually discarded + their rows, or +- carry partially-applied field mutations from the inner transaction that never made it to + disk. + +The next `flush()` — root's own, or a *different* nested transaction reusing the same +`EntityManager` — has no reliable way to know which parts of its unit of work are still +backed by real rows and which were silently discarded at the DB level by a savepoint +rollback it never modeled. That is a structural, silent desync between the DB and the object +graph — exactly the shape of bug that produces the worst kind of data-loss report ("the API +said success, but the row isn't there" or the inverse, "a stale in-memory object gets +re-persisted"). It is not fixable by adding more code on top of savepoints; the ORM layer +Doctrine ships simply doesn't support partial-rollback recovery. + +Two follow-up commits tried to hage this safer without removing savepoints: + +- `24c1077db` ("harden root/nested split against phantom writes and masked errors") — added + `em->clear()` on root failure so a failed callback's pending changes can't leak into the + next transaction on the same `EntityManager`; added a fail-fast check when a swallowed + nested flush failure left the `EntityManager` closed; guarded rollback failures from masking + the original exception; started warning once when savepoints were unexpectedly disabled on + the connection (e.g., an outer transaction started outside this service). + +These closed several real bugs (masked exceptions, phantom writes leaking across retries, +opaque `EntityManagerClosed` errors two levels deep) but did not — and structurally could +not — close the core UnitOfWork/savepoint desync, because the desync is a property of how +Doctrine ORM's UnitOfWork is built, not a bug in this service's bookkeeping. + +### Iteration 2 (final approach) — no savepoints, native `isRollbackOnly` propagation + +Commit `248f8e453` deleted `setNestTransactionsWithSavepoints(true)` from the root transaction +entirely (and the now-pointless "warn when savepoints disabled" mechanism from the nested +path). Verified directly against the vendored DBAL 3.9.4 source +(`vendor/doctrine/dbal/src/Connection.php`): with savepoints off, a nested `rollBack()` (any +nesting level > 1) does exactly this — no SQL, no partial-rollback semantics to desync from: + +```php +$this->isRollbackOnly = true; +--$this->transactionNestingLevel; +``` + +And `commit()`, at **any** nesting level, checks this flag **first**, before anything else: + +```php +if ($this->isRollbackOnly) { + throw ConnectionException::commitFailedRollbackOnly(); +} +``` + +DBAL already provides "one nested failure poisons the whole chain, unconditionally" natively +— the savepoints flag was the only thing suppressing that native protection in favor of a +partial-recovery mechanism the ORM layer cannot safely support. Once the flag is gone, **any** +subsequent `commit()` call anywhere in the chain — nested, root, or Doctrine ORM's own +internal per-`flush()` commit — fails immediately and loudly. A caught-and-swallowed nested +failure can no longer end in a silent, successful root commit; it is structurally impossible, +not merely tested-for. + +**Empirical confirmation** (against a real local MySQL instance, not mocks — Mockery cannot +model DBAL's internal `isRollbackOnly` flag): a 3-level nesting scenario where the middle +level internally catches the deepest level's business-exception failure and returns normally +was re-run before and after this change. Before: the old savepoints-based code reached a +"successful" root commit with the deepest level's writes silently missing. After: the root's +own commit throws (`Doctrine\ORM\OptimisticLockException('Commit failed')`, wrapping DBAL's +`ConnectionException::commitFailedRollbackOnly`) and zero rows are ever durably committed — +the entire operation succeeds or fails as one atomic unit. + +**Trade-off accepted.** This makes "catch a nested failure and continue in the same still-open +outer transaction" universally unsafe, for *any* nested failure — not just post-flush ones (the +pre-fix docblock's "safe only for pre-flush errors" carve-out no longer applies either, since +DBAL sets `isRollbackOnly` unconditionally regardless of whether the nested transaction ever +flushed). This was checked against real call sites before accepting the trade-off: an +automated code review flagged `RegistrationIngestionService::ingestExternalAttendee()` as a +call site that "currently works" with this catch-and-continue pattern and would supposedly +regress. An empirical A/B test (both the true pre-fix `origin/main` code and this branch's new +code, same pattern, same MySQL instance) showed **neither version ever actually made this +pattern safe** — the old code lost the same rows via a double-`EntityManager`-reset/dangling- +reference cascade instead of a single clean exception. The new code's failure mode is more +diagnosable, not a regression. That specific site's "race condition" recovery comment has +never protected against the race in either version; it is logged as a separate, independent +pre-existing bug, out of scope for this hotfix. + +## Consequences + +- **Positive:** A nested transaction failure can never be silently absorbed into a successful + outer commit. This is now a structural DBAL guarantee, not something every call site has to + reason about individually. +- **Positive:** Diagnosability improved — failures now surface as a single + `OptimisticLockException`/`ConnectionException::commitFailedRollbackOnly` instead of a + cascade of `EntityManagerClosed` / dangling-reference errors from double-resetting the + registry. +- **Negative / accepted trade-off:** Any existing "log the inner failure and continue" pattern + built on the old savepoints behavior is now guaranteed to abort the entire outer operation + instead. Verified this was never actually a *working* safety net to begin with (see the A/B + test above), so nothing that previously worked correctly was broken. +- **Negative / accepted trade-off:** Per-row batch operations (CSV imports, bulk approvals, + etc.) that call an inner-transaction-wrapped method per item now abort the *entire* batch on + the first item's failure, unless the call site wraps that specific call in its own local + `try/catch` **outside** the inner `transaction()` closure (i.e., after that item's transaction + has already fully committed or rolled back — see `SummitService::processRegistrationCompaniesData` + below for the one call site in this codebase that already does this correctly). +- **Out of scope, tracked separately:** `OptimisticLockException` misclassified as + non-retryable (`shouldReconnect()` never unwraps `getPrevious()` to check if the underlying + cause was a transient connection blip) — a pre-existing gap this fix reduces one trigger for + but does not close. + +## Post-Review Hardening (same branch) + +Five gaps surfaced by the deep review of PR #533 were closed on top of Iteration 2, each with +a unit test that reproduced the failure first: + +1. **Ambiguous commit failures are never retried.** A connection failure during the root's + real `COMMIT` is ambiguous — the server may have already made the transaction durable and + only the acknowledgment was lost (DBAL decrements its nesting counter in a `finally` even + when the physical commit fails, so nothing is left to roll back). The retry loop used to + re-execute the entire callback in that state, duplicating every write and side effect up to + `MaxRetries` times; `runRootTransaction()` now tracks the commit phase and propagates the + failure instead. The failure surfaces as `AmbiguousCommitException` (the driver exception + preserved as `previous`) rather than the raw driver exception: without the marker type, the + "callers must treat it as operation state unknown" contract is unenforceable — a propagated + `ConnectionLost` looks retryable to the layers above (Laravel queue `tries`, caller-side + retries), which would re-execute the whole callback anyway. Queue jobs should catch it and + `fail()` without retry, then reconcile. It extends plain `\RuntimeException`, so + `shouldReconnect()` can never re-classify it as retryable. + Covered by `testRootTransactionDoesNotRetryWhenCommitFails`. +2. **Closed-EM re-entry can no longer escape the atomicity guarantee.** `isRollbackOnly` is a + per-connection flag. When a nested flush failure closed the `EntityManager` and an + intermediate callback caught it and called `transaction()` again, the closed-EM branch used + to `resetManager()` onto a brand-new `EntityManager` **and a brand-new DBAL connection** + (`EntityManagerFactory` → `DriverManager::getConnection`) — the "recovered" root's commits + were durable even though the outer, rollback-only transaction on the old connection rolled + back: a split-brain partial commit. `transaction()` now refuses that state (closed EM while + its connection still has an active transaction) with a descriptive `RuntimeException`. + Covered by `testTransactionRefusesWhenEntityManagerClosedWithActiveTransaction`. +3. **The fail-fast message no longer repeats the retracted carve-out.** It used to claim + catching nested errors is "only safe for errors thrown before any flush" — contradicting + this ADR's own conclusion that DBAL sets `isRollbackOnly` unconditionally, pre-flush or not. + The message now states that catching a nested `transaction()` failure and continuing is + never safe and directs the reader to let the failure propagate to the root. +4. **A failed rollback discards the manager/connection pair** (found by a Codex companion + review). `safeRollback()` swallows rollback failures by design (the original exception + must never be masked or re-classified as retryable), but the cleanup decision afterwards + looked only at the original exception and `$em->isOpen()`. A business exception followed + by a rollback failure (connection died mid-callback) therefore left an **open** EM wired + to a dead physical handle registered — and DBAL zeroes `transactionNestingLevel` *before* + the physical rollback while clearing `isRollbackOnly` only *after* it succeeds, so the + flag can also be left stuck. Subsequent `transaction()` calls self-heal via the reconnect + path, but direct Registry consumers (repositories, serializers, queue jobs reading + outside `transaction()`) have no retry path and would fail in a chain on a long-lived + worker. `safeRollback()` now reports success/failure and `runRootTransaction()` discards + the broken pair (close EM, close connection, reset a fresh manager into the registry — + best-effort, never masking the original exception, never retrying). The same hygiene + applies to connection-level commit-phase failures, which previously also left the dead + handle registered. Root-only by construction: with savepoints off, a nested `rollBack()` + executes no SQL (flag + counter only), so it cannot fail on a dead connection — a dead + connection during a nested transaction surfaces as the root's own rollback failing, which + this covers. Covered by `testRootTransactionDiscardsManagerWhenRollbackFails` and the + extended `testRootTransactionDoesNotRetryWhenCommitFails`. +5. **Deterministic client-side commit failures are never classified as ambiguous.** The + commit-phase flag used to be set before `$conn->commit()`, but DBAL throws + `ConnectionException::commitFailedRollbackOnly()` **client-side — before the COMMIT is + ever sent to the server** — whenever a caught-and-continued nested failure left the + connection rollback-only and the root `flush()` had an empty changeset + (`UnitOfWork::commit()`'s "Nothing to do" early return never touches the connection, so + the root's own `commit()` is the first commit call in the chain). That + definitively-rolled-back failure surfaced as `AmbiguousCommitException` ("may or may not + be durable — reconcile"), sending operators on false reconciliation work and contradicting + this ADR's own documented failure surface. `runRootTransaction()` now checks + `$conn->isRollbackOnly()` right before entering the commit phase and fails fast with a + descriptive plain `\RuntimeException` naming the real cause (a nested failure caught + mid-chain); `AmbiguousCommitException` is reserved for a COMMIT that was actually sent to + the server. Covered by `testRootTransactionFailsDeterministicallyWhenConnectionIsRollbackOnly`. + +## Test Coverage Added + +Two things were proven for every pair below, empirically (real local MySQL, not mocks — DBAL's +`isRollbackOnly` propagation cannot be modeled by Mockery): (a) the mechanism itself +(`DoctrineTransactionService`'s root/nested split honors the new no-savepoints contract), and +(b) that each specific outer/inner method pair in this codebase actually exhibits the +guaranteed behavior end-to-end. The rows marked in **bold** pin deliberately CONTRASTING, +non-nested shapes for reference — per-row isolation and two sequential root transactions — +rather than the nested-rollback contract itself. + +### `DoctrineTransactionService` itself (unit-level, mocked) + +| File | What's covered | +|---|---| +| `tests/Unit/Services/DoctrineTransactionServiceTest.php` | 28 tests: root/nested routing, savepoints never enabled/queried, fail-fast on a closed `EntityManager`, no retry after an ambiguous commit failure, refusal of closed-EM re-entry while a transaction is still active, rollback failures never masking the original exception, broken manager/connection discarded when the rollback itself fails, deterministic rollback-only failure at root commit never misclassified as ambiguous, `\Error` handled alongside `\Exception`, `em->clear()` on root failure. | + +### Business-service outer/inner pairs (functional, real DB) + +| Outer service → method | Inner method | Test file | Shape proven | +|---|---|---|---| +| `SummitOrderService::createOfflineOrder` | `createTicketsForOrder` | `tests/SummitOrderServiceTest.php` | Full rollback — invalid promo code | +| `SummitOrderService::addTickets` | `createTicketsForOrder` | `tests/SummitOrderServiceTest.php` | Full rollback — missing default badge type | +| `SummitOrderService::processTicketData` | `createOfflineOrder` | `tests/SummitOrderServiceTest.php` | Full rollback per CSV row, with per-row isolation: a failing row is logged and skipped, later rows still commit; a row surfacing `AmbiguousCommitException` is recorded as *unknown outcome* and the source file is kept for reconciliation instead of being deleted | +| `SummitOrderService::requestRefundOrder` | `requestRefundTicket` | `tests/SummitOrderServiceTest.php` | Full rollback — one free ticket in the loop aborts all refund requests | +| `SummitService::processRegistrationCompaniesData` | `addCompany` | `tests/SummitServiceTest.php` | **Per-row isolation, not full rollback** — this call site has its own local `try/catch` outside the inner transaction's closure, so one bad row does not block later rows | +| `SpeakerService::addSpeakerBySummit` | `addSpeaker` + `registerSummitPromoCodeByValue` | `tests/SpeakerServiceRegistrationTest.php` | Full rollback — a registration code already claimed by another speaker undoes the just-created speaker too | +| `PresentationService::submitPresentation` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track | +| `PresentationService::updatePresentationSubmission` | `saveOrUpdatePresentation` | `tests/PresentationServiceTest.php` | Full rollback — nonexistent track, update never partially applies | +| `SummitPromoCodeService::addPromoCode` | `addPromoCodeTicketTypeRule` | `tests/SummitPromoCodeServiceTest.php` | **Partial commit, not full rollback** — this is actually two separate, sequential root transactions; the promo code from the first survives even when the second (ticket-type-rules) transaction fails | +| `SelectionPlanOrderExtraQuestionTypeService::updateExtraQuestionBySelectionPlan` | `updateExtraQuestion` | `tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php` | Full rollback — a question not assigned to the target plan lets the inner label change be written (flushed inside the still-open outer transaction), then the outer's assignment check fails and undoes it | +| `SpeakerService::updateSpeakerBySummit` | `registerSummitPromoCodeByValue` | `tests/SpeakerServiceRegistrationTest.php` | Full rollback — `updateSpeaker`'s already-written title change is undone when the registration code is already claimed by another speaker | +| `SponsorUserSyncService::addSponsorUserToGroup` | `SummitSponsorService::addSponsorUser` | `tests/Unit/Services/SponsorUserPermissionTrackingTest.php` | Full rollback — the eagerly-created `Sponsor_Users` row is written, then an unresolvable `group_slug` undoes it | +| `SummitScheduleSettingsService::seedDefaults` | `add` (looped) | `tests/SummitScheduleSettingsServiceTest.php` | Full rollback — the loop's first successfully-added default is undone when the second's key already exists | +| `SummitSelectedPresentationListService::assignPresentationToMyIndividualList` | `createIndividualSelectionList` | `tests/SummitSelectedPresentationListServiceTest.php` | Full rollback — the just-written new individual list is undone when the presentation lookup afterward fails | +| `SummitService::unPublishEvents` | `unPublishEvent` (looped) | `tests/SummitServiceTest.php` | Full rollback — an earlier item's already-written unpublish is undone when a later item's event id doesn't exist | +| `SummitService::updateAndPublishEvents` | `updateEvent` (looped) | `tests/SummitServiceTest.php` | Full rollback — an earlier item's already-written update+publish is undone when a later item's `location_id` doesn't exist. **Nuance found during implementation:** the exception actually fires inside `updateEvent()`'s own location check, not `publishEvent()`'s — both do the identical existence check on the same payload, but `updateEvent()` runs first and its check has no `isAllowsLocation()` gate, so `publishEvent()`'s own check is structurally unreachable via this call site for this trigger | + +### API/HTTP-level exception-branch coverage + +| Endpoint | Test file | What's covered | +|---|---|---| +| `POST .../attendees/{id}/tickets` (`addAttendeeTicket`) | `tests/oauth2/OAuth2AttendeesApiTest.php` (`testAddAttendeeTicketFailsOnInvalidPromoCode`) | Invalid promo code rolls back the whole `createOfflineOrder`→`createTicketsForOrder` chain at the HTTP boundary | +| `POST .../orders` (`add`) | `tests/oauth2/OAuth2SummitOrdersApiTest.php` (`testCreateSingleTicketOrder`, `testCreateSingleTicketOrderFailsOnInvalidPromoCode`) | Re-enabled two previously `markTestSkipped()` tests whose skip reason no longer matched current code; proves order creation and its invalid-promo-code rollback at the HTTP boundary | + +## Known Gaps / Future Work + +A follow-up codebase-wide search (same outer/inner shape: method A wrapped in its own +`tx_service->transaction()` calling, with no local `try/catch` around the call, method B which +is *also* wrapped in its own separate `tx_service->transaction()`) originally found 11 +additional pairs across 8 more services with no test proving the new rollback contract. 8 of +those 11 are now covered (see Test Coverage Added above). The remaining 3 were verified against +the real code and found to have **no written-then-rolled-back proof reachable through their +specific call site** — the same structural reason a genuine rollback test can't be built for +them, not merely undiscovered test cases: + +| Outer → Inner | Why no test is possible here | +|---|---| +| `SummitRegistrationInvitationService::add`/`update` → `TagService::addTag` | The outer's own pre-check (`$this->tag_repository->getByTag($tag_value)`) already uses the identical normalized comparison (`UPPER(TRIM(...))`, `DoctrineTagRepository.php:54-63`) that `addTag()`'s own duplicate check uses internally — no case/whitespace gap exists between them for a single synchronous request. `addTag()`'s own `ValidationException("Tag %s already exists!")` can only fire via a genuine concurrent race between two overlapping requests, which cannot be expressed deterministically in a test. | +| `SummitSubmissionInvitationService::add`/`update` → `TagService::addTag` | Same reason as above. | +| `SummitRSVPInvitationService::acceptInvitationBySummitEventAndToken` → `SummitRSVPService::rsvpEvent` | The outer method has no write before the nested `rsvpEvent()` call (its own guards, `:312-336`, are pure reads), and its only post-call write (`markAsAcceptedWithRSVP()`, `:343`) runs strictly *after* `rsvpEvent()` returns — so when the inner throws, there is nothing already written for a rollback to undo. Asserting `isAccepted() === false` afterward would hold identically true with `isRollbackOnly` fully disabled. | + +Also dropped as **redundant, not unreachable**: `SpeakerService::updateSpeakerBySummit` → +`updateSpeaker` via the member-already-assigned trigger. `updateSpeaker`'s check throws before +any write and is the outer's first operation, so this specific trigger can't prove rollback +either — but the *same* outer/inner pair is already proven via the `registration_code` trigger +(`updateSpeaker`'s title change commits first, then `registerSummitPromoCodeByValue` fails and +rolls it back), so no second test was needed for this production class. + +`SummitSelectedPresentationListService::getTeamSelectionList`/`getIndividualSelectionList` → +`createTeamSelectionList`/`createIndividualSelectionList` were also dropped: both outer methods +return immediately after the inner create call with no additional outer-side writes, so testing +them would only re-prove the inner methods' own already-covered nested-transaction mechanism, +not a distinct outer/inner rollback risk. Only `assignPresentationToMyIndividualList` (which +does real work — presentation validation — after the nested call) was covered. + +Several other call sites share the same *family* (calling a nested-transaction-wrapped method) +but are already guarded by a local `try/catch` outside the inner closure — the same safe shape +as `SummitService::processRegistrationCompaniesData` above, not a rollback risk: +`SummitRegistrationInvitationService`/`SummitSubmissionInvitationService`'s +`setInvitationMember`/`setInvitationSpeaker` → `MemberService::registerExternalUser` / +`SpeakerService::addSpeaker`; `SpeakerService::sendEmails` → `generateSpeakerAssistance`; +`ScheduleService::publishAll` → `SummitService::publishEvent`; `SummitService::processEventData` +(per-CSV-row) → `SpeakerService::addSpeaker`/`updateSpeaker`. + +With the 8 testable gaps closed and the remaining 3 confirmed structurally unreachable, this +codebase-wide sweep for the outer/inner nested-transaction shape is complete. + +### Broken race recovery in `RegistrationIngestionService::ingestExternalAttendee` (pre-existing, both versions) + +The `// race condition lost, try to get it` recovery +(`RegistrationIngestionService.php:226-244`, and its twin at `:363-381`) catches a failure from +the nested `MemberService::registerExternalUser()` call — inside the still-open per-attendee +root transaction — and tries to read the race winner's row instead. That recovery has **never +once worked when the race actually fires**: the losing worker's unique-constraint violation +happens at the nested flush (`add($member, true)`, `MemberService.php:334`), which closes the +`EntityManager` (ORM behavior) and marks the connection rollback-only, so the recovery read +itself throws `EntityManagerClosed`, gets swallowed by the enclosing `catch` (`:246-248`), and +the root transaction dies anyway — cleanly here (fail-fast guards, real cause named), and on +`origin/main` via the double-`EntityManager`-reset/dangling-reference cascade the empirical A/B +test in Iteration 2 documented. Net effect in both versions: that attendee is skipped +(`ingestExternalAttendee`'s outer log-and-skip catch, `:630-632`) and picked up by the next +feed run. Pre-existing bug, independent of this branch. + +**Recommended fix (follow-up):** drop the in-transaction recovery and retry at the call site +instead — let the nested failure propagate out of the per-attendee root transaction and re-run +`ingestExternalAttendee` once from the ingest loop (`ingestSummit`, `:694-696`). The second +attempt starts a fresh root transaction whose `getByExternalIdExclusiveLock()` read finds the +member the winning worker committed, and ingestion succeeds — making the recovery the comment +promises actually happen for the first time, without catching around a nested `transaction()` +call (the exact shape this ADR concludes is never safe). + +### `AmbiguousCommitException` — queue jobs still blind-retry ambiguous commits + +The in-service half of the ambiguous-commit protection is delivered: `runRootTransaction()` +never re-executes the callback once the real COMMIT has been attempted (hardening item 1). +The log-and-skip import loops are also wired up on this branch: +`SummitOrderService::processTicketData()`, `SummitService::processEventData()` and +`SummitService::processRegistrationCompaniesData()` all catch `AmbiguousCommitException` +separately from their generic per-row `catch`, record the row as *unknown outcome* (error +level) and keep the source file for reconciliation instead of deleting it — each covered by +a `KeepsFile*WhenRowCommitOutcomeUnknown` test. + +What remains outstanding is the queue-job side. The exception's contract directs job +handlers to catch it and fail without retry (`$this->fail($e)`), then reconcile; as of this +branch, **no queue job catches it**. Laravel's queue does not inspect exception types: any +uncaught exception triggers a retry while `tries` remain, so for any job configured with +`tries > 1` (21 jobs declare `$tries` of 2-5, and the ~35 top-level jobs in `app/Jobs/` +without a `$tries` property inherit the worker's `--tries` default) an ambiguous commit +still re-runs the whole job — the exact duplicate-writes scenario the marker type exists to +prevent. + +**Recommended fix (follow-up):** in payment/order-critical queue jobs, catch +`AmbiguousCommitException` and fail the job without retry, logging it at error level as a +reconciliation-required event. diff --git a/app/Models/Foundation/Main/Member.php b/app/Models/Foundation/Main/Member.php index eb418c493..846f6407c 100644 --- a/app/Models/Foundation/Main/Member.php +++ b/app/Models/Foundation/Main/Member.php @@ -587,6 +587,7 @@ public function setTeamMemberships($team_memberships) public function setGroups($groups) { $this->groups = $groups; + $this->groupMembershipCache = []; } /** @@ -2059,6 +2060,7 @@ public function getMembershipType(): ?string public function clearGroups():void{ $this->groups->clear(); + $this->groupMembershipCache = []; } /** * @param Group $group @@ -2067,6 +2069,9 @@ public function add2Group(Group $group) { if ($this->groups->contains($group)) return; $this->groups->add($group); + // belongsToGroup() memoizes per instance against the DB; a mutation makes + // that memo stale (the fresh row/removal becomes visible after flush). + $this->groupMembershipCache = []; } public function removeFromGroup(Group $group) @@ -2074,6 +2079,7 @@ public function removeFromGroup(Group $group) if (!$this->groups->contains($group)) return; $this->groups->removeElement($group); //$group->removeMember($this); + $this->groupMembershipCache = []; } /** diff --git a/app/Models/OAuth2/ResourceServerContext.php b/app/Models/OAuth2/ResourceServerContext.php index 491d30f8c..dd876d585 100644 --- a/app/Models/OAuth2/ResourceServerContext.php +++ b/app/Models/OAuth2/ResourceServerContext.php @@ -423,6 +423,13 @@ private function syncMemberFields(Member $member, ?string $user_email, ?string $ $member_by_email->getId() )); $member_by_email->setEmail(sprintf("%s-invalid@invalid", $member_by_email->getId())); + // Member.Email is unique: flush the invalidation NOW, inside the still-open + // transaction, so the resolved member's own email UPDATE below can never be + // ordered first by the UnitOfWork and hit the unique index while the former + // owner still holds the email. Rolled back with the transaction if anything + // later fails. Same flush-now idiom as MemberService::registerExternalUser's + // twin guard (add($entity, true)). + $this->member_repository->add($member_by_email, true); } $member->setEmail($user_email); } diff --git a/app/Services/Model/Imp/SummitOrderService.php b/app/Services/Model/Imp/SummitOrderService.php index 2a1d27b64..d2a54aa7b 100644 --- a/app/Services/Model/Imp/SummitOrderService.php +++ b/app/Services/Model/Imp/SummitOrderService.php @@ -41,6 +41,8 @@ use App\Services\Model\Strategies\TicketFinder\ITicketFinderStrategyFactory; use App\Services\Utils\CSVReader; use App\Services\Utils\ILockManagerService; +use Exception; +use services\utils\AmbiguousCommitException; use Illuminate\Http\UploadedFile; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Config; @@ -231,7 +233,7 @@ public function __construct( $this->tx_service = $tx_service; } - public function build(Member $owner, Summit $summit, array $payload): Saga + public function build(?Member $owner, Summit $summit, array $payload): Saga { Log::debug(sprintf("SagaFactory::build - summit id %s payload %s", $summit->getId(), json_encode($payload))); @@ -254,7 +256,7 @@ public function build(Member $owner, Summit $summit, array $payload): Saga return $this->buildRegularSaga($owner, $summit, $payload); } - private function buildPrePaidSaga(Member $owner, Summit $summit, array $payload): Saga + private function buildPrePaidSaga(?Member $owner, Summit $summit, array $payload): Saga { Log::debug(sprintf("SagaFactory::buildPrePaidSaga - summit id %s", $summit->getId())); return Saga::start() @@ -272,7 +274,7 @@ private function buildPrePaidSaga(Member $owner, Summit $summit, array $payload) )); } - private function buildRegularSaga(Member $owner, Summit $summit, array $payload): Saga + private function buildRegularSaga(?Member $owner, Summit $summit, array $payload): Saga { Log::debug(sprintf("SagaFactory::buildRegularSaga - summit id %s", $summit->getId())); return Saga::start() @@ -442,7 +444,8 @@ public function run(array $formerState): array } // auto assign should only happen when the user has not paid any order and the order has more than one ticket.... - $shouldAutoAssignFirstTicket = !$this->owner->hasPaidRegistrationOrderForSummit($this->summit) && count($tickets) > 1; + // a guest buyer (null owner, public api) trivially has no paid orders for this summit + $shouldAutoAssignFirstTicket = (is_null($this->owner) || !$this->owner->hasPaidRegistrationOrderForSummit($this->summit)) && count($tickets) > 1; // try to get invitation $invitation = $this->summit->getSummitRegistrationInvitationByEmail($owner_email); @@ -529,10 +532,11 @@ public function run(array $formerState): array ) ); - $attendee_first_name = $this->owner->getFirstName(); - $attendee_last_name = $this->owner->getLastName(); - $attendee_email = $this->owner->getEmail(); - $attendee_company = $this->owner->getCompany(); + // guest buyer (null owner): fall back to the payload's owner data + $attendee_first_name = !is_null($this->owner) ? $this->owner->getFirstName() : $owner_first_name; + $attendee_last_name = !is_null($this->owner) ? $this->owner->getLastName() : $owner_last_name; + $attendee_email = !is_null($this->owner) ? $this->owner->getEmail() : $owner_email; + $attendee_company = !is_null($this->owner) ? $this->owner->getCompany() : $owner_company_name; } else { // use what we have on payload @@ -590,7 +594,8 @@ public function run(array $formerState): array $attendee = $local_attendees[$attendee_email]; } - $attendee_owner = $this->owner->getEmail() === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email); + // $attendee_email was lowercased above - normalize the owner side too + $attendee_owner = !is_null($this->owner) && strtolower($this->owner->getEmail()) === $attendee_email ? $this->owner : $this->member_repository->getByEmail($attendee_email); if (is_null($attendee)) { @@ -4333,11 +4338,10 @@ public function processTicketData(int $summit_id, string $filename) $csv_data = $this->download_strategy->get($path); - $summit = $this->tx_service->transaction(function () use ($summit_id) { + $this->tx_service->transaction(function () use ($summit_id) { $summit = $this->summit_repository->getById($summit_id); if (is_null($summit) || !$summit instanceof Summit) throw new EntityNotFoundException(sprintf("summit %s does not exists.", $summit_id)); - return $summit; }); $reader = CSVReader::buildFrom($csv_data); @@ -4349,319 +4353,352 @@ public function processTicketData(int $summit_id, string $filename) $badge_data_present = $reader->hasColumn("badge_type_id") || $reader->hasColumn("badge_type_name"); - foreach ($reader as $idx => $row) { - - $this->tx_service->transaction(function () use ($summit, $reader, $row, $ticket_data_present, $attendee_data_present, $badge_data_present) { + $rows_with_unknown_outcome = []; - Log::debug(sprintf("SummitOrderService::processTicketData processing row %s", json_encode($row))); - - $ticket = null; - $attendee = null; - // process ticket data (try to get an existent ticket) - if ($ticket_data_present) { - Log::debug("SummitOrderService::processTicketData - has ticket data present ... trying to get ticket"); - - // edit already existent ticket ( could be assigned or not) - if ($reader->hasColumn("number")) { - Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket by number %s", $row['number'])); - $ticket = $this->ticket_repository->getByNumberExclusiveLock($row['number']); - } + foreach ($reader as $idx => $row) { - if (is_null($ticket) && $reader->hasColumn("id")) { - Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket by id %s", $row['id'])); - $ticket = $this->ticket_repository->getByIdExclusiveLock(intval($row['id'])); - } + try { + $this->tx_service->transaction(function () use ($summit_id, $reader, $row, $ticket_data_present, $attendee_data_present, $badge_data_present) { + + // re-fetched per row: a prior row's failed transaction clears the entity + // manager's unit of work, detaching any entity (including $summit) captured + // before the loop started - a later row must not operate on a stale reference + $summit = $this->summit_repository->getById($summit_id); + if (is_null($summit) || !$summit instanceof Summit) + throw new EntityNotFoundException(sprintf("summit %s does not exists.", $summit_id)); + + Log::debug(sprintf("SummitOrderService::processTicketData processing row %s", json_encode($row))); + + $ticket = null; + $attendee = null; + // process ticket data (try to get an existent ticket) + if ($ticket_data_present) { + Log::debug("SummitOrderService::processTicketData - has ticket data present ... trying to get ticket"); + + // edit already existent ticket ( could be assigned or not) + if ($reader->hasColumn("number")) { + Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket by number %s", $row['number'])); + $ticket = $this->ticket_repository->getByNumberExclusiveLock($row['number']); + } - if (!is_null($ticket) && !$ticket->isPaid()) { - Log::warning - ( - sprintf - ( - "SummitOrderService::processTicketData - ticket %s is not paid", - $ticket->getId(), - ) - ); - return; - } + if (is_null($ticket) && $reader->hasColumn("id")) { + Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket by id %s", $row['id'])); + $ticket = $this->ticket_repository->getByIdExclusiveLock(intval($row['id'])); + } - if (!is_null($ticket) && !$ticket->isActive()) { - Log::warning - ( - sprintf + if (!is_null($ticket) && !$ticket->isPaid()) { + Log::warning ( - "SummitOrderService::processTicketData - ticket %s is not active", - $ticket->getId() - ) - ); - return; - } + sprintf + ( + "SummitOrderService::processTicketData - ticket %s is not paid", + $ticket->getId(), + ) + ); + return; + } - if (!is_null($ticket) && !$ticket->hasOrder()) { - Log::warning - ( - sprintf + if (!is_null($ticket) && !$ticket->isActive()) { + Log::warning ( - "SummitOrderService::processTicketData - ticket %s does not belongs to an order.", - $ticket->getId() - ) - ); - return; - } + sprintf + ( + "SummitOrderService::processTicketData - ticket %s is not active", + $ticket->getId() + ) + ); + return; + } - if (!is_null($ticket) && $ticket->getOrder()->getSummitId() != $summit->getId()) { - Log::warning - ( - sprintf + if (!is_null($ticket) && !$ticket->hasOrder()) { + Log::warning ( - "SummitOrderService::processTicketData - ticket %s does not belongs to summit %s", - $ticket->getId(), - $summit->getId() - ) - ); - return; - } - } - // process attendee data ( try to get an existent attendee or create a new one) - if ($attendee_data_present) { - $attendee_email = $row['attendee_email'] ?? ''; - Log::debug(sprintf("SummitOrderService::processTicketData - has attendee data present ... trying to get attendee %s", $attendee_email)); - // check if attendee exists - $attendee = $this->attendee_repository->getBySummitAndEmail($summit, $attendee_email); - $member = $this->member_repository->getByEmail($attendee_email); - - if (is_null($attendee) && !empty($attendee_email)) { - - Log::debug(sprintf("SummitOrderService::processTicketData - attendee %s does not exists", $attendee_email)); - // create attendee ( populate payload) - $payload = [ - 'email' => $attendee_email, - 'first_name' => $row['attendee_first_name'], - 'last_name' => $row['attendee_last_name'], - ]; - - if ($reader->hasColumn('attendee_company')) { - $payload['company'] = $row['attendee_company']; + sprintf + ( + "SummitOrderService::processTicketData - ticket %s does not belongs to an order.", + $ticket->getId() + ) + ); + return; } - if ($reader->hasColumn('attendee_company_id')) { - $payload['company_id'] = intval($row['attendee_company_id']); + if (!is_null($ticket) && $ticket->getOrder()->getSummitId() != $summit->getId()) { + Log::warning + ( + sprintf + ( + "SummitOrderService::processTicketData - ticket %s does not belongs to summit %s", + $ticket->getId(), + $summit->getId() + ) + ); + return; } + } + // process attendee data ( try to get an existent attendee or create a new one) + if ($attendee_data_present) { + $attendee_email = $row['attendee_email'] ?? ''; + Log::debug(sprintf("SummitOrderService::processTicketData - has attendee data present ... trying to get attendee %s", $attendee_email)); + // check if attendee exists + $attendee = $this->attendee_repository->getBySummitAndEmail($summit, $attendee_email); + $member = $this->member_repository->getByEmail($attendee_email); + + if (is_null($attendee) && !empty($attendee_email)) { + + Log::debug(sprintf("SummitOrderService::processTicketData - attendee %s does not exists", $attendee_email)); + // create attendee ( populate payload) + $payload = [ + 'email' => $attendee_email, + 'first_name' => $row['attendee_first_name'], + 'last_name' => $row['attendee_last_name'], + ]; + + if ($reader->hasColumn('attendee_company')) { + $payload['company'] = $row['attendee_company']; + } - Log::debug(sprintf("SummitOrderService::processTicketData creating attendee with payload %s", json_encode($payload))); - - $attendee = SummitAttendeeFactory::build($summit, $payload, $member); + if ($reader->hasColumn('attendee_company_id')) { + $payload['company_id'] = intval($row['attendee_company_id']); + } - if ($reader->hasColumn('attendee_tags')) { - $tags = explode('|', $row['attendee_tags']); - $attendee->clearTags(); - foreach ($tags as $tag_val) { - $tag_val = trim($tag_val); - $tag = $this->tags_repository->getByTag($tag_val); - if (is_null($tag)) { - // create tag - $tag = new Tag($tag_val); - $this->tags_repository->add($tag); + Log::debug(sprintf("SummitOrderService::processTicketData creating attendee with payload %s", json_encode($payload))); + + $attendee = SummitAttendeeFactory::build($summit, $payload, $member); + + if ($reader->hasColumn('attendee_tags')) { + $tags = explode('|', $row['attendee_tags']); + $attendee->clearTags(); + foreach ($tags as $tag_val) { + $tag_val = trim($tag_val); + $tag = $this->tags_repository->getByTag($tag_val); + if (is_null($tag)) { + // create tag + $tag = new Tag($tag_val); + $this->tags_repository->add($tag); + } + $attendee->addTag($tag); } - $attendee->addTag($tag); } + $this->attendee_repository->add($attendee); } - $this->attendee_repository->add($attendee); } - } - if (!is_null($attendee)) { - if (is_null($ticket)) { - Log::debug(sprintf("SummitOrderService::processTicketData ticket is null, trying to create a new one")); + if (!is_null($attendee)) { + if (is_null($ticket)) { + Log::debug(sprintf("SummitOrderService::processTicketData ticket is null, trying to create a new one")); - // create ticket - // first try to get ticket type - $ticket_type = null; - $promo_code = null; + // create ticket + // first try to get ticket type + $ticket_type = null; + $promo_code = null; - if ($reader->hasColumn('ticket_type_name')) { - // use summit context - Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket type by name %s", $row['ticket_type_name'])); - $ticket_type = $this->ticket_type_repository->getByType($summit, $row['ticket_type_name']); - } + if ($reader->hasColumn('ticket_type_name')) { + // use summit context + Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket type by name %s", $row['ticket_type_name'])); + $ticket_type = $this->ticket_type_repository->getByType($summit, $row['ticket_type_name']); + } - if ($reader->hasColumn('ticket_promo_code')) { - // use summit context - Log::debug(sprintf("SummitOrderService::processTicketData trying to get promo code by code %s", $row['ticket_promo_code'])); - $promo_code = $this->promo_code_repository->getBySummitAndCode($summit, $row['ticket_promo_code']); - } + if ($reader->hasColumn('ticket_promo_code')) { + // use summit context + Log::debug(sprintf("SummitOrderService::processTicketData trying to get promo code by code %s", $row['ticket_promo_code'])); + $promo_code = $this->promo_code_repository->getBySummitAndCode($summit, $row['ticket_promo_code']); + } - if ($reader->hasColumn('promo_code_id')) { - Log::debug(sprintf("SummitOrderService::processTicketData trying to get promo code by id %s", $row['promo_code_id'])); - $promo_code = $this->promo_code_repository->getById(intval($row['promo_code_id'])); - if ($promo_code instanceof SummitRegistrationPromoCode && $promo_code->getSummitId() != $summit->getId()) { - Log::debug - ( - sprintf + if ($reader->hasColumn('promo_code_id')) { + Log::debug(sprintf("SummitOrderService::processTicketData trying to get promo code by id %s", $row['promo_code_id'])); + $promo_code = $this->promo_code_repository->getById(intval($row['promo_code_id'])); + if ($promo_code instanceof SummitRegistrationPromoCode && $promo_code->getSummitId() != $summit->getId()) { + Log::debug ( - "promo code %s does not belong to summit %s", - $promo_code->getId(), - $summit->getId() - ) - ); - return; + sprintf + ( + "promo code %s does not belong to summit %s", + $promo_code->getId(), + $summit->getId() + ) + ); + return; + } } - } - if (is_null($promo_code) && $reader->hasColumn('promo_code')) { - // use summit context - Log::debug(sprintf("SummitOrderService::processTicketData trying to get promo code by code %s", $row['promo_code'])); - $promo_code = $this->promo_code_repository->getBySummitAndCode($summit, $row['promo_code']); - } + if (is_null($promo_code) && $reader->hasColumn('promo_code')) { + // use summit context + Log::debug(sprintf("SummitOrderService::processTicketData trying to get promo code by code %s", $row['promo_code'])); + $promo_code = $this->promo_code_repository->getBySummitAndCode($summit, $row['promo_code']); + } - if (is_null($ticket_type) && $reader->hasColumn('ticket_type_id')) { - Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket type by id %s", $row['ticket_type_id'])); - $ticket_type = $this->ticket_type_repository->getById(intval($row['ticket_type_id'])); - if ($ticket_type instanceof SummitTicketType && $ticket_type->getSummitId() != $summit->getId()) { - Log::debug( - sprintf - ( - "ticket type %s does not belong to summit %s", - $ticket_type->getId(), - $summit->getId() - ) + if (is_null($ticket_type) && $reader->hasColumn('ticket_type_id')) { + Log::debug(sprintf("SummitOrderService::processTicketData trying to get ticket type by id %s", $row['ticket_type_id'])); + $ticket_type = $this->ticket_type_repository->getById(intval($row['ticket_type_id'])); + if ($ticket_type instanceof SummitTicketType && $ticket_type->getSummitId() != $summit->getId()) { + Log::debug( + sprintf + ( + "ticket type %s does not belong to summit %s", + $ticket_type->getId(), + $summit->getId() + ) + ); + return; + } + } + + if (is_null($ticket_type)) { + Log::debug + ( + "SummitOrderService::processTicketData - ticket type is not provide, ticket can not be created for attendee" ); return; } - } - if (is_null($ticket_type)) { - Log::debug - ( - "SummitOrderService::processTicketData - ticket type is not provide, ticket can not be created for attendee" - ); - return; - } - - $order_payload = [ - 'ticket_type_id' => $ticket_type->getId(), - 'attendee' => $attendee, - 'owner_email' => $attendee->getEmail(), - 'owner_first_name' => $attendee->getFirstName(), - 'owner_last_name' => $attendee->getSurname(), - 'owner_company' => $attendee->getCompanyName(), - ]; - - if (!is_null($promo_code)) { - Log::debug(sprintf("SummitOrderService::processTicketData adding promo code by code %s to offline order", $promo_code->getId())); - $order_payload['promo_code'] = $promo_code->getCode(); - } + $order_payload = [ + 'ticket_type_id' => $ticket_type->getId(), + 'attendee' => $attendee, + 'owner_email' => $attendee->getEmail(), + 'owner_first_name' => $attendee->getFirstName(), + 'owner_last_name' => $attendee->getSurname(), + 'owner_company' => $attendee->getCompanyName(), + ]; + + if (!is_null($promo_code)) { + Log::debug(sprintf("SummitOrderService::processTicketData adding promo code by code %s to offline order", $promo_code->getId())); + $order_payload['promo_code'] = $promo_code->getCode(); + } - $order = $this->createOfflineOrder($summit, $order_payload); + $order = $this->createOfflineOrder($summit, $order_payload); - $ticket = $order->getFirstTicket(); + $ticket = $order->getFirstTicket(); - } else { - // ticket exists try to re assign it - Log::debug(sprintf("SummitOrderService::processTicketData ticket exists. trying to re assign it ...")); + } else { + // ticket exists try to re assign it + Log::debug(sprintf("SummitOrderService::processTicketData ticket exists. trying to re assign it ...")); - if ($ticket->hasOwner() && $ticket->getOwnerEmail() != $attendee->getEmail()) { - Log::debug(sprintf("SummitOrderService::processTicketData - reasigning ticket to attendee %s", $attendee->getEmail())); - $ticket->getOwner()->sendRevocationTicketEmail($ticket); + if ($ticket->hasOwner() && $ticket->getOwnerEmail() != $attendee->getEmail()) { + Log::debug(sprintf("SummitOrderService::processTicketData - reasigning ticket to attendee %s", $attendee->getEmail())); + $ticket->getOwner()->sendRevocationTicketEmail($ticket); - $ticket->getOwner()->removeTicket($ticket); - } + $ticket->getOwner()->removeTicket($ticket); + } - Log::debug(sprintf("SummitOrderService::processTicketData assigning ticket %s to attendee %s", $ticket->getNumber(), $attendee->getEmail())); + Log::debug(sprintf("SummitOrderService::processTicketData assigning ticket %s to attendee %s", $ticket->getNumber(), $attendee->getEmail())); - $attendee->addTicket($ticket); + $attendee->addTicket($ticket); - $ticket->generateQRCode(); - $ticket->generateHash(); + $ticket->generateQRCode(); + $ticket->generateHash(); - if ($summit->isRegistrationSendTicketEmailAutomatically()) { - Log::debug(sprintf("SummitOrderService::processTicketData sending invitation email to attendee %s", $attendee->getEmail())); - $attendee->sendInvitationEmail($ticket); + if ($summit->isRegistrationSendTicketEmailAutomatically()) { + Log::debug(sprintf("SummitOrderService::processTicketData sending invitation email to attendee %s", $attendee->getEmail())); + $attendee->sendInvitationEmail($ticket); + } } } - } - - if (is_null($ticket)) { - Log::warning("SummitOrderService::processTicketData ticket is null stop current row processing."); - return; - } - - Log::debug(sprintf("SummitOrderService::processTicketData - got ticket %s (%s)", $ticket->getId(), $ticket->getNumber())); - - // badge data is processed BEFORE extra questions: the extra-question permission - // gates ( SummitAttendee::isAllowedQuestion ) read the attendee badge features - // through native SQL, so this row's own badge/feature grants must be applied - // ( and flushed, see upsertAttendeeExtraQuestionAnswers ) first - if ($badge_data_present) { - - $badge_type = null; - if ($reader->hasColumn("badge_type_id")) { - Log::debug(sprintf("SummitOrderService::processTicketData trying to get badge type by id %s", $row['badge_type_id'])); - $badge_type = $summit->getBadgeTypeById(intval($row['badge_type_id'])); + if (is_null($ticket)) { + Log::warning("SummitOrderService::processTicketData ticket is null stop current row processing."); + return; } - if (is_null($badge_type) && $reader->hasColumn("badge_type_name")) { - Log::debug(sprintf("SummitOrderService::processTicketData trying to get badge type by name %s", $row['badge_type_name'])); - $badge_type = $summit->getBadgeTypeByName(trim($row['badge_type_name'])); - } + Log::debug(sprintf("SummitOrderService::processTicketData - got ticket %s (%s)", $ticket->getId(), $ticket->getNumber())); - if (!is_null($badge_type)) - Log::debug(sprintf("SummitOrderService::processTicketData - got badge type %s (%s)", $badge_type->getId(), $badge_type->getName())); + // badge data is processed BEFORE extra questions: the extra-question permission + // gates ( SummitAttendee::isAllowedQuestion ) read the attendee badge features + // through native SQL, so this row's own badge/feature grants must be applied + // ( and flushed, see upsertAttendeeExtraQuestionAnswers ) first + if ($badge_data_present) { - if (!$ticket->hasBadge() && is_null($badge_type)) { - Log::warning("SummitOrderService::processTicketData ticket has no badge and badge type is null, skipping badge processing."); - } else { + $badge_type = null; - if (!$ticket->hasBadge()) { - // create it - Log::debug(sprintf("SummitOrderService::processTicketData - ticket %s (%s) has not badge ... creating it", $ticket->getId(), $ticket->getNumber())); - $badge = SummitBadgeType::buildBadgeFromType($badge_type); - $ticket->setBadge($badge); + if ($reader->hasColumn("badge_type_id")) { + Log::debug(sprintf("SummitOrderService::processTicketData trying to get badge type by id %s", $row['badge_type_id'])); + $badge_type = $summit->getBadgeTypeById(intval($row['badge_type_id'])); } - $badge = $ticket->getBadge(); + if (is_null($badge_type) && $reader->hasColumn("badge_type_name")) { + Log::debug(sprintf("SummitOrderService::processTicketData trying to get badge type by name %s", $row['badge_type_name'])); + $badge_type = $summit->getBadgeTypeByName(trim($row['badge_type_name'])); + } if (!is_null($badge_type)) - $badge->setType($badge_type); - - $clearedFeatures = false; - // check if we are setting any badge feature - Log::debug("SummitOrderService::processTicketData processing badge type features"); - foreach ($summit->getBadgeFeaturesTypes() as $featuresType) { - $feature_name = $featuresType->getName(); - Log::debug(sprintf("SummitOrderService::processTicketData processing badge type feature %s for ticket %s", $feature_name, $ticket->getId())); - if (!$reader->hasColumn($feature_name)) { - Log::debug(sprintf("SummitOrderService::processTicketData badge type feature %s does not exists as column", $feature_name)); - continue; - } + Log::debug(sprintf("SummitOrderService::processTicketData - got badge type %s (%s)", $badge_type->getId(), $badge_type->getName())); - if (!$clearedFeatures) { - $badge->clearFeatures(); - $clearedFeatures = true; - } + if (!$ticket->hasBadge() && is_null($badge_type)) { + Log::warning("SummitOrderService::processTicketData ticket has no badge and badge type is null, skipping badge processing."); + } else { - $mustAdd = intval($row[$feature_name]) === 1; - if (!$mustAdd) { - Log::debug(sprintf("SummitOrderService::processTicketData badge type feature %s not set for ticket %s", $feature_name, $ticket->getId())); - continue; + if (!$ticket->hasBadge()) { + // create it + Log::debug(sprintf("SummitOrderService::processTicketData - ticket %s (%s) has not badge ... creating it", $ticket->getId(), $ticket->getNumber())); + $badge = SummitBadgeType::buildBadgeFromType($badge_type); + $ticket->setBadge($badge); } - Log::debug(sprintf("SummitOrderService::processTicketData - ticket %s (%s) - trying to add new features to ticket badge (%s)", $ticket->getId(), $ticket->getNumber(), $feature_name)); - $feature = $summit->getFeatureTypeByName(trim($feature_name)); - if (is_null($feature)) { - Log::warning(sprintf("SummitOrderService::processTicketData feature %s does not exist on summit %s", $feature, $summit->getId())); - continue; + + $badge = $ticket->getBadge(); + + if (!is_null($badge_type)) + $badge->setType($badge_type); + + $clearedFeatures = false; + // check if we are setting any badge feature + Log::debug("SummitOrderService::processTicketData processing badge type features"); + foreach ($summit->getBadgeFeaturesTypes() as $featuresType) { + $feature_name = $featuresType->getName(); + Log::debug(sprintf("SummitOrderService::processTicketData processing badge type feature %s for ticket %s", $feature_name, $ticket->getId())); + if (!$reader->hasColumn($feature_name)) { + Log::debug(sprintf("SummitOrderService::processTicketData badge type feature %s does not exists as column", $feature_name)); + continue; + } + + if (!$clearedFeatures) { + $badge->clearFeatures(); + $clearedFeatures = true; + } + + $mustAdd = intval($row[$feature_name]) === 1; + if (!$mustAdd) { + Log::debug(sprintf("SummitOrderService::processTicketData badge type feature %s not set for ticket %s", $feature_name, $ticket->getId())); + continue; + } + Log::debug(sprintf("SummitOrderService::processTicketData - ticket %s (%s) - trying to add new features to ticket badge (%s)", $ticket->getId(), $ticket->getNumber(), $feature_name)); + $feature = $summit->getFeatureTypeByName(trim($feature_name)); + if (is_null($feature)) { + Log::warning(sprintf("SummitOrderService::processTicketData feature %s does not exist on summit %s", $feature, $summit->getId())); + continue; + } + Log::debug(sprintf("SummitOrderService::processTicketData badge type feature %s set for ticket %s", $feature_name, $ticket->getId())); + $badge->addFeature($feature); } - Log::debug(sprintf("SummitOrderService::processTicketData badge type feature %s set for ticket %s", $feature_name, $ticket->getId())); - $badge->addFeature($feature); } } - } - // extra questions ( extra_question:{question name} columns ) - $answers_owner = !is_null($attendee) ? $attendee : ($ticket->hasOwner() ? $ticket->getOwner() : null); - if (!is_null($answers_owner)) - $this->upsertAttendeeExtraQuestionAnswers($summit, $answers_owner, $row); - }); + // extra questions ( extra_question:{question name} columns ) + $answers_owner = !is_null($attendee) ? $attendee : ($ticket->hasOwner() ? $ticket->getOwner() : null); + if (!is_null($answers_owner)) + $this->upsertAttendeeExtraQuestionAnswers($summit, $answers_owner, $row); + }); + } catch (AmbiguousCommitException $ex) { + // The row's COMMIT outcome is unknown - it may or may not be durable + // (see DoctrineTransactionService) - so it can be treated as neither + // processed nor failed. Record it and keep going: remaining rows are + // independent. + $rows_with_unknown_outcome[] = $idx; + Log::error(sprintf("SummitOrderService::processTicketData row %s commit outcome unknown.", $idx)); + Log::error($ex); + } catch (Exception $ex) { + Log::warning($ex); + } + } + + if (count($rows_with_unknown_outcome) > 0) { + // Reconciliation required: keep the source file so the unknown-outcome rows + // can be checked against the DB - deleting it here would record an import + // with unverifiable rows as cleanly processed. + Log::error(sprintf( + "SummitOrderService::processTicketData file %s NOT deleted: row(s) %s have unknown commit outcome; reconcile them against the DB.", + $path, + implode(',', $rows_with_unknown_outcome) + )); + return; } Log::debug(sprintf("SummitOrderService::processTicketData deleting file %s from storage %s", $path, $this->download_strategy->getDriver())); diff --git a/app/Services/Model/Imp/SummitService.php b/app/Services/Model/Imp/SummitService.php index b19b6ec08..d789f3ca7 100644 --- a/app/Services/Model/Imp/SummitService.php +++ b/app/Services/Model/Imp/SummitService.php @@ -112,6 +112,7 @@ use models\summit\SummitMediaUploadType; use models\summit\SummitScheduleEmptySpot; use services\apis\IEventbriteAPI; +use services\utils\AmbiguousCommitException; use utils\Filter; use utils\FilterElement; use utils\FilterParser; @@ -2859,11 +2860,10 @@ public function processEventData(int $summit_id, string $filename, bool $send_sp throw new ValidationException(sprintf("file %s does not exists.", $filename)); } - $summit = $this->tx_service->transaction(function () use ($summit_id) { + $this->tx_service->transaction(function () use ($summit_id) { $summit = $this->summit_repository->getById($summit_id); if (is_null($summit) || !$summit instanceof Summit) throw new EntityNotFoundException(sprintf("summit %s does not exists.", $summit_id)); - return $summit; }); $csv = Reader::createFromString($csv_data); @@ -2872,12 +2872,21 @@ public function processEventData(int $summit_id, string $filename, bool $send_sp $header = $csv->getHeader(); //returns the CSV header record $records = $csv->getRecords(); + $rows_with_unknown_outcome = []; + foreach ($records as $idx => $row) { try { // variable to store only added speakers to event $new_speakers = []; - $event = $this->tx_service->transaction(function () use ($summit, $row, &$new_speakers) { + $event = $this->tx_service->transaction(function () use ($summit_id, $row, &$new_speakers) { + + // re-fetched per row: a prior row's failed transaction clears the entity + // manager's unit of work, detaching any entity (including $summit) captured + // before the loop started - a later row must not operate on a stale reference + $summit = $this->summit_repository->getById($summit_id); + if (is_null($summit) || !$summit instanceof Summit) + throw new EntityNotFoundException(sprintf("summit %s does not exists.", $summit_id)); Log::debug(sprintf("SummitService::processEventData processing row %s", json_encode($row))); @@ -3229,11 +3238,31 @@ public function processEventData(int $summit_id, string $filename, bool $send_sp ImportEventSpeakerEmail::dispatch($event, $speaker, $setPasswordLink); }); } + } catch (AmbiguousCommitException $ex) { + // The row's COMMIT outcome is unknown - it may or may not be durable + // (see DoctrineTransactionService) - so it can be treated as neither + // processed nor failed. Record it and keep going: remaining rows are + // independent. Mirrors SummitOrderService::processTicketData. + $rows_with_unknown_outcome[] = $idx; + Log::error(sprintf("SummitService::processEventData row %s commit outcome unknown.", $idx)); + Log::error($ex); } catch (Exception $ex) { Log::warning($ex); } } + if (count($rows_with_unknown_outcome) > 0) { + // Reconciliation required: keep the source file so the unknown-outcome rows + // can be checked against the DB - deleting it here would record an import + // with unverifiable rows as cleanly processed. + Log::error(sprintf( + "SummitService::processEventData file %s NOT deleted: row(s) %s have unknown commit outcome; reconcile them against the DB.", + $path, + implode(',', $rows_with_unknown_outcome) + )); + return; + } + Log::debug(sprintf("SummitService::processEventData deleting file %s from cloud storage %s", $path, $this->download_strategy->getDriver())); $this->download_strategy->delete($path); } @@ -3605,6 +3634,8 @@ public function processRegistrationCompaniesData(int $summit_id, string $filenam $header = $csv->getHeader(); //returns the CSV header record $records = $csv->getRecords(); + $rows_with_unknown_outcome = []; + foreach ($records as $idx => $row) { try { $this->tx_service->transaction(function () use ($row, $summit_id) { @@ -3640,11 +3671,31 @@ public function processRegistrationCompaniesData(int $summit_id, string $filenam $this->addCompany($summit_id, $company->getId()); }); + } catch (AmbiguousCommitException $ex) { + // The row's COMMIT outcome is unknown - it may or may not be durable + // (see DoctrineTransactionService) - so it can be treated as neither + // processed nor failed. Record it and keep going: remaining rows are + // independent. Mirrors SummitOrderService::processTicketData. + $rows_with_unknown_outcome[] = $idx; + Log::error(sprintf("SummitService::processRegistrationCompaniesData row %s commit outcome unknown.", $idx)); + Log::error($ex); } catch (Exception $ex) { Log::warning($ex); } } + if (count($rows_with_unknown_outcome) > 0) { + // Reconciliation required: keep the source file so the unknown-outcome rows + // can be checked against the DB - deleting it here would record an import + // with unverifiable rows as cleanly processed. + Log::error(sprintf( + "SummitService::processRegistrationCompaniesData file %s NOT deleted: row(s) %s have unknown commit outcome; reconcile them against the DB.", + $path, + implode(',', $rows_with_unknown_outcome) + )); + return; + } + Log::debug(sprintf("SummitService::processRegistrationCompaniesData deleting file %s from cloud storage %s", $path, $this->download_strategy->getDriver())); $this->download_strategy->delete($path); } diff --git a/app/Services/Utils/AmbiguousCommitException.php b/app/Services/Utils/AmbiguousCommitException.php new file mode 100644 index 000000000..d32181d12 --- /dev/null +++ b/app/Services/Utils/AmbiguousCommitException.php @@ -0,0 +1,33 @@ +fail($e)). The original driver exception is + * preserved as getPrevious(). + * + * Extends plain \RuntimeException on purpose: shouldReconnect() never matches + * it, so it can never re-enter any retry path in this service. + */ +final class AmbiguousCommitException extends \RuntimeException +{ +} diff --git a/app/Services/Utils/DoctrineTransactionService.php b/app/Services/Utils/DoctrineTransactionService.php index 0c30fb96c..b7b227abc 100644 --- a/app/Services/Utils/DoctrineTransactionService.php +++ b/app/Services/Utils/DoctrineTransactionService.php @@ -22,9 +22,58 @@ use Exception; use libs\utils\ITransactionService; use ErrorException; + /** * Class DoctrineTransactionService * @package App\Services\Utils + * + * Root-aware transaction wrapper. + * + * Root (outermost) transaction: + * - owns the connection lifecycle (may retry on transient errors) + * - sets isolation level + * - flushes the EntityManager and issues the real COMMIT + * - on failure, rolls back and clears the EntityManager so no + * UnitOfWork state leaks into subsequent transactions + * - never retries once the real COMMIT has been attempted: a connection + * failure during COMMIT is ambiguous (the server may have already made + * the transaction durable), so re-executing the callback could duplicate + * every write and side effect; it surfaces as AmbiguousCommitException + * (driver exception preserved as previous) so callers - queue jobs + * especially - can fail without retrying instead of blind-retrying a + * retryable-looking driver exception + * - when the rollback itself fails (or a commit-phase failure is + * connection-level), the manager/connection pair is discarded and a + * fresh manager is reset into the registry, so direct Registry + * consumers (which have no reconnect path of their own) never keep + * working against a dead handle + * + * Nested transaction (called while a DB transaction is already active): + * - uses Doctrine DBAL nesting counter (beginTransaction / commit) + * - flushes the EntityManager so auto-generated IDs are available + * - does NOT retry, reset the EntityManager, or close the connection + * - on error, rolls back only the nested level and re-throws, + * letting the root transaction decide how to handle the failure + * + * Savepoints are never enabled. Nested transactions are pure DBAL nesting- + * counter bookkeeping (beginTransaction / commit / rollBack with no SQL at + * nesting level > 1) - this is deliberate, not an oversight: Doctrine's + * UnitOfWork is not savepoint-aware, so a savepoint rollback can silently + * desynchronize the ORM's belief about what is persisted from what the + * database actually holds. Without savepoints, DBAL's own isRollbackOnly + * flag does the job instead: a nested rollBack() marks the shared connection + * rollback-only (no SQL), and commit() at ANY level - nested, root, or even + * Doctrine ORM's own internal per-flush() commit - checks that flag first + * and fails immediately. A nested failure can therefore never be silently + * absorbed into a successful root commit, even if an intermediate callback + * catches it and continues; the whole business operation succeeds or fails + * as one atomic unit. + * + * The root path re-checks that flag itself right before issuing the real + * COMMIT: a rollback-only connection fails fast as a deterministic error + * (DBAL would throw client-side before the COMMIT ever reaches the server), + * so AmbiguousCommitException is reserved for failures of a COMMIT that was + * actually sent to the server. */ final class DoctrineTransactionService implements ITransactionService { @@ -60,48 +109,10 @@ public function shouldReconnect(\Exception $e):bool $e->getMessage() ) ); - if($e instanceof ErrorException && str_contains($e->getMessage(), "Packets out of order")){ - Log::debug - ( - sprintf - ( - "DoctrineTransactionService::shouldReconnect %s Packets out of order true", - get_class($e), - ) - ); - return true; - } - if($e instanceof RetryableException) { - Log::debug - ( - sprintf - ( - "DoctrineTransactionService::shouldReconnect %s true", - get_class($e), - ) - ); - return true; - } - if($e instanceof ConnectionLost) { - Log::debug - ( - sprintf - ( - "DoctrineTransactionService::shouldReconnect %s true", - get_class($e), - ) - ); - return true; - } - if($e instanceof ConnectionException) { - Log::debug - ( - sprintf - ( - "DoctrineTransactionService::shouldReconnect %s true", - get_class($e), - ) - ); + if ($e instanceof RetryableException + || $e instanceof ConnectionLost + || $e instanceof ConnectionException + || ($e instanceof ErrorException && str_contains($e->getMessage(), "Packets out of order"))) { return true; } if($e instanceof \PDOException){ @@ -131,58 +142,332 @@ public function shouldReconnect(\Exception $e):bool * @return mixed|null * @throws Exception */ - public function transaction(Closure $callback, int $isolationLevel = TransactionIsolationLevel::READ_COMMITTED) + public function transaction(Closure $callback, int $isolationLevel = TransactionIsolationLevel::READ_COMMITTED) { - $retry = 0; - $done = false; - $result = null; + $em = Registry::getManager($this->manager_name); + $conn = $em->getConnection(); + + // Both states are read exactly once so the routing decision cannot + // disagree with itself. + $emIsOpen = $em->isOpen(); + + if ($conn->isTransactionActive()) { + if ($emIsOpen) { + return $this->runNestedTransaction($callback); + } + + // A closed EM whose connection still holds an open transaction means a + // nested failure was caught mid-propagation (the outer transaction() + // frame has not unwound yet). Routing this into runNestedTransaction() + // would hand the callback a dead EntityManager; routing it into + // runRootTransaction() would be worse - resetManager() onto a BRAND-NEW + // connection whose commits are durable even though the outer, + // rollback-only transaction on the old connection rolls back: a + // split-brain partial commit that escapes the atomicity guarantee + // documented above. Refuse instead; the original nested failure must + // propagate to the root. + // Plain \RuntimeException intentionally - shouldReconnect() does not + // match it, so this can never trigger the retry loop. + throw new \RuntimeException( + 'DoctrineTransactionService::transaction the EntityManager was closed while its ' + . 'connection still has an active transaction (typically a nested failure that was ' + . 'caught and execution continued); refusing to start an independent root transaction ' + . 'whose writes would survive the outer rollback. Let the original failure propagate ' + . 'to the root transaction instead.' + ); + } + + return $this->runRootTransaction($callback, $isolationLevel); + } + + /** + * A nested/inner flush failure closes the EntityManager (ORM behavior); reaching + * this point after a callback returns normally means it caught that error and + * continued. Fail fast with the real cause instead of letting the flush below + * die with an opaque EntityManagerClosed one or more nesting levels up from + * where it actually happened. + * + * @param \Doctrine\ORM\EntityManagerInterface $em + * @param string $context + * @throws \RuntimeException + */ + private function failFastIfEntityManagerClosed($em, string $context): void + { + if (!$em->isOpen()) { + // Plain \RuntimeException intentionally - shouldReconnect() does not + // match it, so this can never trigger the retry loop. + throw new \RuntimeException(sprintf( + 'DoctrineTransactionService::%s the EntityManager was closed during the callback ' + . '(typically a nested flush failure that was caught and execution continued); this ' + . 'transaction cannot be committed. Catching a failure from a nested transaction() ' + . 'call and continuing is never safe - the nested rollback already marked the ' + . 'connection rollback-only, so a commit at any level fails. Let the failure ' + . 'propagate to the root transaction instead.', + $context + )); + } + } + + /** + * Rolls back the given connection if it still has an active transaction, + * swallowing any rollback failure. Never lets a rollback failure replace the + * callback's original exception: a reconnectable rollback error would + * otherwise misclassify a business failure as retryable and re-execute + * non-idempotent side effects. + * + * @param \Doctrine\DBAL\Connection $conn + * @param \Throwable $cause + * @param string $context + * @return bool true when the connection is clean (rolled back, or nothing to + * roll back); false when the rollback attempt itself failed and + * the connection state is unknown (dead handle, possibly a stuck + * DBAL rollback-only flag - DBAL zeroes the nesting level before + * the physical rollback and only clears the flag after it succeeds) + */ + private function safeRollback($conn, \Throwable $cause, string $context): bool + { + try { + if ($conn->isTransactionActive()) { + $conn->rollBack(); + } + return true; + } catch (\Throwable $rollbackError) { + Log::warning(sprintf( + "DoctrineTransactionService::%s rollback failed after '%s': %s", + $context, + $cause->getMessage(), + $rollbackError->getMessage() + )); + return false; + } + } + + /** + * Best-effort destructive cleanup for a manager/connection pair left in an + * unknown or broken state (failed rollback, connection-level commit failure, + * reconnectable error before retrying): closes the EntityManager, closes the + * physical connection and swaps a fresh manager into the registry so + * subsequent work on this process - including direct Registry consumers + * (repositories, serializers, queue jobs) reading OUTSIDE transaction(), + * which have no reconnect/retry path of their own - gets a live pair instead + * of a dead handle. + * + * Root-only: never call while an outer transaction still owns this + * connection. Never throws - it must not mask the exception that led here. + * + * @param \Doctrine\ORM\EntityManagerInterface $em + */ + private function discardBrokenManager($em): void + { + try { + if ($em->isOpen()) { + $em->clear(); + $em->close(); + } + } catch (\Throwable $ignore) { + } + + try { + $em->getConnection()->close(); + } catch (\Throwable $ignore) { + } + + try { + Registry::resetManager($this->manager_name); + } catch (\Throwable $ignore) { + } + } + + /** + * Post-failure registry hygiene for the root path. When the pair is known + * to be broken (the rollback itself failed, or a commit-phase failure was + * connection-level) it is discarded entirely; otherwise, if the EM was + * closed (ORM behavior on any failed flush), a live manager is reset into + * the registry so direct Registry consumers (serializers, factories, + * workers) don't hit EntityManagerClosed on their next operation. + * + * @param \Doctrine\ORM\EntityManagerInterface $em + * @param bool $pairIsBroken + */ + private function restoreRegistryAfterFailure($em, bool $pairIsBroken): void + { + if ($pairIsBroken) { + $this->discardBrokenManager($em); + return; + } + + if (!$em->isOpen()) { + Registry::resetManager($this->manager_name); + } + } + + /** + * Root (outermost) transaction: may retry on transient connection errors, + * sets isolation level, flushes, and issues the real COMMIT. + * Retries never happen once the real COMMIT has been attempted (ambiguous + * outcome - see the class docblock). + * + * @param Closure $callback + * @param int $isolationLevel + * @return mixed|null + * @throws Exception + */ + private function runRootTransaction(Closure $callback, int $isolationLevel) + { + $retry = 0; + + while ($retry < self::MaxRetries) { + $em = Registry::getManager($this->manager_name); + $commitStarted = false; + $rollbackFailed = false; - while (!$done and $retry < self::MaxRetries) { try { - $em = Registry::getManager($this->manager_name); if (!$em->isOpen()) { - Log::warning("DoctrineTransactionService::transaction: entity manager is closed!, trying to re open..."); + Log::warning("DoctrineTransactionService::runRootTransaction: entity manager is closed, resetting..."); $em = Registry::resetManager($this->manager_name); } - $em->getConnection()->setTransactionIsolation($isolationLevel); - $em->getConnection()->beginTransaction(); // suspend auto-commit - $result = $callback($this); - $em->flush(); - $em->getConnection()->commit(); - $done = true; - } - catch (Exception $ex) { + $conn = $em->getConnection(); + $conn->setTransactionIsolation($isolationLevel); + $conn->beginTransaction(); + + try { + $result = $callback($this); + $this->failFastIfEntityManagerClosed($em, 'runRootTransaction'); + $em->flush(); + // A rollback-only connection can never commit: DBAL throws + // ConnectionException::commitFailedRollbackOnly() client-side, + // BEFORE the COMMIT is ever sent to the server. Reaching this + // point rollback-only means a nested failure was caught and + // execution continued with nothing left to flush (flush() with + // an empty changeset never touches the connection) - a + // deterministic rollback, not an ambiguous commit. Fail with + // the real cause instead of letting the commit below be + // misclassified as ambiguous. + // Plain \RuntimeException intentionally - shouldReconnect() + // does not match it, so this can never trigger the retry loop. + if ($conn->isRollbackOnly()) { + throw new \RuntimeException( + 'DoctrineTransactionService::runRootTransaction the connection is marked ' + . 'rollback-only (a nested transaction rolled back and its failure was ' + . 'caught mid-chain); this transaction was NOT committed and cannot be. ' + . 'Let the original nested failure propagate to the root instead.' + ); + } + // Anything past this point is the real COMMIT: a connection + // failure here is ambiguous (the server may have already made + // the transaction durable and only the ack was lost), so the + // retry path below must never re-execute the callback. + $commitStarted = true; + $conn->commit(); + return $result; + } catch (\Throwable $inner) { + $rollbackFailed = !$this->safeRollback($conn, $inner, 'runRootTransaction'); + // Root only: discard UnitOfWork state so pending persists/changesets + // from the failed callback cannot leak into the next transaction on + // this same EntityManager (phantom writes in catch-and-continue loops). + // A secondary clear() failure must never mask $inner. + // (ORM 3.3's clear() has no isOpen guard - this protects upgrades.) + try { + $em->clear(); + } catch (\Throwable $ignore) { + } + throw $inner; + } + } catch (Exception $ex) { $retry++; - $em->getConnection()->close(); - $em->close(); - if($em->getConnection()->isTransactionActive()) - $em->getConnection()->rollBack(); - Registry::resetManager($this->manager_name); - - if($this->shouldReconnect($ex)){ - Log::warning - ( - sprintf - ( - "DoctrineTransactionService::transaction should reconnect %s retry %s", - $ex->getMessage(), - $retry - ) + + if ($commitStarted) { + // The COMMIT itself failed after being sent to the server: its + // outcome is unknown, so retrying could duplicate every write + // and side effect of the callback. Propagate as a marker type - + // the raw driver exception (e.g. ConnectionLost) looks retryable + // to the layers above (Laravel queue tries, caller-side retries), + // which would re-execute the whole callback anyway; the marker is + // what lets them treat this as "operation state unknown" instead. + Log::error(sprintf( + "DoctrineTransactionService::runRootTransaction commit outcome unknown after '%s'; not retrying.", + $ex->getMessage() + )); + // A connection-level commit failure leaves a dead handle: discard + // the pair. Cleanup only - still never retry. + $this->restoreRegistryAfterFailure($em, $rollbackFailed || $this->shouldReconnect($ex)); + throw new AmbiguousCommitException( + sprintf( + "DoctrineTransactionService::runRootTransaction commit outcome unknown after '%s'; " + . 'the transaction may or may not be durable - reconcile, do not blind-retry.', + $ex->getMessage() + ), + 0, + $ex ); - if ($retry === self::MaxRetries) { - Log::warning(sprintf("DoctrineTransactionService::transaction Max Retry Reached %s", $retry)); + } + + if ($this->shouldReconnect($ex)) { + Log::warning(sprintf( + "DoctrineTransactionService::runRootTransaction reconnectable error '%s', retry %d/%d", + $ex->getMessage(), + $retry, + self::MaxRetries + )); + + // Root only: destructive recovery is safe here because + // no outer transaction holds references to this EM/connection. + $this->discardBrokenManager($em); + + if ($retry >= self::MaxRetries) { + Log::warning(sprintf("DoctrineTransactionService::runRootTransaction Max Retry Reached %d", $retry)); Log::error($ex); throw $ex; } + continue; } - Log::warning("DoctrineTransactionService::transaction rolling back TX"); + + Log::warning("DoctrineTransactionService::runRootTransaction rolling back TX"); Log::warning($ex); + $this->restoreRegistryAfterFailure($em, $rollbackFailed); throw $ex; + } catch (\Throwable $throwable) { + // \Error throwables (TypeError, etc.) are not \Exception and skip the + // reconnect handling above, which typehints shouldReconnect(\Exception). + // They are never retried, but a callback that closed the EM before + // throwing one must still leave a live manager in the registry. + $this->restoreRegistryAfterFailure($em, $rollbackFailed); + throw $throwable; } } - return $result; + throw new \RuntimeException("DoctrineTransactionService::runRootTransaction exceeded max retries."); + } + + /** + * Nested transaction: runs inside an already-active transaction. + * Flushes so auto-generated IDs are available to the caller, + * but does NOT retry, close the connection, or reset the EntityManager. + * On failure, rolls back the nested level and re-throws to the root. + * + * @param Closure $callback + * @return mixed|null + * @throws \Throwable + */ + private function runNestedTransaction(Closure $callback) + { + $em = Registry::getManager($this->manager_name); + $conn = $em->getConnection(); + + $conn->beginTransaction(); + + try { + $result = $callback($this); + $this->failFastIfEntityManagerClosed($em, 'runNestedTransaction'); + $em->flush(); + $conn->commit(); + return $result; + } catch (\Throwable $ex) { + $this->safeRollback($conn, $ex, 'runNestedTransaction'); + // No resetManager, no close(), no retry — let the root handle recovery. + throw $ex; + } } -} \ No newline at end of file +} diff --git a/tests/PresentationServiceTest.php b/tests/PresentationServiceTest.php new file mode 100644 index 000000000..dbec8ad8f --- /dev/null +++ b/tests/PresentationServiceTest.php @@ -0,0 +1,133 @@ +shouldReceive('getCurrentUser')->with(false)->andReturn(self::$defaultMember); + App::instance('resource_server_context', $resource_server_context_mock); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + self::clearMemberTestData(); + parent::tearDown(); + \Mockery::close(); + } + + /** + * PresentationService::submitPresentation() (PresentationService.php:259) calls the + * nested saveOrUpdatePresentation() (:399) with no try/catch. A nonexistent track_id + * throws EntityNotFoundException at :443-450, rolling back the entire submission - + * including the Presentation entity submitPresentation() itself already built and + * attached to the summit BEFORE calling saveOrUpdatePresentation() (:362-368). + */ + public function testSubmitPresentationRollsBackWhenTrackNotFound() + { + $service = App::make(IPresentationService::class); + + $title = 'Rollback Test Presentation ' . uniqid(); + + $data = [ + 'title' => $title, + 'description' => 'this is a description', + 'social_description' => 'this is a social description', + 'level' => 'N/A', + 'attendees_expected_learnt' => 'super duper', + 'type_id' => self::$defaultPresentationType->getId(), + 'track_id' => 999999999, + 'attending_media' => true, + 'selection_plan_id' => self::$default_selection_plan->getId(), + ]; + + try { + $service->submitPresentation(self::$summit, $data); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + foreach (self::$summit->getEvents() as $event) { + $this->assertNotEquals($title, $event->getTitle()); + } + } + + /** + * PresentationService::updatePresentationSubmission() (PresentationService.php:502) + * also calls the shared nested saveOrUpdatePresentation() with no try/catch. Prove the + * update path rolls back too, leaving the target presentation's track/title unchanged. + */ + public function testUpdatePresentationSubmissionRollsBackWhenTrackNotFound() + { + $service = App::make(IPresentationService::class); + + $original_title = 'Original Presentation ' . uniqid(); + + $presentation = $service->submitPresentation(self::$summit, [ + 'title' => $original_title, + 'description' => 'this is a description', + 'social_description' => 'this is a social description', + 'level' => 'N/A', + 'attendees_expected_learnt' => 'super duper', + 'type_id' => self::$defaultPresentationType->getId(), + 'track_id' => self::$defaultTrack->getId(), + 'attending_media' => true, + 'selection_plan_id' => self::$default_selection_plan->getId(), + ]); + $presentation_id = $presentation->getId(); + $original_track_id = $presentation->getCategoryId(); + + try { + $service->updatePresentationSubmission(self::$summit, $presentation_id, [ + 'title' => 'Updated Title Should Not Persist', + 'type_id' => self::$defaultPresentationType->getId(), + 'track_id' => 999999999, + ]); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + } + + self::$em->clear(); + + $reFetched = self::$em->find(\models\summit\Presentation::class, $presentation_id); + $this->assertNotNull($reFetched); + $this->assertEquals($original_title, $reFetched->getTitle()); + $this->assertEquals($original_track_id, $reFetched->getCategoryId()); + } +} diff --git a/tests/ProtectedApiTestCase.php b/tests/ProtectedApiTestCase.php index 85f1483ae..fa6f10c64 100644 --- a/tests/ProtectedApiTestCase.php +++ b/tests/ProtectedApiTestCase.php @@ -150,6 +150,7 @@ public function get($token_value) SummitScopes::DeleteRegistrationOrders, SummitScopes::UpdateRegistrationOrders, SummitScopes::UpdateMyRegistrationOrders, + SummitScopes::DeleteMyRegistrationOrders, SummitScopes::WriteBadgeScan, SummitScopes::ReadBadgeScan, SummitScopes::CreateRegistrationOrders, diff --git a/tests/ResourceServerContextTest.php b/tests/ResourceServerContextTest.php index ebdaae323..1fe7f91f0 100644 --- a/tests/ResourceServerContextTest.php +++ b/tests/ResourceServerContextTest.php @@ -12,13 +12,53 @@ * limitations under the License. **/ use Illuminate\Support\Facades\App; +use LaravelDoctrine\ORM\Facades\Registry; +use models\main\Member; use models\oauth2\IResourceServerContext; +use models\utils\SilverstripeBaseModel; /** * Class ResourceServerContextTest * @package Tests */ class ResourceServerContextTest extends BrowserKitTestCase { + /** + * Minimal IDP auth-context claims for a user, mirroring testSync's shape. + * user_groups uses the REAL claim shape (array of ['slug' => ...] objects) - + * checkGroups() reads $idpGroup['slug']. + */ + private function buildAuthContext(string $external_id, string $email, string $first_name = 'test', string $last_name = 'test', array $groups = []): array + { + return [ + 'user_id' => $external_id, + 'external_user_id' => $external_id, + 'user_identifier' => 'test', + 'user_email' => $email, + 'user_email_verified' => true, + 'user_first_name' => $first_name, + 'user_last_name' => $last_name, + 'user_groups' => $groups, + ]; + } + + private function persistMember(string $email, string $first_name, string $last_name, ?string $external_id = null): Member + { + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $member = new Member(); + $member->setEmail($email); + $member->setActive(true); + $member->setFirstName($first_name); + $member->setLastName($last_name); + $member->setEmailVerified(true); + // Member.ExternalUserId carries a unique index with default 0, so every + // member needs SOME unique external id (the same pattern InsertMemberTestData + // uses); pass one explicitly when the test needs to control it. + $member->setUserExternalId(is_null($external_id) ? mt_rand() : intval($external_id)); + $em->persist($member); + $em->flush(); + return $member; + } + public function testSync(){ $ctx = App::make(IResourceServerContext::class); $this->assertInstanceOf(IResourceServerContext::class, $ctx); @@ -79,4 +119,158 @@ public function testSetAuthorizationContextResetsUserCache(): void $this->assertFalse($prop->getValue($ctx), 'setAuthorizationContext() must reset cachedCurrentUserResolved'); } + + /** + * No user_id claim => anonymous request: getCurrentUser() must return null and + * cache that null so subsequent calls don't re-attempt resolution. + */ + public function testGetCurrentUserReturnsNullForAnonymousRequest(): void + { + $ctx = App::make(IResourceServerContext::class); + $ctx->setAuthorizationContext([]); + + $this->assertNull($ctx->getCurrentUser()); + + $ref = new \ReflectionClass($ctx); + $prop = $ref->getProperty('cachedCurrentUserResolved'); + $prop->setAccessible(true); + $this->assertTrue($prop->getValue($ctx), 'The null result must be cached'); + + $this->assertNull($ctx->getCurrentUser(), 'The cached null must be returned as-is'); + } + + /** + * Lookup strategy 1: an existing member linked to the IDP external id is + * resolved directly. + */ + public function testGetCurrentUserResolvesExistingMemberByExternalId(): void + { + $external_id = (string)mt_rand(100000000, 999999999); + $email = sprintf('ctx-ext-%s@test.com', uniqid()); + $existing = $this->persistMember($email, 'Existing', 'ByExternalId', $external_id); + + $ctx = App::make(IResourceServerContext::class); + $ctx->setAuthorizationContext($this->buildAuthContext($external_id, $email, 'Existing', 'ByExternalId')); + + $member = $ctx->getCurrentUser(); + + $this->assertNotNull($member); + $this->assertEquals($existing->getId(), $member->getId(), 'Must resolve the pre-existing member, not create a new one'); + } + + /** + * Lookup strategy 2: a member that exists locally by email but is not linked + * to THIS IDP external id is resolved by email - and the claim's external id + * gets linked as a side-effect. + */ + public function testGetCurrentUserResolvesExistingMemberByEmailAndLinksExternalId(): void + { + $email = sprintf('ctx-email-%s@test.com', uniqid()); + $existing = $this->persistMember($email, 'Existing', 'ByEmail'); + + $external_id = (string)mt_rand(100000000, 999999999); + $ctx = App::make(IResourceServerContext::class); + $ctx->setAuthorizationContext($this->buildAuthContext($external_id, $email, 'Existing', 'ByEmail')); + + $member = $ctx->getCurrentUser(); + + $this->assertNotNull($member); + $this->assertEquals($existing->getId(), $member->getId(), 'Must resolve the pre-existing member by email'); + $this->assertEquals(intval($external_id), $member->getUserExternalId(), 'The IDP external id must be linked as a side-effect'); + } + + /** + * A first call with $update_member_fields=false must NOT apply the IDP claim + * fields; a later call with true within the same request must apply them + * exactly once (PHASE A deferred sync) - and the cache must survive the sync + * (Member::setFirstName() invalidates it mid-flight, getCurrentUser() restores it). + */ + public function testGetCurrentUserDefersFieldSyncUntilRequested(): void + { + $external_id = (string)mt_rand(100000000, 999999999); + $email = sprintf('ctx-defer-%s@test.com', uniqid()); + $this->persistMember($email, 'OldFirst', 'OldLast', $external_id); + + $ctx = App::make(IResourceServerContext::class); + $ctx->setAuthorizationContext($this->buildAuthContext($external_id, $email, 'NewFirst', 'NewLast')); + + $member = $ctx->getCurrentUser(true, false); + $this->assertNotNull($member); + $this->assertEquals('OldFirst', $member->getFirstName(), 'update_member_fields=false must not sync claim fields'); + $this->assertEquals('OldLast', $member->getLastName()); + + $member = $ctx->getCurrentUser(true, true); + $this->assertNotNull($member, 'The cache must survive the deferred field sync'); + $this->assertEquals('NewFirst', $member->getFirstName(), 'The deferred field sync must apply the claim fields'); + $this->assertEquals('NewLast', $member->getLastName()); + + $this->assertNotNull($ctx->getCurrentUser(true, true), 'Subsequent calls must keep returning the cached member'); + } + + /** + * A first call with $synch_groups=false must NOT reconcile IDP groups; a later + * call with true within the same request must (PHASE A deferred group sync). + * Uses the REAL claim shape ([['slug' => ...]]). + */ + public function testGetCurrentUserDefersGroupSyncUntilRequested(): void + { + $external_id = (string)mt_rand(100000000, 999999999); + $email = sprintf('ctx-groups-%s@test.com', uniqid()); + $this->persistMember($email, 'Group', 'Sync', $external_id); + + $slug = sprintf('idp-group-%s', uniqid()); + $ctx = App::make(IResourceServerContext::class); + $ctx->setAuthorizationContext( + $this->buildAuthContext($external_id, $email, 'Group', 'Sync', [['slug' => $slug]]) + ); + + $member = $ctx->getCurrentUser(false); + $this->assertNotNull($member); + $this->assertFalse($member->belongsToGroup($slug), 'synch_groups=false must not reconcile IDP groups'); + $member_id = $member->getId(); + + $member = $ctx->getCurrentUser(true); + $this->assertNotNull($member); + + // Asserting on the SAME instance also pins the belongsToGroup() memo + // invalidation: the pre-sync assert above memoized `false` for this slug, + // and add2Group() must clear that memo so this re-check hits the DB. + $this->assertTrue($member->belongsToGroup($slug), 'The deferred group sync must add the IDP group'); + $this->assertEquals($member_id, $member->getId()); + } + + /** + * Email collision guard: when the resolved member's new claim email is already + * held by a DIFFERENT member, that other member's email must be invalidated + * (the IDP is the source of truth) and the claim email applied to the + * resolved member. + */ + public function testGetCurrentUserInvalidatesEmailOfCollidingMember(): void + { + $external_id = (string)mt_rand(100000000, 999999999); + $old_email = sprintf('ctx-collision-a-%s@test.com', uniqid()); + $resolved = $this->persistMember($old_email, 'Colliding', 'Owner', $external_id); + + $claimed_email = sprintf('ctx-collision-b-%s@test.com', uniqid()); + $other = $this->persistMember($claimed_email, 'Other', 'Holder'); + $other_id = $other->getId(); + + $ctx = App::make(IResourceServerContext::class); + $ctx->setAuthorizationContext($this->buildAuthContext($external_id, $claimed_email, 'Colliding', 'Owner')); + + $member = $ctx->getCurrentUser(); + + $this->assertNotNull($member); + $this->assertEquals($resolved->getId(), $member->getId()); + $this->assertEquals($claimed_email, $member->getEmail(), 'The claim email must be applied to the resolved member'); + + $em = Registry::getManager(SilverstripeBaseModel::EntityManager); + $em->clear(); + $other = $em->find(Member::class, $other_id); + $this->assertEquals( + sprintf('%s-invalid@invalid', $other_id), + $other->getEmail(), + 'The colliding member email must be invalidated' + ); + } } \ No newline at end of file diff --git a/tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php b/tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php new file mode 100644 index 000000000..c9b6463a7 --- /dev/null +++ b/tests/SelectionPlanOrderExtraQuestionTypeServiceTest.php @@ -0,0 +1,76 @@ +addExtraQuestion(self::$summit, [ + 'name' => 'Q_' . uniqid(), + 'label' => $original_label, + 'type' => ExtraQuestionTypeConstants::TextQuestionType, + ]); + $question_id = $question->getId(); + + try { + $service->updateExtraQuestionBySelectionPlan(self::$default_selection_plan, $question_id, [ + 'label' => 'Updated Label ' . uniqid(), + ]); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('does not belongs to selection plan', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $reFetched = self::$summit->getSelectionPlanExtraQuestionById($question_id); + $this->assertNotNull($reFetched); + $this->assertEquals($original_label, $reFetched->getLabel()); + } +} diff --git a/tests/SpeakerServiceRegistrationTest.php b/tests/SpeakerServiceRegistrationTest.php new file mode 100644 index 000000000..11c989dfb --- /dev/null +++ b/tests/SpeakerServiceRegistrationTest.php @@ -0,0 +1,141 @@ +addSpeakerBySummit(self::$summit, [ + 'title' => 'Developer!', + 'first_name' => 'Speaker', + 'last_name' => 'A', + 'email' => $speaker_a_email, + 'registration_code' => $registration_code, + ]); + } + + /** + * SpeakerService::addSpeakerBySummit() (SpeakerService.php:300) calls addSpeaker() + * (:193, its own nested transaction, creates the speaker) then, if a registration_code + * is provided, registerSummitPromoCodeByValue() (:419, its own nested transaction) - no + * try/catch around either call. If the code is already assigned to a DIFFERENT speaker + * for the same summit (ValidationException at :439-441), the entire addSpeakerBySummit + * call rolls back, including the speaker just created by the first nested call. + */ + public function testAddSpeakerBySummitRollsBackEntireChainWhenRegistrationCodeAlreadyAssignedToAnotherSpeaker() + { + $service = App::make(ISpeakerService::class); + + $registration_code = 'REG-CODE-' . uniqid(); + + // speaker A registers first, claiming the code + $this->registerSpeakerAClaimingCode($registration_code); + + // speaker B tries to use the SAME code, already assigned to speaker A + $speaker_b_email = 'speaker-b-' . uniqid() . '@test.com'; + + try { + $service->addSpeakerBySummit(self::$summit, [ + 'title' => 'Developer!', + 'first_name' => 'Speaker', + 'last_name' => 'B', + 'email' => $speaker_b_email, + 'registration_code' => $registration_code, + ]); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('another speaker', $ex->getMessage()); + } + + self::$em->clear(); + + $speaker_b = App::make(ISpeakerRepository::class)->getByEmail($speaker_b_email); + $this->assertNull($speaker_b); + } + + /** + * SpeakerService::updateSpeakerBySummit() (SpeakerService.php:364) calls updateSpeaker() + * (:368, its own nested transaction) FIRST, then - only if a registration_code is provided - + * registerSummitPromoCodeByValue() (:382, its own nested transaction) - no try/catch around + * either call. updateSpeaker() commits a real title change; if the registration code is + * already assigned to a DIFFERENT speaker (ValidationException at :439-441), the entire + * updateSpeakerBySummit call rolls back, including the already-committed title change. + */ + public function testUpdateSpeakerBySummitRollsBackAlreadyCommittedTitleWhenRegistrationCodeAlreadyAssignedToAnotherSpeaker() + { + $service = App::make(ISpeakerService::class); + + $registration_code = 'REG-CODE-' . uniqid(); + + // speaker A claims the code + $this->registerSpeakerAClaimingCode($registration_code); + + // speaker B has no registration code yet + $original_title = 'Original Title ' . uniqid(); + $speaker_b_email = 'speaker-b-' . uniqid() . '@test.com'; + $speaker_b = $service->addSpeakerBySummit(self::$summit, [ + 'title' => $original_title, + 'first_name' => 'Speaker', + 'last_name' => 'B', + 'email' => $speaker_b_email, + ]); + $speaker_b_id = $speaker_b->getId(); + + try { + $service->updateSpeakerBySummit(self::$summit, $speaker_b, [ + 'title' => 'Updated Title Should Not Persist', + 'registration_code' => $registration_code, + ]); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('another speaker', $ex->getMessage()); + } + + self::$em->clear(); + + $reFetched = self::$em->find(PresentationSpeaker::class, $speaker_b_id); + $this->assertNotNull($reFetched); + $this->assertEquals($original_title, $reFetched->getTitle()); + } +} diff --git a/tests/SummitOrderServiceTest.php b/tests/SummitOrderServiceTest.php index daf42f230..9bb5a93e8 100644 --- a/tests/SummitOrderServiceTest.php +++ b/tests/SummitOrderServiceTest.php @@ -32,9 +32,13 @@ use App\Services\Utils\ILockManagerService; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\Cache; +use Illuminate\Support\Facades\Config; use Illuminate\Support\Facades\Queue; use libs\utils\ITransactionService; use Mockery; +use services\utils\AmbiguousCommitException; +use models\exceptions\EntityNotFoundException; +use models\exceptions\ValidationException; use models\main\ICompanyRepository; use models\main\IMemberRepository; use models\main\ITagRepository; @@ -386,15 +390,21 @@ public function testAutoAssignDifferentPrePaidTicketsUntilEmpty() { * @param string $csv_content * @return ISummitOrderService */ - private function buildTicketDataImportService(string $csv_content): ISummitOrderService + private function buildTicketDataImportService( + string $csv_content, + ?ITransactionService $tx_service = null, + ?IFileDownloadStrategy $download_strategy = null + ): ISummitOrderService { $upload_strategy = Mockery::mock(IFileUploadStrategy::class); - $download_strategy = Mockery::mock(IFileDownloadStrategy::class); - $download_strategy->shouldReceive('exists')->andReturn(true); - $download_strategy->shouldReceive('get')->andReturn($csv_content); - $download_strategy->shouldReceive('getDriver')->andReturn('mock'); - $download_strategy->shouldReceive('delete'); + if (is_null($download_strategy)) { + $download_strategy = Mockery::mock(IFileDownloadStrategy::class); + $download_strategy->shouldReceive('exists')->andReturn(true); + $download_strategy->shouldReceive('get')->andReturn($csv_content); + $download_strategy->shouldReceive('getDriver')->andReturn('mock'); + $download_strategy->shouldReceive('delete'); + } return new SummitOrderService( App::make(ISummitTicketTypeRepository::class), @@ -416,11 +426,43 @@ private function buildTicketDataImportService(string $csv_content): ISummitOrder App::make(ISummitRefundRequestRepository::class), App::make(ICompanyService::class), App::make(ITicketFinderStrategyFactory::class), - App::make(ITransactionService::class), + $tx_service ?? App::make(ITransactionService::class), App::make(ILockManagerService::class) ); } + /** + * The seed sets the registration period relative to the summit's (future) begin + * date, so reserve() rejects with "registration period is closed" by default. + * Open it around NOW for tests exercising the reservation flow. + */ + private function openRegistrationPeriod(): void + { + $now = new \DateTime('now', new \DateTimeZone('UTC')); + self::$summit->setRawRegistrationBeginDate((clone $now)->sub(new \DateInterval('P1D'))); + self::$summit->setRawRegistrationEndDate((clone $now)->add(new \DateInterval('P30D'))); + self::$em->persist(self::$summit); + self::$em->flush(); + } + + private function disableDefaultBadgeType(): int + { + $badge_type_id = self::$default_badge_type->getId(); + self::$default_badge_type->setIsDefault(false); + return $badge_type_id; + } + + private function restoreDefaultBadgeType(int $badge_type_id): void + { + // The failed transaction's rollback already cleared self::$em, detaching the + // original self::$default_badge_type instance - restore via a fresh lookup so + // the flush actually persists the flag, instead of silently no-op'ing on a + // detached entity. + self::$default_badge_type = self::$em->find(SummitBadgeType::class, $badge_type_id); + self::$default_badge_type->setIsDefault(true); + self::$em->flush(); + } + /** * @param string $name * @param string $type @@ -494,6 +536,317 @@ private function getDefaultAttendee(): SummitAttendee return $attendee; } + /** + * The reserve endpoint is also exposed on the public API (routes/public_api.php) + * with no authenticated member: reserve() accepts ?Member and every saga task + * takes ?Member $owner (ReserveOrderTask carries the whole guest branch), so a + * null owner must produce a valid guest reservation keyed by owner_email. + */ + public function testReserveAsGuestWithoutMemberCreatesOrder() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $guest_email = sprintf('guest-%s@test.com', uniqid()); + $payload = [ + "owner_email" => $guest_email, + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => self::$default_ticket_type->getId()], + ], + ]; + + $order = $service->reserve(null, self::$summit, $payload); + + $this->assertNotNull($order); + $this->assertEquals($guest_email, $order->getOwnerEmail()); + $this->assertFalse($order->hasOwner()); + } + + /** + * Guest buyer with more than one ticket: the auto-assign-first-ticket rule + * (buyer without paid orders + multi-ticket order) must also work with a null + * owner, sourcing the attendee data from the payload's owner_* fields. + */ + public function testReserveAsGuestWithMultipleTicketsAutoAssignsFirstTicket() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $guest_email = sprintf('guest-multi-%s@test.com', uniqid()); + $payload = [ + "owner_email" => $guest_email, + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => self::$default_ticket_type->getId()], + ["type_id" => self::$default_ticket_type->getId()], + ], + ]; + + $order = $service->reserve(null, self::$summit, $payload); + + $this->assertNotNull($order); + $this->assertCount(2, $order->getTickets()); + // exactly the first ticket gets auto-assigned to the guest buyer + $assigned = []; + foreach ($order->getTickets() as $ticket) { + if ($ticket->hasOwner()) $assigned[] = $ticket->getOwnerEmail(); + } + $this->assertEquals([$guest_email], $assigned); + } + + /** + * ReserveTicketsTask rejects a reservation on a sold-out ticket type, and the + * saga compensation leaves no order behind. + */ + public function testReserveFailsWhenTicketTypeSoldOut() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $sold_out_type = new SummitTicketType(); + $sold_out_type->setName('RESERVE SOLD OUT TICKET TYPE'); + $sold_out_type->setCost(100); + $sold_out_type->setCurrency('USD'); + $sold_out_type->setQuantity2Sell(1); + $sold_out_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($sold_out_type); + self::$em->persist(self::$summit); + self::$em->flush(); + $sold_out_type->sell(1); + self::$em->flush(); + + $initial_order_count = self::$summit->getOrders()->count(); + + $payload = [ + "owner_email" => sprintf('guest-soldout-%s@test.com', uniqid()), + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "tickets" => [ + ["type_id" => $sold_out_type->getId()], + ], + ]; + + try { + $service->reserve(null, self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('not available', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $this->assertEquals($initial_order_count, self::$summit->getOrders()->count()); + } + + /** + * ReserveTicketsTask rejects orders mixing ticket types of different currencies. + */ + public function testReserveFailsWhenTicketCurrenciesAreMixed() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $eur_type = new SummitTicketType(); + $eur_type->setName('RESERVE EUR TICKET TYPE'); + $eur_type->setCost(100); + $eur_type->setCurrency('EUR'); + $eur_type->setQuantity2Sell(10); + $eur_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($eur_type); + self::$em->persist(self::$summit); + self::$em->flush(); + + $payload = [ + "owner_email" => sprintf('guest-currency-%s@test.com', uniqid()), + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "tickets" => [ + ["type_id" => self::$default_ticket_type->getId()], + ["type_id" => $eur_type->getId()], + ], + ]; + + try { + $service->reserve(null, self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('same currency', $ex->getMessage()); + } + } + + /** + * PreOrderValidationTask rejects any reservation outside the summit's + * registration period. + */ + public function testReserveFailsWhenRegistrationPeriodClosed() + { + // explicitly close the period (both dates in the past) + $now = new \DateTime('now', new \DateTimeZone('UTC')); + self::$summit->setRawRegistrationBeginDate((clone $now)->sub(new \DateInterval('P30D'))); + self::$summit->setRawRegistrationEndDate((clone $now)->sub(new \DateInterval('P1D'))); + self::$em->persist(self::$summit); + self::$em->flush(); + + $service = App::make(ISummitOrderService::class); + + $payload = [ + "owner_email" => sprintf('guest-closed-%s@test.com', uniqid()), + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "tickets" => [ + ["type_id" => self::$default_ticket_type->getId()], + ], + ]; + + try { + $service->reserve(null, self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('registration period is closed', $ex->getMessage()); + } + } + + /** + * checkout() on a free order needs no payment gateway: it must mark the order + * paid right away. + */ + public function testCheckoutSetsFreeOrderAsPaid() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $free_type = new SummitTicketType(); + $free_type->setName('FREE TICKET TYPE'); + $free_type->setCost(0); + $free_type->setCurrency('USD'); + $free_type->setQuantity2Sell(10); + $free_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($free_type); + self::$em->persist(self::$summit); + self::$em->flush(); + + $payload = [ + "owner_email" => sprintf('guest-free-%s@test.com', uniqid()), + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "tickets" => [ + ["type_id" => $free_type->getId()], + ], + ]; + + $order = $service->reserve(null, self::$summit, $payload); + $this->assertTrue($order->isFree()); + + $order = $service->checkout(self::$summit, $order->getHash(), []); + + $this->assertTrue($order->isPaid()); + } + + public function testCheckoutFailsWhenOrderNotFound() + { + $service = App::make(ISummitOrderService::class); + + try { + $service->checkout(self::$summit, 'non-existent-hash-' . uniqid(), []); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('Order not found', $ex->getMessage()); + } + } + + /** + * A cancelled reservation can not be checked out - the buyer must start over. + */ + public function testCheckoutFailsWhenOrderIsCancelled() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $payload = [ + "owner_email" => sprintf('guest-cancelled-%s@test.com', uniqid()), + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "tickets" => [ + ["type_id" => self::$default_ticket_type->getId()], + ], + ]; + + $order = $service->reserve(null, self::$summit, $payload); + $hash = $order->getHash(); + + $service->cancel(self::$summit, $hash); + + try { + $service->checkout(self::$summit, $hash, []); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('canceled', $ex->getMessage()); + } + } + + /** + * End-to-end cancel: abandoning a reservation must return the consumed seat to + * the ticket type inventory and mark the order cancelled - a bug here means + * overselling or phantom capacity. + */ + public function testCancelRestoresTicketAvailabilityAndCancelsOrder() + { + $this->openRegistrationPeriod(); + $service = App::make(ISummitOrderService::class); + + $capacity_type = new SummitTicketType(); + $capacity_type->setName('CANCEL CAPACITY TICKET TYPE'); + $capacity_type->setCost(100); + $capacity_type->setCurrency('USD'); + $capacity_type->setQuantity2Sell(20); + $capacity_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($capacity_type); + self::$em->persist(self::$summit); + self::$em->flush(); + $type_id = $capacity_type->getId(); + + $payload = [ + "owner_email" => sprintf('guest-cancel-%s@test.com', uniqid()), + "owner_first_name" => "Guest", + "owner_last_name" => "Buyer", + "tickets" => [ + ["type_id" => $type_id], + ], + ]; + + $order = $service->reserve(null, self::$summit, $payload); + $hash = $order->getHash(); + + self::$em->clear(); + $capacity_type = self::$em->find(SummitTicketType::class, $type_id); + $this->assertEquals(1, $capacity_type->getQuantitySold(), 'The reservation must consume one seat'); + + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $cancelled_order = $service->cancel(self::$summit, $hash); + $this->assertTrue($cancelled_order->isCancelled()); + + self::$em->clear(); + $capacity_type = self::$em->find(SummitTicketType::class, $type_id); + $this->assertEquals(0, $capacity_type->getQuantitySold(), 'Cancelling must return the seat to inventory'); + } + + public function testCancelFailsWhenOrderHashNotFound() + { + $service = App::make(ISummitOrderService::class); + + try { + $service->cancel(self::$summit, 'non-existent-hash-' . uniqid()); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('order not found', $ex->getMessage()); + } + } + public function testImportTicketDataSetsExtraQuestionAnswerOnNewAttendee() { Queue::fake(); @@ -937,4 +1290,449 @@ public function testImportTicketDataCreatesBadgeWhenTicketHasNone() $this->assertTrue($ticket->hasBadge()); $this->assertEquals('VIP BADGE', $ticket->getBadge()->getType()->getName()); } + + public function testAddTicketsCommitsAcrossNestedTransaction() + { + $service = App::make(ISummitOrderService::class); + + $order = self::$summit->getOrders()[0]; + $order_id = $order->getId(); + $initial_ticket_count = $order->getTickets()->count(); + + $result = $service->addTickets(self::$summit, $order_id, [ + 'ticket_type_id' => self::$default_ticket_type->getId(), + 'ticket_qty' => 2, + ]); + + $this->assertEquals($initial_ticket_count + 2, $result->getTickets()->count()); + + self::$em->clear(); + + $reFetched = self::$em->find(SummitOrder::class, $order_id); + $this->assertNotNull($reFetched); + $this->assertEquals($initial_ticket_count + 2, $reFetched->getTickets()->count()); + } + + public function testCreateOfflineOrderCommitsAcrossNestedAndSequentialRootTransactions() + { + $service = App::make(ISummitOrderService::class); + + $owner_email = sprintf('offline-order-%s@test.com', uniqid()); + + $payload = [ + 'owner_email' => $owner_email, + 'owner_first_name' => 'Offline', + 'owner_last_name' => 'Buyer', + 'ticket_type_id' => self::$default_ticket_type->getId(), + 'ticket_qty' => 1, + ]; + + $order = $service->createOfflineOrder(self::$summit, $payload); + $order_id = $order->getId(); + + $this->assertTrue($order->isPaid()); + $this->assertEquals(1, $order->getTickets()->count()); + $ticket = $order->getFirstTicket(); + $this->assertNotNull($ticket); + $owner = $ticket->getOwner(); + $this->assertNotNull($owner); + $this->assertEquals($owner_email, $owner->getEmail()); + + self::$em->clear(); + + $reFetched = self::$em->find(SummitOrder::class, $order_id); + $this->assertNotNull($reFetched); + $this->assertTrue($reFetched->isPaid()); + $this->assertEquals(1, $reFetched->getTickets()->count()); + $reFetchedTicket = $reFetched->getFirstTicket(); + $this->assertNotNull($reFetchedTicket); + $reFetchedOwner = $reFetchedTicket->getOwner(); + $this->assertNotNull($reFetchedOwner); + $this->assertEquals($owner_email, $reFetchedOwner->getEmail()); + } + + public function testCreateTicketsForOrderRollsBackEntireChainOnInvalidPromoCode() + { + $service = App::make(ISummitOrderService::class); + + $order = self::$summit->getOrders()[0]; + $order_id = $order->getId(); + $initial_ticket_count = $order->getTickets()->count(); + $ticket_type_id = self::$default_ticket_type->getId(); + $initial_quantity_sold = self::$default_ticket_type->getQuantitySold(); + $bogus_promo_code = 'DOES-NOT-EXIST-' . uniqid(); + + try { + $service->addTickets(self::$summit, $order_id, [ + 'ticket_type_id' => $ticket_type_id, + 'ticket_qty' => 1, + 'promo_code' => $bogus_promo_code, + ]); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('Promo code', $ex->getMessage()); + } + + self::$em->clear(); + + $reFetched = self::$em->find(SummitOrder::class, $order_id); + $this->assertNotNull($reFetched); + $this->assertEquals($initial_ticket_count, $reFetched->getTickets()->count()); + + // proves the SummitTicketType::sell(1) mutation made before the promo-code + // lookup threw was rolled back too, not just the order's ticket collection + $reFetchedTicketType = self::$em->find(SummitTicketType::class, $ticket_type_id); + $this->assertNotNull($reFetchedTicketType); + $this->assertEquals($initial_quantity_sold, $reFetchedTicketType->getQuantitySold()); + } + + public function testCreateOfflineOrderRollsBackEntireChainOnMissingDefaultBadgeType() + { + $service = App::make(ISummitOrderService::class); + $attendee_repository = App::make(ISummitAttendeeRepository::class); + + $badge_type_id = $this->disableDefaultBadgeType(); + + $owner_email = sprintf('offline-order-badge-fail-%s@test.com', uniqid()); + + $payload = [ + 'owner_email' => $owner_email, + 'owner_first_name' => 'Offline', + 'owner_last_name' => 'BadgeFail', + 'ticket_type_id' => self::$default_ticket_type->getId(), + 'ticket_qty' => 1, + ]; + + try { + $service->createOfflineOrder(self::$summit, $payload); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('default badge type', $ex->getMessage()); + } finally { + $this->restoreDefaultBadgeType($badge_type_id); + } + + self::$em->clear(); + + $attendee = $attendee_repository->getBySummitAndEmail(self::$summit, $owner_email); + $this->assertNull($attendee); + } + + /** + * processTicketData() must follow the importer's log-and-skip posture (skills/laravel-api.md + * "CSV Import/Export Conventions" in the FN knowledge vault): a bad row is caught, logged, and + * skipped, never aborts the rest of the file. Row 1 fails for a row-specific reason (its ticket + * type has zero remaining capacity), independent of row 2, which uses a ticket type with normal + * capacity. This proves both halves of the contract: the failing row doesn't propagate out of + * processTicketData() (no exception), and it doesn't block row 2 from being attempted and + * committed - not just that "nothing throws" for the same summit-wide reason. + */ + public function testProcessTicketDataSkipsFailingRowsAndContinuesProcessingRemainingRows() + { + $attendee_repository = App::make(ISummitAttendeeRepository::class); + + $email1 = sprintf('csv-import-row1-%s@test.com', uniqid()); + $email2 = sprintf('csv-import-row2-%s@test.com', uniqid()); + + $exhausted_ticket_type = new SummitTicketType(); + $exhausted_ticket_type->setName('SOLD OUT TICKET TYPE'); + $exhausted_ticket_type->setCost(100); + $exhausted_ticket_type->setCurrency('USD'); + $exhausted_ticket_type->setQuantity2Sell(1); + $exhausted_ticket_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($exhausted_ticket_type); + self::$em->persist(self::$summit); + self::$em->flush(); + // consume the only seat so row 1's sell() call fails on capacity, not on a summit-wide toggle + $exhausted_ticket_type->sell(1); + self::$em->flush(); + + $bad_ticket_type_id = $exhausted_ticket_type->getId(); + $good_ticket_type_id = self::$default_ticket_type->getId(); + + $csv_content = <<buildTicketDataImportService($csv_content); + // must not throw: a bad row is logged and skipped, not fatal to the whole file + $service->processTicketData(self::$summit->getId(), 'tickets.csv'); + + self::$em->clear(); + + $attendee1 = $attendee_repository->getBySummitAndEmail(self::$summit, $email1); + $attendee2 = $attendee_repository->getBySummitAndEmail(self::$summit, $email2); + $this->assertNull($attendee1); + $this->assertNotNull($attendee2); + } + + /** + * Volume happy path: every row of a 20-row CSV is valid, so all 20 attendees + * (each with its ticket) must exist afterwards and the source file must be + * deleted - a fully-processed import leaves nothing to reconcile. + */ + public function testProcessTicketDataImportsAllRowsWhenEveryRowIsValid() + { + $attendee_repository = App::make(ISummitAttendeeRepository::class); + + $bulk_type = new SummitTicketType(); + $bulk_type->setName('BULK OK TICKET TYPE'); + $bulk_type->setCost(100); + $bulk_type->setCurrency('USD'); + $bulk_type->setQuantity2Sell(50); + $bulk_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($bulk_type); + self::$em->persist(self::$summit); + self::$em->flush(); + + $uid = uniqid(); + $emails = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + $email = sprintf('csv-bulk-ok-%02d-%s@test.com', $i, $uid); + $emails[] = $email; + $rows[] = sprintf('%s,CSV,Row%02d,%d', $email, $i, $bulk_type->getId()); + } + $csv_content = "attendee_email,attendee_first_name,attendee_last_name,ticket_type_id\n" . implode("\n", $rows); + + $file_deleted = false; + $download_strategy = Mockery::mock(IFileDownloadStrategy::class); + $download_strategy->shouldReceive('exists')->andReturn(true); + $download_strategy->shouldReceive('get')->andReturn($csv_content); + $download_strategy->shouldReceive('getDriver')->andReturn('mock'); + $download_strategy->shouldReceive('delete')->andReturnUsing(function () use (&$file_deleted) { + $file_deleted = true; + }); + + $service = $this->buildTicketDataImportService($csv_content, null, $download_strategy); + $service->processTicketData(self::$summit->getId(), 'tickets.csv'); + + self::$em->clear(); + + foreach ($emails as $email) { + $attendee = $attendee_repository->getBySummitAndEmail(self::$summit, $email); + $this->assertNotNull($attendee, sprintf('Attendee %s should have been imported', $email)); + $this->assertCount(1, $attendee->getTickets(), sprintf('Attendee %s should have exactly one ticket', $email)); + } + $this->assertTrue($file_deleted, 'A fully-processed import must delete its source file'); + } + + /** + * Volume mixed outcome: 20 rows where 15 fail (sold-out ticket type - sell() + * throws, the row's transaction rolls back fully) interleaved with 5 valid + * rows. The 5 valid rows must import with their tickets, the 15 failing rows + * must leave no attendee behind, and the file is still deleted: a KNOWN + * per-row failure (logged and skipped) is not an unknown outcome, so there + * is nothing to reconcile against the DB. + */ + public function testProcessTicketDataImportsOnlyValidRowsWhenMostRowsFail() + { + $attendee_repository = App::make(ISummitAttendeeRepository::class); + + $good_type = new SummitTicketType(); + $good_type->setName('BULK MIXED OK TICKET TYPE'); + $good_type->setCost(100); + $good_type->setCurrency('USD'); + $good_type->setQuantity2Sell(50); + $good_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($good_type); + + $sold_out_type = new SummitTicketType(); + $sold_out_type->setName('BULK MIXED SOLD OUT TICKET TYPE'); + $sold_out_type->setCost(100); + $sold_out_type->setCurrency('USD'); + $sold_out_type->setQuantity2Sell(1); + $sold_out_type->setAudience(SummitTicketType::Audience_All); + self::$summit->addTicketType($sold_out_type); + self::$em->persist(self::$summit); + self::$em->flush(); + // consume the only seat so every row using this type fails on capacity + $sold_out_type->sell(1); + self::$em->flush(); + + $uid = uniqid(); + $good_emails = []; + $bad_emails = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + // every 4th row is valid (positions 4, 8, 12, 16, 20): failures happen + // both before and after each success, proving order independence + $is_good = ($i % 4 === 0); + $email = sprintf('csv-bulk-mix-%02d-%s@test.com', $i, $uid); + if ($is_good) $good_emails[] = $email; else $bad_emails[] = $email; + $rows[] = sprintf('%s,CSV,Row%02d,%d', $email, $i, $is_good ? $good_type->getId() : $sold_out_type->getId()); + } + $csv_content = "attendee_email,attendee_first_name,attendee_last_name,ticket_type_id\n" . implode("\n", $rows); + + $file_deleted = false; + $download_strategy = Mockery::mock(IFileDownloadStrategy::class); + $download_strategy->shouldReceive('exists')->andReturn(true); + $download_strategy->shouldReceive('get')->andReturn($csv_content); + $download_strategy->shouldReceive('getDriver')->andReturn('mock'); + $download_strategy->shouldReceive('delete')->andReturnUsing(function () use (&$file_deleted) { + $file_deleted = true; + }); + + $service = $this->buildTicketDataImportService($csv_content, null, $download_strategy); + // must not throw: 15 known failures are logged and skipped + $service->processTicketData(self::$summit->getId(), 'tickets.csv'); + + self::$em->clear(); + + $this->assertCount(5, $good_emails); + $this->assertCount(15, $bad_emails); + foreach ($good_emails as $email) { + $attendee = $attendee_repository->getBySummitAndEmail(self::$summit, $email); + $this->assertNotNull($attendee, sprintf('Valid row attendee %s should have been imported', $email)); + $this->assertCount(1, $attendee->getTickets(), sprintf('Attendee %s should have exactly one ticket', $email)); + } + foreach ($bad_emails as $email) { + $this->assertNull( + $attendee_repository->getBySummitAndEmail(self::$summit, $email), + sprintf('Failing row attendee %s should have been fully rolled back', $email) + ); + } + $this->assertTrue($file_deleted, 'Known per-row failures must not block the source file deletion'); + } + + /** + * A row whose transaction surfaces AmbiguousCommitException has an UNKNOWN + * outcome - it may or may not be durable (see DoctrineTransactionService) - so + * the import must not treat it as either processed or failed: remaining rows + * still run (they are independent), but the source file must NOT be deleted, + * so the unknown-outcome row can be reconciled against the DB. + */ + public function testProcessTicketDataKeepsFileAndContinuesWhenRowCommitOutcomeUnknown() + { + $attendee_repository = App::make(ISummitAttendeeRepository::class); + + $email1 = sprintf('csv-ambiguous-row1-%s@test.com', uniqid()); + $email2 = sprintf('csv-ambiguous-row2-%s@test.com', uniqid()); + $ticket_type_id = self::$default_ticket_type->getId(); + + $csv_content = <<inner = $inner; } + public function transaction(\Closure $callback, int $isolationLevel = 2) + { + if (++$this->calls === 2) + throw new AmbiguousCommitException('commit outcome unknown (simulated)'); + return $this->inner->transaction($callback, $isolationLevel); + } + }; + + $file_deleted = false; + $download_strategy = Mockery::mock(IFileDownloadStrategy::class); + $download_strategy->shouldReceive('exists')->andReturn(true); + $download_strategy->shouldReceive('get')->andReturn($csv_content); + $download_strategy->shouldReceive('getDriver')->andReturn('mock'); + $download_strategy->shouldReceive('delete')->andReturnUsing(function () use (&$file_deleted) { + $file_deleted = true; + }); + + $service = $this->buildTicketDataImportService($csv_content, $tx_service, $download_strategy); + // must not throw: the unknown-outcome row is recorded, not fatal to the file + $service->processTicketData(self::$summit->getId(), 'tickets.csv'); + + self::$em->clear(); + + // row 2 was still processed - rows are independent + $this->assertNotNull($attendee_repository->getBySummitAndEmail(self::$summit, $email2)); + // the source file survives so row 1 can be reconciled against the DB + $this->assertFalse($file_deleted, 'The import file must be kept when any row has an unknown commit outcome'); + } + + public function testAddTicketsRollsBackEntireChainOnMissingDefaultBadgeType() + { + $service = App::make(ISummitOrderService::class); + + $order = self::$summit->getOrders()[0]; + $order_id = $order->getId(); + $initial_ticket_count = $order->getTickets()->count(); + + $badge_type_id = $this->disableDefaultBadgeType(); + + try { + $service->addTickets(self::$summit, $order_id, [ + 'ticket_type_id' => self::$default_ticket_type->getId(), + 'ticket_qty' => 1, + ]); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('default badge type', $ex->getMessage()); + } finally { + $this->restoreDefaultBadgeType($badge_type_id); + } + + self::$em->clear(); + + $reFetched = self::$em->find(SummitOrder::class, $order_id); + $this->assertNotNull($reFetched); + $this->assertEquals($initial_ticket_count, $reFetched->getTickets()->count()); + } + + public function testRequestRefundOrderRollsBackEntireChainWhenOneTicketIsFree() + { + Queue::fake(); + Config::set('registration.admin_email', 'admin@test.com'); + + $service = App::make(ISummitOrderService::class); + + // Summit::canEmitRefundRequests() requires a future refund-request deadline; + // InsertSummitTestData does not set one by default. + self::$summit->setRegistrationAllowedRefundRequestTillDate(new \DateTime('+1 day', new \DateTimeZone('UTC'))); + + $order = self::$summit->getOrders()[0]; + $order_id = $order->getId(); + $tickets = $order->getTickets(); + $this->assertGreaterThanOrEqual(2, $tickets->count()); + + $refundableTicket = $tickets[0]; + $freeTicket = $tickets[1]; + $refundableTicketId = $refundableTicket->getId(); + $freeTicketId = $freeTicket->getId(); + + // isPaid() is a pure status flag (set by SummitOrder::setPaid() on every ticket), + // independent of cost - setRawCost(0.0) makes this ticket free without affecting + // its already-paid status, so it still enters requestRefundOrder's loop. + $freeTicket->setRawCost(0.0); + + // InsertSummitTestData sets the order's owner via SummitOrder::setOwner() only, + // which does not populate Member's inverse summit_registration_orders collection; + // requestRefundOrder() looks the order up via that collection with no self-healing + // fallback (unlike updateMyOrder()), so add it explicitly. + self::$defaultMember->addSummitRegistrationOrder($order); + + self::$em->persist($freeTicket); + self::$em->persist(self::$defaultMember); + self::$em->flush(); + + try { + $service->requestRefundOrder(self::$defaultMember, $order_id); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('free', $ex->getMessage()); + } + + self::$em->clear(); + + $reFetchedRefundable = self::$em->find(SummitAttendeeTicket::class, $refundableTicketId); + $reFetchedFree = self::$em->find(SummitAttendeeTicket::class, $freeTicketId); + $this->assertNotNull($reFetchedRefundable); + $this->assertNotNull($reFetchedFree); + $this->assertEquals(0, $reFetchedRefundable->getRefundedRequests()->count()); + $this->assertEquals(0, $reFetchedFree->getRefundedRequests()->count()); + } } diff --git a/tests/SummitPromoCodeServiceTest.php b/tests/SummitPromoCodeServiceTest.php new file mode 100644 index 000000000..e5241e4ee --- /dev/null +++ b/tests/SummitPromoCodeServiceTest.php @@ -0,0 +1,256 @@ +transaction() call (:223-311); the + * ticket-type-rules loop runs in a SECOND, separate tx_service->transaction() call + * (:313-326) that only starts once the first has already committed. A failure inside the + * rules loop (nested addPromoCodeTicketTypeRule() call, EntityNotFoundException at :496-497 + * for a nonexistent ticket_type_id) rolls back the SECOND transaction only - the + * already-committed promo code from the first transaction survives with zero rules applied. + * This test proves that partial-commit behavior explicitly, not a full rollback. + */ + public function testAddPromoCodeSurvivesWhenTicketTypeRuleFails() + { + $service = App::make(ISummitPromoCodeService::class); + + $code = 'TEST_PC_' . uniqid(); + + $data = [ + 'type' => PromoCodesConstants::SpeakerSummitRegistrationPromoCodeTypeAlternate, + 'class_name' => SpeakersSummitRegistrationPromoCode::ClassName, + 'code' => $code, + 'description' => 'TEST PROMO CODE', + 'quantity_available' => 10, + 'allowed_ticket_types' => [], + 'badge_features' => [], + 'valid_since_date' => 1649108093, + 'valid_until_date' => 1649109093, + 'ticket_types_rules' => [ + ['ticket_type_id' => 999999999], + ], + ]; + + try { + $service->addPromoCode(self::$summit, $data); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('Ticket Type', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + $promo_code = self::$summit->getPromoCodeByCode($code); + $this->assertNotNull($promo_code); + $this->assertEquals(0, $promo_code->getAllowedTicketTypes()->count()); + } + + private function buildCsvUpload(string $csv_content): UploadedFile + { + $path = tempnam(sys_get_temp_dir(), 'csv_import_test_'); + file_put_contents($path, $csv_content); + return new UploadedFile($path, 'import.csv', 'text/csv', null, true); + } + + /** + * Volume happy path for importPromoCodes(): every row of a 20-row CSV is a + * valid SUMMIT_PROMO_CODE, so all 20 codes must exist afterwards. + */ + public function testImportPromoCodesImportsAllRowsWhenEveryRowIsValid() + { + $service = App::make(ISummitPromoCodeService::class); + + $uid = strtoupper(uniqid()); + $codes = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + $code = sprintf('BULK_PC_%02d_%s', $i, $uid); + $codes[] = $code; + $rows[] = sprintf('%s,%s,10', $code, SummitRegistrationPromoCode::ClassName); + } + $csv = "code,class_name,quantity_available\n" . implode("\n", $rows); + + $service->importPromoCodes(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + foreach ($codes as $code) { + $this->assertNotNull( + self::$summit->getPromoCodeByCode($code), + sprintf('Promo code %s should have been imported', $code) + ); + } + } + + /** + * Volume mixed outcome for importPromoCodes(): 20 rows where 15 carry an invalid + * class_name (addPromoCode() throws, the import's per-row catch logs and skips) + * interleaved with 5 valid rows. The 5 valid codes must import and the 15 + * failing rows must leave nothing behind - and the import must not throw. + */ + public function testImportPromoCodesImportsOnlyValidRowsWhenMostRowsFail() + { + $service = App::make(ISummitPromoCodeService::class); + + $uid = strtoupper(uniqid()); + $good_codes = []; + $bad_codes = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + // every 4th row is valid: failures happen before and after each success + $is_good = ($i % 4 === 0); + $code = sprintf('BULK_MIX_PC_%02d_%s', $i, $uid); + if ($is_good) $good_codes[] = $code; else $bad_codes[] = $code; + $rows[] = sprintf('%s,%s,10', $code, $is_good ? SummitRegistrationPromoCode::ClassName : 'NOT_A_VALID_CLASS_NAME'); + } + $csv = "code,class_name,quantity_available\n" . implode("\n", $rows); + + // must not throw: failing rows are logged and skipped + $service->importPromoCodes(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + $this->assertCount(5, $good_codes); + $this->assertCount(15, $bad_codes); + foreach ($good_codes as $code) { + $this->assertNotNull( + self::$summit->getPromoCodeByCode($code), + sprintf('Valid promo code %s should have been imported', $code) + ); + } + foreach ($bad_codes as $code) { + $this->assertNull( + self::$summit->getPromoCodeByCode($code), + sprintf('Invalid-class row %s should not have created a promo code', $code) + ); + } + } + + /** + * Volume happy path for importSponsorPromoCodes(): every row of a 20-row CSV is + * a valid SPONSOR_PROMO_CODE tied to a seeded sponsor, so all 20 must import. + */ + public function testImportSponsorPromoCodesImportsAllRowsWhenEveryRowIsValid() + { + $service = App::make(ISummitPromoCodeService::class); + $sponsor_id = self::$sponsors[0]->getId(); + + $uid = strtoupper(uniqid()); + $codes = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + $code = sprintf('BULK_SP_%02d_%s', $i, $uid); + $codes[] = $code; + $rows[] = sprintf('%s,%s,10,%d', $code, SponsorSummitRegistrationPromoCode::ClassName, $sponsor_id); + } + $csv = "code,class_name,quantity_available,sponsor_id\n" . implode("\n", $rows); + + $service->importSponsorPromoCodes(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + foreach ($codes as $code) { + $promo_code = self::$summit->getPromoCodeByCode($code); + $this->assertNotNull($promo_code, sprintf('Sponsor promo code %s should have been imported', $code)); + $this->assertInstanceOf(SponsorSummitRegistrationPromoCode::class, $promo_code); + } + } + + /** + * Volume mixed outcome for importSponsorPromoCodes(): 20 rows where 15 carry a + * class_name outside the sponsor allow-list (the import skips them via its own + * `continue` guard - no exception involved) interleaved with 5 valid rows. + * + * NOTE: an empty sponsor_id is NOT a failure lever here - the service happily + * creates a SponsorSummitRegistrationPromoCode with sponsor = null (verified + * empirically), which is why the invalid-class guard is used instead. + */ + public function testImportSponsorPromoCodesImportsOnlyValidRowsWhenMostRowsFail() + { + $service = App::make(ISummitPromoCodeService::class); + $sponsor_id = self::$sponsors[0]->getId(); + + $uid = strtoupper(uniqid()); + $good_codes = []; + $bad_codes = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + $is_good = ($i % 4 === 0); + $code = sprintf('BULK_MIX_SP_%02d_%s', $i, $uid); + if ($is_good) $good_codes[] = $code; else $bad_codes[] = $code; + $rows[] = sprintf('%s,%s,10,%d', $code, $is_good ? SponsorSummitRegistrationPromoCode::ClassName : 'NOT_AN_ALLOWED_SPONSOR_CLASS', $sponsor_id); + } + $csv = "code,class_name,quantity_available,sponsor_id\n" . implode("\n", $rows); + + // must not throw: failing rows are logged and skipped + $service->importSponsorPromoCodes(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + $this->assertCount(5, $good_codes); + $this->assertCount(15, $bad_codes); + foreach ($good_codes as $code) { + $this->assertNotNull( + self::$summit->getPromoCodeByCode($code), + sprintf('Valid sponsor promo code %s should have been imported', $code) + ); + } + foreach ($bad_codes as $code) { + $this->assertNull( + self::$summit->getPromoCodeByCode($code), + sprintf('Sponsorless row %s should not have created a promo code', $code) + ); + } + } +} diff --git a/tests/SummitRegistrationInvitationServiceTest.php b/tests/SummitRegistrationInvitationServiceTest.php new file mode 100644 index 000000000..ef1a21a2b --- /dev/null +++ b/tests/SummitRegistrationInvitationServiceTest.php @@ -0,0 +1,137 @@ +importInvitationData(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + foreach ($emails as $email) { + $this->assertNotNull( + self::$summit->getSummitRegistrationInvitationByEmail($email), + sprintf('Invitation %s should have been imported', $email) + ); + } + } + + /** + * Volume mixed outcome: 20 rows where 15 carry a nonexistent allowed ticket type + * id (add() throws ValidationException, the per-row catch logs and skips) + * interleaved with 5 valid rows. The 5 valid invitations must import and the 15 + * failing rows must leave nothing behind - and the import must not throw. + */ + public function testImportInvitationDataImportsOnlyValidRowsWhenMostRowsFail() + { + $service = App::make(ISummitRegistrationInvitationService::class); + + // invitations only accept ticket types with the "With Invitation" audience + // (SummitRegistrationInvitation::addTicketType throws for any other audience) + $invitation_ticket_type = new SummitTicketType(); + $invitation_ticket_type->setName('INVITATION ONLY TICKET TYPE'); + $invitation_ticket_type->setCost(100); + $invitation_ticket_type->setCurrency('USD'); + $invitation_ticket_type->setQuantity2Sell(50); + $invitation_ticket_type->setAudience(SummitTicketType::Audience_With_Invitation); + self::$summit->addTicketType($invitation_ticket_type); + self::$em->persist(self::$summit); + self::$em->flush(); + $valid_ticket_type_id = $invitation_ticket_type->getId(); + + $uid = uniqid(); + $good_emails = []; + $bad_emails = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + // every 4th row is valid: failures happen before and after each success + $is_good = ($i % 4 === 0); + $email = sprintf('reg-inv-mix-%02d-%s@test.com', $i, $uid); + if ($is_good) $good_emails[] = $email; else $bad_emails[] = $email; + $rows[] = sprintf('%s,CSV,Row%02d,%s', $email, $i, $is_good ? $valid_ticket_type_id : '999999999'); + } + $csv = "email,first_name,last_name,allowed_ticket_types\n" . implode("\n", $rows); + + // must not throw: failing rows are logged and skipped + $service->importInvitationData(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + $this->assertCount(5, $good_emails); + $this->assertCount(15, $bad_emails); + foreach ($good_emails as $email) { + $this->assertNotNull( + self::$summit->getSummitRegistrationInvitationByEmail($email), + sprintf('Valid row invitation %s should have been imported', $email) + ); + } + foreach ($bad_emails as $email) { + $this->assertNull( + self::$summit->getSummitRegistrationInvitationByEmail($email), + sprintf('Failing row invitation %s should not exist', $email) + ); + } + } +} diff --git a/tests/SummitScheduleSettingsServiceTest.php b/tests/SummitScheduleSettingsServiceTest.php new file mode 100644 index 000000000..d738e6836 --- /dev/null +++ b/tests/SummitScheduleSettingsServiceTest.php @@ -0,0 +1,72 @@ +add(self::$summit, ['key' => 'my-schedule-main']); + + try { + $service->seedDefaults(self::$summit); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException $ex) { + $this->assertStringContainsString('my-schedule-main', $ex->getMessage()); + $this->assertStringContainsString('already exists', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + $has_schedule_main = false; + foreach (self::$summit->getScheduleSettings() as $config) { + if ($config->getKey() === 'schedule-main') { + $has_schedule_main = true; + } + } + $this->assertFalse($has_schedule_main); + } +} diff --git a/tests/SummitSelectedPresentationListServiceTest.php b/tests/SummitSelectedPresentationListServiceTest.php new file mode 100644 index 000000000..99c384b32 --- /dev/null +++ b/tests/SummitSelectedPresentationListServiceTest.php @@ -0,0 +1,94 @@ +shouldReceive('getCurrentUser')->andReturn(self::$defaultMember); + App::instance(\models\oauth2\IResourceServerContext::class, $resource_server_context_mock); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + self::clearMemberTestData(); + parent::tearDown(); + \Mockery::close(); + } + + /** + * SummitSelectedPresentationListService::assignPresentationToMyIndividualList() + * (SummitSelectedPresentationListService.php:379) calls createIndividualSelectionList() + * (:443, its own nested transaction) only when no individual list exists yet for the + * current member - no try/catch. That call commits a real, new + * SummitSelectedPresentationList. AFTER it returns, the outer's own presentation lookup + * (:449-455) throws EntityNotFoundException for a nonexistent presentation_id - rolling + * back the entire call, including the just-committed individual selection list. + */ + public function testAssignPresentationToMyIndividualListRollsBackAlreadyCommittedListWhenPresentationNotFound() + { + $service = App::make(ISummitSelectedPresentationListService::class); + + try { + $service->assignPresentationToMyIndividualList( + self::$summit, + self::$default_selection_plan->getId(), + self::$defaultTrack->getId(), + SummitSelectedPresentation::CollectionMaybe, + 999999999 + ); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('999999999', $ex->getMessage()); + } + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $selectionPlan = self::$em->find(\App\Models\Foundation\Summit\SelectionPlan::class, self::$default_selection_plan->getId()); + $category = self::$summit->getPresentationCategory(self::$defaultTrack->getId()); + $member = self::$member_repository->find(self::$defaultMember->getId()); + + $list = $selectionPlan->getSelectionListByTrackAndTypeAndOwner( + $category, + \models\summit\SummitSelectedPresentationList::Individual, + $member + ); + $this->assertNull($list); + } +} diff --git a/tests/SummitSelectionPlanServiceTest.php b/tests/SummitSelectionPlanServiceTest.php new file mode 100644 index 000000000..87b78a44d --- /dev/null +++ b/tests/SummitSelectionPlanServiceTest.php @@ -0,0 +1,160 @@ +getId(); + $selection_plan_id = self::$default_selection_plan->getId(); + + $uid = uniqid(); + $emails = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + $email = sprintf('allowed-ok-%02d-%s@test.com', $i, $uid); + $emails[] = $email; + $rows[] = $email; + } + $csv_content = "email\n" . implode("\n", $rows); + + $filename = 'allowed_members_all_valid.csv'; + $path = "tmp/selection_plans_allowed_members/{$filename}"; + Storage::disk('swift')->put($path, $csv_content); + + $service->processAllowedMemberData($summit_id, $selection_plan_id, $filename); + + self::$em->clear(); + $summit = self::$summit_repository->find($summit_id); + $selection_plan = $summit->getSelectionPlanById($selection_plan_id); + + foreach ($emails as $email) { + $this->assertTrue( + $selection_plan->containsMember($email), + sprintf('Allowed member %s should have been imported', $email) + ); + } + $this->assertFalse( + Storage::disk('swift')->exists($path), + 'A fully-processed import must delete its source file' + ); + } + + /** + * Volume mixed outcome: 20 rows where 15 are not importable - 10 emails already + * present on the selection plan (skipped by the containsMember guard) and 5 empty + * emails (skipped by the empty guard) - interleaved with 5 new valid emails. The + * 5 new members must import, no duplicates may be created, the method must not + * throw, and the file is still deleted (skips are not failures). + */ + public function testProcessAllowedMemberDataImportsOnlyValidRowsWhenMostRowsAreSkipped() + { + Storage::fake('swift'); + + $service = App::make(ISummitSelectionPlanService::class); + $summit_id = self::$summit->getId(); + $selection_plan_id = self::$default_selection_plan->getId(); + + $uid = uniqid(); + + // pre-load 10 emails so their CSV rows hit the already-present guard + $existing_emails = []; + for ($i = 1; $i <= 10; $i++) { + $email = sprintf('allowed-dup-%02d-%s@test.com', $i, $uid); + $existing_emails[] = $email; + self::$default_selection_plan->addAllowedMember($email); + } + self::$em->persist(self::$default_selection_plan); + self::$em->flush(); + $initial_count = self::$default_selection_plan->getAllowedMembers()->count(); + + $new_emails = []; + for ($i = 1; $i <= 5; $i++) { + $new_emails[] = sprintf('allowed-new-%02d-%s@test.com', $i, $uid); + } + + // interleave: 10 duplicates + 5 empty emails + 5 new, spread across the file + $rows = []; + foreach ($existing_emails as $idx => $email) { + $rows[] = $email; + if ($idx % 2 === 0) $rows[] = ''; // 5 empty-email rows + } + foreach ($new_emails as $email) { + $rows[] = $email; + } + $this->assertCount(20, $rows); + $csv_content = "email\n" . implode("\n", $rows); + + $filename = 'allowed_members_mixed.csv'; + $path = "tmp/selection_plans_allowed_members/{$filename}"; + Storage::disk('swift')->put($path, $csv_content); + + // must not throw: non-importable rows are skipped by the row guards + $service->processAllowedMemberData($summit_id, $selection_plan_id, $filename); + + self::$em->clear(); + $summit = self::$summit_repository->find($summit_id); + $selection_plan = $summit->getSelectionPlanById($selection_plan_id); + + foreach ($new_emails as $email) { + $this->assertTrue( + $selection_plan->containsMember($email), + sprintf('New allowed member %s should have been imported', $email) + ); + } + $this->assertEquals( + $initial_count + 5, + $selection_plan->getAllowedMembers()->count(), + 'Only the 5 new emails should have been added - no duplicates, no empties' + ); + $this->assertFalse( + Storage::disk('swift')->exists($path), + 'Skipped rows are not failures - the file must still be deleted' + ); + } +} diff --git a/tests/SummitServiceTest.php b/tests/SummitServiceTest.php new file mode 100644 index 000000000..373a3b481 --- /dev/null +++ b/tests/SummitServiceTest.php @@ -0,0 +1,473 @@ +shouldReceive('getCurrentUser')->with(false)->andReturn(self::$defaultMember); + App::instance('resource_server_context', $resource_server_context_mock); + } + + protected function tearDown(): void + { + self::clearSummitTestData(); + self::clearMemberTestData(); + parent::tearDown(); + \Mockery::close(); + } + + /** + * self::$defaultEventType has blackout_times='All' (a deliberate InsertSummitTestData + * fixture choice meant to conflict with everything else), and a pre-seeded event of that + * type already occupies most of the summit's first day - unusable for tests that need to + * create their OWN non-conflicting events. Build a dedicated, non-blackout event type. + */ + private function createNonBlackoutEventType(): SummitEventType + { + $type = new SummitEventType(); + $type->setType(ISummitEventType::Lunch); + $type->setBlackoutTimes(SummitEventTypeConstants::BLACKOUT_TIME_NONE); + self::$summit->addEventType($type); + self::$em->persist($type); + self::$em->flush(); + return $type; + } + + /** + * @return \DateTime[] [$start_date, $end_date], both within the summit's date range. + */ + private function nonConflictingEventWindow(): array + { + $start_date = (clone self::$summit->getBeginDate())->add(new \DateInterval('PT1H')); + $end_date = (clone $start_date)->add(new \DateInterval('PT1H')); + return [$start_date, $end_date]; + } + + /** + * SummitService::processRegistrationCompaniesData() (SummitService.php:3586) wraps each + * CSV row's tx_service->transaction() call in a LOCAL try/catch (:3608-3646), outside the + * transaction's own closure - the same log-and-skip shape SummitOrderService::processTicketData() + * now also uses. Prove one bad row (a company already attached to the summit, triggering + * addCompany()'s ValidationException at SummitService.php:3334) does NOT stop a later, valid + * row from being processed. + */ + public function testProcessRegistrationCompaniesDataSkipsFailingRowButProcessesLaterRows() + { + Storage::fake('swift'); + + $service = App::make(ISummitService::class); + $summit_id = self::$summit->getId(); + + // pre-attach an existing company to the summit so the CSV row for it + // triggers addCompany()'s "already has a company" ValidationException + $existing_company = self::$companies[0]; + $service->addCompany($summit_id, $existing_company->getId()); + + $new_company_name = 'New Row Company ' . uniqid(); + + $csv_content = <<getName()}, +{$new_company_name}, +CSV; + + $filename = 'registration_companies_isolation_test.csv'; + Storage::disk('swift')->put("tmp/registration_companies_import/{$filename}", $csv_content); + + // processRegistrationCompaniesData() catches each row's transaction failure + // locally, so it returns normally even though the first row fails. + $service->processRegistrationCompaniesData($summit_id, $filename); + + self::$em->clear(); + self::$summit = self::$summit_repository->find($summit_id); + + $new_company_on_summit = self::$summit->getRegistrationCompanyByName($new_company_name); + $this->assertNotNull($new_company_on_summit); + } + + /** + * SummitService::unPublishEvents() (SummitService.php:1467) loops calling + * unPublishEvent() (:1034, its own nested transaction) per event id - no try/catch. + * The FIRST (valid) id's unpublish commits inside its own nested transaction; a SECOND, + * nonexistent id then throws EntityNotFoundException (:1041), rolling back the ENTIRE + * batch, including the first id's already-committed unpublish. + */ + public function testUnPublishEventsRollsBackAlreadyCommittedUnpublishOnLaterBadEventId() + { + $service = App::make(ISummitService::class); + $event_type = $this->createNonBlackoutEventType(); + [$start_date, $end_date] = $this->nonConflictingEventWindow(); + + // No location_id set - AbstractPublishService::validateBlackOutTimesAndTimes() + // only enforces blackout collisions when the event has a non-null location + // (Summit fixture data seeds many published events spanning the whole date + // range with blackout_times set, so any located event anywhere would collide). + $event = $service->addEvent(self::$summit, [ + 'title' => 'Batch Unpublish Test ' . uniqid(), + 'type_id' => $event_type->getId(), + 'start_date' => $start_date->getTimestamp(), + 'end_date' => $end_date->getTimestamp(), + ]); + $event_id = $event->getId(); + $service->publishEvent(self::$summit, $event_id, []); + + try { + $service->unPublishEvents(self::$summit, ['events' => [$event_id, 999999999]]); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('999999999', $ex->getMessage()); + } + + self::$em->clear(); + $reFetched = self::$em->find(SummitEvent::class, $event_id); + $this->assertNotNull($reFetched); + $this->assertTrue($reFetched->isPublished()); + } + + /** + * SummitService::updateAndPublishEvents() (SummitService.php:1488) loops calling + * updateEvent() (:620, own nested transaction) then publishEvent() (:966, own nested + * transaction) per event - no try/catch. Item 1's update+publish commits fully; item 2's + * bad location_id then rolls back the whole call - including item 1's already-committed + * update+publish. + * + * The exception is thrown by updateEvent()'s underlying saveOrUpdateEvent() + * (SummitService.php:702-708, "location id %s does not exists!") - NOT by publishEvent()'s + * own, textually-identical location check (:999-1008). Since updateAndPublishEvents() + * passes the SAME $event_data to both calls and calls updateEvent() first (:1495-1496), + * saveOrUpdateEvent()'s unconditional (no isAllowsLocation gate) location-existence check + * always preempts publishEvent()'s gated one for any bad location_id in that payload - + * confirmed via code review during spec-verify; publishEvent()'s own check is not reachable + * through this call site. This still proves the pair's core claim (a batch item's nested-tx + * failure rolls back an EARLIER item's already-committed nested-tx work), just via + * updateEvent()'s nested transaction rather than publishEvent()'s specifically. + */ + public function testUpdateAndPublishEventsRollsBackAlreadyCommittedFirstItemWhenSecondItemsUpdateEventFailsOnBadLocationId() + { + $service = App::make(ISummitService::class); + $event_type = $this->createNonBlackoutEventType(); + [$start_date, $end_date] = $this->nonConflictingEventWindow(); + + // event1 has no location_id - it gets published within the batch call, and + // AbstractPublishService::validateBlackOutTimesAndTimes() only enforces blackout + // collisions when the event has a non-null location (see the sibling test above). + $original_title_1 = 'Original Title 1 ' . uniqid(); + $event1 = $service->addEvent(self::$summit, [ + 'title' => $original_title_1, + 'type_id' => $event_type->getId(), + 'start_date' => $start_date->getTimestamp(), + 'end_date' => $end_date->getTimestamp(), + ]); + $event1_id = $event1->getId(); + + $event2 = $service->addEvent(self::$summit, [ + 'title' => 'Event 2 ' . uniqid(), + 'type_id' => $event_type->getId(), + 'location_id' => self::$mainVenue->getId(), + 'start_date' => $start_date->getTimestamp(), + 'end_date' => $end_date->getTimestamp(), + ]); + $event2_id = $event2->getId(); + + try { + $service->updateAndPublishEvents(self::$summit, [ + 'events' => [ + [ + 'id' => $event1_id, + 'title' => 'Updated Title Should Not Persist', + 'start_date' => $start_date->getTimestamp(), + 'end_date' => $end_date->getTimestamp(), + ], + [ + 'id' => $event2_id, + 'start_date' => $start_date->getTimestamp(), + 'end_date' => $end_date->getTimestamp(), + 'location_id' => 999999999, + ], + ], + ]); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + $this->assertStringContainsString('location id', $ex->getMessage()); + } + + self::$em->clear(); + $reFetchedEvent1 = self::$em->find(SummitEvent::class, $event1_id); + $this->assertNotNull($reFetchedEvent1); + $this->assertEquals($original_title_1, $reFetchedEvent1->getTitle()); + $this->assertFalse($reFetchedEvent1->isPublished()); + } + + /** + * Wraps the real transaction service so the Nth transaction() call surfaces + * AmbiguousCommitException (a commit whose outcome is unknown), delegating + * every other call. Bound into the container BEFORE resolving the service so + * the autowired constructor receives it. + */ + private function bindAmbiguousCommitOnCall(int $failing_call): void + { + $inner = App::make(ITransactionService::class); + // ISummitService is a singleton that may already be resolved (with the real + // tx service) by the time the test body runs - drop the cached instance so + // the next App::make() rebuilds it against the wrapper bound below. + App::forgetInstance(ISummitService::class); + App::instance(ITransactionService::class, new class($inner, $failing_call) implements ITransactionService { + private $inner; + private $failing_call; + private $calls = 0; + public function __construct(ITransactionService $inner, int $failing_call) + { + $this->inner = $inner; + $this->failing_call = $failing_call; + } + public function transaction(\Closure $callback, int $isolationLevel = 2) + { + if (++$this->calls === $this->failing_call) + throw new AmbiguousCommitException('commit outcome unknown (simulated)'); + return $this->inner->transaction($callback, $isolationLevel); + } + }); + } + + /** + * A row whose transaction surfaces AmbiguousCommitException has an UNKNOWN + * outcome, so the event import must not treat it as processed or failed: + * remaining rows still run, but the source file must NOT be deleted, so the + * unknown-outcome row can be reconciled against the DB (mirrors + * SummitOrderService::processTicketData). + */ + public function testProcessEventDataKeepsFileWhenRowCommitOutcomeUnknown() + { + Storage::fake('swift'); + + // tx call #1 is the summit existence check; call #2 is row 1 + $this->bindAmbiguousCommitOnCall(2); + $service = App::make(ISummitService::class); + + $summit_id = self::$summit->getId(); + $type_name = self::$defaultEventType->getType(); + $track_title = self::$defaultTrack->getTitle(); + + $uid = uniqid(); + $title1 = sprintf('CSV Ambiguous Event 01 %s', $uid); + $title2 = sprintf('CSV Ambiguous Event 02 %s', $uid); + $csv_content = "type,track,title,description\n" + . sprintf("%s,%s,%s,Row one\n", $type_name, $track_title, $title1) + . sprintf("%s,%s,%s,Row two", $type_name, $track_title, $title2); + + $filename = 'events_ambiguous.csv'; + $path = "tmp/events_imports/{$filename}"; + Storage::disk('swift')->put($path, $csv_content); + + // must not throw: the unknown-outcome row is recorded, not fatal + $service->processEventData($summit_id, $filename, false); + + self::$em->clear(); + + $this->assertNull( + self::$em->getRepository(SummitEvent::class)->findOneBy(['title' => $title1]), + 'The unknown-outcome row must not have persisted an event' + ); + $this->assertNotNull( + self::$em->getRepository(SummitEvent::class)->findOneBy(['title' => $title2]), + 'Rows after the unknown-outcome one must still be processed' + ); + $this->assertTrue( + Storage::disk('swift')->exists($path), + 'The import file must be kept when any row has an unknown commit outcome' + ); + } + + /** + * Same contract for the registration companies import: an unknown-outcome row + * must keep the source file for reconciliation while later rows still run. + */ + public function testProcessRegistrationCompaniesDataKeepsFileWhenRowCommitOutcomeUnknown() + { + Storage::fake('swift'); + + // no pre-loop transaction here: tx call #1 is row 1 + $this->bindAmbiguousCommitOnCall(1); + $service = App::make(ISummitService::class); + + $summit_id = self::$summit->getId(); + $company1 = 'Ambiguous Row Company ' . uniqid(); + $company2 = 'Valid Row Company ' . uniqid(); + + $csv_content = <<put($path, $csv_content); + + // must not throw: the unknown-outcome row is recorded, not fatal + $service->processRegistrationCompaniesData($summit_id, $filename); + + self::$em->clear(); + self::$summit = self::$summit_repository->find($summit_id); + + $this->assertNull( + self::$summit->getRegistrationCompanyByName($company1), + 'The unknown-outcome row must not have attached a company' + ); + $this->assertNotNull( + self::$summit->getRegistrationCompanyByName($company2), + 'Rows after the unknown-outcome one must still be processed' + ); + $this->assertTrue( + Storage::disk('swift')->exists($path), + 'The import file must be kept when any row has an unknown commit outcome' + ); + } + + /** + * Volume happy path for processEventData(): every row of a 20-row CSV is valid + * (existing event type + track, title, description), so all 20 events must exist + * afterwards and the source file must be deleted. + */ + public function testProcessEventDataImportsAllRowsWhenEveryRowIsValid() + { + Storage::fake('swift'); + + $service = App::make(ISummitService::class); + $summit_id = self::$summit->getId(); + $type_name = self::$defaultEventType->getType(); + $track_title = self::$defaultTrack->getTitle(); + + $uid = uniqid(); + $titles = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + $title = sprintf('CSV Bulk Event %02d %s', $i, $uid); + $titles[] = $title; + $rows[] = sprintf('%s,%s,%s,Bulk import row %02d', $type_name, $track_title, $title, $i); + } + $csv_content = "type,track,title,description\n" . implode("\n", $rows); + + $filename = 'events_all_valid.csv'; + $path = "tmp/events_imports/{$filename}"; + Storage::disk('swift')->put($path, $csv_content); + + $service->processEventData($summit_id, $filename, false); + + self::$em->clear(); + + foreach ($titles as $title) { + $event = self::$em->getRepository(SummitEvent::class)->findOneBy(['title' => $title]); + $this->assertNotNull($event, sprintf('Event "%s" should have been imported', $title)); + $this->assertEquals($summit_id, $event->getSummitId()); + } + $this->assertFalse( + Storage::disk('swift')->exists($path), + 'A fully-processed import must delete its source file' + ); + } + + /** + * Volume mixed outcome for processEventData(): 20 rows where 15 carry a + * nonexistent event type (the row transaction throws EntityNotFoundException, + * the per-row catch logs and skips) interleaved with 5 valid rows. The 5 valid + * events must import, the 15 failing rows must leave nothing behind, the import + * must not throw, and the file is still deleted. + */ + public function testProcessEventDataImportsOnlyValidRowsWhenMostRowsFail() + { + Storage::fake('swift'); + + $service = App::make(ISummitService::class); + $summit_id = self::$summit->getId(); + $type_name = self::$defaultEventType->getType(); + $track_title = self::$defaultTrack->getTitle(); + + $uid = uniqid(); + $good_titles = []; + $bad_titles = []; + $rows = []; + for ($i = 1; $i <= 20; $i++) { + // every 4th row is valid: failures happen before and after each success + $is_good = ($i % 4 === 0); + $title = sprintf('CSV Mixed Event %02d %s', $i, $uid); + if ($is_good) $good_titles[] = $title; else $bad_titles[] = $title; + $rows[] = sprintf( + '%s,%s,%s,Mixed import row %02d', + $is_good ? $type_name : 'NON EXISTENT EVENT TYPE', + $track_title, + $title, + $i + ); + } + $csv_content = "type,track,title,description\n" . implode("\n", $rows); + + $filename = 'events_mixed.csv'; + $path = "tmp/events_imports/{$filename}"; + Storage::disk('swift')->put($path, $csv_content); + + // must not throw: failing rows are logged and skipped + $service->processEventData($summit_id, $filename, false); + + self::$em->clear(); + + $this->assertCount(5, $good_titles); + $this->assertCount(15, $bad_titles); + foreach ($good_titles as $title) { + $this->assertNotNull( + self::$em->getRepository(SummitEvent::class)->findOneBy(['title' => $title]), + sprintf('Valid row event "%s" should have been imported', $title) + ); + } + foreach ($bad_titles as $title) { + $this->assertNull( + self::$em->getRepository(SummitEvent::class)->findOneBy(['title' => $title]), + sprintf('Failing row event "%s" should not exist', $title) + ); + } + $this->assertFalse( + Storage::disk('swift')->exists($path), + 'Known per-row failures must not block the source file deletion' + ); + } +} diff --git a/tests/SummitSubmissionInvitationServiceTest.php b/tests/SummitSubmissionInvitationServiceTest.php new file mode 100644 index 000000000..8d8a7ac63 --- /dev/null +++ b/tests/SummitSubmissionInvitationServiceTest.php @@ -0,0 +1,120 @@ +importInvitationData(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + foreach ($emails as $email) { + $this->assertNotNull( + self::$summit->getSubmissionInvitationByEmail($email), + sprintf('Submission invitation %s should have been imported', $email) + ); + } + } + + /** + * Volume upsert semantics: 20 rows spanning only 5 distinct emails (each repeated + * 4 times with a different first_name). The import treats a repeated email as an + * UPDATE of the existing invitation - not a failure - so exactly 5 invitations + * must exist afterwards, each carrying the LAST row's first_name for its email. + */ + public function testImportInvitationDataUpsertsDuplicateEmailsInsteadOfDuplicating() + { + $service = App::make(ISummitSubmissionInvitationService::class); + + $uid = uniqid(); + $emails = []; + for ($i = 1; $i <= 5; $i++) { + $emails[] = sprintf('sub-inv-dup-%02d-%s@test.com', $i, $uid); + } + + $rows = []; + for ($pass = 1; $pass <= 4; $pass++) { + foreach ($emails as $idx => $email) { + $rows[] = sprintf('%s,Pass%02d,Row%02d', $email, $pass, $idx + 1); + } + } + $this->assertCount(20, $rows); + $csv = "email,first_name,last_name\n" . implode("\n", $rows); + + // must not throw: repeated emails take the update path + $service->importInvitationData(self::$summit, $this->buildCsvUpload($csv)); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + + foreach ($emails as $email) { + $invitation = self::$summit->getSubmissionInvitationByEmail($email); + $this->assertNotNull($invitation, sprintf('Submission invitation %s should exist exactly once', $email)); + $this->assertEquals( + 'Pass04', + $invitation->getFirstName(), + sprintf('Invitation %s should carry the LAST row\'s first_name (update path)', $email) + ); + } + } +} diff --git a/tests/Unit/Services/DoctrineTransactionServiceTest.php b/tests/Unit/Services/DoctrineTransactionServiceTest.php new file mode 100644 index 000000000..0e889c200 --- /dev/null +++ b/tests/Unit/Services/DoctrineTransactionServiceTest.php @@ -0,0 +1,1315 @@ +container = new \Illuminate\Container\Container(); + $this->container->instance('app', $this->container); + $this->container->instance('log', new class { + public function __call($name, $args) { /* swallow */ } + }); + Facade::setFacadeApplication($this->container); + } + + protected function tearDown(): void + { + Facade::clearResolvedInstances(); + Facade::setFacadeApplication(null); + Mockery::close(); + parent::tearDown(); + } + + // ───────────────────────────────────────────────────────────────────────── + // Helpers + // ───────────────────────────────────────────────────────────────────────── + + /** + * Build mocked EM + Connection and register them in the facade. + * + * @param bool $transactionActive Initial state of isTransactionActive() + * @return array{EntityManagerInterface&\Mockery\MockInterface, Connection&\Mockery\MockInterface, ManagerRegistry&\Mockery\MockInterface} + */ + private function buildMocks(bool $transactionActive = false): array + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isTransactionActive')->andReturn($transactionActive)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->byDefault(); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault(); + + $this->container->instance(ManagerRegistry::class, $registry); + + return [$em, $conn, $registry]; + } + + // ───────────────────────────────────────────────────────────────────────── + // ROOT TRANSACTION TESTS + // ───────────────────────────────────────────────────────────────────────── + + public function testRootTransactionCommitsAndFlushes(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: false); + + $conn->shouldReceive('setTransactionIsolation') + ->once() + ->with(TransactionIsolationLevel::READ_COMMITTED); + $conn->shouldReceive('beginTransaction')->once(); + $em->shouldReceive('flush')->once(); + $conn->shouldReceive('commit')->once(); + + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () { + return 'success'; + }); + + $this->assertSame('success', $result); + } + + /** + * DBAL savepoints are never enabled: nested transactions rely entirely on + * DBAL's native isRollbackOnly propagation (rollBack() at nesting level > 1 + * without savepoints just marks the connection + decrements the counter; + * commit() at ANY level checks isRollbackOnly first) instead of savepoints, + * which desynchronize Doctrine's UnitOfWork from the database. + */ + public function testRootTransactionNeverEnablesSavepoints(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: false); + + $conn->shouldReceive('setNestTransactionsWithSavepoints')->never(); + $conn->shouldReceive('setTransactionIsolation') + ->once() + ->with(TransactionIsolationLevel::READ_COMMITTED); + $conn->shouldReceive('beginTransaction')->once(); + $em->shouldReceive('flush')->once(); + $conn->shouldReceive('commit')->once(); + + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () { + return 'success'; + }); + + $this->assertSame('success', $result); + } + + public function testRootTransactionRollsBackOnNonRetryableException(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: false); + + $conn->shouldReceive('beginTransaction')->once(); + // false: routing check picks the ROOT path; true: rollback check in the inner catch + // (a blanket `true` here would silently reroute this test to the nested path) + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('rollBack')->once(); + $conn->shouldReceive('commit')->never(); + $em->shouldReceive('flush')->never(); + + $service = new DoctrineTransactionService('default'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('business error'); + + $service->transaction(function () { + throw new \RuntimeException('business error'); + }); + } + + public function testRootTransactionRetriesOnConnectionLost(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->byDefault(); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + // First call: isTransactionActive returns false (root detection) + // then returns true inside the inner catch for rollback check + $txActiveSequence = [false, true, false]; + $txActiveIndex = 0; + $conn->shouldReceive('isTransactionActive')->andReturnUsing( + function () use (&$txActiveSequence, &$txActiveIndex) { + $val = $txActiveSequence[$txActiveIndex] ?? false; + $txActiveIndex++; + return $val; + } + ); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->andReturn($em); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () use (&$callCount) { + $callCount++; + if ($callCount === 1) { + throw new TestRetryableException('Connection lost'); + } + return 'recovered'; + }); + + $this->assertSame('recovered', $result); + $this->assertSame(2, $callCount, 'Callback should be retried after connection lost'); + } + + public function testRootTransactionResetsManagerOnConnectionError(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isTransactionActive')->andReturn(false, true, false); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->byDefault(); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->atLeast()->once(); // destructive recovery (also cleared by the root inner catch) + $em->shouldReceive('close')->once(); // destructive recovery + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + $service->transaction(function () use (&$callCount) { + $callCount++; + if ($callCount === 1) { + throw new TestRetryableException('gone'); + } + return 'ok'; + }); + + $this->assertSame(2, $callCount); + } + + /** + * transaction()'s root-vs-nested routing must not trust conn->isTransactionActive() + * alone: a closed EntityManager while its connection still reports an active + * transaction means a nested failure was caught mid-propagation (the outer + * transaction() frame has not unwound yet). Routing that state into + * runNestedTransaction() would hand the callback a dead EntityManager, and + * routing it into runRootTransaction() would be worse: resetManager() builds a + * brand-new EntityManager AND a brand-new DBAL connection, so the "recovered" + * root would commit durable writes on the fresh connection while the outer, + * rollback-only transaction on the old connection rolls back - a split-brain + * partial commit. The only safe move is to refuse and let the original nested + * failure propagate to the root. + */ + public function testTransactionRefusesWhenEntityManagerClosedWithActiveTransaction(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isTransactionActive')->andReturn(true); + $conn->shouldReceive('beginTransaction')->never(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(false); // closed + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->never(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + return 'must-never-run'; + }); + $this->fail('Expected refusal when the EM is closed while a transaction is still active'); + } catch (\RuntimeException $e) { + $this->assertStringContainsString('refusing to start an independent root transaction', $e->getMessage()); + $this->assertSame(0, $callCount, 'Callback must never execute in this state'); + } + } + + // ───────────────────────────────────────────────────────────────────────── + // NESTED TRANSACTION TESTS + // ───────────────────────────────────────────────────────────────────────── + + public function testNestedTransactionFlushesAndCommits(): void + { + // Connection already has an active transaction → detected as nested + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $conn->shouldReceive('beginTransaction')->once(); + $em->shouldReceive('flush')->once(); + $conn->shouldReceive('commit')->once(); + // Must NOT set isolation level in nested mode + $conn->shouldReceive('setTransactionIsolation')->never(); + + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () { + return 'nested-result'; + }); + + $this->assertSame('nested-result', $result); + } + + /** + * Nested transactions never inspect the connection's savepoint flag - the + * savepoints-disabled warning mechanism (Finding 2) is removed entirely, + * since nested no longer relies on savepoints at all. + */ + public function testNestedTransactionNeverQueriesSavepointsFlag(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $conn->shouldReceive('getNestTransactionsWithSavepoints')->never(); + $conn->shouldReceive('beginTransaction')->once(); + $em->shouldReceive('flush')->once(); + $conn->shouldReceive('commit')->once(); + + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () { + return 'nested-result'; + }); + + $this->assertSame('nested-result', $result); + } + + public function testNestedTransactionDoesNotRetryOnConnectionError(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $conn->shouldReceive('isTransactionActive')->andReturn(true); + $conn->shouldReceive('rollBack')->once(); + + // Must NOT reset manager or close connection in nested + $registry = $this->container->make(ManagerRegistry::class); + $registry->shouldReceive('resetManager')->never(); + $conn->shouldReceive('close')->never(); + $em->shouldReceive('close')->never(); + $em->shouldReceive('clear')->never(); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + throw new TestRetryableException('inner connection lost'); + }); + $this->fail('Expected exception was not thrown'); + } catch (TestRetryableException $e) { + // expected + } + + $this->assertSame(1, $callCount, 'Nested transaction must NOT retry'); + } + + public function testNestedTransactionRollsBackAndRethrowsOnError(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $conn->shouldReceive('isTransactionActive')->andReturn(true); + $conn->shouldReceive('rollBack')->once(); + $conn->shouldReceive('commit')->never(); + $em->shouldReceive('flush')->never(); // flush not reached on error + $em->shouldReceive('clear')->never(); // nested must never discard the outer's in-flight UoW state + + $service = new DoctrineTransactionService('default'); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('inner failure'); + + $service->transaction(function () { + throw new \LogicException('inner failure'); + }); + } + + /** + * Mirrors testRootTransactionFailsFastWhenCallbackClosedEntityManager for + * the nested path (finding #9): with 2+ levels of nesting, a deeper + * nested flush failure closes the EntityManager; if a middle callback + * catches that and continues, THIS nested transaction must also fail + * fast with a clear error - not proceed to flush() and surface an + * opaque EntityManagerClosed one level higher than before. + * + * isOpen() sequence: true (transaction() routing sees the still-open EM, + * so this call runs as NESTED), then false (the deeper flush failure has + * closed the EM by the time the post-callback guard runs). + */ + public function testNestedTransactionFailsFastWhenDeeperNestedFlushClosedEntityManager(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $em->shouldReceive('isOpen')->andReturn(true, false); + $conn->shouldReceive('isTransactionActive')->andReturn(true); + $conn->shouldReceive('rollBack')->once(); + $em->shouldReceive('flush')->never(); + $conn->shouldReceive('commit')->never(); + + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () { + return 'swallowed-deeper-failure'; + }); + $this->fail('Expected fail-fast RuntimeException was not thrown'); + } catch (\RuntimeException $e) { + $this->assertStringContainsString('EntityManager was closed', $e->getMessage()); + } + } + + // ───────────────────────────────────────────────────────────────────────── + // DIRECT NESTING: inner transaction() inside outer transaction() + // Simulates the email-send pattern (SummitPromoCodeService, etc.) + // ───────────────────────────────────────────────────────────────────────── + + /** + * Simulates the pattern at SummitPromoCodeService.php:980-998, + * SummitRSVPInvitationService.php:448-458, etc. + * + * Outer transaction starts (root), inner transaction is called from within + * the outer's closure. Inner must detect the active transaction and run + * as nested (no retry, no EM reset). + */ + public function testDirectNestingInnerRunsAsNested(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->once(); // only root sets isolation + $conn->shouldReceive('close')->byDefault(); + + // Track transaction active state: false initially (root detection), + // then true once beginTransaction is called (for nested detection) + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->andReturnUsing(function () use (&$txActive) { + // Only outer commit sets txActive to false + // In real Doctrine this decrements a counter, simulate with simple bool + }); + $conn->shouldReceive('rollBack')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + $innerExecuted = false; + $result = $service->transaction(function ($tx) use ($service, &$innerExecuted) { + // Simulates the inner transaction (e.g. getByIdExclusiveLock pattern) + $innerResult = $service->transaction(function () use (&$innerExecuted) { + $innerExecuted = true; + return 'promo_code_entity'; + }); + + // Outer continues with the result (e.g. dispatches email) + return "dispatched:{$innerResult}"; + }); + + $this->assertTrue($innerExecuted); + $this->assertSame('dispatched:promo_code_entity', $result); + } + + /** + * Direct nesting: inner transaction throws — exception propagates to outer, + * outer catches and the root handles the full rollback. + * No EM reset or connection close happens during inner failure. + * + * This test verifies structural invariants only (no EM reset, no + * connection close). It does NOT verify that this outer-catches-and- + * continues pattern is safe: on a real DBAL connection, the inner + * rollBack() sets isRollbackOnly=true, so the root's eventual commit() + * would throw ConnectionException::commitFailedRollbackOnly (or the + * ORM's OptimisticLockException wrapping it) instead of silently + * succeeding - this mock does not model isRollbackOnly, so it cannot + * assert that. See the plan's real-MySQL verification for that proof. + */ + public function testDirectNestingInnerErrorPropagatesWithoutDestroyingEM(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('close')->never(); // inner must NOT close connection + + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->never(); // inner must NOT clear EM + $em->shouldReceive('close')->never(); // inner must NOT close EM + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->never(); // inner must NOT reset + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + $outerCaughtException = null; + $result = $service->transaction(function () use ($service, &$outerCaughtException) { + try { + $service->transaction(function () { + throw new \RuntimeException('lock acquisition failed'); + }); + } catch (\RuntimeException $e) { + $outerCaughtException = $e; + // Outer catches and continues (e.g. skips email dispatch) + return 'skipped'; + } + }); + + $this->assertNotNull($outerCaughtException); + $this->assertSame('lock acquisition failed', $outerCaughtException->getMessage()); + $this->assertSame('skipped', $result); + } + + /** + * Direct nesting with connection error in inner: inner must NOT retry + * and must NOT do destructive recovery. Exception propagates to root + * which then handles the reconnect logic. + * + * This is the exact scenario that was broken in the old code: + * inner connection error would close/reset EM, destroying the outer TX. + */ + public function testDirectNestingConnectionErrorInInnerDoesNotDestroyOuterTx(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + // resetManager will be called by the ROOT's retry, not by the nested tx + $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + $outerCallCount = 0; + $innerCallCount = 0; + + // The outer root transaction should catch the ConnectionLost from the inner, + // and since it propagates through the root's inner try/catch, + // the root will retry with reconnection. + $result = $service->transaction(function () use ($service, &$outerCallCount, &$innerCallCount) { + $outerCallCount++; + if ($outerCallCount === 1) { + // First outer attempt: inner throws connection error + $service->transaction(function () use (&$innerCallCount) { + $innerCallCount++; + throw new TestRetryableException('packets out of order'); + }); + } + // Second outer attempt (after retry): succeeds + return 'recovered'; + }); + + $this->assertSame('recovered', $result); + $this->assertSame(2, $outerCallCount, 'Root should retry after inner connection error'); + $this->assertSame(1, $innerCallCount, 'Inner must NOT retry — only called once'); + } + + // ───────────────────────────────────────────────────────────────────────── + // INDIRECT NESTING: service method that opens its own transaction + // called from within another transaction + // ───────────────────────────────────────────────────────────────────────── + + /** + * Simulates SummitOrderService.php:2287 calling CompanyService::addCompany() + * from within its own transaction. addCompany() opens its own transaction() + * which should run as nested. + */ + public function testIndirectNestingServiceCallRunsAsNested(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->once(); // only root + $conn->shouldReceive('close')->byDefault(); + + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + // Simulate CompanyService::addCompany() — has its own transaction() call + $addCompany = function () use ($service) { + return $service->transaction(function () { + // Simulates: CompanyFactory::build + repository->add + return company + return (object)['id' => 99, 'name' => 'Test Corp']; + }); + }; + + // Simulate SummitOrderService outer transaction calling addCompany + $result = $service->transaction(function () use ($addCompany) { + $company = $addCompany(); + // Caller uses the result (attendee->setCompany) + return "assigned:{$company->name}"; + }); + + $this->assertSame('assigned:Test Corp', $result); + } + + /** + * Simulates SummitService.php:3079 calling SpeakerService::addSpeaker() + * from within a transaction. The inner addSpeaker() transaction flushes + * (so IDs are available) but doesn't retry or reset EM. + */ + public function testIndirectNestingInnerFlushGeneratesIds(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + + // Track flush calls — both root and nested should flush + $flushCount = 0; + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->andReturnUsing(function () use (&$flushCount) { + $flushCount++; + }); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + // Simulate SpeakerService::addSpeaker — its own transaction + $addSpeaker = function (array $data) use ($service) { + return $service->transaction(function () use ($data) { + // After flush, auto-increment ID would be available + return (object)['id' => 42, 'email' => $data['email']]; + }); + }; + + $result = $service->transaction(function () use ($addSpeaker) { + $speaker = $addSpeaker(['email' => 'speaker@example.com']); + // Caller uses speaker ID immediately + return "speaker_id:{$speaker->id}"; + }); + + $this->assertSame('speaker_id:42', $result); + // Nested flush (addSpeaker) + root flush = 2 total + $this->assertSame(2, $flushCount, 'Both nested and root should flush'); + } + + /** + * Simulates SponsorUserSyncService.php:171 calling + * SummitSponsorService::addSponsorUser() which fails. + * The inner error propagates to the outer without destroying EM. + */ + public function testIndirectNestingInnerServiceErrorDoesNotDestroyOuter(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->never(); // critical: must NOT reset + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + // Simulate addSponsorUser that throws validation error + $addSponsorUser = function () use ($service) { + return $service->transaction(function () { + throw new \InvalidArgumentException('Sponsor not found.'); + }); + }; + + // Outer transaction catches the inner failure and handles gracefully + $result = $service->transaction(function () use ($addSponsorUser) { + try { + $addSponsorUser(); + } catch (\InvalidArgumentException $e) { + return "handled:{$e->getMessage()}"; + } + return 'unreachable'; + }); + + $this->assertSame('handled:Sponsor not found.', $result); + } + + /** + * Simulates ScheduleService.php:510 calling SummitService::publishEvent() + * from within a transaction. publishEvent() has its own transaction that + * runs as nested — verifies the full happy-path flow with multiple + * indirect nested calls in sequence. + */ + public function testIndirectNestingMultipleNestedCallsInSequence(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('setTransactionIsolation')->once(); // only root + $conn->shouldReceive('close')->byDefault(); + + $txActive = false; + $conn->shouldReceive('isTransactionActive')->andReturnUsing(function () use (&$txActive) { + return $txActive; + }); + $conn->shouldReceive('beginTransaction')->andReturnUsing(function () use (&$txActive) { + $txActive = true; + }); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->never(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + // Simulate multiple service calls (TagService::addTag, SpeakerService::addSpeaker) + $addTag = function (string $name) use ($service) { + return $service->transaction(function () use ($name) { + return (object)['id' => rand(1, 100), 'tag' => $name]; + }); + }; + + $addSpeaker = function (string $email) use ($service) { + return $service->transaction(function () use ($email) { + return (object)['id' => rand(1, 100), 'email' => $email]; + }); + }; + + $publishEvent = function () use ($service) { + return $service->transaction(function () { + return 'published'; + }); + }; + + // Outer transaction makes multiple service calls (simulates SummitSubmissionInvitationService) + $result = $service->transaction(function () use ($addTag, $addSpeaker, $publishEvent) { + $tag1 = $addTag('cloud'); + $tag2 = $addTag('kubernetes'); + $speaker = $addSpeaker('speaker@test.org'); + $status = $publishEvent(); + return "{$tag1->tag},{$tag2->tag},{$speaker->email},{$status}"; + }); + + $this->assertSame('cloud,kubernetes,speaker@test.org,published', $result); + } + + // ───────────────────────────────────────────────────────────────────────── + // EDGE CASES + // ───────────────────────────────────────────────────────────────────────── + + /** + * Root transaction exhausts max retries on persistent connection errors. + */ + public function testRootTransactionThrowsAfterMaxRetries(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->byDefault(); + $conn->shouldReceive('commit')->never(); + $conn->shouldReceive('rollBack')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->never(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->andReturn($em)->byDefault(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + throw new TestRetryableException('persistent failure'); + }); + $this->fail('Expected retryable exception was not thrown'); + } catch (TestRetryableException $e) { + // Should have been called MaxRetries times + $this->assertSame(DoctrineTransactionService::MaxRetries, $callCount); + } + } + + /** + * A connection failure during the root COMMIT itself is ambiguous: the server + * may have already made the transaction durable even though the client never + * received the acknowledgment (ack-lost scenario). Retrying would re-execute + * the whole callback and duplicate every write and side effect (orders, + * tickets, emails), so commit-phase failures must bypass the reconnect/retry + * path and propagate immediately. + * + * Because the failure is connection-level, the dead physical handle must not + * stay registered either: the manager/connection pair is discarded (closed + + * resetManager) so subsequent work on this worker - including direct Registry + * reads outside transaction() - gets a live pair. Cleanup only: still no retry. + * + * The failure surfaces as AmbiguousCommitException (original as previous): + * the in-service guard alone can't stop the layer above (Laravel queue + * tries, caller-side retries) from re-executing the whole callback on a + * retryable-looking driver exception - the marker type is what lets a job + * handler fail() without retry. It must never match shouldReconnect(). + */ + public function testRootTransactionDoesNotRetryWhenCommitFails(): void + { + [$em, $conn, $registry] = $this->buildMocks(transactionActive: false); + + // DBAL decrements the nesting level in a finally block even when the + // physical COMMIT fails, so by the time the exception is caught no + // transaction is active - buildMocks' byDefault(false) models this. + $original = new TestRetryableException('server has gone away during COMMIT'); + $conn->shouldReceive('commit') + ->once() + ->andThrow($original); + $conn->shouldReceive('rollBack')->never(); + $conn->shouldReceive('close')->once(); + + $em->shouldReceive('close')->once(); + $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + return 'ok'; + }); + $this->fail('Expected the commit-phase failure to propagate'); + } catch (AmbiguousCommitException $e) { + $this->assertSame(1, $callCount, 'Callback must not be re-executed after an ambiguous commit failure'); + $this->assertSame($original, $e->getPrevious(), 'The driver exception must be preserved as previous'); + $this->assertStringContainsString('commit outcome unknown', $e->getMessage()); + $this->assertFalse($service->shouldReconnect($e), 'The marker type must never classify as retryable'); + } + } + + /** + * The commit-phase ambiguity guard must not swallow DETERMINISTIC client-side + * commit failures. DBAL's Connection::commit() checks its rollback-only flag + * FIRST and throws ConnectionException::commitFailedRollbackOnly() before the + * COMMIT is ever sent to the server (DBAL 3.9.4, Connection.php) - at that + * point the transaction is guaranteed NOT committed, so classifying it as + * "outcome unknown" (AmbiguousCommitException) would send callers on false + * reconciliation work for a plain, fully-rolled-back failure. + * + * Reachable when a nested transaction rolled back (marking the connection + * rollback-only), an intermediate callback caught the failure and continued, + * and the root flush() had an empty changeset (UnitOfWork's "Nothing to do" + * early return never touches the connection), so the root commit() is the + * first commit call in the whole chain. + * + * Expected: fail fast BEFORE attempting the real COMMIT, with a plain + * \RuntimeException naming the rollback-only cause - never + * AmbiguousCommitException, and never a retry. + */ + public function testRootTransactionFailsDeterministicallyWhenConnectionIsRollbackOnly(): void + { + $conn = Mockery::mock(Connection::class); + // false: routing check picks the ROOT path; true: rollback check in the inner catch + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->once(); + // Poisoned by a nested rollback whose exception was caught mid-chain + // (the catch-and-continue anti-pattern documented in ADR-003). + $conn->shouldReceive('isRollbackOnly')->andReturn(true); + // Without the pre-commit guard, the real commit() throws DBAL's + // client-side rollback-only error - model it so the misclassification + // reproduces; with the guard in place this is never reached. + $conn->shouldReceive('commit')->andThrow(ConnectionException::commitFailedRollbackOnly()); + // The transaction is still active, so the real ROLLBACK runs and succeeds. + $conn->shouldReceive('rollBack')->once(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true); + // Empty changeset: flush() returns without touching the connection. + $em->shouldReceive('flush')->once(); + $em->shouldReceive('clear')->once(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + // Deterministic business-level failure on a live pair: no destructive recovery. + $registry->shouldReceive('resetManager')->never(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + return 'ok'; + }); + $this->fail('Expected the rollback-only failure to propagate'); + } catch (\RuntimeException $e) { + $this->assertNotInstanceOf( + AmbiguousCommitException::class, + $e, + 'A client-side rollback-only failure is deterministic (nothing was sent to the server) and must not be classified as ambiguous' + ); + $this->assertStringContainsString('rollback-only', $e->getMessage()); + } + $this->assertSame(1, $callCount, 'A deterministic rollback-only failure must never trigger the retry loop'); + } + + /** + * When the ROOT rollback itself fails (typically the connection died during + * the callback), safeRollback() must keep the original business exception - + * but the manager/connection pair is now in an unknown state: DBAL zeroes the + * nesting level before the physical rollback and only clears its rollback-only + * flag after it succeeds, so the registry would otherwise keep an open EM + * wired to a dead handle. Direct Registry consumers (repositories, serializers, + * queue jobs reading outside transaction()) have no retry path and would fail + * in a chain on a long-lived worker. The pair must be discarded: EM closed, + * connection closed, fresh manager reset into the registry - while the + * ORIGINAL exception still propagates and no retry happens. + */ + public function testRootTransactionDiscardsManagerWhenRollbackFails(): void + { + $conn = Mockery::mock(Connection::class); + // false: routing check picks the ROOT path; true: rollback check in the inner catch + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->once(); + $conn->shouldReceive('commit')->never(); + $conn->shouldReceive('rollBack') + ->once() + ->andThrow(new TestRetryableException('connection died during rollback')); + $conn->shouldReceive('close')->once(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + // Stays open throughout: a business error is not a flush failure + $em->shouldReceive('isOpen')->andReturn(true); + $em->shouldReceive('flush')->never(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->once(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + throw new \LogicException('business error with dying connection'); + }); + $this->fail('Expected the original business exception to propagate'); + } catch (\LogicException $e) { + $this->assertStringContainsString('business error with dying connection', $e->getMessage()); + } + $this->assertSame(1, $callCount, 'A rollback failure must never trigger the retry loop'); + } + + /** + * Nested transaction that returns null — ensures null is properly propagated. + * Covers the SummitPromoCodeService pattern where inner TX returns null + * when promo code is not the expected type. + */ + public function testNestedTransactionReturnsNull(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () { + return null; + }); + + $this->assertNull($result); + } + + /** + * Root transaction with closed EntityManager — should reset before proceeding. + */ + public function testRootTransactionResetsClosedEntityManager(): void + { + $conn = Mockery::mock(Connection::class); + $conn->shouldReceive('isTransactionActive')->andReturn(false); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->byDefault(); + $conn->shouldReceive('isRollbackOnly')->andReturn(false)->byDefault(); + $conn->shouldReceive('commit')->byDefault(); + $conn->shouldReceive('rollBack')->byDefault(); + $conn->shouldReceive('close')->byDefault(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(false); // EM is closed + $em->shouldReceive('flush')->byDefault(); + $em->shouldReceive('clear')->byDefault(); + $em->shouldReceive('close')->byDefault(); + + $freshEm = Mockery::mock(EntityManagerInterface::class); + $freshEm->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $freshEm->shouldReceive('isOpen')->andReturn(true); + $freshEm->shouldReceive('flush')->once(); + $freshEm->shouldReceive('clear')->byDefault(); + $freshEm->shouldReceive('close')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($freshEm); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + $result = $service->transaction(function () { + return 'from-fresh-em'; + }); + + $this->assertSame('from-fresh-em', $result); + } + + /** + * A non-retryable failure in a ROOT transaction must discard the + * UnitOfWork state ($em->clear()) after rolling back, so pending + * persists/changesets from the failed callback cannot leak into the + * NEXT transaction on the same EntityManager (phantom writes in + * catch-and-continue loops), while staying non-destructive on the + * connection/manager (no resetManager, no close). + */ + public function testRootTransactionDiscardsPendingEntitiesAfterNonRetryableFailure(): void + { + $conn = Mockery::mock(Connection::class); + // false: routing check picks the ROOT path; true: rollback check in the inner catch + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->once(); + $conn->shouldReceive('commit')->never(); + $conn->shouldReceive('rollBack')->once(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + $em->shouldReceive('isOpen')->andReturn(true)->byDefault(); + $em->shouldReceive('flush')->never(); + $em->shouldReceive('clear')->once(); // the UoW discard under test + $em->shouldReceive('close')->never(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->never(); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage('business error'); + + $service->transaction(function () { + throw new \RuntimeException('business error'); + }); + } + + /** + * A callback that swallowed a nested flush failure returns normally with a + * CLOSED EntityManager (ORM closes it on any failed flush). The root must + * fail fast with a clear error BEFORE its own flush - instead of the opaque + * EntityManagerClosed - must NOT retry, and must leave a live manager in + * the registry (closed-EM reset branch). + */ + public function testRootTransactionFailsFastWhenCallbackClosedEntityManager(): void + { + $conn = Mockery::mock(Connection::class); + // false: routing check picks the ROOT path; true: rollback check in the inner catch + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->once(); + $conn->shouldReceive('commit')->never(); + $conn->shouldReceive('rollBack')->once(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + // true: transaction() routing check; true: loop-top open check; false: post-callback + // fail-fast check (repeats false so the closed-EM resetManager branch in the outer + // catch fires too) + $em->shouldReceive('isOpen')->andReturn(true, true, false); + $em->shouldReceive('flush')->never(); + $em->shouldReceive('clear')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em); + + $this->container->instance(ManagerRegistry::class, $registry); + + $callCount = 0; + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () use (&$callCount) { + $callCount++; + return 'swallowed-inner-failure'; + }); + $this->fail('Expected fail-fast RuntimeException was not thrown'); + } catch (\RuntimeException $e) { + $this->assertStringContainsString('EntityManager was closed', $e->getMessage()); + } + $this->assertSame(1, $callCount, 'Fail-fast must not trigger the retry loop'); + } + + /** + * A rollback failure in the NESTED path must never replace the callback's + * original exception: masking it would let the root classify the rollback + * error (often reconnectable) instead of the business error, re-running + * non-idempotent side effects on retry. + */ + public function testNestedTransactionPreservesCallbackExceptionWhenRollbackFails(): void + { + [$em, $conn] = $this->buildMocks(transactionActive: true); + + $conn->shouldReceive('isTransactionActive')->andReturn(true); + $conn->shouldReceive('rollBack')->once()->andThrow(new TestRetryableException('connection died during savepoint rollback')); + $conn->shouldReceive('commit')->never(); + $em->shouldReceive('flush')->never(); + + $service = new DoctrineTransactionService('default'); + + try { + $service->transaction(function () { + throw new \LogicException('inner business error'); + }); + $this->fail('Expected the callback exception to propagate'); + } catch (\LogicException $e) { + $this->assertStringContainsString('inner business error', $e->getMessage()); + } + } + + /** + * PHP \Error throwables from the callback must still reach the closed-EM + * recovery branch: a TypeError with a closed EM must reset the manager so + * direct Registry consumers get a live one afterwards. + */ + public function testRootTransactionRecoversClosedManagerWhenCallbackThrowsError(): void + { + $conn = Mockery::mock(Connection::class); + // false: routing check picks the ROOT path; true: rollback check in the inner catch + $conn->shouldReceive('isTransactionActive')->andReturn(false, true); + $conn->shouldReceive('setTransactionIsolation')->byDefault(); + $conn->shouldReceive('beginTransaction')->once(); + $conn->shouldReceive('commit')->never(); + $conn->shouldReceive('rollBack')->once(); + + $em = Mockery::mock(EntityManagerInterface::class); + $em->shouldReceive('getConnection')->andReturn($conn)->byDefault(); + // true: transaction() routing check; true: loop-top check; false: closed-EM + // recovery check in the outer catch + $em->shouldReceive('isOpen')->andReturn(true, true, false); + $em->shouldReceive('flush')->never(); + $em->shouldReceive('clear')->byDefault(); + + $registry = Mockery::mock(ManagerRegistry::class); + $registry->shouldReceive('getManager')->with('default')->andReturn($em)->byDefault(); + $registry->shouldReceive('resetManager')->with('default')->once()->andReturn($em); + + $this->container->instance(ManagerRegistry::class, $registry); + + $service = new DoctrineTransactionService('default'); + + $this->expectException(\TypeError::class); + $this->expectExceptionMessage('boom'); + + $service->transaction(function () { + throw new \TypeError('boom'); + }); + } +} diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 68a3acef6..9303e6b94 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -14,6 +14,7 @@ use App\Models\Foundation\Main\IGroup; use App\Services\Model\ISponsorUserSyncService; +use models\exceptions\EntityNotFoundException; use Tests\InsertMemberTestData; use Tests\InsertSummitTestData; use Tests\TestCase; @@ -89,6 +90,16 @@ private function getPermissions(int $sponsor_id, int $member_id): array return json_decode($raw, true) ?? []; } + private function assertNoSponsorUsersRowExists(int $sponsor_id, int $member_id): void + { + $conn = self::$em->getConnection(); + $exists = $conn->executeQuery( + 'SELECT COUNT(*) FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?', + [$sponsor_id, $member_id] + )->fetchOne(); + $this->assertEquals(0, (int)$exists, 'Pre-condition: no Sponsor_Users row should exist'); + } + // ------------------------------------------------------------------------- // addSponsorUserToGroup // ------------------------------------------------------------------------- @@ -108,14 +119,8 @@ public function testAddSponsorUserToGroupEagerlyCreatesRowAndWritesPermissionOnR $external_id = self::$member->getUserExternalId(); $summit_id = self::$summit->getId(); - $conn = self::$em->getConnection(); - // Confirm no row exists before the call. - $exists = $conn->executeQuery( - 'SELECT COUNT(*) FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?', - [$sponsor_id, $member_id] - )->fetchOne(); - $this->assertEquals(0, (int)$exists, 'Pre-condition: no Sponsor_Users row should exist'); + $this->assertNoSponsorUsersRowExists($sponsor_id, $member_id); $this->getService()->addSponsorUserToGroup($external_id, IGroup::Sponsors, $sponsor_id, $summit_id); @@ -123,6 +128,44 @@ public function testAddSponsorUserToGroupEagerlyCreatesRowAndWritesPermissionOnR $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member_id)); } + /** + * SponsorUserSyncService::addSponsorUserToGroup() (SponsorUserSyncService.php:147) calls + * SummitSponsorService::addSponsorUser() (SummitSponsorService.php:459, its own nested + * transaction) at :171 when no Sponsor_Users row exists yet - no try/catch. That nested + * call commits a real Sponsor_Users row (same eager-creation path as the test above, this + * time with a valid sponsor). If the outer's own subsequent group lookup then fails + * (EntityNotFoundException at :191 for an unresolvable group_slug), the entire + * addSponsorUserToGroup call rolls back, including the just-committed Sponsor_Users row. + */ + public function testAddSponsorUserToGroupRollsBackAlreadyCommittedSponsorUserRowWhenGroupNotFound(): void + { + $sponsor_id = self::$sponsors[1]->getId(); + $member_id = self::$member->getId(); + $external_id = self::$member->getUserExternalId(); + $summit_id = self::$summit->getId(); + $bogus_group_slug = 'no-such-group-' . uniqid(); + + $this->assertNoSponsorUsersRowExists($sponsor_id, $member_id); + + try { + $this->getService()->addSponsorUserToGroup($external_id, $bogus_group_slug, $sponsor_id, $summit_id); + $this->fail('Expected EntityNotFoundException was not thrown'); + } catch (EntityNotFoundException $ex) { + // Assert the specific "Group ... not found" message - addSponsorUserToGroup() + // has two OTHER EntityNotFoundException throw sites sharing the same "not found" + // substring (member lookup, summit lookup), so a generic substring check can't + // distinguish this scenario from those. + $this->assertStringContainsString($bogus_group_slug, $ex->getMessage()); + $this->assertStringContainsString('not found', $ex->getMessage()); + } + + self::$em->clear(); + $this->assertEmpty($this->getPermissions($sponsor_id, $member_id)); + // getPermissions() returns [] both when the row is gone and when it survived + // with an empty Permissions column - verify the INSERT itself was rolled back. + $this->assertNoSponsorUsersRowExists($sponsor_id, $member_id); + } + /** * The group slug must be written into the Sponsor_Users.Permissions JSON * column for the correct (SponsorID, MemberID) row. diff --git a/tests/oauth2/OAuth2AttendeesApiTest.php b/tests/oauth2/OAuth2AttendeesApiTest.php index a995314be..62530ec82 100644 --- a/tests/oauth2/OAuth2AttendeesApiTest.php +++ b/tests/oauth2/OAuth2AttendeesApiTest.php @@ -378,6 +378,45 @@ public function testAddAttendeeTicket(){ return $ticket; } + public function testAddAttendeeTicketFailsOnInvalidPromoCode(){ + $attendee = self::$summit->getAttendeeByMember(self::$defaultMember); + $this->assertNotNull($attendee); + $attendee_id = $attendee->getId(); + $initial_ticket_count = $attendee->getTickets()->count(); + + $params = [ + 'id' => self::$summit->getId(), + 'attendee_id' => $attendee_id, + ]; + + $data = [ + 'ticket_type_id' => self::$default_ticket_type->getId(), + 'promo_code' => 'DOES-NOT-EXIST-' . uniqid(), + ]; + + $response = $this->action( + "POST", + "OAuth2SummitAttendeesApiController@addAttendeeTicket", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $content = $response->getContent(); + $this->assertResponseStatus(404); + $decoded = json_decode($content); + $this->assertNotNull($decoded); + $this->assertStringContainsString('Promo code', $decoded->message); + + self::$em->clear(); + $reFetched = App::make(\models\summit\ISummitAttendeeRepository::class)->getById($attendee_id); + $this->assertNotNull($reFetched); + $this->assertEquals($initial_ticket_count, $reFetched->getTickets()->count()); + } + public function testDeleteAttendeeTicket(){ $attendee = self::$summit->getAttendeeByMember(self::$defaultMember); $params = [ diff --git a/tests/oauth2/OAuth2SummitOrdersApiTest.php b/tests/oauth2/OAuth2SummitOrdersApiTest.php index 447aee92c..c4ea354a7 100644 --- a/tests/oauth2/OAuth2SummitOrdersApiTest.php +++ b/tests/oauth2/OAuth2SummitOrdersApiTest.php @@ -62,8 +62,8 @@ final class OAuth2SummitOrdersApiTest extends ProtectedApiTestCase protected function setUp():void { parent::setUp(); - self::$test_secret_key = env('TEST_STRIPE_SECRET_KEY'); - self::$test_public_key = env('TEST_STRIPE_PUBLISHABLE_KEY'); + self::$test_secret_key = env('TEST_STRIPE_SECRET_KEY', 'sk_test_dummy_key'); + self::$test_public_key = env('TEST_STRIPE_PUBLISHABLE_KEY', 'pk_test_dummy_key'); self::$live_secret_key = env('LIVE_STRIPE_SECRET_KEY'); self::$live_public_key = env('LIVE_STRIPE_PUBLISHABLE_KEY'); self::$defaultMember = self::$member; @@ -344,8 +344,26 @@ public function testUpdateOrder(){ return $order; } - public function testReserveWithoutActivePaymentProfile(){ - $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member'); + /** + * The seed sets the registration period relative to the summit's (future) begin + * date, so reserve() rejects with "registration period is closed" by default. + * Open it around NOW for tests exercising the reservation flow. + */ + private function openRegistrationPeriod(): void + { + $now = new \DateTime('now', new \DateTimeZone('UTC')); + self::$summit->setRawRegistrationBeginDate((clone $now)->sub(new \DateInterval('P1D'))); + self::$summit->setRawRegistrationEndDate((clone $now)->add(new \DateInterval('P30D'))); + self::$em->persist(self::$summit); + self::$em->flush(); + } + + /** + * Reserve does not require an ACTIVE payment profile: the default payment + * gateway strategy provides a fallback, so the reservation is created anyway. + */ + public function testReserveSucceedsWithoutActivePaymentProfile(){ + $this->openRegistrationPeriod(); self::$profile->disable(); self::$em->persist(self::$profile); @@ -367,11 +385,6 @@ public function testReserveWithoutActivePaymentProfile(){ ] ]; - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" - ]; - $response = $this->action( "POST", "OAuth2SummitOrdersApiController@reserve", @@ -379,12 +392,11 @@ public function testReserveWithoutActivePaymentProfile(){ [], [], [], - $headers, + $this->getAuthHeaders(), json_encode($data) ); $content = $response->getContent(); - //$this->assertResponseStatus(412); $this->assertResponseStatus(201); $order = json_decode($content); $this->assertTrue(!is_null($order)); @@ -392,12 +404,10 @@ public function testReserveWithoutActivePaymentProfile(){ } public function testReserveWithSummit(){ - $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member'); - - $res = memory_get_peak_usage(true); + $this->openRegistrationPeriod(); $summitId = self::$summit->getId(); - $companyId = 5; + $companyId = self::$companies[0]->getId(); $service = App::make(ISummitService::class); $service->addCompany($summitId, $companyId); @@ -418,11 +428,6 @@ public function testReserveWithSummit(){ ] ]; - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" - ]; - $response = $this->action( "POST", "OAuth2SummitOrdersApiController@reserve", @@ -430,7 +435,7 @@ public function testReserveWithSummit(){ [], [], [], - $headers, + $this->getAuthHeaders(), json_encode($data) ); @@ -438,12 +443,14 @@ public function testReserveWithSummit(){ $this->assertResponseStatus(201); $order = json_decode($content); $this->assertTrue(!is_null($order)); - $res = memory_get_peak_usage(true); return $order; } public function testReserveWithActivePaymentProfile(){ - $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member'); + if (self::$test_secret_key === 'sk_test_dummy_key') { + $this->markTestSkipped('Valid Stripe test credentials required (TEST_STRIPE_SECRET_KEY env var).'); + } + $this->openRegistrationPeriod(); self::$profile->activate(); self::$em->persist(self::$profile); @@ -465,11 +472,6 @@ public function testReserveWithActivePaymentProfile(){ ] ]; - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" - ]; - $response = $this->action( "POST", "OAuth2SummitOrdersApiController@reserve", @@ -477,7 +479,7 @@ public function testReserveWithActivePaymentProfile(){ [], [], [], - $headers, + $this->getAuthHeaders(), json_encode($data) ); @@ -489,7 +491,7 @@ public function testReserveWithActivePaymentProfile(){ } public function testReserveFailingPromoCode(){ - $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member'); + $this->openRegistrationPeriod(); $params = [ 'id' => self::$summit->getId(), @@ -507,11 +509,6 @@ public function testReserveFailingPromoCode(){ ] ]; - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" - ]; - $response = $this->action( "POST", "OAuth2SummitOrdersApiController@reserve", @@ -519,7 +516,7 @@ public function testReserveFailingPromoCode(){ [], [], [], - $headers, + $this->getAuthHeaders(), json_encode($data) ); @@ -527,6 +524,63 @@ public function testReserveFailingPromoCode(){ $this->assertResponseStatus(412); } + /** + * Abandoning a reservation through DELETE .../orders/{hash} must return 204 and + * leave the order cancelled. + */ + public function testCancelReservedOrder(){ + $this->openRegistrationPeriod(); + + $params = [ + 'id' => self::$summit->getId(), + ]; + + $data = [ + "owner_email" => "smarcet@gmail.com", + "owner_first_name" => "Sebastian", + "owner_last_name" => "Marcet", + "owner_company" => "Pumant", + "tickets" => [ + ["type_id" => self::$ticketType->getId()], + ] + ]; + + $response = $this->action( + "POST", + "OAuth2SummitOrdersApiController@reserve", + $params, + [], + [], + [], + $this->getAuthHeaders(), + json_encode($data) + ); + + $this->assertResponseStatus(201); + $order = json_decode($response->getContent()); + $this->assertNotEmpty($order->hash); + + $response = $this->action( + "DELETE", + "OAuth2SummitOrdersApiController@cancel", + [ + 'id' => self::$summit->getId(), + 'hash' => $order->hash, + ], + [], + [], + [], + $this->getAuthHeaders() + ); + + $this->assertResponseStatus(204); + + self::$em->clear(); + $cancelled_order = self::$em->find(\models\summit\SummitOrder::class, $order->id); + $this->assertNotNull($cancelled_order); + $this->assertTrue($cancelled_order->isCancelled()); + } + public function testTicketAssignmentWithoutExtraQuestions(){ $order = self::$summit->getOrders()->first(); if(is_null($order)) $this->markTestSkipped('No orders in test data'); @@ -785,10 +839,8 @@ public function testGetTicketByHash(){ } public function testCreateSingleTicketOrder(){ - $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member'); - $params = [ - 'summit_id' => self::$summit->getId() + 'id' => self::$summit->getId() ]; $data = [ @@ -796,15 +848,11 @@ public function testCreateSingleTicketOrder(){ 'owner_last_name' => 'Marcet', 'owner_email' => 'smarcet@gmail.com', 'ticket_type_id' => self::$ticketType->getId(), + 'ticket_qty' => 1, "owner_company" => "Pumant", //'promo_code' => 'STAFF' ]; - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" - ]; - $response = $this->action( "POST", "OAuth2SummitOrdersApiController@add", @@ -812,7 +860,7 @@ public function testCreateSingleTicketOrder(){ [], [], [], - $headers, + $this->getAuthHeaders(), json_encode($data) ); @@ -823,11 +871,11 @@ public function testCreateSingleTicketOrder(){ return $order; } - public function testCreateSingleTicketOrderNotComplete(){ - $this->markTestSkipped('reserve endpoint is public (no auth.user middleware) but SagaFactory::build() requires non-null Member'); + public function testCreateSingleTicketOrderFailsOnInvalidPromoCode(){ + $initial_order_count = self::$summit->getOrders()->count(); $params = [ - 'summit_id' => self::$summit->getId() + 'id' => self::$summit->getId() ]; $data = [ @@ -835,13 +883,9 @@ public function testCreateSingleTicketOrderNotComplete(){ 'owner_last_name' => 'Marcet', 'owner_email' => 'smarcet@gmail.com', 'ticket_type_id' => self::$ticketType->getId(), + 'ticket_qty' => 1, "owner_company" => "Pumant", - //'promo_code' => 'STAFF' - ]; - - $headers = [ - "HTTP_Authorization" => " Bearer " . $this->access_token, - "CONTENT_TYPE" => "application/json" + 'promo_code' => 'DOES-NOT-EXIST-' . uniqid(), ]; $response = $this->action( @@ -851,15 +895,19 @@ public function testCreateSingleTicketOrderNotComplete(){ [], [], [], - $headers, + $this->getAuthHeaders(), json_encode($data) ); $content = $response->getContent(); - $this->assertResponseStatus(201); - $order = json_decode($content); - $this->assertTrue(!is_null($order)); - return $order; + $this->assertResponseStatus(404); + $decoded = json_decode($content); + $this->assertNotNull($decoded); + $this->assertStringContainsString('Promo code', $decoded->message); + + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); + $this->assertEquals($initial_order_count, self::$summit->getOrders()->count()); } /**