From b4d920fab6bbbd81853f7e14c8d4bd11c227745f Mon Sep 17 00:00:00 2001 From: Ash Allen Date: Fri, 17 Mar 2023 15:14:03 +0000 Subject: [PATCH 1/7] Added check to prevent more than 4 articles from being pinned. --- app/Http/Controllers/Admin/ArticlesController.php | 6 ++++++ app/Models/Article.php | 10 ++++++++++ lang/en/errors.php | 1 + 3 files changed, 17 insertions(+) diff --git a/app/Http/Controllers/Admin/ArticlesController.php b/app/Http/Controllers/Admin/ArticlesController.php index ed4b0958a..8a18ff904 100644 --- a/app/Http/Controllers/Admin/ArticlesController.php +++ b/app/Http/Controllers/Admin/ArticlesController.php @@ -67,6 +67,12 @@ public function togglePinnedStatus(Article $article) { $this->authorize(ArticlePolicy::PINNED, $article); + if (!$article->isPinned() && !$article->canBePinned()) { + $this->error('errors.article_pinned_limit_exceeded'); + + return redirect()->route('articles.show', $article->slug()); + } + $article->is_pinned = ! $article->isPinned(); $article->save(); diff --git a/app/Models/Article.php b/app/Models/Article.php index b49dc09d8..e52e8b44e 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -356,4 +356,14 @@ public function toFeedItem(): FeedItem ->link(route('articles.show', $this->slug())) ->authorName($this->author()->name()); } + + /** + * Check whether the article can be pinned. Only 4 articles can be pinned at a time. + * + * @return bool + */ + public function canBePinned(): bool + { + return $this->pinned()->count() < 4; + } } diff --git a/lang/en/errors.php b/lang/en/errors.php index 55c8844e8..635144801 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -6,4 +6,5 @@ 'github_invalid_state' => 'The request timed out. Please try again.', 'github_account_too_young' => 'Your Github account needs to be older than 2 weeks in order to register.', 'github_account_exists' => 'We already found a user with the given GitHub account (:username). Would you like to login instead?', + 'article_pinned_limit_exceeded' => 'There are already 4 pinned articles. Please unpin one before pinning another.', ]; From 66c26f9be4e6ede02429894165bbe681608fbbc8 Mon Sep 17 00:00:00 2001 From: Ash Allen Date: Fri, 17 Mar 2023 15:25:20 +0000 Subject: [PATCH 2/7] Added and updated tests. --- tests/Feature/AdminTest.php | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/tests/Feature/AdminTest.php b/tests/Feature/AdminTest.php index 560beb269..35827195f 100644 --- a/tests/Feature/AdminTest.php +++ b/tests/Feature/AdminTest.php @@ -264,7 +264,9 @@ $this->loginAsAdmin(); - $this->put("/admin/articles/{$article->slug()}/pinned"); + $response = $this->put("/admin/articles/{$article->slug()}/pinned"); + $response->assertSessionHas('success'); + $response->assertSessionMissing('error'); expect($article->fresh()->isPinned())->toBeTrue(); }); @@ -297,6 +299,26 @@ expect($article->fresh()->isPinned())->toBeFalse(); }); +test('admin cannot pin article if there are already four pinned articles', function () { + Article::factory() + ->count(4) + ->create([ + 'submitted_at' => now(), + 'approved_at' => now(), + 'is_pinned' => true, + ]); + + $article = Article::factory()->create(['submitted_at' => now(), 'approved_at' => now()]); + + $this->loginAsAdmin(); + + $response = $this->put("/admin/articles/{$article->slug()}/pinned"); + $response->assertSessionMissing('success'); + $response->assertSessionHas('error'); + + expect($article->fresh()->isPinned())->toBeFalse(); +}); + test('admins can unpin articles', function () { $article = Article::factory()->create([ 'submitted_at' => now(), From e6b17b6b3b40e46a38e006a90f9bb3390c3f6832 Mon Sep 17 00:00:00 2001 From: ash-jc-allen Date: Fri, 17 Mar 2023 15:26:45 +0000 Subject: [PATCH 3/7] Fix code styling --- app/Http/Controllers/Admin/ArticlesController.php | 2 +- app/Models/Article.php | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/Http/Controllers/Admin/ArticlesController.php b/app/Http/Controllers/Admin/ArticlesController.php index 8a18ff904..78acb8b65 100644 --- a/app/Http/Controllers/Admin/ArticlesController.php +++ b/app/Http/Controllers/Admin/ArticlesController.php @@ -67,7 +67,7 @@ public function togglePinnedStatus(Article $article) { $this->authorize(ArticlePolicy::PINNED, $article); - if (!$article->isPinned() && !$article->canBePinned()) { + if (! $article->isPinned() && ! $article->canBePinned()) { $this->error('errors.article_pinned_limit_exceeded'); return redirect()->route('articles.show', $article->slug()); diff --git a/app/Models/Article.php b/app/Models/Article.php index e52e8b44e..533493428 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -359,8 +359,6 @@ public function toFeedItem(): FeedItem /** * Check whether the article can be pinned. Only 4 articles can be pinned at a time. - * - * @return bool */ public function canBePinned(): bool { From acab0aaf768a9a849622bd989a4c218f4ac5131d Mon Sep 17 00:00:00 2001 From: Ash Allen Date: Fri, 17 Mar 2023 16:07:33 +0000 Subject: [PATCH 4/7] Unpin the oldest article when pinning a 5th article. --- .../Controllers/Admin/ArticlesController.php | 18 ++++++++---- lang/en/errors.php | 1 - tests/Feature/AdminTest.php | 29 +++++++++++++------ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/app/Http/Controllers/Admin/ArticlesController.php b/app/Http/Controllers/Admin/ArticlesController.php index 78acb8b65..5d8fb7660 100644 --- a/app/Http/Controllers/Admin/ArticlesController.php +++ b/app/Http/Controllers/Admin/ArticlesController.php @@ -11,6 +11,7 @@ use App\Policies\ArticlePolicy; use App\Queries\SearchArticles; use Illuminate\Auth\Middleware\Authenticate; +use Illuminate\Support\Facades\DB; class ArticlesController extends Controller { @@ -67,14 +68,19 @@ public function togglePinnedStatus(Article $article) { $this->authorize(ArticlePolicy::PINNED, $article); - if (! $article->isPinned() && ! $article->canBePinned()) { - $this->error('errors.article_pinned_limit_exceeded'); + $article = DB::transaction(function () use ($article): Article { + if (!$article->isPinned()) { + Article::pinned() + ->oldest() + ->take(1) + ->update(['is_pinned' => false]); + } - return redirect()->route('articles.show', $article->slug()); - } + $article->is_pinned = !$article->isPinned(); + $article->save(); - $article->is_pinned = ! $article->isPinned(); - $article->save(); + return $article; + }); $this->success($article->isPinned() ? 'admin.articles.pinned' : 'admin.articles.unpinned'); diff --git a/lang/en/errors.php b/lang/en/errors.php index 635144801..55c8844e8 100644 --- a/lang/en/errors.php +++ b/lang/en/errors.php @@ -6,5 +6,4 @@ 'github_invalid_state' => 'The request timed out. Please try again.', 'github_account_too_young' => 'Your Github account needs to be older than 2 weeks in order to register.', 'github_account_exists' => 'We already found a user with the given GitHub account (:username). Would you like to login instead?', - 'article_pinned_limit_exceeded' => 'There are already 4 pinned articles. Please unpin one before pinning another.', ]; diff --git a/tests/Feature/AdminTest.php b/tests/Feature/AdminTest.php index 35827195f..db5a7e926 100644 --- a/tests/Feature/AdminTest.php +++ b/tests/Feature/AdminTest.php @@ -264,9 +264,7 @@ $this->loginAsAdmin(); - $response = $this->put("/admin/articles/{$article->slug()}/pinned"); - $response->assertSessionHas('success'); - $response->assertSessionMissing('error'); + $this->put("/admin/articles/{$article->slug()}/pinned"); expect($article->fresh()->isPinned())->toBeTrue(); }); @@ -299,9 +297,15 @@ expect($article->fresh()->isPinned())->toBeFalse(); }); -test('admin cannot pin article if there are already four pinned articles', function () { - Article::factory() +test('admin can pin articles if there are already 4 pinned articles', function () { + $pinnedArticles = Article::factory() ->count(4) + ->sequence( + ['created_at' => now()->subDays(3)], + ['created_at' => now()->subDays(2)], + ['created_at' => now()->subDays(1)], + ['created_at' => now()], + ) ->create([ 'submitted_at' => now(), 'approved_at' => now(), @@ -312,11 +316,18 @@ $this->loginAsAdmin(); - $response = $this->put("/admin/articles/{$article->slug()}/pinned"); - $response->assertSessionMissing('success'); - $response->assertSessionHas('error'); + $this->put("/admin/articles/{$article->slug()}/pinned"); + + expect($article->fresh()->isPinned())->toBeTrue(); + + // Assert the oldest pinned article is no longer pinned + expect($pinnedArticles[0]->fresh()->isPinned())->toBeFalse(); + + // Assert the other pinned articles are still pinned + expect($pinnedArticles[1]->fresh()->isPinned())->toBeTrue(); + expect($pinnedArticles[2]->fresh()->isPinned())->toBeTrue(); + expect($pinnedArticles[3]->fresh()->isPinned())->toBeTrue(); - expect($article->fresh()->isPinned())->toBeFalse(); }); test('admins can unpin articles', function () { From 5b9f1e18c1b74e5ee10b6e3ae1f2ed7a6684bfac Mon Sep 17 00:00:00 2001 From: Ash Allen Date: Fri, 17 Mar 2023 16:07:48 +0000 Subject: [PATCH 5/7] Remove unneeded method. --- app/Models/Article.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/Models/Article.php b/app/Models/Article.php index 533493428..b49dc09d8 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -356,12 +356,4 @@ public function toFeedItem(): FeedItem ->link(route('articles.show', $this->slug())) ->authorName($this->author()->name()); } - - /** - * Check whether the article can be pinned. Only 4 articles can be pinned at a time. - */ - public function canBePinned(): bool - { - return $this->pinned()->count() < 4; - } } From 4062781100700eff123a008bd447ee1cbe9f0d78 Mon Sep 17 00:00:00 2001 From: ash-jc-allen Date: Fri, 17 Mar 2023 16:08:22 +0000 Subject: [PATCH 6/7] Fix code styling --- app/Http/Controllers/Admin/ArticlesController.php | 4 ++-- tests/Feature/AdminTest.php | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Admin/ArticlesController.php b/app/Http/Controllers/Admin/ArticlesController.php index 5d8fb7660..18b0bab25 100644 --- a/app/Http/Controllers/Admin/ArticlesController.php +++ b/app/Http/Controllers/Admin/ArticlesController.php @@ -69,14 +69,14 @@ public function togglePinnedStatus(Article $article) $this->authorize(ArticlePolicy::PINNED, $article); $article = DB::transaction(function () use ($article): Article { - if (!$article->isPinned()) { + if (! $article->isPinned()) { Article::pinned() ->oldest() ->take(1) ->update(['is_pinned' => false]); } - $article->is_pinned = !$article->isPinned(); + $article->is_pinned = ! $article->isPinned(); $article->save(); return $article; diff --git a/tests/Feature/AdminTest.php b/tests/Feature/AdminTest.php index db5a7e926..b60f392ab 100644 --- a/tests/Feature/AdminTest.php +++ b/tests/Feature/AdminTest.php @@ -327,7 +327,6 @@ expect($pinnedArticles[1]->fresh()->isPinned())->toBeTrue(); expect($pinnedArticles[2]->fresh()->isPinned())->toBeTrue(); expect($pinnedArticles[3]->fresh()->isPinned())->toBeTrue(); - }); test('admins can unpin articles', function () { From cc75881bf9c680ae9369971533fd4d9647509d96 Mon Sep 17 00:00:00 2001 From: Dries Vints Date: Sun, 15 Oct 2023 12:10:51 +0200 Subject: [PATCH 7/7] wip --- .../Controllers/Admin/ArticlesController.php | 16 ++-------- .../Articles/ArticlesController.php | 2 +- app/Models/Article.php | 4 +-- tests/Feature/AdminTest.php | 32 ------------------- 4 files changed, 5 insertions(+), 49 deletions(-) diff --git a/app/Http/Controllers/Admin/ArticlesController.php b/app/Http/Controllers/Admin/ArticlesController.php index aa1f2240e..fbf118ca6 100644 --- a/app/Http/Controllers/Admin/ArticlesController.php +++ b/app/Http/Controllers/Admin/ArticlesController.php @@ -12,7 +12,6 @@ use App\Queries\SearchArticles; use Illuminate\Auth\Middleware\Authenticate; use Illuminate\Http\RedirectResponse; -use Illuminate\Support\Facades\DB; use Illuminate\View\View; class ArticlesController extends Controller @@ -70,19 +69,8 @@ public function togglePinnedStatus(Article $article): RedirectResponse { $this->authorize(ArticlePolicy::PINNED, $article); - $article = DB::transaction(function () use ($article): Article { - if (! $article->isPinned()) { - Article::pinned() - ->oldest() - ->take(1) - ->update(['is_pinned' => false]); - } - - $article->is_pinned = ! $article->isPinned(); - $article->save(); - - return $article; - }); + $article->is_pinned = ! $article->isPinned(); + $article->save(); $this->success($article->isPinned() ? 'admin.articles.pinned' : 'admin.articles.unpinned'); diff --git a/app/Http/Controllers/Articles/ArticlesController.php b/app/Http/Controllers/Articles/ArticlesController.php index 6da01b3d0..fd9e30842 100644 --- a/app/Http/Controllers/Articles/ArticlesController.php +++ b/app/Http/Controllers/Articles/ArticlesController.php @@ -42,7 +42,7 @@ public function index(Request $request) ->get(); $articles = Article::published() - ->notPinned() + ->notPinned($pinnedArticles) ->{$filter}(); $tags = Tag::whereHas('articles', function ($query) { diff --git a/app/Models/Article.php b/app/Models/Article.php index 4f9aa86b7..f38b5696d 100644 --- a/app/Models/Article.php +++ b/app/Models/Article.php @@ -260,9 +260,9 @@ public function scopePinned(Builder $query): Builder return $query->where('is_pinned', true); } - public function scopeNotPinned(Builder $query): Builder + public function scopeNotPinned(Builder $query, Collection $pinnedArticles): Builder { - return $query->where('is_pinned', false); + return $query->whereNotIn('id', $pinnedArticles->pluck('id')); } public function scopeShared(Builder $query): Builder diff --git a/tests/Feature/AdminTest.php b/tests/Feature/AdminTest.php index b60f392ab..560beb269 100644 --- a/tests/Feature/AdminTest.php +++ b/tests/Feature/AdminTest.php @@ -297,38 +297,6 @@ expect($article->fresh()->isPinned())->toBeFalse(); }); -test('admin can pin articles if there are already 4 pinned articles', function () { - $pinnedArticles = Article::factory() - ->count(4) - ->sequence( - ['created_at' => now()->subDays(3)], - ['created_at' => now()->subDays(2)], - ['created_at' => now()->subDays(1)], - ['created_at' => now()], - ) - ->create([ - 'submitted_at' => now(), - 'approved_at' => now(), - 'is_pinned' => true, - ]); - - $article = Article::factory()->create(['submitted_at' => now(), 'approved_at' => now()]); - - $this->loginAsAdmin(); - - $this->put("/admin/articles/{$article->slug()}/pinned"); - - expect($article->fresh()->isPinned())->toBeTrue(); - - // Assert the oldest pinned article is no longer pinned - expect($pinnedArticles[0]->fresh()->isPinned())->toBeFalse(); - - // Assert the other pinned articles are still pinned - expect($pinnedArticles[1]->fresh()->isPinned())->toBeTrue(); - expect($pinnedArticles[2]->fresh()->isPinned())->toBeTrue(); - expect($pinnedArticles[3]->fresh()->isPinned())->toBeTrue(); -}); - test('admins can unpin articles', function () { $article = Article::factory()->create([ 'submitted_at' => now(),