Replaced menu partials with Refinery::Pages::MenuPresenter.

This has many advantages not least of which is that we can test it without requesting pages.

* Using config_accessor and attr_accessor instead of passing options around.
* Added leaf? to MenuItem and made use of has_children? in the MenuPresenter
* All methods besides initialize and to_html are private to this class.
* Moved Refinery::Core#menu_css to the Refinery::Pages::MenuPresenter.
* Uses MenuItem#depth instead of scanning ancestors.
* Renamed levels to max_depth and added within_max_depth?
* Refactored Menu and MenuItem as proper objects, not pretending to be hashes or arrays (ht: @awagener)
* Move Menu#initialize logic to Menu#append so that more items can be appended after initialize.
* Moved all page presenters and their specs under app/presenters and spec/presenters where they belong.

core/app/helpers/refinery/menu_helper.rb

@@ -13,65 +13,5 @@ def cache_if(condition, name = {}, &block)
       nil
     end
 
-    # Determines whether any page underneath the supplied page is the current page according to rails.
-    # Just calls selected_page? for each descendant of the supplied page
-    # unless it first quickly determines that there are no descendants.
-    def descendant_page_selected?(page)
-      (page.rgt != page.lft + 1) && page.descendants.any? { |descendant| selected_page?(descendant) }
-    end
-
-    def selected_page_or_descendant_page_selected?(page)
-      selected_page?(page) || descendant_page_selected?(page)
-    end
-
-    # Determine whether the supplied page is the currently open page according to Refinery.
-    def selected_page?(page)
-      path = request.path
-      path = path.force_encoding('utf-8') if path.respond_to?(:force_encoding)
-
-      # Ensure we match the path without the locale, if present.
-      if path =~ %r{^/#{::I18n.locale}/}
-        path = path.split(%r{^/#{::I18n.locale}}).last
-        path = "/" if path.blank?
-      end
-
-      # First try to match against a "menu match" value, if available.
-      return true if page.try(:menu_match).present? && path =~ Regexp.new(page.menu_match)
-
-      # Find the first url that is a string.
-      url = [page.url]
-      url << ['', page.url[:path]].compact.flatten.join('/') if page.url.respond_to?(:keys)
-      url = url.last.match(%r{^/#{::I18n.locale.to_s}(/.*)}) ? $1 : url.detect{|u| u.is_a?(String)}
-
-      # Now use all possible vectors to try to find a valid match
-      [path, URI.decode(path)].include?(url) || path == "/#{page.original_id}"
-    end
-
-    # This was extracted from app/views/refinery/_menu_branch.html.erb
-    # to remove the complexity of that template by reducing logic in the view.
-    def menu_branch_css(local_assigns)
-      options = local_assigns.dup
-      options.update(:sibling_count => options[:menu_branch].shown_siblings.length) unless options[:sibling_count]
-
-      css = []
-      css << Refinery::Core.menu_css[:selected] if selected_page_or_descendant_page_selected?(local_assigns[:menu_branch]) unless Refinery::Core.menu_css[:selected].nil?
-      css << Refinery::Core.menu_css[:first] if options[:menu_branch_counter] == 0 unless Refinery::Core.menu_css[:first].nil?
-      css << Refinery::Core.menu_css[:last] if options[:menu_branch_counter] == options[:sibling_count] unless Refinery::Core.menu_css[:last].nil?
-      css
-    end
-
-    def menu_branch_children(branch, options)
-      if !options[:menu_levels] || branch.ancestors.length < options[:menu_levels]
-        branch.children
-      else
-        []
-      end
-    end
-
-    def menu_roots(options)
-      roots = options.delete(:roots)
-      roots.presence || (options.delete(:collection) || refinery_menu_pages).roots
-    end
-
   end
 end

core/app/views/refinery/_header.html.erb

@@ -1,7 +1,4 @@
 <h1 id='logo'>
   <%= link_to Refinery::Core.site_name, refinery.root_path %>
 </h1>
-<%= render(:partial => "/refinery/menu", :locals => {
-             :dom_id => 'menu',
-             :css => 'menu'
-           }) %>
+<%= Refinery::Pages::MenuPresenter.new(refinery_menu_pages, self).to_html %>

core/lib/generators/refinery/core/templates/config/initializers/refinery/core.rb.erb

@@ -25,9 +25,6 @@ Refinery::Core.configure do |config|
   # Enable/disable authenticity token on frontend
   # config.authenticity_token_on_frontend = <%= Refinery::Core.authenticity_token_on_frontend.inspect %>
 
-  # CSS class selectors for menu helper
-  # config.menu_css = <%= Refinery::Core.menu_css.inspect %>
-
   # Should set this if concerned about DOS attacks. See
   # http://markevans.github.com/dragonfly/file.Configuration.html#Configuration
   # config.dragonfly_secret = <%= Refinery::Core.dragonfly_secret.inspect %>

core/lib/refinery/core/configuration.rb

@@ -4,7 +4,7 @@ module Core
 
     config_accessor :rescue_not_found, :s3_backend, :base_cache_key, :site_name,
                     :google_analytics_page_code, :authenticity_token_on_frontend,
-                    :menu_css, :dragonfly_secret,
+                    :dragonfly_secret,
                     :wymeditor_whitelist_tags, :javascripts, :stylesheets,
                     :s3_bucket_name, :s3_region, :s3_access_key_id,
                     :s3_secret_access_key, :force_ssl, :backend_route
@@ -15,7 +15,6 @@ module Core
     self.site_name = "Company Name"
     self.google_analytics_page_code = "UA-xxxxxx-x"
     self.authenticity_token_on_frontend = true
-    self.menu_css = { :selected => "selected", :first => "first", :last => "last" }
     self.dragonfly_secret = Array.new(24) { rand(256) }.pack('C*').unpack('H*').first
     self.wymeditor_whitelist_tags = {}
     self.javascripts = []

core/lib/refinery/menu.rb

@@ -2,10 +2,14 @@ module Refinery
   class Menu
 
     def initialize(objects = nil)
-      objects.each do |item|
+       append(objects)
+    end
+
+    def append(objects)
+      Array(objects).each do |item|
         item = item.to_refinery_menu_item if item.respond_to?(:to_refinery_menu_item)
-        items << MenuItem.new(item.merge(:menu => self))
-      end if objects
+        items << MenuItem.new(self, item)
+      end
     end
 
     attr_accessor :items
@@ -15,18 +19,13 @@ def items
     end
 
     def roots
-      @roots ||= items.select(&:orphan?)
+      @roots ||= select(&:orphan?)
     end
 
     def to_s
-      items.map(&:title).join(' ')
-    end
-
-    def inspect
-      items.map(&:inspect)
+      map(&:title).join(' ')
     end
 
-    # The delegation is specified so crazily so that it works on 1.8.x and 1.9.x
-    delegate *((Array.instance_methods - Object.instance_methods) << {:to => :items})
+    delegate :inspect, :map, :select, :to => :items
   end
 end

core/lib/refinery/menu_item.rb

@@ -1,67 +1,38 @@
 module Refinery
-  class MenuItem < HashWithIndifferentAccess
+  class MenuItem
 
-    class << self
-      def attributes
-        [:title, :parent_id, :lft, :rgt, :depth, :url, :menu, :menu_match]
-      end
+    attr_accessor :menu, :title, :parent_id, :lft, :rgt, :depth, :url, :menu_match,
+                  :original_type, :original_id
 
-      def apply_attributes!
-        attributes.each do |attribute|
-          class_eval %{
-            def #{attribute}
-              @#{attribute} ||= self[:#{attribute}]
-            end
-          } unless self.respond_to?(attribute)
-          class_eval %{
-            def #{attribute}=(attr)
-              @#{attribute} = attr
-            end
-          } unless self.respond_to?(:"#{attribute}=")
-        end
+    def initialize(menu, options = {})
+      @menu = menu
+      remap!(options).each do |key, value|
+        send "#{key}=", value
       end
     end
 
-    def original_id
-      @original_id ||= self[:id]
-    end
-
-    def original_type
-      @original_type ||= self[:type]
+    def remap!(options)
+      options[:original_id] = options.delete(:id)
+      options[:original_type] = options.delete(:type)
+      options
     end
 
-    apply_attributes!
-
     def ancestors
-      return @ancestors if @ancestors
-      @ancestors = []
-      p = self
-      @ancestors << p until(p = p.parent).nil?
-
-      @ancestors
+      @ancestors ||= generate_ancestors
     end
 
     def children
-      @children ||= if has_children?
-        menu.select { |item| item.original_type == original_type && item.parent_id == original_id }
-      else
-        []
-      end
+      @children ||= generate_children
     end
 
     def descendants
-      @descendants ||= if has_descendants?
-        menu.select{|item| item.original_type == original_type && item.lft > lft && item.rgt < rgt}
-      else
-        []
-      end
+      @descendants ||= generate_descendants
     end
 
     def has_children?
-      @has_children ||= (rgt > lft + 1)
+      !leaf?
     end
-    # really, they're the same.
-    alias_method :has_descendants?, :has_children?
+    alias_method :has_descendants?, :has_children? # really, they're the same.
 
     def has_parent?
       !parent_id.nil?
@@ -71,23 +42,44 @@ def orphan?
       !has_parent?
     end
 
-    def inspect
-      hash = {}
-
-      self.class.attributes.each do |attribute|
-        hash[attribute] = self[attribute]
-      end
-
-      hash.inspect
+    def leaf?
+      @leaf ||= rgt.to_i - lft.to_i == 1
     end
 
     def parent
-      @parent ||= (menu.detect{|item| item.original_type == original_type && item.original_id == parent_id} if has_parent?)
+      @parent ||= ancestors.detect { |item| item.original_id == parent_id }
     end
 
     def siblings
       @siblings ||= ((has_parent? ? parent.children : menu.roots) - [self])
     end
     alias_method :shown_siblings, :siblings
+
+    private
+    # At present a MenuItem can only have children of the same type to avoid id
+    # conflicts like a Blog::Post and a Page both having an id of 42
+    def compatible_with?(item)
+      original_type == item.original_type
+    end
+
+    def generate_ancestors
+      if has_parent?
+        menu.select { |item| compatible_with?(item) && item.lft < lft && item.rgt > rgt }
+      else
+        []
+      end
+    end
+
+    def generate_children
+      descendants.select { |item| item.parent_id == original_id }
+    end
+
+    def generate_descendants
+      if has_descendants?
+        menu.select { |item| compatible_with?(item) && item.lft > lft && item.rgt < rgt }
+      else
+        []
+      end
+    end
   end
 end

pages/app/controllers/refinery/pages_controller.rb

@@ -4,7 +4,7 @@ class PagesController < ::ApplicationController
     before_filter :find_page_for_preview, :only => [:preview]
 
     # Save whole Page after delivery
-    after_filter { |c| c.write_cache? }
+    after_filter :write_cache?
 
     # This action is usually accessed with the root path, normally '/'
     def home

pages/app/helpers/refinery/admin/pages_helper.rb

@@ -35,17 +35,13 @@ def page_meta_information(page)
           ::I18n.t('draft', :scope => 'refinery.admin.pages.page')
         end if page.draft?
 
-        meta_information.html_safe
+        meta_information
       end
 
       # We show the title from the next available locale
       # if there is no title for the current locale
       def page_title_with_translations(page)
-        if page.title.present?
-          page.title
-        else
-          page.translations.detect {|t| t.title.present?}.title
-        end
+        page.title.presence || page.translations.detect {|t| t.title.present?}.title
       end
     end
   end

pages/app/models/refinery/page.rb

@@ -113,7 +113,7 @@ def in_menu
       end
 
       def fast_menu
-        live.in_menu.order('lft ASC').includes(:translations)
+        live.in_menu.order(arel_table[:lft]).includes(:parent, :translations)
       end
 
       # Wrap up the logic of finding the pages based on the translations table.
@@ -240,8 +240,8 @@ def url_marketable
       Refinery::Pages::Url::Marketable.new(self).url
     end
 
-    def uncached_nested_url
-      [parent.try(:uncached_nested_url), to_param.to_s].compact.flatten
+    def nested_url
+      [parent.try(:nested_url), to_param.to_s].compact.flatten
     end
 
     # Returns an array with all ancestors to_param, allow with its own
@@ -250,7 +250,7 @@ def uncached_nested_url
     #
     #   ['about', 'mission']
     #
-    alias_method :nested_url, :uncached_nested_url
+    alias_method :uncached_nested_url, :nested_url
 
     # Returns the string version of nested_url, i.e., the path that should be generated
     # by the router