Merged IssuesController change_status and add_note actions.

The 'Change status' specific form removed and now accessible from issue/show view with no additional request (click on 'Update' to show the form).
The 'Change issue status' permission is removed. To change the status, the user just needs to have either 'Edit' or 'Add note' permissions and some workflow transitions allowed.

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

10 files changed, 109 insertions, 112 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 39a1e2d13..11112289e 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -21,7 +21,7 @@ class IssuesController < ApplicationController
   before_filter :find_optional_project, :only => [:index, :changes]
   accept_key_auth :index, :changes
   
-  cache_sweeper :issue_sweeper, :only => [ :edit, :change_status, :destroy ]
+  cache_sweeper :issue_sweeper, :only => [ :edit, :update, :destroy ]
 
   helper :projects
   include ProjectsHelper   
@@ -82,7 +82,8 @@ class IssuesController < ApplicationController
   def show
     @custom_values = @issue.custom_values.find(:all, :include => :custom_field, :order => "#{CustomField.table_name}.position")
     @journals = @issue.journals.find(:all, :include => [:user, :details], :order => "#{Journal.table_name}.created_on ASC")
-    @status_options = @issue.status.find_new_statuses_allowed_to(User.current.role_for_project(@project), @issue.tracker)
+    @status_options = @issue.new_statuses_allowed_to(User.current)
+    @activities = Enumeration::get_values('ACTI')
     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") }
@@ -115,47 +116,42 @@ class IssuesController < ApplicationController
     end		
   end
   
-  def add_note
+  # 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])
-    attachments = attach_files(@issue, params[:attachments])
-    attachments.each {|a| journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
-    if journal.save
-      flash[:notice] = l(:notice_successful_update)
-      Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
-      redirect_to :action => 'show', :id => @issue
-      return
+    # 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
-    show
-  end
-
-  def change_status
-    @status_options = @issue.status.find_new_statuses_allowed_to(User.current.role_for_project(@project), @issue.tracker)
-    @new_status = IssueStatus.find(params[:new_status_id])
-    if params[:confirm]
-      begin
-        journal = @issue.init_journal(User.current, params[:notes])
-        @issue.status = @new_status
-        if @issue.update_attributes(params[:issue])
-          attachments = attach_files(@issue, params[:attachments])
-          attachments.each {|a| journal.details << JournalDetail.new(:property => 'attachment', :prop_key => a.id, :value => a.filename)}
-          # Log time
-          if current_role.allowed_to?(:log_time)
-            @time_entry ||= TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
-            @time_entry.attributes = params[:time_entry]
-            @time_entry.save
-          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
+        # Log spend time
+        if current_role.allowed_to?(:log_time)
+          @time_entry = TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => Date.today)
+          @time_entry.attributes = params[:time_entry]
+          @time_entry.save
+        end
+        if !journal.new_record?
+          # Only send notification if something was actually changed
           flash[:notice] = l(:notice_successful_update)
           Mailer.deliver_issue_edit(journal) if Setting.notified_events.include?('issue_updated')
-          redirect_to :action => 'show', :id => @issue
         end
-      rescue ActiveRecord::StaleObjectError
-        # Optimistic locking exception
-        flash[:error] = l(:notice_locking_conflict)
+        redirect_to(params[:back_to] || {:action => 'show', :id => @issue})
       end
-    end    
-    @assignable_to = @project.members.find(:all, :include => :user).collect{ |m| m.user }
-    @activities = Enumeration::get_values('ACTI')
+    end
+  rescue ActiveRecord::StaleObjectError
+    # Optimistic locking exception
+    flash.now[:error] = l(:notice_locking_conflict)
   end
 
   def destroy
@@ -177,11 +173,11 @@ class IssuesController < ApplicationController
   def context_menu
     @priorities = Enumeration.get_values('IPRI').reverse
     @statuses = IssueStatus.find(:all, :order => 'position')
-    @allowed_statuses = @issue.status.find_new_statuses_allowed_to(User.current.role_for_project(@project), @issue.tracker)
+    @allowed_statuses = @issue.new_statuses_allowed_to(User.current)
     @assignables = @issue.assignable_users
     @assignables << @issue.assigned_to if @issue.assigned_to && !@assignables.include?(@issue.assigned_to)
     @can = {:edit => User.current.allowed_to?(:edit_issues, @project),
-            :change_status => User.current.allowed_to?(:change_issue_status, @project),
+            :assign => (@allowed_statuses.any? || User.current.allowed_to?(:edit_issues, @project)),
             :add => User.current.allowed_to?(:add_issues, @project),
             :move => User.current.allowed_to?(:move_issues, @project),
             :copy => (@project.trackers.include?(@issue.tracker) && User.current.allowed_to?(:add_issues, @project)),
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index a58eef719..2ae742dfa 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -252,11 +252,9 @@ class ProjectsController < ApplicationController
       redirect_to :controller => 'issues', :action => 'index', :project_id => @project
       return
     end
-    if current_role && User.current.allowed_to?(:change_issue_status, @project)
-      # Find potential statuses the user could be allowed to switch issues to
-      @available_statuses = Workflow.find(:all, :include => :new_status,
-                                                :conditions => {:role_id => current_role.id}).collect(&:new_status).compact.uniq
-    end
+    # Find potential statuses the user could be allowed to switch issues to
+    @available_statuses = Workflow.find(:all, :include => :new_status,
+                                              :conditions => {:role_id => current_role.id}).collect(&:new_status).compact.uniq
     render :update do |page|
       page.hide 'query_form'
       page.replace_html  'bulk-edit', :partial => 'issues/bulk_edit_form'
diff --git a/app/models/issue.rb b/app/models/issue.rb
index f7b01ea6a..419c6cdc7 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -180,6 +180,13 @@ class Issue < ActiveRecord::Base
     project.assignable_users
   end
   
+  # Returns an array of status that user is able to apply
+  def new_statuses_allowed_to(user)
+    statuses = status.find_new_statuses_allowed_to(user.role_for_project(project), tracker)
+    statuses << status unless statuses.empty?
+    statuses.uniq.sort
+  end
+  
   # Returns the mail adresses of users that should be notified for the issue
   def recipients
     recipients = project.recipients
diff --git a/app/models/issue_status.rb b/app/models/issue_status.rb
index a5d228405..ddff9c005 100644
--- a/app/models/issue_status.rb
+++ b/app/models/issue_status.rb
@@ -56,6 +56,10 @@ class IssueStatus < ActiveRecord::Base
       false
   end
 
+  def <=>(status)
+    position <=> status.position
+  end
+  
   def to_s; name end
 
 private
diff --git a/app/views/issues/_bulk_edit_form.rhtml b/app/views/issues/_bulk_edit_form.rhtml
index bc3f62e6d..e9e1cef86 100644
--- a/app/views/issues/_bulk_edit_form.rhtml
+++ b/app/views/issues/_bulk_edit_form.rhtml
@@ -2,7 +2,7 @@
 <fieldset class="box"><legend><%= l(:label_bulk_edit_selected_issues) %></legend>
 
 <p>
-<% if @available_statuses %>
+<% if @available_statuses.any? %>
 <label><%= l(:field_status) %>: 
 <%= select_tag('status_id', "<option value=\"\">#{l(:label_no_change_option)}</option>" + options_from_collection_for_select(@available_statuses, :id, :name)) %></label>
 <% end %>
diff --git a/app/views/issues/_update.rhtml b/app/views/issues/_update.rhtml
new file mode 100644
index 000000000..3cf593806
--- /dev/null
+++ b/app/views/issues/_update.rhtml
@@ -0,0 +1,42 @@
+<% labelled_tabular_form_for(:issue, @issue, :url => {:action => 'update', :id => @issue}, :html => {:multipart => true}) 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) %>
+<% end %>
diff --git a/app/views/issues/change_status.rhtml b/app/views/issues/change_status.rhtml
deleted file mode 100644
index a1e294556..000000000
--- a/app/views/issues/change_status.rhtml
+++ /dev/null
@@ -1,38 +0,0 @@
-<h2><%=l(:label_issue)%> #<%= @issue.id %>: <%=h @issue.subject %></h2>
-
-<%= error_messages_for 'issue' %>
-<% labelled_tabular_form_for(:issue, @issue, :url => {:action => 'change_status', :id => @issue}, :html => {:multipart => true}) do |f| %>
-
-<%= hidden_field_tag 'confirm', 1 %>
-<%= hidden_field_tag 'new_status_id', @new_status.id %>
-<%= f.hidden_field :lock_version %>
-
-<div class="box">
-<div class="splitcontentleft">
-<p><label><%=l(:label_issue_status_new)%></label> <%= @new_status.name %></p>
-<p><%= f.select :assigned_to_id, (@issue.assignable_users.collect {|m| [m.name, m.id]}), :include_blank => true %></p>
-<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>
-<div class="splitcontentright">
-<% if authorize_for('timelog', 'edit') %>
-<% fields_for :time_entry, @time_entry, { :builder => TabularFormBuilder, :lang => current_language} do |time_entry| %>
-<p><%= time_entry.text_field :hours, :size => 6, :label => :label_spent_time %> <%= l(:field_hours) %></p>
-<p><%= time_entry.text_field :comments, :size => 40 %></p>
-<p><%= time_entry.select :activity_id, (@activities.collect {|p| [p.name, p.id]}) %></p>
-<% end %>
-<% end %>
-</div>
-
-<div class="clear"></div>
-
-<p><label for="notes"><%= l(:field_notes) %></label>
-<%= text_area_tag 'notes', @notes, :cols => 60, :rows => 10, :class => 'wiki-edit' %></p>
-
-<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>
-</div>
-
-<%= submit_tag l(:button_save) %>
-<% end %>
diff --git a/app/views/issues/context_menu.rhtml b/app/views/issues/context_menu.rhtml
index 3af49fb04..0f11bb943 100644
--- a/app/views/issues/context_menu.rhtml
+++ b/app/views/issues/context_menu.rhtml
@@ -6,8 +6,8 @@
 		<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 => 'change_status', :id => @issue, :new_status_id => s},
-		                              :selected => (s == @issue.status), :disabled => !(@can[:change_status] && @allowed_statuses.include?(s)) %></li>
+		    <li><%= context_menu_link s.name, {:controller => 'issues', :action => 'update', :id => @issue, :issue => {:status_id => s}},
+		                              :selected => (s == @issue.status), :disabled => !(@allowed_statuses.include?(s)) %></li>
 		<% end %>
 		</ul>
 	</li>
@@ -24,11 +24,11 @@
 		<a href="#" class="submenu"><%= l(:field_assigned_to) %></a>
 		<ul>
 		<% @assignables.each do |u| %>
-		    <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[:edit] || @can[:change_status]) %></li>
+		    <li><%= context_menu_link u.name, {:controller => 'issues', :action => 'update', :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 => 'edit', :id => @issue, 'issue[assigned_to_id]' => '', :back_to => back_to}, :method => :post,
-		                              :selected => @issue.assigned_to.nil?, :disabled => !(@can[:edit] || @can[:change_status]) %></li>
+		    <li><%= context_menu_link l(:label_nobody), {:controller => 'issues', :action => 'update', :id => @issue, :issue => {:assigned_to_id => nil}, :back_to => back_to}, :method => :post,
+		                              :selected => @issue.assigned_to.nil?, :disabled => !@can[:assign] %></li>
 		</ul>
 	</li>
 	<li><%= context_menu_link l(:button_copy), {:controller => 'projects', :action => 'add_issue', :id => @project, :copy_from => @issue},
diff --git a/app/views/issues/show.rhtml b/app/views/issues/show.rhtml
index 91b638216..8cd44424c 100644
--- a/app/views/issues/show.rhtml
+++ b/app/views/issues/show.rhtml
@@ -1,5 +1,5 @@
 <div class="contextual">
-<%= show_and_goto_link(l(:label_add_note), 'add-note', :class => 'icon icon-note') if authorize_for('issues', 'add_note') %>
+<%= 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) %>
 <%= link_to_if_authorized l(:button_log_time), {:controller => 'timelog', :action => 'edit', :issue_id => @issue}, :class => 'icon icon-time' %>
 <%= watcher_tag(@issue, User.current) %>
@@ -81,16 +81,6 @@ end %>
 
 </div>
 
-<% if authorize_for('issues', 'change_status') and @status_options and !@status_options.empty? %>
-  <% form_tag({:controller => 'issues', :action => 'change_status', :id => @issue}) do %>
-  <p><%=l(:label_change_status)%> :
-  <select name="new_status_id">
-  <%= options_from_collection_for_select @status_options, "id", "name", @issue.status_id %>
-  </select>
-  <%= submit_tag l(:button_change) %></p>
-  <% end %>
-<% end %>
-
 <% if @journals.any? %>
 <div id="history">
 <h3><%=l(:label_history)%></h3>
@@ -98,18 +88,12 @@ end %>
 </div>
 <% end %>
 
-<% if authorize_for('issues', 'add_note') %>
-  <a name="add-note-anchor"></a>
-  <div id="add-note" class="box" style="display:none;">
-  <h3><%= l(:label_add_note) %></h3>
-  <% form_tag({:controller => 'issues', :action => 'add_note', :id => @issue}, :class => "tabular", :multipart => true) do %>
-  <p><label for="notes"><%=l(:field_notes)%></label>
-    <%= text_area_tag 'notes', '', :cols => 60, :rows => 10, :class => 'wiki-edit'  %></p>
-    <%= wikitoolbar_for 'notes' %>
-    <%= render :partial => 'attachments/form' %>
-  <%= submit_tag l(:button_add) %>
-  <%= toggle_link l(:button_cancel), 'add-note' %>
-  <% end %>  
+<% if authorize_for('issues', 'update') %>
+  <a name="update-anchor"></a>
+  <div id="update" style="display:none;">
+  <h3><%= l(:button_update) %></h3>
+  <%= render :partial => 'update' %>
+  <%= toggle_link l(:button_cancel), 'update' %>
   </div>
 <% end %>
 
diff --git a/app/views/issues/update.rhtml b/app/views/issues/update.rhtml
new file mode 100644
index 000000000..44e72da87
--- /dev/null
+++ b/app/views/issues/update.rhtml
@@ -0,0 +1,4 @@
+<h2><%= @issue.tracker.name %> #<%= @issue.id %>: <%=h @issue.subject %></h2>
+
+<%= error_messages_for 'issue' %>
+<%= render :partial => 'update' %>