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 %>