Skip to content

WIP: Default page templates #3918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 12, 2023
Merged

WIP: Default page templates #3918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3599a96
search-box-cancel placement
lennertdaniels Dec 2, 2022
1dbc358
Add default_template as Book setting
lennertdaniels Dec 2, 2022
99ae759
Prefill new pages with book's default template
lennertdaniels Dec 2, 2022
ec3b06d
Add notice to Page delete confirmation when in use as a template
lennertdaniels Dec 2, 2022
ac519b3
Guest create page: name field autofocus
lennertdaniels Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/Entities/Models/Book.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class Book extends Entity implements HasCoverImage

public $searchFactor = 1.2;

protected $fillable = ['name', 'description'];
protected $fillable = ['name', 'description', 'default_template'];
protected $hidden = ['pivot', 'image_id', 'deleted_at'];

/**
Expand Down Expand Up @@ -78,6 +78,14 @@ public function coverImageTypeKey(): string
return 'cover_book';
}

/**
* Get the Page that is used as default template for newly created pages within this Book.
*/
public function defaultTemplate(): BelongsTo
{
return $this->belongsTo(Page::class, 'default_template');
}

/**
* Get all pages within this book.
*/
Expand Down
6 changes: 6 additions & 0 deletions app/Entities/Repos/PageRepo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,12 @@ public function getNewDraftPage(Entity $parent)
$page->book_id = $parent->id;
}

if ($page->book->defaultTemplate) {
$page->forceFill([
'html' => $page->book->defaultTemplate->html,
]);
}

Comment on lines +151 to +156
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently ignores permissions. I think we'll need to respect page permissions, so view access is required for the template to be used, otherwise other side-effects will be introduced (Image access permissions for example).

Comment on lines +151 to +156
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need to support markdown also here, and probably the original used editor.
I suspect it should just be a case of also copying the markdown and editor values from the original page model.

$page->save();
$page->refresh()->rebuildPermissions();

Expand Down
16 changes: 15 additions & 1 deletion app/Http/Controllers/BookController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use BookStack\Actions\ActivityType;
use BookStack\Actions\View;
use BookStack\Entities\Models\Bookshelf;
use BookStack\Entities\Models\Page;
use BookStack\Entities\Repos\BookRepo;
use BookStack\Entities\Tools\BookContents;
use BookStack\Entities\Tools\Cloner;
Expand Down Expand Up @@ -79,8 +80,14 @@ public function create(string $shelfSlug = null)

$this->setPageTitle(trans('entities.books_create'));

$templates = Page::visible()
->where('template', '=', true)
->orderBy('name', 'asc')
->get();
Comment on lines +83 to +86
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, this'll be loading in all template pages, with all their properties (including html, markdown & text) into memory for use in a list. Probably fine for the majority of use-cases but I'm uncomfortable with the scalability of this.

It's a pain but ideally template selection would be done via front-end querying, much like what's shown when using the "Move" or "Copy" interfaces, to prevent requiring load of all options on form load.


return view('books.create', [
'bookshelf' => $bookshelf,
'templates' => $templates,
]);
}

Expand All @@ -98,6 +105,7 @@ public function store(Request $request, string $shelfSlug = null)
'description' => ['string', 'max:1000'],
'image' => array_merge(['nullable'], $this->getImageValidationRules()),
'tags' => ['array'],
'default_template' => ['nullable', 'exists:pages,id'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now there's nothing stopping a user from setting a page template they don't have access to.
We probably should have some light validation to check the user can view and that the template page is marked as a template.

This exists validation check could also be abused to check the existence of a certain page id what the user may not have access to. Not sure how well we currently defend against that in other areas of the app, but probably would be something I'd attempt to avoid where possible.

]);

$bookshelf = null;
Expand Down Expand Up @@ -151,7 +159,12 @@ public function edit(string $slug)
$this->checkOwnablePermission('book-update', $book);
$this->setPageTitle(trans('entities.books_edit_named', ['bookName' => $book->getShortName()]));

return view('books.edit', ['book' => $book, 'current' => $book]);
$templates = Page::visible()
->where('template', '=', true)
->orderBy('name', 'asc')
->get();
Comment on lines +162 to +165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per create method comment.


return view('books.edit', ['book' => $book, 'current' => $book, 'templates' => $templates]);
}

/**
Expand All @@ -171,6 +184,7 @@ public function update(Request $request, string $slug)
'description' => ['string', 'max:1000'],
'image' => array_merge(['nullable'], $this->getImageValidationRules()),
'tags' => ['array'],
'default_template' => ['nullable', 'exists:pages,id'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per create method comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, if the template option is removed when editing a book the column value will end up as 0. Since nulls are supported in this column, i think the value should end up null instead.

]);

if ($request->has('image_reset')) {
Expand Down
4 changes: 3 additions & 1 deletion app/Http/Controllers/PageController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace BookStack\Http\Controllers;

use BookStack\Actions\View;
use BookStack\Entities\Models\Book;
use BookStack\Entities\Models\Page;
use BookStack\Entities\Repos\PageRepo;
use BookStack\Entities\Tools\BookContents;
Expand Down Expand Up @@ -74,7 +75,6 @@ public function createAsGuest(Request $request, string $bookSlug, string $chapte
$page = $this->pageRepo->getNewDraftPage($parent);
$this->pageRepo->publishDraft($page, [
'name' => $request->get('name'),
'html' => '',
]);

return redirect($page->getUrl('/edit'));
Expand Down Expand Up @@ -266,11 +266,13 @@ public function showDelete(string $bookSlug, string $pageSlug)
$page = $this->pageRepo->getBySlug($bookSlug, $pageSlug);
$this->checkOwnablePermission('page-delete', $page);
$this->setPageTitle(trans('entities.pages_delete_named', ['pageName' => $page->getShortName()]));
$times_used_as_template = Book::where('default_template', '=', $page->id)->count();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable name does not follow project conventions, but use may be redundant (lang file comments).


return view('pages.delete', [
'book' => $page->book,
'page' => $page,
'current' => $page,
'times_used_as_template' => $times_used_as_template,
]);
}

Expand Down
32 changes: 32 additions & 0 deletions database/migrations/2022_12_02_104541_add_default_template_to_books.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

use Illuminate\Database\Migrations\Migration;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Support\Facades\Schema;

class AddDefaultTemplateToBooks extends Migration
{
/**
* Run the migrations.
*
* @return void
*/
public function up()
{
Schema::table('books', function (Blueprint $table) {
$table->integer('default_template')->nullable();
});
}

/**
* Reverse the migrations.
*
* @return void
*/
public function down()
{
Schema::table('books', function (Blueprint $table) {
$table->dropColumn('default_template');
});
}
}
3 changes: 3 additions & 0 deletions resources/lang/en/entities.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@
'pages_delete_draft' => 'Delete Draft Page',
'pages_delete_success' => 'Page deleted',
'pages_delete_draft_success' => 'Draft page deleted',
'pages_delete_warning_template' => '{0}|{1}Be careful: this page is used as a template for :count book.|[2,*]Be careful: this page is used as a template for :count books.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to show the count here (Which again can complicate matters when permissions are in play) but I just think we need the user to be aware of the consequences, something like:

This page is in active use as a book default page template. Such books will no longer have a page default template assigned after this page is deleted.

'pages_delete_confirm' => 'Are you sure you want to delete this page?',
'pages_delete_draft_confirm' => 'Are you sure you want to delete this draft page?',
'pages_editing_named' => 'Editing Page :pageName',
Expand Down Expand Up @@ -328,6 +329,8 @@
'templates_replace_content' => 'Replace page content',
'templates_append_content' => 'Append to page content',
'templates_prepend_content' => 'Prepend to page content',
'default_template' => 'Default Page Template',
'default_template_explain' => "Assign a default template that will be used for all new pages in this book.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably include a note on permissions, advising that the page template will only be used if the page creator has view permission for the page template.


// Profile View
'profile_user_for_x' => 'User for :time',
Expand Down
2 changes: 1 addition & 1 deletion resources/sass/_forms.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ div[editor-type="markdown"] .title-input.page-title input[type="text"] {
&.flexible input {
width: 100%;
}
.search-box-cancel {
button.search-box-cancel {
left: auto;
right: 0;
}
Expand Down
5 changes: 4 additions & 1 deletion resources/views/books/create.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@
<main class="content-wrap card">
<h1 class="list-heading">{{ trans('entities.books_create') }}</h1>
<form action="{{ isset($bookshelf) ? $bookshelf->getUrl('/create-book') : url('/books') }}" method="POST" enctype="multipart/form-data">
@include('books.parts.form', ['returnLocation' => isset($bookshelf) ? $bookshelf->getUrl() : url('/books')])
@include('books.parts.form', [
'templates' => $templates,
'returnLocation' => isset($bookshelf) ? $bookshelf->getUrl() : url('/books')
])
</form>
</main>
</div>
Expand Down
6 changes: 5 additions & 1 deletion resources/views/books/edit.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
<h1 class="list-heading">{{ trans('entities.books_edit') }}</h1>
<form action="{{ $book->getUrl() }}" method="POST" enctype="multipart/form-data">
<input type="hidden" name="_method" value="PUT">
@include('books.parts.form', ['model' => $book, 'returnLocation' => $book->getUrl()])
@include('books.parts.form', [
'model' => $book,
'templates' => $templates,
'returnLocation' => $book->getUrl()
])
</form>
</main>

Expand Down
9 changes: 9 additions & 0 deletions resources/views/books/parts/form.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,15 @@
</div>
</div>

<div class="form-group collapsible" component="collapsible" id="template-control">
<button refs="collapsible@trigger" type="button" class="collapse-title text-primary" aria-expanded="false">
<label for="template-manager">{{ trans('entities.default_template') }}</label>
</button>
<div refs="collapsible@content" class="collapse-content">
@include('entities.template-manager', ['entity' => $book ?? null, 'templates' => $templates])
</div>
</div>

<div class="form-group text-right">
<a href="{{ $returnLocation }}" class="button outline">{{ trans('common.cancel') }}</a>
<button type="submit" class="button">{{ trans('entities.books_save') }}</button>
Expand Down
10 changes: 10 additions & 0 deletions resources/views/entities/template-manager.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<p class="text-muted small">
{!! nl2br(e(trans('entities.default_template_explain'))) !!}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no newlines in the source string so this can be a simple non-escaped template tag. If there are multiple clear lines I try to split the translation itself these days.

</p>

<select name="default_template" id="default_template">
<option value="">---</option>
@foreach ($templates as $template)
<option @if(isset($entity) && $entity->default_template === $template->id) selected @endif value="{{ $template->id }}">{{ $template->name }}</option>
@endforeach
</select>
3 changes: 3 additions & 0 deletions resources/views/pages/delete.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@
<div class="card content-wrap auto-height">
<h1 class="list-heading">{{ $page->draft ? trans('entities.pages_delete_draft') : trans('entities.pages_delete') }}</h1>

@if ($times_used_as_template > 0)
<p>{{ trans_choice('entities.pages_delete_warning_template', $times_used_as_template) }}</p>
@endif

<div class="grid half v-center">
<div>
Expand Down
2 changes: 1 addition & 1 deletion resources/views/pages/guest-create.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

<div class="form-group title-input">
<label for="name">{{ trans('entities.pages_name') }}</label>
@include('form.text', ['name' => 'name'])
@include('form.text', ['name' => 'name', 'autofocus' => true])
</div>

<div class="form-group text-right">
Expand Down
2 changes: 1 addition & 1 deletion resources/views/pages/parts/template-manager.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<div class="search-box flexible mb-m" style="display: {{ count($templates) > 0 ? 'block' : 'none' }}">
<input refs="template-manager@searchInput" type="text" name="template-search" placeholder="{{ trans('common.search') }}">
<button refs="template-manager@searchButton" tabindex="-1" type="button">@icon('search')</button>
<button refs="template-manager@searchCancel" class="search-box-cancel text-neg" type="button" style="display: none">@icon('close')</button>
<button refs="template-manager@searchCancel" class="search-box-cancel text-neg" tabindex="-1" type="button" style="display: none">@icon('close')</button>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd generally advise slipping small fixes into larger works otherwise they get blocked by the larger feature and/or conflict if the fix gets applied in the meantime.

</div>

<div refs="template-manager@list">
Expand Down