Skip to content

fix: restore cross-organisation invite flow for existing users#205

Open
roncodes wants to merge 9 commits intomainfrom
fix/cross-org-invite-flow
Open

fix: restore cross-organisation invite flow for existing users#205
roncodes wants to merge 9 commits intomainfrom
fix/cross-org-invite-flow

Conversation

@roncodes
Copy link
Copy Markdown
Member

Problem

Commit 7b80327 introduced CreateUserRequest with a hard Rule::unique('users', 'email') validation rule wired as $createRequest on UserController. This validation fires before any controller logic runs, meaning the historical cross-org invite branch — "if the email already exists, invite the user instead of creating a duplicate" — was completely unreachable. Any attempt to add a user whose email was already in the system returned:

An account with this email address already exists

A second compounding issue: the inviteUser() controller method (which contained the correct invite logic) had no registered route in routes.php. Only resend-invite was wired; invite-user was never registered.


Changes

src/Http/Requests/CreateUserRequest.php

  • Remove Rule::unique from the email field. Uniqueness is a business concern handled by the controller, not a shape/format concern that belongs in a FormRequest.
  • Make phone sometimes (optional). Invited users — both new and existing — may not supply a phone number at invite time; they complete their profile after accepting the invitation.

src/Http/Controllers/Internal/v1/UserController.php

  • createRecord(): After validation, check whether the submitted email already exists. If it does, redirect to the cross-org invite flow instead of attempting a duplicate insert. Returns invited: true in the response so the frontend can show the appropriate success message.
  • inviteUser(): Refactored to use the new shared inviteExistingUser() helper for the existing-user path, and adds an already-member guard.
  • inviteExistingUser() (new private method): Shared helper used by both createRecord() and inviteUser(). Checks for duplicate invitations via Invite::isAlreadySentToJoinCompany(), creates the Invite record, and dispatches the UserInvited notification.
  • acceptCompanyInvite(): Added idempotency guard — checks for an existing CompanyUser row before creating one, preventing a duplicate pivot row if the invite link is clicked more than once.

src/routes.php

  • Register the missing POST users/invite-user route pointing to UserController@inviteUser.

Behaviour After This Fix

Scenario Before After
Add user with new email Works Works (unchanged)
Add user with email from another org An account with this email address already exists ✅ Invite sent, invited: true returned
Add user already in current org ❌ Unique constraint error ✅ Clear error: This user is already a member of your organisation
Accept invite link twice ❌ Duplicate company_users row ✅ Idempotent — second click is a no-op
POST users/invite-user endpoint ❌ 404 (no route) ✅ Registered and functional

Related

  • Companion PR in fleetbase/iam-engine: adds the dedicated Invite User button and dialog to the IAM Users UI.

## Problem

Commit 7b80327 introduced CreateUserRequest with a hard
Rule::unique('users', 'email') validation rule wired as $createRequest
on UserController. This validation fires before any controller logic
runs, meaning the historical cross-org invite branch — 'if the email
already exists, invite the user instead of creating a duplicate' — was
completely unreachable. Any attempt to add a user whose email was
already in the system returned:

  'An account with this email address already exists'

A second issue: the inviteUser() controller method (which contained the
correct invite logic) had no registered route in routes.php. Only
resend-invite was wired; invite-user was never registered.

## Changes

### src/Http/Requests/CreateUserRequest.php
- Remove Rule::unique from the email field. Uniqueness is a business
  concern handled by the controller, not a shape/format concern that
  belongs in a FormRequest.
- Make phone 'sometimes' (optional). Invited users — both new and
  existing — may not supply a phone number at invite time; they complete
  their profile after accepting the invitation.

### src/Http/Controllers/Internal/v1/UserController.php
- createRecord(): After validation, check whether the submitted email
  already exists. If it does, redirect to the cross-org invite flow
  instead of attempting a duplicate insert. Returns invited:true in the
  response so the frontend can show the appropriate success message.
- inviteUser(): Refactored to use the new shared inviteExistingUser()
  helper for the existing-user path, and adds an already-member guard.
- inviteExistingUser() [new private method]: Shared helper used by both
  createRecord() and inviteUser(). Checks for duplicate invitations via
  Invite::isAlreadySentToJoinCompany(), creates the Invite record, and
  dispatches the UserInvited notification.
- acceptCompanyInvite(): Added idempotency guard — checks for an
  existing CompanyUser row before creating one, preventing a duplicate
  pivot row if the invite link is clicked more than once.

### src/routes.php
- Register the missing POST users/invite-user route pointing to
  UserController@inviteUser.
Ronald A Richardson added 8 commits April 20, 2026 23:17
…ting

The base Model constructor and Setting constructor both used the config key
'fleetbase.db.connection', which does not exist in config/fleetbase.php.
The correct key is 'fleetbase.connection.db'.

Because config() returns null for a missing key, $this->connection was being
set to null in every model's constructor. Laravel then falls back to
config('database.default') to resolve the connection. In sandbox mode,
Auth::setSandboxSession() sets database.default to 'sandbox', so every
model — including those that explicitly declare $connection = 'mysql'
(e.g. Invite, Setting) — was silently writing to the sandbox database,
because the parent constructor overwrote the child's property with null.

Symptoms:
- Invite records created during sandbox sessions were written to the sandbox
  database (or lost entirely if the sandbox invites table did not exist),
  while the email was still sent using the in-memory $invitation object
  (which had the code set by the 'creating' hook before the failed save).
  The user received an invite email with a code that could not be validated
  because no matching row existed in the production invites table.
- Setting reads/writes in sandbox mode were hitting the sandbox DB instead
  of the production settings table.

Fix:
- Model::__construct(): only apply the connection override when the child
  class has not already declared an explicit $connection, and use the
  correct config key 'fleetbase.connection.db' with a safe 'mysql' fallback.
- Setting::__construct(): same guard — preserve the explicit $connection =
  'mysql' declared on the class and use the correct config key.
…switching

The constructors introduced in both Model.php and Setting.php called:

    $this->connection = config('fleetbase.db.connection');

This config key does not exist (correct key: fleetbase.connection.db),
so config() returned null, overwriting the child class's declared
$connection property on every instantiation.

A subsequent attempt to guard with empty($this->connection) overcorrected
in the opposite direction: models without an explicit $connection
declaration (the sandbox-aware ones) were always resolved to 'mysql',
ignoring the database.default switch that sandbox mode sets to 'sandbox'.

The correct fix is simply to remove both constructors entirely:

- PHP initialises class property declarations before any constructor runs,
  so models that explicitly declare $connection = 'mysql' (Invite, Setting,
  User, Company, Role, Permission, etc.) will always use that value — no
  constructor logic needed.

- Models that do NOT declare $connection (ApiCredential, CompanyUser,
  Activity, etc.) correctly inherit null from Eloquent's base class, which
  causes Laravel to resolve the connection from config('database.default').
  In sandbox mode that is 'sandbox'; in normal mode it is 'mysql'. This is
  exactly the intended behaviour.

No functional change for any model — this purely restores the correct
connection resolution that existed before the constructors were added.
…onnectionName()

Reverts the two overcorrecting commits (561d3cd, 3865ec7) and replaces
them with the minimal, correct changes:

## 1. Config key typo fix (Model.php, Setting.php)

The base Model constructor and Setting constructor both used:

    $this->connection = config('fleetbase.db.connection');

The correct key is 'fleetbase.connection.db'. The wrong key returned null,
which caused $this->connection to be set to null on every instantiation,
making all models fall back to config('database.default'). In sandbox mode
that is 'sandbox', so models that explicitly declared $connection = 'mysql'
(Invite, Setting, User, Company, etc.) were silently overwritten.

Fixed to: config('fleetbase.connection.db', 'mysql')

The constructor is intentionally kept — it is the correct place to resolve
the configured production connection name for models that do not declare
their own $connection.

## 2. Invite::getConnectionName() override (Invite.php)

Invites must always be stored in and read from the production database,
regardless of sandbox mode. The $connection property alone is insufficient
because the base Model constructor overwrites it at instantiation time.

Overriding getConnectionName() is the Laravel-idiomatic way to enforce a
hard connection binding at the model level — it is called by Eloquent for
every query and cannot be overridden by runtime config changes.
The constructors in Model.php and Setting.php both set:

    $this->connection = config('fleetbase.connection.db', 'mysql');

On main, the wrong config key ('fleetbase.db.connection') returned null,
making the constructors effectively no-ops — models without an explicit
$connection declaration correctly fell back to database.default, which
sandbox mode switches to 'sandbox'. The typo fix made the constructors
actively harmful by always forcing 'mysql', breaking sandbox switching
for every model that does not declare its own $connection.

The correct fix is to remove both constructors entirely:
- Models that declare $connection = 'mysql' (User, Company, Invite, etc.)
  have that value set by PHP at class instantiation — no constructor needed.
- Models without an explicit $connection (ApiCredential, CompanyUser, etc.)
  correctly inherit null from Eloquent and follow database.default, which
  is 'sandbox' in sandbox mode and 'mysql' otherwise.

The Invite model is handled separately via getConnectionName() override.
These files should not have been modified. The constructor changes were
careless and had wide blast radius across all Fleetbase models. Reverting
to the exact state on main.
…ng to sandbox

The base Model constructor reads config('fleetbase.connection.db') to
resolve which database connection to use. Previously this config key was
never updated when sandbox mode was activated — only database.default was
switched to 'sandbox'. This meant that any model instantiated after the
sandbox switch would still resolve 'mysql' (or null, depending on the
config key typo) from the constructor, while models that relied solely on
database.default would correctly use 'sandbox'.

The fix updates both keys atomically in setSandboxSession() so that the
two connection resolution paths stay in sync:
- database.default       → used by models with no explicit $connection
- fleetbase.connection.db → used by the base Model constructor
…e config

setSandboxSession() now updates both database.default and
fleetbase.connection.db to 'sandbox' when sandbox mode is active.
The previous getConnectionName() implementation used
config('fleetbase.connection.db', 'mysql') — but since that config key
is now set to 'sandbox' during sandbox sessions, the fallback 'mysql'
would never fire and Invite records would still be written to sandbox.

Using env('DB_CONNECTION', 'mysql') reads the value directly from the
environment at boot time, which is never mutated by setSandboxSession(),
guaranteeing that Invite always resolves to the production connection
regardless of sandbox mode.
…ting

The constructors in Model.php and Setting.php both referenced the config
key 'fleetbase.db.connection' which does not exist in config/fleetbase.php.
The correct key is 'fleetbase.connection.db' (see config/fleetbase.php:29).

Previously this typo caused config() to return null, making the constructors
effectively no-ops and leaving connection resolution entirely to
database.default. Now that setSandboxSession() also updates
fleetbase.connection.db alongside database.default, the constructor will
correctly resolve to 'sandbox' in sandbox mode and to the configured
production connection otherwise — which is the intended behaviour.
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.

1 participant