Merged IssuesController #edit and #update into a single actions.

Users with 'edit issues' permission can now update any property including custom fields when adding a note or changing the status (#519, #581, #587).

git-svn-id: http://redmine.rubyforge.org/svn/trunk@1129 e93f8b46-1217-0410-a6f0-8f06a7374b81

10 files changed, 88 insertions, 126 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index b722d9340..dbb49405c 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -25,7 +25,7 @@ class IssuesController < ApplicationController
   before_filter :find_optional_project, :only => [:index, :changes]
   accept_key_auth :index, :changes
   
-  cache_sweeper :issue_sweeper, :only => [ :new, :edit, :update, :destroy ]
+  cache_sweeper :issue_sweeper, :only => [ :new, :edit, :destroy ]
 
   helper :journals
   helper :projects
@@ -85,10 +85,12 @@ class IssuesController < ApplicationController
   end
   
   def show
-    @custom_values = @issue.custom_values.find(:all, :include => :custom_field, :order => "#{CustomField.table_name}.position")
+    @custom_values = @project.custom_fields_for_issues(@issue.tracker).collect { |x| @issue.custom_values.find_by_custom_field_id(x.id) || CustomValue.new(:custom_field => x, :customized => @issue) }
     @journals = @issue.journals.find(:all, :include => [:user, :details], :order => "#{Journal.table_name}.created_on ASC")
-    @status_options = @issue.new_statuses_allowed_to(User.current)
+    @allowed_statuses = @issue.new_statuses_allowed_to(User.current)
+    @edit_allowed = User.current.allowed_to?(:edit_issues, @project)
     @activities = Enumeration::get_values('ACTI')
+    @priorities = Enumeration::get_values('IPRI')
     respond_to do |format|
       format.html { render :template => 'issues/show.rhtml' }
       format.pdf  { send_data(render(:template => 'issues/show.rfpdf', :layout => false), :type => 'application/pdf', :filename => "#{@project.identifier}-#{@issue.id}.pdf") }
@@ -140,48 +142,33 @@ class IssuesController < ApplicationController
     render :layout => !request.xhr?
   end
   
+  # Attributes that can be updated on workflow transition (without :edit permission)
+  # TODO: make it configurable (at least per role)
+  UPDATABLE_ATTRS_ON_TRANSITION = %w(status_id assigned_to_id fixed_version_id done_ratio) unless const_defined?(:UPDATABLE_ATTRS_ON_TRANSITION)
+  
   def edit
+    @allowed_statuses = @issue.new_statuses_allowed_to(User.current)
+    @activities = Enumeration::get_values('ACTI')
     @priorities = Enumeration::get_values('IPRI')
     @custom_values = []
+    @edit_allowed = User.current.allowed_to?(:edit_issues, @project)
     if request.get?
       @custom_values = @project.custom_fields_for_issues(@issue.tracker).collect { |x| @issue.custom_values.find_by_custom_field_id(x.id) || CustomValue.new(:custom_field => x, :customized => @issue) }
     else
-      begin
-        journal = @issue.init_journal(User.current)
-        # Retrieve custom fields and values
-        if params["custom_fields"]
-          @custom_values = @project.custom_fields_for_issues(@issue.tracker).collect { |x| CustomValue.new(:custom_field => x, :customized => @issue, :value => params["custom_fields"][x.id.to_s]) }
-          @issue.custom_values = @custom_values
-        end
-        @issue.attributes = params[:issue]
-        if @issue.save
-          flash[:notice] = l(:notice_successful_update)
-          Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
-          redirect_to(params[:back_to] || {:action => 'show', :id => @issue})
-        end
-      rescue ActiveRecord::StaleObjectError
-        # Optimistic locking exception
-        flash[:error] = l(:notice_locking_conflict)
+      @notes = params[:notes]
+      journal = @issue.init_journal(User.current, @notes)
+      # User can change issue attributes only if he has :edit permission or if a workflow transition is allowed
+      if (@edit_allowed || !@allowed_statuses.empty?) && params[:issue]
+        attrs = params[:issue].dup
+        attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) } unless @edit_allowed
+        attrs.delete(:status_id) unless @allowed_statuses.detect {|s| s.id.to_s == attrs[:status_id].to_s}
+        @issue.attributes = attrs
+      end
+      # Update custom fields if user has :edit permission
+      if @edit_allowed && params[:custom_fields]
+        @custom_values = @project.custom_fields_for_issues(@issue.tracker).collect { |x| CustomValue.new(:custom_field => x, :customized => @issue, :value => params["custom_fields"][x.id.to_s]) }
+        @issue.custom_values = @custom_values
       end
-    end		
-  end
-  
-  # Attributes that can be updated on workflow transition
-  # TODO: make it configurable (at least per role)
-  UPDATABLE_ATTRS_ON_TRANSITION = %w(status_id assigned_to_id fixed_version_id done_ratio) unless const_defined?(:UPDATABLE_ATTRS_ON_TRANSITION)
-  
-  def update
-    @status_options = @issue.new_statuses_allowed_to(User.current)
-    @activities = Enumeration::get_values('ACTI')
-    journal = @issue.init_journal(User.current, params[:notes])
-    # User can change issue attributes only if a workflow transition is allowed
-    if !@status_options.empty? && params[:issue]
-      attrs = params[:issue].dup
-      attrs.delete_if {|k,v| !UPDATABLE_ATTRS_ON_TRANSITION.include?(k) }
-      attrs.delete(:status_id) unless @status_options.detect {|s| s.id.to_s == attrs[:status_id].to_s}
-      @issue.attributes = attrs
-    end
-    if request.post?
       attachments = attach_files(@issue, params[:attachments])
       attachments.each {|a| journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
       if @issue.save
@@ -243,7 +230,7 @@ class IssuesController < ApplicationController
   def preview
     issue = Issue.find_by_id(params[:id])
     @attachements = issue.attachments if issue
-    @text = (params[:issue] ? params[:issue][:description] : nil) || params[:notes]
+    @text = params[:notes] || (params[:issue] ? params[:issue][:description] : nil)
     render :partial => 'common/preview'
   end
   
diff --git a/app/views/issues/_edit.rhtml b/app/views/issues/_edit.rhtml
new file mode 100644
index 000000000..0f843e855
--- /dev/null
+++ b/app/views/issues/_edit.rhtml
@@ -0,0 +1,39 @@
+<% labelled_tabular_form_for :issue, @issue,
+                             :url => {:action => 'edit', :id => @issue},
+                             :html => {:id => 'issue-form',
+                                       :multipart => true} do |f| %>
+    <%= error_messages_for 'issue' %>
+    <div class="box">
+    <% if @edit_allowed || !@allowed_statuses.empty? %>
+        <fieldset>
+        <legend><%= l(:label_change_properties) %>
+        <% if !@issue.new_record? && !@issue.errors.any? && @edit_allowed %>
+        <small>(<%= link_to l(:label_more), {}, :onclick => 'Effect.toggle("issue_descr_fields", "appear", {duration:0.3}); return false;' %>)</small>
+        <% end %>
+        </legend>
+        <%= render :partial => (@edit_allowed ? 'form' : 'form_update'), :locals => {:f => f} %>
+        </fieldset>
+    <% end %>
+    
+    <fieldset><legend><%= l(:field_notes) %></legend>
+    <%= text_area_tag 'notes', @notes, :cols => 60, :rows => 10, :class => 'wiki-edit' %>
+    <%= wikitoolbar_for 'notes' %>
+    
+    <p id="attachments_p"><label><%=l(:label_attachment_new)%>
+    <%= image_to_function 'add.png', 'addFileField();return false;' %></label>
+    <%= file_field_tag 'attachments[]', :size => 30  %> <em>(<%= l(:label_max_size) %>: <%= number_to_human_size(Setting.attachment_max_size.to_i.kilobytes) %>)</em></p>
+    </fieldset>
+    </div>
+    
+    <%= f.hidden_field :lock_version %>
+    <%= submit_tag l(:button_submit) %>
+    <%= link_to_remote l(:label_preview), 
+                       { :url => { :controller => 'issues', :action => 'preview', :id => @issue },
+                         :method => 'post',
+                         :update => 'preview',
+                         :with => 'Form.serialize("issue-form")',
+                         :complete => "location.hash='preview'"
+                       }, :accesskey => accesskey(:preview) %>
+<% end %>
+
+<div id="preview" class="wiki"></div>
diff --git a/app/views/issues/_form.rhtml b/app/views/issues/_form.rhtml
index d11cea84c..6a4cd0f5f 100644
--- a/app/views/issues/_form.rhtml
+++ b/app/views/issues/_form.rhtml
@@ -1,6 +1,3 @@
-<%= error_messages_for 'issue' %>
-<div class="box">
-
 <% if @issue.new_record? %>
 <p><%= f.select :tracker_id, @project.trackers.collect {|t| [t.name, t.id]}, :required => true %></p>
 <%= observe_field :issue_tracker_id, :url => { :action => :new },
@@ -8,15 +5,17 @@
                                      :with => "Form.serialize('issue-form')" %>
 <% end %>
 
+<div id="issue_descr_fields" <%= 'style="display:none"' unless @issue.new_record? || @issue.errors.any? %>>
 <p><%= f.text_field :subject, :size => 80, :required => true %></p>
 <p><%= f.text_area :description, :required => true,
                    :cols => 60,
                    :rows => (@issue.description.blank? ? 10 : [[10, @issue.description.length / 50].max, 100].min),
                    :accesskey => accesskey(:edit),
                    :class => 'wiki-edit' %></p>
+</div>
 
 <div class="splitcontentleft">
-<% if @issue.new_record? %>
+<% if @issue.new_record? || @allowed_statuses %>
 <p><%= f.select :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), :required => true %></p>
 <% else %>
 <p><label><%= l(:field_status) %></label> <%= @issue.status.name %></p>
@@ -49,7 +48,6 @@
 <%= image_to_function "add.png", "addFileField();return false" %></label>
 <%= file_field_tag 'attachments[]', :size => 30  %> <em>(<%= l(:label_max_size) %>: <%= number_to_human_size(Setting.attachment_max_size.to_i.kilobytes) %>)</em></p>
 <% end %>
-</div>
 
 <%= wikitoolbar_for 'issue_description' %>
 
diff --git a/app/views/issues/_form_update.rhtml b/app/views/issues/_form_update.rhtml
new file mode 100644
index 000000000..25e81a7fd
--- /dev/null
+++ b/app/views/issues/_form_update.rhtml
@@ -0,0 +1,10 @@
+<div class="splitcontentleft">
+<p><%= f.select :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), :required => true %></p>
+<p><%= f.select :assigned_to_id, (@issue.assignable_users.collect {|m| [m.name, m.id]}), :include_blank => true %></p>
+</div>
+<div class="splitcontentright">
+<p><%= f.select :done_ratio, ((0..10).to_a.collect {|r| ["#{r*10} %", r*10] }) %></p>
+<%= content_tag('p', f.select(:fixed_version_id, 
+                          (@project.versions.sort.collect {|v| [v.name, v.id]}),
+                          { :include_blank => true })) unless @project.versions.empty? %>
+</div>
diff --git a/app/views/issues/_update.rhtml b/app/views/issues/_update.rhtml
deleted file mode 100644
index 49d1473d9..000000000
--- a/app/views/issues/_update.rhtml
+++ /dev/null
@@ -1,54 +0,0 @@
-<% labelled_tabular_form_for(:issue, @issue, :url => {:action => 'update', :id => @issue},
-                                             :html => {:multipart => true,
-                                                       :id => 'issue-form'}) do |f| %>
-
-<div class="box">
-<% unless @status_options.empty? %>
-<%= f.hidden_field :lock_version %>
-<fieldset><legend><%= l(:label_change_properties) %></legend>
-    <div class="splitcontentleft">
-    <p><%= f.select :status_id, (@status_options.collect {|p| [p.name, p.id]}), :required => true %></p>
-    <p><%= f.select :assigned_to_id, (@issue.assignable_users.collect {|m| [m.name, m.id]}), :include_blank => true %></p>
-    </div>
-    <div class="splitcontentright">
-    <p><%= f.select :done_ratio, ((0..10).to_a.collect {|r| ["#{r*10} %", r*10] }) %></p>
-    <p><%= f.select :fixed_version_id, (@project.versions.sort.collect {|v| [v.name, v.id]}), { :include_blank => true } %></p>
-    </div>
-</fieldset>
-<% end%>
-<% if authorize_for('timelog', 'edit') %>
-<fieldset><legend><%= l(:button_log_time) %></legend>
-    <% fields_for :time_entry, @time_entry, { :builder => TabularFormBuilder, :lang => current_language} do |time_entry| %>
-    <div class="splitcontentleft">
-    <p><%= time_entry.text_field :hours, :size => 6, :label => :label_spent_time %> <%= l(:field_hours) %></p>
-    </div>
-    <div class="splitcontentright">
-    <p><%= time_entry.text_field :comments, :size => 40 %></p>
-    <p><%= time_entry.select :activity_id, (@activities.collect {|p| [p.name, p.id]}) %></p>
-    </div>
-    <% end %>
-</fieldset>
-<% end %>
-
-<fieldset><legend><%= l(:field_notes) %></legend>
-<%= text_area_tag 'notes', @notes, :cols => 60, :rows => 10, :class => 'wiki-edit' %>
-<%= wikitoolbar_for 'notes' %>
-
-<p id="attachments_p"><label><%=l(:label_attachment_new)%>
-<%= image_to_function "add.png", "addFileField();return false" %></label>
-<%= file_field_tag 'attachments[]', :size => 30  %> <em>(<%= l(:label_max_size) %>: <%= number_to_human_size(Setting.attachment_max_size.to_i.kilobytes) %>)</em></p>
-</fieldset>
-</div>
-
-<%= submit_tag l(:button_submit) %>
-<%= link_to_remote l(:label_preview), 
-                   { :url => { :controller => 'issues', :action => 'preview', :id => @issue },
-                     :method => 'post',
-                     :update => 'preview',
-                     :with => "Form.serialize('issue-form')",
-                     :complete => "window.location.hash='preview'"
-                   }, :accesskey => accesskey(:preview) %> |
-<%= toggle_link l(:button_cancel), 'update' %>
-<% end %>
-
-<div id="preview" class="wiki"></div>
diff --git a/app/views/issues/context_menu.rhtml b/app/views/issues/context_menu.rhtml
index 9691a7713..46b177067 100644
--- a/app/views/issues/context_menu.rhtml
+++ b/app/views/issues/context_menu.rhtml
@@ -6,7 +6,7 @@
 		<a href="#" class="submenu" onclick="return false;"><%= l(:field_status) %></a>
 		<ul>
 		<% @statuses.each do |s| %>
-		    <li><%= context_menu_link s.name, {:controller => 'issues', :action => 'update', :id => @issue, :issue => {:status_id => s}},
+		    <li><%= context_menu_link s.name, {:controller => 'issues', :action => 'edit', :id => @issue, :issue => {:status_id => s}},
 		                              :selected => (s == @issue.status), :disabled => !(@allowed_statuses.include?(s)) %></li>
 		<% end %>
 		</ul>
@@ -24,10 +24,10 @@
 		<a href="#" class="submenu"><%= l(:field_assigned_to) %></a>
 		<ul>
 		<% @assignables.each do |u| %>
-		    <li><%= context_menu_link u.name, {:controller => 'issues', :action => 'update', :id => @issue, :issue => {:assigned_to_id => u}, :back_to => back_to}, :method => :post,
+		    <li><%= context_menu_link u.name, {:controller => 'issues', :action => 'edit', :id => @issue, :issue => {:assigned_to_id => u}, :back_to => back_to}, :method => :post,
 		                              :selected => (u == @issue.assigned_to), :disabled => !@can[:assign] %></li>
 		<% end %>
-		    <li><%= context_menu_link l(:label_nobody), {:controller => 'issues', :action => 'update', :id => @issue, :issue => {:assigned_to_id => nil}, :back_to => back_to}, :method => :post,
+		    <li><%= context_menu_link l(:label_nobody), {:controller => 'issues', :action => 'edit', :id => @issue, :issue => {:assigned_to_id => nil}, :back_to => back_to}, :method => :post,
 		                              :selected => @issue.assigned_to.nil?, :disabled => !@can[:assign] %></li>
 		</ul>
 	</li>
diff --git a/app/views/issues/edit.rhtml b/app/views/issues/edit.rhtml
index 1577216ed..97f26a205 100644
--- a/app/views/issues/edit.rhtml
+++ b/app/views/issues/edit.rhtml
@@ -1,19 +1,3 @@
 <h2><%=h "#{@issue.tracker.name} ##{@issue.id}" %></h2>
 
-<% labelled_tabular_form_for :issue, @issue,
-                             :url => {:action => 'edit'},
-                             :html => {:id => 'issue-form'} do |f| %>
-    <%= render :partial => 'form', :locals => {:f => f} %>
-    <%= f.hidden_field :lock_version %>
-    <%= submit_tag l(:button_save) %>
-    <%= link_to_remote l(:label_preview), 
-                       { :url => { :controller => 'issues', :action => 'preview', :id => @issue },
-                         :method => 'post',
-                         :update => 'preview',
-                         :with => "Form.serialize('issue-form')",
-                         :complete => "location.href='#preview-top'"
-                       }, :accesskey => accesskey(:preview) %>
-<% end %>
-
-<a name="preview-top"></a>
-<div id="preview" class="wiki"></div>
+<%= render :partial => 'edit' %>
diff --git a/app/views/issues/new.rhtml b/app/views/issues/new.rhtml
index 8ff07f226..1e9e323fe 100644
--- a/app/views/issues/new.rhtml
+++ b/app/views/issues/new.rhtml
@@ -2,7 +2,10 @@
 
 <% labelled_tabular_form_for :issue, @issue, 
                              :html => {:multipart => true, :id => 'issue-form'} do |f| %>
+    <%= error_messages_for 'issue' %>
+    <div class="box">
     <%= render :partial => 'issues/form', :locals => {:f => f} %>
+    </div>
     <%= submit_tag l(:button_create) %>
     <%= link_to_remote l(:label_preview), 
                        { :url => { :controller => 'issues', :action => 'preview', :id => @issue },
diff --git a/app/views/issues/show.rhtml b/app/views/issues/show.rhtml
index d29b1b88f..a16dc60e0 100644
--- a/app/views/issues/show.rhtml
+++ b/app/views/issues/show.rhtml
@@ -1,6 +1,5 @@
 <div class="contextual">
-<%= show_and_goto_link(l(:button_update), 'update', :class => 'icon icon-note') if authorize_for('issues', 'update') %>
-<%= link_to_if_authorized l(:button_edit), {:controller => 'issues', :action => 'edit', :id => @issue}, :class => 'icon icon-edit', :accesskey => accesskey(:edit) %>
+<%= show_and_goto_link(l(:button_update), 'update', :class => 'icon icon-edit', :accesskey => accesskey(:edit)) if authorize_for('issues', 'edit') %>
 <%= link_to_if_authorized l(:button_log_time), {:controller => 'timelog', :action => 'edit', :issue_id => @issue}, :class => 'icon icon-time' %>
 <%= watcher_tag(@issue, User.current) %>
 <%= link_to_if_authorized l(:button_copy), {:controller => 'issues', :action => 'new', :project_id => @project, :copy_from => @issue }, :class => 'icon icon-copy' %>
@@ -89,11 +88,11 @@ end %>
 </div>
 <% end %>
 
-<% if authorize_for('issues', 'update') %>
+<% if authorize_for('issues', 'edit') %>
   <a name="update-anchor"></a>
   <div id="update" style="display:none;">
   <h3><%= l(:button_update) %></h3>
-  <%= render :partial => 'update' %>
+  <%= render :partial => 'edit' %>
   </div>
 <% end %>
 
diff --git a/app/views/issues/update.rhtml b/app/views/issues/update.rhtml
deleted file mode 100644
index 44e72da87..000000000
--- a/app/views/issues/update.rhtml
+++ /dev/null
@@ -1,4 +0,0 @@
-<h2><%= @issue.tracker.name %> #<%= @issue.id %>: <%=h @issue.subject %></h2>
-
-<%= error_messages_for 'issue' %>
-<%= render :partial => 'update' %>