Repository navigation
feat: Configure streaks - #186
Conversation
Code Review Summary✨ This PR introduces a configurable streak system for Trakli. It adds Streaks are advanced by Feature coverage in |
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| try { | ||
| $data = $this->applyApiQuery($request, $groupsQuery); | ||
|
|
||
| // update user streak |
There was a problem hiding this comment.
Manually calling streak updates in every controller method is redundant and fragile. Consider moving this logic to a Middleware or a Global Observer/Event Listener to handle 'activity' streaks automatically.
| // update user streak | |
| // Logic moved to a central middleware or event listener |
| $table->string('type'); // 'app_check_in', 'under_budget', 'transaction_logging' | ||
| $table->integer('current_streak')->default(0); | ||
| $table->integer('longest_streak')->default(0); | ||
| $table->timestamp('last_activity_date'); |
There was a problem hiding this comment.
The last_activity_date column is a timestamp but doesn't allow nulls. This might cause issues during record creation if not explicitly handled in the model's create/update logic. Also, consider adding an index if this column is used frequently for filtering or sorting.
| $table->timestamp('last_activity_date'); | |
| $table->timestamp('last_activity_date')->useCurrent(); |
Someone who records their spending several days running now has that run counted, daily and weekly, and hears about it at three days and at each milestone after. A run that breaks starts again while the best one so far is kept. Runs belong to an owner rather than a user, so shared owners such as a group can earn one without changing the schema. Recording a transaction and reaching the API are counted separately.
|
This repository is not connected to any of your workspaces. Please connect it at https://app.sourceant.ai to get reviews on it. |
|
Coverage Report |
|
|
||
| // Check-ins are counted but not mailed: a day that earns both streaks | ||
| // would otherwise send two near-identical messages. | ||
| 'types' => ['transaction'], |
There was a problem hiding this comment.
The type filter duplicates the enum's backing value as a loose string. If StreakType::TRANSACTION is ever renamed, this config silently stops matching and milestone mail is disabled with no error. Reference the enum value so the two cannot drift.
| 'types' => ['transaction'], | |
| 'types' => [\App\Enums\StreakType::TRANSACTION->value], |
| use Illuminate\Queue\SerializesModels; | ||
|
|
||
| class StreakMilestoneMail extends Mailable | ||
| { |
There was a problem hiding this comment.
periodLabel() builds the plural by appending a literal 's' to a translated label. This bypasses the framework pluralizer and produces incorrect output for any label that is not a regular English noun once the string is translated. Str::plural() accepts a count and handles singular/plural correctly, keeping the value the envelope subject and both views consume unchanged in shape.
| { | |
| private function periodLabel(): string | |
| { | |
| return \Illuminate\Support\Str::plural($this->streak->period->label(), $this->milestone); | |
| } |
The email was built from stock components the project does not set up, so it could not render at all. It now uses the same header, panel, button and footer as the rest, and can be opened from the local mail preview or sent to a real inbox for checking.
| 'last_tracked_on' => now()->toDateString(), | ||
| ]); | ||
|
|
||
| $streak->setRelation('owner', new User(['first_name' => $request->query('name', 'Alex')])); |
There was a problem hiding this comment.
The preview owner is built with new User([...]), i.e. through mass assignment, whereas every other fake user in this controller is populated with forceFill(). If first_name is not mass-assignable on User (which is why fakeUser() uses forceFill() for exactly these attributes), new User(['first_name' => ...]) silently yields a model with a null first name, the greeting block @if (! empty($streak->owner?->first_name)) is skipped, and the name query parameter has no visible effect on the preview. Populating the attribute explicitly keeps the preview deterministic regardless of the model's $fillable/$guarded configuration.
| $streak->setRelation('owner', new User(['first_name' => $request->query('name', 'Alex')])); | |
| $streak->setRelation('owner', (new User())->forceFill([ | |
| 'first_name' => $request->query('name', 'Alex'), | |
| ])); |
|
|
||
| Mail::to($user)->send($mailable); | ||
| $this->info("Sent '{$type}' test email to {$user->email}."); | ||
| } |
There was a problem hiding this comment.
Contract parity with MailPreviewController::buildStreak() (line 149), which hydrates the owner relation before handing the model to StreakMilestoneMail. The shared views (streak-milestone.blade.php line 7 and streak-milestone-text.blade.php line 1) read $streak->owner->first_name, so this path currently depends on lazy-loading an owner() relation on a never-persisted model and issues an extra query, while the preview path uses an explicit relation. Setting the already-loaded $user keeps the two producers behaving identically and removes the query.
| } | |
| private function buildStreakMail(User $user): StreakMilestoneMail | |
| { | |
| $milestone = 7; | |
| $streak = new Streak([ | |
| 'owner_type' => User::class, | |
| 'owner_id' => $user->id, | |
| 'type' => StreakType::TRANSACTION, | |
| 'period' => StreakPeriod::DAILY, | |
| 'current_length' => $milestone, | |
| 'longest_length' => 12, | |
| 'started_on' => Carbon::now()->subDays($milestone - 1)->toDateString(), | |
| 'last_tracked_on' => Carbon::now()->toDateString(), | |
| ]); | |
| // The views read $streak->owner directly; hydrate it here so this path | |
| // matches MailPreviewController and avoids a lazy-load query. | |
| $streak->setRelation('owner', $user); | |
| return new StreakMilestoneMail($streak, $milestone); | |
| } |
| 'last_tracked_on' => now()->toDateString(), | ||
| ]); | ||
|
|
||
| $streak->setRelation('owner', new User(['first_name' => $request->query('name', 'Alex')])); |
There was a problem hiding this comment.
This is the only place in the class that fabricates a fake User with mass assignment. Every other builder routes through fakeUser(), which uses forceFill() precisely to sidestep $fillable. If first_name is not fillable, new User(['first_name' => …]) silently drops it and the @if (! empty($streak->owner?->first_name)) greeting block in both streak templates disappears with no error — the preview would no longer match a real send. Use forceFill() so the fabricated owner is always populated.
| $streak->setRelation('owner', new User(['first_name' => $request->query('name', 'Alex')])); | |
| $owner = new User(); | |
| $owner->forceFill(['first_name' => $request->query('name', 'Alex')]); | |
| $streak->setRelation('owner', $owner); |
A day was the server's day, so someone far enough east or west could record every evening and still be told their run had broken. Days now follow the timezone the account already carries, and a run is only mentioned by email to someone who still wants email.
| * A day belongs to whoever lived it. Falls back to the application zone | ||
| * for an owner that carries no preference, such as a group. | ||
| */ | ||
| private function timezoneFor(Model $owner): string |
There was a problem hiding this comment.
timezoneFor() returns the configured timezone string without validating it. That value flows straight into $occurredAt->setTimezone(...) on line 52, and DateTimeZone/setTimezone() throws InvalidArgumentException for an unknown identifier. A single malformed value in the owner's timezone config (e.g. 'Foo/Bar') would then make every transaction creation and every authenticated request (via RecordCheckInStreak) fail with a 500 instead of degrading gracefully. Validate against the known identifiers and fall back to the application zone, matching the existing "no preference" fallback behaviour.
| private function timezoneFor(Model $owner): string | |
| private function timezoneFor(Model $owner): string | |
| { | |
| $configured = method_exists($owner, 'getConfigValue') | |
| ? $owner->getConfigValue(ConfigurationKeys::TIMEZONE) | |
| : null; | |
| if (is_string($configured) && $configured !== '' && in_array($configured, timezone_identifiers_list(), true)) { | |
| return $configured; | |
| } | |
| return config('app.timezone'); | |
| } |
| use App\Enums\StreakType; | ||
| use App\Mail\StreakMilestoneMail; | ||
| use App\Models\Streak; | ||
| use App\Models\User; |
There was a problem hiding this comment.
NotificationService is used unqualified in the constructor but the diff adds no import for it. Because this file is in App\Services, PHP resolves the bare name to App\Services\NotificationService. If the class actually lives in another namespace (the searches in this revision could not locate it, and the test suite already pulls Whilesmart\ModelConfiguration\… classes), every container resolution of StreakService will fatal with Class not found. Add the explicit import so the dependency contract is unambiguous regardless of where the class is declared.
| use App\Models\User; | |
| use App\Models\User; | |
| use App\Services\NotificationService; | |
| use App\Support\ConfigurationKeys; |
| $key = "streak:check-in:{$user->getKey()}:" . now()->toDateString(); | ||
|
|
There was a problem hiding this comment.
The cache guard and the streak bucket disagree on what "today" means. now() here is evaluated in the application timezone, while StreakService::advance() buckets $occurredAt in the owner's timezone (StreakService.php line 52). For any owner whose zone is offset from the app zone, the guard window (app day) and the bucket window (owner day) are misaligned, so the guard can suppress a check-in that belongs to a new owner-local day. Example: an owner in UTC+13 makes their last request of local day D at 23:00 local (10:00 UTC, day D) — the key for UTC day D is now set. Their first request of local day D+1 at 01:00 local (12:00 UTC, still day D) hits the same key, so track() is skipped and local day D+1 is never recorded; if they do not return before local day D+2, the check-in streak breaks spuriously. Deriving the key (and TTL) from the owner's local day keeps the guard aligned with the bucket it is protecting, while preserving the "one cache read per request" property.
| $key = "streak:check-in:{$user->getKey()}:" . now()->toDateString(); | |
| $timezone = method_exists($user, 'getConfigValue') | |
| ? $user->getConfigValue('timezone') | |
| : null; | |
| $timezone = is_string($timezone) && in_array($timezone, timezone_identifiers_list(), true) | |
| ? $timezone | |
| : config('app.timezone'); | |
| $key = "streak:check-in:{$user->getKey()}:" . now($timezone)->toDateString(); | |
| if (Cache::add($key, true, now($timezone)->endOfDay())) { | |
| $this->streaks->track($user, StreakType::CHECK_IN); | |
| } |
| public function __construct( | ||
| private readonly NotificationService $notifications, | ||
| ) { | ||
| } |
There was a problem hiding this comment.
Both periods are announced unconditionally, so a single action that crosses a milestone on more than one period queues two near-identical mails. The comment in config/streaks.php states the intent is one message per action, but the types filter only prevents cross-type duplicates, not cross-period ones. The cross-period case is reachable: with activity on the Saturday and Sunday of week N-1 plus the Monday of week N, daily reaches 3 (Sat=1, Sun=2, Mon=3) and weekly reaches 3 on that same Monday. Announce only the longest period that actually sends, so one action still yields one message. This relies on announce() reporting whether it sent (see the matching suggestion on its return type).
| } | |
| foreach (StreakPeriod::cases() as $period) { | |
| $streak = $this->advance($owner, $type, $period, $occurredAt); | |
| if ($streak !== null) { | |
| $advanced[] = $streak; | |
| } | |
| } | |
| // One action earns one message: `cases()` is ordered shortest-first, so | |
| // walk the advanced streaks longest-first and stop at the first send. | |
| foreach (array_reverse($advanced) as $streak) { | |
| if ($this->announce($streak)) { | |
| break; | |
| } | |
| } | |
| return $advanced; |
| $user = $request->user(); | ||
|
|
||
| if ($user !== null) { | ||
| $key = "streak:check-in:{$user->getKey()}:" . now()->toDateString(); |
There was a problem hiding this comment.
The middleware's dedupe guard and the service that actually counts the streak use two different definitions of "today". StreakService::advance() buckets the action with StreakPeriod::bucket($occurredAt->setTimezone($this->timezoneFor($owner))), i.e. the owner's local day, but the cache key here is built from now()->toDateString(), i.e. the application-timezone day (config('app.timezone'), UTC by default). When the owner's local day transitions inside a single app-timezone day, the guard suppresses the second track() call for that app day even though it belongs to a new local bucket, so that local day's check-in is never recorded (the service cannot recover it, because it is never called). Concretely, a user in Pacific/Auckland whose only requests land at 05:00 UTC and 20:00 UTC on the same server day has the 20:00 request silently dropped for the new local day. Deriving the key from the same owner-local day the service uses keeps the guard and the bucketing contract in agreement. Reading ConfigurationKeys::TIMEZONE (the constant StreakService::timezoneFor() already relies on) avoids introducing a divergent definition of the owner's zone.
| $key = "streak:check-in:{$user->getKey()}:" . now()->toDateString(); | |
| $timezone = method_exists($user, 'getConfigValue') | |
| ? ($user->getConfigValue(\App\Support\ConfigurationKeys::TIMEZONE) ?: config('app.timezone')) | |
| : config('app.timezone'); | |
| $bucket = \Carbon\CarbonImmutable::now($timezone)->startOfDay(); | |
| $key = "streak:check-in:{$user->getKey()}:{$bucket->toDateString()}"; | |
| if (Cache::add($key, true, $bucket->endOfDay())) { |
| $key = "streak:check-in:{$user->getKey()}:" . now()->toDateString(); | ||
|
|
There was a problem hiding this comment.
Duplicate day-dedup with a conflicting clock. StreakService::advance() already guarantees "a day advances at most once" by comparing the stored last_tracked_on bucket to the current bucket, and it computes that bucket in the owner's timezone via timezoneFor() (StreakService line 52). This middleware re-implements the same rule with a second clock: now()->toDateString() uses the application timezone. For an owner whose timezone is ahead of the app zone (e.g. Pacific/Auckland), their local day rolls over hours after the app day, and the cache key stays pinned to the already-added app day, so a genuine new local check-in day is swallowed before track() is ever called. The service would have counted it; the guard suppresses it — a concrete contract mismatch between the two implementations of "one check-in per day". Key the cache by the same local date the service buckets on. This requires exposing the resolution the service already performs, e.g. add to StreakService:
public function localDate(Model $owner): string
{
return CarbonImmutable::now($this->timezoneFor($owner))->toDateString();
}(The alternative is to drop the guard and let the idempotent service own dedup, at the cost of one read per authenticated request, which is why the shared-timezone fix is preferable.)
| $key = "streak:check-in:{$user->getKey()}:" . now()->toDateString(); | |
| $key = "streak:check-in:{$user->getKey()}:" . $this->streaks->localDate($user); | |
| if (Cache::add($key, true, now()->endOfDay())) { | |
| $this->streaks->track($user, StreakType::CHECK_IN); | |
| } |
A check-in was skipped whenever someone's own day turned over inside a server day, breaking a run they had kept. An action that carried both the daily and the weekly count past a milestone also sent two almost identical messages; the longer run is the one worth hearing about.
2 participants
No description provided.