Redmine

The following change works in the reported case.

diff --git a/app/models/issue.rb b/app/models/issue.rb
index ab9f794db..d957d5933 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -1851,7 +1851,7 @@ class Issue < ActiveRecord::Base
               estimated * ratio
             end
             progress = done / (average * children.count)
-            p.done_ratio = progress.floor
+            p.done_ratio = progress.to_f.floor
           end
         end
       end

irb(main):029:0> done
=> 0.109e5
irb(main):030:0> average
=> 0.9909090909090909090909090909090909091e1
irb(main):031:0> children.count
=> 11
irb(main):032:0> progress
=> 0.99999999999999999999999999999999999999082568807e2
irb(main):033:0> progress.floor
=> 99
irb(main):034:0> progress.to_f
=> 100.0
irb(main):035:0> progress.to_f.floor
=> 100

Another solution. Probably this is more accurate than #37151#note-2.

diff --git a/app/models/issue.rb b/app/models/issue.rb
index ab9f794db..519b26fd8 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -1838,19 +1838,20 @@ class Issue < ActiveRecord::Base
           if children.any?
             child_with_total_estimated_hours = children.select {|c| c.total_estimated_hours.to_f > 0.0}
             if child_with_total_estimated_hours.any?
-              average =
-                child_with_total_estimated_hours.sum(&:total_estimated_hours).to_d /
-                  child_with_total_estimated_hours.count
+              average = Rational(
+                child_with_total_estimated_hours.sum(&:total_estimated_hours).to_s,
+                child_with_total_estimated_hours.count
+              )
             else
-              average = BigDecimal('1.0')
+              average = Rational('1.0')
             end
             done = children.sum do |c|
-              estimated = (c.total_estimated_hours || 0.0).to_d
+              estimated = Rational(c.total_estimated_hours.to_f.to_s)
               estimated = average unless estimated > 0.0
               ratio = c.closed? ? 100 : (c.done_ratio || 0)
               estimated * ratio
             end
-            progress = done / (average * children.count)
+            progress = Rational(done, average * children.count)
             p.done_ratio = progress.floor
           end
         end

irb(main):023:0> done
=> (10900/1)
irb(main):024:0> average
=> (109/11)
irb(main):025:0> children.count
=> 11
irb(main):026:0> progress
=> (100/1)
irb(main):027:0> progress.floor
=> 100

I think your latter patch with the Rationals is more correct than mixing Decimals and Floats would be. Thank you Maeda-san!

As a slight optimization however, I don't think it is necessary to use to_s in the numbers before parsing the Rationals. I think Kernel#Rational also accepts Floats and Integers at least down to Ruby 2.4.

Thus, I believe this should be functionally equivalent to your patch in #37151#note-5 above:

diff --git a/app/models/issue.rb b/app/models/issue.rb
index ab9f794db..519b26fd8 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -1838,19 +1838,20 @@ class Issue < ActiveRecord::Base
           if children.any?
             child_with_total_estimated_hours = children.select {|c| c.total_estimated_hours.to_f > 0.0}
             if child_with_total_estimated_hours.any?
-              average =
-                child_with_total_estimated_hours.sum(&:total_estimated_hours).to_d /
-                  child_with_total_estimated_hours.count
+              average = Rational(
+                child_with_total_estimated_hours.sum(&:total_estimated_hours),
+                child_with_total_estimated_hours.count
+              )
             else
-              average = BigDecimal('1.0')
+              average = Rational(1)
             end
             done = children.sum do |c|
-              estimated = (c.total_estimated_hours || 0.0).to_d
-              estimated = average unless estimated > 0.0
+              estimated = Rational(c.total_estimated_hours.to_f)
+              estimated = average unless estimated > 0
               ratio = c.closed? ? 100 : (c.done_ratio || 0)
               estimated * ratio
             end
             progress = done / (average * children.count)
             p.done_ratio = progress.floor
           end
         end

Holger Just wrote:

As a slight optimization however, I don't think it is necessary to use to_s in the numbers before parsing the Rationals. I think Kernel#Rational also accepts Floats and Integers at least down to Ruby 2.4.

Thank you for optimizing the patch. But I found that it breaks an existing test.

Failure:                                                      
IssueSubtaskingTest#test_done_ratio_of_parent_with_completed_children_should_not_be_99 [test/unit/issue_subtasking_test.rb:250]:
Expected: 100                                                 
  Actual: 99                                                  

rails test test/unit/issue_subtasking_test.rb:244

This is because the value of Rational("8.1") and Rational(8.1) are not the same. Rational("8.1") represents 0.8 while Rational(8.1) represents 8.09999999999999964472.... So, I think it is safe to convert a float value to string before passing the value to Kernel.#Rational.

irb(main):001:0> Rational("0.1")
=> (1/10)
irb(main):002:0> Rational(0.1)
=> (3602879701896397/36028797018963968)

Using Float#to_d instead of Float#to_s also works as expected. It may be better because using Float#to_d makes it clear that floating-point errors are taken into account.

diff --git a/app/models/issue.rb b/app/models/issue.rb
index ab9f794db..2b488c0fd 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -1838,19 +1838,20 @@ class Issue < ActiveRecord::Base
           if children.any?
             child_with_total_estimated_hours = children.select {|c| c.total_estimated_hours.to_f > 0.0}
             if child_with_total_estimated_hours.any?
-              average =
-                child_with_total_estimated_hours.sum(&:total_estimated_hours).to_d /
-                  child_with_total_estimated_hours.count
+              average = Rational(
+                child_with_total_estimated_hours.sum(&:total_estimated_hours).to_d,
+                child_with_total_estimated_hours.count
+              )
             else
-              average = BigDecimal('1.0')
+              average = Rational(1)
             end
             done = children.sum do |c|
-              estimated = (c.total_estimated_hours || 0.0).to_d
+              estimated = Rational(c.total_estimated_hours&.to_d || 0)
               estimated = average unless estimated > 0.0
               ratio = c.closed? ? 100 : (c.done_ratio || 0)
               estimated * ratio
             end
-            progress = done / (average * children.count)
+            progress = Rational(done, average * children.count)
             p.done_ratio = progress.floor
           end
         end