WIP Targets #edit/#update

NestedUniqueness properly restores collection.target

app/controllers/targets_controller.rb

@@ -5,10 +5,9 @@ class TargetsController < ApplicationController
 
   include Concerns::Finders
 
-  before_action :find_binding_goal_by_project_id, only: [:edit]
   before_action :find_project_by_project_id, only: [:subthresholds]
   before_action :find_quantity_by_quantity_id, only: [:toggle_exposure]
-  before_action :find_goal_by_goal_id, only: [:index, :new, :create]
+  before_action :find_goal_by_goal_id, only: [:index, :new, :create, :edit, :update]
   before_action :find_goal, only: [:toggle_exposure]
   before_action :authorize
   #before_action :set_view_params
@@ -39,21 +38,32 @@ class TargetsController < ApplicationController
       end
     else
       @targets = @goal.targets.select(&:changed_for_autosave?)
-      @targets.each { |t| t.thresholds.new unless t.thresholds.present? }
+        .each { |t| t.thresholds.new unless t.thresholds.present? }
       render :new
     end
   end
 
   def edit
-    @targets = @goal.targets.where(effective_from: params[:date]).to_a
+    @targets = @goal.targets.joins(:quantity).where(effective_from: params[:date])
+      .order('quantities.lft' => :asc).to_a
     @effective_from = @targets.first&.effective_from
   end
 
   def update
     # TODO: DRY same code with #create
-    @goal = @project.goals.find(params[:goal_id]) if params[:goal_id].present?
-    @goal ||= @project.goals.new
-    @goal.attributes = goal_params unless @goal.is_binding?
+    @effective_from = params[:goal].delete(:effective_from)
+    params[:goal][:targets_attributes].each { |ta| ta[:effective_from] = @effective_from }
+
+    byebug
+    if @goal.update(targets_params)
+      flash.now[:notice] = t('.success')
+      prepare_targets
+    else
+      @targets = @goal.targets.where(id: targets_params[:targets_attributes].pluck(:id))
+      @targets += @goal.targets.select(&:changed_for_autosave?)
+        .each { |t| t.thresholds.new unless t.thresholds.present? }
+      render :edit
+    end
   end
 
   def destroy

app/helpers/targets_helper.rb

@@ -1,9 +1,9 @@
 module TargetsHelper
-  def action_links(d)
-    link_to(l(:button_reapply), reapply_goal_targets_path(@project, d, @view_params),
+  def action_links(date)
+    link_to(l(:button_reapply), reapply_goal_targets_path(@goal, date, @view_params),
             {remote: true, class: "icon icon-reload"}) +
-    link_to(l(:button_edit), edit_goal_targets_path(@project, d, @view_params),
+    link_to(l(:button_edit), edit_goal_targets_path(@goal, date, @view_params),
             {remote: true, class: "icon icon-edit"}) +
-    delete_link(target_path(d), {remote: true, data: {}})
+    delete_link(target_path(date), {remote: true, data: {}})
   end
 end

app/models/target.rb

@@ -17,6 +17,7 @@ class Target < ActiveRecord::Base
     errors.add(:thresholds, :quantity_mismatch) unless quantities == ancestors
   end
   #validates :scope, inclusion: {in: [:ingredient, :meal, :day], if: -> { quantity&.diet? }}
+  # FIXME: does not seem to work
   validates :effective_from, presence: {if: :is_binding?}, absence: {unless: :is_binding?}
 
   after_initialize do

app/models/validations/nested_uniqueness.rb

@@ -44,10 +44,10 @@ module Validations::NestedUniqueness
   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?)
+      records = collection.target.dup unless collection.loaded?
 
-      preserved_records = records.reject(&:marked_for_destruction?)
+      preserved_records = records.select(&:changed?).reject(&:marked_for_destruction?)
+      # TODO: do scoping on options[:attributes] and remove options[:scope]
       scope = options[:scope]&.map { |attr| [attr, preserved_records.map(&attr).uniq] }.to_h
 
       seen = {}
@@ -62,7 +62,7 @@ module Validations::NestedUniqueness
           end
         end
       end
-      unless was_loaded
+      if records
         collection.proxy_association.reset
         records.each { |r| collection.proxy_association.add_to_target(r) }
       end

app/views/targets/_edit_form.html.erb

@@ -1,9 +1,9 @@
-<%= labelled_form_for @targets,
-  url: project_targets_path(@project, @view_params),
-  method: :patch, remote: true,
-  html: {id: 'edit-target-form', name: 'edit-target-form'} do |f| %>
+<%= labelled_form_for @goal, url: goal_targets_path(@goal), method: :patch, remote: true,
+  html: {id: 'edit-target-form', name: 'edit-target-form'} do |goal_f| %>
 
-  <%= render partial: 'targets/form', locals: {f: f} %>
+  <div class="box tabular">
+    <%= render partial: 'targets/form', locals: {goal_f: goal_f} %>
+  </div>
 
   <div class="tabular">
     <p>

app/views/targets/_form.html.erb

@@ -4,10 +4,11 @@
   </p>
 <% end %>
 
-<%= goal_f.fields_for :targets, @targets, child_index: '' do |target_f| %>
+<%= goal_f.fields_for :targets, @targets, index: '', child_index: '' do |target_f| %>
   <%= error_messages_for target_f.object %>
 
   <p class="target">
+  <%= target_f.hidden_field :id %>
   <%= target_f.hidden_field :_destroy %>
   <em class="info"><%= t ".choose_quantity" %></em>
   <%= target_f.select :quantity_id, quantity_options,

app/views/targets/_thresholds_form.html.erb

@@ -1,5 +1,5 @@
 <% target_f.object.thresholds.new unless last_quantity.leaf? %>
-<%= target_f.fields_for :thresholds, child_index: '' do |threshold_f| %>
+<%= target_f.fields_for :thresholds, index: '', child_index: '' do |threshold_f| %>
   <% threshold_q = threshold_f.object.quantity %>
   <% parent_id = threshold_q.nil? ? last_quantity.id : threshold_q.parent_id  %>
   <%= threshold_f.collection_select :quantity_id,
@@ -16,6 +16,7 @@
                  }
                });
                return false;"} %>
+  <%# TODO: update above ajax with -1/no change/+1 logic based on copying existing threshold and proper ID/destroy handling %>
   <%#  {autocomplete: 'off',
      data: {remote: true, url: subthresholds_project_targets_path(parent_id: parent_id)}} %>
 

config/routes.rb

@@ -11,9 +11,10 @@ resources :projects, shallow: true do
     member do
       post 'toggle_exposure', controller: :targets
     end
-    resources :targets, shallow: true, except: [:show, :edit, :update] do
+    resources :targets, except: [:show, :edit, :update] do
       collection do
         get 'edit/:date', action: :edit, as: :edit
+        patch '', action: :update
         post 'reapply/:date', action: :reapply, as: :reapply
       end
     end

test/system/targets_test.rb

@@ -86,6 +86,7 @@ class TargetsTest < BodyTrackingSystemTestCase
                       '@project.targets.reload.count' => 1, 'Threshold.count' => 1 do
       visit goal_targets_path(@project.goals.binding)
       click_link t('targets.contextual.link_new_target')
+
       within 'form#new-target-form' do
         fill_in t(:field_effective_from), with: date
         within 'p.target' do
@@ -142,6 +143,7 @@ class TargetsTest < BodyTrackingSystemTestCase
                       'Threshold.count' => thresholds.length do
       visit goal_targets_path(@project.goals.binding)
       click_link t('targets.contextual.link_new_target')
+
       within 'form#new-target-form' do
         within 'p.target' do
           select quantity.name
@@ -167,9 +169,13 @@ class TargetsTest < BodyTrackingSystemTestCase
   end
 
   def test_create_multiple_targets
+    # TODO
   end
 
-  def test_create_form_retains_data_on_failure
+  def test_create_properly_handles_data_on_failure
+    # TODO
+    # * restoring non-empty targets values
+    # * removing empty targets
   end
 
   def test_create_duplicate_for_persisted_target_should_fail
@@ -206,8 +212,10 @@ class TargetsTest < BodyTrackingSystemTestCase
     assert_no_difference 'Target.count' do
       visit goal_targets_path(@project.goals.binding)
       click_link t('targets.contextual.link_new_target')
+
       within 'form#new-target-form' do
         fill_in t(:field_effective_from), with: date
+
         within :xpath, '//p[@class="target"][1]' do
           select quantity.name
           select @project.quantities.target.roots.sample.name
@@ -215,6 +223,7 @@ class TargetsTest < BodyTrackingSystemTestCase
           select @project.units.sample.shortname
         end
         click_link t('targets.form.button_new_target')
+
         within :xpath, '//p[@class="target"][2]' do
           select quantity.name
           select @project.quantities.target.roots.sample.name
@@ -222,42 +231,67 @@ class TargetsTest < BodyTrackingSystemTestCase
           select @project.units.sample.shortname
         end
         click_on t(:button_create)
+
         assert_selector :xpath, '//p[@class="target"][last()]//preceding-sibling::div',
           text: msg
       end
     end
   end
 
-  def test_create_and_simultaneously_remove_persisted_duplicate
+  def test_edit_binding_target
+    # TODO: extend with item + scope
+    target = @project.goals.binding.targets.sample
+    date = Date.current + rand(-10..10).days
+    quantity = @project.quantities.except_targets.where.not(id: target.quantity.id).sample
+    thresholds =
+      @project.quantities.target.where.not(parent: nil).sample.self_and_ancestors.map do |q|
+        [q, rand(-2000.0..2000.0).to_d(4), @project.units.sample]
       end
 
-  # TODO: test_create_failure(s)
-  # * restoring non-empty targets values
-  # * removing empty targets
-
-  def test_edit_binding_target
-    target = @project.goals.binding.targets.sample
     visit goal_targets_path(@project.goals.binding)
     assert_no_selector 'form#edit-target-form'
 
     within find('td', text: target.effective_from).ancestor('tr') do
       click_link t(:button_edit)
+      assert_selector :xpath, 'following-sibling::tr//form[@id="edit-target-form"]'
+    end
 
-      within :xpath, 'following-sibling::tr//form' do
+    assert_no_difference 'Target.count' do
+      within 'form#edit-target-form' do
         assert has_field?(t(:field_effective_from), with: target.effective_from)
+        fill_in t(:field_effective_from), with: date
 
-        # 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
-          assert has_select?(selected: t.condition)
+        within find('select option[selected]', exact_text: target.quantity.name)
+          .ancestor('p.target') do
+          target.thresholds.each do |threshold|
+            assert has_select?(selected: threshold.quantity.name)
             assert has_field?(with: threshold.value)
             assert has_select?(selected: threshold.unit.shortname)
           end
+
+          select quantity.name
+          thresholds.each do |t_quantity, t_value, t_unit|
+            within select(t_quantity.name).ancestor('select') do
+              find(:xpath, 'following-sibling::input').set(t_value)
+              find(:xpath, 'following-sibling::select[1]').select(t_unit.shortname)
+            end
           end
         end
 
-    assert_selector 'form#edit-target-form', count: 1
+        click_on t(:button_save)
+      end
+    end
+
+    target.reload
+    assert_equal date, target.effective_from
+    assert_equal quantity, target.quantity
+    assert_equal thresholds.length, target.thresholds.length
+    thresholds.each_with_index do |threshold, index|
+      t_quantity, t_value, t_unit = threshold
+      assert_equal t_quantity, target.thresholds[index].quantity
+      assert_equal t_value, target.thresholds[index].value
+      assert_equal t_unit, target.thresholds[index].unit
+    end
   end
 
   def test_update
@@ -293,5 +327,10 @@ class TargetsTest < BodyTrackingSystemTestCase
   end
 
   def test_update_swap_targets
+    # TODO
+  end
+
+  def test_update_add_and_simultaneously_remove_persisted_duplicate
+    # TODO
   end
 end