fix(help): readonly users can't complete first-login onboarding (Fixes #3683)

[marcelfolaron]

Summary

Fixes #3683 — readonly users (role level 5) are trapped in an infinite first-login onboarding modal loop.

FirstTaskStep::handle() called the permission-guarded Tickets::quickAddTicket() before writing the firstLoginCompleted flag. Readonly users lack TicketsPermissions::CREATE, so the POST /help/firstLogin?step=end threw a 403 before the flag was ever persisted — and Helpermodal kept isFirstLogin true on every render, re-showing the modal indefinitely with no escape.

Reported and root-caused by @christiansteier (thank you — the trace was spot-on, incl. the observation that readonly is the de-facto default in the Add-User dialog).

Root cause

app/Domain/Help/Services/FirstTaskStep.php — the completion flag write was coupled to the optional first-task creation:

• quickAddTicket() is guarded by #[RequiresPermission(TicketsPermissions::CREATE, ...)].
• readonly role = ['scope' => 'project', 'verbs' => ['view']] → 403 on the POST.
• saveSetting('user.<id>.firstLoginCompleted', true) on the next line never runs.

Change

Wrap the optional ticket creation in a try/catch(Throwable) so a permission failure (or any other error) is report()-ed but does not block the load-bearing completion-flag write. Also skip creation on an empty headline. The first task is a convenience; onboarding completion is the invariant.

Single file: app/Domain/Help/Services/FirstTaskStep.php.

Test plan

• ./vendor/bin/pint --test
• ./vendor/bin/phpstan analyse --level=5
• php -l app/Domain/Help/Services/FirstTaskStep.php
• Full unit suite (./vendor/bin/phpunit)
• Add regression test: readonly-user handle() persists firstLoginCompleted and does not throw; editor/manager still creates the task (happy path unregressed).
• Manual: first-login as a readonly user → modal completes and does not reappear; first-login as an editor → task created + onboarding complete.

Acceptance checklist

1. Readonly first-login completes; firstLoginCompleted written; modal doesn't loop.
2. Editor/manager first-login still creates the first task + completes onboarding.
3. CI green (Pint + PHPStan L5 + php -l + unit) with the readonly regression test.

Notes / not in scope

• @christiansteier's complementary suggestion to preselect a sensible default role (e.g. editor) in the invite form (so admins don't silently create readonly users) is a good second fix but is a UX/product decision — left for a separate PR and a human call.
• Opened as draft; not marking ready or merging. Review gate: @marcelfolaron / @broskees.

cc @marcelfolaron @broskees

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Leantime OSS Maintainer Bot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Copilot]

Pull request overview

Fixes an onboarding dead-end where readonly users (lacking TicketsPermissions::CREATE) could never complete first-login onboarding because the completion flag write was blocked by a permission error thrown during optional first-task creation.

Changes:

• Wrap first-task creation (Tickets::quickAddTicket()) in a try/catch so onboarding completion proceeds even when ticket creation fails (e.g., 403 for readonly users).
• Skip ticket creation when the headline is empty.
• Add an in-code explanation of the invariant: the firstLoginCompleted flag must be persisted regardless of ticket creation success.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[marcelfolaron]

Status: triaged / fix-in-draft · Priority: P2 (functional lockout for readonly users; no data loss or security exposure) · Next action: drop the unused Auth import + add the readonly regression test, then mark ready · Owner: @marcelfolaron / @broskees

Automated maintainer first-response (advisory only — not an approval; not marking this draft ready; merge authority stays with @marcelfolaron / @broskees). Reviewed at head e89b1a72. Root-cause analysis lives on the linked issue #3683; this is the PR-level review of the diff.

1. Intent: Decouples the load-bearing firstLoginCompleted write from the optional first-task creation in FirstTaskStep::handle(), so a permission-limited user (readonly, no TicketsPermissions::CREATE) no longer 403s on quickAddTicket() before the completion flag is persisted — fixing the infinite onboarding-modal loop in #3683.

2. Change requests:

1. ⚠ CR Documentation #1 — unused import will fail CI. app/Domain/Help/Services/FirstTaskStep.php:6 adds use Leantime\Domain\Auth\Services\Auth; but Auth is never referenced anywhere in the diff. PHPStan (and likely a Pint/lint rule) flags unused imports — this will red the unit/static jobs. Remove the import (the only new import you actually use is Throwable).

   - use Leantime\Domain\Auth\Services\Auth;
     use Leantime\Domain\Help\Contracts\OnboardingSteps;

2. ⚠ CR Gitignore #2 (blocker) — behavior change ships without the regression test. The try/catch is the right shape, but the core guarantee (readonly handle() persists the flag and does not throw) is untested. Per the test-and-CI gate, a behavior change needs a test: assert that with a headline param and quickAddTicket() throwing a permission error, handle() still returns true and saveSetting('user.<id>.firstLoginCompleted', true) is called exactly once; plus a happy-path case (editor/manager) that still creates the task. This is the checklist item already listed as unchecked in the PR body — it's the blocker to ready.
3. ◐ CR Create Email Notification Class (including user settings to opt out) #3 — confirm report($e) is the intended sink, not silent swallow. report($e) routes to the configured logger so the permission failure is still observable — good. Just confirm this doesn't spam the log on every readonly first-login in a way that looks like a recurring error to ops; a one-line log-level note (or a narrower catch on the permission exception type rather than Throwable) would keep a genuine unexpected failure from being hidden among expected readonly denials. Non-blocking — Throwable + report() is defensible as-is.
4. ✓ Empty-headline guard is a good add. $params['headline'] !== '' alongside isset() correctly skips creation on a blank headline rather than creating an empty ticket — a real improvement over the original.

3. Acceptance checklist (must pass before merge):

1. Readonly first-login completes: handle() persists firstLoginCompleted and does not throw when quickAddTicket() 403s; modal does not reappear (CR Gitignore #2 regression test covers this).
2. Editor/manager first-login still creates the first task and completes onboarding (happy path unregressed).
3. CI green: Pint + PHPStan level 5 + php -l + full unit suite — with the unused Auth import removed (CR Documentation #1) and the readonly regression test present (CR Gitignore #2).

4. CI status: ⚠️ Couldn't read check-runs this cycle — confirm in the UI. Two flags: (a) the unused Auth import (CR #1) will fail static analysis until removed; (b) missing regression test on a behavior change (CR #2) is treated as a blocker to ready-for-review per the standing review policy. Both are quick to clear.

Advisory only — not an approval, not marking ready, not merging. Merge authority stays with @marcelfolaron / @broskees.