Repository navigation
SCA-1808: Offer yearly switches in mobile retention with a UID holdout - #25158
Git-on-my-level wants to merge 17 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84bf580661
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from utils.yearly_switch_retention import erase_assignment | ||
|
|
||
| erase_assignment(uid, client) |
There was a problem hiding this comment.
Every account wipe now imports utils.yearly_switch_retention from the database layer, reversing the dependency hierarchy explicitly required by backend/AGENTS.md. This also couples the shared erasure primitive to the retention business-logic module and its experiment/telemetry imports just to delete one document. Move the assignment-deletion primitive into database/ and import it at module scope from both callers, preserving the existing retry-on-failure behavior.
AGENTS.md reference: backend/AGENTS.md:L168-L174
Useful? React with 👍 / 👎.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd41b1354c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from database.account_deletion_policy import account_deletion_blocks_access | ||
| from database.conversations import get_conversations_count |
There was a problem hiding this comment.
The new admission handler imports its deletion policy, conversation counter, and fallback recorder inside request/error branches; utils/yearly_switch_retention.py::admit likewise imports the Firestore accessor inside the function. These additions violate the explicit module-level-import requirement in backend/AGENTS.md, leaving dependencies hidden until those branches execute. Move these imports to module scope while continuing to call get_firestore_client() lazily. This is separate from the previously flagged database-to-utils dependency reversal.
AGENTS.md reference: backend/AGENTS.md:L168-L174
Useful? React with 👍 / 👎.
| if (!await _checkEnabled() || !mounted || _candidate != candidate) return; | ||
| final switched = await showYearlySwitchSheet(context, | ||
| surface: widget.surface, load: widget.loadPreview, apply: widget.apply, readEnabled: widget.readEnabled); |
There was a problem hiding this comment.
If a reader taps the yearly row and then Cancel Subscription or the discount button while _checkEnabled() is pending, the parent sets busy=true, but this continuation still opens the yearly confirmation because it only checked widget.busy before the await. With a slow cancellation or discount request, this exposes another billing action while the first is running; the yearly sheet calls the payment API directly and does not honor UsageProvider's payment-loading guard. Recheck widget.busy after the asynchronous flag read and before opening the sheet.
Useful? React with 👍 / 👎.
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
|
@kodjima33 UI changes involving Git-on-my-level require your approval. Review the current diff, then comment |
SCA-1808 offers the existing-price yearly switch before the mobile cancellation discount and once on Home for engaged monthly subscribers. Both placements share durable 50/50 UID experiment enrollment; only a newly enrolled treatment UID can display either offer. Controls survive retries, devices and flag toggles. Engagement v1 is eight completed, visible, non-discarded saved captures in 14 days, not eight distinct active days.
Existing invoice preview and price/amount/proration-pinned switch APIs remain the only billing path. Cancellation stays reachable while admission loads, after refusal and after payment failure. Both existing discount events are registered; yearly observations have closed placement/experiment properties. Account wipe explicitly erases the existing top-level UID experiment row and propagates failures for retry. No payer ledger, identity key, exporter or retention policy is introduced.
Hold before merge: David’s exact render/copy approval and UI-author approval remain required. The owner authorized retaining upstream
cf2cb6b0cefor source preparation only: above 130% text size, yearly preview values stack below labels. The candidate packet includes normal/150% comparisons; normal previews are byte-identical before/after, and yearly/cancellation English copy is unchanged. No approval is inferred from tests or the owner’s source-readiness decision. Prices, plans, ceilings, production flags and rollout are unchanged.Admission stays dark until an approval reference and separately authorized rollout are configured. After those gates,
YEARLY_SWITCH_RETENTION_ENABLEDdefaults on; explicit false stops both new placements while preserving controls, cancellation and the existing settings switch. No production access, merge, deployment or activation is part of this PR.UID assignment is not payer clustering. Linked accounts and duplicate customers may contaminate arms. Missing/ambiguous billing linkage is unknown/excluded with coverage reported; overall Stripe totals cannot prove arm-specific effects. Revenue basis, refunds/currency handling, UID denominator, missingness and uncertainty are specified in
backend/docs/experiments/EXP-006-yearly-switch-retention.md. Small cells and recoverable complements are suppressed. Measured lift is unknown; direct churn credit is 0pp.The verifier fails closed: local receipts, authority booleans, signatures and plausible URLs cannot establish provenance. An owner-authorized adapter must authenticate the producer and independently verify referenced facts. Exact release/approval, assignment/non-overlap, protected take-up, 30/60-day paid ends and realized net revenue by enrolled UID remain live acceptance requirements.
Required read-only live/maturity command:
Current result: WAITING, exit 2 for authorized serving, approval and mature arm-linked aggregate evidence. Source correctness and renders do not establish LIVE acceptance.
The owner-authorized second-look correction fixes exactly three findings:
discussion_r4560109077: yearly confirmation rechecks the parent busy state after its asynchronous enabled read. A regression delays that read, starts real Cancel Subscription, and verifies no yearly preview/sheet opens until cancellation finishes; a later attempt works.discussion_r4555538010: the UID assignment erase and experiment ID live indatabase/yearly_switch_retention.py. Account wipe imports the primitive at module scope. Delete target/order, propagated failure and retry semantics stay unchanged; account-purge failure/retry is exercised through the real wipe helper.discussion_r4560109069: the retention route and admission helper use module-level imports. Firestore clients are still constructed only at call time, with a default-client call-boundary regression.Current main
4f13c9db8ec089b7bb22c21a2eb8b485548073fcis merged without rebase/force-push to include the landed upstream guardrail repairs. No guardrail is weakened. The Firestore runtime witness patches the new module-level references while retaining its existing signature/profile, call-count and network-blocking assertions. The People purge fingerprint is re-audited after the import move; People subtree and generation-head erasure are unchanged.Focused validation: 165 backend tests (billing, admission, verifier, erasure, strict People generation and inventory); eight widget tests; clean targeted Flutter analysis; 31 owned loopback emulator checks, stopped afterward. Render/copy and repository-check receipts are recorded on SCA-1808. The existing candidate packet remains subject to David’s exact approval; synthetic prices are not live prices. No device/debugger/deployed-client acceptance is claimed.
The non-admitted mounted-Home correction remains intact: later genuine Home presentations may retry; rebuilds and focus-only changes do not. Existing assignment owns stable controls and once-only/cross-placement admission.
Return through in_review is routing under the latest owner RULING, not another full review round or author verdict. The independent second-look findings above are implementation dispositions, not an independent SHIP claim. David’s exact final render/copy approval and exact-head UI-author approval remain mandatory. Live/maturity acceptance remains WAITING; no activation or churn lift is authorized.
Candidate packet:
.agent-artifacts/ui-evidence/yearly-retention-20261115/PACKET.md; original renders and exact copy remain in.agent-artifacts/ui-evidence/yearly-retention/. The preserved upstream accessibility layout stacks preview values above 130% text size.Product invariants: INV-UI-1, INV-UI-3, INV-DATA-1.
Failure-Class: none
Why this is a shared primitive: both placements share UID experiment admission and the existing preview/charge path; account wipe and the admission deletion fence share the database-owned erase. No new billing or identity authority.
Current integration head:
26bdd57bcfafc3883d7373299452fc78b1cdde6e.Existing immutable render packet head:
32d7201844647eadd12425eb9249dca97031c38f.Integration repair on November 15: merged current
main(27ff8bb57f37567fb526b2956208c788d95972b8) into the same branch without rebase or force-push. Preserved both feature-flag lists and re-audited the combined account-erasure inventory. The inherited shard 8/8 timeout is addressed by upstreamc2e2e50972, which keeps the test assertions and its five-second cleanup deadline. Regenerated the Swift and four TypeScript clients' method-count comments (550 → 552); no executable code changed in those follow-ups.Bounded checks on m1: 389 backend tests pass, including the entire previously failing outbox file, billing, UID admission, retryable account erasure and strict People generation. Seventeen widget tests and eight render checks pass; targeted Flutter analysis is clean. All six fresh placement/preview/failure rasters are byte-identical to the prior packet, and all 129 yearly/cancellation/discount/retention English keys match. Original visible packet remains unchanged. These tests ran on the integration commit; the final follow-ups change only generated method-count comments.
Shared preflight: all 70 checks passed at
dd66f42ac111971f5cd0c80184a1a052fb69fdd7(399.68s). The final follow-up changes only four TypeScript method-count comments; Swift and TypeScript generator checks pass at the final head. Five final-head metadata checks and the bounded push gate pass, including backend typechecking (zero errors), selected backend units, OpenAPI/generated-output checks and a cold desktop Swift debug build. Shared preflight was reused via its documented hatch; remaining hook checks were retained. Receipts are attached on SCA-1808. Hosted CI is running and was not awaited. Required read-only live/maturity command remains WAITING, exit 2. David's exact render/copy and exact-head UI-author approval holds remain; no third full review, production access, merge, deployment, flag activation or churn-lift claim is authorized.