--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
The suggested fix works fine but it is much slower than the current code. I think we need to consider whether this will affect the performance of Redmine.

<pre>
$ bin/rails r bench-29855.rb
Warming up --------------------------------------
before 12.236k i/100ms
after 997.000 i/100ms
Calculating -------------------------------------
before 159.524k (± 4.7%) i/s - 807.576k in 5.073660s
after 10.597k (± 3.4%) i/s - 53.838k in 5.086474s

Comparison:
before: 159524.1 i/s
after: 10597.0 i/s - 15.05x slower
</pre>

<pre><code class="ruby">
require 'benchmark/ips'

include Redmine::Utils::DateCalculation

Benchmark.ips do |x|
x.report('before') do
add_working_days(Date.today, 30)
end

x.report('after') do
result = Date.today
30.times do
result = next_working_date(result + 1)
end
result
end

x.compare!
end
</code></pre>
--------------------------------------------------------------------------------
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:

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

</pre>
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
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:
<pre><code diff="ruby">
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
</code></pre>

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

--------------------------------------------------------------------------------
Marius BALTEANU wrote:

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

@#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#+@.

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

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

relates,Closed,14846,Calculation of the start date of following issues ignores the "non-working days" setting