Repository navigation
refactor(auth): remove invite-code onboarding - #9113
NotThatKindOfDrLiz merged 17 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@dcadenas I pushed four takeover commits to address the review findings and waited for the replacement CI run to finish. Fixed:
Escalated: none. Declined: none. Verification:
I also updated the PR description to reflect the startup migration and #9116. @mbradley remains requested; I’m approving this revision. Please review the takeover commits and merge when you’re ready. |
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Reviewed the final takeover head after all local and hosted checks passed. The invite-removal scope is complete, the legacy logging invariant is preserved, and the compatibility cleanup has an explicit removal tracker.
|
Overlap heads-up: this broad auth cleanup intersects #9112 in Android configuration and localization output, and draft #8540 across the localization/generated-localization set. Please coordinate merge order; after either competing PR lands, rebase onto fresh origin/main, regenerate localization output, and verify that invite removal, authenticated key export, and list UI copy all remain intact. |
mbradley
left a comment
mbradley
left a comment
There was a problem hiding this comment.
@dcadenas I pushed one commit. It closes a gap the invite removal left in logging.
The PR keeps redactUriStringForLogs and rewrites its doc to say it covers legacy invite paths. That reasoning holds: iOS still claims /invite/* through the served AASA, so those links still open the app. But parseDeepLink no longer has an /invite branch, so a legacy link falls through to the unknown-path log, and that log printed uri.path raw. Before this PR the invite branch logged Parsed invite deep link (code [REDACTED]). After it, the code reaches diagnostics.
f63d82996 routes that log site through the helper, the way the other four sites in mobile/lib/services/deep_link_service.dart already are, and adds a test that goes through parseDeepLink.
The existing regression test calls redactUriStringForLogs directly and never enters parseDeepLink, which is why it stayed green through this change. The new test fails without the fix:
Expected: not contains 'ABCD-EFGH-SECRET'
Actual: 'Unknown deep link path: /invite/ABCD-EFGH-SECRET'
The message now reads Unknown deep link and carries the redacted URI, since the value is no longer only a path.
Validation
dart analyze and dart format clean on both files. 84 tests pass in mobile/test/services/deep_link_service_test.dart.
Hosted CI has not run on this head. GitHub created no Actions check suite for f63d82996, and the branch has conflicted with main since #9093 landed at 05:43Z today. The last full green run was on 0852b0dce, before that commit. Other pull requests built normally in the same window, so this is the branch state and not the runners. A rebase should restore the run.
Left to you
Both items come from the branch sitting behind main. @NotThatKindOfDrLiz already called for a rebase in her merge-order note; these are the specifics.
codemagic.yaml conflicts. #9093 added two lines on main next to the comment block this PR deletes. The resolution is to keep main's check_ios_analytics_product.sh lines and drop the eight-line GH_ACTIONS_PR_PREVIEW comment. I am not on @divinevideo/platform, so I left the file alone.
After the merge, two ratchet baselines need regenerating. They are accurate for this branch and stale against main:
service_god_file_sizes.txt lib/services/video_event_service.dart main 6499 branch 6411
async_safety_counts.txt test/blocs/fullscreen_feed/..._bloc_test.dart main 4 branch 5
async_safety_counts.txt test/unit/subscription_manager_tdd_test.dart absent on main, present here
Main deleted subscription_manager_tdd_test.dart and drove the other counts down after this branch was cut. On the merged tree the baseline reads as a raise:
FAIL [async_safety_ceiling]: baseline ADDED a key or RAISED a ceiling vs origin/main (may only shrink):
+added discarded_futures|test/unit/subscription_manager_tdd_test.dart 2
^raised discarded_futures|test/blocs/fullscreen_feed/fullscreen_feed_bloc_test.dart 4 -> 5
Regenerate under the SDK pin, per the note at mobile/scripts/check_async_safety_ceiling.sh:31-37.
One question
mobile/integration_test/e2e/service_suite_exclusions.txt:8 excludes video_creation_flow_test.dart because it "requires the invite service", and this PR removes that service from local_stack/docker-compose.yml. Nothing else under integration_test/e2e/ mentions invites, so the dependency looks environmental. Does that suite come off the exclusion list now, or does it still need something the invite container was providing?
@NotThatKindOfDrLiz the head moved past your approval.
Approving.
f63d829 to
140bd8c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@dcadenas Takeover complete on current head Fixed:
Declined: none. Escalated/unresolved: none. Verification is green on the current head: the pre-push gate passed 1,392 affected tests, and every hosted check passed, including all four Mobile CI shards, generated files, analysis, web preview, analytics build, and all 14 Linux service integration suites. Please review the takeover commits and keep the merge decision with you. |
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Reviewed current takeover head 879004c after all local and hosted checks passed. The branch is rebased, the retired invite flow is covered by Linux service CI, and the headless secure-storage dependency is isolated to the test harness.
879004c to
3c36c8e
Compare
This comment has been minimized.
This comment has been minimized.
|
Final takeover update: Current head:
No items remain escalated or unresolved. @dcadenas, please review the takeover commits; the merge decision remains yours. @realmeylisdev is requested through the normal reviewer-team rotation. |
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Approved current takeover head 3c36c8e after the final rebase. origin/main is an ancestor, the worktree is clean, the 1,392-test pre-push gate passed, and every hosted check—including all 14 Linux service suites—is green.
3c36c8e to
7d8a6fa
Compare
This comment has been minimized.
This comment has been minimized.
|
Superseding the earlier handoff after two additional
@dcadenas, the takeover is complete; please review the pushed commits and make the merge decision. @realmeylisdev remains requested through the normal reviewer-team rotation. |
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Approved final takeover head 7d8a6fa. GitHub reports MERGEABLE/CLEAN, current main is an ancestor, the worktree is clean, all 1,394 affected pre-push tests passed, and every hosted check—including all 14 Linux service suites—is green.
Removing the /invite branch from parseDeepLink sends those URIs to the
unknown-path log, which prints uri.path raw. Before this PR the branch
logged "Parsed invite deep link (code [REDACTED])", so the code never
reached diagnostics. iOS still claims /invite/* through the served AASA, so
a legacy link opens the app and writes the code out.
The redaction helper and its doc were deliberately kept for exactly this
case, and the same file already routes four other log sites through it. This
one was missed. Its test covers the helper rather than the path, which is why
it stayed green: reverting this fix leaves that test passing.
Verified the leak and the fix at the path level:
Expected: not contains 'ABCD-EFGH-SECRET'
Actual: 'Unknown deep link path: /invite/ABCD-EFGH-SECRET'
The message now says "Unknown deep link" and carries the redacted URI, since
the value is no longer only a path.
Claude-Session: https://claude.ai/code/session_01FscwT5Nw4noChSRnGbbybo
7d8a6fa to
dd0a7d4
Compare
Mobile PR PreviewPreview refreshed for Last refresh:
|
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Re-approved the final rebased head dd0a7d4. I resolved the current-main conflict, preserved the newer detached async cleanup behavior, restored the router async import required by current main, and reconciled the async-safety ratchet. All hosted checks, including the 14-suite service integration job, are green.
|
Final takeover handoff for |
Description
Invite codes are no longer part of onboarding, but the app still carried a remotely controlled invite gate and its supporting code. That leftover code could re-enable obsolete onboarding or surface invite language when server configuration changed, so this removes it end to end. Collaborator invitations and email-verification PINs are separate features and keep working.
What changed
invite_api_clientpackage.EmailVerificationCubitacross environment changes by keying its provider on its remaining live dependencies.mainand reconciled the current Codemagic analytics validation.Legacy
/invite/*links/inviteapp-link prefix, so legacy links fall through to the web page./invite/*during the coordinated retirement. The mobile coverage test records that hold explicitly. Until the web claim is withdrawn, legacy invite values remain redacted from diagnostics.Related issue
Closes #8537
Refs #9116
Refs divinevideo/divine-context#100
Out of scope
invite.Verification
dart analyzeoverlib,test,integration_test, andtools: no issues.video_creation_flow_test.dart.Risks
/invite/*interim behavior remains until the web-owned AASA claim is withdrawn.Type of change