Add support for parent and child page restrictions in page_layout definition #1776

mickenorlen · 2020-04-05T13:54:59Z

Update 8/7

What is this pull request for?

Add support for restricting parent/child_pages:

Update 17/5

Renamed sub_pages to child_pages and added include/exclude as suggested by @mamhoff.

# config/alchemy/page_layouts.yml

- name: blog
  child_pages: 
     include: [blog_post] # array of allowed child_pages.

- name: blog_post
  parent_pages: 
     include: [blog] # array of parent pages under which page can be created
  child_pages: false # hides add child_page button from page tree

- name: no_blogs_pages # Excludes example
  child_pages:
    exclude: [blog] 
  parent_pages:
    exclude: [blog]

Note

If child_pages = false it will entirely hide the create child_page button (+) in page_tree. See screenshot 1.
if selectable_layouts.length == 1 i added autoselect and removed blank option in create selection. See screenshot 2

Changes in the code

Passed in the page_layout of the parent_page to PageLayout.selectable_layouts to be able to check the definitions.
Added allowing_child_pages: page.definition['child_pages'] != false to the page_tree_serializer so i could hide the create (+) button.

Screenshots

No (+) button if child_pages = false
No blank/"Please choose" option when only 1 to choose.

Tests

I wanted to check with you if the overall implementation looks good before fixing the tests. Currently 2 fails saying undefined method page_layout for nil:NilClass

rspec ./spec/features/admin/page_creation_feature_spec.rb:21 # Page creation overlay GUI when having a Page in the clipboard contains tabs for creating a new page and pasting from clipboard
rspec ./spec/requests/alchemy/admin/pages_controller_spec.rb:261 # Alchemy::Admin::PagesController with logged in editor user #new with current language present pages in clipboard should load all pages from clipboard

Checklist

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have added tests to cover this change

mamhoff

I have a few implementation comments, and I would like to suggest to only implement ONE of sub_pages or parent_pages. If a page layout can only have certain subpages, then that's all you should ever need, am I right?

app/serializers/alchemy/page_tree_serializer.rb

mamhoff · 2020-04-06T07:45:05Z

app/views/alchemy/admin/pages/_new_page_form.html.erb

    required: true,
+    selected: @page_layouts.length == 1 ? @page_layouts.first : nil,


page_layouts.first.presence instead would do the trick

nice catch will fix next round

Actually I don't think i can change this as i select first only if length == 1 - Not if first is present

lib/alchemy/page_layout.rb

codeclimate · 2020-04-29T12:16:24Z

lib/alchemy/page_layout.rb

      #
-      def selectable_layouts(language_id, only_layoutpages = false)
+      def selectable_layouts(language_id, only_layoutpages = false, page_layout_name = nil)


Method selectable_layouts has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

tvdeyen

Thanks.

I like the overall idea of this feature a lot. But the implementation should be much easier. I do not see a need for child pages being able to define parent layouts. This does not make any sense. Children can not exist without a parent.

So in my thoughts something like

- name: blog
  sub_pages:
    only: post

and

- name: standard
  sub_pages:
    except:
      - post
      - blog

is a great API we would except.

The selectable layouts methods are worth refactoring so that its easier to understand what is going on. In the end it should be easy as:

All defined page layouts for that site - already taken unique ones - not allowed layouts = selectable layouts for the new page

And we need tests of course.

Again, thanks for that feature. I like it.

app/models/alchemy/page.rb

app/serializers/alchemy/page_tree_serializer.rb

app/views/alchemy/admin/pages/_page.html.erb

lib/alchemy/page_layout.rb

tvdeyen · 2020-06-11T07:51:38Z

lib/alchemy/page_layout.rb

+          (excludes && !excludes.include?(layout['name']))
+      end
+
+      def layout_available_from_child?(layout)


I don't understand why we would need that. We are never able to create a parent for an existing child. That's not possible. Can you elaborate what this is for?

It's not about creating a parent for a child. But under which parents the layout will be available. Previously there was a setting hide = true which made certain layouts hidden from creation. I just deleted this hide setting from the conditions in my last push as this can be achived with `layout['parent_pages'] = false.

But the reason why I think this feature makes sense to have is the following. Imagine that you have a mature website with some 15 page layouts available. Then you want a new type of blog post layout special_blog_post that should only be creatable under the blog layout.

With the new feature you simply set parent_pages['only'] = ['blog'] in the special_blog_post settings.

Without it you would have to revisit all other page_layout and append `child_pages['except'] = [ ... 'special_blog_post']

... Which feels a lot more tedious/error prone. In the end this logic is only executed on admin page creation, and we're already looping through all layouts to check the rest of the conditions - so i dont think it could have any significant impact on performance?

What do you think?

tvdeyen · 2020-06-11T07:53:15Z

lib/alchemy/page_layout.rb

      #
-      def selectable_layouts(language_id, only_layoutpages = false)
+      def selectable_layouts(language_id, only_layoutpages = false, page_layout_name = nil)


I agree this method is overly complex and combined with the other methods its worth refactoring. I think this can be implemented much easier.

I'm not sure I can simplify this. I added more comments to the code to clear up any misunderstanding.

Also - I chose to call it parent_layout_name instead of parent_page_layout to make it clear that it is a name (String) and not a layout (Object/hash) from definition we're expecting - As has been done in a bunch of other places there

tvdeyen · 2020-06-11T07:54:39Z

lib/alchemy/page_layout.rb

        @language_id = language_id
+        @page_layout = page_layout_name.present? ? all.detect{ |layout| layout['name'] == page_layout_name } : nil


Why is this necessary in here? We not filter the available page layouts with select reject instead?

This gets the layout of the parent page (Object/hash from definition where we can check for child_page restrictions) from the parent layout name (String).

Then just below we filter with select and all the conditions.

mickenorlen · 2020-06-29T13:08:51Z

@tvdeyen Thanks for your comments. I updated most of the things you suggested and added tests.

Some questions/thoughts:

See code comments where i explain/argue for the parent_pages setting
I see you prefer the term sub_pages. But IF you agree the parent_pages setting is something to keep in the end - I think parent_pages and child_pages are more naturally related terms. What do you think?

The selectable layouts methods are worth refactoring so that its easier to understand what is going on. In the end it should be easy as:

All defined page layouts for that site - already taken unique ones - not allowed layouts = selectable layouts for the new page

This seems kind of like what we have?

In method selectable_layouts - This code is from before and I'm guessing it is still relevant? Basically we select filter through all the available layouts with a condition to check if its a layoutpage or not...

  all.select do |layout|
    if only_layoutpages
      layout['layoutpage'] && layout_available?(layout)
    else
      !layout['layoutpage'] && layout_available?(layout)
    end
  end

Then below we have the conditions tested for each layout in the select filter. I only added the first two (and removed the old redundant hide see code comments about parent_pages setting).

  def layout_available?(layout)
    layout_available_from_parent?(layout) && layout_available_from_child?(layout) &&
      !already_taken?(layout) && available_on_site?(layout)
  end

Which equates to: (?)

All defined page layouts for that site - already taken unique ones - not allowed layouts = selectable layouts for the new page

So I'm not sure how you want to simplify this.

codeclimate · 2020-07-07T14:25:09Z

lib/alchemy/page_layout.rb

      #
-      def selectable_layouts(language_id, only_layoutpages = false)
+      def selectable_layouts(language_id, only_layoutpages = false, parent_layout_name = nil)


Method selectable_layouts has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.

tvdeyen · 2020-08-26T11:51:11Z

Could you please extract the "No please choose if only one layout available" feature into a dedicated PR?

tvdeyen

I looked at this again today and although I think this is a nice way to structure pages I still think that the page layout code is getting more and more complex. Since I need to maintain this going forward I need more time to get convinced. Maybe refactoring the code into smaller understandable classes and methods would help. But in the current form I tend to not accept this although I like that feature.

tvdeyen · 2020-09-30T07:04:47Z

app/controllers/alchemy/admin/pages_controller.rb

@@ -57,9 +57,9 @@ def info

      def new
        @page ||= Page.new(layoutpage: params[:layoutpage] == "true", parent_id: params[:parent_id])
-        @page_layouts = PageLayout.layouts_for_select(@current_language.id, @page.layoutpage?)
+        @page_layouts = PageLayout.layouts_for_select(@current_language.id, @page.layoutpage?, @page.parent &.page_layout)


Shouldn't this be?

Suggested change

@page_layouts = PageLayout.layouts_for_select(@current_language.id, @page.layoutpage?, @page.parent &.page_layout)

@page_layouts = PageLayout.layouts_for_select(@current_language.id, @page.layoutpage?, @page.parent&.page_layout)

A test would have could this. Could you please add one?

app/controllers/alchemy/admin/pages_controller.rb

tvdeyen · 2020-09-30T07:07:24Z

app/serializers/alchemy/page_tree_serializer.rb

@@ -55,6 +55,7 @@ def page_hash(page, has_children, level, folded)
        public: page.public?,
        restricted: page.restricted?,
        page_layout: page.page_layout,
+        child_pages_allowed: page.definition["child_pages"] != false,


I do not like the name of this attribute

allowed_page_types, or maybe nestable_pages nestable_child_pages?

Now i named it for what it actually is show_create_page_btn. Which can be true even if there are no pages available, as the button is only hidden if definition['child_pages'] == false.

Also added tests for controller method calls. I've been using this feature for quite some time already, [space] &. works just as fine as [no_space].& - but i still removed the space as you like.

I think the implementation is good now and for me the feature is essential for controlling the page tree structure. It is also just as you requested it to be as explained in this comment #1776 (comment)

I would say it cannot be simplified more. You have to give me more concrete feedback if you still disagree.

tvdeyen · 2020-10-26T19:48:21Z

@mickenorlen we switched the default branch to main. I already updated this PR, but you want to pull the main branch and rebase this branch against this as well, so you get future updates. sorry for any inconvenience and thanks for the contribution.

…inition child_pages lists which pages can be created under given page layout. parent_pages lists under which page layouts given page can be created. Both will determine which page layouts are available for selection. Set by: child/parent_pages: {include/exclude: [layout_name, ...]

mickenorlen · 2020-10-29T13:16:01Z

@tvdeyen Force pushed a fresh single squash merge commit into main as rebase was a mess.

github-actions · 2022-03-21T01:39:54Z

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
This PR will be closed in 7 days if no further activity happens.

github-actions · 2022-03-28T01:47:38Z

This pull request has not seen any activiy in a long time.
Probably because of missing tests or a necessary rebase.
Please open a new PR to latest main if you want to continue working on this.
Thanks for the contribution.

mickenorlen force-pushed the parent-and-sub-page-restrictions branch 3 times, most recently from af1fc18 to 8209294 Compare April 5, 2020 14:27

mamhoff reviewed Apr 6, 2020

View reviewed changes

mickenorlen mentioned this pull request Apr 9, 2020

Features for new blog guide: tag selection, restricted sub_pages, min img dim, content editor selectors, content editor loaded event #1764

Closed

mickenorlen force-pushed the parent-and-sub-page-restrictions branch from 8209294 to 1cf16b7 Compare April 29, 2020 12:15

codeclimate bot reviewed Apr 29, 2020

View reviewed changes

mickenorlen force-pushed the parent-and-sub-page-restrictions branch from 1cf16b7 to 2494f31 Compare May 17, 2020 12:45

mickenorlen changed the title ~~Add support for parent and sub page restrictions in page_layout definition~~ Add support for parent and child page restrictions in page_layout definition May 17, 2020

mickenorlen force-pushed the parent-and-sub-page-restrictions branch 2 times, most recently from 03bdcef to f6bd940 Compare May 17, 2020 13:04

tvdeyen reviewed Jun 11, 2020

View reviewed changes

tvdeyen added confirmed needs tests labels Jun 11, 2020

mickenorlen force-pushed the parent-and-sub-page-restrictions branch from ac9f4d8 to 984a619 Compare June 29, 2020 11:29

mickenorlen requested a review from tvdeyen June 29, 2020 13:19

codeclimate bot reviewed Jul 7, 2020

View reviewed changes

mickenorlen force-pushed the parent-and-sub-page-restrictions branch from 018ff48 to 8e7d2d7 Compare July 8, 2020 08:16

mickenorlen mentioned this pull request Sep 7, 2020

Autoselect first if only one layout available #1932

Merged

tvdeyen reviewed Sep 30, 2020

View reviewed changes

tvdeyen added discussion needed and removed confirmed labels Sep 30, 2020

mickenorlen requested a review from tvdeyen October 14, 2020 09:26

tvdeyen changed the base branch from master to main October 26, 2020 19:48

mickenorlen force-pushed the parent-and-sub-page-restrictions branch from 5aea006 to e1fe6ff Compare October 29, 2020 13:11

github-actions bot added the Stale label Mar 21, 2022

github-actions bot closed this Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parent and child page restrictions in page_layout definition #1776

Add support for parent and child page restrictions in page_layout definition #1776

mickenorlen commented Apr 5, 2020 •

edited

Loading

mamhoff left a comment

mamhoff Apr 6, 2020

mickenorlen Apr 6, 2020

mickenorlen Jun 29, 2020

codeclimate bot Apr 29, 2020

tvdeyen left a comment

tvdeyen Jun 11, 2020

mickenorlen Jun 29, 2020

tvdeyen Jun 11, 2020

mickenorlen Jun 29, 2020 •

edited

Loading

tvdeyen Jun 11, 2020

mickenorlen Jun 29, 2020 •

edited

Loading

mickenorlen commented Jun 29, 2020 •

edited

Loading

codeclimate bot Jul 7, 2020

tvdeyen commented Aug 26, 2020

tvdeyen left a comment

tvdeyen Sep 30, 2020

tvdeyen Sep 30, 2020

mickenorlen Oct 14, 2020

tvdeyen commented Oct 26, 2020

mickenorlen commented Oct 29, 2020

github-actions bot commented Mar 21, 2022

github-actions bot commented Mar 28, 2022

		required: true,
		selected: @page_layouts.length == 1 ? @page_layouts.first : nil,

		@language_id = language_id
		@page_layout = page_layout_name.present? ? all.detect{ \|layout\| layout['name'] == page_layout_name } : nil

	@page_layouts = PageLayout.layouts_for_select(@current_language.id, @page.layoutpage?, @page.parent &.page_layout)
	@page_layouts = PageLayout.layouts_for_select(@current_language.id, @page.layoutpage?, @page.parent&.page_layout)

Add support for parent and child page restrictions in page_layout definition #1776

Add support for parent and child page restrictions in page_layout definition #1776

Conversation

mickenorlen commented Apr 5, 2020 • edited Loading

Update 8/7

What is this pull request for?

Update 17/5

Note

Changes in the code

Screenshots

Tests

Checklist

mamhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Apr 29, 2020

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickenorlen Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickenorlen Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

mickenorlen commented Jun 29, 2020 • edited Loading

codeclimate bot Jul 7, 2020

Choose a reason for hiding this comment

tvdeyen commented Aug 26, 2020

tvdeyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen commented Oct 26, 2020

mickenorlen commented Oct 29, 2020

github-actions bot commented Mar 21, 2022

github-actions bot commented Mar 28, 2022

mickenorlen commented Apr 5, 2020 •

edited

Loading

mickenorlen Jun 29, 2020 •

edited

Loading

mickenorlen Jun 29, 2020 •

edited

Loading

mickenorlen commented Jun 29, 2020 •

edited

Loading