-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
WIP: Default page templates #3918
WIP: Default page templates #3918
Conversation
@ssddanbrown Would you mind taking a look if you find the time? Thanks. |
Hi @lennertdaniels, Should review towards the end of the month unless things go further wrong (Current release cycle is being a challenge). |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if ($page->book->defaultTemplate) { | ||
$page->forceFill([ | ||
'html' => $page->book->defaultTemplate->html, | ||
]); | ||
} | ||
|
There was a problem hiding this comment.
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).
if ($page->book->defaultTemplate) { | ||
$page->forceFill([ | ||
'html' => $page->book->defaultTemplate->html, | ||
]); | ||
} | ||
|
There was a problem hiding this comment.
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.
$templates = Page::visible() | ||
->where('template', '=', true) | ||
->orderBy('name', 'asc') | ||
->get(); |
There was a problem hiding this comment.
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.
@@ -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'], |
There was a problem hiding this comment.
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.
$templates = Page::visible() | ||
->where('template', '=', true) | ||
->orderBy('name', 'asc') | ||
->get(); |
There was a problem hiding this comment.
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.
@@ -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(); |
There was a problem hiding this comment.
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).
@@ -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.', |
There was a problem hiding this comment.
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.
@@ -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.", |
There was a problem hiding this comment.
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.
@@ -0,0 +1,10 @@ | |||
<p class="text-muted small"> | |||
{!! nl2br(e(trans('entities.default_template_explain'))) !!} |
There was a problem hiding this comment.
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.
@@ -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> |
There was a problem hiding this comment.
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.
Hi @lennertdaniels, |
Hi @ssddanbrown, I'm slowly chipping away at your remarks whenever I find some spare time :) Will update the PR in due time. |
Hi @lennertdaniels, have you managed to make any progress on this PR? It's a feature I'd really love to have in BookStack :) |
Hi @lennertdaniels, |
@ssddanbrown please do! |
Alright, I'll assign this to take on for the next release.
No reason to be sorry, thanks for contributing and getting this started! |
Okay, now all merged with follow-up work continued in #4721. Thanks again for getting this started @lennertdaniels! |
I took a stab at implementing default page templates as indicated in #1803.
What I have implemented:
What I haven't fully considered or implemented yet:
Random small fixes I came across:
If anyone with more thorough knowledge of the codebase would like to shine some light onto this PR, that would be great.