Refactoring standing line item delete logic

Using StandingOrderForm rather than separate endpoint

app/assets/javascripts/admin/standing_orders/controllers/standing_line_items_controller.js.coffee

@@ -2,20 +2,30 @@ angular.module("admin.standingOrders").controller "StandingLineItemsController",
   $scope.newItem = { variant_id: 0, quantity: 1 }
 
   $scope.addStandingLineItem = ->
-    existing_ids = $scope.standingOrder.standing_line_items.map (sli) -> sli.variant_id
-    if $scope.newItem.variant_id in existing_ids
-      InfoDialog.open 'error', t('admin.standing_orders.product_already_in_order')
+    match = $scope.match()
+    if match
+      if match._destroy
+        angular.extend(match, $scope.newItem)
+        delete match._destroy
+      else
+        InfoDialog.open 'error', t('admin.standing_orders.product_already_in_order')
     else
       $scope.standing_order_form.$setDirty()
       $scope.standingOrder.buildItem($scope.newItem)
 
   $scope.removeStandingLineItem = (item) ->
-    if confirm(t('are_you_sure'))
-      $scope.standing_order_form.$setDirty()
-      $scope.standingOrder.removeItem(item)
+    $scope.standing_order_form.$setDirty()
+    $scope.standingOrder.removeItem(item)
+
+  $scope.match = ->
+    matching = $scope.standingOrder.standing_line_items.filter (sli) ->
+      sli.variant_id == $scope.newItem.variant_id
+    return matching[0] if matching.length > 0
+    null
 
   $scope.estimatedSubtotal = ->
     $scope.standingOrder.standing_line_items.reduce (subtotal, item) ->
+      return subtotal if item._destroy
       subtotal += item.price_estimate * item.quantity
     , 0
 

app/assets/javascripts/admin/standing_orders/services/standing_order_prototype.js.coffee

@@ -11,15 +11,7 @@ angular.module("admin.standingOrders").factory 'StandingOrderPrototype', ($http,
       InfoDialog.open 'error', response.data.errors[0]
 
   removeItem: (item) ->
-    index = @standing_line_items.indexOf(item)
-    if item.id?
-      $http.delete("/admin/standing_line_items/#{item.id}").then (response) =>
-        $injector.get('StandingOrders').afterRemoveItem(@id, item.id) if $injector.has('StandingOrders')
-        @standing_line_items.splice(index,1)
-      , (response) ->
-        InfoDialog.open 'error', response.data.errors[0]
-    else
-      @standing_line_items.splice(index,1)
+    item._destroy = true
 
   create: ->
     StatusMessage.display 'progress', 'Saving...'

app/controllers/admin/standing_line_items_controller.rb

@@ -7,11 +7,6 @@ module Admin
 
     respond_to :json
 
-    respond_override destroy: { json: {
-      success: lambda { render nothing: true, :status => 204 },
-      failure: lambda { render json: { errors: @standing_line_item.errors.full_messages }, status: :unprocessable_entity }
-    } }
-
     def build
       return render json: { errors: ['Unauthorised'] }, status: :unauthorized unless @shop
       if @variant

app/controllers/admin/standing_orders_controller.rb

@@ -79,7 +79,7 @@ module Admin
     def wrap_nested_attrs
       if params[:standing_line_items].is_a? Array
         attributes = params[:standing_line_items].map do |sli|
-          sli.slice(*StandingLineItem.attribute_names)
+          sli.slice(*StandingLineItem.attribute_names + ["_destroy"])
         end
         params[:standing_order][:standing_line_items_attributes] = attributes
       end

app/forms/standing_order_form.rb

@@ -17,9 +17,9 @@ class StandingOrderForm
   end
 
   def save
-    @standing_order.transaction do
+    standing_order.transaction do
       validate_price_estimates
-      @standing_order.assign_attributes(params)
+      standing_order.assign_attributes(params)
 
       initialise_orders!
       remove_obsolete_orders!
@@ -42,6 +42,10 @@ class StandingOrderForm
         end
       end
 
+      standing_line_items.select(&:marked_for_destruction?).each do |sli|
+        updateable_line_items(sli).destroy_all
+      end
+
       future_and_undated_orders.each(&:save)
 
       standing_order.save
@@ -49,8 +53,8 @@ class StandingOrderForm
   end
 
   def json_errors
-    @standing_order.errors.messages.inject({}) do |errors, (k,v)|
-      errors[k] = v.map{ |msg| @standing_order.errors.full_message(k,msg) }
+    standing_order.errors.messages.inject({}) do |errors, (k,v)|
+      errors[k] = v.map{ |msg| standing_order.errors.full_message(k,msg) }
       errors
     end
   end

app/models/standing_order.rb

@@ -14,7 +14,8 @@ class StandingOrder < ActiveRecord::Base
   alias_attribute :billing_address, :bill_address
   alias_attribute :shipping_address, :ship_address
 
-  accepts_nested_attributes_for :standing_line_items, :bill_address, :ship_address
+  accepts_nested_attributes_for :standing_line_items, allow_destroy: true
+  accepts_nested_attributes_for :bill_address, :ship_address
 
   validates_presence_of :shop, :customer, :schedule, :payment_method, :shipping_method
   validates_presence_of :billing_address, :shipping_address, :begins_at

app/views/admin/standing_orders/_standing_line_items.html.haml

@@ -14,7 +14,7 @@
         %span= t(:total)
       %th.orders-actions.actions
   %tbody
-    %tr.item{ id: "sli_{{$index}}", ng: { repeat: 'item in standingOrder.standing_line_items', class: { even: 'even', odd: 'odd' } } }
+    %tr.item{ id: "sli_{{$index}}", ng: { repeat: "item in standingOrder.standing_line_items | filter:{ _destroy: '!true' }", class: { even: 'even', odd: 'odd' } } }
       %td.description {{ item.description }}
       %td.price.align-center {{ item.price_estimate | currency }}
       %td.qty

config/routes.rb

@@ -179,7 +179,7 @@ Openfoodnetwork::Application.routes.draw do
 
     resources :standing_orders, only: [:index, :new, :create, :edit, :update]
 
-    resources :standing_line_items, only: [:destroy], format: :json do
+    resources :standing_line_items, only: [], format: :json do
       post :build, on: :collection
     end
   end

spec/controllers/admin/standing_line_items_controller_spec.rb

@@ -95,30 +95,4 @@ describe Admin::StandingLineItemsController, type: :controller do
       end
     end
   end
-
-  describe "destroy" do
-    let!(:standing_order) { create(:standing_order_with_items) }
-    let!(:item) { standing_order.standing_line_items.first }
-    let!(:user) { create(:user) }
-    let(:params) { { format: :json, id: item.id } }
-
-    context 'as an enterprise user' do
-      before { allow(controller).to receive(:spree_current_user) { user } }
-
-      context "that does not manage the relevant shop" do
-        it "returns an error" do
-          spree_post :destroy, params
-          expect(response).to redirect_to spree.unauthorized_path
-        end
-      end
-
-      context 'that manages that relevant shop' do
-        before { standing_order.shop.update_attributes(owner: user) }
-
-        it 'removes the item' do
-          expect{ spree_post :destroy, params }.to change{standing_order.standing_line_items.count}.by(-1)
-        end
-      end
-    end
-  end
 end

spec/features/admin/standing_orders_spec.rb

@@ -129,7 +129,7 @@ feature 'Standing Orders' do
 
         # Deleting the existing product and adding a new product
         within 'table#standing-line-items tr.item', match: :first do
-          accept_alert "Are you sure?" do find("a.delete-item").click end
+          find("a.delete-item").click
         end
         targetted_select2_search product2.name, from: '#add_variant_id', dropdown_css: '.select2-drop'
         fill_in 'add_quantity', with: 3
@@ -221,7 +221,7 @@ feature 'Standing Orders' do
 
           # Remove variant1 from the standing order
           within '#sli_0', match: :first do
-            accept_alert "Are you sure?" do find("a.delete-item").click end
+            find("a.delete-item").click
           end
 
           # Total should be $35.25

spec/forms/standing_order_form_spec.rb

@@ -242,6 +242,24 @@ module OpenFoodNetwork
       end
     end
 
+    describe "removing an existing line item" do
+      let(:standing_order) { create(:standing_order_with_items) }
+      let(:sli) { standing_order.standing_line_items.first }
+      let(:variant) { sli.variant}
+      let(:params) { { standing_line_items_attributes: [ { id: sli.id, _destroy: true } ] } }
+      let(:form) { StandingOrderForm.new(standing_order, params) }
+
+      before { form.send(:initialise_orders!) }
+
+      it "removes the line item and updates totals on all orders" do
+        expect(standing_order.orders.first.reload.total.to_f).to eq 59.97
+        form.save
+        line_items = Spree::LineItem.where(order_id: standing_order.orders, variant_id: variant.id)
+        expect(line_items.count).to be 0
+        expect(standing_order.orders.first.reload.total.to_f).to eq 39.98
+      end
+    end
+
     describe "validating price_estimates on standing line items" do
       let(:params) { { } }
       let(:form) { StandingOrderForm.new(nil, params) }

spec/javascripts/unit/admin/standing_orders/controllers/standing_line_items_controller_spec.js.coffee

@@ -0,0 +1,71 @@
+describe "StandingLineItemsCtrl", ->
+  scope = null
+  http = null
+  standingOrder = { id: 1 }
+  standingLineItem = { id: 2, variant_id: 44 }
+
+  beforeEach ->
+    module('admin.standingOrders')
+
+    inject ($controller, $rootScope, $httpBackend) ->
+      scope = $rootScope
+      http = $httpBackend
+      scope.standingOrder = standingOrder
+      standingOrder.standing_line_items = [standingLineItem]
+      $controller 'StandingLineItemsController', {$scope: scope}
+
+  describe "match", ->
+    describe "when newItem.variant_id matches an existing sli", ->
+      beforeEach ->
+        scope.newItem.variant_id = 44
+
+      it "returns the matching sli ", ->
+        expect(scope.match()).toEqual standingLineItem
+
+    describe "when newItem.variant_id dosn't match an existing sli", ->
+      beforeEach ->
+        scope.newItem.variant_id = 43
+
+      it "returns null", ->
+        expect(scope.match()).toEqual null
+
+  describe "addStandingLineItem", ->
+    beforeEach ->
+      scope.standing_order_form = jasmine.createSpyObj('standing_order_form', ['$setDirty'])
+      standingOrder.buildItem = jasmine.createSpy('buildItem')
+
+    describe "when an item with a matching variant_id is found", ->
+      match = null
+
+      beforeEach ->
+        match = { }
+        scope.newItem.someProperty = "lalala"
+        spyOn(scope, "match").and.returnValue(match)
+
+      describe "when the matching item isn't marked for destruction", ->
+        InfoDialog = null
+
+        beforeEach inject (_InfoDialog_) ->
+          InfoDialog = _InfoDialog_
+          spyOn(InfoDialog, "open")
+
+        it "shows a message to the user", ->
+          scope.addStandingLineItem()
+          expect(InfoDialog.open).toHaveBeenCalled()
+
+      describe "when the matching item is marked for destruction", ->
+        beforeEach -> match._destroy = true
+
+        it "remove the delete flag from the match and merges properties from scope.newItem", ->
+          scope.addStandingLineItem()
+          expect(match._destroy).toBeUndefined()
+          expect(match.someProperty).toEqual "lalala"
+
+    describe "when no match is found", ->
+      beforeEach ->
+        spyOn(scope, "match").and.returnValue(null)
+
+      it "sets the form to $dirty and called buildItem on scope.standingOrder", ->
+        scope.addStandingLineItem()
+        expect(scope.standing_order_form.$setDirty).toHaveBeenCalled()
+        expect(standingOrder.buildItem).toHaveBeenCalledWith(scope.newItem)