replaces instance variable in view by methods in controller

Replaces every instance variable in the view layer by a method call to the controller. This is done in order to:

1) better separate concerns.
2) Lazy loading of objects that might be used by a block. As the purpose of the plugin is to allow the user to choose from a list of available blocks, not all blocks will be used all the times. This also means that not all information is required to be loaded all the time. With this commit, we load only what is required for the current block selection by having each block call a method that loads the information it requires.

app/controllers/my_projects_overviews_controller.rb

@@ -18,9 +18,7 @@ class MyProjectsOverviewsController < ApplicationController
 
   unloadable
 
-  before_filter :find_project, :find_user, :find_my_project_overview
+  before_filter :find_project, :find_user
-  before_filter :find_page_blocks, :find_project_details
-  before_filter :find_attachments
   before_filter :authorize
   before_filter :jump_to_project_menu_item, :only => :index
 
@@ -47,9 +45,6 @@ class MyProjectsOverviewsController < ApplicationController
 
   # User's page layout configuration
   def page_layout
-    @block_options = []
-    BLOCKS.each {|k, v| @block_options << [l("my.blocks.#{v}", :default => [v, v.to_s.humanize]), k.dasherize]}
-    @block_options << [l(:label_custom_element), :custom_element]
   end
 
   def update_custom_element
@@ -59,13 +54,13 @@ class MyProjectsOverviewsController < ApplicationController
 
     if params["attachments"]
       # Attach files and save them
-      attachments = Attachment.attach_files(@overview, params["attachments"])
+      attachments = Attachment.attach_files(overview, params["attachments"])
       unless attachments[:unsaved].blank?
         flash[:error] = l(:warning_attachments_not_saved, attachments[:unsaved].size)
       end
     end
 
-    @overview.save_custom_element(block_name, block_title, textile)
+    overview.save_custom_element(block_name, block_title, textile)
 
     redirect_to :back
   end
@@ -77,23 +72,21 @@ class MyProjectsOverviewsController < ApplicationController
     block = params[:block].to_s.underscore
     if (BLOCKS.keys.include? block)
       # remove if already present in a group
-      %w(top left right hidden).each {|f| @overview.send(f).delete block }
+      %w(top left right hidden).each {|f| overview.send(f).delete block }
       # add it hidden
-      @overview.hidden.unshift block
+      overview.hidden.unshift block
-      @overview.save!
+      overview.save!
-      render(:partial => "block",
+      render :partial => "block",
-             :locals => { :user => @user,
+             :locals => { :block_name => block }
-               :project => @project,
-               :block_name => block})
     elsif block == "custom_element"
-      @overview.hidden.unshift @overview.new_custom_element
+      overview.hidden.unshift overview.new_custom_element
-      @overview.save!
+      overview.save!
       render(:partial => "block_textilizable",
-             :locals => { :user => @user,
+             :locals => { :user => user,
-               :project => @project,
+                          :project => project,
-               :block_title => l(:label_custom_element),
+                          :block_title => l(:label_custom_element),
-               :block_name => @overview.hidden.first.first,
+                          :block_name => overview.hidden.first.first,
-               :textile => @overview.hidden.first.last})
+                          :textile => overview.hidden.first.last})
     else
       render :nothing => true
     end
@@ -103,8 +96,8 @@ class MyProjectsOverviewsController < ApplicationController
   # params[:block] : id of the block to remove
   def remove_block
     block = param_to_block(params[:block])
-    %w(top left right hidden).each {|f| @overview.send(f).delete block }
+    %w(top left right hidden).each {|f| overview.send(f).delete block }
-    @overview.save!
+    overview.save!
     render :nothing => true
   end
 
@@ -115,14 +108,14 @@ class MyProjectsOverviewsController < ApplicationController
     group = params[:group]
     if group.is_a?(String)
       group_items = (params["list-#{group}"] || []).collect {|x| param_to_block(x) }
-      unless group_items.size < @overview.send(group).size
+      unless group_items.size < overview.send(group).size
         # We are adding or re-ordering, not removing
         # Remove group blocks if they are presents in other groups
-        @overview.update_attributes('top' => (@overview.top - group_items),
+        overview.update_attributes('top' => (overview.top - group_items),
-                                    'left' => (@overview.left - group_items),
+                                   'left' => (overview.left - group_items),
-                                    'right' => (@overview.right - group_items),
+                                   'right' => (overview.right - group_items),
-                                    'hidden' => (@overview.hidden - group_items))
+                                   'hidden' => (overview.hidden - group_items))
-        @overview.update_attribute(group, group_items)
+        overview.update_attribute(group, group_items)
       end
     end
     render :nothing => true
@@ -131,69 +124,129 @@ class MyProjectsOverviewsController < ApplicationController
   def param_to_block(param)
     block = param.to_s.underscore
     unless (BLOCKS.keys.include? block)
-      block = @overview.custom_elements.detect {|ary| ary.first == block}
+      block = overview.custom_elements.detect {|ary| ary.first == block}
     end
     block
   end
 
   def destroy_attachment
-    if @user.allowed_to?(:edit_project, @project)
+    if user.allowed_to?(:edit_project, project)
       begin
         att = Attachment.find(params[:attachment_id].to_i)
-        @overview.attachments.delete(att)
+        overview.attachments.delete(att)
-        @overview.save
+        overview.save
       rescue ActiveRecord::RecordNotFound
       end
     end
-    @attachments -= [att]
+    attachments -= [att]
     render :partial => 'page_layout_attachments'
   end
 
-  private
+  helper_method :users_by_role,
+                :childprojects,
+                :recent_news,
+                :trackers,
+                :open_issues_by_tracker,
+                :total_issues_by_tracker,
+                :assigned_issues,
+                :total_hours,
+                :project,
+                :user,
+                :blocks,
+                :block_options,
+                :overview,
+                :attachments,
+                :render_block,
+                :object_callback
 
-  def find_my_project_overview
+  def render_block name
-    @overview = MyProjectsOverview.find(:first, :conditions => "project_id = #{@project.id}")
-    # Auto-create missing overviews
-    @overview ||= MyProjectsOverview.create!(:project_id => @project.id)
   end
 
-  def find_user
+  def users_by_role
-    @user = User.current
+    @users_by_role ||= project.users_by_role
   end
 
-  def find_page_blocks
+  def childprojects
-    @blocks = {
+    @childprojects ||= project.children.visible.all
-      'top' => @overview.top,
-      'left' => @overview.left,
-      'right' => @overview.right,
-      'hidden' => @overview.hidden
-    }
   end
 
-  def find_project_details
+  def recent_news
-    @users_by_role = @project.users_by_role
+    @news ||= project.news.all :limit => 5,
-    @subprojects = @project.children.visible.all
-    @news = @project.news.find(:all, :limit => 5,
                                :include => [ :author, :project ],
-                               :order => "#{News.table_name}.created_on DESC")
+                               :order => "#{News.table_name}.created_on DESC"
-    @trackers = @project.rolled_up_trackers
 
-    cond = @project.project_condition(Setting.display_subprojects_issues?)
+  end
 
-    @open_issues_by_tracker = Issue.visible.count(:group => :tracker,
+  def trackers
-                                            :include => [:project, :status, :tracker],
+    @trackers ||= project.rolled_up_trackers
-                                            :conditions => ["(#{cond}) AND #{IssueStatus.table_name}.is_closed=?", false])
+  end
-    @total_issues_by_tracker = Issue.visible.count(:group => :tracker,
-                                            :include => [:project, :status, :tracker],
-                                            :conditions => cond)
 
-    if User.current.allowed_to?(:view_time_entries, @project)
+  def open_issues_by_tracker
-      @total_hours = TimeEntry.visible.sum(:hours, :include => :project, :conditions => cond).to_f
+    @open_issues_by_tracker ||= Issue.visible.count(:group => :tracker,
+                                                    :include => [:project, :status, :tracker],
+                                                    :conditions => ["(#{subproject_condition}) AND #{IssueStatus.table_name}.is_closed=?", false])
+  end
+
+  def total_issues_by_tracker
+    @total_issues_by_tracker ||= Issue.visible.count(:group => :tracker,
+                                                     :include => [:project, :status, :tracker],
+                                                     :conditions => subproject_condition)
+
+  end
+
+  def assigned_issues
+    @assigned_issues ||= Issue.visible.open.find(:all,
+                                                 :conditions => { :assigned_to_id => User.current.id },
+                                                 :limit => 10,
+                                                 :include => [ :status, :project, :tracker, :priority ],
+                                                 :order => "#{IssuePriority.table_name}.position DESC, #{Issue.table_name}.updated_on DESC")
+  end
+
+  def total_hours
+    if User.current.allowed_to?(:view_time_entries, project)
+      @total_hours ||= TimeEntry.visible.sum(:hours, :include => :project, :conditions => subproject_condition).to_f
     end
   end
 
-  def find_attachments
+  def project
-    @attachments = @overview.attachments || []
+    @project
+  end
+
+  def user
+    @user
+  end
+
+  def blocks
+    @blocks ||= {
+      'top' => overview.top,
+      'left' => overview.left,
+      'right' => overview.right,
+      'hidden' => overview.hidden
+    }
+  end
+
+  def block_options
+    @block_options = []
+    BLOCKS.each {|k, v| @block_options << [l("my.blocks.#{v}", :default => [v, v.to_s.humanize]), k.dasherize]}
+    @block_options << [l(:label_custom_element), :custom_element]
+  end
+
+  def overview
+    @overview ||= MyProjectsOverview.find_or_create_by_project_id(project.id)
+  end
+
+  def attachments
+    @attachments = overview.attachments || []
+  end
+
+  private
+
+  def subproject_condition
+    @subproject_condition ||= project.project_condition(Setting.display_subprojects_issues?)
+  end
+
+  def find_user
+    @user = User.current
   end
 
   def default_breadcrumb
@@ -203,7 +256,7 @@ class MyProjectsOverviewsController < ApplicationController
   def jump_to_project_menu_item
     if params[:jump]
       # try to redirect to the requested menu item
-      redirect_to_project_menu_item(@project, params[:jump]) && return
+      redirect_to_project_menu_item(project, params[:jump]) && return
     end
   end
 end

app/views/my_projects_overviews/_page_layout_attachments.rhtml

@@ -1,9 +1,9 @@
-<% for attachment in @attachments %>
+<% for attachment in attachments %>
   <p><%= link_to_attachment attachment, :class => 'icon icon-attachment' -%>
     <%= h(" - #{attachment.description}") unless attachment.description.blank? %>
     <span class="size">(<%= number_to_human_size attachment.filesize %>)</span>
     <%= link_to_remote image_tag('delete.png'),
-	:url => { :action => 'destroy_attachment', :attachment_id => attachment.id, :id => @project.id },
+	:url => { :action => 'destroy_attachment', :attachment_id => attachment.id, :id => project.id },
         :confirm => l(:text_are_you_sure),
         :class => 'delete',
         :title => l(:button_delete),

app/views/my_projects_overviews/blocks/_issuetracking.rhtml

@@ -1,24 +1,24 @@
 <h2 class="page-layout-only"><%=l(:label_issue_tracking)%></h2>
 <h3><%=l(:label_issue_tracking)%></h3>
-<% if User.current.allowed_to?(:view_issues, @project) %>
+<% if User.current.allowed_to?(:view_issues, project) %>
   <div class="issues overview">
     <ul>
-      <% for tracker in @trackers %>
+      <% for tracker in trackers %>
-        <li><%= link_to h(tracker.name), :controller => 'issues', :action => 'index', :project_id => @project,
+        <li><%= link_to h(tracker.name), :controller => 'issues', :action => 'index', :project_id => project,
                 :set_filter => 1,
                 "tracker_id" => tracker.id %>:
-          <%= l(:label_x_open_issues_abbr_on_total, :count => @open_issues_by_tracker[tracker].to_i,
+          <%= l(:label_x_open_issues_abbr_on_total, :count => open_issues_by_tracker[tracker].to_i,
-                :total => @total_issues_by_tracker[tracker].to_i) %>
+                :total => total_issues_by_tracker[tracker].to_i) %>
         </li>
       <% end %>
     </ul>
     <p>
-      <%= link_to l(:label_issue_view_all), :controller => 'issues', :action => 'index', :project_id => @project, :set_filter => 1 %>
+      <%= link_to l(:label_issue_view_all), :controller => 'issues', :action => 'index', :project_id => project, :set_filter => 1 %>
-      <% if User.current.allowed_to?(:view_calendar, @project, :global => true) %>
+      <% if User.current.allowed_to?(:view_calendar, project, :global => true) %>
-        | <%= link_to(l(:label_calendar), :controller => 'calendars', :action => 'show', :project_id => @project) %>
+        | <%= link_to(l(:label_calendar), :controller => 'calendars', :action => 'show', :project_id => project) %>
       <% end %>
-      <% if User.current.allowed_to?(:view_gantt, @project, :global => true) %>
+      <% if User.current.allowed_to?(:view_gantt, project, :global => true) %>
-        | <%= link_to(l(:label_gantt), :controller => 'gantts', :action => 'show', :project_id => @project) %>
+        | <%= link_to(l(:label_gantt), :controller => 'gantts', :action => 'show', :project_id => project) %>
       <% end %>
     </p>
   </div>

app/views/my_projects_overviews/blocks/_members.rhtml

@@ -1,9 +1,11 @@
 <h2 class="page-layout-only"><%=l(:label_member_plural)%></h2>
 <h3><%=l(:label_member_plural)%></h3>
-<% if @users_by_role.any? %>
+<% if users_by_role.any? %>
   <div class="members overview">
-    <p><% @users_by_role.keys.sort.each do |role| %>
+    <p>
-        <%=h role %>: <%= @users_by_role[role].sort.collect{|u| link_to_user u}.join(", ") %><br />
+      <% users_by_role.keys.sort.each do |role| %>
-    <% end %></p>
+        <%=h role %>: <%= users_by_role[role].sort.collect{ |u| link_to_user u }.join(", ") %><br />
+      <% end %>
+    </p>
   </div>
 <% end %>

app/views/my_projects_overviews/blocks/_news.rhtml

@@ -1,8 +1,13 @@
 <h2 class="page-layout-only"><%=l(:label_news)%></h2>
 <h3><%=l(:label_news_latest)%></h3>
-<% if @news.any? && authorize_for('news', 'index') %>
+<% if recent_news.any? && authorize_for('news', 'index') %>
   <div class="news overview">
-    <%= render :partial => 'news/news', :collection => @news %>
+    <%= render :partial => 'news/news', :collection => recent_news %>
-    <p><%= link_to l(:label_news_view_all), :controller => 'news', :action => 'index', :project_id => @project %></p>
+    <p>
+      <%= link_to l(:label_news_view_all),
+                  :controller => 'news',
+                  :action => 'index',
+                  :project_id => project %>
+    </p>
   </div>
 <% end %>

app/views/my_projects_overviews/blocks/_projectdetails.rhtml

@@ -2,12 +2,12 @@
 <h3><%=l(:label_project_details)%></h3>
 <div class="overview project_details">
   <ul>
-  <% unless @project.homepage.blank? %><li><%=l(:field_homepage)%>: <%= auto_link(h(@project.homepage)) %></li><% end %>
+  <% unless project.homepage.blank? %><li><%=l(:field_homepage)%>: <%= auto_link(h(project.homepage)) %></li><% end %>
-  <% if @subprojects.any? %>
+  <% if childprojects.any? %>
     <li><%=l(:label_subproject_plural)%>:
-      <%= @subprojects.collect{|p| link_to(h(p), project_url(p))}.join(", ") %></li>
+      <%= childprojects.collect{|p| link_to(h(p), project_url(p))}.join(", ") %></li>
   <% end %>
-  <% @project.visible_custom_field_values.each do |custom_value| %>
+  <% project.visible_custom_field_values.each do |custom_value| %>
     <% if !custom_value.value.blank? %>
       <li><%= h(custom_value.custom_field.name) %>: <%=h show_value(custom_value) %></li>
     <% end %>

app/views/my_projects_overviews/blocks/_subprojects.html.erb

@@ -1,7 +1,9 @@
 <h2 class="page-layout-only"><%=l(:label_subproject_plural)%></h2>
 <h3><%=l(:label_subproject_plural)%></h3>
-<% if @subprojects.any? %>
+<% if childprojects.any? %>
-<div class="subprojects overview">
+  <div class="subprojects overview">
-  <p><%= @subprojects.collect{ |p| link_to(h(p), project_url(p)) }.join(", ") %></p>
+    <p>
-</div>
+      <%= childprojects.collect{ |p| link_to(h(p), project_url(p)) }.join(", ") %>
+    </p>
+  </div>
 <% end %>

app/views/my_projects_overviews/index.rhtml

@@ -1,14 +1,14 @@
 <div class="contextual">
     <%=
       context_links = []
-      if User.current.allowed_to?(:add_subprojects, @project)
+      if User.current.allowed_to?(:add_subprojects, project)
         context_links << link_to(l(:label_subproject_new),
                                  { :controller => 'projects',
                                    :action => 'new',
-                                   :parent_id => @project },
+                                   :parent_id => project },
                                  :class => 'icon icon-add')
       end
-      if User.current.allowed_to?(:edit_project, @project)
+      if User.current.allowed_to?(:edit_project, project)
         context_links << link_to(l(:label_personalize_page),
                                  { :action => 'page_layout' },
                                  :class => 'icon icon-edit')
@@ -21,42 +21,40 @@
 
 <% ['top', 'left', 'right'].each do |position| %>
   <div id="list-<%= position %>" class="splitcontent<%= position %>">
-    <% @blocks[position].each do |b| %>
+    <% blocks[position].each do |b| %>
       <div class="mypage-box">
         <div class="wiki">
           <% if MyProjectsOverviewsController::BLOCKS.keys.include? b %>
-            <%= render :partial => "my_projects_overviews/blocks/#{b}",
+            <%= render :partial => "my_projects_overviews/blocks/#{b}" %>
-                       :locals => { :project => @project,
-                                    :user => @user,
-                                    :news => @news } %>
           <% elsif b.respond_to? :to_ary %>
             <%= render :partial => "my_projects_overviews/blocks/textilizable",
-                       :locals => { :project => @project,
+                       :locals => { :project => project,
-                                    :user => @user,
+                                    :user => user,
                                     :block_title => b[1],
                                     :textile => b.last } %>
           <% end %>
         </div>
       </div>
-    <% end if @blocks[position] %>
+    <% end if blocks[position] %>
   </div>
 <% end %>
 
 <%= context_menu :controller => 'issues', :action => 'context_menu' %>
 
 <% content_for :sidebar do %>
-    <% if @total_hours.present? %>
+  <% if total_hours.present? %>
     <h3><%= l(:label_spent_time) %></h3>
-    <p><span class="icon icon-time"><%= l_hours(@total_hours) %></span></p>
+    <p><span class="icon icon-time"><%= l_hours(total_hours) %></span></p>
     <p>
-      <%= link_to(l(:label_details), {:controller => 'timelog', :action => 'index', :project_id => @project}) %> |
+      <%= link_to(l(:label_details), {:controller => 'timelog', :action => 'index', :project_id => project}) %> |
-      <%= link_to(l(:label_report), {:controller => 'time_entry_reports', :action => 'report', :project_id => @project}) %>
+
+      <%= link_to(l(:label_report), {:controller => 'time_entry_reports', :action => 'report', :project_id => project}) %>
       <% if authorize_for('timelog', 'new') %>
-        | <%= link_to l(:button_log_time), {:controller => 'timelog', :action => 'new', :project_id => @project} %>
+        | <%= link_to l(:button_log_time), {:controller => 'timelog', :action => 'new', :project_id => project} %>
       <% end %>
     </p>
-    <% end %>
+  <% end %>
-    <%= call_hook(:view_projects_show_sidebar_bottom, :project => @project) %>
+  <%= call_hook(:view_projects_show_sidebar_bottom, :project => project) %>
 <% end %>
 
 <% content_for :header_tags do %>

app/views/my_projects_overviews/page_layout.rhtml

@@ -94,7 +94,7 @@ function addBlock() {
 <div class="contextual">
 <% form_tag({:action => "add_block"}, :id => "block-form") do %>
   <%= select_tag 'block',
-                 "<option>--#{t(:button_add)}--</option>" + options_for_select(@block_options),
+                 "<option>--#{t(:button_add)}--</option>" + options_for_select(block_options),
                  :id => "block-select",
                  :onChange => "addBlock();"
 %>
@@ -107,9 +107,9 @@ function addBlock() {
 <h4><%=l(:label_visible_elements) %></h4>
 <% (field_list - ['hidden']).each do |f| %>
   <div id="list-<%= f %>" class="splitcontent<%= f %> block-receiver">
-    <% @blocks[f].each do |b| %>
+    <% blocks[f].each do |b| %>
       <% if MyProjectsOverviewsController::BLOCKS.keys.include? b %>
-        <%= render(:partial => 'block', :locals => {:block_name => b}) %>
+        <%= render(:partial => 'block', :locals => { :block_name => b }) %>
       <% elsif b.respond_to? :to_ary %>
         <%= render(:partial => 'block_textilizable',
                    :locals => {:block_name => b.first,
@@ -123,7 +123,7 @@ function addBlock() {
 <div style="clear: both"></div>
 <h4><%=l(:label_hidden_elements) %></h4>
 <div id="list-hidden" class="block-receiver">
-  <% @blocks['hidden'].each do |b| %>
+  <% blocks['hidden'].each do |b| %>
     <% if MyProjectsOverviewsController::BLOCKS.keys.include? b %>
       <%= render(:partial => 'block', :locals => {:block_name => b}) %>
     <% elsif b.respond_to? :to_ary %>