fix: best answer counts not updated on post/discussion delete (#87)

* chore: create tests to highlight the defect (failing) * fix: listen for deletion, reduce ba count * Apply fixes from StyleCI * chore: enable backend testing * feat: provide cli command to re-sync all user ba counts * Apply fixes from StyleCI --------- Co-authored-by: StyleCI Bot <[email protected]>
FriendsOfFlarum · Oct 29, 2023 · 022f775 · 022f775
1 parent 192b198
commit 022f775
Show file tree

Hide file tree

Showing 17 changed files with 619 additions and 40 deletions.
diff --git a/.github/workflows/backend.yml b/.github/workflows/backend.yml
@@ -6,7 +6,7 @@ jobs:
   run:
     uses: flarum/framework/.github/workflows/REUSABLE_backend.yml@main
     with:
-      enable_backend_testing: false
+      enable_backend_testing: true
       enable_phpstan: true
 
       backend_directory: .
diff --git a/.gitignore b/.gitignore
@@ -2,3 +2,4 @@ js/dist
 js/node_modules
 vendor
 composer.lock
+.phpunit.result.cache
diff --git a/composer.json b/composer.json
@@ -62,18 +62,36 @@
         },
         "flarum-cli": {
             "modules": {
-                "githubActions": true
+                "githubActions": true,
+                "backendTesting": true
             }
         }
     },
     "scripts": {
         "analyse:phpstan": "phpstan analyse",
-        "clear-cache:phpstan": "phpstan clear-result-cache"
+        "clear-cache:phpstan": "phpstan clear-result-cache",
+        "test": [
+            "@test:unit",
+            "@test:integration"
+        ],
+        "test:unit": "phpunit -c tests/phpunit.unit.xml",
+        "test:integration": "phpunit -c tests/phpunit.integration.xml",
+        "test:setup": "@php tests/integration/setup.php"
     },
     "scripts-descriptions": {
-        "analyse:phpstan": "Run static analysis"
+        "analyse:phpstan": "Run static analysis",
+        "test": "Runs all tests.",
+        "test:unit": "Runs all unit tests.",
+        "test:integration": "Runs all integration tests.",
+        "test:setup": "Sets up a database for use with integration tests. Execute this only once."
     },
     "require-dev": {
-        "flarum/phpstan": "^1.8"
+        "flarum/phpstan": "^1.8",
+        "flarum/testing": "^1.0.0"
+    },
+    "autoload-dev": {
+        "psr-4": {
+            "FoF\\BestAnswer\\Tests\\": "tests/"
+        }
     }
 }
diff --git a/extend.php b/extend.php
@@ -113,6 +113,7 @@
 
     (new Extend\Console())
         ->command(Console\NotifyCommand::class)
+        ->command(Console\UpdateBestAnswerCounts::class)
         ->schedule(Console\NotifyCommand::class, Console\NotifySchedule::class),
 
     (new Extend\Filter(DiscussionFilterer::class))

diff --git a/src/BestAnswerRepository.php b/src/BestAnswerRepository.php
@@ -68,4 +68,31 @@ public function tagEnabledForBestAnswer(Discussion $discussion): bool
 
         return $enabled;
     }
+
+    /**
+     * Calculate the number of best answers for a user.
+     * This is used when `best_answer_count` is `null` on the user, usually because either the user
+     * has not already been awarded any best answers, or the extension was updated to include this feature
+     * and the user has not yet been serialized.
+     *
+     * @param User $user
+     *
+     * @return int
+     */
+    public function calculateBestAnswersForUser(User $user): int
+    {
+        $count = Discussion::whereNotNull('best_answer_post_id')
+            ->leftJoin('posts', 'posts.id', '=', 'discussions.best_answer_post_id')
+            ->where('posts.user_id', $user->id)
+            ->count();
+
+        // Use a standalone query and not attribute update+save because otherwise data added by extensions
+        // with Extend\ApiController::prepareDataForSerialization() ends up being added to the SQL UPDATE clause,
+        // and breaks Flarum since those are often not real columns
+        $user->newQuery()
+            ->where('id', $user->id)
+            ->update(['best_answer_count' => $count]);
+
+        return $count;
+    }
 }
diff --git a/src/Console/NotifySchedule.php b/src/Console/NotifySchedule.php
@@ -17,23 +17,31 @@
 
 class NotifySchedule
 {
-    public function __invoke(Event $event)
+    /**
+     * @var SettingsRepositoryInterface
+     */
+    public $settings;
+
+    public function __construct(SettingsRepositoryInterface $settings)
     {
-        $settings = resolve(SettingsRepositoryInterface::class);
+        $this->settings = $settings;
+    }
 
+    public function __invoke(Event $event)
+    {
         $event->hourly()
             ->withoutOverlapping();
 
-        if ((bool) $settings->get('fof-best-answer.schedule_on_one_server')) {
+        if ((bool) $this->settings->get('fof-best-answer.schedule_on_one_server')) {
             $event->onOneServer();
         }
 
-        if ((bool) $settings->get('fof-best-answer.stop_overnight')) {
+        if ((bool) $this->settings->get('fof-best-answer.stop_overnight')) {
             $event->between('8:00', '21:00');
             //TODO expose times back to config options
         }
 
-        if ((bool) $settings->get('fof-best-answer.store_log_output')) {
+        if ((bool) $this->settings->get('fof-best-answer.store_log_output')) {
             $paths = resolve(Paths::class);
             $event->appendOutputTo($paths->storage.(DIRECTORY_SEPARATOR.'logs'.DIRECTORY_SEPARATOR.'fof-best-answer.log'));
         }

diff --git a/src/Console/UpdateBestAnswerCounts.php b/src/Console/UpdateBestAnswerCounts.php
@@ -0,0 +1,62 @@
+<?php
+
+/*
+ * This file is part of fof/best-answer.
+ *
+ * Copyright (c) FriendsOfFlarum.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FoF\BestAnswer\Console;
+
+use Flarum\User\User;
+use FoF\BestAnswer\BestAnswerRepository;
+use Illuminate\Console\Command;
+
+class UpdateBestAnswerCounts extends Command
+{
+    /**
+     * @var BestAnswerRepository
+     */
+    public $bestAnswers;
+
+    public function __construct(BestAnswerRepository $bestAnswers)
+    {
+        parent::__construct();
+        $this->bestAnswers = $bestAnswers;
+    }
+
+    /**
+     * @var string
+     */
+    protected $signature = 'fof:best-answer:update-counts';
+
+    /**
+     * @var string
+     */
+    protected $description = 'Update best answer counts for all users';
+
+    public function handle()
+    {
+        $this->info('Updating best answer counts...');
+
+        $userCount = User::count();
+
+        $bar = $this->output->createProgressBar($userCount);
+
+        User::query()->chunkById(100, function ($users) use ($bar) {
+            foreach ($users as $user) {
+                $user->best_answer_count = $this->bestAnswers->calculateBestAnswersForUser($user);
+                $user->save();
+
+                $bar->advance();
+            }
+        });
+
+        $bar->finish();
+
+        $this->info('Done!');
+    }
+}
diff --git a/src/Listeners/RecalculateBestAnswerCounts.php b/src/Listeners/RecalculateBestAnswerCounts.php
@@ -11,6 +11,9 @@
 
 namespace FoF\BestAnswer\Listeners;
 
+use Flarum\Discussion\Event\Deleting as DiscussionDeleting;
+use Flarum\Post\Event\Deleting as PostDeleting;
+use Flarum\Post\Post;
 use FoF\BestAnswer\Events\BestAnswerSet;
 use FoF\BestAnswer\Events\BestAnswerUnset;
 use Illuminate\Contracts\Events\Dispatcher;
@@ -21,6 +24,8 @@ public function subscribe(Dispatcher $events)
     {
         $events->listen(BestAnswerSet::class, [$this, 'addBestAnswerCount']);
         $events->listen(BestAnswerUnset::class, [$this, 'subtractBestAnswerCount']);
+        $events->listen(PostDeleting::class, [$this, 'handlePostDeletion']);
+        $events->listen(DiscussionDeleting::class, [$this, 'handleDiscussionDeletion']);
     }
 
     public function addBestAnswerCount(BestAnswerSet $event): void
@@ -42,4 +47,38 @@ public function subtractBestAnswerCount(BestAnswerUnset $event): void
             $author->save();
         }
     }
+
+    public function handlePostDeletion(PostDeleting $event): void
+    {
+        $post = $event->post;
+
+        if ($post->discussion->best_answer_post_id === $post->id) {
+            $post->discussion->best_answer_post_id = null;
+            $post->discussion->best_answer_user_id = null;
+            $post->discussion->best_answer_set_at = null;
+            $post->discussion->save();
+
+            $author = $post->user;
+
+            if ($author) {
+                $author->best_answer_count--;
+                $author->save();
+            }
+        }
+    }
+
+    public function handleDiscussionDeletion(DiscussionDeleting $event): void
+    {
+        $discussion = $event->discussion;
+
+        if ($discussion->best_answer_post_id) {
+            $post = Post::find($discussion->best_answer_post_id);
+            $author = $post->user;
+
+            if ($author) {
+                $author->best_answer_count--;
+                $author->save();
+            }
+        }
+    }
 }
diff --git a/src/Listeners/SaveBestAnswerToDatabase.php b/src/Listeners/SaveBestAnswerToDatabase.php
@@ -91,7 +91,7 @@ public function handle(Saving $event)
         $this->notifications->delete(new SelectBestAnswerBlueprint($discussion));
     }
 
-    private function removeBestAnswer(Discussion $discussion, User $actor): void
+    protected function removeBestAnswer(Discussion $discussion, User $actor): void
     {
         /** @var Post|null $post */
         $post = $discussion->bestAnswerPost;
@@ -112,7 +112,7 @@ private function removeBestAnswer(Discussion $discussion, User $actor): void
         });
     }
 
-    private function setBestAnswer(Discussion $discussion, User $actor, int $id): void
+    protected function setBestAnswer(Discussion $discussion, User $actor, int $id): void
     {
         /** @var Post|null $post */
         $post = $discussion->posts()->find($id);

diff --git a/src/UserBestAnswerCount.php b/src/UserBestAnswerCount.php
@@ -12,42 +12,24 @@
 namespace FoF\BestAnswer;
 
 use Flarum\Api\Serializer\UserSerializer;
-use Flarum\Discussion\Discussion;
 use Flarum\User\User;
 
 class UserBestAnswerCount
 {
-    public function __invoke(UserSerializer $serializer, User $user, array $attributes): array
-    {
-        $attributes['bestAnswerCount'] = $user->best_answer_count ?? $this->calculateBestAnswersForUser($user);
-
-        return $attributes;
-    }
-
     /**
-     * Calculate the number of best answers for a user.
-     * This is used when `best_answer_count` is `null` on the user, usually because either the user
-     * has not already been awarded any best answers, or the extension was updated to include this feature
-     * and the user has not yet been serialized.
-     *
-     * @param User $user
-     *
-     * @return int
+     * @var BestAnswerRepository
      */
-    private function calculateBestAnswersForUser(User $user): int
+    public $bestAnswers;
+
+    public function __construct(BestAnswerRepository $bestAnswers)
     {
-        $count = Discussion::whereNotNull('best_answer_post_id')
-            ->leftJoin('posts', 'posts.id', '=', 'discussions.best_answer_post_id')
-            ->where('posts.user_id', $user->id)
-            ->count();
+        $this->bestAnswers = $bestAnswers;
+    }
 
-        // Use a standalone query and not attribute update+save because otherwise data added by extensions
-        // with Extend\ApiController::prepareDataForSerialization() ends up being added to the SQL UPDATE clause,
-        // and breaks Flarum since those are often not real columns
-        $user->newQuery()
-            ->where('id', $user->id)
-            ->update(['best_answer_count' => $count]);
+    public function __invoke(UserSerializer $serializer, User $user, array $attributes): array
+    {
+        $attributes['bestAnswerCount'] = $user->best_answer_count ?? $this->bestAnswers->calculateBestAnswersForUser($user);
 
-        return $count;
+        return $attributes;
     }
 }
diff --git a/tests/fixtures/.gitkeep b/tests/fixtures/.gitkeep