From f99af807d044b55ef8ec6c43d8ad8c7d61b47146 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 4 Oct 2021 20:26:55 +0100 Subject: [PATCH] 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. --- app/Entities/Models/PageRevision.php | 11 +++-- app/Entities/Tools/PageEditActivity.php | 34 +++++++++----- app/Http/Controllers/PageController.php | 25 ++--------- resources/js/components/page-editor.js | 4 +- resources/lang/en/entities.php | 1 + tests/Entity/PageDraftTest.php | 60 +++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 39 deletions(-) diff --git a/app/Entities/Models/PageRevision.php b/app/Entities/Models/PageRevision.php index c1a74f66b..b994e7a04 100644 --- 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 Illuminate\Database\Eloquent\Relations\BelongsTo; /** * Class PageRevision. @@ -14,11 +15,13 @@ use Carbon\Carbon; * @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 Page $page */ class PageRevision extends Model { @@ -26,20 +29,16 @@ class PageRevision extends Model /** * 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 \Illuminate\Database\Eloquent\Relations\BelongsTo */ - public function page() + public function page(): BelongsTo { return $this->belongsTo(Page::class); } diff --git a/app/Entities/Tools/PageEditActivity.php b/app/Entities/Tools/PageEditActivity.php index 2e620aa2c..f23506a8c 100644 --- 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. - * - * @return bool */ public function hasActiveEditing(): bool { @@ -44,21 +42,35 @@ class PageEditActivity } /** - * Check if the page has been updated since the draft has been saved. - * - * @return bool + * Get any editor clash warning messages to show for the given draft revision. + * @param PageRevision|Page $draft + * @return string[] */ - public function hasPageBeenUpdatedSinceDraftSaved(PageRevision $draft): bool + public function getWarningMessagesForDraft($draft): array { - return $draft->page->updated_at->timestamp >= $draft->updated_at->timestamp; + $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. + */ + protected function hasPageBeenUpdatedSinceDraftCreated(PageRevision $draft): bool + { + 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. - * - * @param PageRevision $draft - * - * @return string */ public function getEditingActiveDraftMessage(PageRevision $draft): string { diff --git a/app/Http/Controllers/PageController.php b/app/Http/Controllers/PageController.php index 4818b5211..9025db162 100644 --- 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\Entities\Models\PageRevision; 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); } - // 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'])); - - $updateTime = $draft->updated_at->timestamp; + $warnings = (new PageEditActivity($page))->getWarningMessagesForDraft($draft); 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 e753b58e1..5f35e6499 100644 --- a/resources/js/components/page-editor.js +++ b/resources/js/components/page-editor.js @@ -40,6 +40,7 @@ class PageEditor { frequency: 30000, last: 0, }; + this.shownWarningsCache = new Set(); 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(); - if (resp.data.warning.length > 0) { + if (resp.data.warning && !this.shownWarningsCache.has(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. diff --git a/resources/lang/en/entities.php b/resources/lang/en/entities.php index 1be9c18e0..0a4068eea 100644 --- 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_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', diff --git a/tests/Entity/PageDraftTest.php b/tests/Entity/PageDraftTest.php index b2fa4bb31..21a74768f 100644 --- 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\PageRevision; use BookStack\Entities\Repos\PageRepo; use Tests\TestCase; @@ -80,6 +81,65 @@ class PageDraftTest extends TestCase ->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' => '

updated draft

', + ]); + + /** @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' => '

admin update

', + ]); + + $resp = $this->actingAs($editor)->put('/ajax/page/' . $page->id . '/save-draft', [ + 'name' => $page->name, + 'html' => '

updated draft again

', + ]); + + $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' => '

updated draft

', + ]); + + $resp = $this->actingAs($editor)->put('/ajax/page/' . $page->id . '/save-draft', [ + 'name' => $page->name, + 'html' => '

updated draft again

', + ]); + + $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 */