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

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)),

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'

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

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

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 %>

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 %>

app/views/issues/change_status.rhtml

@@ -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 %>

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},

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 %>
 

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' %>