Skip to content

fix(registration-stats): filter on SummitAttendeeTicket.SummitID, count the histogram once and cache the response - #649

Open
romanetar wants to merge 4 commits into
mainfrom
fix/registration-stats-ticket-summit-filter
Open

romanetar wants to merge 4 commits into
mainfrom
fix/registration-stats-ticket-summit-filter

Conversation

@romanetar

@romanetar romanetar commented Oct 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

ref: https://app.clickup.com/t/86bcjg0u6

The admin dashboard polls GET /api/v1/summits/all/{id}/registration-stats about every 44 s and each call runs 16 raw SQL queries. Almost all of them filtered on SummitOrder.SummitID, which makes the optimizer scan SummitAttendeeTicket and probe SummitOrder once per ticket.

  • Migration: backfills SummitAttendeeTicket.SummitID from the order for tickets that have an order but no summit.
  • Predicate: ticket and refund queries filter on SummitAttendeeTicket.SummitID (existing index) and drop the SummitOrder join where it only carried the summit filter. getTotalOrdersCount counts orders, so it keeps SummitOrder.SummitID.
  • Histogram: getPurchasedTicketsGroupedBy no longer runs the histogram a second time inside COUNT(*). The buckets are counted and paged in PHP.
  • Cache: the serializer output is cached per summit, date range and expand/fields/relations. Lifetime: CACHE_API_RESPONSE_REGISTRATION_STATS_LIFETIME (default 600, 0 disables it).

Known limitation (unchanged)

The purchased-tickets histogram uses a single fixed offset for the whole summit, the one in effect on its begin date. Tickets bought under a different offset (e.g. across a DST change) can land in the neighbouring bucket. Behavior is kept on purpose to return the same payload as before. It is documented above getTimeZoneOffset() and pinned by a test.

Tests

New, run locally in the summit-api container, all passing:

  • tests/Repositories/RegistrationStatsPayloadTest.php (5 tests): results compared with the old SummitOrder join, including a ticket bought at 23:30 local time and one on the DST change day.
  • tests/Repositories/RegistrationStatsCacheTest.php (4 tests): one execution inside the lifetime, re-run after expiry, LIFETIME=0, different date ranges.

tests/oauth2/OAuth2SummitApiTest.php fails entirely, with and without this change: its tearDown calls clearSummitTestData() without clearOrdersTestData(). The bodies of testGetCurrentSummitRegStats and testGetAttendeesCheckinsOverTimeStats ran their assertions. I did not touch that file.

Deployment notes

  • Migration: Version20261021120000 runs a single UPDATE ... JOIN SummitOrder on SummitAttendeeTicket (only rows with an order and a NULL SummitID; 2 rows expected on prod, both summit 69). It must be applied together with this release, because the queries now rely on SummitAttendeeTicket.SummitID. down() is a no-op, so a rollback does not undo the backfill (harmless).
  • New env var (optional): CACHE_API_RESPONSE_REGISTRATION_STATS_LIFETIME, in seconds, default 600. No action needed to get the default. Added to .env.example.
  • Behavior change: the dashboard figures can be up to 10 minutes old. Set the variable to 0 and restart to go back to live counts without a deploy.
  • Cache store: uses the default Laravel cache driver (redis in the environments that set CACHE_DRIVER=redis). Entries are keyed per summit, date range and expand/fields/relations, and expire by TTL; nothing invalidates them earlier.
  • No new index: relies on the existing IDX_SummitAttendeeTicket_SummitID.
  • After deploy: check SELECT COUNT(*) FROM SummitAttendeeTicket WHERE SummitID IS NULL AND OrderID IS NOT NULL returns 0, and compare the digests of the registration stats queries on prod-12.

Not verified

  • EXPLAIN and latency targets on prod-12 (needs access to that database).
  • The other registration-stats methods were not individually EXPLAINed.
  • The full test suite was not run.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Registration statistics responses are now cached for 10 minutes by default.
    • Cache duration can be configured, including disabling caching with a zero-second setting.
    • Registration statistics support consistent date-range caching and pagination across histogram results.
  • Bug Fixes

    • Improved ticket-related registration statistics accuracy, including behavior across daylight-saving time changes.
    • Existing ticket data is automatically associated with its summit when required.
  • Tests

    • Added coverage for caching, expiration, disabled caching, date ranges, statistics accuracy, and histogram pagination.

@github-actions

Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-649/

This page is automatically updated on each push to this PR.

…t.SummitID and compute the histogram once

Replace SummitOrder.SummitID with SummitAttendeeTicket.SummitID (indexed) in the
ticket and refund queries and drop the SummitOrder join where it only carried the
summit filter. getTotalOrdersCount still counts orders, so it keeps its filter.

getPurchasedTicketsGroupedBy no longer wraps the histogram in COUNT(*) to get the
total: the buckets are counted and paged in PHP.

Document that the histogram uses a single fixed summit offset (known limitation,
behavior unchanged).
…ummit and date range

The lifetime comes from cache_api_response.registration_stats_response_lifetime
(CACHE_API_RESPONSE_REGISTRATION_STATS_LIFETIME, default 600). A value of 0
disables the cache.
@romanetar romanetar changed the title perf(registration-stats): filter on SummitAttendeeTicket.SummitID, count the histogram once and cache the response fix(registration-stats): filter on SummitAttendeeTicket.SummitID, count the histogram once and cache the response Oct 21, 2026
@romanetar
romanetar force-pushed the fix/registration-stats-ticket-summit-filter branch from 18e9fdd to 0fc1b24 Compare October 21, 2026 14:47
@github-actions

Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-649/

This page is automatically updated on each push to this PR.

@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: b63f5ee2-8558-4126-8cd4-78bc2edb3f69
📥 Commits

Reviewing files that changed from the base of the PR and between ccad5fd and 0fc1b24.

📒 Files selected for processing: 7
  • .env.example
  • app/ModelSerializers/Summit/SummitRegistrationStatsSerializer.php
  • app/Models/Foundation/Summit/SummitRegistrationStats.php
  • config/cache_api_response.php
  • database/migrations/model/Version20261021120000.php
  • tests/Repositories/RegistrationStatsCacheTest.php
  • tests/Repositories/RegistrationStatsPayloadTest.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Registration statistics queries now scope tickets through ticket summit IDs and paginate histogram buckets in memory. The serializer supports configurable response caching. A migration backfills missing ticket summit IDs from related orders, and tests cover query results and cache behavior.

Changes: Registration statistics

Layer / File(s) Summary
Ticket summit scope and histogram behavior
database/migrations/model/Version20261021120000.php, app/Models/Foundation/Summit/SummitRegistrationStats.php, tests/Repositories/RegistrationStatsPayloadTest.php
A migration fills missing ticket summit IDs from related orders. Statistics queries now scope tickets by their summit IDs, and histogram pagination uses array_slice. Integration tests compare payload results with legacy order-join queries and check date buckets and pagination.
Configurable response caching
.env.example, config/cache_api_response.php, app/ModelSerializers/Summit/SummitRegistrationStatsSerializer.php, tests/Repositories/RegistrationStatsCacheTest.php
The serializer caches statistics responses using a configurable lifetime and a key that includes summit, date range, and serialization options. Tests cover cache reuse, expiry, disabled caching, and separate date ranges.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Config
  participant SummitRegistrationStatsSerializer
  participant Cache
  participant SummitRegistrationStats
  Config->>SummitRegistrationStatsSerializer: Provide cache lifetime
  SummitRegistrationStatsSerializer->>Cache: Look up response by cache key
  Cache-->>SummitRegistrationStatsSerializer: Return cached array or cache miss
  SummitRegistrationStatsSerializer->>SummitRegistrationStats: Build statistics on cache miss
  SummitRegistrationStats-->>SummitRegistrationStatsSerializer: Return statistics
  SummitRegistrationStatsSerializer->>Cache: Store response for configured lifetime
Loading

Suggested reviewers: smarcet

Merge Risk: ⚪ Minimal · up to 0fc1b

The reviewed changes are mergeable after normal checks, provided the backfill migration is deployed with the query changes.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 54.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 6 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely summarizes the main changes: SummitID filtering, single histogram counting, and response caching.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 54.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 35 functions across 6 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@romanetar
romanetar requested a review from smarcet October 21, 2026 15:17

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant