clean code and refactoring a lot + handle non published page

app/models/account.rb

@@ -1,6 +1,6 @@
 class Account
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps  
+  include Locomotive::Mongoid::Document
   
   # devise modules
   devise :database_authenticatable, :recoverable, :rememberable, :trackable, :validatable

app/models/asset.rb

@@ -1,4 +1,5 @@
-class Asset
+class Asset 
+  
   include Mongoid::Document
   include Mongoid::Timestamps
   

app/models/asset_collection.rb

@@ -1,7 +1,6 @@
-class AssetCollection  
+class AssetCollection
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
-  include Mongoid::CustomFields
   
   ## fields ##
   field :name, :type => String

app/models/content_instance.rb

@@ -1,4 +1,5 @@
-class ContentInstance  
+class ContentInstance
+
   include Mongoid::Document
   include Mongoid::Timestamps
   

app/models/content_type.rb

@@ -1,7 +1,6 @@
-class ContentType  
+class ContentType
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
-  include Mongoid::CustomFields
   
   ## fields ##
   field :name

app/models/extensions/page/parts.rb

@@ -0,0 +1,61 @@
+module Models
+  
+  module Extensions
+    
+    module Page
+      
+      module Parts  
+
+        extend ActiveSupport::Concern
+
+        included do
+                              
+          before_create { |p| p.parts << PagePart.build_body_part if p.parts.empty? }
+          
+        end 
+        
+        module InstanceMethods
+          
+          def parts_attributes=(attributes)    
+            self.update_parts(attributes.values.map { |attrs| PagePart.new(attrs) })
+          end
+          
+          def joined_parts
+            self.parts.enabled.map(&:template).join('')
+          end
+          
+          protected
+          
+          def update_parts(parts)
+            performed = []
+
+            # add / update
+            parts.each do |part|
+              if (existing = self.parts.detect { |p| p.id == part.id || p.slug == part.slug })
+                existing.attributes = part.attributes.delete_if { |k, v| %w{_id slug}.include?(k) }
+              else
+                self.parts << (existing = part)
+              end
+              performed << existing unless existing.disabled?
+            end
+
+            # disable missing parts
+            (self.parts.map(&:slug) - performed.map(&:slug)).each do |slug|
+              self.parts.detect { |p| p.slug == slug }.disabled = true
+            end
+          end
+
+          def update_parts!(new_parts)
+            self.update_parts(new_parts)
+            self.save
+          end
+          
+        end
+        
+      end
+      
+    end
+    
+  end
+  
+end

app/models/extensions/page/render.rb

@@ -0,0 +1,25 @@
+module Models
+  
+  module Extensions
+    
+    module Page
+      
+      module Render
+
+        def render(context)
+          self.template.render(context)
+
+          if self.layout
+            self.layout.template.render(context)
+          else
+            ::Liquid::Template.parse("{{ content_for_layout }}").render(context)
+          end
+        end
+        
+      end
+      
+    end
+    
+  end
+  
+end

app/models/extensions/page/tree.rb

@@ -0,0 +1,104 @@
+module Models
+  
+  module Extensions
+    
+    module Page
+      
+      module Tree  
+
+        extend ActiveSupport::Concern
+
+        included do
+          include Mongoid::Acts::Tree
+          
+          ## fields ##
+          field :position, :type => Integer
+          
+          ## behaviours ##
+          acts_as_tree :order => ['position', 'asc']
+          
+          ## callbacks ##
+          before_validate :reset_parent
+          before_save { |p| p.parent_id = nil if p.parent_id.blank? }
+          before_save :change_parent
+          before_create { |p| p.send(:fix_position, false) }
+          before_create :add_to_list_bottom
+          before_destroy :remove_from_list
+
+          # Fixme (Didier L.): Instances methods are defined before the include itself
+          alias :ancestors :hacked_ancestors
+          alias :fix_position :hacked_fix_position
+        end 
+        
+        module InstanceMethods
+          
+          def sort_children!(ids)
+            ids.each_with_index do |id, position|
+              child = self.children.detect { |p| p._id == id }
+              child.position = position
+              child.save
+            end
+          end
+          
+          def parent=(owner) # missing in acts_as_tree
+            @_parent = owner
+            self.fix_position(false)
+            self.instance_variable_set :@_will_move, true
+          end
+          
+          def hacked_ancestors
+            return [] if root?
+            self.class.find(self.path.clone << nil) # bug in mongoid (it does not handle array with one element) 
+          end          
+          
+          protected
+          
+          def change_parent
+            if self.parent_id_changed?
+              self.fix_position(false)
+              self.add_to_list_bottom
+              self.instance_variable_set :@_will_move, true
+            end
+          end
+
+          def hacked_fix_position(perform_save = true)
+            if parent.nil?
+              self[parent_id_field] = nil
+              self[path_field] = []
+              self[depth_field] = 0
+            else
+              self[parent_id_field] = parent._id
+              self[path_field] = parent[path_field] + [parent._id]
+              self[depth_field] = parent[depth_field] + 1
+              self.save if perform_save
+            end
+          end          
+          
+          def reset_parent
+            if self.parent_id_changed?
+              @_parent = nil
+            end
+          end
+          
+          def add_to_list_bottom  
+            self.position = (::Page.where(:_id.ne => self._id).and(:parent_id => self.parent_id).max(:position) || 0) + 1
+          end
+
+          def remove_from_list
+            return if (self.site rescue nil).nil?
+
+            ::Page.where(:parent_id => self.parent_id).and(:position.gt => self.position).each do |p|
+              p.position -= 1
+              p.save
+            end
+          end
+          
+        end
+        
+      end
+      
+    end
+    
+  end
+  
+end

app/models/liquid_template.rb

@@ -1,44 +1,32 @@
-class LiquidTemplate  
+class LiquidTemplate
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
   
   ## fields ##
   field :name
   field :slug
   field :value
-  field :template, :type => Binary
   
   ## associations ##
   belongs_to_related :site
   
   ## callbacks ##
   before_validate :normalize_slug
-  before_validate :store_template
   
   ## validations ##
   validates_presence_of   :site, :name, :slug, :value
   validates_uniqueness_of :slug, :scope => :site_id
   
+  ## behaviours ##
+  liquify_template :value
+  
   ## methods ##
-  
+    
-  def template
-    Marshal.load(read_attribute(:template).to_s) rescue nil
-  end
-  
   protected
   
   def normalize_slug
     self.slug = self.name.clone if self.slug.blank? && self.name.present?
     self.slug.slugify!(:without_extension => true, :downcase => true) if self.slug.present?
   end
-  
+      
-  def store_template
-    begin
-      parsed_template = Liquid::Template.parse(self.value)
-      self.template = BSON::Binary.new(Marshal.dump(parsed_template))
-    rescue Liquid::SyntaxError => error
-      self.errors.add :value, :liquid_syntax_error
-    end
-  end
-    
 end

app/models/membership.rb

@@ -1,6 +1,6 @@
 class Membership
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
   
   ## fields ##
   field :admin, :type => Boolean, :default => false

app/models/page.rb

@@ -1,7 +1,11 @@
-class Page  
+class Page
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
-  include Mongoid::Acts::Tree
+
+  ## Extensions ##  
+  include Models::Extensions::Page::Tree
+  include Models::Extensions::Page::Parts
+  include Models::Extensions::Page::Render
   
   ## fields ##
   field :title
@@ -10,8 +14,6 @@ class Page
   field :published, :type => Boolean, :default => false
   field :keywords
   field :description
-  field :position, :type => Integer
-  field :template, :type => Binary
   
   ## associations ##
   belongs_to_related :site
@@ -19,17 +21,9 @@ class Page
   embeds_many :parts, :class_name => 'PagePart'
     
   ## callbacks ##
-  before_validate :reset_parent
   before_validate :normalize_slug
-  before_validate :store_template
   before_save { |p| p.fullpath = p.fullpath(true) }
-  before_save { |p| p.parent_id = nil if p.parent_id.blank? }
-  before_save :change_parent
-  before_create { |p| p.parts << PagePart.build_body_part if p.parts.empty? }
-  before_create { |p| p.fix_position(false) }
-  before_create :add_to_list_bottom
   before_destroy :do_not_remove_index_and_404_pages
-  before_destroy :remove_from_list
   
   ## validations ##
   validates_presence_of     :site, :title, :slug
@@ -38,11 +32,11 @@ class Page
   
   ## named scopes ##
   named_scope :latest_updated, :order_by => [[:updated_at, :desc]], :limit => Locomotive.config.lastest_items_nb
-  named_scope :index, :where => { :slug => 'index', :depth => 0 }
+  named_scope :index, :where => { :slug => 'index', :depth => 0, :published => true }
-  named_scope :not_found, :where => { :slug => '404', :depth => 0 }
+  named_scope :not_found, :where => { :slug => '404', :depth => 0, :published => true }
   
   ## behaviours ##
-  acts_as_tree :order => ['position', 'asc']
+  liquify_template :joined_parts
   
   ## methods ##
   
@@ -54,24 +48,6 @@ class Page
     self.slug == '404' && self.depth.to_i == 0
   end
   
-  def parts_attributes=(attributes)    
-    self.update_parts(attributes.values.map { |attrs| PagePart.new(attrs) })
-  end
-    
-  def parent=(owner) # missing in acts_as_tree
-    @_parent = owner
-    self.fix_position(false)
-    self.instance_variable_set :@_will_move, true
-  end
-  
-  def sort_children!(ids)
-    ids.each_with_index do |id, position|
-      child = self.children.detect { |p| p._id == id }
-      child.position = position
-      child.save
-    end
-  end
-
   def fullpath(force = false)
     if read_attribute(:fullpath).present? && !force
       return read_attribute(:fullpath)
@@ -89,26 +65,7 @@ class Page
   def url
     "http://#{self.site.domains.first}/#{self.fullpath}.html"
   end
-  
+    
-  def template
-    Marshal.load(read_attribute(:template).to_s) rescue nil
-  end
-  
-  def ancestors
-    return [] if root?
-    self.class.find(self.path.clone << nil) # bug in mongoid (it does not handle array with one element) 
-  end
-  
-  def render(context)
-    self.template.render(context)
-                
-    if self.layout
-      self.layout.template.render(context)
-    else
-      Liquid::Template.parse("{{ content_for_layout }}").render(context)
-    end
-  end
-  
   protected
   
   def do_not_remove_index_and_404_pages
@@ -121,83 +78,10 @@ class Page
       raise I18n.t('errors.messages.protected_page')
     end
   end
-  
+      
-  def update_parts(parts)
-    performed = []
-    
-    # add / update
-    parts.each do |part|
-      if (existing = self.parts.detect { |p| p.id == part.id || p.slug == part.slug })
-        existing.attributes = part.attributes.delete_if { |k, v| %w{_id slug}.include?(k) }
-      else
-        self.parts << (existing = part)
-      end
-      performed << existing unless existing.disabled?
-    end
-    
-    # disable missing parts
-    (self.parts.map(&:slug) - performed.map(&:slug)).each do |slug|
-      self.parts.detect { |p| p.slug == slug }.disabled = true
-    end
-  end
-  
-  def update_parts!(new_parts)
-    self.update_parts(new_parts)
-    self.save
-  end
-  
-  def change_parent
-    if self.parent_id_changed?
-      self.fix_position(false)
-      self.add_to_list_bottom
-      self.instance_variable_set :@_will_move, true
-    end
-  end
-  
-  def fix_position(perform_save = true)
-    if parent.nil?
-      self[parent_id_field] = nil
-      self[path_field] = []
-      self[depth_field] = 0
-    else
-      self[parent_id_field] = parent._id
-      self[path_field] = parent[path_field] + [parent._id]
-      self[depth_field] = parent[depth_field] + 1
-      self.save if perform_save
-    end
-  end
-  
-  def reset_parent
-    if self.parent_id_changed?
-      @_parent = nil
-    end
-  end
-  
-  def add_to_list_bottom  
-    self.position = (Page.where(:_id.ne => self._id).and(:parent_id => self.parent_id).max(:position) || 0) + 1
-  end
-  
-  def remove_from_list
-    return if (self.site rescue nil).nil?
-    
-    Page.where(:parent_id => self.parent_id).and(:position.gt => self.position).each do |p|
-      p.position -= 1
-      p.save
-    end
-  end
-    
   def normalize_slug  
     self.slug = self.title.clone if self.slug.blank? && self.title.present?    
     self.slug.slugify!(:without_extension => true) if self.slug.present?
   end
-  
+    
-  def store_template
-    begin
-      parsed_template = Liquid::Template.parse(self.parts.enabled.map(&:template).join(''))
-      self.template = BSON::Binary.new(Marshal.dump(parsed_template))
-    rescue Liquid::SyntaxError => error
-      self.errors.add :template, :liquid_syntax_error
-    end
-  end
-  
 end

app/models/page_part.rb

@@ -1,4 +1,5 @@
 class PagePart
+  
   include Mongoid::Document
 
   ## fields ##

app/models/site.rb

@@ -1,6 +1,6 @@
-class Site  
+class Site
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
   
   ## fields ##
   field :name

app/models/theme_asset.rb

@@ -1,6 +1,6 @@
 class ThemeAsset
-  include Mongoid::Document
+  
-  include Mongoid::Timestamps
+  include Locomotive::Mongoid::Document
   
   ## fields ##
   field :slug

app/views/admin/pages/edit.html.haml

@@ -4,7 +4,7 @@
   = render 'admin/shared/menu/contents'
 
 - content_for :buttons do
-  = admin_button_tag :show, '#', :class => 'show'
+  = admin_button_tag :show, "/#{@page.fullpath}", :class => 'show'
 
 %p= t('.help')
 

doc/TODO

@@ -2,20 +2,24 @@ BOARD:
 - theme assets
 - theme assets picker (???)
 
+- assets uploader: remove old files if new one
+- theme assets: disable version if not image
+
 BACKLOG:
-- file type (icons)
 
 - devise messages in French
 - localize devise emails
 - refactoring admin crud (pages + layouts + snippets)
-- refactoring page.rb => create module pagetree
 
+- new types for custom field
+  - file
+  - boolean
+  - date
 - refactoring: CustomFields::CustomField => CustomFields::Field
 - optimization custom_fields: use dynamic class for a collection instead of modifying the metaclass each time we build an item
 
 BUGS:
-- theme assets: disable version if not image
+- when assigning new layout, disabled parts show up :-( (js problem)
-- assets uploader: remove old files if new one
 
 NICE TO HAVE:
 - asset collections: custom resizing if image
@@ -85,4 +89,6 @@ x contents (CRUD)
 x contents sub menu => BUG
 x liquid rendering engine
 x contents pagination
-x how to disable a page part in layout ? (BUG)
+x how to disable a page part in layout ? (BUG)
+x non published page (redirect to 404 ?)
+x refactoring page.rb => create module pagetree

lib/locomotive.rb

@@ -1,6 +1,7 @@
 # require 'locomotive/patches'
 require 'locomotive/configuration'
 require 'locomotive/liquid'
+require 'locomotive/mongoid'
 
 module Locomotive
 

lib/locomotive/liquid/db_file_system.rb

@@ -1,10 +1,10 @@
 module Locomotive
   
   module Liquid
-  
+      
-    # Works only with snippets 
     class DbFileSystem
         
+      # Works only with snippets 
       def read_template_file(site, template_path)
         raise FileSystemError, "Illegal snippet name '#{template_path}'" unless template_path =~ /^[^.\/][a-zA-Z0-9_\/]+$/
     

lib/locomotive/liquid/liquify_template.rb

@@ -0,0 +1,62 @@
+module Locomotive
+  
+  module Liquid
+      
+    module LiquifyTemplate
+      
+      def self.included(base)
+        base.extend(ClassMethods)
+      end
+
+      # Store the parsed version of a liquid template into a column in order to increase performance
+      # See http://cjohansen.no/en/rails/liquid_email_templates_in_rails 
+      #
+      # class Page
+      #   liquify_template :body
+      # end
+      #
+      # page = Page.new :body => '...some liquid tags'
+      # page.template # Liquid::Template
+      # 
+      #
+      module ClassMethods
+
+        def liquify_template(source = :value)
+          field :serialized_template, :type => Binary
+          before_validate :store_template
+          
+          class_eval <<-EOV
+            def liquify_template_source
+              self.send(:#{source.to_s})
+            end
+          EOV
+          
+          include InstanceMethods
+        end
+
+      end
+      
+      module InstanceMethods
+        
+        def template
+          Marshal.load(read_attribute(:serialized_template).to_s) rescue nil
+        end
+                
+        protected
+        
+        def store_template
+          begin
+            template = ::Liquid::Template.parse(self.liquify_template_source)
+            self.serialized_template = BSON::Binary.new(Marshal.dump(template))
+          rescue ::Liquid::SyntaxError => error
+            self.errors.add :template, :liquid_syntax_error
+          end
+        end
+        
+      end
+      
+    end
+          
+  end
+  
+end

lib/locomotive/mongoid.rb

@@ -0,0 +1 @@
+require 'locomotive/mongoid/document'

lib/locomotive/mongoid/document.rb

@@ -0,0 +1,20 @@
+module Locomotive
+  
+  module Mongoid
+  
+    module Document
+    
+      extend ActiveSupport::Concern
+
+      included do
+        include ::Mongoid::Document
+        include ::Mongoid::Timestamps
+        include ::Mongoid::CustomFields
+        include Locomotive::Liquid::LiquifyTemplate
+      end
+    
+    end
+  
+  end
+  
+end

lib/locomotive/render.rb

@@ -23,8 +23,13 @@ module Locomotive
         path.gsub!(/^\//, '')
         path = 'index' if path.blank?
         
-        current_site.pages.where(:fullpath => path).first ||
+        if page = current_site.pages.where(:fullpath => path).first
-        current_site.pages.not_found.first
+          if not page.published? and current_account.nil?
+            page = nil
+          end
+        end
+
+        page || current_site.pages.not_found.first
       end
       
       def locomotive_context

spec/lib/locomotive/render_spec.rb

@@ -28,22 +28,27 @@ describe 'Locomotive rendering system' do
   
   context 'when retrieving page' do
     
+    before(:each) do
+      @page = Factory.build(:page, :site => nil, :published => true)
+      @controller
+    end
+    
     it 'should retrieve the index page /' do
       @controller.request.fullpath = '/'
-      @controller.current_site.pages.expects(:where).with({ :fullpath => 'index' }).returns([true])
+      @controller.current_site.pages.expects(:where).with({ :fullpath => 'index' }).returns([@page])
-      @controller.send(:locomotive_page).should be_true
+      @controller.send(:locomotive_page).should_not be_nil
     end
     
     it 'should also retrieve the index page (index.html)' do
       @controller.request.fullpath = '/index.html'
-      @controller.current_site.pages.expects(:where).with({ :fullpath => 'index' }).returns([true])
+      @controller.current_site.pages.expects(:where).with({ :fullpath => 'index' }).returns([@page])
-      @controller.send(:locomotive_page).should be_true
+      @controller.send(:locomotive_page).should_not be_nil
     end
     
     it 'should retrieve it based on the full path' do
       @controller.request.fullpath = '/about_us/team.html'
-      @controller.current_site.pages.expects(:where).with({ :fullpath => 'about_us/team' }).returns([true])
+      @controller.current_site.pages.expects(:where).with({ :fullpath => 'about_us/team' }).returns([@page])
-      @controller.send(:locomotive_page).should be_true
+      @controller.send(:locomotive_page).should_not be_nil
     end
     
     it 'should return the 404 page if the page does not exist' do
@@ -52,6 +57,29 @@ describe 'Locomotive rendering system' do
       @controller.send(:locomotive_page).should be_true
     end
     
+    context 'non published page' do
+      
+      before(:each) do
+        @page.published = false
+        @controller.current_account = nil
+      end
+    
+      it 'should return the 404 page if the page has not been published yet' do
+        @controller.request.fullpath = '/contact'        
+        @controller.current_site.pages.expects(:where).with({ :fullpath => 'contact' }).returns([@page])
+        @controller.current_site.pages.expects(:not_found).returns([true])
+        @controller.send(:locomotive_page).should be_true
+      end
+      
+      it 'should not return the 404 page if the page has not been published yet and admin is logged in' do
+        @controller.current_account = true
+        @controller.request.fullpath = '/contact'
+        @controller.current_site.pages.expects(:where).with({ :fullpath => 'contact' }).returns([@page])
+        @controller.send(:locomotive_page).should == @page
+      end
+      
+    end
+    
   end
   
 end

spec/support/locomotive.rb

@@ -7,7 +7,7 @@ module Locomotive
    
     include Locomotive::Render
    
-    attr_accessor :output, :current_site
+    attr_accessor :output, :current_site, :current_account
    
     def render(options = {})
       self.output = options[:text]
@@ -20,6 +20,7 @@ module Locomotive
     def request
       @request ||= TestRequest.new
     end
+
   end
   
   class TestResponse

vendor/plugins/custom_fields/lib/custom_fields.rb

@@ -8,7 +8,6 @@ module Mongoid
   module CustomFields  
     extend ActiveSupport::Concern
     included do
-      puts "loading from CustomFieldsFor"
       include ::CustomFields::CustomFieldsFor
     end  
   end

vendor/plugins/custom_fields/lib/custom_fields/custom_fields_for.rb

@@ -25,8 +25,6 @@ module CustomFields
     module ClassMethods
       
       def custom_fields_for(collection_name)
-        puts "settings custom fields for #{collection_name}"
-        
         singular_name = collection_name.to_s.singularize
         
         class_eval <<-EOV