diff --git a/VERSION b/VERSION index 8c6b1d1..ee62e7a 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.3.39 +1.3.40 diff --git a/app/Livewire/Admin/MailTemplates.php b/app/Livewire/Admin/MailTemplates.php index e8d4874..99ea784 100644 --- a/app/Livewire/Admin/MailTemplates.php +++ b/app/Livewire/Admin/MailTemplates.php @@ -78,16 +78,61 @@ class MailTemplates extends Component } } - /** Move one up or down the list an operator picks from. */ - public function move(string $uuid, int $by): void + /** + * Swap one with its neighbour. + * + * It used to add or subtract a fixed amount, which is not the same thing: + * with the seeded values ten apart, ±15 usually landed correctly — and + * sometimes landed on a value another template already had, or on the 0 + * floor along with three others. Equal `sort` values fall back to the name, + * so a click could reorder something else, or visibly nothing, which is + * indistinguishable from a click that never registered. + * + * A swap always moves exactly one place, or does nothing at the ends, and + * both of those are what the operator expects to see. + */ + public function move(string $uuid, int $direction): void { $this->authorize('customers.manage'); $template = MailTemplate::query()->where('uuid', $uuid)->first(); - if ($template !== null) { - $template->update(['sort' => max(0, $template->sort + $by)]); + if ($template === null) { + return; } + + // The one immediately above or below in the order the page shows — + // read through the same ordering, so the swap agrees with the list. + $neighbour = MailTemplate::query() + ->when($direction < 0, fn ($q) => $q + ->where(fn ($w) => $w->where('sort', '<', $template->sort) + ->orWhere(fn ($t) => $t->where('sort', $template->sort)->where('name', '<', $template->name))) + ->orderByDesc('sort')->orderByDesc('name')) + ->when($direction > 0, fn ($q) => $q + ->where(fn ($w) => $w->where('sort', '>', $template->sort) + ->orWhere(fn ($t) => $t->where('sort', $template->sort)->where('name', '>', $template->name))) + ->orderBy('sort')->orderBy('name')) + ->first(); + + if ($neighbour === null) { + return; // already at the end; nothing to swap with + } + + // Distinct values, even where the two were equal before: leaving them + // equal would put the order back in the hands of the name. + $mine = $template->sort; + $theirs = $neighbour->sort; + + if ($mine === $theirs) { + // Moving UP means taking the lower value, so the one the mover + // receives is below the neighbour's — not above it. The other way + // round swaps them into the order they were already in, which looks + // exactly like the bug this method was rewritten to fix. + $theirs = $mine + ($direction < 0 ? -1 : 1); + } + + $template->update(['sort' => $theirs]); + $neighbour->update(['sort' => $mine]); } public function render() diff --git a/resources/js/app.js b/resources/js/app.js index 1ba40a6..ad65d72 100644 --- a/resources/js/app.js +++ b/resources/js/app.js @@ -131,8 +131,96 @@ function recoverFrom419() { window.location.reload(); } +// A thin bar at the top of the window while a Livewire request is in flight. +// +// Reported as "man klickt und sieht nicht was passiert": every action in the +// console is a round trip, and a round trip with no sign of itself reads as a +// click that missed. wire:loading fixes that per element, one element at a time, +// and only where somebody remembered to add it — so this is the floor underneath +// all of them. +// +// Deliberately delayed: a request that answers in 80 ms does not need a bar, and +// flashing one for every keystroke on a .live field is worse than nothing. +// SHOW_AFTER_MS is the threshold below which a person perceives the response as +// immediate anyway. +const BUSY_SHOW_AFTER_MS = 140; +const BUSY_FADE_MS = 220; + +function requestIndicator() { + let bar = null; + let showTimer = null; + let inFlight = 0; + + const element = () => { + if (bar !== null) return bar; + + bar = document.createElement('div'); + // Inline styles, not a class: this has to work on the login page, on the + // 503 page, and anywhere else the app's stylesheet may not have loaded + // the utilities it would otherwise need. + bar.style.cssText = [ + 'position:fixed', 'top:0', 'left:0', 'height:2px', 'width:0%', + 'background:#c2560a', 'z-index:2147483647', 'pointer-events:none', + 'transition:width .25s ease-out,opacity .22s linear', 'opacity:0', + ].join(';'); + bar.setAttribute('aria-hidden', 'true'); + document.body.appendChild(bar); + + return bar; + }; + + const show = () => { + const el = element(); + el.style.opacity = '1'; + // Not to 100: the bar must never claim to be finished while the answer + // is still on its way. It creeps and then completes. + el.style.width = '70%'; + }; + + const hide = () => { + if (bar === null) return; + bar.style.width = '100%'; + bar.style.opacity = '0'; + window.setTimeout(() => { + if (inFlight === 0 && bar !== null) bar.style.width = '0%'; + }, BUSY_FADE_MS); + }; + + return { + start() { + inFlight++; + if (showTimer !== null) return; + showTimer = window.setTimeout(() => { + showTimer = null; + if (inFlight > 0) show(); + }, BUSY_SHOW_AFTER_MS); + }, + // Called for success AND failure: a request that fails is exactly the + // one somebody must not be left waiting on. + stop() { + inFlight = Math.max(0, inFlight - 1); + if (inFlight > 0) return; + + if (showTimer !== null) { + window.clearTimeout(showTimer); + showTimer = null; + } + + hide(); + }, + }; +} + +const busy = requestIndicator(); + document.addEventListener('livewire:init', () => { - window.Livewire.hook('request', ({ fail }) => { + window.Livewire.hook('request', ({ fail, respond }) => { + busy.start(); + // respond fires for every answer, whatever its status; fail() below + // handles the ones worth acting on. Both paths must release the bar, + // and respond runs first — so stopping here covers the failures too. + respond(() => busy.stop()); + fail(({ status, preventDefault }) => { // 503: the application is in maintenance mode, which during a // deployment is not a fault — it is the event being watched. @@ -159,6 +247,11 @@ document.addEventListener('livewire:init', () => { }); }); +// A navigation (wire:navigate) is not a request hook, and the bar would +// otherwise sit there after the page it belonged to is gone. +document.addEventListener('livewire:navigating', () => busy.start()); +document.addEventListener('livewire:navigated', () => busy.stop()); + // Connection banner ("Keine Verbindung" / "Verbindung wiederhergestellt" — // see connectionBanner() below): navigator.onLine and the browser's // online/offline events are both used, but neither is trusted alone — diff --git a/resources/views/livewire/admin/mail-templates.blade.php b/resources/views/livewire/admin/mail-templates.blade.php index 6f59c92..9fd668c 100644 --- a/resources/views/livewire/admin/mail-templates.blade.php +++ b/resources/views/livewire/admin/mail-templates.blade.php @@ -84,7 +84,17 @@ @else
@foreach ($templates as $template) + {{-- `opening` is local to the row: fetching the modal is a round + trip, and a click with nothing to show for it reads as a + click that missed. It clears when the modal is up (the + package announces that) and on a timer as well, so a request + that never answers cannot leave the row spinning for ever. --}}
$template->active, @@ -105,29 +115,43 @@

{{ $template->subject }}

- {{-- Icons, and one word for the state change. Two full-width - buttons per entry was most of the height. --}} + {{-- Every button reports its own click: pressed state on + the way down, disabled while the request is out. The + global bar at the top of the window says that SOMETHING + is happening; these say which thing. --}}
- - {{-- R20: editing opens a modal. A five-line textarea in a - list row is the height jump the rule exists to stop. --}} + list row is the height jump the rule exists to stop. + + The spinner is this button's own: opening the modal + is a Livewire request made by the MODAL component, + so wire:loading on this page never sees it. --}}
diff --git a/tests/Feature/Admin/MailTemplateTest.php b/tests/Feature/Admin/MailTemplateTest.php index 6180a43..538a234 100644 --- a/tests/Feature/Admin/MailTemplateTest.php +++ b/tests/Feature/Admin/MailTemplateTest.php @@ -10,6 +10,7 @@ use App\Models\SentMail; use App\Models\Subscription; use App\Services\Mail\MailTemplateRenderer; use App\Support\CompanyProfile; +use Illuminate\Support\Facades\File; use Livewire\Livewire; /** @@ -302,3 +303,84 @@ it('leaves an edited template alone when the seed runs again', function () { expect($template->fresh()->body)->toBe('Meine eigene Fassung.') ->and(MailTemplate::query()->where('name', 'Anfrage bestätigen')->count())->toBe(1); }); + +// ---- Moving one, and seeing that it moved ---- + +it('swaps a template with its neighbour rather than nudging a number', function () { + // It used to add or subtract a fixed amount, which is not the same thing: + // with values ten apart that usually landed right, and sometimes landed on a + // value another template already had — equal sort falls back to the name, so + // a click could reorder something else, or visibly nothing at all, which is + // indistinguishable from a click that never registered. + MailTemplate::query()->delete(); + + $first = MailTemplate::create(['name' => 'A', 'subject' => 'S', 'body' => 'B', 'sort' => 10]); + $second = MailTemplate::create(['name' => 'B', 'subject' => 'S', 'body' => 'B', 'sort' => 20]); + $third = MailTemplate::create(['name' => 'C', 'subject' => 'S', 'body' => 'B', 'sort' => 30]); + + $order = fn () => MailTemplate::query()->orderBy('sort')->orderBy('name')->pluck('name')->all(); + + $page = Livewire::actingAs(operator('Owner'), 'operator')->test(MailTemplates::class); + + $page->call('move', $third->uuid, -1); + expect($order())->toBe(['A', 'C', 'B']); + + $page->call('move', $third->uuid, -1); + expect($order())->toBe(['C', 'A', 'B']); + + $page->call('move', $third->uuid, 1); + expect($order())->toBe(['A', 'C', 'B']); +}); + +it('does nothing at the ends instead of pretending', function () { + // Both are what an operator expects to see: the top one cannot go up. A + // number that silently changed without the order changing was the bug. + MailTemplate::query()->delete(); + + $top = MailTemplate::create(['name' => 'A', 'subject' => 'S', 'body' => 'B', 'sort' => 10]); + $bottom = MailTemplate::create(['name' => 'B', 'subject' => 'S', 'body' => 'B', 'sort' => 20]); + + $page = Livewire::actingAs(operator('Owner'), 'operator')->test(MailTemplates::class); + $page->call('move', $top->uuid, -1); + $page->call('move', $bottom->uuid, 1); + + expect(MailTemplate::query()->orderBy('sort')->pluck('name')->all())->toBe(['A', 'B']) + ->and($top->fresh()->sort)->toBe(10) + ->and($bottom->fresh()->sort)->toBe(20); +}); + +it('separates two templates that shared a sort value', function () { + // Equal values put the order in the hands of the name, so a swap between + // them has to leave them distinct or the next click does nothing. + MailTemplate::query()->delete(); + + $a = MailTemplate::create(['name' => 'A', 'subject' => 'S', 'body' => 'B', 'sort' => 5]); + $b = MailTemplate::create(['name' => 'B', 'subject' => 'S', 'body' => 'B', 'sort' => 5]); + + Livewire::actingAs(operator('Owner'), 'operator') + ->test(MailTemplates::class) + ->call('move', $b->uuid, -1); + + expect($a->fresh()->sort)->not->toBe($b->fresh()->sort) + ->and(MailTemplate::query()->orderBy('sort')->orderBy('name')->pluck('name')->all())->toBe(['B', 'A']); +}); + +it('says a click landed, on the button and at the top of the window', function () { + // Reported as "man klickt und sieht nicht was passiert". Every action here is + // a round trip; a round trip with no sign of itself reads as a click that + // missed. The bar is the floor under every page, the per-button state says + // WHICH thing is happening. + $page = File::get(resource_path('views/livewire/admin/mail-templates.blade.php')); + + expect($page)->toContain('wire:loading.attr="disabled"') + ->toContain('animate-spin') + // Scoped to the one button, not to the whole page: every row disabling + // itself because a neighbour is moving is its own kind of confusing. + ->toContain('wire:target="move('); + + expect(File::get(resource_path('js/app.js'))) + ->toContain('BUSY_SHOW_AFTER_MS') + // Released on failure too — the request somebody must not be left + // waiting on is exactly the one that went wrong. + ->toContain('respond(() => busy.stop())'); +});