WIP: Default page templates by lennertdaniels · Pull Request #3918 · BookStackApp/BookStack

lennertdaniels · 2022-12-15T10:37:04Z

I took a stab at implementing default page templates as indicated in #1803.

What I have implemented:

There's a book-level setting to pick a default template
A new page automatically copies the template's html on creation
A new guests page also has the template applied to it
Deleted pages don't work as a template anymore
When deleting a page that is used as a default template somewhere, there's a warning on the delete confirmation screen

What I haven't fully considered or implemented yet:

Any permission/scope problems
If there's a difference in handling HTML vs. markdown templates
A better selector to pick the default template (pagination, AJAX search) like the one in the book.edit sidebar (template-manager)
Should other entities also support a default template? (User, Shelf, Chapter) If so, which one has priority when multiple default templates are chosen on different levels?
Applying a template from a URL parameter (@cod3monk and Link to Template Draft #3794)

Random small fixes I came across:

Autofocus on the name field on guest-create.blade
Template Manager: fixed cancel icon placement

If anyone with more thorough knowledge of the codebase would like to shine some light onto this PR, that would be great.

lennertdaniels · 2023-01-14T18:55:56Z

@ssddanbrown Would you mind taking a look if you find the time? Thanks.

ssddanbrown · 2023-01-14T19:18:36Z

Hi @lennertdaniels,
Sorry for my lack of response so far, I plan to review pending PRs after finishing up the next release.
I remember doing a quick scan across the PR code when opened, and from what I can remember it was all looking really good, with maybe just needing some thought into ensuring the selection is scalable/efficient. Of course I need to look into this to properly review your code and considerations.
The one bit of feedback I will provide now is to not worry about any optional sub-features at this time (query param, other entities). I prefer to keep implementation/scope narrow and only expand on proven requirement/demand, to keep the platform surface area reasonable.

Should review towards the end of the month unless things go further wrong (Current release cycle is being a challenge).

lennertdaniels · 2023-01-15T17:05:26Z

@ssddanbrown absolutely fine, take your time. I just wasn't sure wether I had to add some tag/reviewer/assignee or something to this PR. :)

I'll take your quick feedback into consideration and work on it a little more over the coming weeks.

ssddanbrown

Thanks for this @lennertdaniels, All was clear and seemed to function well.
Just raised my thoughts/findings from a deeper dive into the code.
Feel free to query or ask for clarification/assistance/guidance for any of my notes.

Ideally we'd have testing to cover the functionality additions here also, at least to cover the main functionality. Happy to help or advise on that inf you're unfamiliar with testing.

ssddanbrown · 2023-02-08T11:03:16Z

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


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).

ssddanbrown · 2023-02-08T11:05:54Z

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


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.

ssddanbrown · 2023-02-08T11:14:07Z

+        $templates = Page::visible()
+            ->where('template', '=', true)
+            ->orderBy('name', 'asc')
+            ->get();


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.

ssddanbrown · 2023-02-08T11:18:50Z

            'description' => ['string', 'max:1000'],
            'image'       => array_merge(['nullable'], $this->getImageValidationRules()),
            'tags'        => ['array'],
+            'default_template'  => ['nullable', 'exists:pages,id'], 


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.

ssddanbrown · 2023-02-08T11:20:12Z

+        $templates = Page::visible()
+            ->where('template', '=', true)
+            ->orderBy('name', 'asc')
+            ->get();


As per create method comment.

ssddanbrown · 2023-02-08T11:33:24Z

        $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();


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

ssddanbrown · 2023-02-08T11:39:16Z

    '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.',


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.

ssddanbrown · 2023-02-08T11:40:22Z

    '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.",


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.

ssddanbrown · 2023-02-08T11:42:57Z

@@ -0,0 +1,10 @@
+<p class="text-muted small">
+    {!! nl2br(e(trans('entities.default_template_explain'))) !!}


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.

ssddanbrown · 2023-02-08T11:46:53Z

        <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>


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.

ssddanbrown · 2023-04-06T17:44:57Z

Hi @lennertdaniels,
Would you be looking to address the review points above yourself? Or do you need a certain level of support/advise? Or would you prefer for me to carry this on from this point?

lennertdaniels · 2023-04-18T09:04:43Z

Hi @ssddanbrown,

I'm slowly chipping away at your remarks whenever I find some spare time :) Will update the PR in due time.
The feedback was very useful, thanks!

melat0nin · 2023-07-04T09:32:07Z

Hi @lennertdaniels, have you managed to make any progress on this PR? It's a feature I'd really love to have in BookStack :)

ssddanbrown · 2023-10-30T16:49:02Z

Hi @lennertdaniels,
Would you be okay for me to take this on and finish things off from this point?

lennertdaniels · 2023-10-30T21:51:53Z

@ssddanbrown please do!
I wasn't able to dedicate a lot of time to this PR, sorry. I'm excited to see this feature come alive :)

ssddanbrown · 2023-10-30T23:13:06Z

Alright, I'll assign this to take on for the next release.
Please keep your fork & branch, and this PR up and active in the meantime until this is merged.

I wasn't able to dedicate a lot of time to this PR, sorry.

No reason to be sorry, thanks for contributing and getting this started!

ssddanbrown · 2023-12-12T16:07:26Z

Okay, now all merged with follow-up work continued in #4721.
This will be part of the next feature release.

Thanks again for getting this started @lennertdaniels!

lennertdaniels added 5 commits December 2, 2022 13:10

search-box-cancel placement

3599a96

Add default_template as Book setting

1dbc358

Prefill new pages with book's default template

99ae759

Add notice to Page delete confirmation when in use as a template

ec3b06d

Guest create page: name field autofocus

ac519b3

ssddanbrown requested changes Feb 8, 2023

View reviewed changes

ssddanbrown added this to the Next Feature Release milestone Oct 30, 2023

ssddanbrown self-assigned this Oct 30, 2023

ssddanbrown mentioned this pull request Dec 11, 2023

Continued: Default book templates #4721

Merged

14 tasks

ssddanbrown merged commit 968bc8c into BookStackApp:development Dec 12, 2023

ssddanbrown mentioned this pull request Dec 12, 2023

Default page templates #1803

Closed

cod3monk mentioned this pull request Jan 11, 2024

Extend Default Pagetemplate to Chapters #4764

Closed

1 task

		@@ -0,0 +1,10 @@
		<p class="text-muted small">
		{!! nl2br(e(trans('entities.default_template_explain'))) !!}

Uh oh!

Uh oh!

Conversation

lennertdaniels commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I have implemented:

What I haven't fully considered or implemented yet:

Random small fixes I came across:

Uh oh!

lennertdaniels commented Jan 14, 2023

Uh oh!

ssddanbrown commented Jan 14, 2023

Uh oh!

lennertdaniels commented Jan 15, 2023

Uh oh!

ssddanbrown left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssddanbrown commented Apr 6, 2023

Uh oh!

lennertdaniels commented Apr 18, 2023

Uh oh!

melat0nin commented Jul 4, 2023

Uh oh!

ssddanbrown commented Oct 30, 2023

Uh oh!

lennertdaniels commented Oct 30, 2023

Uh oh!

ssddanbrown commented Oct 30, 2023

Uh oh!

ssddanbrown commented Dec 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

lennertdaniels commented Dec 15, 2022 •

edited

Loading