From a57815edbb510dcda89aec92d3e12681b3377ac4 Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Thu, 1 Dec 2016 16:33:55 +1100 Subject: [PATCH] Refactoring standing line item delete logic Using StandingOrderForm rather than separate endpoint --- .../standing_line_items_controller.js.coffee | 22 ++++-- .../standing_order_prototype.js.coffee | 10 +-- .../admin/standing_line_items_controller.rb | 5 -- .../admin/standing_orders_controller.rb | 2 +- app/forms/standing_order_form.rb | 12 ++-- app/models/standing_order.rb | 3 +- .../_standing_line_items.html.haml | 2 +- config/routes.rb | 2 +- .../standing_line_items_controller_spec.rb | 26 ------- spec/features/admin/standing_orders_spec.rb | 4 +- spec/forms/standing_order_form_spec.rb | 18 +++++ ...nding_line_items_controller_spec.js.coffee | 71 +++++++++++++++++++ 12 files changed, 121 insertions(+), 56 deletions(-) create mode 100644 spec/javascripts/unit/admin/standing_orders/controllers/standing_line_items_controller_spec.js.coffee diff --git a/app/assets/javascripts/admin/standing_orders/controllers/standing_line_items_controller.js.coffee b/app/assets/javascripts/admin/standing_orders/controllers/standing_line_items_controller.js.coffee index b9ebb14faa..1e545c9bcb 100644 --- a/app/assets/javascripts/admin/standing_orders/controllers/standing_line_items_controller.js.coffee +++ b/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 diff --git a/app/assets/javascripts/admin/standing_orders/services/standing_order_prototype.js.coffee b/app/assets/javascripts/admin/standing_orders/services/standing_order_prototype.js.coffee index 9597cbe6a0..976d5acfa3 100644 --- a/app/assets/javascripts/admin/standing_orders/services/standing_order_prototype.js.coffee +++ b/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...' diff --git a/app/controllers/admin/standing_line_items_controller.rb b/app/controllers/admin/standing_line_items_controller.rb index ec1bdffd82..60e09f96e8 100644 --- a/app/controllers/admin/standing_line_items_controller.rb +++ b/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 diff --git a/app/controllers/admin/standing_orders_controller.rb b/app/controllers/admin/standing_orders_controller.rb index 0f187a73f7..fd752a700f 100644 --- a/app/controllers/admin/standing_orders_controller.rb +++ b/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 diff --git a/app/forms/standing_order_form.rb b/app/forms/standing_order_form.rb index d61a4e9cfa..1d26aa9b34 100644 --- a/app/forms/standing_order_form.rb +++ b/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 diff --git a/app/models/standing_order.rb b/app/models/standing_order.rb index cd2dcc9869..bfc5803068 100644 --- a/app/models/standing_order.rb +++ b/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 diff --git a/app/views/admin/standing_orders/_standing_line_items.html.haml b/app/views/admin/standing_orders/_standing_line_items.html.haml index 6a4b40e484..f14e72cab3 100644 --- a/app/views/admin/standing_orders/_standing_line_items.html.haml +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index ca19716a40..0056651de9 100644 --- a/config/routes.rb +++ b/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 diff --git a/spec/controllers/admin/standing_line_items_controller_spec.rb b/spec/controllers/admin/standing_line_items_controller_spec.rb index 3cbdf78a93..fbb51015d8 100644 --- a/spec/controllers/admin/standing_line_items_controller_spec.rb +++ b/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 diff --git a/spec/features/admin/standing_orders_spec.rb b/spec/features/admin/standing_orders_spec.rb index f57224b077..2cac69418f 100644 --- a/spec/features/admin/standing_orders_spec.rb +++ b/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 diff --git a/spec/forms/standing_order_form_spec.rb b/spec/forms/standing_order_form_spec.rb index e506836eaf..03a31a53ff 100644 --- a/spec/forms/standing_order_form_spec.rb +++ b/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) } diff --git a/spec/javascripts/unit/admin/standing_orders/controllers/standing_line_items_controller_spec.js.coffee b/spec/javascripts/unit/admin/standing_orders/controllers/standing_line_items_controller_spec.js.coffee new file mode 100644 index 0000000000..4d7adf3fdf --- /dev/null +++ b/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)