From 0b9401b089a5bf7761268ac6a8a93cdfb2a9765d Mon Sep 17 00:00:00 2001 From: cryptogopher Date: Mon, 19 Apr 2021 00:38:28 +0200 Subject: [PATCH] Proper nested records uniqueness validation Test pass: test_create_duplicate_for_persisted_target_should_fail --- app/controllers/targets_controller.rb | 3 +- app/models/goal.rb | 5 + app/models/measurement.rb | 3 +- app/models/quantity_value.rb | 18 ++-- app/models/readout.rb | 3 +- app/models/validations/nested_uniqueness.rb | 104 ++++++++++++++++++++ app/views/targets/_form.html.erb | 2 + app/views/targets/_new_form.html.erb | 1 - config/locales/en.yml | 2 + test/system/targets_test.rb | 49 +++++++-- 10 files changed, 168 insertions(+), 22 deletions(-) create mode 100644 app/models/validations/nested_uniqueness.rb diff --git a/app/controllers/targets_controller.rb b/app/controllers/targets_controller.rb index 0a81187..ebf2fe3 100644 --- a/app/controllers/targets_controller.rb +++ b/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, diff --git a/app/models/goal.rb b/app/models/goal.rb index 29899b8..2cba479 100644 --- a/app/models/goal.rb +++ b/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?} diff --git a/app/models/measurement.rb b/app/models/measurement.rb index ff1588c..eaa3134 100644 --- a/app/models/measurement.rb +++ b/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] } diff --git a/app/models/quantity_value.rb b/app/models/quantity_value.rb index c95097b..6af8b35 100644 --- a/app/models/quantity_value.rb +++ b/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 diff --git a/app/models/readout.rb b/app/models/readout.rb index 09789d0..c84405b 100644 --- a/app/models/readout.rb +++ b/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 diff --git a/app/models/validations/nested_uniqueness.rb b/app/models/validations/nested_uniqueness.rb new file mode 100644 index 0000000..1c86be2 --- /dev/null +++ b/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 diff --git a/app/views/targets/_form.html.erb b/app/views/targets/_form.html.erb index 1660d64..383ca45 100644 --- a/app/views/targets/_form.html.erb +++ b/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 %> +

<%= target_f.hidden_field :_destroy %> <%= t ".choose_quantity" %> diff --git a/app/views/targets/_new_form.html.erb b/app/views/targets/_new_form.html.erb index aeb8374..ae1c955 100644 --- a/app/views/targets/_new_form.html.erb +++ b/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 %>

<%= render partial: 'targets/form', locals: {goal_f: goal_f} %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 7ac0950..ffb655e 100644 --- a/config/locales/en.yml +++ b/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' diff --git a/test/system/targets_test.rb b/test/system/targets_test.rb index da82b8b..3321e4e 100644 --- a/test/system/targets_test.rb +++ b/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