Skip to content

Ask people to agree to the terms and privacy policy before they sign in - #82

Open
shanerbaner82 wants to merge 2 commits into
mainfrom
feat/accept-terms
Open

shanerbaner82 wants to merge 2 commits into
mainfrom
feat/accept-terms

Conversation

@shanerbaner82

Copy link
Copy Markdown

What

The sign-in screen has a checkbox, "I agree to the terms of service and privacy policy. There's no tolerance for objectionable content or abusive users.", with links to both pages (dev-null.sh/terms and /privacy, in the app's browser). Sign in with GitHub and Sign in with Apple stay disabled until it's ticked. This is what App Review's guideline 1.2 asks of an app with user-generated content.

The phone sends terms_version (Client::TermsVersion, 2026-10-23) with the sign-in call for both ways in, so devnull records what was accepted.

Depends on

dev-null-sh/dev-null.sh#1042, which records the version and updates the terms and privacy wording. Until it's merged the server ignores terms_version; sign-in still works. Client::TermsVersion has to match the server's config('legal.terms_version'), or the server answers 422.

Not in this PR

People already signed in on a phone aren't asked. The server now says in GET /me whether they accepted the version in force (terms_accepted), so a gate for them can follow.

Verified

Pest passes (1189, arch test excluded). Checked on the iPhone 17 Pro simulator: buttons grey until the box is ticked, then enabled.

🤖 Generated with Claude Code

Neither Sign in with GitHub nor Sign in with Apple works until the box is ticked, and links open the terms and privacy policy in the app's browser. The phone tells devnull which version it showed as it signs in (terms_version), which devnull records.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@simonhamp

Copy link
Copy Markdown
Member

Nice. Just waiting on the API PR to get merged first

@RaphaelFakhri RaphaelFakhri left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GitHub sign-in drops terms_version whenever onboarding is the root screen. In that case AuthCallback replaces /onboarding with a fresh Onboarding, so $acceptedTerms is back to false when finishSignIn() runs from mount(), and redeemSignIn() posts {"code":"c","code_verifier":"..."} with no version. The pop-back path (onResume() on the same instance) does send "terms_version":"2026-10-23", so you only see the bug when the callback arrives as a replace. The existing test "follows the deep link back, redeems the code with the verifier and signs in" also locks this in: it still expects a body without terms_version. Since signIn() and signInWithApple() already refuse to start without the tick, you can send Client::TermsVersion unconditionally in finishSignIn() and useAppleToken(), or carry it on the SignInAttempt.

The branch also conflicts with main in resources/views/native/onboarding.blade.php after #92, and vendor/bin/pint --test flags the double blank lines this PR adds to tests/Pest.php and OnboardingTest.php. CONTRIBUTING.md asks for before and after screenshots on iOS and Android in light and dark mode for this screen.

tests/Feature/Screens/OnboardingTest.php line 209: This asserts the GitHub redeem body without terms_version, which contradicts "sends terms_version with the sign-in call for both ways in". Add 'terms_version' => Client::TermsVersion here once the replace path sends it.


try {
$user = app(Client::class)->redeemSignIn($code, $attempt->verifier());
$user = app(Client::class)->redeemSignIn($code, $attempt->verifier(), $this->acceptedTerms ? Client::TermsVersion : null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When AuthCallback replaces onboarding, this runs from mount() on a new instance where $acceptedTerms is false, so the version is never sent. An attempt only exists after a ticked signIn(), so passing Client::TermsVersion here unconditionally is safe.

->and(file_get_contents(base_path('.env.example')))->not->toContain('DEVNULL_GITHUB_CLIENT_ID')->toContain("NATIVEPHP_DEEPLINK_SCHEME=devnull\n")
->and(file_get_contents(base_path('README.md')))->not->toContain('DEVNULL_GITHUB_CLIENT_ID')->not->toContain('device flow');
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pint (no_extra_blank_lines) flags the double blank line here, and again in tests/Pest.php at line 954.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants