Commit fc7060b9 authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix(branches): stop the branch switcher 500ing on an unbuilt executive view

config/branch_lock.php names executive.dashboard as the lock's destination,
but that route was never built. route() throws on an undefined name, so
switching to "كل الفروع" crashed in production after the session had already
been written — the user landed in all-branches mode via an error page.

isLocked() already refused to lock without the route, and that was believed
to make the whole feature dormant. It only made the *gating* dormant: the
guard sits on the decision, while the crash is at the dereference. Four other
sites turned the same name into a URL, and BranchSwitcher's was outside the
gate entirely. Auth/Login reached it only through the config key, so it does
not even contain the string "executive".

Route every caller through BranchContext::redirectRouteName(), which returns
the configured route when it exists and degrades to the dashboard when it
does not. The dashboard is the right fallback while the view is unbuilt: the
lock is dormant, so it is already unfiltered and showing the every-branch
numbers the user asked for.

The test pins the resolver in both directions and fails if any Livewire
component or middleware reads branch_lock.redirect_route directly again.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent 7af211fc
......@@ -6,6 +6,7 @@
use App\Domain\Identity\Services\PermissionService;
use App\Models\User;
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Route;
use Illuminate\Support\Str;
/**
......@@ -139,6 +140,35 @@ private function computeCanViewAll(?User $user): bool
&& $this->permissions->can($user, config('branch_lock.permission'));
}
/**
* Where the branch lock sends a user.
*
* The configured target is the cross-branch view, which is not built yet.
* route() throws on an undefined name rather than returning null, so every
* caller that *navigates* — the switcher, the login redirect, the
* middleware, the dashboard — must take the name from here instead of
* reading the config key raw. isLocked() guards the callers that *gate*;
* it cannot guard the ones that merely need somewhere to send a user.
*
* Falling back to the dashboard is correct while the view is unbuilt: the
* lock is dormant, so the dashboard is already unfiltered and showing
* exactly the every-branch numbers the user asked for.
*/
public function redirectRouteName(): string
{
$configured = config('branch_lock.redirect_route');
return $this->redirectRouteExists() ? $configured : 'dashboard';
}
/**
* Whether the cross-branch view the lock depends on has been built.
*/
public function redirectRouteExists(): bool
{
return Route::has(config('branch_lock.redirect_route'));
}
/**
* Viewing every branch, and therefore restricted to the cross-branch pages.
*/
......@@ -148,7 +178,7 @@ public function isLocked(): bool
// so until that route exists locking would 500 every page including the
// redirect target. Keeps this dormant rather than dangerous while the
// feature is only partly built.
if (! \Illuminate\Support\Facades\Route::has(config('branch_lock.redirect_route'))) {
if (! $this->redirectRouteExists()) {
return false;
}
......
......@@ -31,7 +31,7 @@ public function handle(Request $request, Closure $next): Response
return $next($request);
}
$redirectRoute = config('branch_lock.redirect_route');
$redirectRoute = $this->branch->redirectRouteName();
// Loop guard: the destination must always be reachable, even if someone
// removes it from the allow-list by accident.
......
......@@ -70,7 +70,7 @@ public function login(AuthService $authService): void
$branch->resolveForUser($result->user);
$defaultRoute = $branch->isLocked()
? config('branch_lock.redirect_route')
? $branch->redirectRouteName()
: app(LoginRedirectService::class)->getRedirectRoute($result->user);
$this->redirectIntended(route($defaultRoute));
......
......@@ -47,7 +47,7 @@ public function updatedSelectedBranch($value): void
$user->update(['preferred_branch_id' => null]);
$this->isPinned = false;
$target = route(config('branch_lock.redirect_route'));
$target = route($ctx->redirectRouteName());
} else {
$branchId = (int) $value;
$ctx->set($branchId);
......
......@@ -34,8 +34,10 @@ public function mount(): void
// Checked before the role map so someone viewing every branch is never
// dropped into a branch-scoped dashboard showing one branch's numbers.
if (app(\App\Domain\Shared\Context\BranchContext::class)->isLocked()) {
$this->redirect(route(config('branch_lock.redirect_route')));
$branchContext = app(\App\Domain\Shared\Context\BranchContext::class);
if ($branchContext->isLocked()) {
$this->redirect(route($branchContext->redirectRouteName()));
return;
}
......
<?php
namespace Tests\Feature;
use App\Domain\Shared\Context\BranchContext;
use Illuminate\Support\Facades\Route;
use Tests\TestCase;
/**
* The branch lock names its destination in config, but the cross-branch view
* it points at is not built yet. route() throws on an undefined name, so every
* caller that navigates — rather than merely gates — must resolve the target
* through BranchContext instead of reading the config key raw.
*/
class BranchLockRedirectTest extends TestCase
{
public function test_redirect_target_is_always_a_defined_route(): void
{
$name = app(BranchContext::class)->redirectRouteName();
$this->assertTrue(
Route::has($name),
"Branch-lock redirect target [{$name}] is not a defined route; route() would throw."
);
}
public function test_redirect_target_degrades_to_the_dashboard_while_unbuilt(): void
{
config(['branch_lock.redirect_route' => 'executive.not-built-yet']);
$this->assertSame('dashboard', app(BranchContext::class)->redirectRouteName());
}
public function test_redirect_target_honours_the_configured_route_once_it_exists(): void
{
Route::get('/__executive-probe', fn () => '')->name('executive.probe');
// ->name() is applied after the route is added, so the router's name
// lookup has to be rebuilt before Route::has() can see it.
Route::getRoutes()->refreshNameLookups();
config(['branch_lock.redirect_route' => 'executive.probe']);
$this->assertSame('executive.probe', app(BranchContext::class)->redirectRouteName());
}
public function test_lock_stays_dormant_while_its_destination_is_missing(): void
{
config(['branch_lock.redirect_route' => 'executive.not-built-yet']);
$this->assertFalse(app(BranchContext::class)->isLocked());
}
public function test_no_caller_reads_the_redirect_route_key_directly(): void
{
$offenders = [];
foreach (['app/Livewire', 'app/Http/Middleware'] as $dir) {
$files = new \RecursiveIteratorIterator(
new \RecursiveDirectoryIterator(base_path($dir))
);
foreach ($files as $file) {
if ($file->getExtension() !== 'php') {
continue;
}
if (str_contains(file_get_contents($file->getPathname()), "branch_lock.redirect_route")) {
$offenders[] = str_replace(base_path().'/', '', $file->getPathname());
}
}
}
$this->assertSame(
[],
$offenders,
'These read branch_lock.redirect_route directly instead of BranchContext::redirectRouteName(): '
.implode(', ', $offenders)
);
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment