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.

I have attached two patches.

The first patch fixes add_working_days. It first advances the date by whole weeks and then counts the remaining working days one by one. The cause of the bug is that, when the number of working days is a multiple of the working days per week, no working days remain to be counted, so the result falls on the same day of the week as the given date. If the given date is a non-working day, next_working_date then moves the result to the following week.

To fix this, the patch always leaves at least one working day to be counted one by one, so that the result is always a working day. Unlike the earlier patch, it does not advance one day at a time, so the calculation stays fast. The tests are taken from fix-29855.patch by Mizuki ISHIKAWA.

The second patch makes working_days consistent with add_working_days, as Jean-Philippe suggested in #note-8. The cause of the inconsistency is that working_days(from, to) counts the working days from from up to the day before to, while add_working_days counts the working days after the given date. For example, add_working_days(Sat 2012-10-20, 5) returns Fri 2012-10-26, but working_days(Sat 2012-10-20, Fri 2012-10-26) returns 4.

To fix this, working_days(from, to) now counts the working days after from, up to and including to, and the example above returns 5. Regarding #note-10, the end date is now counted, but the start date is not, in the same way as Date#-. Without non-working days, the result is the same as to - from. Therefore, working_days(Tue 2012-10-09, Mon 2012-10-15) is still 4.

The second patch also changes Issue#working_duration, because the change to working_days alone would cause a regression when an issue is rescheduled.

Like Issue#duration, working_duration does not count the first day of an issue. For example, both return 4 for an issue from Mon 2012-10-15 to Fri 2012-10-19. Therefore, two issues that cover the same working days should have the same working_duration. With the new working_days alone, this is not the case, because it counts Monday for an issue that starts on Saturday.

Issue#reschedule_on calls working_duration, moves the new start date to the next working day, and then adds the result of working_duration to it. The table below shows the result of rescheduling the issue from Sat 2012-10-13 to Fri 2012-10-19 to Mon 2012-10-22.

To fix this, working_duration now counts from the first working day on or after the start date, in the same way as reschedule_on.

As a result, the expected values of six existing assertions for working_days in test/unit/lib/redmine/utils/date_calculation_test.rb change, as shown below. In all of them, Saturday and Sunday are non-working days. The expected values of the existing tests for Issue#reschedule_on do not change.

The values marked "incorrect" were not consistent. For example, Tue 2012-10-09 to Fri 2012-10-12 was 3, but Tue 2012-10-09 to Sun 2012-10-14 was 4, although both periods include the same four working days. With this patch, both are 3.

The values marked "new definition" change because working_days now counts from the day after the start date, as described above. The previous values were correct under the previous definition.

In Redmine itself, working_days is used only by Issue#working_duration, which is used only by Issue#reschedule_on. As described above, working_duration is changed to match the new definition, so rescheduling issues works correctly. However, plugins that call working_days directly may get different results.

Setting the target version to 7.1.0.