From 87aaaf6971af99deb71271b634d0cc07a5d2bb9e Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Wed, 31 Aug 2022 11:13:51 +0200 Subject: [PATCH] feat(subscriptions): add option to send notifications when not caught up (#3503) --- extensions/subscriptions/extend.php | 3 + .../subscriptions/js/{forum.js => forum.ts} | 0 extensions/subscriptions/js/package.json | 9 +-- ...ettings.js => addSubscriptionSettings.tsx} | 15 ++++- extensions/subscriptions/js/tsconfig.json | 17 +++++ extensions/subscriptions/locale/en.yml | 4 +- .../src/Job/SendReplyNotification.php | 41 ++++++++++-- .../api/discussions/ReplyNotificationTest.php | 67 +++++++++++++------ yarn.lock | 6 +- 9 files changed, 125 insertions(+), 37 deletions(-) rename extensions/subscriptions/js/{forum.js => forum.ts} (100%) rename extensions/subscriptions/js/src/forum/{addSubscriptionSettings.js => addSubscriptionSettings.tsx} (63%) create mode 100644 extensions/subscriptions/js/tsconfig.json diff --git a/extensions/subscriptions/extend.php b/extensions/subscriptions/extend.php index 97259d8e9..29ae4bf56 100644 --- a/extensions/subscriptions/extend.php +++ b/extensions/subscriptions/extend.php @@ -61,4 +61,7 @@ return [ (new Extend\SimpleFlarumSearch(DiscussionSearcher::class)) ->addGambit(SubscriptionFilterGambit::class), + + (new Extend\User()) + ->registerPreference('flarum-subscriptions.notify_for_all_posts', 'boolval', false), ]; diff --git a/extensions/subscriptions/js/forum.js b/extensions/subscriptions/js/forum.ts similarity index 100% rename from extensions/subscriptions/js/forum.js rename to extensions/subscriptions/js/forum.ts diff --git a/extensions/subscriptions/js/package.json b/extensions/subscriptions/js/package.json index 43c59de63..3b1351cd8 100644 --- a/extensions/subscriptions/js/package.json +++ b/extensions/subscriptions/js/package.json @@ -4,11 +4,12 @@ "version": "0.0.0", "prettier": "@flarum/prettier-config", "devDependencies": { - "prettier": "^2.5.1", + "@flarum/prettier-config": "^1.0.0", "flarum-webpack-config": "^2.0.0", - "webpack": "^5.65.0", - "webpack-cli": "^4.9.1", - "@flarum/prettier-config": "^1.0.0" + "prettier": "^2.7.1", + "webpack": "^5.73.0", + "webpack-cli": "^4.10.0", + "flarum-tsconfig": "^1.0.2" }, "scripts": { "dev": "webpack --mode development --watch", diff --git a/extensions/subscriptions/js/src/forum/addSubscriptionSettings.js b/extensions/subscriptions/js/src/forum/addSubscriptionSettings.tsx similarity index 63% rename from extensions/subscriptions/js/src/forum/addSubscriptionSettings.js rename to extensions/subscriptions/js/src/forum/addSubscriptionSettings.tsx index 7d0b93119..77dc4dc2c 100644 --- a/extensions/subscriptions/js/src/forum/addSubscriptionSettings.js +++ b/extensions/subscriptions/js/src/forum/addSubscriptionSettings.tsx @@ -4,7 +4,7 @@ import SettingsPage from 'flarum/forum/components/SettingsPage'; import Switch from 'flarum/common/components/Switch'; export default function () { - extend(SettingsPage.prototype, 'notificationsItems', function (items) { + extend(SettingsPage.prototype, 'notificationsItems', function (this: SettingsPage, items) { items.add( 'followAfterReply', Switch.component( @@ -23,5 +23,18 @@ export default function () { app.translator.trans('flarum-subscriptions.forum.settings.follow_after_reply_label') ) ); + + items.add( + 'notifyForAllPosts', + { + this.user!.savePreferences({ 'flarum-subscriptions.notify_for_all_posts': val }); + }} + > + {app.translator.trans('flarum-subscriptions.forum.settings.notify_for_all_posts_label')} + + ); }); } diff --git a/extensions/subscriptions/js/tsconfig.json b/extensions/subscriptions/js/tsconfig.json new file mode 100644 index 000000000..c24c74031 --- /dev/null +++ b/extensions/subscriptions/js/tsconfig.json @@ -0,0 +1,17 @@ +{ + // Use Flarum's tsconfig as a starting point + "extends": "flarum-tsconfig", + // This will match all .ts, .tsx, .d.ts, .js, .jsx files in your `src` folder + // and also tells your Typescript server to read core's global typings for + // access to `dayjs` and `$` in the global namespace. + "include": ["src/**/*", "../vendor/*/*/js/dist-typings/@types/**/*", "@types/**/*"], + "compilerOptions": { + // This will output typings to `dist-typings` + "declarationDir": "./dist-typings", + "baseUrl": ".", + "paths": { + "flarum/*": ["../vendor/flarum/core/js/dist-typings/*"], + "@flarum/core/*": ["../vendor/flarum/core/js/dist-typings/*"] + } + } +} diff --git a/extensions/subscriptions/locale/en.yml b/extensions/subscriptions/locale/en.yml index f1159f74e..e724708fc 100644 --- a/extensions/subscriptions/locale/en.yml +++ b/extensions/subscriptions/locale/en.yml @@ -1,12 +1,10 @@ flarum-subscriptions: - ## # UNIQUE KEYS - The following keys are used in only one location each. ## # Translations in this namespace are used by the forum user interface. forum: - # These translations are displayed as tooltips for discussion badges. badge: following_tooltip: => flarum-subscriptions.ref.following @@ -33,6 +31,7 @@ flarum-subscriptions: # These translations are used in the Settings page. settings: follow_after_reply_label: Automatically follow discussions that I reply to + notify_for_all_posts_label: Notify about every new post instead of only the last in a discussion notify_new_post_label: Someone posts in a discussion I'm following # These translations are used in the subscription menu displayed to the right of the post stream. @@ -49,7 +48,6 @@ flarum-subscriptions: # Translations in this namespace are used in emails sent by the forum. email: - # These translations are used in emails sent when a post is made in a subscribed discussion new_post: subject: "[New Post] {title}" diff --git a/extensions/subscriptions/src/Job/SendReplyNotification.php b/extensions/subscriptions/src/Job/SendReplyNotification.php index 6f3d42170..6dc5c776b 100644 --- a/extensions/subscriptions/src/Job/SendReplyNotification.php +++ b/extensions/subscriptions/src/Job/SendReplyNotification.php @@ -11,10 +11,14 @@ namespace Flarum\Subscriptions\Job; use Flarum\Notification\NotificationSyncer; use Flarum\Post\Post; +use Flarum\Settings\SettingsRepositoryInterface; use Flarum\Subscriptions\Notification\NewPostBlueprint; +use Flarum\User\User; use Illuminate\Bus\Queueable; use Illuminate\Contracts\Queue\ShouldQueue; +use Illuminate\Database\Eloquent\Collection; use Illuminate\Queue\SerializesModels; +use Illuminate\Support\Arr; class SendReplyNotification implements ShouldQueue { @@ -41,20 +45,45 @@ class SendReplyNotification implements ShouldQueue $this->lastPostNumber = $lastPostNumber; } - public function handle(NotificationSyncer $notifications) + public function handle(NotificationSyncer $notifications, SettingsRepositoryInterface $settings) { $post = $this->post; $discussion = $post->discussion; - $notify = $discussion->readers() + $usersToNotify = []; + + $followers = $discussion->readers() + ->select('users.id', 'users.preferences', 'discussion_user.last_read_post_number') ->where('users.id', '!=', $post->user_id) - ->where('discussion_user.subscription', 'follow') - ->where('discussion_user.last_read_post_number', $this->lastPostNumber - 1) - ->get(); + ->where('discussion_user.subscription', 'follow'); + + $followers->chunk(150, function (Collection $followers) use (&$usersToNotify) { + $allUnreadUsers = []; + $firstUnreadUsers = []; + + /** + * @var \Flarum\User\User $user + */ + foreach ($followers as $user) { + $notifyForAll = $user->getPreference('flarum-subscriptions.notify_for_all_posts', false); + + if ($notifyForAll) { + $allUnreadUsers[] = $user; + } + // Only notify if this is the next post after the user's last read post + // i.e., their next new post to read + elseif ($user->last_read_post_number === $this->lastPostNumber - 1) { + $firstUnreadUsers[] = $user; + } + } + + $userIdsToNotify = Arr::pluck(array_merge($allUnreadUsers, $firstUnreadUsers), 'id'); + $usersToNotify = array_merge($usersToNotify, User::query()->whereIn('id', $userIdsToNotify)->get()->all()); + }); $notifications->sync( new NewPostBlueprint($post), - $notify->all() + $usersToNotify ); } } diff --git a/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php b/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php index 6c6ef785f..943c5fed6 100644 --- a/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php +++ b/extensions/subscriptions/tests/integration/api/discussions/ReplyNotificationTest.php @@ -27,49 +27,79 @@ class ReplyNotificationTest extends TestCase $this->prepareDatabase([ 'users' => [ $this->normalUser(), + ['id' => 3, 'username' => 'acme', 'email' => 'acme@machine.local', 'is_email_confirmed' => 1, 'preferences' => json_encode(['flarum-subscriptions.notify_for_all_posts' => true])], + ['id' => 4, 'username' => 'acme2', 'email' => 'acme2@machine.local', 'is_email_confirmed' => 1], ], 'discussions' => [ ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1, 'last_post_number' => 1, 'last_post_id' => 1], ['id' => 2, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 2, 'comment_count' => 1, 'last_post_number' => 1, 'last_post_id' => 2], + + ['id' => 33, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 33, 'comment_count' => 6, 'last_post_number' => 6, 'last_post_id' => 38], ], 'posts' => [ ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 1], ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 1], + + ['id' => 33, 'discussion_id' => 33, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 1], + ['id' => 34, 'discussion_id' => 33, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 2], + ['id' => 35, 'discussion_id' => 33, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 3], + ['id' => 36, 'discussion_id' => 33, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 4], + ['id' => 37, 'discussion_id' => 33, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 5], + ['id' => 38, 'discussion_id' => 33, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

foo bar

', 'number' => 6], ], 'discussion_user' => [ ['discussion_id' => 1, 'user_id' => 1, 'last_read_post_number' => 1, 'subscription' => 'follow'], + ['discussion_id' => 1, 'user_id' => 2, 'last_read_post_number' => 1, 'subscription' => 'follow'], ['discussion_id' => 2, 'user_id' => 1, 'last_read_post_number' => 1, 'subscription' => 'follow'], + + ['discussion_id' => 33, 'user_id' => 2, 'last_read_post_number' => 1, 'subscription' => 'follow'], + ['discussion_id' => 33, 'user_id' => 3, 'last_read_post_number' => 1, 'subscription' => 'follow'], ] ]); } - /** @test */ - public function replying_to_a_discussion_with_comment_post_as_last_post_sends_reply_notification() + /** + * @dataProvider replyingSendsNotificationsDataProvider + * @test + */ + public function replying_to_a_discussion_with_comment_post_as_last_post_sends_reply_notification(int $userId, int $discussionId, int $newNotificationCount) { $this->app(); /** @var User $mainUser */ - $mainUser = User::query()->find(1); + $mainUser = User::query()->find($userId); $this->assertEquals(0, $mainUser->getUnreadNotificationCount()); - $this->send( - $this->request('POST', '/api/posts', [ - 'authenticatedAs' => 2, - 'json' => [ - 'data' => [ - 'attributes' => [ - 'content' => 'reply with predetermined content for automated testing - too-obscure', - ], - 'relationships' => [ - 'discussion' => ['data' => ['id' => 1]], + for ($i = 0; $i < 5; $i++) { + $this->send( + $this->request('POST', '/api/posts', [ + 'authenticatedAs' => 4, + 'json' => [ + 'data' => [ + 'attributes' => [ + 'content' => 'reply with predetermined content for automated testing - too-obscure', + ], + 'relationships' => [ + 'discussion' => ['data' => ['id' => $discussionId]], + ], ], ], - ], - ]) - ); + ])->withAttribute('bypassThrottling', true) + ); + } - $this->assertEquals(1, $mainUser->getUnreadNotificationCount()); + $this->assertEquals($newNotificationCount, $mainUser->getUnreadNotificationCount()); + } + + public function replyingSendsNotificationsDataProvider(): array + { + return [ + 'admin receives a notification when another replies to a discussion they are following and caught up to' => [1, 1, 1], + 'user receives a notification when another replies to a discussion they are following and caught up to' => [2, 1, 1], + + 'user receives notification for every new post to a discussion they are following when preference is on' => [3, 33, 5], + ]; } /** @test */ @@ -136,9 +166,6 @@ class ReplyNotificationTest extends TestCase public function deleting_last_posts_then_posting_new_one_sends_reply_notification(array $postIds) { $this->prepareDatabase([ - 'users' => [ - ['id' => 3, 'username' => 'acme', 'email' => 'acme@machine.local', 'is_email_confirmed' => 1], - ], 'discussions' => [ ['id' => 3, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 2, 'first_post_id' => 1, 'comment_count' => 5, 'last_post_number' => 5, 'last_post_id' => 10], ], diff --git a/yarn.lock b/yarn.lock index 8c19fbdb8..3e3f61caa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1799,9 +1799,9 @@ enhanced-resolve@^5.9.2: tapable "^2.2.0" enhanced-resolve@^5.9.3: - version "5.10.0" - resolved "https://registry.yarnpkg.com/enhanced-resolve/-/enhanced-resolve-5.10.0.tgz#0dc579c3bb2a1032e357ac45b8f3a6f3ad4fb1e6" - integrity sha512-T0yTFjdpldGY8PmuXXR0PyQ1ufZpEGiHVrp7zHKB7jdR4qlmZHhONVM5AQOAWXuF/w3dnHbEQVrNptJgt7F+cQ== + version "5.9.3" + resolved "https://registry.yarnpkg.com/enhanced-resolve/-/enhanced-resolve-5.9.3.tgz#44a342c012cbc473254af5cc6ae20ebd0aae5d88" + integrity sha512-Bq9VSor+kjvW3f9/MiiR4eE3XYgOl7/rS8lnSxbRbF3kS0B2r+Y9w5krBWxZgDxASVZbdYrn5wT4j/Wb0J9qow== dependencies: graceful-fs "^4.2.4" tapable "^2.2.0"