Defect #29855
closed
add_working_days may return wrong date when the start date is a non-working day
Added by Yutaka Hara about 8 years ago. Updated 13 days ago.
Description
Redmine::Util::DateCalculation#add_working_days(date, n) returns wrong date when date is holiday and n is a multiple of 5.
Example:
irb(main):004:0> Setting.non_working_week_days => ["6", "7"] irb(main):001:0> include Redmine::Utils::DateCalculation irb(main):002:0> add_working_days(Date.new(2018, 10, 27), 5) => Mon, 05 Nov 2018 # Expected Fri, 02 Nov 2018 irb(main):003:0> add_working_days(Date.new(2018, 10, 28), 5) => Mon, 05 Nov 2018 # Expected Fri, 02 Nov 2018
Tested with trunk@17598
Files
| fix-29855.patch (1.64 KB) fix-29855.patch | Mizuki ISHIKAWA, 2018-11-07 07:48 | ||
| 0001-Fix-add_working_days-returning-a-date-one-working-da.patch (2.36 KB) 0001-Fix-add_working_days-returning-a-date-one-working-da.patch | Go MAEDA, 2026-10-14 10:18 | ||
| 0002-Make-working_days-consistent-with-add_working_days-w.patch (5.57 KB) 0002-Make-working_days-consistent-with-add_working_days-w.patch | Go MAEDA, 2026-10-14 10:18 |
Related issues
Updated by Go MAEDA about 8 years ago
- Related to Defect #14846: Calculation of the start date of following issues ignores the "non-working days" setting added
Updated by Go MAEDA about 8 years ago
- Description updated (diff)
- Status changed from New to Confirmed
- Affected version set to 3.3.7
I have confirmed that 3.3-stable and 3.4-stable are also affected.
Updated by Mizuki ISHIKAWA almost 8 years ago
- File fix-29855.patch fix-29855.patch added
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.
Updated by Go MAEDA almost 8 years ago
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.
$ 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
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
Updated by Go MAEDA almost 8 years ago
- Assignee set to Jean-Philippe Lang
- Target version set to 3.3.9
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.
Updated by Jean-Philippe Lang almost 8 years ago
- Assignee changed from Jean-Philippe Lang to Yutaka Hara
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
Updated by Jean-Philippe Lang almost 8 years ago
- Assignee changed from Yutaka Hara to Go MAEDA
Updated by Marius BĂLTEANU almost 8 years ago
- Assignee changed from Go MAEDA to Jean-Philippe Lang
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.
Updated by Jean-Philippe Lang almost 8 years ago
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#+.
Updated by Marius BĂLTEANU almost 8 years ago
- Assignee changed from Jean-Philippe Lang to Go MAEDA
Jean-Philippe Lang wrote:
#working_daysand#add_working_daysare used to reschedule an issue when the start date is changed. Its duration is calculated with#working_daysand the new due date is calculated with#add_working_days. If there is no "non working day", they should behave likeDate#-andDate#+.
Thanks, but are still not clear for me the expected results so I'll leave Go Maeda or Mizuki ISHIKAWA to fix this issue.
Updated by Jean-Philippe Lang almost 8 years ago
- Target version deleted (
3.3.9)
Updated by Go MAEDA almost 8 years ago
- Target version set to Candidate for next minor release
Updated by Go MAEDA 18 days ago
- File 0001-Fix-add_working_days-returning-a-date-one-working-da.patch 0001-Fix-add_working_days-returning-a-date-one-working-da.patch added
- File 0002-Make-working_days-consistent-with-add_working_days-w.patch 0002-Make-working_days-consistent-with-add_working_days-w.patch added
- Target version changed from Candidate for next minor release to Candidate for next major release
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 | Working days covered | working_duration (new working_days only) |
working_duration (this patch) |
|---|---|---|---|
| Mon 2012-10-15 to Fri 2012-10-19 | Mon to Fri | 4 | 4 |
| Sat 2012-10-13 to Fri 2012-10-19 | Mon to Fri | 5 (incorrect) | 4 |
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.
| Calculation | working_duration |
New due date | Working days covered |
|---|---|---|---|
Previous working_days |
4 | Fri 2012-10-26 | 5 |
New working_days only |
5 (incorrect) | Mon 2012-10-29 (incorrect) | 6 (incorrect) |
| This patch | 4 | Fri 2012-10-26 | 5 |
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.
| From | To | Before | After | Reason for the change |
|---|---|---|---|---|
| Tue 2012-10-09 | Sat 2012-10-27 | 14 (incorrect) | 13 | The end date is a non-working day. |
| Tue 2012-10-09 | Sun 2012-10-14 | 4 (incorrect) | 3 | The end date is a non-working day. |
| Tue 2012-10-09 | Sat 2012-10-27 | 13 (incorrect) | 12 | The end date is a non-working day. Wed 2012-10-10 is a registered non-working date. |
| Sun 2012-10-14 | Wed 2012-10-17 | 2 | 3 (new definition) | The start date is a non-working day. |
| Sun 2012-10-14 | Tue 2012-10-30 | 11 | 12 (new definition) | The start date is a non-working day. |
| Wed 2012-10-10 | Thu 2012-10-11 | 0 | 1 (new definition) | The start date is a registered non-working date. |
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.
Updated by Go MAEDA 13 days ago
- Status changed from Confirmed to Closed
- Resolution set to Fixed
Committed the patches in r25277 and r25278. Thank you to Mizuki ISHIKAWA for the tests and to Jean-Philippe Lang for the suggestions.
The behavior changes as follows. In the examples, Saturday and Sunday are non-working days.
- Bug fix: When the due date of a preceding issue is a non-working day, the following issue no longer starts one working day late. Previously, this happened when the delay was one less than a multiple of the number of working days per week (4, 9, 14, ... days). For example, if the preceding issue is due on Sun 2026-10-11 and the delay is 4 days, the following issue now starts on Fri 2026-10-16 instead of Mon 2026-10-19. The correct start date was also rejected because it was earlier than the soonest start date. This is the problem originally reported by Yutaka Hara. (r25277)
- API change: As Jean-Philippe Lang suggested in #note-8,
Redmine::Utils::DateCalculation#working_days(from, to)now counts the working days afterfrom, up to and includingto, so that it is consistent withadd_working_days. The result changes only whenfromortois a non-working day. Plugins that call this method directly may be affected. (r25278) - Bug fix: Rescheduling an issue whose due date is a non-working day no longer makes it one working day longer. For example, an issue from Tue 2026-10-06 to Sat 2026-10-10 that is rescheduled to Tue 2026-10-13 now ends on Fri 2026-10-16 instead of Mon 2026-10-19. Rescheduling an issue whose start date is a non-working day is unchanged. This problem was found while revising
working_days. (r25278)