Move JSON line items responses to a new controller

Extracts the JSON response from the admin's line item controller which
are only used by the bulk management feature into its own controller.

This decouples spree from an OFN-only feature and allows to remove
unnecessary code. Furthermore, Admin::LineItemsController is gone in
Spree 2.4.0.beta. See: https://github.com/spree/spree/pull/5280

app/assets/javascripts/admin/resources/resources/line_item_resource.js.coffee

@@ -1,5 +1,5 @@
 angular.module("admin.resources").factory 'LineItemResource', ($resource) ->
-  $resource('/admin/:orders/:order_number/line_items/:id.json', {}, {
+  $resource('/admin/bulk_line_items/:id.json', {}, {
     'index':
       method: 'GET'
       isArray: true

app/assets/javascripts/admin/resources/services/line_items.js.coffee

@@ -26,7 +26,7 @@ angular.module("admin.resources").factory 'LineItems', ($q, LineItemResource) ->
     save: (lineItem) ->
       deferred = $q.defer()
       lineItem.errors = {}
-      lineItem.$update({id: lineItem.id, orders: "orders", order_number: lineItem.order.number})
+      lineItem.$update({id: lineItem.id})
       .then( (data) =>
         @pristineByID[lineItem.id] = angular.copy(lineItem)
         deferred.resolve(data)
@@ -54,7 +54,7 @@ angular.module("admin.resources").factory 'LineItems', ($q, LineItemResource) ->
 
     delete: (lineItem, callback=null) ->
       deferred = $q.defer()
-      lineItem.$delete({id: lineItem.id, orders: "orders", order_number: lineItem.order.number})
+      lineItem.$delete({id: lineItem.id})
       .then( (data) =>
         delete @byID[lineItem.id]
         delete @pristineByID[lineItem.id]

app/controllers/admin/bulk_line_items_controller.rb

@@ -0,0 +1,53 @@
+module Admin
+  class BulkLineItemsController < Spree::Admin::BaseController
+
+    # GET /admin/bulk_line_items.json
+    #
+    def index
+      order_params = params[:q].andand.delete :order
+      orders = OpenFoodNetwork::Permissions.new(spree_current_user).editable_orders.ransack(order_params).result
+      line_items = OpenFoodNetwork::Permissions.new(spree_current_user).editable_line_items.where(order_id: orders).ransack(params[:q])
+      render_as_json line_items.result.reorder('order_id ASC, id ASC')
+    end
+
+    # PUT /admin/bulk_line_items/:id.json
+    #
+    def update
+      load_line_item
+      authorize! :update, @line_item.order
+
+      if @line_item.update_attributes(params[:line_item])
+        render nothing: true, status: 204 # No Content, does not trigger ng resource auto-update
+      else
+        render json: { errors: @line_item.errors }, status: 412
+      end
+    end
+
+    # DELETE /admin/bulk_line_items/:id.json
+    #
+    def destroy
+      load_line_item
+      authorize! :update, @line_item.order
+
+      @line_item.destroy
+      render nothing: true, status: 204 # No Content, does not trigger ng resource auto-update
+    end
+
+    private
+
+    def load_line_item
+      @line_item = Spree::LineItem.find(params[:id])
+    end
+
+    def model_class
+      Spree::LineItem
+    end
+
+    # Returns the appropriate serializer for this controller
+    #
+    # @return [Api::Admin::LineItemSerializer]
+    def serializer(_ams_prefix)
+      Api::Admin::LineItemSerializer
+    end
+  end
+end

app/controllers/spree/admin/line_items_controller_decorator.rb

@@ -2,19 +2,6 @@ Spree::Admin::LineItemsController.class_eval do
   prepend_before_filter :load_order, except: :index
   around_filter :apply_enterprise_fees_with_lock, only: :update
 
-  # TODO make updating line items faster by creating a bulk update method
-
-  def index
-    respond_to do |format|
-      format.json do
-        order_params = params[:q].andand.delete :order
-        orders = OpenFoodNetwork::Permissions.new(spree_current_user).editable_orders.ransack(order_params).result
-        line_items = OpenFoodNetwork::Permissions.new(spree_current_user).editable_line_items.where(order_id: orders).ransack(params[:q])
-        render_as_json line_items.result.reorder('order_id ASC, id ASC')
-      end
-    end
-  end
-
   def create
     variant = Spree::Variant.find(params[:line_item][:variant_id])
     OpenFoodNetwork::ScopeVariantToHub.new(@order.distributor).scope(variant)
@@ -34,7 +21,6 @@ Spree::Admin::LineItemsController.class_eval do
 
   # TODO: simplify this, 3 formats per action is too much
   #       we need `js` format for admin/orders/edit (jquery-rails gem)
-  #       we need `json` format for admin/orders/bulk_management (angular)
   #       we don't know if `html` format is needed
   def update
     respond_to do |format|
@@ -46,19 +32,11 @@ Spree::Admin::LineItemsController.class_eval do
           render json: { errors: @line_item.errors }, status: 412
         end
       }
-      format.json {
-        if @line_item.update_attributes(params[:line_item])
-          render nothing: true, status: 204 # No Content, does not trigger ng resource auto-update
-        else
-          render json: { errors: @line_item.errors }, status: 412
-        end
-      }
     end
   end
 
   # TODO: simplify this, 3 formats per action is too much:
   #       we need `js` format for admin/orders/edit (jquery-rails gem)
-  #       we need `json` format for admin/orders/bulk_management (angular)
   #       we don't know if `html` format is needed
   def destroy
     @line_item.destroy
@@ -66,7 +44,6 @@ Spree::Admin::LineItemsController.class_eval do
     respond_to do |format|
       format.html { render_order_form }
       format.js { render nothing: true, status: 204 } # No Content
-      format.json { render nothing: true, status: 204 } # No Content
     end
   end
 

config/routes.rb

@@ -53,6 +53,10 @@ Openfoodnetwork::Application.routes.draw do
     end
   end
 
+  namespace :admin do
+    resources :bulk_line_items
+  end
+
   get '/checkout', :to => 'checkout#edit' , :as => :checkout
   put '/checkout', :to => 'checkout#update' , :as => :update_checkout
   get '/checkout/paypal_payment/:order_id', to: 'checkout#paypal_payment', as: :paypal_payment
@@ -265,8 +269,6 @@ Spree::Core::Engine.routes.prepend do
       get :print_ticket, on: :member
       get :managed, on: :collection
     end
-
-    resources :line_items, only: [:index], format: :json
   end
 
   resources :orders do

spec/controllers/admin/bulk_line_items_controller_spec.rb

@@ -0,0 +1,249 @@
+require 'spec_helper'
+
+describe Admin::BulkLineItemsController do
+  include AuthenticationWorkflow
+
+  describe '#index' do
+    render_views
+
+    let(:line_item_attributes) { [:id, :quantity, :max_quantity, :price, :supplier, :final_weight_volume, :units_product, :units_variant, :order] }
+    let!(:dist1) { FactoryGirl.create(:distributor_enterprise) }
+    let!(:order1) { FactoryGirl.create(:order, state: 'complete', completed_at: 1.day.ago, distributor: dist1, billing_address: FactoryGirl.create(:address) ) }
+    let!(:order2) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.zone.now, distributor: dist1, billing_address: FactoryGirl.create(:address) ) }
+    let!(:order3) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.zone.now, distributor: dist1, billing_address: FactoryGirl.create(:address) ) }
+    let!(:line_item1) { FactoryGirl.create(:line_item, order: order1) }
+    let!(:line_item2) { FactoryGirl.create(:line_item, order: order2) }
+    let!(:line_item3) { FactoryGirl.create(:line_item, order: order2) }
+    let!(:line_item4) { FactoryGirl.create(:line_item, order: order3) }
+
+    context "as a normal user" do
+      before { controller.stub spree_current_user: create_enterprise_user }
+
+      it "should deny me access to the index action" do
+        spree_get :index, :format => :json
+        expect(response).to redirect_to spree.unauthorized_path
+      end
+    end
+
+    context "as an administrator" do
+
+      before do
+        controller.stub spree_current_user: quick_login_as_admin
+      end
+
+      context "when no ransack params are passed in" do
+        before do
+          spree_get :index, :format => :json
+        end
+
+        it "retrieves a list of line_items with appropriate attributes, including line items with appropriate attributes" do
+          keys = json_response.first.keys.map{ |key| key.to_sym }
+          line_item_attributes.all?{ |attr| keys.include? attr }.should == true
+        end
+
+        it "sorts line_items in ascending id line_item" do
+          ids = json_response.map{ |line_item| line_item['id'] }
+          ids[0].should < ids[1]
+          ids[1].should < ids[2]
+        end
+
+        it "formats final_weight_volume as a float" do
+          json_response.map{ |line_item| line_item['final_weight_volume'] }.all?{ |fwv| fwv.is_a?(Float) }.should == true
+        end
+
+        it "returns distributor object with id key" do
+          json_response.map{ |line_item| line_item['supplier'] }.all?{ |d| d.has_key?('id') }.should == true
+        end
+      end
+
+      context "when ransack params are passed in for line items" do
+        before do
+          spree_get :index, :format => :json, q: { order_id_eq: order2.id }
+        end
+
+        it "retrives a list of line items which match the criteria" do
+          expect(json_response.map{ |line_item| line_item['id'] }).to eq [line_item2.id, line_item3.id]
+        end
+      end
+
+      context "when ransack params are passed in for orders" do
+        before do
+          spree_get :index, :format => :json, q: { order: { completed_at_gt: 2.hours.ago } }
+        end
+
+        it "retrives a list of line items whose orders match the criteria" do
+          expect(json_response.map{ |line_item| line_item['id'] }).to eq [line_item2.id, line_item3.id, line_item4.id]
+        end
+      end
+    end
+
+    context "as an enterprise user" do
+      let(:supplier) { create(:supplier_enterprise) }
+      let(:distributor1) { create(:distributor_enterprise) }
+      let(:distributor2) { create(:distributor_enterprise) }
+      let(:coordinator) { create(:distributor_enterprise) }
+      let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator) }
+      let!(:order1) { FactoryGirl.create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, distributor: distributor1, billing_address: FactoryGirl.create(:address) ) }
+      let!(:line_item1) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) }
+      let!(:line_item2) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) }
+      let!(:order2) { FactoryGirl.create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, distributor: distributor2, billing_address: FactoryGirl.create(:address) ) }
+      let!(:line_item3) { FactoryGirl.create(:line_item, order: order2, product: FactoryGirl.create(:product, supplier: supplier)) }
+
+      context "producer enterprise" do
+        before do
+          controller.stub spree_current_user: supplier.owner
+          spree_get :index, :format => :json
+        end
+
+        it "does not display line items for which my enterprise is a supplier" do
+          expect(response).to redirect_to spree.unauthorized_path
+        end
+      end
+
+      context "coordinator enterprise" do
+        before do
+          controller.stub spree_current_user: coordinator.owner
+          spree_get :index, :format => :json
+        end
+
+        it "retrieves a list of line_items" do
+          keys = json_response.first.keys.map{ |key| key.to_sym }
+          line_item_attributes.all?{ |attr| keys.include? attr }.should == true
+        end
+      end
+
+      context "hub enterprise" do
+        before do
+          controller.stub spree_current_user: distributor1.owner
+          spree_get :index, :format => :json
+        end
+
+        it "retrieves a list of line_items" do
+          keys = json_response.first.keys.map{ |key| key.to_sym }
+          line_item_attributes.all?{ |attr| keys.include? attr }.should == true
+        end
+      end
+    end
+  end
+
+  describe '#update' do
+    let(:supplier) { create(:supplier_enterprise) }
+    let(:distributor1) { create(:distributor_enterprise) }
+    let(:coordinator) { create(:distributor_enterprise) }
+    let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator) }
+    let!(:order1) { FactoryGirl.create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, distributor: distributor1, billing_address: FactoryGirl.create(:address) ) }
+    let!(:line_item1) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) }
+    let(:line_item_params) { { quantity: 3, final_weight_volume: 3000, price: 3.00 } }
+    let(:params) { { id: line_item1.id, order_id: order1.number, line_item: line_item_params } }
+
+    context "as an enterprise user" do
+      context "producer enterprise" do
+        before do
+          controller.stub spree_current_user: supplier.owner
+          spree_put :update, params
+        end
+
+        it "does not allow access" do
+          expect(response).to redirect_to spree.unauthorized_path
+        end
+      end
+
+      context "coordinator enterprise" do
+        render_views
+
+        before do
+          controller.stub spree_current_user: coordinator.owner
+        end
+
+        # Used in admin/orders/bulk_management
+        context 'when the request is JSON (angular)' do
+          before { params[:format] = :json }
+
+          it "updates the line item" do
+            spree_put :update, params
+            line_item1.reload
+            expect(line_item1.quantity).to eq 3
+            expect(line_item1.final_weight_volume).to eq 3000
+            expect(line_item1.price).to eq 3.00
+          end
+
+          it "returns an empty JSON response" do
+            spree_put :update, params
+            expect(response.body).to eq ' '
+          end
+
+          it 'returns a 204 response' do
+            spree_put :update, params
+            expect(response.status).to eq 204
+          end
+
+          context 'when the line item params are not correct' do
+            let(:line_item_params) { { price: 'hola' } }
+            let(:errors) { { 'price' => ['is not a number'] } }
+
+            it 'returns a JSON with the errors' do
+              spree_put :update, params
+              expect(JSON.parse(response.body)['errors']).to eq(errors)
+            end
+
+            it 'returns a 412 response' do
+              spree_put :update, params
+              expect(response.status).to eq 412
+            end
+          end
+        end
+      end
+
+      context "hub enterprise" do
+        before do
+          controller.stub spree_current_user: distributor1.owner
+          xhr :put, :update, params
+        end
+
+        it "updates the line item" do
+          line_item1.reload
+          expect(line_item1.quantity).to eq 3
+          expect(line_item1.final_weight_volume).to eq 3000
+          expect(line_item1.price).to eq 3.00
+        end
+      end
+    end
+  end
+
+  describe '#destroy' do
+    render_views
+
+    let(:supplier) { create(:supplier_enterprise) }
+    let(:distributor1) { create(:distributor_enterprise) }
+    let(:coordinator) { create(:distributor_enterprise) }
+    let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator) }
+    let!(:order1) { FactoryGirl.create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, distributor: distributor1, billing_address: FactoryGirl.create(:address) ) }
+    let!(:line_item1) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) }
+    let(:params) { { id: line_item1.id, order_id: order1.number } }
+
+    before do
+      controller.stub spree_current_user: coordinator.owner
+    end
+
+    # Used in admin/orders/bulk_management
+    context 'when the request is JSON (angular)' do
+      before { params[:format] = :json }
+
+      it 'destroys the line item' do
+        expect {
+          spree_delete :destroy, params
+        }.to change { Spree::LineItem.where(id: line_item1).count }.from(1).to(0)
+      end
+
+      it 'returns an empty JSON response' do
+        spree_delete :destroy, params
+        expect(response.body).to eq ' '
+      end
+
+      it 'returns a 204 response' do
+        spree_delete :destroy, params
+        expect(response.status).to eq 204
+      end
+    end
+  end
+end

spec/controllers/spree/admin/line_items_controller_spec.rb

@@ -3,129 +3,6 @@ require 'spec_helper'
 describe Spree::Admin::LineItemsController do
   include AuthenticationWorkflow
 
-  describe "#index" do
-    render_views
-
-    let(:line_item_attributes) { [:id, :quantity, :max_quantity, :price, :supplier, :final_weight_volume, :units_product, :units_variant, :order] }
-    let!(:dist1) { FactoryGirl.create(:distributor_enterprise) }
-    let!(:order1) { FactoryGirl.create(:order, state: 'complete', completed_at: 1.day.ago, distributor: dist1, billing_address: FactoryGirl.create(:address) ) }
-    let!(:order2) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.zone.now, distributor: dist1, billing_address: FactoryGirl.create(:address) ) }
-    let!(:order3) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.zone.now, distributor: dist1, billing_address: FactoryGirl.create(:address) ) }
-    let!(:line_item1) { FactoryGirl.create(:line_item, order: order1) }
-    let!(:line_item2) { FactoryGirl.create(:line_item, order: order2) }
-    let!(:line_item3) { FactoryGirl.create(:line_item, order: order2) }
-    let!(:line_item4) { FactoryGirl.create(:line_item, order: order3) }
-
-    context "as a normal user" do
-      before { controller.stub spree_current_user: create_enterprise_user }
-
-      it "should deny me access to the index action" do
-        spree_get :index, :format => :json
-        expect(response).to redirect_to spree.unauthorized_path
-      end
-    end
-
-    context "as an administrator" do
-
-      before do
-        controller.stub spree_current_user: quick_login_as_admin
-      end
-
-      context "when no ransack params are passed in" do
-        before do
-          spree_get :index, :format => :json
-        end
-
-        it "retrieves a list of line_items with appropriate attributes, including line items with appropriate attributes" do
-          keys = json_response.first.keys.map{ |key| key.to_sym }
-          line_item_attributes.all?{ |attr| keys.include? attr }.should == true
-        end
-
-        it "sorts line_items in ascending id line_item" do
-          ids = json_response.map{ |line_item| line_item['id'] }
-          ids[0].should < ids[1]
-          ids[1].should < ids[2]
-        end
-
-        it "formats final_weight_volume as a float" do
-          json_response.map{ |line_item| line_item['final_weight_volume'] }.all?{ |fwv| fwv.is_a?(Float) }.should == true
-        end
-
-        it "returns distributor object with id key" do
-          json_response.map{ |line_item| line_item['supplier'] }.all?{ |d| d.has_key?('id') }.should == true
-        end
-      end
-
-      context "when ransack params are passed in for line items" do
-        before do
-          spree_get :index, :format => :json, q: { order_id_eq: order2.id }
-        end
-
-        it "retrives a list of line items which match the criteria" do
-          expect(json_response.map{ |line_item| line_item['id'] }).to eq [line_item2.id, line_item3.id]
-        end
-      end
-
-      context "when ransack params are passed in for orders" do
-        before do
-          spree_get :index, :format => :json, q: { order: { completed_at_gt: 2.hours.ago } }
-        end
-
-        it "retrives a list of line items whose orders match the criteria" do
-          expect(json_response.map{ |line_item| line_item['id'] }).to eq [line_item2.id, line_item3.id, line_item4.id]
-        end
-      end
-    end
-
-    context "as an enterprise user" do
-      let(:supplier) { create(:supplier_enterprise) }
-      let(:distributor1) { create(:distributor_enterprise) }
-      let(:distributor2) { create(:distributor_enterprise) }
-      let(:coordinator) { create(:distributor_enterprise) }
-      let(:order_cycle) { create(:simple_order_cycle, coordinator: coordinator) }
-      let!(:order1) { FactoryGirl.create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, distributor: distributor1, billing_address: FactoryGirl.create(:address) ) }
-      let!(:line_item1) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) }
-      let!(:line_item2) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) }
-      let!(:order2) { FactoryGirl.create(:order, order_cycle: order_cycle, state: 'complete', completed_at: Time.zone.now, distributor: distributor2, billing_address: FactoryGirl.create(:address) ) }
-      let!(:line_item3) { FactoryGirl.create(:line_item, order: order2, product: FactoryGirl.create(:product, supplier: supplier)) }
-
-      context "producer enterprise" do
-        before do
-          controller.stub spree_current_user: supplier.owner
-          spree_get :index, :format => :json
-        end
-
-        it "does not display line items for which my enterprise is a supplier" do
-          expect(response).to redirect_to spree.unauthorized_path
-        end
-      end
-
-      context "coordinator enterprise" do
-        before do
-          controller.stub spree_current_user: coordinator.owner
-          spree_get :index, :format => :json
-        end
-
-        it "retrieves a list of line_items" do
-          keys = json_response.first.keys.map{ |key| key.to_sym }
-          line_item_attributes.all?{ |attr| keys.include? attr }.should == true
-        end
-      end
-
-      context "hub enterprise" do
-        before do
-          controller.stub spree_current_user: distributor1.owner
-          spree_get :index, :format => :json
-        end
-
-        it "retrieves a list of line_items" do
-          keys = json_response.first.keys.map{ |key| key.to_sym }
-          line_item_attributes.all?{ |attr| keys.include? attr }.should == true
-        end
-      end
-    end
-  end
-
   describe "#create" do
     let!(:variant) { create(:variant, price: 88) }
     let!(:vo) { create(:variant_override, hub: distributor, variant: variant, price: 11.11) }