Reviewed and refactored additional editor draft save warnings

- Added testing to cover warning cases.
- Refactored logic to be simpler and move much of the business out of
  the controller.
- Added new message that's more suitable to the case this was handling.
- For detecting an outdated draft, checked the draft created_at time
  instead of updated_at to better fit the scenario being checked.
- Updated some method types to align with those potentially being used
  in the logic of the code.
- Added a cache of shown messages on the front-end to prevent them
  re-showing on every save during the session, even if dismissed.

diff --git a/app/Entities/Models/PageRevision.php b/app/Entities/Models/PageRevision.php
index c1a74f66b2a92d5664a48ebb0649a1365539e17b..b994e7a04ceec606e56757362e87a74372311dad 100644 (file)
--- a/app/Entities/Models/PageRevision.php
+++ b/app/Entities/Models/PageRevision.php
@@ -5,6 +5,7 @@ namespace BookStack\Entities\Models;
 use BookStack\Auth\User;
 use BookStack\Model;
 use Carbon\Carbon;
 use BookStack\Auth\User;
 use BookStack\Model;
 use Carbon\Carbon;

+use Illuminate\Database\Eloquent\Relations\BelongsTo;

 
 /**
  * Class PageRevision.
 
 /**
  * Class PageRevision.


@@ -14,11 +15,13 @@ use Carbon\Carbon;
  * @property string $book_slug
  * @property int    $created_by
  * @property Carbon $created_at
  * @property string $book_slug
  * @property int    $created_by
  * @property Carbon $created_at

+ * @property Carbon $updated_at

  * @property string $type
  * @property string $summary
  * @property string $markdown
  * @property string $html
  * @property int    $revision_number
  * @property string $type
  * @property string $summary
  * @property string $markdown
  * @property string $html
  * @property int    $revision_number

+ * @property Page   $page

  */
 class PageRevision extends Model
 {
  */
 class PageRevision extends Model
 {


@@ -26,20 +29,16 @@ class PageRevision extends Model
 
     /**
      * Get the user that created the page revision.
 
     /**
      * Get the user that created the page revision.

-     *
-     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo

      */
      */

-    public function createdBy()
+    public function createdBy(): BelongsTo

     {
         return $this->belongsTo(User::class, 'created_by');
     }
 
     /**
      * Get the page this revision originates from.
     {
         return $this->belongsTo(User::class, 'created_by');
     }
 
     /**
      * Get the page this revision originates from.

-     *
-     * @return \Illuminate\Database\Eloquent\Relations\BelongsTo

      */
      */

-    public function page()
+    public function page(): BelongsTo

     {
         return $this->belongsTo(Page::class);
     }
     {
         return $this->belongsTo(Page::class);
     }

diff --git a/app/Entities/Tools/PageEditActivity.php b/app/Entities/Tools/PageEditActivity.php
index 2e620aa2c11b0b403510d920d91c846ab45427a9..f23506a8c138ebb5be2e9020f8b3dd97fb0a0603 100644 (file)
--- a/app/Entities/Tools/PageEditActivity.php
+++ b/app/Entities/Tools/PageEditActivity.php
@@ -21,8 +21,6 @@ class PageEditActivity
 
     /**
      * Check if there's active editing being performed on this page.
 
     /**
      * Check if there's active editing being performed on this page.

-     *
-     * @return bool

      */
     public function hasActiveEditing(): bool
     {
      */
     public function hasActiveEditing(): bool
     {


@@ -43,22 +41,36 @@ class PageEditActivity
         return trans('entities.pages_draft_edit_active.message', ['start' => $userMessage, 'time' => $timeMessage]);
     }
 
         return trans('entities.pages_draft_edit_active.message', ['start' => $userMessage, 'time' => $timeMessage]);
     }
 

+    /**
+     * Get any editor clash warning messages to show for the given draft revision.
+     * @param PageRevision|Page $draft
+     * @return string[]
+     */
+    public function getWarningMessagesForDraft($draft): array
+    {
+        $warnings = [];
+
+        if ($this->hasActiveEditing()) {
+            $warnings[] = $this->activeEditingMessage();
+        }
+
+        if ($draft instanceof PageRevision && $this->hasPageBeenUpdatedSinceDraftCreated($draft)) {
+            $warnings[] = trans('entities.pages_draft_page_changed_since_creation');
+        }
+
+        return $warnings;
+    }
+

     /**
      * Check if the page has been updated since the draft has been saved.
     /**
      * Check if the page has been updated since the draft has been saved.

-     *
-     * @return bool

      */
      */

-    public function hasPageBeenUpdatedSinceDraftSaved(PageRevision $draft): bool
+    protected function hasPageBeenUpdatedSinceDraftCreated(PageRevision $draft): bool

     {
     {

-        return $draft->page->updated_at->timestamp >= $draft->updated_at->timestamp;
+        return $draft->page->updated_at->timestamp > $draft->created_at->timestamp;

     }
 
     /**
      * Get the message to show when the user will be editing one of their drafts.
     }
 
     /**
      * Get the message to show when the user will be editing one of their drafts.

-     *
-     * @param PageRevision $draft
-     *
-     * @return string

      */
     public function getEditingActiveDraftMessage(PageRevision $draft): string
     {
      */
     public function getEditingActiveDraftMessage(PageRevision $draft): string
     {

diff --git a/app/Http/Controllers/PageController.php b/app/Http/Controllers/PageController.php
index 4818b52116ce968f9533fe712400565a8f96d6d8..9025db162ab949709287a298c6918464c177cfb1 100644 (file)
--- a/app/Http/Controllers/PageController.php
+++ b/app/Http/Controllers/PageController.php
@@ -4,6 +4,7 @@ namespace BookStack\Http\Controllers;
 
 use BookStack\Actions\View;
 use BookStack\Entities\Models\Page;
 
 use BookStack\Actions\View;
 use BookStack\Entities\Models\Page;

+use BookStack\Entities\Models\PageRevision;

 use BookStack\Entities\Repos\PageRepo;
 use BookStack\Entities\Tools\BookContents;
 use BookStack\Entities\Tools\NextPreviousContentLocator;
 use BookStack\Entities\Repos\PageRepo;
 use BookStack\Entities\Tools\BookContents;
 use BookStack\Entities\Tools\NextPreviousContentLocator;


@@ -258,32 +259,14 @@ class PageController extends Controller
             return $this->jsonError(trans('errors.guests_cannot_save_drafts'), 500);
         }
 
             return $this->jsonError(trans('errors.guests_cannot_save_drafts'), 500);
         }
 

-        // Check for active editing or time conflict
-        $warnings = [];
-        $jsonResponseWarning = '';
-        $editActivity = new PageEditActivity($page);
-        if ($editActivity->hasActiveEditing()) {
-            $warnings[] = $editActivity->activeEditingMessage();
-        }
-        $userDraft = $this->pageRepo->getUserDraft($page);
-        if ($userDraft !== null) {
-            if ($editActivity->hasPageBeenUpdatedSinceDraftSaved($userDraft)) {
-                $warnings[] = $editActivity->getEditingActiveDraftMessage($userDraft);
-            }
-        }
-        if (count($warnings) > 0) {
-            $jsonResponseWarning = implode("\n", $warnings);
-        }
-

         $draft = $this->pageRepo->updatePageDraft($page, $request->only(['name', 'html', 'markdown']));
         $draft = $this->pageRepo->updatePageDraft($page, $request->only(['name', 'html', 'markdown']));

-
-        $updateTime = $draft->updated_at->timestamp;
+        $warnings = (new PageEditActivity($page))->getWarningMessagesForDraft($draft);

 
         return response()->json([
             'status'    => 'success',
             'message'   => trans('entities.pages_edit_draft_save_at'),
 
         return response()->json([
             'status'    => 'success',
             'message'   => trans('entities.pages_edit_draft_save_at'),

-            'warning'   => $jsonResponseWarning,
-            'timestamp' => $updateTime,
+            'warning'   => implode("\n", $warnings),
+            'timestamp' => $draft->updated_at->timestamp,

         ]);
     }
 
         ]);
     }
 

diff --git a/resources/js/components/page-editor.js b/resources/js/components/page-editor.js
index e753b58e18309879e3ddfbfdfa5ec78702830e8b..5f35e64992c50b793bdebf33edcb46a634fea4b9 100644 (file)
--- a/resources/js/components/page-editor.js
+++ b/resources/js/components/page-editor.js
@@ -40,6 +40,7 @@ class PageEditor {
             frequency: 30000,
             last: 0,
         };
             frequency: 30000,
             last: 0,
         };

+        this.shownWarningsCache = new Set();

 
         if (this.pageId !== 0 && this.draftsEnabled) {
             window.setTimeout(() => {
 
         if (this.pageId !== 0 && this.draftsEnabled) {
             window.setTimeout(() => {


@@ -119,8 +120,9 @@ class PageEditor {
             }
             this.draftNotifyChange(`${resp.data.message} ${Dates.utcTimeStampToLocalTime(resp.data.timestamp)}`);
             this.autoSave.last = Date.now();
             }
             this.draftNotifyChange(`${resp.data.message} ${Dates.utcTimeStampToLocalTime(resp.data.timestamp)}`);
             this.autoSave.last = Date.now();

-            if (resp.data.warning.length > 0) {
+            if (resp.data.warning && !this.shownWarningsCache.has(resp.data.warning)) {

                 window.$events.emit('warning', resp.data.warning);
                 window.$events.emit('warning', resp.data.warning);

+                this.shownWarningsCache.add(resp.data.warning);

             }
         } catch (err) {
             // Save the editor content in LocalStorage as a last resort, just in case.
             }
         } catch (err) {
             // Save the editor content in LocalStorage as a last resort, just in case.

diff --git a/resources/lang/en/entities.php b/resources/lang/en/entities.php
index 1be9c18e041818ad34df7c6bcc06563bf272f708..0a4068eea9f24e2b4b7a8dbaecbabbdb5b3fe099 100644 (file)
--- a/resources/lang/en/entities.php
+++ b/resources/lang/en/entities.php
@@ -234,6 +234,7 @@ return [
     'pages_initial_name' => 'New Page',
     'pages_editing_draft_notification' => 'You are currently editing a draft that was last saved :timeDiff.',
     'pages_draft_edited_notification' => 'This page has been updated by since that time. It is recommended that you discard this draft.',
     'pages_initial_name' => 'New Page',
     'pages_editing_draft_notification' => 'You are currently editing a draft that was last saved :timeDiff.',
     'pages_draft_edited_notification' => 'This page has been updated by since that time. It is recommended that you discard this draft.',

+    'pages_draft_page_changed_since_creation' => 'This page has been updated since this draft was created. It is recommended that you discard this draft or take care not to overwrite any page changes.',

     'pages_draft_edit_active' => [
         'start_a' => ':count users have started editing this page',
         'start_b' => ':userName has started editing this page',
     'pages_draft_edit_active' => [
         'start_a' => ':count users have started editing this page',
         'start_b' => ':userName has started editing this page',

diff --git a/tests/Entity/PageDraftTest.php b/tests/Entity/PageDraftTest.php
index b2fa4bb318968edccec1965e6b128940b8575217..21a74768f319b92100d023d2cb49fc546e02d103 100644 (file)
--- a/tests/Entity/PageDraftTest.php
+++ b/tests/Entity/PageDraftTest.php
@@ -4,6 +4,7 @@ namespace Tests\Entity;
 
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Models\Page;
 
 use BookStack\Entities\Models\Book;
 use BookStack\Entities\Models\Page;

+use BookStack\Entities\Models\PageRevision;

 use BookStack\Entities\Repos\PageRepo;
 use Tests\TestCase;
 
 use BookStack\Entities\Repos\PageRepo;
 use Tests\TestCase;
 


@@ -80,6 +81,65 @@ class PageDraftTest extends TestCase
             ->assertElementNotContains('.notification', 'Admin has started editing this page');
     }
 
             ->assertElementNotContains('.notification', 'Admin has started editing this page');
     }
 

+    public function test_draft_save_shows_alert_if_draft_older_than_last_page_update()
+    {
+        $admin = $this->getAdmin();
+        $editor = $this->getEditor();
+        /** @var Page $page */
+        $page = Page::query()->first();
+
+        $this->actingAs($editor)->put('/ajax/page/' . $page->id . '/save-draft', [
+            'name' => $page->name,
+            'html' => '<p>updated draft</p>',
+        ]);
+
+        /** @var PageRevision $draft */
+        $draft = $page->allRevisions()
+            ->where('type', '=', 'update_draft')
+            ->where('created_by', '=', $editor->id)
+            ->first();
+        $draft->created_at = now()->subMinute(1);
+        $draft->save();
+
+        $this->actingAs($admin)->put($page->refresh()->getUrl(), [
+            'name' => $page->name,
+            'html' => '<p>admin update</p>',
+        ]);
+
+        $resp = $this->actingAs($editor)->put('/ajax/page/' . $page->id . '/save-draft', [
+            'name' => $page->name,
+            'html' => '<p>updated draft again</p>',
+        ]);
+
+        $resp->assertJson([
+            'warning' => 'This page has been updated since this draft was created. It is recommended that you discard this draft or take care not to overwrite any page changes.',
+        ]);
+    }
+
+    public function test_draft_save_shows_alert_if_draft_edit_started_by_someone_else()
+    {
+        $admin = $this->getAdmin();
+        $editor = $this->getEditor();
+        /** @var Page $page */
+        $page = Page::query()->first();
+
+        $this->actingAs($admin)->put('/ajax/page/' . $page->id . '/save-draft', [
+            'name' => $page->name,
+            'html' => '<p>updated draft</p>',
+        ]);
+
+        $resp = $this->actingAs($editor)->put('/ajax/page/' . $page->id . '/save-draft', [
+            'name' => $page->name,
+            'html' => '<p>updated draft again</p>',
+        ]);
+
+        $resp->assertJson([
+            'warning' => 'Admin has started editing this page in the last 60 minutes. Take care not to overwrite each other\'s updates!',
+        ]);
+    }
+
+
+

     public function test_draft_pages_show_on_homepage()
     {
         /** @var Book $book */
     public function test_draft_pages_show_on_homepage()
     {
         /** @var Book $book */