https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292018-10-28T01:46:11ZRedmineRedmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=881192018-10-28T01:46:11ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-1 status-5 priority-4 priority-default closed" href="/issues/14846">Defect #14846</a>: Calculation of the start date of following issues ignores the "non-working days" setting</i> added</li></ul> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=881212018-10-28T01:47:26ZGo MAEDA
<ul><li><strong>Category</strong> set to <i>Issues</i></li></ul> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=881222018-10-28T01:53:35ZGo MAEDA
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/88122/diff?detail_id=70393">diff</a>)</li><li><strong>Status</strong> changed from <i>New</i> to <i>Confirmed</i></li><li><strong>Affected version</strong> set to <i>3.3.7</i></li></ul><p>I have confirmed that 3.3-stable and 3.4-stable are also affected.</p> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=881702018-10-30T01:29:18ZGo MAEDA
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/88170/diff?detail_id=70461">diff</a>)</li></ul> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=882852018-11-07T06:56:07ZMizuki ISHIKAWA
<ul><li><strong>File</strong> <a href="/attachments/21769">fix-29855.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/21769/fix-29855.patch">fix-29855.patch</a> added</li></ul><p>I think that applying this patch will solve the problem.<br />The code of the add_working_days method changes quite a bit, but all the tests succeed.</p>
<p>Any feedback is welcome.</p> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=885032018-11-25T06:54:54ZGo MAEDA
<ul></ul><p>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.</p>
<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 syntaxhl"><span class="nb">require</span> <span class="s1">'benchmark/ips'</span>
<span class="kp">include</span> <span class="no">Redmine</span><span class="o">::</span><span class="no">Utils</span><span class="o">::</span><span class="no">DateCalculation</span>
<span class="no">Benchmark</span><span class="p">.</span><span class="nf">ips</span> <span class="k">do</span> <span class="o">|</span><span class="n">x</span><span class="o">|</span>
<span class="n">x</span><span class="p">.</span><span class="nf">report</span><span class="p">(</span><span class="s1">'before'</span><span class="p">)</span> <span class="k">do</span>
<span class="n">add_working_days</span><span class="p">(</span><span class="no">Date</span><span class="p">.</span><span class="nf">today</span><span class="p">,</span> <span class="mi">30</span><span class="p">)</span>
<span class="k">end</span>
<span class="n">x</span><span class="p">.</span><span class="nf">report</span><span class="p">(</span><span class="s1">'after'</span><span class="p">)</span> <span class="k">do</span>
<span class="n">result</span> <span class="o">=</span> <span class="no">Date</span><span class="p">.</span><span class="nf">today</span>
<span class="mi">30</span><span class="p">.</span><span class="nf">times</span> <span class="k">do</span>
<span class="n">result</span> <span class="o">=</span> <span class="n">next_working_date</span><span class="p">(</span><span class="n">result</span> <span class="o">+</span> <span class="mi">1</span><span class="p">)</span>
<span class="k">end</span>
<span class="n">result</span>
<span class="k">end</span>
<span class="n">x</span><span class="p">.</span><span class="nf">compare!</span>
<span class="k">end</span>
</code></pre> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=886682018-12-01T08:50:31ZGo MAEDA
<ul><li><strong>Assignee</strong> set to <i>Jean-Philippe Lang</i></li><li><strong>Target version</strong> set to <i>3.3.9</i></li></ul><p>Jean-Philippe, do you think we can accept this performance deterioration?</p>
<p>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.</p> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=886952018-12-02T07:53:06ZJean-Philippe Langjp_lang@yahoo.fr
<ul><li><strong>Assignee</strong> changed from <i>Jean-Philippe Lang</i> to <i>Yutaka Hara</i></li></ul><p>Mizuki ISHIKAWA wrote:</p>
<blockquote>
<p>Any feedback is welcome.</p>
</blockquote>
<p><code>DateCalculation#working_days</code> should be fixed in a similar way to be consistent with the proposed fix. These new assertions should pass:</p>
<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> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=886962018-12-02T07:55:08ZJean-Philippe Langjp_lang@yahoo.fr
<ul><li><strong>Assignee</strong> changed from <i>Yutaka Hara</i> to <i>Go MAEDA</i></li></ul> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=886992018-12-02T11:39:47ZMarius BĂLTEANU
<ul><li><strong>Assignee</strong> changed from <i>Go MAEDA</i> to <i>Jean-Philippe Lang</i></li></ul><p>I took a look and there are some strange (or wrong) test cases the we should review before changing anything else.</p>
<p>Taking the following test scenario:<br /><pre><code>
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></p>
<p><code>assert_working_days 4, '2012-10-09', '2012-10-15'</code> <br />2012-10-09 was Tuesday<br />2012-10-15 was Monday<br />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).</p>
<p>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).</p>
<p>Jean-Philippe, what do you think? I'm in favour of including the end date in the count.</p> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=887132018-12-02T16:23:53ZJean-Philippe Langjp_lang@yahoo.fr
<ul></ul><p>Marius BALTEANU wrote:</p>
<blockquote>
<p>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).</p>
</blockquote>
<p><code>#working_days</code> and <code>#add_working_days</code> are used to reschedule an issue when the start date is changed. Its duration is calculated with <code>#working_days</code> and the new due date is calculated with <code>#add_working_days</code>. If there is no "non working day", they should behave like <code>Date#-</code> and <code>Date#+</code>.</p> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=887142018-12-02T16:35:05ZMarius BĂLTEANU
<ul><li><strong>Assignee</strong> changed from <i>Jean-Philippe Lang</i> to <i>Go MAEDA</i></li></ul><p>Jean-Philippe Lang wrote:</p>
<blockquote>
<p><code>#working_days</code> and <code>#add_working_days</code> are used to reschedule an issue when the start date is changed. Its duration is calculated with <code>#working_days</code> and the new due date is calculated with <code>#add_working_days</code>. If there is no "non working day", they should behave like <code>Date#-</code> and <code>Date#+</code>.</p>
</blockquote>
<p>Thanks, but are still not clear for me the expected results so I'll leave Go Maeda or Mizuki ISHIKAWA to fix this issue.</p> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=888312018-12-08T06:33:54ZJean-Philippe Langjp_lang@yahoo.fr
<ul><li><strong>Target version</strong> deleted (<del><i>3.3.9</i></del>)</li></ul> Redmine - Defect #29855: add_working_days returns wrong datehttps://www.redmine.org/issues/29855?journal_id=888352018-12-08T08:28:29ZGo MAEDA
<ul><li><strong>Target version</strong> set to <i>Candidate for next minor release</i></li></ul>