Repository navigation
Make achievements payload match Progresso - #480
Conversation
|
Approach:
This keeps retirement as metadata instead of deleting definitions because historical rows and public profiles still need resolvable names and descriptions. It filters by the explicit retired IDs rather than category because the Sharing category also contains live client-driven achievements. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The branch does not yet contain the implementation described by this pull request.
Reviewed changes Reviewed the complete branch diff and the achievement contract scope claimed by the pull request.
- Empty implementation commit: The head contains one commit and changes zero files relative to
main. - No contract surface: There is no API or mobile payload change to assess at this head.
⚠️ The requested behavior is not implemented
This branch changes zero files, so it cannot make achievements available to authenticated users or retire the stated targets. Merging this head would leave production behavior unchanged while presenting the ticket as completed.
Technical details
# Add the achievement implementation before merge
## Affected sites
- PR head `644b7aac74c52966eb3a0e6e13cc8b07b2955e2d`: no files differ from base `71c0240b982de47c0da3038fedace47a8727fe37`
- PR description: the summary claims behavior changes while verification still says `Pending implementation`
## Required outcome
- Implement the achievement availability and target retirement behavior described by the PR.
- Add the applicable unit coverage, then request review on the resulting diff.GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The new free-user response and analytics capture both misrepresent production behavior and should be corrected before merge.
Reviewed changes Reviewed the complete implementation added since the prior Pullfrog review at 644b7aa.
- Achievement availability: Exposed active achievement payloads to free and Pro users while retaining the whole-profile feature flag.
- Retired definitions: Marked seven social and cooperative achievements as retired, removed their grant paths and progress queries, and preserved earned historical badges.
- Stable response shape: Kept
AchievementsLockedandProTeaserwhile returningfalseandnull. - Product analytics: Added
achievements_viewedwithisProandearnedCountthrough the existing PostHog abstraction. - Regression coverage: Updated query, service, retirement, social-path, and analytics unit tests.
GPT Sol ｜ 𝕏
|
Review fix approach for #332:
This keeps the reviewed retirement and response contract unchanged. I rejected adding a separate achievement flag or a client supplied view marker because the ticket requires the existing GamificationFreeTier predicate and assigns pure human view precision to the blocked UI ticket. |
There was a problem hiding this comment.
Important
The new commit fixes the forward earning paths and narrows analytics emission, but the rollout still leaves existing free users unable to earn several badges that the payload presents as unlocked.
Reviewed changes Reviewed the substantive changes in 14c828e since the prior Pullfrog review at df0a065.
- Expanded free-tier earning: Removed Pro-only guards from active habit, goal, bad-habit, and onboarding achievement paths while retaining the shared feature-flag predicate.
- Narrowed analytics emission: Removed
achievements_viewedfrom the general profile query and kept capture on the dedicated achievements query. - Added prospective coverage: Added focused tests proving newly qualifying free users receive representative active achievements.
⚠️ Existing free users cannot earn one-time badges
The new commit makes these award paths available to free users only when the qualifying milestone occurs after deployment. Users who already created or completed the relevant records remain permanently ineligible for five active badges even though the response now presents them as unlocked.
Technical details
# Reconcile one-time achievements when eligibility expands
## Affected sites
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:223-224`: `Liftoff` requires `TotalLogCount == 1`, so the next log cannot grant it to a user with prior logs.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:251-255`: `FirstOrbit` requires the current habit count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:264-267`: `MissionControl` requires the current goal count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:280-284`: `GoalCrusher` requires the completed-goal count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:333-345`: an already-completed checklist returns before `OnboardingComplete` can be evaluated.
- `src/Orbit.Application/Gamification/Backfill/XpAwardLogBackfillService.cs`: the only gamification backfill reconstructs XP logs and does not reconcile achievements.
## Required outcome
- Idempotently grant these one-time active achievements to existing free users who satisfied their criteria while achievement earning was Pro-gated.
- Preserve the current behavior for users who have not yet satisfied each criterion and avoid duplicate achievement XP.
- Cover counts greater than one and an already-completed onboarding checklist in free-tier tests.GPT Sol ｜ 𝕏
|
Review fix approach for the existing-user reconciliation:
I rejected query-side reconciliation because GET handlers should not mutate XP and notifications. I also rejected changing equality checks to greater-than checks alone because existing inactive users would remain stranded until another matching event. |
|
Delivery status for the merge update:
The PR now needs an approving Pullfrog review or owner dismissal of the superseded review before GitHub will report it mergeable. |
|
@pullfrog review |
There was a problem hiding this comment.
Important
Retired badges can make the Progresso achievement counter exceed its total. The numerator and denominator need consistent semantics before merge.
Reviewed changes Reviewed the complete branch diff at bc1b119 against main, including API behavior and current mobile and web consumer assumptions.
- Achievement availability: Returns active achievement payloads to free and Pro users while preserving the existing whole-profile feature flag.
- Retired definitions: Marks seven social and cooperative achievements as retired, removes their grant and progress paths, and preserves earned historical badges.
- Earning policy: Applies the free-tier gamification predicate to active habit, goal, bad-habit, and onboarding achievement paths.
- Stable contract and analytics: Keeps
AchievementsLockedandProTeaserin the response, and emitsachievements_viewedonly from the dedicated achievements query. - Regression coverage: Updates query, service, retirement, social-path, and analytics unit tests.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The new commit fixes the achievement ratio without introducing new issues. The earlier rollout review remains outstanding.
Reviewed changes Reviewed the counter correction in 2417bea since the prior Pullfrog review at bc1b119.
- Consistent achievement total: Counted the returned active and earned retired badges, preventing the displayed numerator from exceeding its denominator.
- Regression coverage: Added a focused test proving all active badges plus one retired badge reports equal earned and total counts.
GPT Sol ｜ 𝕏
|
Final review fix handoff:
Local verification is green with 0 build errors, 12 focused profile tests, and 5,765 tests in the full suite. This PR now needs Thomas to dismiss the superseded rollout review or make a new scope decision. |
|
@pullfrog review |
There was a problem hiding this comment.
Important
Existing free users who crossed one-time milestones while those awards were Pro-gated remain unable to earn five active badges after this rollout.
Reviewed changes Reviewed the complete achievement availability, retirement, earning-policy, analytics, generated-artifact, and regression-test diff at 2417bea against main.
- Achievement availability: Returns active achievement payloads to free and Pro users while preserving the whole-profile feature flag.
- Retired achievements: Marks seven social and cooperative definitions as retired, removes their award and progress paths, and keeps earned historical badges resolvable.
- Free-tier earning: Removes Pro-only guards from active habit, goal, bad-habit, and onboarding award paths under the existing gamification unlock predicate.
- Response consistency: Keeps the existing contract fields and counts each returned active or earned retired badge in the total.
- Analytics and tests: Emits
achievements_viewedfrom the dedicated achievements query and updates focused application coverage.
⚠️ Existing free users remain stranded on one-time achievements
The removed Pro guards only help when a qualifying transition happens after deployment. A free user with existing qualifying data cannot naturally re-enter the exact-one or checklist-completion branches, so Progresso can now display Liftoff, First Orbit, Mission Control, Goal Crusher, and All Systems Go as active but unearned even though the user already satisfied them.
Technical details
# Reconcile one-time achievements when eligibility expands
## Affected sites
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:223-224`: `Liftoff` requires `TotalLogCount == 1`, so another log cannot award it to a user with multiple existing logs.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:251-255`: `FirstOrbit` requires the top-level habit count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:264-267`: `MissionControl` requires the goal count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:280-284`: `GoalCrusher` requires the completed-goal count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:333-335`: an already-completed checklist returns before `OnboardingComplete` is evaluated.
- `src/Orbit.Infrastructure/Migrations/20260630175236_EnableGamificationFreeTierFlag.cs:14-19`: the free-tier flag was enabled before this rollout, so affected accounts have already accumulated qualifying state.
- `tests/Orbit.Application.Tests/Services/GamificationServiceTests.cs:523-538`, `568-583`, `1004-1021`, `1074-1098`, and `1128-1145`: the new tests cover only fresh count-one transitions and a checklist completed during the current call.
## Required outcome
- Idempotently grant these five active achievements to eligible free users who satisfied their criteria while achievement earning was Pro-gated.
- Avoid duplicate achievement XP and preserve current behavior for users who have not met each criterion.
- Cover counts greater than one and an already-completed onboarding checklist.
## Suggested approach
- Reconcile from persisted habits, logs, goals, and onboarding state through the existing `IGamificationService.TryGrantAchievementsAsync` persistence and XP funnel rather than mutating from a query handler.GPT Sol ｜ 𝕏
Disposition of the stranded one-time achievements finding: filed, not fixed hereThe finding is correct and well evidenced. All five sites were confirmed against this branch at Filed as https://github.com/thomasluizon/orbit-tickets/issues/360, blocked by Why it is filed rather than fixed in this pull request.
Nothing else in the review is outstanding. All three review threads are resolved, and the incremental |
|
@pullfrog review |
There was a problem hiding this comment.
Important
The existing-user reconciliation gap is confirmed and still affects the payload this PR begins serving. Filing a follow-up records the defect, but it does not make this rollout correct for affected accounts.
Reviewed changes Reviewed the complete achievement availability, retirement, earning-policy, analytics, generated-artifact, test, and mobile-consumer scope at 2417bea against main.
- Achievement availability: Returns active achievement payloads to free and Pro users while preserving the whole-profile feature flag.
- Retired achievements: Marks seven definitions as retired, removes their award and progress paths, and keeps earned historical badges resolvable.
- Free-tier earning: Removes Pro-only guards from active habit, goal, bad-habit, and onboarding award paths under the existing gamification unlock predicate.
- Response consistency: Keeps the existing contract fields and counts each returned active or earned retired badge in the total.
- Analytics and coverage: Emits
achievements_viewedfrom the dedicated achievements query and updates focused application tests. - Consumer parity: The mobile shared schema remains compatible because no response field was removed, renamed, retyped, or newly required.
⚠️ Existing free users still receive incorrect one-time achievement state
The production free-tier flag was enabled before this rollout, so affected accounts have already crossed one-time milestones while awards were Pro-gated. This PR immediately exposes those active badges as unearned, but the exact-one checks and completed-checklist early return give those users no path to correct that state; a separately filed follow-up does not prevent the incorrect response between deployments.
Technical details
# Reconcile expanded one-time achievement eligibility before exposure
## Affected sites
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:223-224` — `Liftoff` only grants when the persisted log count is exactly one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:251-255` — `FirstOrbit` only grants when the top-level habit count is exactly one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:264-267` — `MissionControl` only grants when the goal count is exactly one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:280-284` — `GoalCrusher` only grants when the completed-goal count is exactly one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:333-345` — an already-completed onboarding checklist returns before `OnboardingComplete` can be evaluated.
- `src/Orbit.Infrastructure/Migrations/20260630175236_EnableGamificationFreeTierFlag.cs:14-19` — the free-tier flag was enabled before this availability change.
- `tests/Orbit.Application.Tests/Services/GamificationServiceTests.cs:523-538`, `568-583`, `1004-1021`, `1074-1098`, and `1128-1145` — the added coverage exercises fresh transitions only, not existing qualifying state.
## Required outcome
- Ensure eligible existing free users do not receive a permanently false unearned state for these five active achievements.
- Deliver idempotent reconciliation before or atomically with catalog exposure, or keep exposure gated until the filed reconciliation can deploy.
- Preserve achievement XP idempotency and cover counts greater than one plus an already-completed checklist.GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The reconciliation can still expose incorrect one-time achievement state because it starts after the API and omits soft deleted historical evidence.
Reviewed changes Reviewed the reconciliation added in 12d349c since the prior Pullfrog review at 2417bea.
- Eligibility reconstruction: Derived five one-time badges from users, habits, positive logs, goals, onboarding state, and existing grants.
- Shared grant funnel: Routed missing ids through
IGamificationServicefor idempotent XP, level, notification, feed, and persistence behavior. - Startup coordination: Added feature flag and completion marker checks around a one-time hosted sweep.
- Regression coverage: Added application and infrastructure tests for eligible and ineligible state, idempotence, marker behavior, and failure handling.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
Startup reconciliation now ignores the feature flag that controls free-tier achievement earning, so a disabled rollout can still create permanent badges and XP.
Reviewed changes Reviewed the startup coordination and generated architecture refresh added after a025af6.
- Blocking reconciliation: Replaced the delayed background sweep with startup gating and in-process retries until reconciliation succeeds or startup is cancelled.
- Generated architecture: Refreshed the architecture outputs to include the final reconciliation service and test references.
GPT Sol ｜ 𝕏
|
Round 5 approach for #332:
I rejected resolving flags one account at a time because the implementation proves that path grows one Users query per free account. I also rejected bypassing IFeatureFlagService from Application because feature-plan evaluation belongs in the existing flag abstraction. |
There was a problem hiding this comment.
Important
Deferred legacy accounts can now read the achievement catalog before their historical reconciliation completes.
Reviewed changes Reviewed ab398524 since the prior Pullfrog review at 80e57f2, including the API behavior and mobile contract impact.
- New-account initialization: Marked newly created users reconciled so accounts provisioned after the startup sweep are not stranded.
- Atomic reconciliation persistence: Moved each account stamp into the retryable achievement grant save and added a relational concurrency regression test.
- Read eligibility: Removed reconciliation timestamps from both achievement read decisions, leaving access based on Pro status or
gamification_free_tier.
GPT Sol ｜ 𝕏
…hievements-progresso # Conflicts: # architecture.html # architecture.json
|
@pullfrog review |
There was a problem hiding this comment.
Important
The latest architecture refresh and merge do not address the existing reconciliation-readiness finding. The full branch also has two blocking rollout risks: new accounts can be permanently excluded after a feature-flag cycle, and first deployment waits on an unbounded fleet-wide history load.
Reviewed changes Reviewed the complete branch at ad85cb1, including the achievement contract, free-tier earning, retired definitions, per-account reconciliation, hosted lifecycle, persistence behavior, tests, and the current mobile schema.
- Achievement availability: Returns the active catalog to Pro users and free users enabled for
gamification_free_tierwhile retaining existing response fields. - Retired achievements: Keeps seven historical definitions resolvable, removes their grant and progress paths, and displays retired badges only when earned.
- Free-tier earning: Applies the shared gamification unlock predicate to active achievement award paths.
- Historical reconciliation: Adds a per-account timestamp, reconstructs five one-time milestones, and grants them through the existing XP and notification funnel.
- Hosted rollout: Runs reconciliation during host startup and retries feature-locked accounts in the background.
- Consumer compatibility: The current mobile Zod schema still accepts
achievementsLocked, nullableproTeaser, and the returned achievement objects without a breaking field change.
GPT Sol ｜ 𝕏
|
Approach for review round 9: expose one single account reconciliation path from AchievementEligibilityReconciliationService and have the fleet sweep reuse it. GetAchievementsQuery will call that path only after entitlement succeeds and only when the historical backfill stamp is absent, before loading earned badges. Tests will cover the first read grant and stamp, a no work second read, locked access, and concurrent inline and background reconciliation with idempotent XP. This keeps the existing background worker as a backstop and avoids a second eligibility implementation. |
…hievements-progresso # Conflicts: # architecture.html # architecture.json
There was a problem hiding this comment.
Important
The new lazy repair path can return 500 when it overlaps the background sweep because its duplicate-insert conflict bypasses the existing retry.
Reviewed changes Reviewed the inline historical reconciliation added in 99ae1a3 since the prior Pullfrog review at ad85cb1.
- Reconciled on first read: Added synchronous historical eligibility repair before the dedicated achievements query loads earned badges.
- Extracted shared eligibility loading: Reused the same persisted habit, log, goal, onboarding, and earned-badge reconstruction for fleet and single-account repair.
- Hardened idempotence: Skipped grant work when the retryable persistence path reloads an account whose reconciliation stamp is already present.
- Added concurrency coverage: Exercised simultaneous inline and background repair with two contexts and asserted one badge, one XP award, and one persisted stamp.
GPT Sol ｜ 𝕏
…hievements-progresso # Conflicts: # architecture.html # architecture.json
|
@pullfrog review |
There was a problem hiding this comment.
Important
The current head still contains the previously anchored unbounded startup sweep, permanent flag-cycle stamp, and PostgreSQL duplicate-grant race. The mobile and web profile path also remains unreconciled, as noted inline.
Reviewed changes Reviewed the complete 51-file, 20-commit branch at 71bfd20, including the current mobile consumer contract and endpoint usage.
- Catalog behavior: Exposes 32 active achievements to eligible free and Pro accounts, retains earned retired badges, and keeps existing response fields.
- Earning policy: Extends active habit, goal, bad-habit, and onboarding awards to unlocked free accounts while removing retired social grant paths.
- Historical reconciliation: Reconstructs five one-time milestones, grants through the XP funnel, and records per-account completion.
- Rollout lifecycle: Adds startup and deferred sweeps plus a dedicated achievements-query inline repair.
- Persistence and coverage: Adds the nullable user stamp, partial index, migration, concurrency-oriented tests, and updated application and infrastructure coverage.
- Consumer contract: Confirms the shared Zod schema remains field-compatible and that both shipped achievements screens consume the profile endpoint.
GPT Sol ｜ 𝕏
|
Round 10 approach for #332:
I am reusing the repository's DbUniqueViolation pattern and one reconciliation implementation. I rejected process locks because the inline read and hosted sweep run in separate scopes and the database uniqueness rule is the authoritative concurrency boundary. |
There was a problem hiding this comment.
Important
The delta fixes the profile, startup, and duplicate-insert paths, but the existing feature-flag-cycle reconciliation finding remains open and still requires correction before merge.
Reviewed changes Reviewed the reconciliation simplification in b3ad8656 since the prior Pullfrog review at 71bfd20b.
- Aligned read repair: Reconciled unlocked unstamped accounts before both the dedicated achievements response and the gamification profile consumed by mobile and web.
- Bounded fleet processing: Limited each worker pass to 100 accounts plus one lookahead row and moved the first pass off the host startup path.
- Convergent grant races: Retried PostgreSQL unique violations through a tracking reset and persisted-state reload so concurrent inline and background grants converge.
- Corrected account initialization: Left new accounts unstamped until an unlocked reconciliation actually evaluates their persisted eligibility.
- Expanded regression coverage: Added paging, nonblocking startup, profile repair, and provider-shaped unique-conflict tests while preserving the shipped mobile contract.
GPT Sol ｜ 𝕏
|
Round 11 approach: merge origin/main into this branch without rebasing. Resolve the three social command conflicts by keeping main's NotificationUrls based null URL shape while preserving this branch's removal of retired achievement grants. Resolve architecture.html from one side only, then regenerate architecture.json and architecture.html with tools/arch-map.mjs and rebuild the API contract if openapi.json moves. Verify the seven retired ids remain only in AchievementDefinitions.cs, no inline streak or social notification URL returns, and round 10 reconciliation behavior remains unchanged. This shape preserves both tickets' independent behavior and avoids hand editing generated artifacts. |
There was a problem hiding this comment.
Important
No new Critical or High issues were introduced by this synchronization. The existing feature flag cycle reconciliation thread remains open and still blocks approval.
Reviewed changes Reviewed the main synchronization in d6b0bc53 since the prior Pullfrog review at b3ad8656.
- Integrated social access guards: Reconciled the current accountability acceptance, friendship acceptance, and cheer guard entry points without restoring retired achievement award paths.
- Preserved consumer compatibility: Kept the achievement response contract unchanged and compatible with the shipped mobile Zod schema.
- Validated affected commands: Confirmed all 31 focused accountability, friendship, and cheer command tests pass.
GPT Sol ｜ 𝕏
|
@pullfrog review |
There was a problem hiding this comment.
Important
Reconciliation still misses eligibility across later flag cycles and reopened goals, and it can race normal award paths in a way that rolls back earned XP.
Reviewed changes Reviewed the complete branch at d6b0bc5, the existing same-head feedback, and the current mobile consumer contract.
- Catalog access and retirement: Exposes the active catalog to eligible free and Pro accounts, retains earned retired badges, and removes retired social grant paths.
- Unified earning and analytics: Applies the free-tier predicate to active achievement earning and emits
achievements_viewedfrom the dedicated query. - Historical reconciliation: Reconstructs five one-time milestones, repairs both achievement read paths, and records account-level completion.
- Persistence and worker lifecycle: Adds the nullable user stamp, partial index, bounded hosted worker, and duplicate-grant retry coverage.
- Consumer compatibility: Keeps the DTO shape append-only and compatible with the shipped
orbit-ui-mobileZod schema; no paired mobile PR is open.
GPT Sol ｜ 𝕏
|
Split approach for #332:
This removes the historical backfill subsystem rather than attempting a twelfth reconciliation repair round. |
…hievements-progresso # Conflicts: # architecture.html # architecture.json
|
There was a problem hiding this comment.
Important
The new delta removes the only historical repair path while immediately exposing the active catalog, so existing free accounts can remain permanently incorrect. This must be resolved before merge.
Reviewed changes Reviewed the reconciliation removal and main synchronization since the prior Pullfrog review at d6b0bc5.
- Removed historical reconciliation: Deleted the eligibility service, worker, user stamp, migration, query hooks, registrations, repository helpers, and dedicated tests, and deferred that behavior to a follow-up.
- Preserved ordinary grant convergence: Retained the PostgreSQL unique-violation retry and focused test for concurrent
TryGrantAchievementsAsynccalls. - Integrated current base behavior: Brought in the latest
mainchanges, including per-goal reward identity, without changing the shipped achievement DTO shape. - Refreshed generated architecture: Removed the deleted reconciliation components and references from the architecture artifacts.
⚠️ Historical milestones are exposed without any repair path
The current award predicates grant these badges only on the original transition, but the new commit deletes every reconciliation path before both achievement queries expose the active catalog. Existing free accounts that crossed those transitions while awards were Pro-gated will remain visibly unearned and will never receive the corresponding achievement XP.
Technical details
# Reconcile existing one-time eligibility before exposure
## Affected sites
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:223-224`: `Liftoff` requires the total log count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:251-255`: `FirstOrbit` requires the top-level habit count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:264-267`: `MissionControl` requires the goal count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:288-292`: `GoalCrusher` requires the completed-goal count to equal one.
- `src/Orbit.Application/Gamification/Services/GamificationService.cs:340-342`: an already-completed checklist returns before `OnboardingComplete` can be evaluated.
- `src/Orbit.Application/Gamification/Queries/GetAchievementsQuery.cs:42-58` and `src/Orbit.Application/Gamification/Queries/GetGamificationProfileQuery.cs:53-65`: both paths expose the active catalog without historical repair.
- `tests/Orbit.Application.Tests/Services/GamificationServiceTests.cs:526-540`, `571-585`, `1030-1046`, `1099-1124`, and `1153-1170`: coverage exercises only fresh transitions after enablement.
- PR body: the cited follow-up `#362` resolves to an unrelated, already-closed observability pull request in this repository.
## Required outcome
- Before or atomically with catalog exposure, idempotently grant these five badges and their XP to existing accounts with durable qualifying evidence, or keep affected reads gated until that repair deploys.
- Preserve exactly-once XP and achievement delivery.
- Cover counts greater than one, an already-completed onboarding checklist, and historical goal completion evidence.GPT Sol ｜ 𝕏
Disposition of the historical-repair finding: split by an explicit human decision, filed as #362The finding is correct on its facts and it is not being dismissed. It is being sequenced, by Thomas, Filed as https://github.com/thomasluizon/orbit-tickets/issues/362, blocked by this ticket, in the Why this pull request should merge without it. This PR is a strict improvement for every account the finding names. Today a free account sees an empty So the gap that remains is strictly narrower than the gap that is closed, and no account is worse off Why it is being split rather than finished here. Eleven review rounds. Every one raised genuinely new, real defects rather than repeats, and the last That is a subsystem, not a follow-up chore. It deserves to be designed once, against a written set of This was escalated and decided by a human. The same split was proposed earlier in this run and No reconciliation code, state, migration or test remains on this branch. |
|
@pullfrog review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the complete 36-file branch at fd016ba, the current orbit-ui-mobile consumers, passing required checks, and the owner's same-head disposition of the prior historical-repair finding.
- Catalog access: Eligible free and Pro accounts receive the active achievement catalog through both API paths used by the shipped clients.
- Retired definitions: Seven social achievements remain resolvable for earned historical badges while their grant paths and progress reads are removed.
- Earning and grant safety: Active achievements follow the free-tier predicate, and concurrent duplicate grants reset tracking and converge on persisted state without replaying XP or push delivery.
- Contract compatibility: No shipped DTO field is removed, renamed, retyped, or newly required; the mobile Zod schema accepts the retained lock and teaser fields plus the additive progress fields.
- Rollout boundary: Historical repair for five one-time milestones is deliberately sequenced into the private
orbit-tickets#362follow-up after an explicit owner decision, rather than remaining an accidental omission in this branch.
GPT Sol ｜ 𝕏
Closes #332
Summary
Historical eligibility split
This PR ungates achievements. It does not backfill history.
Five one-time achievements remain unearned for free accounts that crossed their milestone before this change: Liftoff, First Orbit, Mission Control, Goal Crusher, and All Systems Go. That work is tracked by #362, which is blocked by this ticket.
This is a pre-existing condition that this PR does not worsen. Before this change, a free account earns no achievement at all. After it, every eligible free account sees and earns active achievements from deploy onward.
The achievement eligibility reconciliation service, hosted worker, user state, migration, query hooks, repository helpers, registrations, and reconciliation-only tests were removed from this branch. XpAwardLogBackfillService remains because it is unrelated.
Concurrent grant safety
TryGrantAchievementsAsync keeps the duplicate-key retry as a standalone safety property for ordinary award paths. Two concurrent report-event requests can still attempt the same UserAchievement insert without any reconciliation worker. The retry clears tracking and recomputes against persisted state so the losing request does not duplicate XP or push delivery.
External interface evidence
The installed Npgsql 10.0.3 assembly was reflected before using its test exception shape. Npgsql.PostgresException exposes a string SqlState property and a four string constructor. The duplicate-grant test uses SqlState 23505, which the existing DbUniqueViolation helper recognizes. Reproduce by loading Npgsql.dll with System.Reflection and listing PostgresException constructors and the SqlState property.
Verification
Assumptions