Proper nested records uniqueness validation

Test pass: test_create_duplicate_for_persisted_target_should_fail

app/controllers/targets_controller.rb

@@ -81,7 +81,8 @@ class TargetsController < ApplicationController
           :id,
           :quantity_id,
           :scope,
-          :destroy,
+          :effective_from,
+          :_destroy,
           thresholds_attributes: [
             :id,
             :quantity_id,

app/models/goal.rb

@@ -7,6 +7,11 @@ class Goal < ActiveRecord::Base
   has_many :quantities, -> { order "lft" }, through: :exposures
 
   accepts_nested_attributes_for :targets, allow_destroy: true
+  include Validations::NestedUniqueness
+  validates_nested_uniqueness_for :targets,
+    :effective_from, :quantity_id, :item_type, :item_id, :scope,
+    scope: [:effective_from]
+
   validates :is_binding, uniqueness: {scope: :project_id}, if: :is_binding?
   validates :name, presence: true, uniqueness: {scope: :project_id},
     exclusion: {in: [I18n.t('goals.binding.name')], unless: :is_binding?}

app/models/measurement.rb

@@ -17,8 +17,7 @@ class Measurement < ActiveRecord::Base
   validates :readouts, presence: true
   accepts_nested_attributes_for :readouts, allow_destroy: true,
     reject_if: proc { |attrs| attrs['quantity_id'].blank? && attrs['value'].blank? }
-  # Readout quantity_id + unit_id uniqueness validation. Cannot be effectively
-  # checked on Readout model level.
+  # Readout uniqueness validation
   validate do
     quantities = self.readouts.reject { |r| r.marked_for_destruction? }
       .map { |r| [r.quantity_id, r.unit_id] }

app/models/quantity_value.rb

@@ -11,12 +11,14 @@ class QuantityValue < ActiveRecord::Base
   end
   belongs_to :unit, required: true
 
-  # Uniqueness is checked exclusively on the other end of association level.
-  # Otherwise validation may not pass when multiple Values are updated at once
-  # and some quantity_id is moved from one Value to the other (without duplication).
-  # For the same reason Value quantity_id uniqueness has to be checked on the
-  # other end when multiple Values are first created. Relying on local check
-  # only would make all newly added records pass as valid despite duplications.
-  #validates :quantity, uniqueness: {scope: [:measurement_id, :unit_id]}
-  #validates :quantity, uniqueness: {scope: :food_id}
+  # Uniqueness is checked exclusively in the model accepting nested attributes.
+  # Otherwise validation may give invalid results for batch create/update actions,
+  # because either:
+  # * in-memory records in batch are not unique but validates_uniqueness_of only
+  #   validates every single in-memory record against database content (and so
+  #   misses non-uniqueness inside batch)
+  # or
+  # * batch update action may include swapping of unique values of 2 or more
+  #   records and checking in-memory records for uniqueness one-by-one against
+  #   database will falsely signal uniqueness violation
 end

app/models/readout.rb

@@ -4,8 +4,7 @@ class Readout < QuantityValue
   belongs_to :measurement, inverse_of: :readouts, polymorphic: true, required: true,
     foreign_key: 'registry_id', foreign_type: 'registry_type'
 
-  # Uniqueness NOT validated here, see Value for explanation
-  #validates :quantity, uniqueness: {scope: [:measurement_id, :unit_id]}
+  # Readout uniqueness NOT validated here, see Value for explanation
   validates :value, numericality: true
 
   delegate :completed_at, to: :measurement

app/models/validations/nested_uniqueness.rb

@@ -0,0 +1,104 @@
+module Validations::NestedUniqueness
+  extend ActiveSupport::Concern
+
+  included do
+    class_attribute :nested_uniqueness_options, instance_writer: false, default: {}
+    class_attribute :nested_uniqueness_duplicates, instance_writer: false, default: {}
+  end
+
+  module ClassMethods
+    def validates_nested_uniqueness_for(*attr_names)
+      options = {scope: nil}
+      options.update(attr_names.extract_options!)
+      options.assert_valid_keys(:scope)
+
+      if association_name = attr_names.shift
+        if attr_names.empty?
+          raise ArgumentError, "No unique attributes given for name `#{association_name}'."
+        else
+          options[:attributes] = attr_names
+
+          nested_uniqueness_options = self.nested_uniqueness_options.dup
+          nested_uniqueness_options[association_name.to_sym] = options
+          self.nested_uniqueness_options = nested_uniqueness_options
+
+          before_validation :before_validation_nested_uniqueness
+          after_validation :after_validation_nested_uniqueness
+
+          reflection = reflect_on_association(association_name)
+          reflection.klass.class_eval <<-eoruby, __FILE__, __LINE__ + 1
+            validate do
+              if #{reflection.inverse_of.name}
+                   .nested_uniqueness_duplicates[:#{reflection.name}].include?(self)
+                errors.add(:base, :duplicated_record) 
+              end
+            end
+          eoruby
+        end
+      end
+    end
+  end
+
+  private
+
+  def before_validation_nested_uniqueness
+    nested_uniqueness_options.each do |association_name, options|
+      collection = send(association_name)
+      was_loaded = collection.loaded?
+      records = collection.target.select(&:changed?)
+
+      preserved_records = records.reject(&:marked_for_destruction?)
+      scope = options[:scope]&.map { |attr| [attr, preserved_records.map(&attr).uniq] }.to_h
+
+      seen = {}
+      nested_uniqueness_duplicates[association_name] = []
+      collection.where(scope).scoping do
+        collection.reject(&:marked_for_destruction?).each do |r|
+          key = options[:attributes].map { |attr| r.send(attr) }
+          if seen[key]
+            nested_uniqueness_duplicates[association_name] << (r.changed? ? r : seen[key])
+          else
+            seen[key] = r
+          end
+        end
+      end
+      unless was_loaded
+        collection.proxy_association.reset
+        records.each { |r| collection.proxy_association.add_to_target(r) }
+      end
+    end
+  end
+
+  def after_validation_nested_uniqueness
+    nested_uniqueness_duplicates.clear
+  end
+
+  #def before_validation_nested_uniqueness do
+  #  was_loaded = targets.loaded?
+  #  records = targets.target.select(&:changed?)
+  #  dates = records.reject(&:marked_for_destruction?).map(&:effective_from).uniq
+  #  seen = {}
+  #  @duplicated_records = []
+  #  targets.where(effective_from: dates).scoping do
+  #    targets.reject(&:marked_for_destruction?).each do |t|
+  #      key = [t.effective_from, t.quantity_id, t.item_type, t.item_id, t.scope]
+  #      if seen[key]
+  #        @duplicated_records << (t.changed? ? t : seen[key])
+  #      else
+  #        seen[key] = t
+  #      end
+  #    end
+  #  end
+  #  unless was_loaded
+  #    targets.proxy_association.reset
+  #    records.each { |t| targets.proxy_association.add_to_target(t) }
+  #  end
+  #end
+
+  #def after_validation_nested_uniqueness do
+  #  @duplicated_records = nil
+  #end
+  #def validate_nested_targets_uniqueness(record)
+  #  record.errors.add(:base, :duplicated_target) if @duplicated_records.include?(record)
+  #end
+end

app/views/targets/_form.html.erb

@@ -5,6 +5,8 @@
 <% end %>
 
 <%= goal_f.fields_for :targets, @targets, child_index: '' do |target_f| %>
+  <%= error_messages_for target_f.object %>
+
   <p class="target">
   <%= target_f.hidden_field :_destroy %>
   <em class="info"><%= t ".choose_quantity" %></em>

app/views/targets/_new_form.html.erb

@@ -4,7 +4,6 @@
 <%= labelled_form_for @goal, url: goal_targets_path(@goal), method: :post, remote: true,
   html: {id: 'new-target-form', name: 'new-target-form'} do |goal_f| %>
 
-  <%= error_messages_for *@targets %>
   <div class="box tabular">
     <%= render partial: 'targets/form', locals: {goal_f: goal_f} %>
   </div>

config/locales/en.yml

@@ -33,6 +33,8 @@ en:
       models:
         target:
           attributes:
+            base:
+              duplicated_record: 'target for this quantity, item and scope already exists'
             thresholds:
               count_mismatch: 'count invalid for given condition'
               quantity_mismatch: 'should refer to the same quantity'

test/system/targets_test.rb

@@ -76,7 +76,7 @@ class TargetsTest < BodyTrackingSystemTestCase
   end
 
   def test_create_binding_target
-    date = Date.current + rand(-100..100).days
+    date = Date.current + rand(-10..10).days
     quantity = @project.quantities.except_targets.sample
     threshold_quantity = @project.quantities.target.roots.sample
     threshold_value = rand(-2000.0..2000.0).to_d(4)
@@ -169,23 +169,53 @@ class TargetsTest < BodyTrackingSystemTestCase
   def test_create_multiple_targets
   end
 
+  def test_create_duplicate_for_persisted_target_should_fail
+    # TODO: extend with item + scope
+    source = @project.targets.sample
+    threshold_quantity = @project.quantities.target.roots.sample
+    threshold_value = rand(-2000.0..2000.0).to_d(4)
+    threshold_unit = @project.units.sample
+
+    assert_no_difference 'Target.count' do
+      visit goal_targets_path(source.goal)
+      click_link t('targets.contextual.link_new_target')
+      within 'form#new-target-form' do
+        fill_in t(:field_effective_from), with: source.effective_from
+        within 'p.target' do
+          select source.quantity.name
+          select threshold_quantity.name
+          fill_in with: threshold_value
+          select threshold_unit.shortname
+        end
+        click_on t(:button_create)
+      end
+    end
+    msg = I18n.t('activerecord.errors.models.target.attributes.base.duplicated_record')
+    assert_selector :xpath, '//form//p[@class="target"]//preceding-sibling::div', text: msg
+  end
+
+  def test_create_duplicated_targets_should_fail
+  end
+
+  def test_create_and_simultaneously_remove_persisted_duplicate
+  end
+
   # TODO: test_create_failure(s)
   # * restoring non-empty targets values
   # * removing empty targets
 
   def test_edit_binding_target
-    t = Target.offset(rand(Target.count)).take
-    visit project_targets_path(@project)
+    target = @project.goals.binding.targets.sample
+    visit goal_targets_path(@project.goals.binding)
     assert_no_selector 'form#edit-target-form'
 
-    within find('td', text: t.effective_from).ancestor('tr') do
+    within find('td', text: target.effective_from).ancestor('tr') do
       click_link t(:button_edit)
 
-      # FIXME: should look for any form (w/o ID) in first following TR
-      within find(:xpath, 'following-sibling::*//form[@id="edit-target-form"]') do
-        assert has_select?(t(:field_goal), selected: t.goal.name)
-        assert has_field?(t(:field_effective_from), with: t.effective_from)
+      within find(:xpath, 'following-sibling::tr//form') do
+        assert has_field?(t(:field_effective_from), with: target.effective_from)
 
+        # NOTE: this assumes target.quantity is unique which is not enforced now
         threshold = t.thresholds.first
         within find('select option[selected]', exact_text: threshold.quantity.name)
                  .ancestor('p') do
@@ -230,4 +260,7 @@ class TargetsTest < BodyTrackingSystemTestCase
     assert_equal value, t.thresholds.first.value
     assert_equal unit, t.thresholds.first.unit
   end
+
+  def test_update_swap_targets
+  end
 end