From 6b3714817c094aaf1b33077a8f569348bf070bce Mon Sep 17 00:00:00 2001
From: cryptogopher <git6453hub@michalczyk.pro>
Date: Sat, 13 Jun 2020 18:18:53 +0200
Subject: [PATCH] Formula reports ambiguous quantity identifiers

Formula returns error.details for Rails 4
Formula distinguishes root path (::Identifier)
Quantity validates name changes against introducing formulas'
ambiguities
---
 app/models/formula.rb  | 25 +++++++++++++++++++++----
 app/models/quantity.rb | 23 ++++++++++++++++-------
 config/locales/en.yml  |  1 +
 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/app/models/formula.rb b/app/models/formula.rb
index 9f541ee..179d7cd 100644
--- a/app/models/formula.rb
+++ b/app/models/formula.rb
@@ -8,7 +8,16 @@ class Formula < ActiveRecord::Base
 
   validates :code, presence: true
   validate do
-    parse.each { |message, params| errors.add(:code, message, params) }
+    messages = parse.each { |message, params| errors.add(:code, message, params) }
+    # NOTE: mimics errors.details available in Rails 5.2; remove once not needed;
+    # used only in Quantity :name change validation
+    unless errors.instance_variable_defined?(:@details)
+      class << errors
+        attr_accessor :details
+      end
+      errors.details = Hash.new { |h,k| h[k] = []}
+      messages.each { |message, *| errors.details[:code] << {error: message} }
+    end
   end
 
   after_initialize do
@@ -86,15 +95,23 @@ class Formula < ActiveRecord::Base
     (quantity.project.try(&:quantities) || Quantity.where(project: nil))
       .select { |q| q_names.include?(q.name) }.each do |q|
 
-      # NOTE: after upgrade do Ruby 2.7 replace with Enumerator#produce
+      # NOTE: after upgrade to Ruby 2.7 replace with Enumerator#produce
       current, path = q, q.name
-      while current
+      loop do
         q_paths[path] = q_paths.has_key?(path) ? nil : q
+        break unless current
         current, path = current.parent, "#{current.parent.try(:name)}::#{path}"
       end
     end
+
     quantities = []
-    identifiers.reject! { |i| q_paths[i] && quantities << q_paths[i] }
+    identifiers.reject! do |i|
+      if q_paths.has_key?(i)
+        q = q_paths[i]
+        q.nil? ? errors << [:ambiguous_dependency, {identifier: i}] : quantities << q
+      end
+    end
+
     models = identifiers.map(&:safe_constantize).compact || []
     (identifiers - models.map(&:class_name)).each do |i|
       errors << [:unknown_dependency, {identifier: i}]
diff --git a/app/models/quantity.rb b/app/models/quantity.rb
index 7a71cf9..757c999 100644
--- a/app/models/quantity.rb
+++ b/app/models/quantity.rb
@@ -6,7 +6,7 @@ class Quantity < ActiveRecord::Base
   }
 
   acts_as_nested_set dependent: :destroy, scope: :project
-  belongs_to :project, required: false
+  belongs_to :project, inverse_of: :quantities, required: false
   has_many :nutrients, dependent: :restrict_with_error
   has_many :readouts, dependent: :restrict_with_error
   has_many :values, class_name: 'QuantityValue', dependent: :restrict_with_error
@@ -19,17 +19,26 @@ class Quantity < ActiveRecord::Base
     formula.mark_for_destruction if formula.present? && formula.code.blank?
   end
 
-  validates :name, presence: true
+  # TODO: :name should be validated against model names (Meal, Ingredient etc.)
   # Quantity :name uniqueness relaxed to formulas unambiguity
-  validate if: -> { name_changed? } do
+  validates :name, presence: true, uniqueness: {scope: [:project_id, :parent_id]}
+  # Formula ambiguity vlidation delayed after save, as otherwise there seems to
+  # be no other way to validate against newly changed :name
+  after_save do
+    next unless name_changed?
     formulas = project.formulas.where('formulas.code LIKE ?', "%#{name}%").includes(:quantity)
-    # FIXME: should actually parse formulas in formulas and check for exact name match;
-    # current code is just quick'n'dirty partial solution
-    if formulas.exists?
-      quantity_names = formulas.map { |f| "'#{f.quantity.name}'" }.join(',')
+    next unless formulas.exists?
+
+    quantity_names = formulas.reject(&:valid?)
+      .select { |f| f.errors.details[:code].any? { |e| e[:error] == :ambiguous_dependency } }
+      .map { |f| "'#{f.quantity.name}'" }.join(', ')
+
+    unless quantity_names.blank?
       errors.add(:name, :name_ambiguous, names: quantity_names)
+      raise ActiveRecord::RecordInvalid.new(self)
     end
   end
+
   validates :domain, inclusion: {in: domains.keys}
   validate if: -> { parent.present? } do
     errors.add(:parent, :parent_domain_mismatch) unless domain == parent.domain
diff --git a/config/locales/en.yml b/config/locales/en.yml
index a5af9ff..050a5f2 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -54,6 +54,7 @@ en:
               disallowed_keyword: 'includes disallowed keyword "%{keyword}"'
               disallowed_method: 'includes disallowed method call "%{method}"'
               unknown_dependency: 'contains undefined dependency "%{identifier}"'
+              ambiguous_dependency: 'contains ambiguous reference to "%{identifier}"'
               computation_failed: 'computation failed for "%{quantity}": %{description}
                 (%{count} values missing)'
   body_trackers: