From 98b3d0f89e7ec922124080924b12f8feb0da1a39 Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Wed, 20 May 2015 12:24:01 +0930 Subject: [PATCH] Simplify and improve notifications API. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out that the idea of “sending” a notification is flawed. (What happens if the notification subject is deleted shortly after? The notified user would end up with a dud notification which would be confusing. What about if a post is edited to mention an extra user? If you sent out notifications, the users who’ve already been mentioned would get a duplicate notification.) Instead, I’ve introduced the idea of notification “syncing”. Whenever a change is made to a piece of data (e.g. a post is created, edited, or deleted), you make a common notification and “sync” it to a set of users. The users who’ve received this notification before won’t get it again. It will be sent out to new users, and hidden from users who’ve received it before but are no longer recipients (e.g. users who’ve been “unmentioned” in a post). To keep track of this, we use the existing notifications database table, with an added `is_deleted` column. The syncing/diffing is handled all behind the scenes; the API is extremely simple (see Core\Notifications\DiscussionRenamedNotification + Core\Events\Handlers\DiscussionRenamedNotifier) --- .../discussion-renamed-notification.js | 2 +- ...2_24_000000_create_notifications_table.php | 3 +- src/Api/Actions/Notifications/IndexAction.php | 2 +- src/Api/Serializers/BaseSerializer.php | 5 +- .../Serializers/NotificationSerializer.php | 18 +++- src/Core/CoreServiceProvider.php | 14 ++- .../Commands/PostReplyCommandHandler.php | 10 +- .../Events/DiscussionRenamedNotifier.php | 37 +++----- src/Core/Models/User.php | 21 ++++- .../DiscussionRenamedNotification.php | 38 ++++++++ .../Notifications/NotificationAbstract.php | 54 +++++++++++ .../Notifications/NotificationInterface.php | 60 ++++++++++++ ...tionEmailer.php => NotificationMailer.php} | 13 +-- .../NotificationServiceProvider.php | 38 -------- src/Core/Notifications/NotificationSyncer.php | 92 +++++++++++++++++++ src/Core/Notifications/Notifier.php | 86 ----------------- .../Senders/NotificationAlerter.php | 34 ------- .../Senders/NotificationSender.php | 11 --- .../Senders/RetractableSender.php | 8 -- .../Types/DiscussionRenamedNotification.php | 46 ---------- .../Types/EmailableNotification.php | 8 -- src/Core/Notifications/Types/Notification.php | 16 ---- .../EloquentNotificationRepository.php | 8 +- .../NotificationRepositoryInterface.php | 4 +- src/Extend/NotificationType.php | 19 ++-- 25 files changed, 334 insertions(+), 313 deletions(-) create mode 100644 src/Core/Notifications/DiscussionRenamedNotification.php create mode 100644 src/Core/Notifications/NotificationAbstract.php create mode 100644 src/Core/Notifications/NotificationInterface.php rename src/Core/Notifications/{Senders/NotificationEmailer.php => NotificationMailer.php} (58%) delete mode 100644 src/Core/Notifications/NotificationServiceProvider.php create mode 100644 src/Core/Notifications/NotificationSyncer.php delete mode 100644 src/Core/Notifications/Notifier.php delete mode 100644 src/Core/Notifications/Senders/NotificationAlerter.php delete mode 100644 src/Core/Notifications/Senders/NotificationSender.php delete mode 100644 src/Core/Notifications/Senders/RetractableSender.php delete mode 100644 src/Core/Notifications/Types/DiscussionRenamedNotification.php delete mode 100644 src/Core/Notifications/Types/EmailableNotification.php delete mode 100644 src/Core/Notifications/Types/Notification.php diff --git a/js/forum/src/components/discussion-renamed-notification.js b/js/forum/src/components/discussion-renamed-notification.js index 77290a1da..067288a04 100644 --- a/js/forum/src/components/discussion-renamed-notification.js +++ b/js/forum/src/components/discussion-renamed-notification.js @@ -8,7 +8,7 @@ export default class DiscussionRenamedNotification extends Notification { return super.view({ href: app.route.discussion(notification.subject(), notification.content().postNumber), icon: 'pencil', - content: [username(notification.sender()), ' renamed'] + content: [username(notification.sender()), ' changed the title'] }); } } diff --git a/migrations/2015_02_24_000000_create_notifications_table.php b/migrations/2015_02_24_000000_create_notifications_table.php index aea969118..830797196 100644 --- a/migrations/2015_02_24_000000_create_notifications_table.php +++ b/migrations/2015_02_24_000000_create_notifications_table.php @@ -14,7 +14,7 @@ class CreateNotificationsTable extends Migration public function up() { Schema::create('notifications', function (Blueprint $table) { - + $table->increments('id'); $table->integer('user_id')->unsigned(); $table->integer('sender_id')->unsigned()->nullable(); @@ -24,6 +24,7 @@ class CreateNotificationsTable extends Migration $table->binary('data')->nullable(); $table->dateTime('time'); $table->boolean('is_read')->default(0); + $table->boolean('is_deleted')->default(0); }); } diff --git a/src/Api/Actions/Notifications/IndexAction.php b/src/Api/Actions/Notifications/IndexAction.php index 8bc24129a..4a23c05aa 100644 --- a/src/Api/Actions/Notifications/IndexAction.php +++ b/src/Api/Actions/Notifications/IndexAction.php @@ -73,6 +73,6 @@ class IndexAction extends SerializeCollectionAction $user->markNotificationsAsRead()->save(); - return $this->notifications->findByUser($user->id, $request->limit, $request->offset); + return $this->notifications->findByUser($user, $request->limit, $request->offset); } } diff --git a/src/Api/Serializers/BaseSerializer.php b/src/Api/Serializers/BaseSerializer.php index 846992a0d..6fe6a1487 100644 --- a/src/Api/Serializers/BaseSerializer.php +++ b/src/Api/Serializers/BaseSerializer.php @@ -64,9 +64,8 @@ abstract class BaseSerializer extends SerializerAbstract } } - if (is_array($serializer)) { - $class = get_class(is_object($data) ? $data : $model->$relation()->getRelated()); - $serializer = $serializer[$class]; + if ($serializer instanceof Closure) { + $serializer = $serializer($model, $data); } $serializer = new $serializer($this->actor, $links); return $many ? $serializer->collection($data) : $serializer->resource($data); diff --git a/src/Api/Serializers/NotificationSerializer.php b/src/Api/Serializers/NotificationSerializer.php index f3b57fac4..fc21ccaf9 100644 --- a/src/Api/Serializers/NotificationSerializer.php +++ b/src/Api/Serializers/NotificationSerializer.php @@ -4,10 +4,21 @@ class NotificationSerializer extends BaseSerializer { /** * The resource type. + * * @var string */ protected $type = 'notifications'; + /** + * A map of notification types (key) to the serializer that should be used + * to output the notification's subject (value). + * + * @var array + */ + public static $subjects = [ + 'discussionRenamed' => 'Flarum\Api\Serializers\DiscussionBasicSerializer' + ]; + /** * Serialize attributes of an notification model for JSON output. * @@ -40,9 +51,8 @@ class NotificationSerializer extends BaseSerializer public function subject() { - return $this->hasOne([ - 'Flarum\Core\Models\Discussion' => 'Flarum\Api\Serializers\DiscussionSerializer', - 'Flarum\Core\Models\CommentPost' => 'Flarum\Api\Serializers\PostSerializer' - ]); + return $this->hasOne(function ($notification) { + return static::$subjects[$notification->type]; + }); } } diff --git a/src/Core/CoreServiceProvider.php b/src/Core/CoreServiceProvider.php index e91d3ec89..32d792960 100644 --- a/src/Core/CoreServiceProvider.php +++ b/src/Core/CoreServiceProvider.php @@ -16,6 +16,7 @@ use League\Flysystem\Adapter\Local; use Flarum\Core\Events\RegisterDiscussionGambits; use Flarum\Core\Events\RegisterUserGambits; use Flarum\Extend\Permission; +use Flarum\Extend\NotificationType; class CoreServiceProvider extends ServiceProvider { @@ -43,6 +44,12 @@ class CoreServiceProvider extends ServiceProvider 'Flarum\Core\Handlers\Commands' ); }); + + $events->subscribe('Flarum\Core\Handlers\Events\DiscussionRenamedNotifier'); + $this->extend( + (new NotificationType('Flarum\Core\Notifications\DiscussionRenamedNotification', 'Flarum\Api\Serializers\DiscussionBasicSerializer')) + ->enableByDefault('alert'), + ); } /** @@ -52,8 +59,6 @@ class CoreServiceProvider extends ServiceProvider */ public function register() { - $this->app->register('Flarum\Core\Notifications\NotificationServiceProvider'); - // Register a singleton entity that represents this forum. This entity // will be used to check for global forum permissions (like viewing the // forum, registering, and starting discussions.) @@ -89,6 +94,11 @@ class CoreServiceProvider extends ServiceProvider $this->app->when('Flarum\Core\Handlers\Commands\DeleteAvatarCommandHandler') ->needs('League\Flysystem\FilesystemInterface') ->give($avatarFilesystem); + + $this->app->bind( + 'Flarum\Core\Repositories\NotificationRepositoryInterface', + 'Flarum\Core\Repositories\EloquentNotificationRepository' + ); } public function registerGambits() diff --git a/src/Core/Handlers/Commands/PostReplyCommandHandler.php b/src/Core/Handlers/Commands/PostReplyCommandHandler.php index 68b39b80d..f930c2625 100644 --- a/src/Core/Handlers/Commands/PostReplyCommandHandler.php +++ b/src/Core/Handlers/Commands/PostReplyCommandHandler.php @@ -4,7 +4,7 @@ use Flarum\Core\Events\PostWillBeSaved; use Flarum\Core\Repositories\DiscussionRepositoryInterface as DiscussionRepository; use Flarum\Core\Models\CommentPost; use Flarum\Core\Support\DispatchesEvents; -use Flarum\Core\Notifications\Notifier; +use Flarum\Core\Notifications\NotificationSyncer; class PostReplyCommandHandler { @@ -12,12 +12,12 @@ class PostReplyCommandHandler protected $discussions; - protected $notifier; + protected $notifications; - public function __construct(DiscussionRepository $discussions, Notifier $notifier) + public function __construct(DiscussionRepository $discussions, NotificationSyncer $notifications) { $this->discussions = $discussions; - $this->notifier = $notifier; + $this->notifications = $notifications; } public function handle($command) @@ -46,7 +46,7 @@ class PostReplyCommandHandler $post->save(); - $this->notifier->onePerUser(function () use ($post) { + $this->notifications->onePerUser(function () use ($post) { $this->dispatchEventsFor($post); }); diff --git a/src/Core/Handlers/Events/DiscussionRenamedNotifier.php b/src/Core/Handlers/Events/DiscussionRenamedNotifier.php index 79342cc1b..1833f471a 100755 --- a/src/Core/Handlers/Events/DiscussionRenamedNotifier.php +++ b/src/Core/Handlers/Events/DiscussionRenamedNotifier.php @@ -2,15 +2,17 @@ use Flarum\Core\Events\DiscussionWasRenamed; use Flarum\Core\Models\DiscussionRenamedPost; -use Flarum\Core\Notifications\Types\DiscussionRenamedNotification; -use Flarum\Core\Notifications\Notifier; +use Flarum\Core\Notifications\DiscussionRenamedNotification; +use Flarum\Core\Notifications\NotificationSyncer; use Illuminate\Contracts\Events\Dispatcher; class DiscussionRenamedNotifier { - public function __construct(Notifier $notifier) + protected $notifications; + + public function __construct(NotificationSyncer $notifications) { - $this->notifier = $notifier; + $this->notifications = $notifications; } /** @@ -25,28 +27,19 @@ class DiscussionRenamedNotifier public function whenDiscussionWasRenamed(DiscussionWasRenamed $event) { - $post = $this->createPost($event); - - $post = $event->discussion->addPost($post); - - if ($event->discussion->start_user_id !== $event->user->id) { - $notification = new DiscussionRenamedNotification($event->discussion, $post->user, $post); - - if ($post->exists) { - $this->notifier->send($notification, [$post->discussion->startUser]); - } else { - $this->notifier->retract($notification); - } - } - } - - protected function createPost(DiscussionWasRenamed $event) - { - return DiscussionRenamedPost::reply( + $post = DiscussionRenamedPost::reply( $event->discussion->id, $event->user->id, $event->oldTitle, $event->discussion->title ); + + $post = $event->discussion->addPost($post); + + if ($event->discussion->start_user_id !== $event->user->id) { + $notification = new DiscussionRenamedNotification($post); + + $this->notifications->sync($notification, $post->exists ? [$event->discussion->startUser] : []); + } } } diff --git a/src/Core/Models/User.php b/src/Core/Models/User.php index 03dfe9e3c..0bedb6ef9 100755 --- a/src/Core/Models/User.php +++ b/src/Core/Models/User.php @@ -327,7 +327,13 @@ class User extends Model public function getUnreadNotificationsCount() { - return $this->notifications()->where('time', '>', $this->notification_read_time ?: 0)->where('is_read', 0)->count(\DB::raw('DISTINCT type, subject_id')); + $types = array_keys(Notification::getTypes()); + + return $this->notifications() + ->whereIn('type', array_filter($types, [$this, 'shouldAlert'])) + ->where('time', '>', $this->notification_read_time ?: 0) + ->where('is_read', 0) + ->count(\DB::raw('DISTINCT type, subject_id')); } public function getPreferencesAttribute($value) @@ -354,14 +360,19 @@ class User extends Model ]; } - public static function notificationPreferenceKey($type, $sender) + public static function notificationPreferenceKey($type, $method) { - return 'notify_'.$type.'_'.$sender; + return 'notify_'.$type.'_'.$method; } - public function shouldNotify($type, $method) + public function shouldAlert($type) { - return $this->preference(static::notificationPreferenceKey($type, $method)); + return $this->preference(static::notificationPreferenceKey($type, 'alert')); + } + + public function shouldEmail($type) + { + return $this->preference(static::notificationPreferenceKey($type, 'email')); } public function preference($key, $default = null) diff --git a/src/Core/Notifications/DiscussionRenamedNotification.php b/src/Core/Notifications/DiscussionRenamedNotification.php new file mode 100644 index 000000000..e047826b7 --- /dev/null +++ b/src/Core/Notifications/DiscussionRenamedNotification.php @@ -0,0 +1,38 @@ +post = $post; + } + + public function getSubject() + { + return $this->post->discussion; + } + + public function getSender() + { + return $this->post->user; + } + + public function getData() + { + return ['postNumber' => (int) $this->post->number]; + } + + public static function getType() + { + return 'discussionRenamed'; + } + + public static function getSubjectModel() + { + return 'Flarum\Core\Models\Discussion'; + } +} diff --git a/src/Core/Notifications/NotificationAbstract.php b/src/Core/Notifications/NotificationAbstract.php new file mode 100644 index 000000000..72a67f3c3 --- /dev/null +++ b/src/Core/Notifications/NotificationAbstract.php @@ -0,0 +1,54 @@ +forum = $forum; } - public function send(Notification $notification, User $user) + public function send(NotificationInterface $notification, User $user) { $this->mailer->send( $notification->getEmailView(), @@ -25,9 +23,4 @@ class NotificationEmailer implements NotificationSender } ); } - - public static function compatibleWith($class) - { - return (new ReflectionClass($class))->implementsInterface('Flarum\Core\Notifications\Types\EmailableNotification'); - } } diff --git a/src/Core/Notifications/NotificationServiceProvider.php b/src/Core/Notifications/NotificationServiceProvider.php deleted file mode 100644 index 918ab2dbc..000000000 --- a/src/Core/Notifications/NotificationServiceProvider.php +++ /dev/null @@ -1,38 +0,0 @@ -subscribe('Flarum\Core\Handlers\Events\DiscussionRenamedNotifier'); - - $notifier->registerMethod('alert', 'Flarum\Core\Notifications\Senders\NotificationAlerter'); - $notifier->registerMethod('email', 'Flarum\Core\Notifications\Senders\NotificationEmailer'); - - $this->extend( - (new NotificationType('Flarum\Core\Notifications\Types\DiscussionRenamedNotification'))->enableByDefault('alert') - ); - } - - public function register() - { - $this->app->bind( - 'Flarum\Core\Repositories\NotificationRepositoryInterface', - 'Flarum\Core\Repositories\EloquentNotificationRepository' - ); - - $this->app->singleton('Flarum\Core\Notifications\Notifier'); - $this->app->alias('Flarum\Core\Notifications\Notifier', 'flarum.notifier'); - } -} diff --git a/src/Core/Notifications/NotificationSyncer.php b/src/Core/Notifications/NotificationSyncer.php new file mode 100644 index 000000000..7ca70e389 --- /dev/null +++ b/src/Core/Notifications/NotificationSyncer.php @@ -0,0 +1,92 @@ +notifications = $notifications; + $this->mailer = $mailer; + } + + /** + * Sync a notification so that it is visible to the specified users, and not + * visible to anyone else. If it is being made visible for the first time, + * attempt to send the user an email. + * + * @param \Flarum\Core\Notifications\NotificationInterface $notification + * @param \Flarum\Core\Models\User[] $users + * @return void + */ + public function sync(NotificationInterface $notification, array $users) + { + $attributes = [ + 'type' => $notification::getType(), + 'sender_id' => $notification->getSender()->id, + 'subject_id' => $notification->getSubject()->id, + 'data' => ($data = $notification->getData()) ? json_encode($data) : null + ]; + + $toDelete = Notification::where($attributes)->get(); + $toUndelete = []; + $newRecipients = []; + + foreach ($users as $user) { + $existing = $toDelete->where('user_id', $user->id)->first(); + + if (($k = $toDelete->search($existing)) !== false) { + $toUndelete[] = $existing->id; + $toDelete->pull($k); + } elseif (! $this->onePerUser || ! in_array($user->id, $this->sentTo)) { + $newRecipients[] = $user; + $this->sentTo[] = $user->id; + } + } + + if (count($toDelete)) { + Notification::whereIn('id', $toDelete->lists('id'))->update(['is_deleted' => true]); + } + + if (count($toUndelete)) { + Notification::whereIn('id', $toUndelete)->update(['is_deleted' => false]); + } + + if (count($newRecipients)) { + $now = Carbon::now('utc')->toDateTimeString(); + + Notification::insert( + array_map(function ($user) use ($attributes, $notification, $now) { + return $attributes + ['user_id' => $user->id, 'time' => $now]; + }, $newRecipients) + ); + + foreach ($newRecipients as $user) { + if ($user->shouldEmail($notification::getType())) { + $this->mailer->send($notification, $user); + } + } + } + } + + public function onePerUser(Closure $callback) + { + $this->sentTo = []; + $this->onePerUser = true; + + $callback(); + + $this->onePerUser = false; + } +} diff --git a/src/Core/Notifications/Notifier.php b/src/Core/Notifications/Notifier.php deleted file mode 100644 index 7e692f095..000000000 --- a/src/Core/Notifications/Notifier.php +++ /dev/null @@ -1,86 +0,0 @@ -container = $container; - } - - public function send(Notification $notification, array $users) - { - foreach ($users as $user) { - if ($this->onePerUser && in_array($user->id, $this->sentTo)) { - continue; - } - - foreach ($this->methods as $method => $sender) { - $sender = $this->container->make($sender); - - if ($sender::compatibleWith($notification) && - $user->shouldNotify($notification::getType(), $method)) { - $sender->send($notification, $user); - $this->sentTo[] = $user->id; - } - } - } - } - - public function retract(Notification $notification) - { - foreach ($this->methods as $method => $sender) { - $sender = $this->container->make($sender); - - if ($sender instanceof RetractableSender && $sender::compatibleWith($notification)) { - $sender->retract($notification); - } - } - } - - public function registerMethod($name, $class) - { - $this->methods[$name] = $class; - } - - public function registerType($class) - { - $this->types[] = $class; - } - - public function getMethods() - { - return $this->methods; - } - - public function getTypes() - { - return $this->types; - } - - public function onePerUser(Closure $callback) - { - $this->sentTo = []; - $this->onePerUser = true; - - $callback(); - - $this->onePerUser = false; - } -} diff --git a/src/Core/Notifications/Senders/NotificationAlerter.php b/src/Core/Notifications/Senders/NotificationAlerter.php deleted file mode 100644 index 9d02fab48..000000000 --- a/src/Core/Notifications/Senders/NotificationAlerter.php +++ /dev/null @@ -1,34 +0,0 @@ -id, - $notification::getType(), - $notification->getSender()->id, - $notification->getSubject()->id, - $notification->getAlertData() - ); - - $model->save(); - } - - public function retract(Notification $notification) - { - $models = NotificationModel::where('type', $notification::getType()) - ->where('subject_id', $notification->getSubject()->id) - ->delete(); - } - - public static function compatibleWith($className) - { - return (new ReflectionClass($className))->implementsInterface('Flarum\Core\Notifications\Types\AlertableNotification'); - } -} diff --git a/src/Core/Notifications/Senders/NotificationSender.php b/src/Core/Notifications/Senders/NotificationSender.php deleted file mode 100644 index a94abab6d..000000000 --- a/src/Core/Notifications/Senders/NotificationSender.php +++ /dev/null @@ -1,11 +0,0 @@ -discussion = $discussion; - $this->sender = $sender; - $this->post = $post; - } - - public function getSubject() - { - return $this->discussion; - } - - public function getSender() - { - return $this->sender; - } - - public function getAlertData() - { - return ['postNumber' => $this->post->number]; - } - - public static function getType() - { - return 'discussionRenamed'; - } - - public static function getSubjectModel() - { - return 'Flarum\Core\Models\Discussion'; - } -} diff --git a/src/Core/Notifications/Types/EmailableNotification.php b/src/Core/Notifications/Types/EmailableNotification.php deleted file mode 100644 index 17691c24d..000000000 --- a/src/Core/Notifications/Types/EmailableNotification.php +++ /dev/null @@ -1,8 +0,0 @@ -where('user_id', $userId) - ->whereIn('type', array_keys(Notification::getTypes())) + ->where('user_id', $user->id) + ->whereIn('type', array_filter(array_keys(Notification::getTypes()), [$user, 'shouldAlert'])) + ->where('is_deleted', false) ->groupBy('type', 'subject_id') ->orderBy('time', 'desc') ->skip($offset) diff --git a/src/Core/Repositories/NotificationRepositoryInterface.php b/src/Core/Repositories/NotificationRepositoryInterface.php index cf7678ecd..d23f5d8f1 100644 --- a/src/Core/Repositories/NotificationRepositoryInterface.php +++ b/src/Core/Repositories/NotificationRepositoryInterface.php @@ -1,6 +1,8 @@ class = $class; + $this->serializer = $serializer; } public function enableByDefault($method) @@ -24,17 +28,16 @@ class NotificationType implements ExtenderInterface public function extend(Application $app) { - $notifier = $app['flarum.notifier']; $class = $this->class; - $notifier->registerType($class); - Notification::registerType($class); - foreach ($notifier->getMethods() as $method => $sender) { - if ($sender::compatibleWith($class)) { - User::registerPreference(User::notificationPreferenceKey($class::getType(), $method), 'boolval', in_array($method, $this->enabled)); - } + User::registerPreference(User::notificationPreferenceKey($class::getType(), 'alert'), 'boolval', in_array('alert', $this->enabled)); + + if ($class::isEmailable()) { + User::registerPreference(User::notificationPreferenceKey($class::getType(), 'email'), 'boolval', in_array('email', $this->enabled)); } + + NotificationSerializer::$subjects[$class::getType()] = $this->serializer; } }