enables logging time on update

app/controllers/work_packages_controller.rb

@@ -120,7 +120,7 @@ class WorkPackagesController < ApplicationController
   end
 
   def update
-    work_package.update_by(current_user, permitted_params.update_work_package)
+    work_package.update_by(current_user, permitted_params.update_work_package(:project => project))
 
     flash[:notice] = l(:notice_successful_update)
 

app/helpers/work_packages_helper.rb

@@ -187,8 +187,7 @@ module WorkPackagesHelper
 
   def work_package_show_spent_time_attribute(work_package)
     work_package_show_table_row(:spent_time) do
-      #This check can be removed as soon as spent_hours is part of work_package and not Issue
+      work_package.spent_hours > 0 ?
-      work_package.respond_to?(:spent_hours) && work_package.spent_hours > 0 ?
         link_to(l_hours(work_package.spent_hours), issue_time_entries_path(work_package)) :
         "-"
     end

app/models/issue.rb

@@ -421,15 +421,6 @@ class Issue < WorkPackage
     notified.collect(&:mail)
   end
 
-  # Returns the total number of hours spent on this issue and its descendants
-  #
-  # Example:
-  #   spent_hours => 0.0
-  #   spent_hours => 50.2
-  def spent_hours
-    @spent_hours ||= self_and_descendants.joins(:time_entries).sum("#{TimeEntry.table_name}.hours").to_f || 0.0
-  end
-
   # Returns the time scheduled for this issue.
   #
   # Example:

app/models/permitted_params.rb

@@ -87,8 +87,8 @@ class PermittedParams < Struct.new(:params, :user)
     params[:work_package].permit(*permitted)
   end
 
-  def update_work_package
+  def update_work_package(args = {})
-    permitted = permitted_attributes(:new_work_package)
+    permitted = permitted_attributes(:new_work_package, args)
 
     params[:work_package].permit(*permitted)
   end
@@ -127,7 +127,7 @@ class PermittedParams < Struct.new(:params, :user)
   protected
 
   def permitted_attributes(key, additions = {})
-    merged_args = { :user => user }.merge(additions)
+    merged_args = { :params => params, :user => user }.merge(additions)
 
     self.class.permitted_attributes[:new_work_package].map do |permission|
       if permission.respond_to?(:call)
@@ -160,9 +160,20 @@ class PermittedParams < Struct.new(:params, :user)
                                                      :notes,
                                                      { attachments: [:file, :description] },
                                                      Proc.new do |args|
-                                                       args[:user].allowed_to?(:add_work_package_watchers, args[:project]) ?
+                                                       # avoid costly allowed_to? if the param is not there at all
-                                                         { :watcher_user_ids => [] } :
+                                                       if args[:params]["work_package"].has_key?("watcher_user_ids") &&
-                                                         nil
+                                                          args[:user].allowed_to?(:add_work_package_watchers, args[:project])
+
+                                                         { :watcher_user_ids => [] }
+                                                       end
+                                                     end,
+                                                     Proc.new do |args|
+                                                       # avoid costly allowed_to? if the param is not there at all
+                                                       if args[:params]["work_package"].has_key?("time_entry") &&
+                                                          args[:user].allowed_to?(:log_time, args[:project])
+
+                                                         { time_entry: [:hours, :activity_id, :comments] }
+                                                       end
                                                      end
                                                    ],
                                :color_move => [:move_to],

app/models/work_package.rb

@@ -272,6 +272,12 @@ class WorkPackage < ActiveRecord::Base
 
     raw_attachments = attributes.delete(:attachments)
 
+    if log_attributes = attributes.delete(:time_entry)
+      log_attributes = { :user => user,
+                         :spent_on => Date.today }.merge(log_attributes)
+      time_entries.build(log_attributes)
+    end
+
     if update_attributes(attributes)
       attachments = Attachment.attach_files(self, raw_attachments)
     end
@@ -331,4 +337,13 @@ class WorkPackage < ActiveRecord::Base
     Setting.issue_done_ratio == 'issue_status'
   end
 
+  # Returns the total number of hours spent on this issue and its descendants
+  #
+  # Example:
+  #   spent_hours => 0.0
+  #   spent_hours => 50.2
+  def spent_hours
+    @spent_hours ||= self_and_descendants.joins(:time_entries)
+                                         .sum("#{TimeEntry.table_name}.hours").to_f || 0.0
+  end
 end

app/views/work_packages/_edit.html.erb

@@ -54,7 +54,8 @@ See doc/COPYRIGHT.rdoc for more details.
         </legend>
 
         <%= render :partial => 'time_entry',
-                   :locals => { :time_entry => time_entry } %>
+                   :locals => { :time_entry => time_entry,
+                                :f => f } %>
       </fieldset>
     <% end %>
 

app/views/work_packages/_time_entry.html.erb

@@ -10,15 +10,14 @@ See doc/COPYRIGHT.rdoc for more details.
 
 ++#%>
 
-<%= fields_for :time_entry,
+<%= f.fields_for :time_entry,
-               time_entry,
+                 time_entry,
-               { :builder => TabularFormBuilder,
+                 { :builder => TabularFormBuilder,
                  :lang => current_language} do |fields| %>
 
   <div class="splitcontentleft">
     <p>
       <%= fields.text_field :hours,
-                            :size => 6,
                             :label => :label_spent_time %>
       <%= TimeEntry.human_attribute_name(:hours) %>
     </p>
@@ -31,7 +30,7 @@ See doc/COPYRIGHT.rdoc for more details.
   </div>
 
   <p>
-    <%= fields.text_field :comments, :size => 60 %>
+    <%= fields.text_field :comments %>
   </p>
   <% time_entry.custom_field_values.each do |value| %>
     <p>

features/work_packages/log_time_on_update.feature

@@ -0,0 +1,47 @@
+#-- copyright
+#
+# OpenProject is a project management system.
+#
+# Copyright (C) 2012-2013 the OpenProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+#
+Feature: Logging time on work package update
+  Background:
+    Given there is 1 user with:
+      | login     | manager |
+      | firstname | the     |
+      | lastname  | manager |
+    And there is 1 project with the following:
+      | identifier | ecookbook |
+      | name       | ecookbook |
+    And there is a role "manager"
+    And the role "manager" may have the following rights:
+      | edit_work_packages |
+      | view_work_packages |
+      | log_time           |
+    And I am working in project "ecookbook"
+    And the user "manager" is a "manager"
+    And there are the following status:
+      | name    | default |
+      | status1 | true    |
+    And there are the following planning elements in project "ecookbook":
+      | subject |
+      | pe1     |
+    And there is an activity "design"
+    And I am already logged in as "manager"
+
+  Scenario: Logging time
+    When I go to the edit page of the work package called "pe1"
+     And I fill in the following:
+       | Spent time | 5         |
+       | Activity   | design    |
+       | Comment    | Needed it |
+     And I submit the form by the "Submit" button
+
+    Then the work package should be shown with the following values:
+       | Spent time | 5.00      |

features/work_packages/update.feature

@@ -1,3 +1,15 @@
+#-- copyright
+#
+# OpenProject is a project management system.
+#
+# Copyright (C) 2012-2013 the OpenProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
 Feature: Updating work packages
   Background:
     Given there is 1 user with:

spec/controllers/work_packages_controller_spec.rb

@@ -389,7 +389,9 @@ describe WorkPackagesController do
 
       before do
         controller.stub!(:work_package).and_return(planning_element)
-        controller.send(:permitted_params).should_receive(:update_work_package).and_return(wp_params)
+        controller.send(:permitted_params).should_receive(:update_work_package)
+                                          .with(:project => planning_element.project)
+                                          .and_return(wp_params)
         planning_element.should_receive(:update_by).with(current_user, wp_params).and_return(true)
       end
 

spec/helpers/work_packages_helper_spec.rb

@@ -62,6 +62,26 @@ describe WorkPackagesHelper do
     end
   end
 
+  describe :work_package_show_spent_time_attribute do
+    it "should show a spent time link pointing to the time entries of the work package" do
+      stub_work_package.stub(:spent_hours).and_return(5.0)
+
+      field = helper.work_package_show_spent_time_attribute(stub_work_package).field
+
+      expected_href = issue_time_entries_path(stub_work_package)
+
+      field.should have_css(".spent-time a[@href='#{ expected_href }']", :text => '5.0')
+    end
+
+    it "should show a '-' if spent time is 0" do
+      stub_work_package.stub(:spent_hours).and_return(0.0)
+
+      field = helper.work_package_show_spent_time_attribute(stub_work_package).field
+
+      field.should have_css(".spent-time", :text => '-')
+    end
+  end
+
   describe :work_package_form_issue_category_attribute do
     let(:stub_project) { FactoryGirl.build_stubbed(:project) }
     let(:stub_category) { FactoryGirl.build_stubbed(:issue_category) }

spec/models/permitted_params_spec.rb

@@ -555,6 +555,28 @@ describe PermittedParams do
 
       PermittedParams.new(params, user).update_work_package.should == hash
     end
+
+    it "should permit time_entry if the user has the log_time permission" do
+      hash = { "time_entry" => { "hours" => "5", "activity_id" => "1", "comments" => "lorem" } }
+
+      project = double('project')
+      user.stub(:allowed_to?).with(:log_time, project).and_return(true)
+
+      params = ActionController::Parameters.new(:work_package => hash)
+
+      PermittedParams.new(params, user).update_work_package(:project => project).should == hash
+    end
+
+    it "should not permit time_entry if the user lacks the log_time permission" do
+      hash = { "time_entry" => { "hours" => "5", "activity_id" => "1", "comments" => "lorem" } }
+
+      project = double('project')
+      user.stub(:allowed_to?).with(:log_time, project).and_return(false)
+
+      params = ActionController::Parameters.new(:work_package => hash)
+
+      PermittedParams.new(params, user).update_work_package(:project => project).should == {}
+    end
   end
 
   describe :user do

spec/models/work_package_spec.rb

@@ -144,6 +144,48 @@ describe WorkPackage do
 
           instance.update_by(user, { :attachments => raw_attachments })
         end
+
+        it "should only attach the attachment when saving was successful" do
+          raw_attachments = [double('attachment')]
+          attachment = FactoryGirl.build(:attachment)
+
+          Attachment.should_not_receive(:attach_files)
+
+          instance.update_by(user, { :subject => "", :attachments => raw_attachments })
+        end
+
+        it "should add a time entry" do
+          activity = FactoryGirl.create(:time_entry_activity)
+
+          instance.update_by(user, { :time_entry => { "hours" => "5",
+                                                      "activity_id" => activity.id.to_s,
+                                                      "comments" => "blubs" } } )
+
+          instance.should have(1).time_entries
+
+          entry = instance.time_entries.first
+
+          entry.should be_persisted
+          entry.work_package.should == instance
+          entry.user.should == user
+          entry.project.should == instance.project
+          entry.spent_on.should == Date.today
+        end
+
+        it "should not persist the time entry if the #{subclass}'s update fails" do
+          activity = FactoryGirl.create(:time_entry_activity)
+
+          instance.update_by(user, { :subject => '',
+                                     :time_entry => { "hours" => "5",
+                                                      "activity_id" => activity.id.to_s,
+                                                      "comments" => "blubs" } } )
+
+          instance.should have(1).time_entries
+
+          entry = instance.time_entries.first
+
+          entry.should_not be_persisted
+        end
       end
     end
   end