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
2023-11-14 10:31:44 +00:00 · 2023-11-14 10:31:44 +00:00 · d41fd7a8dd
commit d41fd7a8dd
parent 65ac197be4
10 changed files with 140 additions and 53 deletions
--- a/app/Activity/Notifications/MessageParts/EntityLinkMessageLine.php
+++ b/app/Activity/Notifications/MessageParts/EntityLinkMessageLine.php
@ -0,0 +1,29 @@
+<?php
+
+namespace BookStack\Activity\Notifications\MessageParts;
+
+use BookStack\Entities\Models\Entity;
+use Illuminate\Contracts\Support\Htmlable;
+use Stringable;
+
+/**
+ * A link to a specific entity in the system, with the text showing its name.
+ */
+class EntityLinkMessageLine implements Htmlable, Stringable
+{
+    public function __construct(
+        protected Entity $entity,
+        protected int $nameLength = 120,
+    ) {
+    }
+
+    public function toHtml(): string
+    {
+        return '<a href="' . e($this->entity->getUrl()) . '">' . e($this->entity->getShortName($this->nameLength)) . '</a>';
+    }
+
+    public function __toString(): string
+    {
+        return "{$this->entity->getShortName($this->nameLength)} ({$this->entity->getUrl()})";
+    }
+}
--- a/app/Activity/Notifications/MessageParts/EntityPathMessageLine.php
+++ b/app/Activity/Notifications/MessageParts/EntityPathMessageLine.php
@ -0,0 +1,35 @@
+<?php
+
+namespace BookStack\Activity\Notifications\MessageParts;
+
+use BookStack\Entities\Models\Entity;
+use Illuminate\Contracts\Support\Htmlable;
+use Stringable;
+
+/**
+ * A link to a specific entity in the system, with the text showing its name.
+ */
+class EntityPathMessageLine implements Htmlable, Stringable
+{
+    /**
+     * @var EntityLinkMessageLine[]
+     */
+    protected array $entityLinks;
+
+    public function __construct(
+        protected array $entities
+    ) {
+        $this->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(' &gt; ', $entityHtmls);
+    }
+
+    public function __toString(): string
+    {
+        return implode(' > ', $this->entityLinks);
+    }
+}
--- 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);
+    }
 }
--- 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));
    }
--- 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));
    }
--- 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));
--- 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:',
--- 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:',
--- 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:',
--- 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);
+    }
 }