From d41fd7a8dd2a9e4f4117a07c9f87346dbbd661f6 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 14 Nov 2023 10:31:44 +0000 Subject: [PATCH] Notifications: Review of PR to include path path #4629 - Merged book and chapter name items to a single page path list item which has links to parent page/chapter. - Added permission filtering to page path elements. - Added page path to also be on comment notifications. - Updated testing to cover. - Added new Message Line objects to support. Done during review of #4629 --- .../MessageParts/EntityLinkMessageLine.php | 29 +++++++++++++++ .../MessageParts/EntityPathMessageLine.php | 35 ++++++++++++++++++ .../Messages/BaseActivityNotification.php | 20 +++++++++++ .../Messages/CommentCreationNotification.php | 14 +++++--- .../Messages/PageCreationNotification.php | 27 ++++---------- .../Messages/PageUpdateNotification.php | 25 ++++--------- lang/de/notifications.php | 2 -- lang/de_informal/notifications.php | 2 -- lang/en/notifications.php | 3 +- tests/Activity/WatchTest.php | 36 +++++++++++++++++-- 10 files changed, 140 insertions(+), 53 deletions(-) create mode 100644 app/Activity/Notifications/MessageParts/EntityLinkMessageLine.php create mode 100644 app/Activity/Notifications/MessageParts/EntityPathMessageLine.php diff --git a/app/Activity/Notifications/MessageParts/EntityLinkMessageLine.php b/app/Activity/Notifications/MessageParts/EntityLinkMessageLine.php new file mode 100644 index 000000000..599833cce --- /dev/null +++ b/app/Activity/Notifications/MessageParts/EntityLinkMessageLine.php @@ -0,0 +1,29 @@ +entity->getUrl()) . '">' . e($this->entity->getShortName($this->nameLength)) . ''; + } + + public function __toString(): string + { + return "{$this->entity->getShortName($this->nameLength)} ({$this->entity->getUrl()})"; + } +} diff --git a/app/Activity/Notifications/MessageParts/EntityPathMessageLine.php b/app/Activity/Notifications/MessageParts/EntityPathMessageLine.php new file mode 100644 index 000000000..4b0f6e6cf --- /dev/null +++ b/app/Activity/Notifications/MessageParts/EntityPathMessageLine.php @@ -0,0 +1,35 @@ +entityLinks = array_map(fn (Entity $entity) => new EntityLinkMessageLine($entity, 24), $this->entities); + } + + public function toHtml(): string + { + $entityHtmls = array_map(fn (EntityLinkMessageLine $line) => $line->toHtml(), $this->entityLinks); + return implode(' > ', $entityHtmls); + } + + public function __toString(): string + { + return implode(' > ', $this->entityLinks); + } +} diff --git a/app/Activity/Notifications/Messages/BaseActivityNotification.php b/app/Activity/Notifications/Messages/BaseActivityNotification.php index 322df5d94..ca86eb81b 100644 --- a/app/Activity/Notifications/Messages/BaseActivityNotification.php +++ b/app/Activity/Notifications/Messages/BaseActivityNotification.php @@ -3,8 +3,12 @@ namespace BookStack\Activity\Notifications\Messages; use BookStack\Activity\Models\Loggable; +use BookStack\Activity\Notifications\MessageParts\EntityPathMessageLine; use BookStack\Activity\Notifications\MessageParts\LinkedMailMessageLine; use BookStack\App\MailNotification; +use BookStack\Entities\Models\Entity; +use BookStack\Entities\Models\Page; +use BookStack\Permissions\PermissionApplicator; use BookStack\Translation\LocaleDefinition; use BookStack\Users\Models\User; use Illuminate\Bus\Queueable; @@ -44,4 +48,20 @@ abstract class BaseActivityNotification extends MailNotification $locale->trans('notifications.footer_reason_link'), ); } + + /** + * Build a line which provides the book > chapter path to a page. + * Takes into account visibility of these parent items. + * Returns null if no path items can be used. + */ + protected function buildPagePathLine(Page $page, User $notifiable): ?EntityPathMessageLine + { + $permissions = new PermissionApplicator($notifiable); + + $path = array_filter([$page->book, $page->chapter], function (?Entity $entity) use ($permissions) { + return !is_null($entity) && $permissions->checkOwnableUserAccess($entity, 'view'); + }); + + return empty($path) ? null : new EntityPathMessageLine($path); + } } diff --git a/app/Activity/Notifications/Messages/CommentCreationNotification.php b/app/Activity/Notifications/Messages/CommentCreationNotification.php index 094ab30b7..30d0ffa2b 100644 --- a/app/Activity/Notifications/Messages/CommentCreationNotification.php +++ b/app/Activity/Notifications/Messages/CommentCreationNotification.php @@ -3,6 +3,7 @@ namespace BookStack\Activity\Notifications\Messages; use BookStack\Activity\Models\Comment; +use BookStack\Activity\Notifications\MessageParts\EntityLinkMessageLine; use BookStack\Activity\Notifications\MessageParts\ListMessageLine; use BookStack\Entities\Models\Page; use BookStack\Users\Models\User; @@ -19,14 +20,17 @@ class CommentCreationNotification extends BaseActivityNotification $locale = $notifiable->getLocale(); + $listLines = array_filter([ + $locale->trans('notifications.detail_page_name') => new EntityLinkMessageLine($page), + $locale->trans('notifications.detail_page_path') => $this->buildPagePathLine($page, $notifiable), + $locale->trans('notifications.detail_commenter') => $this->user->name, + $locale->trans('notifications.detail_comment') => strip_tags($comment->html), + ]); + return $this->newMailMessage($locale) ->subject($locale->trans('notifications.new_comment_subject', ['pageName' => $page->getShortName()])) ->line($locale->trans('notifications.new_comment_intro', ['appName' => setting('app-name')])) - ->line(new ListMessageLine([ - $locale->trans('notifications.detail_page_name') => $page->name, - $locale->trans('notifications.detail_commenter') => $this->user->name, - $locale->trans('notifications.detail_comment') => strip_tags($comment->html), - ])) + ->line(new ListMessageLine($listLines)) ->action($locale->trans('notifications.action_view_comment'), $page->getUrl('#comment' . $comment->local_id)) ->line($this->buildReasonFooterLine($locale)); } diff --git a/app/Activity/Notifications/Messages/PageCreationNotification.php b/app/Activity/Notifications/Messages/PageCreationNotification.php index e98f0c20c..0b98ad30c 100644 --- a/app/Activity/Notifications/Messages/PageCreationNotification.php +++ b/app/Activity/Notifications/Messages/PageCreationNotification.php @@ -2,9 +2,9 @@ namespace BookStack\Activity\Notifications\Messages; +use BookStack\Activity\Notifications\MessageParts\EntityLinkMessageLine; use BookStack\Activity\Notifications\MessageParts\ListMessageLine; use BookStack\Entities\Models\Page; -use BookStack\Entities\Models\Chapter; use BookStack\Users\Models\User; use Illuminate\Notifications\Messages\MailMessage; @@ -14,32 +14,19 @@ class PageCreationNotification extends BaseActivityNotification { /** @var Page $page */ $page = $this->detail; - $book = $page->book; - $chapterId = $page->chapter_id; - $chapter = $chapterId ? Chapter::find($chapterId) : null; $locale = $notifiable->getLocale(); - $listMessageData = [ - $locale->trans('notifications.detail_page_name') => $page->name, - '' => '', - ]; - - if ($chapter) { - $listMessageData += [ - $locale->trans('notifications.detail_chapter_name') => $chapter->name, - ]; - } - - $listMessageData += [ - $locale->trans('notifications.detail_book_name') => $book->name, + $listLines = array_filter([ + $locale->trans('notifications.detail_page_name') => new EntityLinkMessageLine($page), + $locale->trans('notifications.detail_page_path') => $this->buildPagePathLine($page, $notifiable), $locale->trans('notifications.detail_created_by') => $this->user->name, - ]; + ]); return $this->newMailMessage($locale) ->subject($locale->trans('notifications.new_page_subject', ['pageName' => $page->getShortName()])) - ->line($locale->trans('notifications.new_page_intro', ['appName' => setting('app-name')], $locale)) - ->line(new ListMessageLine($listMessageData)) + ->line($locale->trans('notifications.new_page_intro', ['appName' => setting('app-name')])) + ->line(new ListMessageLine($listLines)) ->action($locale->trans('notifications.action_view_page'), $page->getUrl()) ->line($this->buildReasonFooterLine($locale)); } diff --git a/app/Activity/Notifications/Messages/PageUpdateNotification.php b/app/Activity/Notifications/Messages/PageUpdateNotification.php index a303a7883..80ee378cc 100644 --- a/app/Activity/Notifications/Messages/PageUpdateNotification.php +++ b/app/Activity/Notifications/Messages/PageUpdateNotification.php @@ -2,9 +2,9 @@ namespace BookStack\Activity\Notifications\Messages; +use BookStack\Activity\Notifications\MessageParts\EntityLinkMessageLine; use BookStack\Activity\Notifications\MessageParts\ListMessageLine; use BookStack\Entities\Models\Page; -use BookStack\Entities\Models\Chapter; use BookStack\Users\Models\User; use Illuminate\Notifications\Messages\MailMessage; @@ -14,32 +14,19 @@ class PageUpdateNotification extends BaseActivityNotification { /** @var Page $page */ $page = $this->detail; - $book = $page->book; - $chapterId = $page->chapter_id; - $chapter = $chapterId ? Chapter::find($chapterId) : null; $locale = $notifiable->getLocale(); - $listMessageData = [ - $locale->trans('notifications.detail_page_name') => $page->name, - '' => '', - ]; - - if ($chapter) { - $listMessageData += [ - $locale->trans('notifications.detail_chapter_name') => $chapter->name, - ]; - } - - $listMessageData += [ - $locale->trans('notifications.detail_book_name') => $book->name, + $listLines = array_filter([ + $locale->trans('notifications.detail_page_name') => new EntityLinkMessageLine($page), + $locale->trans('notifications.detail_page_path') => $this->buildPagePathLine($page, $notifiable), $locale->trans('notifications.detail_updated_by') => $this->user->name, - ]; + ]); return $this->newMailMessage($locale) ->subject($locale->trans('notifications.updated_page_subject', ['pageName' => $page->getShortName()])) ->line($locale->trans('notifications.updated_page_intro', ['appName' => setting('app-name')])) - ->line(new ListMessageLine($listMessageData)) + ->line(new ListMessageLine($listLines)) ->line($locale->trans('notifications.updated_page_debounce')) ->action($locale->trans('notifications.action_view_page'), $page->getUrl()) ->line($this->buildReasonFooterLine($locale)); diff --git a/lang/de/notifications.php b/lang/de/notifications.php index c1691f89a..314f0bfe3 100644 --- a/lang/de/notifications.php +++ b/lang/de/notifications.php @@ -12,8 +12,6 @@ return [ 'updated_page_intro' => 'Eine Seite wurde in :appName aktualisiert:', 'updated_page_debounce' => 'Um eine Flut von Benachrichtigungen zu vermeiden, werden Sie für eine gewisse Zeit keine Benachrichtigungen für weitere Bearbeitungen dieser Seite durch denselben Bearbeiter erhalten.', - 'detail_book_name' => 'Name des Buches:', - 'detail_chapter_name' => 'Name des Kapitels:', 'detail_page_name' => 'Name der Seite:', 'detail_commenter' => 'Kommentator:', 'detail_comment' => 'Kommentar:', diff --git a/lang/de_informal/notifications.php b/lang/de_informal/notifications.php index 7b01bccd1..fc6204d50 100644 --- a/lang/de_informal/notifications.php +++ b/lang/de_informal/notifications.php @@ -12,8 +12,6 @@ return [ 'updated_page_intro' => 'Eine Seite wurde in :appName aktualisiert:', 'updated_page_debounce' => 'Um eine Flut von Benachrichtigungen zu vermeiden, wirst du für eine gewisse Zeit keine Benachrichtigungen für weitere Bearbeitungen dieser Seite durch denselben Bearbeiter erhalten.', - 'detail_book_name' => 'Buchname:', - 'detail_chapter_name' => 'Kapitelname:', 'detail_page_name' => 'Seitenname:', 'detail_commenter' => 'Kommentator:', 'detail_comment' => 'Kommentar:', diff --git a/lang/en/notifications.php b/lang/en/notifications.php index f476ee5fc..1afd23f1d 100644 --- a/lang/en/notifications.php +++ b/lang/en/notifications.php @@ -12,9 +12,8 @@ return [ 'updated_page_intro' => 'A page has been updated in :appName:', 'updated_page_debounce' => 'To prevent a mass of notifications, for a while you won\'t be sent notifications for further edits to this page by the same editor.', - 'detail_book_name' => 'Book Name:', - 'detail_chapter_name' => 'Chapter Name:', 'detail_page_name' => 'Page Name:', + 'detail_page_path' => 'Page Path:', 'detail_commenter' => 'Commenter:', 'detail_comment' => 'Comment:', 'detail_created_by' => 'Created By:', diff --git a/tests/Activity/WatchTest.php b/tests/Activity/WatchTest.php index 5b9ae5a4c..42216a379 100644 --- a/tests/Activity/WatchTest.php +++ b/tests/Activity/WatchTest.php @@ -12,7 +12,6 @@ use BookStack\Activity\Tools\ActivityLogger; use BookStack\Activity\Tools\UserEntityWatchOptions; use BookStack\Activity\WatchLevels; use BookStack\Entities\Models\Entity; -use BookStack\Entities\Tools\TrashCan; use BookStack\Settings\UserNotificationPreferences; use Illuminate\Support\Facades\Notification; use Tests\TestCase; @@ -268,6 +267,7 @@ class WatchTest extends TestCase return $mail->subject === 'New comment on page: ' . $entities['page']->getShortName() && str_contains($mailContent, 'View Comment') && str_contains($mailContent, 'Page Name: ' . $entities['page']->name) + && str_contains($mailContent, 'Page Path: ' . $entities['book']->getShortName(24) . ' > ' . $entities['chapter']->getShortName(24)) && str_contains($mailContent, 'Commenter: ' . $admin->name) && str_contains($mailContent, 'Comment: My new comment response'); }); @@ -285,12 +285,13 @@ class WatchTest extends TestCase $this->actingAs($admin); $this->entities->updatePage($entities['page'], ['name' => 'Updated page', 'html' => 'new page content']); - $notifications->assertSentTo($editor, function (PageUpdateNotification $notification) use ($editor, $admin) { + $notifications->assertSentTo($editor, function (PageUpdateNotification $notification) use ($editor, $admin, $entities) { $mail = $notification->toMail($editor); $mailContent = html_entity_decode(strip_tags($mail->render()), ENT_QUOTES); return $mail->subject === 'Updated page: Updated page' && str_contains($mailContent, 'View Page') && str_contains($mailContent, 'Page Name: Updated page') + && str_contains($mailContent, 'Page Path: ' . $entities['book']->getShortName(24) . ' > ' . $entities['chapter']->getShortName(24)) && str_contains($mailContent, 'Updated By: ' . $admin->name) && str_contains($mailContent, 'you won\'t be sent notifications for further edits to this page by the same editor'); }); @@ -314,12 +315,13 @@ class WatchTest extends TestCase $page = $entities['chapter']->pages()->where('draft', '=', true)->first(); $this->post($page->getUrl(), ['name' => 'My new page', 'html' => 'My new page content']); - $notifications->assertSentTo($editor, function (PageCreationNotification $notification) use ($editor, $admin) { + $notifications->assertSentTo($editor, function (PageCreationNotification $notification) use ($editor, $admin, $entities) { $mail = $notification->toMail($editor); $mailContent = html_entity_decode(strip_tags($mail->render()), ENT_QUOTES); return $mail->subject === 'New page: My new page' && str_contains($mailContent, 'View Page') && str_contains($mailContent, 'Page Name: My new page') + && str_contains($mailContent, 'Page Path: ' . $entities['book']->getShortName(24) . ' > ' . $entities['chapter']->getShortName(24)) && str_contains($mailContent, 'Created By: ' . $admin->name); }); } @@ -405,4 +407,32 @@ class WatchTest extends TestCase $this->assertDatabaseMissing('watches', ['watchable_type' => 'page', 'watchable_id' => $page->id]); } + + public function test_page_path_in_notifications_limited_by_permissions() + { + $chapter = $this->entities->chapterHasPages(); + $page = $chapter->pages()->first(); + $book = $chapter->book; + $notification = new PageCreationNotification($page, $this->users->editor()); + + $viewer = $this->users->viewer(); + $viewerRole = $viewer->roles()->first(); + + $content = html_entity_decode(strip_tags($notification->toMail($viewer)->render()), ENT_QUOTES); + $this->assertStringContainsString('Page Path: ' . $book->getShortName(24) . ' > ' . $chapter->getShortName(24), $content); + + $this->permissions->setEntityPermissions($page, ['view'], [$viewerRole]); + $this->permissions->setEntityPermissions($chapter, [], [$viewerRole]); + + $content = html_entity_decode(strip_tags($notification->toMail($viewer)->render()), ENT_QUOTES); + $this->assertStringContainsString('Page Path: ' . $book->getShortName(24), $content); + $this->assertStringNotContainsString(' > ' . $chapter->getShortName(24), $content); + + $this->permissions->setEntityPermissions($book, [], [$viewerRole]); + + $content = html_entity_decode(strip_tags($notification->toMail($viewer)->render()), ENT_QUOTES); + $this->assertStringNotContainsString('Page Path:', $content); + $this->assertStringNotContainsString($book->getShortName(24), $content); + $this->assertStringNotContainsString($chapter->getShortName(24), $content); + } }