From 8db1fa4e77b31f786b0d4b7d04aeb05ac811296c Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 9 Aug 2017 14:14:10 +0200 Subject: [PATCH] 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 --- .../resources/line_item_resource.js.coffee | 2 +- .../resources/services/line_items.js.coffee | 4 +- .../admin/bulk_line_items_controller.rb | 53 ++++ .../admin/line_items_controller_decorator.rb | 23 -- config/routes.rb | 6 +- .../admin/bulk_line_items_controller_spec.rb | 249 ++++++++++++++++++ .../spree/admin/line_items_controller_spec.rb | 123 --------- 7 files changed, 309 insertions(+), 151 deletions(-) create mode 100644 app/controllers/admin/bulk_line_items_controller.rb create mode 100644 spec/controllers/admin/bulk_line_items_controller_spec.rb diff --git a/app/assets/javascripts/admin/resources/resources/line_item_resource.js.coffee b/app/assets/javascripts/admin/resources/resources/line_item_resource.js.coffee index 4301f8df82..2ec5703c82 100644 --- a/app/assets/javascripts/admin/resources/resources/line_item_resource.js.coffee +++ b/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 diff --git a/app/assets/javascripts/admin/resources/services/line_items.js.coffee b/app/assets/javascripts/admin/resources/services/line_items.js.coffee index 182ef81e6a..7c96caecd8 100644 --- a/app/assets/javascripts/admin/resources/services/line_items.js.coffee +++ b/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] diff --git a/app/controllers/admin/bulk_line_items_controller.rb b/app/controllers/admin/bulk_line_items_controller.rb new file mode 100644 index 0000000000..044038bbfb --- /dev/null +++ b/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 diff --git a/app/controllers/spree/admin/line_items_controller_decorator.rb b/app/controllers/spree/admin/line_items_controller_decorator.rb index 7d76cda245..637fc82a11 100644 --- a/app/controllers/spree/admin/line_items_controller_decorator.rb +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index 02c459cea0..b91779166c 100644 --- a/config/routes.rb +++ b/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 diff --git a/spec/controllers/admin/bulk_line_items_controller_spec.rb b/spec/controllers/admin/bulk_line_items_controller_spec.rb new file mode 100644 index 0000000000..a993285c59 --- /dev/null +++ b/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 diff --git a/spec/controllers/spree/admin/line_items_controller_spec.rb b/spec/controllers/spree/admin/line_items_controller_spec.rb index 2aced0ad0c..0c9a4e4835 100644 --- a/spec/controllers/spree/admin/line_items_controller_spec.rb +++ b/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) }