Merge pull request AlchemyCMS#1868 from tvdeyen/remove-attach-to-menu

Remove Page#visible

app/assets/stylesheets/alchemy/sitemap.scss

@@ -197,6 +197,10 @@ $sitemap-url-xlarge-width: 350px;
 
 .page_status {
   display: inline-block;
+
+  .alchemy-dialog & {
+    display: block;
+  }
 }
 
 #sitemap_heading {
@@ -226,7 +230,7 @@ $sitemap-url-xlarge-width: 350px;
 
   .page_status {
     padding-left: 2 * $default-padding;
-    margin-right: 214px;
+    margin-right: 188px;
     margin-left: auto;
 
     @media screen and (min-width: $large-screen-break-point) {

app/controllers/alchemy/admin/pages_controller.rb

@@ -291,24 +291,14 @@ def create_tree(items, rootpage)
       # This function will add a node's own slug into their ancestor's path
       # in order to create the full URL of a node
       #
-      # NOTE: Invisible pages are not part of the full path of their children
-      #
       # @param [String]
       #   The node's ancestors path
       # @param [Hash]
       #   A children node
       #
       def process_url(ancestors_path, item)
         default_urlname = (ancestors_path.blank? ? "" : "#{ancestors_path}/") + item["slug"].to_s
-
-        pair = { my_urlname: default_urlname, children_path: default_urlname }
-
-        if item["visible"] == false
-          # children ignore an ancestor in their path if invisible
-          pair[:children_path] = ancestors_path
-        end
-
-        pair
+        { my_urlname: default_urlname, children_path: default_urlname }
       end
 
       def load_page

app/models/alchemy/page.rb

@@ -17,7 +17,6 @@
 #  rgt              :integer
 #  parent_id        :integer
 #  depth            :integer
-#  visible          :boolean          default(FALSE)
 #  locked_by        :integer
 #  restricted       :boolean          default(FALSE)
 #  robot_index      :boolean          default(TRUE)
@@ -44,7 +43,6 @@ class Page < BaseRecord
 
     DEFAULT_ATTRIBUTES_FOR_COPY = {
       autogenerate_elements: false,
-      visible: false,
       public_on: nil,
       public_until: nil,
       locked_at: nil,
@@ -78,7 +76,6 @@ class Page < BaseRecord
       :tag_list,
       :title,
       :urlname,
-      :visible,
       :layoutpage,
       :menu_id,
     ]
@@ -138,9 +135,6 @@ class Page < BaseRecord
     after_update :create_legacy_url,
       if: :saved_change_to_urlname?
 
-    after_update :attach_to_menu!,
-      if: :should_attach_to_menu?
-
     after_update -> { nodes.update_all(updated_at: Time.current) }
 
     # Concerns
@@ -152,8 +146,6 @@ class Page < BaseRecord
     # site_name accessor
     delegate :name, to: :site, prefix: true, allow_nil: true
 
-    attr_accessor :menu_id
-
     # Class methods
     #
     class << self
@@ -545,18 +537,5 @@ def create_legacy_url
     def set_published_at
       self.published_at = Time.current
     end
-
-    def attach_to_menu!
-      node = Alchemy::Node.find_by!(id: menu_id, language_id: language_id)
-      node.children.create!(
-        language_id: language_id,
-        page_id: id,
-        name: name,
-      )
-    end
-
-    def should_attach_to_menu?
-      menu_id.present? && nodes.none?
-    end
   end
 end

app/models/alchemy/page/fixed_attributes.rb

@@ -15,7 +15,6 @@ module Alchemy
   #       fixed_attributes:
   #         - public_on: nil
   #         - public_until: nil
-  #         - visible: false
   #
   class Page::FixedAttributes
     attr_reader :page

app/models/alchemy/page/page_naming.rb

@@ -22,7 +22,7 @@ module Page::PageNaming
         if: -> { title.blank? }
 
       after_update :update_descendants_urlnames,
-        if: :should_update_descendants_urlnames?
+        if: :saved_change_to_urlname?
 
       after_move :update_urlname!
     end
@@ -35,7 +35,7 @@ def renamed?
     # Makes a slug of all ancestors urlnames including mine and delimit them be slash.
     # So the whole path is stored as urlname in the database.
     def update_urlname!
-      new_urlname = nested_url_name(slug)
+      new_urlname = nested_url_name
       if urlname != new_urlname
         legacy_urls.create(urlname: urlname)
         update_column(:urlname, new_urlname)
@@ -47,25 +47,8 @@ def slug
       urlname.to_s.split("/").last
     end
 
-    # Returns an array of visible/non-language_root ancestors.
-    def visible_ancestors
-      return [] unless parent
-
-      if new_record?
-        parent.visible_ancestors.tap do |base|
-          base.push(parent) if parent.visible?
-        end
-      else
-        ancestors.visible.contentpages.where(language_root: nil).to_a
-      end
-    end
-
     private
 
-    def should_update_descendants_urlnames?
-      saved_change_to_urlname? || saved_change_to_visible?
-    end
-
     def update_descendants_urlnames
       reload
       descendants.each(&:update_urlname!)
@@ -75,7 +58,7 @@ def update_descendants_urlnames
     # Either from name, or if present, from urlname.
     # The urlname contains the whole path including parent urlnames.
     def set_urlname
-      self[:urlname] = nested_url_name(slug)
+      self[:urlname] = nested_url_name
     end
 
     def set_title
@@ -87,26 +70,17 @@ def set_title
     # Names shorter than 3 will be filled up with dashes,
     # so it does not collidate with the language code.
     #
-    def convert_url_name(value)
-      url_name = convert_to_urlname(value.blank? ? name : value)
-      if url_name.length < 3
-        ("-" * (3 - url_name.length)) + url_name
-      else
-        url_name
-      end
-    end
-
-    def nested_url_name(value)
-      (ancestor_slugs << convert_url_name(value)).join("/")
+    def converted_url_name
+      url_name = convert_to_urlname(slug.blank? ? name : slug)
+      url_name.rjust(3, "-")
     end
 
-    # Slugs of all visible/non-language_root ancestors.
-    # Returns [], if there is no parent, the parent is
-    # the root page itself.
-    def ancestor_slugs
-      return [] if parent.nil?
-
-      visible_ancestors.map(&:slug).compact
+    def nested_url_name
+      if parent&.language_root?
+        converted_url_name
+      else
+        [parent&.urlname, converted_url_name].compact.join("/")
+      end
     end
   end
 end

app/models/alchemy/page/page_natures.rb

@@ -61,7 +61,6 @@ def locked?
     def status
       {
         public: public?,
-        visible: visible?,
         locked: locked?,
         restricted: restricted?,
       }

app/models/alchemy/page/page_scopes.rb

@@ -32,10 +32,6 @@ module Page::PageScopes
       #
       scope :not_locked, -> { where(locked_at: nil, locked_by: nil) }
 
-      # All visible pages
-      #
-      scope :visible, -> { where(visible: true) }
-
       # All not restricted pages
       #
       scope :not_restricted, -> { where(restricted: false) }

app/serializers/alchemy/page_tree_serializer.rb

@@ -53,7 +53,6 @@ def page_hash(page, has_children, level, folded)
         id: page.id,
         name: page.name,
         public: page.public?,
-        visible: page.visible?,
         restricted: page.restricted?,
         page_layout: page.page_layout,
         slug: page.slug,
@@ -105,7 +104,6 @@ def page_permissions(page, ability)
     def page_status_titles(page)
       {
         public: page.status_title(:public),
-        visible: page.status_title(:visible),
         restricted: page.status_title(:restricted),
       }
     end

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

@@ -10,7 +10,6 @@
     <div class="control_group">
       <%= render 'alchemy/admin/pages/publication_fields' %>
       <%= page_status_checkbox(@page, :restricted) %>
-      <%= render 'alchemy/admin/pages/menu_fields', f: f %>
       <% if configuration(:sitemap)['show_flag'] %>
         <%= page_status_checkbox(@page, :sitemap) %>
       <% end %>

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

@@ -1,4 +1,4 @@
-<li id="page_{{id}}" class="page_level_{{level}} {{page_layout}}" data-slug="{{slug}}" data-restricted="{{restricted}}" data-visible="{{visible}}">
+<li id="page_{{id}}" class="page_level_{{level}} {{page_layout}}" data-slug="{{slug}}" data-restricted="{{restricted}}">
   <div class="sitemap_page{{#if locked}} locked{{/if}}" name="{{name}}">
     <div class="sitemap_left_images<% if @sorting %>{{#unless root}} handle{{/unless}}<% end %>">
       <% unless @sorting %>
@@ -150,10 +150,6 @@
         <i class="icon fas fa-fw fa-compass {{#unless public}}disabled{{/unless}}" data-fa-transform="shrink-2"></i>
         <span class="hint-bubble">{{status_titles.public}}</span>
       </span>
-      <span class="page_status with-hint">
-        <i class="icon fas fa-fw fa-eye {{#unless visible}}disabled{{/unless}}" data-fa-transform="shrink-2"></i>
-        <span class="hint-bubble">{{status_titles.visible}}</span>
-      </span>
       <span class="page_status with-hint">
         <i class="icon fas fa-fw fa-lock {{#unless restricted}}disabled{{/unless}}" data-fa-transform="shrink-2"></i>
         <span class="hint-bubble">{{status_titles.restricted}}</span>

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

@@ -2,10 +2,6 @@
   <%= render_icon(:compass, transform: 'shrink-2', class: @page.public? ? nil : 'disabled') %>
   <span class="hint-bubble"><%= page.status_title(:public) %></span>
 </span>
-<span class="page_status with-hint">
-  <%= render_icon(:eye, transform: 'shrink-2', class: @page.visible? ? nil : 'disabled') %>
-  <span class="hint-bubble"><%= page.status_title(:visible) %></span>
-</span>
 <span class="page_status with-hint">
   <%= render_icon(:lock, transform: 'shrink-2', class: @page.restricted? ? nil : 'disabled') %>
   <span class="hint-bubble"><%= page.status_title(:restricted) %></span>

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

@@ -24,10 +24,6 @@
         <%= render_icon(:compass, transform: 'shrink-2', class: @page.public? ? nil : 'disabled') %>
         <%= @page.status_title(:public) %>
       </span>
-      <span class="page_status">
-        <%= render_icon(:eye, transform: 'shrink-2', class: @page.visible? ? nil : 'disabled') %>
-        <%= @page.status_title(:visible) %>
-      </span>
       <span class="page_status">
         <%= render_icon(:lock, transform: 'shrink-2', class: @page.restricted? ? nil : 'disabled') %>
         <%= @page.status_title(:restricted) %>

config/alchemy/config.yml

@@ -11,7 +11,7 @@ auto_logout_time: 30
 
 # === Redirect Options
 #
-#   redirect_to_public_child  [Boolean]   # Alchemy redirects to the first public child page found, if a page is not visible.
+#   redirect_to_public_child  [Boolean]   # Alchemy redirects to the first public child page found, if a page is not public.
 #
 redirect_to_public_child: true
 

config/locales/alchemy.en.yml

@@ -481,9 +481,6 @@ en:
     page_published: "Published page"
     page_restricted: "restricted"
     page_states:
-      visible:
-        "true": "Page is visible in navigation."
-        "false": "Page is not visible in navigation."
       public:
         "true": "Page is published."
         "false": "Page is unpublished."
@@ -578,7 +575,6 @@ en:
         button_label: Upload image(s)
         upload_success: "Picture %{name} uploaded successfully"
         upload_failure: "Error while uploading %{name}: %{error}"
-    visible: "visible"
     want_to_create_new_language: "Do you want to create a new empty language tree?"
     want_to_make_copy_of_existing_language: "Do you want to copy an existing language tree?"
     "We need at least one default.": "A default language must exist."
@@ -826,7 +822,6 @@ en:
         updated_at: "Updated at"
         urlname: "URL-Path"
         slug: "Slug"
-        visible: "visible in navigation"
       alchemy/picture:
         image_file_name: "Filename"
         image_file_height: "Height"

db/migrate/20200519073500_remove_visible_from_alchemy_pages.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+class RemoveVisibleFromAlchemyPages < ActiveRecord::Migration[5.2]
+  class LocalPage < ActiveRecord::Base
+    self.table_name = "alchemy_pages"
+
+    scope :invisible, -> { where(visible: [false, nil]) }
+    scope :contentpages, -> { where(layoutpage: [false, nil]) }
+  end
+
+  def up
+    if LocalPage.invisible.contentpages.where.not(parent_id: nil).any?
+      abort "You have invisible pages in your database! " \
+            "Please re-structure your page tree before running this migration. " \
+            "You might also downgrade to Alchemy 4.6 and " \
+            "run the `alchemy:upgrade:4.6:restructure_page_tree` rake task."
+    end
+
+    remove_column :alchemy_pages, :visible
+  end
+
+  def down
+    add_column :alchemy_pages, :visible, :boolean, default: false
+  end
+end

lib/alchemy/permissions.rb

@@ -36,7 +36,7 @@ def initialize(user)
     module GuestUser
       def alchemy_guest_user_rules
         can([:show, :download], Alchemy::Attachment) { |a| !a.restricted? }
-        can :see,               Alchemy::Page,       restricted: false, visible: true
+        can :see, Alchemy::Page, restricted: false
 
         can :read, Alchemy::Content, Alchemy::Content.available.not_restricted do |c|
           c.public? && !c.restricted? && !c.trashed?
@@ -64,7 +64,7 @@ def alchemy_member_rules
 
         # Resources
         can [:show, :download], Alchemy::Attachment
-        can :see,               Alchemy::Page, restricted: true, visible: true
+        can :see, Alchemy::Page, restricted: true
 
         can :read, Alchemy::Content, Alchemy::Content.available do |c|
           c.public? && !c.trashed?