Feature: Add recurring event support#1143
Draft
daveearley wants to merge 8 commits intov2.0.0-alpha.1from
Draft
Conversation
|
🚅 Deployed to the Hi.Events-pr-1143 environment in Hi.Events
|
…ing-events # Conflicts: # VERSION # backend/composer.json # frontend/package.json # frontend/src/locales/de.js # frontend/src/locales/de.po # frontend/src/locales/en.js # frontend/src/locales/en.po # frontend/src/locales/es.js # frontend/src/locales/es.po # frontend/src/locales/fr.js # frontend/src/locales/fr.po # frontend/src/locales/hu.js # frontend/src/locales/hu.po # frontend/src/locales/it.js # frontend/src/locales/it.po # frontend/src/locales/nl.js # frontend/src/locales/nl.po # frontend/src/locales/pl.js # frontend/src/locales/pl.po # frontend/src/locales/pt-br.js # frontend/src/locales/pt-br.po # frontend/src/locales/pt.js # frontend/src/locales/pt.po # frontend/src/locales/ru.js # frontend/src/locales/ru.po # frontend/src/locales/se.js # frontend/src/locales/se.po # frontend/src/locales/tr.js # frontend/src/locales/tr.po # frontend/src/locales/vi.js # frontend/src/locales/vi.po # frontend/src/locales/zh-cn.js # frontend/src/locales/zh-cn.po # frontend/src/locales/zh-hk.js # frontend/src/locales/zh-hk.po
railway-app
bot
requested a deployment
to
Hi.Events / Hi.Events-pr-1143
CI was failing on OrderCreateRequestValidationServiceTest because the recurring-events feature added tests that issue raw DB::selectOne queries against the application schema (specifically order_items.event_occurrence_id), but the test database only had its schema when a developer happened to have already run migrations against it. Fix: hook into createApplication() — which runs once per test method, before any DatabaseTransactions trait opens its wrapping transaction — and call migrate:fresh exactly once per phpunit process. A static flag short-circuits subsequent calls so we pay the cost (a few seconds) only once. Why createApplication and not setUp: - DatabaseTransactions begins its transaction inside the parent setUp() flow, so calling migrate from setUp wraps the migration in a transaction - One of the recurring-events migrations runs CREATE INDEX CONCURRENTLY, which postgres refuses inside a transaction block - createApplication runs before any trait setup, so the migration executes on a clean connection The _test guard moves to the same hook so it fires before any DB connection is opened, not just before the first query. TestCase shrinks back to its original two-line form; all the test-DB plumbing now lives in CreatesApplication where it can run early enough. Tested locally against a freshly-recreated empty hievents_test database: - BaseRepositoryTest: 46 passed (creates its own br_test_widgets schema on top of the migrated one) - Full Unit suite: 579 passed, 1 deprecated, 1 skipped - Guard still fires when DB_DATABASE is forced to a non-_test value Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Audit of the recurring-events branch surfaced several real
correctness/security issues introduced by the feature; this commit
fixes the verified blockers and adds test coverage for each.
- B1 (IDOR): UpsertCheckInListRequest and SendMessageRequest validated
event_occurrence_id only with `exists:event_occurrences,id`, with no
ownership check. An attacker holding access to event A could supply
an occurrence id from event B and have it accepted. Both requests
now use the existing CreateAttendeeRequest convention:
`Rule::exists(...)->where('event_id', $eventId)->whereNull('deleted_at')`.
- B4 (refund stats race): EventStatisticsRefundService used a classic
read-modify-write pattern on the new occurrence + occurrence-daily
stats rows, with no version check, so concurrent refunds on the
same occurrence could lose updates. Switched both occurrence paths
to atomic SQL increments via DB::raw, matching the pattern already
in ProductQuantityUpdateService, and bumped `version + 1` so any
optimistic-locking reader still detects the change.
- B7 (visibility bypass): OrderCreateRequestValidationService never
consulted product_occurrence_visibility, so a direct POST could
buy a product hidden from a specific occurrence. Added
validateOccurrenceProductVisibility() (now batched via a single
findWhereIn lookup) so order creation enforces the same allow-list
semantics as ProductFilterService::filterByOccurrenceVisibility.
- B8 (cancellation race / double dispatch): CancelOccurrenceHandler
and BulkCancelOccurrencesJob both did findFirstWhere + status check
+ update without a row-level lock, so two concurrent cancellations
could pass the same status check and dispatch the refund + email
side-effects twice. Added EventOccurrenceRepository::findByIdLocked
(SELECT ... FOR UPDATE), reworked the single handler to lock the
row inside its existing transaction, and rewrote the bulk job to
wrap each iteration in its own transaction. Side-effects fire only
when the locked row actually transitions from non-cancelled to
cancelled.
- Architecture cleanup: OrderCreateRequestValidationService had a
raw `DB::selectOne(...)` for its reserved-quantity calculation,
violating the codebase rule that Eloquent stays in repositories.
Moved the query to OrderItemRepository::getReservedQuantityForOccurrence
and injected the repository into the validation service.
- Perf: refactored EventStatisticsRefundService::updateForRefund to
load the order with order_items exactly once and reuse the grouped
result for both occurrence + occurrence-daily passes (was loading
+ grouping twice). Skips the occurrence pass entirely for
non-recurring orders.
Tests
-----
- New OrderItemRepositoryTest (integration test, 7 cases) exercises
getReservedQuantityForOccurrence against the real PostgreSQL test
schema: covers active reservations, expired reservations, non-RESERVED
statuses, soft-deleted orders, soft-deleted order items, and
per-occurrence scoping.
- New EventStatisticsRefundService tests (3 cases) cover the new
occurrence path that previously had zero coverage: single-occurrence
refund, multi-occurrence split, and skip-when-no-occurrence-items.
Existing tests strengthened with shouldNotReceive() on the occurrence
repos so the skip path is provably exercised.
- New CancelOccurrenceHandlerTest case for the IDOR guard:
findByIdLocked returns an occurrence belonging to a different event
and the handler rejects with ResourceNotFoundException.
- New OrderCreateRequestValidationServiceTest cases for the visibility
check + the multi-occurrence batching (asserts findWhereIn is called
exactly once with all distinct occurrence ids).
- Updated CancelOccurrenceHandlerTest and BulkCancelOccurrencesJobTest
to mock findByIdLocked instead of findFirstWhere and assert getEventId
on the locked row.
All 541 unit tests + 7 new repository integration tests pass.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reorganises the test infrastructure so that "unit" actually means
unit and the slower DB-touching tests live where they belong, then
wires both suites into CI.
Test reorg
----------
Moved BaseRepositoryTest and the new OrderItemRepositoryTest (plus
their Widget fixture classes) from tests/Unit/Repository/ to
tests/Feature/Repository/, with namespaces updated. These tests
extend Tests\TestCase, boot Laravel, and hit the real PostgreSQL
test database via DatabaseTransactions — by Laravel's standard
testing taxonomy they're feature/integration tests, not unit tests.
Bootstrap
---------
CreatesApplication::createApplication still always runs the _test
database guard (so a misconfigured environment can never accidentally
point at a non-test database), but now only runs migrate:fresh when
the test class actually uses one of Laravel's database testing traits
(DatabaseTransactions / RefreshDatabase / DatabaseMigrations). The
detection is automatic via class_uses_recursive — no env var or
per-test opt-in.
Pure unit tests no longer pay the multi-second migrate:fresh cost
and no longer need a live database connection at all. Verified
locally by running `--testsuite=Unit` with DB_HOST=invalid.host.local:
the suite still passes (541 tests in ~5s), proving migration is
truly skipped. The _test guard still fires when DB_DATABASE is
pointed at anything not ending in `_test`.
CI workflow
-----------
Renamed .github/workflows/unit-tests.yml -> tests.yml and rewrote it
to run the two suites as distinct phpunit invocations:
- Unit step runs first, does not depend on Postgres being ready
- Wait for Postgres
- Feature step runs against the live test database
The Postgres service container is still defined because Feature
tests need it; Unit tests run in parallel with the service container
warming up. Job-level env keeps DB_DATABASE=hievents_test so the
guard passes for both steps. CI was previously running only
tests/Unit, which meant the existing Feature tests (Auth, EmailTemplates)
were silently rotting — they're now in CI and must stay green.
Email template token assertions
-------------------------------
EmailTemplateTokenTest had three pre-existing failures that were
hidden because the file lived in tests/Feature/ which CI never
touched. The assertions checked for underscore-style tokens
(`{{ order_first_name }}`, `{{ event_title }}`, etc.) but the
LiquidTemplateRenderer implementation returns dot-notation
(`{{ order.first_name }}`, `{{ event.title }}`). Updated all 7
assertions to use the actual token format. Descriptions were already
correct.
Verification
------------
- Unit suite: 541 passed in 5.34s, no DB connection required
- Feature suite: 74 passed in 3.94s (all green, including the 7
new OrderItemRepositoryTest cases and the 6 EmailTemplateTokenTest
cases that were previously broken)
- _test guard still fires for unit tests with a non-_test DB name
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TBD