From 65ac197be440b90deccfd13d4eb8e6bec11e23b0 Mon Sep 17 00:00:00 2001 From: Sascha Date: Thu, 26 Oct 2023 14:01:38 +0200 Subject: [PATCH 1/2] Added book name to the mail template added book name synced with actual file from dev branch added book name add book name added book name extended with chaptername extended with chapter name Update PageUpdateNotification.php Update notifications.php Update notifications.php Update notifications.php correction of chapter syntax correction of chapter syntax --- .../Messages/PageCreationNotification.php | 25 ++++++++++++++++--- .../Messages/PageUpdateNotification.php | 25 ++++++++++++++++--- lang/de/notifications.php | 2 ++ lang/de_informal/notifications.php | 2 ++ lang/en/notifications.php | 2 ++ 5 files changed, 48 insertions(+), 8 deletions(-) diff --git a/app/Activity/Notifications/Messages/PageCreationNotification.php b/app/Activity/Notifications/Messages/PageCreationNotification.php index da028aa8c..e98f0c20c 100644 --- a/app/Activity/Notifications/Messages/PageCreationNotification.php +++ b/app/Activity/Notifications/Messages/PageCreationNotification.php @@ -4,6 +4,7 @@ namespace BookStack\Activity\Notifications\Messages; 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; @@ -13,16 +14,32 @@ 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, + $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([ - $locale->trans('notifications.detail_page_name') => $page->name, - $locale->trans('notifications.detail_created_by') => $this->user->name, - ])) + ->line(new ListMessageLine($listMessageData)) ->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 1c8155d29..a303a7883 100644 --- a/app/Activity/Notifications/Messages/PageUpdateNotification.php +++ b/app/Activity/Notifications/Messages/PageUpdateNotification.php @@ -4,6 +4,7 @@ namespace BookStack\Activity\Notifications\Messages; 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; @@ -13,16 +14,32 @@ 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, + $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([ - $locale->trans('notifications.detail_page_name') => $page->name, - $locale->trans('notifications.detail_updated_by') => $this->user->name, - ])) + ->line(new ListMessageLine($listMessageData)) ->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 314f0bfe3..c1691f89a 100644 --- a/lang/de/notifications.php +++ b/lang/de/notifications.php @@ -12,6 +12,8 @@ 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 fc6204d50..7b01bccd1 100644 --- a/lang/de_informal/notifications.php +++ b/lang/de_informal/notifications.php @@ -12,6 +12,8 @@ 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 5539ae9a9..f476ee5fc 100644 --- a/lang/en/notifications.php +++ b/lang/en/notifications.php @@ -12,6 +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_commenter' => 'Commenter:', 'detail_comment' => 'Comment:', From d41fd7a8dd2a9e4f4117a07c9f87346dbbd661f6 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 14 Nov 2023 10:31:44 +0000 Subject: [PATCH 2/2] 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); + } }