Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions app/Livewire/Books/BookIndex.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,19 @@ public function updateStatus(Book $book, string $status): void
'date_finished' => $status === 'read' && ! $book->date_finished ? now() : $book->date_finished,
]);

// Auto-remove from queue when marked as read
// Auto-remove from queue when marked as read. Queue reordering is
// organizational, so it must not bump updated_at — otherwise every
// shifted book jumps to the top of the "Recently Updated" sort.
if ($status === 'read' && $book->queue_position !== null) {
$oldPosition = $book->queue_position;
$book->update(['queue_position' => null]);

Book::where('user_id', Auth::id())
->where('queue_position', '>', $oldPosition)
->decrement('queue_position');
Book::withoutTimestamps(function () use ($book, $oldPosition) {
$book->update(['queue_position' => null]);

Book::where('user_id', Auth::id())
->where('queue_position', '>', $oldPosition)
->decrement('queue_position');
});
}
}

Expand Down
20 changes: 13 additions & 7 deletions app/Livewire/Books/BookShow.php
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ public function addToQueue(): void
->max('queue_position');
$maxPosition = is_numeric($maxPosition) ? (int) $maxPosition : 0;

$this->book->update(['queue_position' => $maxPosition + 1]);
// Queue position is organizational, not content — don't bump updated_at.
Book::withoutTimestamps(fn () => $this->book->update(['queue_position' => $maxPosition + 1]));
$this->book->refresh();
}

Expand All @@ -70,13 +71,18 @@ public function removeFromQueue(): void
$this->authorize('update', $this->book);

$oldPosition = $this->book->queue_position;
$this->book->update(['queue_position' => null]);

if ($oldPosition !== null) {
Book::where('user_id', Auth::id())
->where('queue_position', '>', $oldPosition)
->decrement('queue_position');
}
// Reordering the queue must not touch updated_at, or every shifted book
// would jump to the top of the "Recently Updated" sort on /books.
Book::withoutTimestamps(function () use ($oldPosition) {
$this->book->update(['queue_position' => null]);

if ($oldPosition !== null) {
Book::where('user_id', Auth::id())
->where('queue_position', '>', $oldPosition)
->decrement('queue_position');
}
});

$this->book->refresh();
}
Expand Down
57 changes: 35 additions & 22 deletions app/Livewire/Books/ReadQueue.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,22 +27,27 @@ public function addToQueue(Book $book): void
->max('queue_position');
$maxPosition = is_numeric($maxPosition) ? (int) $maxPosition : 0;

$book->update(['queue_position' => $maxPosition + 1]);
// Queue position is organizational, not content — don't bump updated_at.
Book::withoutTimestamps(fn () => $book->update(['queue_position' => $maxPosition + 1]));
}

public function removeFromQueue(Book $book): void
{
$this->authorize('update', $book);

$oldPosition = $book->queue_position;
$book->update(['queue_position' => null]);

// Reorder remaining items
if ($oldPosition !== null) {
Book::where('user_id', Auth::id())
->where('queue_position', '>', $oldPosition)
->decrement('queue_position');
}
// Reordering must not touch updated_at, or shifted books jump to the
// top of the "Recently Updated" sort on /books.
Book::withoutTimestamps(function () use ($book, $oldPosition) {
$book->update(['queue_position' => null]);

if ($oldPosition !== null) {
Book::where('user_id', Auth::id())
->where('queue_position', '>', $oldPosition)
->decrement('queue_position');
}
});
}

public function moveUp(Book $book): void
Expand All @@ -58,8 +63,10 @@ public function moveUp(Book $book): void
->first();

if ($swapWith) {
$swapWith->update(['queue_position' => $book->queue_position]);
$book->update(['queue_position' => $book->queue_position - 1]);
Book::withoutTimestamps(function () use ($book, $swapWith) {
$swapWith->update(['queue_position' => $book->queue_position]);
$book->update(['queue_position' => $book->queue_position - 1]);
});
}
}

Expand All @@ -76,8 +83,10 @@ public function moveDown(Book $book): void
->first();

if ($swapWith) {
$swapWith->update(['queue_position' => $book->queue_position]);
$book->update(['queue_position' => $book->queue_position + 1]);
Book::withoutTimestamps(function () use ($book, $swapWith) {
$swapWith->update(['queue_position' => $book->queue_position]);
$book->update(['queue_position' => $book->queue_position + 1]);
});
}
}

Expand All @@ -89,12 +98,14 @@ public function moveToTop(Book $book): void
return;
}

// Shift all items above down by 1
Book::where('user_id', Auth::id())
->where('queue_position', '<', $book->queue_position)
->increment('queue_position');
Book::withoutTimestamps(function () use ($book) {
// Shift all items above down by 1
Book::where('user_id', Auth::id())
->where('queue_position', '<', $book->queue_position)
->increment('queue_position');

$book->update(['queue_position' => 1]);
$book->update(['queue_position' => 1]);
});
}

public function moveToBottom(Book $book): void
Expand All @@ -113,12 +124,14 @@ public function moveToBottom(Book $book): void
return;
}

// Shift all items below up by 1
Book::where('user_id', Auth::id())
->where('queue_position', '>', $book->queue_position)
->decrement('queue_position');
Book::withoutTimestamps(function () use ($book, $maxPosition) {
// Shift all items below up by 1
Book::where('user_id', Auth::id())
->where('queue_position', '>', $book->queue_position)
->decrement('queue_position');

$book->update(['queue_position' => $maxPosition]);
$book->update(['queue_position' => $maxPosition]);
});
}

public function updateStatus(Book $book, string $status): void
Expand Down
99 changes: 99 additions & 0 deletions tests/Feature/BookQueueTimestampTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
<?php

declare(strict_types=1);

use App\Enums\ReadingStatus;
use App\Livewire\Books\BookIndex;
use App\Livewire\Books\ReadQueue;
use App\Models\Book;
use App\Models\User;
use Livewire\Livewire;

beforeEach(function () {
$this->user = User::factory()->create();
});

/**
* Create `count` queued books (positions 1..count) whose updated_at is
* backdated to a known point, so we can detect any spurious "touch".
*/
function queuedBooks(User $user, int $count, Carbon\Carbon $backdatedTo): Illuminate\Support\Collection
{
$books = collect(range(1, $count))->map(fn (int $position) => Book::factory()->wantToRead()->create([
'user_id' => $user->id,
'queue_position' => $position,
]));

Book::withoutTimestamps(fn () => Book::whereIn('id', $books->pluck('id'))
->update(['updated_at' => $backdatedTo]));

return $books->each->refresh();
}

it('does not bump other queued books updated_at when one is marked read', function () {
$past = now()->subDays(10);
[$first, $second, $third] = queuedBooks($this->user, 3, $past)->all();

// Marking the top-of-queue book read auto-removes it and re-numbers the rest.
Livewire::actingAs($this->user)
->test(BookIndex::class)
->call('updateStatus', $first->id, 'read');

$second->refresh();
$third->refresh();

// The shifted books were re-numbered...
expect($second->queue_position)->toBe(1)
->and($third->queue_position)->toBe(2);

// ...but their updated_at must be untouched (this is the regression we fixed).
expect($second->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'))
->and($third->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'));

// The book we actually changed *should* be updated for real.
$first->refresh();
expect($first->status)->toBe(ReadingStatus::Read)
->and($first->queue_position)->toBeNull()
->and($first->updated_at->timestamp)->toBeGreaterThan($past->timestamp);
});

it('does not bump other queued books updated_at when one is removed from the queue', function () {
$past = now()->subDays(10);
[$first, $second, $third] = queuedBooks($this->user, 3, $past)->all();

Livewire::actingAs($this->user)
->test(ReadQueue::class)
->call('removeFromQueue', $first->id);

$second->refresh();
$third->refresh();

expect($second->queue_position)->toBe(1)
->and($third->queue_position)->toBe(2)
->and($second->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'))
->and($third->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'));
});

it('does not bump updated_at when reordering the queue', function () {
$past = now()->subDays(10);
[$first, $second, $third] = queuedBooks($this->user, 3, $past)->all();

// moveToTop exercises the mass increment() branch.
Livewire::actingAs($this->user)
->test(ReadQueue::class)
->call('moveToTop', $third->id);

$first->refresh();
$second->refresh();
$third->refresh();

// Positions reshuffled: the third book is now first.
expect($third->queue_position)->toBe(1)
->and($first->queue_position)->toBe(2)
->and($second->queue_position)->toBe(3);

// No book's updated_at was touched by the reorder.
expect($first->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'))
->and($second->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'))
->and($third->updated_at->format('Y-m-d H:i:s'))->toBe($past->format('Y-m-d H:i:s'));
});
Loading