Redmine

I have confirmed that 3.3-stable and 3.4-stable are also affected.

I think that applying this patch will solve the problem.
The code of the add_working_days method changes quite a bit, but all the tests succeed.

Any feedback is welcome.

Jean-Philippe, do you think we can accept this performance deterioration?

I think it is OK because 'add_working_days' method will not be executed hundreds of times by the user's single operation. So, it does not affect the performance of Redmine.

Mizuki ISHIKAWA wrote:

Any feedback is welcome.

DateCalculation#working_days should be fixed in a similar way to be consistent with the proposed fix. These new assertions should pass:

Index: test/unit/lib/redmine/utils/date_calculation.rb
===================================================================
--- test/unit/lib/redmine/utils/date_calculation.rb    (revision 17671)
+++ test/unit/lib/redmine/utils/date_calculation.rb    (working copy)
@@ -41,6 +41,8 @@
       assert_working_days  8, '2012-10-11', '2012-10-23'
       assert_working_days  2, '2012-10-14', '2012-10-17'
       assert_working_days 11, '2012-10-14', '2012-10-30'
+      assert_working_days  5, '2012-10-20', '2012-10-26'
+      assert_working_days  5, '2012-10-21', '2012-10-26'
     end
   end

I took a look and there are some strange (or wrong) test cases the we should review before changing anything else.

Taking the following test scenario:

  def test_working_days_with_non_working_week_days
    with_settings :non_working_week_days => %w(6 7) do
      assert_working_days 14, '2012-10-09', '2012-10-27'
      assert_working_days  4, '2012-10-09', '2012-10-15'
      assert_working_days  4, '2012-10-09', '2012-10-14'
      assert_working_days  3, '2012-10-09', '2012-10-12'
      assert_working_days  8, '2012-10-09', '2012-10-19'
      assert_working_days  8, '2012-10-11', '2012-10-23'
      assert_working_days  2, '2012-10-14', '2012-10-17'
      assert_working_days 11, '2012-10-14', '2012-10-30'
    end
  end

assert_working_days 4, '2012-10-09', '2012-10-15'
2012-10-09 was Tuesday
2012-10-15 was Monday
The number of the expected working days according to the test is 4. But in my opinion, it should be 5 days (Tuesday, Wednesday, Thursday, Friday and Monday). 4 could be only if we exclude the end date from the count. if we do this, than the number of the expected days for the 2 assertions proposed by Jean-Philippe should be 4 because we need to exclude Friday (2012-10-26).

Also, it sound incorrect to say that between '2012-10-09 - 2012-10-15 (Tuesday - Monday)' and '2012-10-09 - 2012-10-14 (Tuesday - Sunday)' are the same number of working days (4).

Jean-Philippe, what do you think? I'm in favour of including the end date in the count.

Jean-Philippe Lang wrote:

#working_days and #add_working_days are used to reschedule an issue when the start date is changed. Its duration is calculated with #working_days and the new due date is calculated with #add_working_days. If there is no "non working day", they should behave like Date#- and Date#+.

Thanks, but are still not clear for me the expected results so I'll leave Go Maeda or Mizuki ISHIKAWA to fix this issue.