Clean up leaderboard code

[skyfallwastaken]

Summary of the problem

The leaderboard code is extremely messy and not well tested.

Describe your changes

Centralises leaderboard code in a few files to clean things up a bit

Screenshots / Media

[greptile-apps]

Greptile Summary

This PR consolidates leaderboard logic that was previously spread across LeaderboardService, LeaderboardCache, and LeaderboardDateRange into the Leaderboard model and a new Leaderboard::Builder class, and fixes a bug in CleanupOldLeaderboardsJob where boards were pruned by created_at instead of start_date.

• Three service modules are deleted and replaced by Leaderboard.fetch, Leaderboard.regenerate, Leaderboard#range, and a private cache_key helper — all co-located where the domain object lives.
• Leaderboard::Builder extracts the full build pipeline (heartbeat aggregation, two-phase streak computation, entry upsert, finalization) into its own class with named constants and is backed by new test coverage.
• CleanupOldLeaderboardsJob now filters on start_date instead of created_at, preventing live boards whose DB rows are old from being reaped; tests pin this corrected behaviour.

Confidence Score: 4/5

Safe to merge; the refactoring is functionally equivalent to the code it replaces and the cleanup job bug fix is well-tested.

The consolidation is clean and all deleted service modules have no remaining callers. The only non-trivial risks are the except(:id) passthrough in the controller (currently equivalent to the old allowlist but less defensive going forward) and the silent removal of :weekly date normalization (non-issue today since the period type was dropped from the enum). Both are stylistic concerns rather than present defects.

app/controllers/leaderboards_controller.rb (implicit user-field passthrough) and app/models/leaderboard.rb (unused _period parameter context).

Important Files Changed

Filename	Overview
app/models/leaderboard.rb	Core model gains fetch, regenerate, normalize_date, write_cache, range, and cache_key class methods — consolidating what previously lived in three separate service modules. Logic is correct and matches the old behavior.
app/models/leaderboard/builder.rb	New file extracts the leaderboard build algorithm from LeaderboardUpdateJob into a dedicated Builder class; constants are well-named and the two-phase streak logic is preserved faithfully.
app/controllers/leaderboards_controller.rb	Replaces LeaderboardService.get with Leaderboard.fetch; entry mapping simplified with e.merge + except(:id), which is functionally equivalent to the old explicit allowlist given the current LeaderboardPageCache shape.
app/jobs/cleanup_old_leaderboards_job.rb	Cutoff column changed from created_at to start_date, which correctly prevents deleting live boards whose DB rows were created long ago. Test coverage is added for this regression fix.
app/jobs/leaderboard_update_job.rb	Job body reduced to a single delegation call to Leaderboard.regenerate; all build logic moved to Builder.
app/services/leaderboard_cache.rb	Deleted; its CACHE_EXPIRATION constant and global_key/write/read/fetch methods are absorbed into the Leaderboard model. No callers remain.
app/services/leaderboard_date_range.rb	Deleted; normalize_date and calculate are folded into Leaderboard.normalize_date and Leaderboard#range respectively. No callers remain.
app/services/leaderboard_service.rb	Deleted; replaced by Leaderboard.fetch. No callers remain.
test/models/leaderboard_test.rb	New test file covering normalize_date, range, fetch (cache hit/miss, unfinished/deleted boards), and regenerate — good coverage of the new Leaderboard API.
test/models/leaderboard/builder_test.rb	New test file covering force rebuild, MIN_TOTAL_SECONDS filtering, stale-entry pruning, and two-phase streak optimization (both branches).
test/jobs/cleanup_old_leaderboards_job_test.rb	New tests validating the start_date-based retention logic, including the regression case where today's board has an old created_at.
app/controllers/api/v1/leaderboard_controller.rb	Trivial substitution of LeaderboardService.get with Leaderboard.fetch in both daily and weekly API endpoints.

Sequence Diagram

sequenceDiagram
    participant C as Controller
    participant L as Leaderboard
    participant RC as Rails.cache
    participant DB as Database
    participant J as LeaderboardUpdateJob
    participant B as Leaderboard::Builder

    C->>L: fetch(period:, date:)
    L->>RC: read(cache_key)
    alt cache hit
        RC-->>L: board
        L-->>C: board
    else cache miss
        L->>DB: ready.find_by(start_date, period_type)
        alt board found
            DB-->>L: board
            L->>RC: write(cache_key, board)
            L-->>C: board
        else board missing
            L->>J: perform_later(period, date)
            L-->>C: nil
            J->>L: regenerate(period:, date:, force:)
            L->>B: new(period:, date:).call(force:)
            B->>DB: find_or_create_by!(start_date, period_type)
            B->>DB: upsert LeaderboardEntries
            B->>DB: board.update!(finished_generating_at)
            B->>RC: write_cache(board)
            B->>RC: LeaderboardPageCache.warm
        end
    end

Loading

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
app/models/leaderboard.rb:42-45
The `_period` parameter is intentionally unused, silently dropping the `beginning_of_week` normalization that `LeaderboardDateRange.normalize_date` applied for `:weekly` periods. Since `:weekly` is absent from the enum today this is harmless, but the underscore prefix is the only signal — a future reader adding a `:weekly` period type wouldn't know normalization used to happen here. A short comment would make the intent explicit.

```suggestion
  # `_period` is intentionally unused; period-specific normalization
  # (e.g. snapping :weekly to beginning_of_week) was removed along with
  # the :weekly period type. Keep the parameter for a stable signature.
  def self.normalize_date(date, _period)
    date = Date.current if date.blank?
    date.is_a?(Date) ? date : Date.parse(date.to_s)
  end
```

### Issue 2 of 2
app/controllers/leaderboards_controller.rb:78-84
**Implicit allowlist via `except(:id)`**

The old code was an explicit allowlist of six user fields. The new `e[:user].except(:id)` passes through every field that `LeaderboardPageCache#serialize_user` produces — currently identical, but if a new field (e.g. an internal flag or email) is added to `serialize_user` in the future it will automatically be forwarded to the frontend without any review gate here. An explicit `slice` of the known safe keys keeps the surface locked to what the client actually needs.

_{Reviews (1): Last reviewed commit: "Clean up leaderboard code" | Re-trigger Greptile}

[Copilot]

Pull request overview

This PR centralizes leaderboard logic into the Leaderboard model (and a dedicated builder) while removing the older service modules, with new tests added to cover the rebuilt behavior.

Changes:

• Move leaderboard fetching/caching + date normalization into Leaderboard.fetch and board generation into Leaderboard::Builder.
• Simplify LeaderboardUpdateJob to delegate to Leaderboard.regenerate, and adjust old leaderboard cleanup to reap by start_date.
• Add unit tests covering leaderboard fetching/ranges, builder behavior, and cleanup retention rules.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/models/leaderboard/builder_test.rb	Adds coverage for builder rebuild behavior, min-duration filtering, stale entry pruning, and streak two-phase optimization.
test/models/leaderboard_test.rb	Adds coverage for date normalization, range semantics, fetch caching/enqueue behavior, and regeneration behavior.
test/jobs/cleanup_old_leaderboards_job_test.rb	Adds coverage for pruning by start_date retention (and preserving today’s board).
app/services/leaderboard_service.rb	Removed in favor of model-based Leaderboard.fetch.
app/services/leaderboard_date_range.rb	Removed; date normalization/range logic moved into Leaderboard.
app/services/leaderboard_cache.rb	Removed; caching moved into Leaderboard/LeaderboardPageCache.
app/models/leaderboard/builder.rb	Introduces Leaderboard::Builder to encapsulate leaderboard entry generation + streak computation + cache warming.
app/models/leaderboard.rb	Adds ready scope, fetch, regenerate, normalize_date, and range, plus cache write helpers.
app/jobs/leaderboard_update_job.rb	Delegates job work to Leaderboard.regenerate.
app/jobs/cleanup_old_leaderboards_job.rb	Changes pruning logic to use start_date retention cutoff.
app/controllers/leaderboards_controller.rb	Switches to Leaderboard.fetch and simplifies entry user payload shaping.
app/controllers/api/v1/leaderboard_controller.rb	Switches API endpoints to Leaderboard.fetch.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.