From 180598c603fb35526b8d673d9757a981ffe06438 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Mon, 22 Jul 2019 18:41:47 +0100 Subject: [PATCH 01/13] Convert spree/api/variants_controller to AMS by changing base_controller, using render json instad of respond with, deleting rabl templates and adapting specs Delete unused pagination spec --- app/controllers/spree/api/variants_controller.rb | 15 ++++++++------- app/serializers/api/variant_serializer.rb | 6 ++++-- app/views/spree/api/products/bulk_show.v1.rabl | 8 +------- app/views/spree/api/variants/bulk_index.v1.rabl | 2 -- app/views/spree/api/variants/bulk_show.v1.rabl | 7 ------- .../spree/api/variants_controller_spec.rb | 15 +-------------- 6 files changed, 14 insertions(+), 39 deletions(-) delete mode 100644 app/views/spree/api/variants/bulk_index.v1.rabl delete mode 100644 app/views/spree/api/variants/bulk_show.v1.rabl diff --git a/app/controllers/spree/api/variants_controller.rb b/app/controllers/spree/api/variants_controller.rb index fe0fa1d793..e634a3900f 100644 --- a/app/controllers/spree/api/variants_controller.rb +++ b/app/controllers/spree/api/variants_controller.rb @@ -1,19 +1,20 @@ module Spree module Api - class VariantsController < Spree::Api::BaseController + class VariantsController < ::Api::BaseController respond_to :json + skip_authorization_check only: :index before_filter :product def index @variants = scope.includes(:option_values).ransack(params[:q]).result. page(params[:page]).per(params[:per_page]) - respond_with(@variants) + render json: @variants, each_serializer: ::Api::VariantSerializer end def show @variant = scope.includes(:option_values).find(params[:id]) - respond_with(@variant) + render json: @variant, serializer: ::Api::VariantSerializer end def new; end @@ -22,7 +23,7 @@ module Spree authorize! :create, Variant @variant = scope.new(params[:variant]) if @variant.save - respond_with(@variant, status: 201, default_template: :show) + render json: @variant, serializer: ::Api::VariantSerializer, status: 201 else invalid_resource!(@variant) end @@ -32,7 +33,7 @@ module Spree authorize! :update, Variant @variant = scope.find(params[:id]) if @variant.update_attributes(params[:variant]) - respond_with(@variant, status: 200, default_template: :show) + render json: @variant, serializer: ::Api::VariantSerializer, status: 200 else invalid_resource!(@product) end @@ -43,14 +44,14 @@ module Spree authorize! :delete, @variant VariantDeleter.new.delete(@variant) - respond_with @variant, status: 204 + render json: @variant, serializer: ::Api::VariantSerializer, status: 204 end def destroy authorize! :delete, Variant @variant = scope.find(params[:id]) @variant.destroy - respond_with(@variant, status: 204) + render json: @variant, serializer: ::Api::VariantSerializer, status: 204 end private diff --git a/app/serializers/api/variant_serializer.rb b/app/serializers/api/variant_serializer.rb index b7cc365120..3972c6bf4c 100644 --- a/app/serializers/api/variant_serializer.rb +++ b/app/serializers/api/variant_serializer.rb @@ -1,6 +1,8 @@ class Api::VariantSerializer < ActiveModel::Serializer - attributes :id, :is_master, :on_hand, :name_to_display, :unit_to_display, :unit_value - attributes :options_text, :on_demand, :price, :fees, :price_with_fees, :product_name + attributes :id, :is_master, :product_name + attributes :options_text, :unit_value, :unit_description, :unit_to_display + attributes :display_as, :display_name, :name_to_display + attributes :price, :on_demand, :on_hand, :fees, :price_with_fees attributes :tag_list delegate :price, to: :object diff --git a/app/views/spree/api/products/bulk_show.v1.rabl b/app/views/spree/api/products/bulk_show.v1.rabl index 3baf5a5069..0786c5d844 100644 --- a/app/views/spree/api/products/bulk_show.v1.rabl +++ b/app/views/spree/api/products/bulk_show.v1.rabl @@ -18,10 +18,4 @@ node( :producer_id, &:supplier_id ) node( :category_id, &:primary_taxon_id ) node( :supplier ) do |p| partial 'api/enterprises/bulk_show', object: p.supplier -end -node( :variants ) do |p| - partial 'spree/api/variants/bulk_index', object: p.variants.reorder('spree_variants.id ASC') -end -node( :master ) do |p| - partial 'spree/api/variants/bulk_show', object: p.master -end +end \ No newline at end of file diff --git a/app/views/spree/api/variants/bulk_index.v1.rabl b/app/views/spree/api/variants/bulk_index.v1.rabl deleted file mode 100644 index 69f1b82f5f..0000000000 --- a/app/views/spree/api/variants/bulk_index.v1.rabl +++ /dev/null @@ -1,2 +0,0 @@ -collection @variants -extends "spree/api/variants/bulk_show" diff --git a/app/views/spree/api/variants/bulk_show.v1.rabl b/app/views/spree/api/variants/bulk_show.v1.rabl deleted file mode 100644 index 8044ded0a0..0000000000 --- a/app/views/spree/api/variants/bulk_show.v1.rabl +++ /dev/null @@ -1,7 +0,0 @@ -object @variant - -attributes :id, :options_text, :unit_value, :unit_description, :on_demand, :display_as, :display_name - -# Infinity is not a valid JSON object, but Rails encodes it anyway -node( :on_hand ) { |v| v.on_hand.nil? ? 0 : ( v.on_hand.to_f.finite? ? v.on_hand : t(:on_demand) ) } -node( :price ) { |v| v.price.nil? ? 0.to_f : v.price } diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index e32f34809a..5ffede16d0 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -29,7 +29,7 @@ module Spree end it "retrieves a list of variants with appropriate attributes" do - spree_get :index, template: 'bulk_index', format: :json + spree_get :index, format: :json keys = json_response.first.keys.map(&:to_sym) expect(attributes.all?{ |attr| keys.include? attr }).to eq(true) @@ -108,19 +108,6 @@ module Spree end end - context "pagination" do - it "can select the next page of variants" do - second_variant = create(:variant) - api_get :index, page: 2, per_page: 1 - - keys = json_response["variants"].first.keys.map(&:to_sym) - expect(standard_attributes.all?{ |attr| keys.include? attr }).to eq(true) - expect(json_response["total_count"]).to eq(14) - expect(json_response["current_page"]).to eq(2) - expect(json_response["pages"]).to eq(14) - end - end - it "can see a single variant" do api_get :show, id: variant.to_param From c3fbf9cdf97f1760d6e5ff3b4e7a77e1abb221b6 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Tue, 30 Jul 2019 23:32:23 +0100 Subject: [PATCH 02/13] Remove unused pagination from index and respective specs, fix spec for search by sku by adding sku to the serializer and adapt a few specs to pass with AMS attrivbutes, --- .../spree/api/variants_controller.rb | 7 +- app/serializers/api/variant_serializer.rb | 2 +- .../spree/api/products/bulk_show.v1.rabl | 2 +- .../spree/api/variants_controller_spec.rb | 77 ++----------------- 4 files changed, 12 insertions(+), 76 deletions(-) diff --git a/app/controllers/spree/api/variants_controller.rb b/app/controllers/spree/api/variants_controller.rb index e634a3900f..2f1e6ab342 100644 --- a/app/controllers/spree/api/variants_controller.rb +++ b/app/controllers/spree/api/variants_controller.rb @@ -3,12 +3,11 @@ module Spree class VariantsController < ::Api::BaseController respond_to :json - skip_authorization_check only: :index + skip_authorization_check only: [:index, :show] before_filter :product def index - @variants = scope.includes(:option_values).ransack(params[:q]).result. - page(params[:page]).per(params[:per_page]) + @variants = scope.includes(:option_values).ransack(params[:q]).result render json: @variants, each_serializer: ::Api::VariantSerializer end @@ -17,8 +16,6 @@ module Spree render json: @variant, serializer: ::Api::VariantSerializer end - def new; end - def create authorize! :create, Variant @variant = scope.new(params[:variant]) diff --git a/app/serializers/api/variant_serializer.rb b/app/serializers/api/variant_serializer.rb index 3972c6bf4c..518dd60d7c 100644 --- a/app/serializers/api/variant_serializer.rb +++ b/app/serializers/api/variant_serializer.rb @@ -1,5 +1,5 @@ class Api::VariantSerializer < ActiveModel::Serializer - attributes :id, :is_master, :product_name + attributes :id, :is_master, :product_name, :sku attributes :options_text, :unit_value, :unit_description, :unit_to_display attributes :display_as, :display_name, :name_to_display attributes :price, :on_demand, :on_hand, :fees, :price_with_fees diff --git a/app/views/spree/api/products/bulk_show.v1.rabl b/app/views/spree/api/products/bulk_show.v1.rabl index 0786c5d844..5f6b600287 100644 --- a/app/views/spree/api/products/bulk_show.v1.rabl +++ b/app/views/spree/api/products/bulk_show.v1.rabl @@ -18,4 +18,4 @@ node( :producer_id, &:supplier_id ) node( :category_id, &:primary_taxon_id ) node( :supplier ) do |p| partial 'api/enterprises/bulk_show', object: p.supplier -end \ No newline at end of file +end diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 5ffede16d0..2e5fef6cd2 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -9,10 +9,6 @@ module Spree let!(:variant2) { FactoryBot.create(:variant) } let!(:variant3) { FactoryBot.create(:variant) } let(:attributes) { [:id, :options_text, :price, :on_hand, :unit_value, :unit_description, :on_demand, :display_as, :display_name] } - let!(:standard_attributes) { - [:id, :name, :sku, :price, :weight, :height, - :width, :depth, :is_master, :cost_price, :permalink] - } before do allow(controller).to receive(:spree_current_user) { current_api_user } @@ -45,50 +41,12 @@ module Spree expect(variant.deleted_at).to be_nil end - it "can see a paginated list of variants" do - api_get :index - - keys = json_response["variants"].first.keys.map(&:to_sym) - expect(standard_attributes.all?{ |attr| keys.include? attr }).to eq(true) - expect(json_response["count"]).to eq(11) - expect(json_response["current_page"]).to eq(1) - expect(json_response["pages"]).to eq(1) - end - - it 'can control the page size through a parameter' do - create(:variant) - api_get :index, per_page: 1 - - expect(json_response['count']).to eq(1) - expect(json_response['current_page']).to eq(1) - expect(json_response['pages']).to eq(14) - end - - it 'can query the results through a paramter' do + it 'can query the results through a parameter' do expected_result = create(:variant, sku: 'FOOBAR') api_get :index, q: { sku_cont: 'FOO' } - expect(json_response['count']).to eq(1) - expect(json_response['variants'].first['sku']).to eq expected_result.sku - end - - it "variants returned contain option values data" do - api_get :index - - option_values = json_response["variants"].last["option_values"] - expect(option_values.first).to have_attributes(keys: ["id", - "name", - "presentation", - "option_type_name", - "option_type_id"]) - end - - it "variants returned contain images data" do - variant.images.create!(attachment: image("thinking-cat.jpg")) - - api_get :index - - expect(json_response["variants"].last["images"]).not_to be_nil + expect(json_response.size).to eq(1) + expect(json_response.first['sku']).to eq expected_result.sku end # Regression test for spree#2141 @@ -99,12 +57,12 @@ module Spree it "is not returned in the results" do api_get :index - expect(json_response["variants"].count).to eq(10) # there are 11 variants + expect(json_response.count).to eq(10) # there are 11 variants end it "is not returned even when show_deleted is passed" do api_get :index, show_deleted: true - expect(json_response["variants"].count).to eq(10) # there are 11 variants + expect(json_response.count).to eq(10) # there are 11 variants end end @@ -112,26 +70,7 @@ module Spree api_get :show, id: variant.to_param keys = json_response.keys.map(&:to_sym) - expect((standard_attributes + [:options_text, :option_values, :images]).all?{ |attr| keys.include? attr }).to eq(true) - option_values = json_response["option_values"] - expect(option_values.first).to have_attributes(keys: ["id", "name", "presentation", "option_type_name", "option_type_id"]) - end - - it "can see a single variant with images" do - variant.images.create!(attachment: image("thinking-cat.jpg")) - api_get :show, id: variant.to_param - - keys = json_response.keys.map(&:to_sym) - expect((standard_attributes + [:images]).all?{ |attr| keys.include? attr }).to eq(true) - option_values_keys = json_response["option_values"].first.keys.map(&:to_sym) - expect([:name, :presentation, :option_type_id].all?{ |attr| option_values_keys.include? attr }).to eq(true) - end - - it "can learn how to create a new variant" do - api_get :new - - expect(json_response["attributes"]).to eq(standard_attributes.map(&:to_s)) - expect(json_response["required_attributes"]).to be_empty + expect((attributes).all?{ |attr| keys.include? attr }).to eq(true) end it "cannot create a new variant if not an admin" do @@ -229,7 +168,7 @@ module Spree it "are visible by admin" do api_get :index, show_deleted: 1 - expect(json_response["variants"].count).to eq(2) + expect(json_response.count).to eq(2) end end @@ -237,7 +176,7 @@ module Spree original_number_of_variants = variant.product.variants.count api_post :create, variant: { sku: "12345", unit_value: "weight", unit_description: "L" } - expect(standard_attributes.all?{ |attr| json_response.include? attr.to_s }).to eq(true) + expect(attributes.all?{ |attr| json_response.include? attr.to_s }).to eq(true) expect(response.status).to eq(201) expect(json_response["sku"]).to eq("12345") expect(variant.product.variants.count).to eq(original_number_of_variants + 1) From 07aececdcff76f8ac46cf2a2215ab899022aa60e Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 09:47:29 +0100 Subject: [PATCH 03/13] Remove unused route api/products managed --- .../spree/api/products_controller.rb | 11 ----------- config/routes/spree.rb | 1 - .../spree/api/products_controller_spec.rb | 17 ----------------- 3 files changed, 29 deletions(-) diff --git a/app/controllers/spree/api/products_controller.rb b/app/controllers/spree/api/products_controller.rb index 615596383e..03bf8a7775 100644 --- a/app/controllers/spree/api/products_controller.rb +++ b/app/controllers/spree/api/products_controller.rb @@ -58,17 +58,6 @@ module Spree respond_with(@product, status: 204) end - def managed - authorize! :admin, Spree::Product - authorize! :read, Spree::Product - - @products = product_scope. - ransack(params[:q]).result. - managed_by(current_api_user). - page(params[:page]).per(params[:per_page]) - respond_with(@products, default_template: :index) - end - # TODO: This should be named 'managed'. Is the action above used? Maybe we should remove it. def bulk_products @products = OpenFoodNetwork::Permissions.new(current_api_user).editable_products. diff --git a/config/routes/spree.rb b/config/routes/spree.rb index 05a709f0ff..2007665100 100644 --- a/config/routes/spree.rb +++ b/config/routes/spree.rb @@ -64,7 +64,6 @@ Spree::Core::Engine.routes.prepend do resources :products do collection do - get :managed get :bulk_products get :overridable end diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index c726487052..47907526b5 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -25,11 +25,6 @@ module Spree .to receive(:has_spree_role?).with("admin").and_return(false) end - it "should deny me access to managed products" do - spree_get :managed, template: 'bulk_index', format: :json - assert_unauthorized! - end - it "retrieves a list of products" do api_get :index expect(json_response["products"].first).to have_attributes(keys: all_attributes) @@ -152,12 +147,6 @@ module Spree user end - it "retrieves a list of managed products" do - spree_get :managed, template: 'bulk_index', format: :json - response_keys = json_response.first.keys - expect(attributes.all?{ |attr| response_keys.include? attr }).to eq(true) - end - it "soft deletes my products" do spree_delete :soft_delete, product_id: product.to_param, format: :json expect(response.status).to eq(204) @@ -179,12 +168,6 @@ module Spree .to receive(:has_spree_role?).with("admin").and_return(true) end - it "retrieves a list of managed products" do - spree_get :managed, template: 'bulk_index', format: :json - response_keys = json_response.first.keys - expect(attributes.all?{ |attr| response_keys.include? attr }).to eq(true) - end - it "retrieves a list of products with appropriate attributes" do spree_get :index, template: 'bulk_index', format: :json response_keys = json_response.first.keys From cc51537e93bd44b5edf14cfdfa599f6ee1c5482f Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 09:48:12 +0100 Subject: [PATCH 04/13] Convert spree/api/products_controller from rabl to ams --- .../spree/api/products_controller.rb | 15 +++++++------ .../spree/api/products/bulk_index.v1.rabl | 2 -- .../spree/api/products/bulk_show.v1.rabl | 21 ------------------- 3 files changed, 7 insertions(+), 31 deletions(-) delete mode 100644 app/views/spree/api/products/bulk_index.v1.rabl delete mode 100644 app/views/spree/api/products/bulk_show.v1.rabl diff --git a/app/controllers/spree/api/products_controller.rb b/app/controllers/spree/api/products_controller.rb index 03bf8a7775..bc19befc67 100644 --- a/app/controllers/spree/api/products_controller.rb +++ b/app/controllers/spree/api/products_controller.rb @@ -11,15 +11,14 @@ module Spree else @products = product_scope.ransack(params[:q]).result end - @products = @products.page(params[:page]).per(params[:per_page]) - respond_with(@products) + render json: @products, each_serializer: ::Api::Admin::ProductSerializer end def show @product = find_product(params[:id]) - respond_with(@product) + render json: @product, serializer: ::Api::Admin::ProductSerializer end def new; end @@ -30,7 +29,7 @@ module Spree @product = Product.new(params[:product]) begin if @product.save - respond_with(@product, status: 201, default_template: :show) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 else invalid_resource!(@product) end @@ -44,7 +43,7 @@ module Spree authorize! :update, Product @product = find_product(params[:id]) if @product.update_attributes(params[:product]) - respond_with(@product, status: 200, default_template: :show) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 200 else invalid_resource!(@product) end @@ -55,7 +54,7 @@ module Spree @product = find_product(params[:id]) @product.update_attribute(:deleted_at, Time.zone.now) @product.variants_including_master.update_all(deleted_at: Time.zone.now) - respond_with(@product, status: 204) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 end # TODO: This should be named 'managed'. Is the action above used? Maybe we should remove it. @@ -83,7 +82,7 @@ module Spree @product = find_product(params[:product_id]) authorize! :delete, @product @product.destroy - respond_with(@product, status: 204) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 end # POST /api/products/:product_id/clone @@ -95,7 +94,7 @@ module Spree @product = original_product.duplicate - respond_with(@product, status: 201, default_template: :show) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 end private diff --git a/app/views/spree/api/products/bulk_index.v1.rabl b/app/views/spree/api/products/bulk_index.v1.rabl deleted file mode 100644 index dfb9f20f2a..0000000000 --- a/app/views/spree/api/products/bulk_index.v1.rabl +++ /dev/null @@ -1,2 +0,0 @@ -collection @products.order('id ASC') -extends "spree/api/products/bulk_show" diff --git a/app/views/spree/api/products/bulk_show.v1.rabl b/app/views/spree/api/products/bulk_show.v1.rabl deleted file mode 100644 index 5f6b600287..0000000000 --- a/app/views/spree/api/products/bulk_show.v1.rabl +++ /dev/null @@ -1,21 +0,0 @@ -object @product - -# TODO: This is used by bulk product edit when a product is cloned. -# But the list of products is serialized by Api::Admin::ProductSerializer. -# This should probably be unified. - -attributes :id, :name, :sku, :variant_unit, :variant_unit_scale, :variant_unit_name, :on_demand, :inherits_properties -attributes :on_hand, :price, :available_on, :permalink_live, :tax_category_id - -# Infinity is not a valid JSON object, but Rails encodes it anyway -node( :taxon_ids ) { |p| p.taxons.map(&:id).join(",") } -node( :on_hand ) { |p| p.on_hand.nil? ? 0 : p.on_hand.to_f.finite? ? p.on_hand : t(:on_demand) } -node( :price ) { |p| p.price.nil? ? '0.0' : p.price } - -node( :available_on ) { |p| p.available_on.blank? ? "" : p.available_on.strftime("%F %T") } -node( :permalink_live, &:permalink ) -node( :producer_id, &:supplier_id ) -node( :category_id, &:primary_taxon_id ) -node( :supplier ) do |p| - partial 'api/enterprises/bulk_show', object: p.supplier -end From 4d74d246e891584ef08030522e5e1e807f07acd3 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 10:29:12 +0100 Subject: [PATCH 05/13] Remove spree/api/products_controller index and new actions, not used --- .../spree/api/products_controller.rb | 13 -- .../spree/api/products_controller_spec.rb | 135 ------------------ 2 files changed, 148 deletions(-) diff --git a/app/controllers/spree/api/products_controller.rb b/app/controllers/spree/api/products_controller.rb index bc19befc67..b8d7f81a44 100644 --- a/app/controllers/spree/api/products_controller.rb +++ b/app/controllers/spree/api/products_controller.rb @@ -5,24 +5,11 @@ module Spree class ProductsController < Spree::Api::BaseController respond_to :json - def index - if params[:ids] - @products = product_scope.where(id: params[:ids]) - else - @products = product_scope.ransack(params[:q]).result - end - @products = @products.page(params[:page]).per(params[:per_page]) - - render json: @products, each_serializer: ::Api::Admin::ProductSerializer - end - def show @product = find_product(params[:id]) render json: @product, serializer: ::Api::Admin::ProductSerializer end - def new; end - def create authorize! :create, Product params[:product][:available_on] ||= Time.zone.now diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index 47907526b5..f372d91e75 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -25,68 +25,6 @@ module Spree .to receive(:has_spree_role?).with("admin").and_return(false) end - it "retrieves a list of products" do - api_get :index - expect(json_response["products"].first).to have_attributes(keys: all_attributes) - - expect(json_response["count"]).to eq(1) - expect(json_response["current_page"]).to eq(1) - expect(json_response["pages"]).to eq(1) - end - - it "retrieves a list of products by id" do - api_get :index, ids: [product.id] - expect(json_response["products"].first).to have_attributes(keys: all_attributes) - expect(json_response["count"]).to eq(1) - expect(json_response["current_page"]).to eq(1) - expect(json_response["pages"]).to eq(1) - end - - it "does not return inactive products when queried by ids" do - api_get :index, ids: [inactive_product.id] - expect(json_response["count"]).to eq(0) - end - - it "does not list unavailable products" do - api_get :index - expect(json_response["products"].first["name"]).not_to eq("inactive") - end - - context "pagination" do - it "can select the next page of products" do - second_product = create(:product) - api_get :index, page: 2, per_page: 1 - expect(json_response["products"].first).to have_attributes(keys: all_attributes) - expect(json_response["total_count"]).to eq(2) - expect(json_response["current_page"]).to eq(2) - expect(json_response["pages"]).to eq(2) - end - - it 'can control the page size through a parameter' do - create(:product) - api_get :index, per_page: 1 - expect(json_response['count']).to eq(1) - expect(json_response['total_count']).to eq(2) - expect(json_response['current_page']).to eq(1) - expect(json_response['pages']).to eq(2) - end - end - - context "jsonp" do - it "retrieves a list of products of jsonp" do - api_get :index, callback: 'callback' - expect(response.body).to match(/^callback\(.*\)$/) - expect(response.header['Content-Type']).to include('application/javascript') - end - end - - it "can search for products" do - create(:product, name: "The best product in the world") - api_get :index, q: { name_cont: "best" } - expect(json_response["products"].first).to have_attributes(keys: all_attributes) - expect(json_response["count"]).to eq(1) - end - it "gets a single product" do product.master.images.create!(attachment: image("thinking-cat.jpg")) product.variants.create!(unit_value: "1", unit_description: "thing") @@ -128,15 +66,6 @@ module Spree expect(response.status).to eq(404) end - it "can learn how to create a new product" do - api_get :new - expect(json_response["attributes"]).to eq(["id", "name", "description", "price", "available_on", "permalink", "meta_description", "meta_keywords", "shipping_category_id", "taxon_ids"]) - required_attributes = json_response["required_attributes"] - expect(required_attributes).to include("name") - expect(required_attributes).to include("price") - expect(required_attributes).to include("shipping_category_id") - end - include_examples "modifying product actions are restricted" end @@ -168,45 +97,6 @@ module Spree .to receive(:has_spree_role?).with("admin").and_return(true) end - it "retrieves a list of products with appropriate attributes" do - spree_get :index, template: 'bulk_index', format: :json - response_keys = json_response.first.keys - expect(attributes.all?{ |attr| response_keys.include? attr }).to eq(true) - end - - it "sorts products in ascending id order" do - FactoryBot.create(:product, supplier: supplier) - FactoryBot.create(:product, supplier: supplier) - - spree_get :index, template: 'bulk_index', format: :json - - ids = json_response.map{ |product| product['id'] } - expect(ids[0]).to be < ids[1] - expect(ids[1]).to be < ids[2] - end - - it "formats available_on to 'yyyy-mm-dd hh:mm'" do - spree_get :index, template: 'bulk_index', format: :json - expect(json_response.map{ |product| product['available_on'] }.all?{ |a| a.match("^\\d{4}-\\d{2}-\\d{2} \\d{2}:\\d{2}:\\d{2}$") }).to eq(true) - end - - it "returns permalink as permalink_live" do - spree_get :index, template: 'bulk_index', format: :json - expect(json_response.detect{ |product_in_response| product_in_response['id'] == product.id }['permalink_live']).to eq(product.permalink) - end - - it "should allow available_on to be nil" do - spree_get :index, template: 'bulk_index', format: :json - expect(json_response.size).to eq(2) - - another_product = FactoryBot.create(:product) - another_product.available_on = nil - another_product.save! - - spree_get :index, template: 'bulk_index', format: :json - expect(json_response.size).to eq(3) - end - it "soft deletes a product" do spree_delete :soft_delete, product_id: product.to_param, format: :json expect(response.status).to eq(204) @@ -214,31 +104,6 @@ module Spree expect(product.deleted_at).not_to be_nil end - it "can see all products" do - api_get :index - expect(json_response["products"].count).to eq(2) - expect(json_response["count"]).to eq(2) - expect(json_response["current_page"]).to eq(1) - expect(json_response["pages"]).to eq(1) - end - - # Regression test for #1626 - context "deleted products" do - before do - create(:product, deleted_at: 1.day.ago) - end - - it "does not include deleted products" do - api_get :index - expect(json_response["products"].count).to eq(2) - end - - it "can include deleted products" do - api_get :index, show_deleted: 1 - expect(json_response["products"].count).to eq(3) - end - end - it "can create a new product" do api_post :create, product: { name: "The Other Product", price: 19.99, From 4497173213cc53f7b3c2ba2c4d8bb0cef44d2f4b Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 11:23:11 +0100 Subject: [PATCH 06/13] Adapt spree/api/products_controller_spec to AMS serializer --- .../spree/api/products_controller_spec.rb | 34 ++++++++++++++----- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index f372d91e75..70b8b805bd 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -11,7 +11,8 @@ module Spree let(:product_other_supplier) { create(:product, supplier: supplier2) } let(:product_with_image) { create(:product_with_image, supplier: supplier) } let(:attributes) { ["id", "name", "supplier", "price", "on_hand", "available_on", "permalink_live"] } - let(:all_attributes) { ["id", "name", "description", "price", "available_on", "permalink", "meta_description", "meta_keywords", "shipping_category_id", "taxon_ids", "variants", "option_types", "product_properties"] } + let(:all_attributes) { ["id", "name", "price", "available_on", "variants"] } + let(:variants_attributes) { ["id", "options_text", "unit_value", "unit_description", "unit_to_display", "on_demand", "display_as", "display_name", "name_to_display", "sku", "on_hand", "price"] } let(:current_api_user) { build(:user) } @@ -31,10 +32,9 @@ module Spree product.variants.first.images.create!(attachment: image("thinking-cat.jpg")) product.set_property("spree", "rocks") api_get :show, id: product.to_param - expect(json_response).to have_attributes(keys: all_attributes) - expect(json_response['variants'].first).to have_attributes(keys: ["id", "name", "sku", "price", "weight", "height", "width", "depth", "is_master", "cost_price", "permalink", "option_values", "images"]) - expect(json_response['variants'].first['images'].first).to have_attributes(keys: ["id", "position", "attachment_content_type", "attachment_file_name", "type", "attachment_updated_at", "attachment_width", "attachment_height", "alt", "viewable_type", "viewable_id", "attachment_url"]) - expect(json_response["product_properties"].first).to have_attributes(keys: ["id", "product_id", "property_id", "value", "property_name"]) + + expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) + expect(variants_attributes.all?{ |attr| json_response['variants'].first.keys.include? attr }).to eq(true) end context "finds a product by permalink first then by id" do @@ -46,22 +46,25 @@ module Spree specify do api_get :show, id: product.to_param - expect(json_response["permalink"]).to match(/and-1-ways/) + + expect(json_response["permalink_live"]).to match(/and-1-ways/) product.destroy api_get :show, id: other_product.id - expect(json_response["permalink"]).to match(/droids/) + expect(json_response["permalink_live"]).to match(/droids/) end end it "cannot see inactive products" do api_get :show, id: inactive_product.to_param + expect(json_response["error"]).to eq("The resource you were looking for could not be found.") expect(response.status).to eq(404) end it "returns a 404 error when it cannot find a product" do api_get :show, id: "non-existant" + expect(json_response["error"]).to eq("The resource you were looking for could not be found.") expect(response.status).to eq(404) end @@ -78,6 +81,7 @@ module Spree it "soft deletes my products" do spree_delete :soft_delete, product_id: product.to_param, format: :json + expect(response.status).to eq(204) expect { product.reload }.not_to raise_error expect(product.deleted_at).not_to be_nil @@ -85,6 +89,7 @@ module Spree it "is denied access to soft deleting another enterprises' product" do spree_delete :soft_delete, product_id: product_other_supplier.to_param, format: :json + assert_unauthorized! expect { product_other_supplier.reload }.not_to raise_error expect(product_other_supplier.deleted_at).to be_nil @@ -99,6 +104,7 @@ module Spree it "soft deletes a product" do spree_delete :soft_delete, product_id: product.to_param, format: :json + expect(response.status).to eq(204) expect { product.reload }.not_to raise_error expect(product.deleted_at).not_to be_nil @@ -113,12 +119,14 @@ module Spree variant_unit: "items", variant_unit_name: "things", unit_description: "things" } - expect(json_response).to have_attributes(keys: all_attributes) + + expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) expect(response.status).to eq(201) end it "cannot create a new product with invalid attributes" do api_post :create, product: {} + expect(response.status).to eq(422) expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") errors = json_response["errors"] @@ -127,11 +135,13 @@ module Spree it "can update a product" do api_put :update, id: product.to_param, product: { name: "New and Improved Product!" } + expect(response.status).to eq(200) end it "cannot update a product with an invalid attribute" do api_put :update, id: product.to_param, product: { name: "" } + expect(response.status).to eq(422) expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") expect(json_response["errors"]["name"]).to eq(["can't be blank"]) @@ -140,6 +150,7 @@ module Spree it "can delete a product" do expect(product.deleted_at).to be_nil api_delete :destroy, id: product.to_param + expect(response.status).to eq(204) expect(product.reload.deleted_at).not_to be_nil end @@ -154,6 +165,7 @@ module Spree it 'denies access' do spree_post :clone, product_id: product.id, format: :json + assert_unauthorized! end end @@ -167,16 +179,19 @@ module Spree it 'responds with a successful response' do spree_post :clone, product_id: product.id, format: :json + expect(response.status).to eq(201) end it 'clones the product' do spree_post :clone, product_id: product.id, format: :json + expect(json_response['name']).to eq("COPY OF #{product.name}") end it 'clones a product with image' do spree_post :clone, product_id: product_with_image.id, format: :json + expect(response.status).to eq(201) expect(json_response['name']).to eq("COPY OF #{product_with_image.name}") end @@ -190,16 +205,19 @@ module Spree it 'responds with a successful response' do spree_post :clone, product_id: product.id, format: :json + expect(response.status).to eq(201) end it 'clones the product' do spree_post :clone, product_id: product.id, format: :json + expect(json_response['name']).to eq("COPY OF #{product.name}") end it 'clones a product with image' do spree_post :clone, product_id: product_with_image.id, format: :json + expect(response.status).to eq(201) expect(json_response['name']).to eq("COPY OF #{product_with_image.name}") end From 78ab8521412cee67ca115af1d8e881d5641eba63 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 11:23:43 +0100 Subject: [PATCH 07/13] Make spree/api/products_controller work with AMS --- app/controllers/spree/api/products_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/spree/api/products_controller.rb b/app/controllers/spree/api/products_controller.rb index b8d7f81a44..dacd6b6948 100644 --- a/app/controllers/spree/api/products_controller.rb +++ b/app/controllers/spree/api/products_controller.rb @@ -2,9 +2,11 @@ require 'open_food_network/permissions' module Spree module Api - class ProductsController < Spree::Api::BaseController + class ProductsController < ::Api::BaseController respond_to :json + skip_authorization_check only: [:show] + def show @product = find_product(params[:id]) render json: @product, serializer: ::Api::Admin::ProductSerializer From 18974c68e19f1a1275c65823ce1a3895ae2c4401 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 11:24:55 +0100 Subject: [PATCH 08/13] Remove orphan price check from price model This is a quick fix. This check is breaking product deletion in some situations and orphan Prices are not really a problem in the DB --- app/models/spree/price_decorator.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/models/spree/price_decorator.rb b/app/models/spree/price_decorator.rb index 79e809b8ba..00219eeedd 100644 --- a/app/models/spree/price_decorator.rb +++ b/app/models/spree/price_decorator.rb @@ -4,6 +4,12 @@ module Spree private + def check_price + if currency.nil? + self.currency = Spree::Config[:currency] + end + end + def refresh_products_cache variant.andand.refresh_products_cache end From 6c054e60784fd4c538950abebd5ddb83e7b10077 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 12:18:27 +0100 Subject: [PATCH 09/13] Add bulk_products and overridable to skip_authorization_check so these endpoints work with AMS --- app/controllers/spree/api/products_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/spree/api/products_controller.rb b/app/controllers/spree/api/products_controller.rb index dacd6b6948..8a8d4475f5 100644 --- a/app/controllers/spree/api/products_controller.rb +++ b/app/controllers/spree/api/products_controller.rb @@ -5,7 +5,7 @@ module Spree class ProductsController < ::Api::BaseController respond_to :json - skip_authorization_check only: [:show] + skip_authorization_check only: [:show, :bulk_products, :overridable] def show @product = find_product(params[:id]) From b7f7038934cc8d68d8b7f651f42f787e1f2df178 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Wed, 31 Jul 2019 14:36:36 +0100 Subject: [PATCH 10/13] Remove api/enterprises rabl template, it was only used as a member in the now removed rabl variants/products templates --- app/views/api/enterprises/bulk_show.v1.rabl | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 app/views/api/enterprises/bulk_show.v1.rabl diff --git a/app/views/api/enterprises/bulk_show.v1.rabl b/app/views/api/enterprises/bulk_show.v1.rabl deleted file mode 100644 index 4cc671a3f1..0000000000 --- a/app/views/api/enterprises/bulk_show.v1.rabl +++ /dev/null @@ -1,3 +0,0 @@ -object @enterprise - -attributes :id, :name From 31bac9641fc5ee0a96bff8d73d1cd173d737d9e9 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 1 Aug 2019 14:28:55 +0100 Subject: [PATCH 11/13] Move api products and variants routes and ctrl out of spree namespace --- .../{spree => }/api/products_controller.rb | 0 .../{spree => }/api/variants_controller.rb | 0 config/routes/api.rb | 15 +++++++++++++++ config/routes/spree.rb | 15 --------------- .../{spree => }/api/products_controller_spec.rb | 0 .../{spree => }/api/variants_controller_spec.rb | 0 6 files changed, 15 insertions(+), 15 deletions(-) rename app/controllers/{spree => }/api/products_controller.rb (100%) rename app/controllers/{spree => }/api/variants_controller.rb (100%) rename spec/controllers/{spree => }/api/products_controller_spec.rb (100%) rename spec/controllers/{spree => }/api/variants_controller_spec.rb (100%) diff --git a/app/controllers/spree/api/products_controller.rb b/app/controllers/api/products_controller.rb similarity index 100% rename from app/controllers/spree/api/products_controller.rb rename to app/controllers/api/products_controller.rb diff --git a/app/controllers/spree/api/variants_controller.rb b/app/controllers/api/variants_controller.rb similarity index 100% rename from app/controllers/spree/api/variants_controller.rb rename to app/controllers/api/variants_controller.rb diff --git a/config/routes/api.rb b/config/routes/api.rb index 99c7f76eda..bb8c98ba3b 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -1,5 +1,20 @@ Openfoodnetwork::Application.routes.draw do namespace :api do + resources :products do + collection do + get :bulk_products + get :overridable + end + delete :soft_delete + post :clone + + resources :variants do + delete :soft_delete + end + end + + resources :variants, :only => [:index] + resources :enterprises do post :update_image, on: :member get :managed, on: :collection diff --git a/config/routes/spree.rb b/config/routes/spree.rb index 2007665100..5eb389c93e 100644 --- a/config/routes/spree.rb +++ b/config/routes/spree.rb @@ -62,21 +62,6 @@ Spree::Core::Engine.routes.prepend do get :authorise_api, on: :collection end - resources :products do - collection do - get :bulk_products - get :overridable - end - delete :soft_delete - post :clone - - resources :variants do - delete :soft_delete - end - end - - resources :variants, :only => [:index] - resources :orders do get :managed, on: :collection diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/api/products_controller_spec.rb similarity index 100% rename from spec/controllers/spree/api/products_controller_spec.rb rename to spec/controllers/api/products_controller_spec.rb diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/api/variants_controller_spec.rb similarity index 100% rename from spec/controllers/spree/api/variants_controller_spec.rb rename to spec/controllers/api/variants_controller_spec.rb From aa3c1aa0fe4d9fdd4a89acc17e00b73dd6735da6 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 1 Aug 2019 14:30:11 +0100 Subject: [PATCH 12/13] Remove Spree module declaration from these files as they were moved out of the spree namespace --- app/controllers/api/products_controller.rb | 190 +++++----- app/controllers/api/variants_controller.rb | 130 ++++--- .../api/products_controller_spec.rb | 358 +++++++++--------- .../api/variants_controller_spec.rb | 330 ++++++++-------- 4 files changed, 500 insertions(+), 508 deletions(-) diff --git a/app/controllers/api/products_controller.rb b/app/controllers/api/products_controller.rb index 8a8d4475f5..6efa38806c 100644 --- a/app/controllers/api/products_controller.rb +++ b/app/controllers/api/products_controller.rb @@ -1,126 +1,124 @@ require 'open_food_network/permissions' -module Spree - module Api - class ProductsController < ::Api::BaseController - respond_to :json +module Api + class ProductsController < ::Api::BaseController + respond_to :json - skip_authorization_check only: [:show, :bulk_products, :overridable] + skip_authorization_check only: [:show, :bulk_products, :overridable] - def show - @product = find_product(params[:id]) - render json: @product, serializer: ::Api::Admin::ProductSerializer - end + def show + @product = find_product(params[:id]) + render json: @product, serializer: ::Api::Admin::ProductSerializer + end - def create - authorize! :create, Product - params[:product][:available_on] ||= Time.zone.now - @product = Product.new(params[:product]) - begin - if @product.save - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 - else - invalid_resource!(@product) - end - rescue ActiveRecord::RecordNotUnique - @product.permalink = nil - retry - end - end - - def update - authorize! :update, Product - @product = find_product(params[:id]) - if @product.update_attributes(params[:product]) - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 200 + def create + authorize! :create, Product + params[:product][:available_on] ||= Time.zone.now + @product = Product.new(params[:product]) + begin + if @product.save + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 else invalid_resource!(@product) end + rescue ActiveRecord::RecordNotUnique + @product.permalink = nil + retry end + end - def destroy - authorize! :delete, Product - @product = find_product(params[:id]) - @product.update_attribute(:deleted_at, Time.zone.now) - @product.variants_including_master.update_all(deleted_at: Time.zone.now) - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 + def update + authorize! :update, Product + @product = find_product(params[:id]) + if @product.update_attributes(params[:product]) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 200 + else + invalid_resource!(@product) end + end - # TODO: This should be named 'managed'. Is the action above used? Maybe we should remove it. - def bulk_products - @products = OpenFoodNetwork::Permissions.new(current_api_user).editable_products. - merge(product_scope). - order('created_at DESC'). - ransack(params[:q]).result. - page(params[:page]).per(params[:per_page]) + def destroy + authorize! :delete, Product + @product = find_product(params[:id]) + @product.update_attribute(:deleted_at, Time.zone.now) + @product.variants_including_master.update_all(deleted_at: Time.zone.now) + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 + end - render_paged_products @products - end + # TODO: This should be named 'managed'. Is the action above used? Maybe we should remove it. + def bulk_products + @products = OpenFoodNetwork::Permissions.new(current_api_user).editable_products. + merge(product_scope). + order('created_at DESC'). + ransack(params[:q]).result. + page(params[:page]).per(params[:per_page]) - def overridable - producers = OpenFoodNetwork::Permissions.new(current_api_user). - variant_override_producers.by_name + render_paged_products @products + end - @products = paged_products_for_producers producers + def overridable + producers = OpenFoodNetwork::Permissions.new(current_api_user). + variant_override_producers.by_name - render_paged_products @products - end + @products = paged_products_for_producers producers - def soft_delete - authorize! :delete, Spree::Product - @product = find_product(params[:product_id]) - authorize! :delete, @product - @product.destroy - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 - end + render_paged_products @products + end - # POST /api/products/:product_id/clone - # - def clone - authorize! :create, Spree::Product - original_product = find_product(params[:product_id]) - authorize! :update, original_product + def soft_delete + authorize! :delete, Spree::Product + @product = find_product(params[:product_id]) + authorize! :delete, @product + @product.destroy + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 + end - @product = original_product.duplicate + # POST /api/products/:product_id/clone + # + def clone + authorize! :create, Spree::Product + original_product = find_product(params[:product_id]) + authorize! :update, original_product - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 - end + @product = original_product.duplicate - private + render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 + end - # Copied and modified from Spree::Api::BaseController to allow - # enterprise users to access inactive products - def product_scope - # This line modified - if current_api_user.has_spree_role?("admin") || current_api_user.enterprises.present? - scope = Spree::Product - if params[:show_deleted] - scope = scope.with_deleted - end - else - scope = Spree::Product.active + private + + # Copied and modified from Spree::Api::BaseController to allow + # enterprise users to access inactive products + def product_scope + # This line modified + if current_api_user.has_spree_role?("admin") || current_api_user.enterprises.present? + scope = Spree::Product + if params[:show_deleted] + scope = scope.with_deleted end - - scope.includes(:master) + else + scope = Spree::Product.active end - def paged_products_for_producers(producers) - Spree::Product.scoped. - merge(product_scope). - where(supplier_id: producers). - by_producer.by_name. - ransack(params[:q]).result. - page(params[:page]).per(params[:per_page]) - end + scope.includes(:master) + end - def render_paged_products(products) - serializer = ActiveModel::ArraySerializer.new( - products, - each_serializer: ::Api::Admin::ProductSerializer - ) + def paged_products_for_producers(producers) + Spree::Product.scoped. + merge(product_scope). + where(supplier_id: producers). + by_producer.by_name. + ransack(params[:q]).result. + page(params[:page]).per(params[:per_page]) + end - render text: { products: serializer, pages: products.num_pages }.to_json - end + def render_paged_products(products) + serializer = ActiveModel::ArraySerializer.new( + products, + each_serializer: ::Api::Admin::ProductSerializer + ) + + render text: { products: serializer, pages: products.num_pages }.to_json end end end diff --git a/app/controllers/api/variants_controller.rb b/app/controllers/api/variants_controller.rb index 2f1e6ab342..8c5ddafc39 100644 --- a/app/controllers/api/variants_controller.rb +++ b/app/controllers/api/variants_controller.rb @@ -1,81 +1,79 @@ -module Spree - module Api - class VariantsController < ::Api::BaseController - respond_to :json +module Api + class VariantsController < ::Api::BaseController + respond_to :json - skip_authorization_check only: [:index, :show] - before_filter :product + skip_authorization_check only: [:index, :show] + before_filter :product - def index - @variants = scope.includes(:option_values).ransack(params[:q]).result - render json: @variants, each_serializer: ::Api::VariantSerializer + def index + @variants = scope.includes(:option_values).ransack(params[:q]).result + render json: @variants, each_serializer: ::Api::VariantSerializer + end + + def show + @variant = scope.includes(:option_values).find(params[:id]) + render json: @variant, serializer: ::Api::VariantSerializer + end + + def create + authorize! :create, Variant + @variant = scope.new(params[:variant]) + if @variant.save + render json: @variant, serializer: ::Api::VariantSerializer, status: 201 + else + invalid_resource!(@variant) end + end - def show - @variant = scope.includes(:option_values).find(params[:id]) - render json: @variant, serializer: ::Api::VariantSerializer + def update + authorize! :update, Variant + @variant = scope.find(params[:id]) + if @variant.update_attributes(params[:variant]) + render json: @variant, serializer: ::Api::VariantSerializer, status: 200 + else + invalid_resource!(@product) end + end - def create - authorize! :create, Variant - @variant = scope.new(params[:variant]) - if @variant.save - render json: @variant, serializer: ::Api::VariantSerializer, status: 201 + def soft_delete + @variant = scope.find(params[:variant_id]) + authorize! :delete, @variant + + VariantDeleter.new.delete(@variant) + render json: @variant, serializer: ::Api::VariantSerializer, status: 204 + end + + def destroy + authorize! :delete, Variant + @variant = scope.find(params[:id]) + @variant.destroy + render json: @variant, serializer: ::Api::VariantSerializer, status: 204 + end + + private + + def product + @product ||= Spree::Product.find_by_permalink(params[:product_id]) if params[:product_id] + end + + def scope + if @product + unless current_api_user.has_spree_role?("admin") || params[:show_deleted] + variants = @product.variants_including_master else - invalid_resource!(@variant) + variants = @product.variants_including_master.with_deleted end - end - - def update - authorize! :update, Variant - @variant = scope.find(params[:id]) - if @variant.update_attributes(params[:variant]) - render json: @variant, serializer: ::Api::VariantSerializer, status: 200 - else - invalid_resource!(@product) - end - end - - def soft_delete - @variant = scope.find(params[:variant_id]) - authorize! :delete, @variant - - VariantDeleter.new.delete(@variant) - render json: @variant, serializer: ::Api::VariantSerializer, status: 204 - end - - def destroy - authorize! :delete, Variant - @variant = scope.find(params[:id]) - @variant.destroy - render json: @variant, serializer: ::Api::VariantSerializer, status: 204 - end - - private - - def product - @product ||= Spree::Product.find_by_permalink(params[:product_id]) if params[:product_id] - end - - def scope - if @product - unless current_api_user.has_spree_role?("admin") || params[:show_deleted] - variants = @product.variants_including_master - else - variants = @product.variants_including_master.with_deleted + else + variants = Variant.scoped + if current_api_user.has_spree_role?("admin") + unless params[:show_deleted] + variants = Variant.active end else - variants = Variant.scoped - if current_api_user.has_spree_role?("admin") - unless params[:show_deleted] - variants = Variant.active - end - else - variants = variants.active - end + variants = variants.active end - variants end + variants end end end diff --git a/spec/controllers/api/products_controller_spec.rb b/spec/controllers/api/products_controller_spec.rb index 70b8b805bd..32efb09ddc 100644 --- a/spec/controllers/api/products_controller_spec.rb +++ b/spec/controllers/api/products_controller_spec.rb @@ -1,226 +1,224 @@ require 'spec_helper' -module Spree - describe Spree::Api::ProductsController, type: :controller do - render_views +describe Api::ProductsController, type: :controller do + render_views - let(:supplier) { create(:supplier_enterprise) } - let(:supplier2) { create(:supplier_enterprise) } - let!(:product) { create(:product, supplier: supplier) } - let!(:inactive_product) { create(:product, available_on: Time.zone.now.tomorrow, name: "inactive") } - let(:product_other_supplier) { create(:product, supplier: supplier2) } - let(:product_with_image) { create(:product_with_image, supplier: supplier) } - let(:attributes) { ["id", "name", "supplier", "price", "on_hand", "available_on", "permalink_live"] } - let(:all_attributes) { ["id", "name", "price", "available_on", "variants"] } - let(:variants_attributes) { ["id", "options_text", "unit_value", "unit_description", "unit_to_display", "on_demand", "display_as", "display_name", "name_to_display", "sku", "on_hand", "price"] } + let(:supplier) { create(:supplier_enterprise) } + let(:supplier2) { create(:supplier_enterprise) } + let!(:product) { create(:product, supplier: supplier) } + let!(:inactive_product) { create(:product, available_on: Time.zone.now.tomorrow, name: "inactive") } + let(:product_other_supplier) { create(:product, supplier: supplier2) } + let(:product_with_image) { create(:product_with_image, supplier: supplier) } + let(:attributes) { ["id", "name", "supplier", "price", "on_hand", "available_on", "permalink_live"] } + let(:all_attributes) { ["id", "name", "price", "available_on", "variants"] } + let(:variants_attributes) { ["id", "options_text", "unit_value", "unit_description", "unit_to_display", "on_demand", "display_as", "display_name", "name_to_display", "sku", "on_hand", "price"] } - let(:current_api_user) { build(:user) } + let(:current_api_user) { build(:user) } + before do + allow(controller).to receive(:spree_current_user) { current_api_user } + end + + context "as a normal user" do before do - allow(controller).to receive(:spree_current_user) { current_api_user } + allow(current_api_user) + .to receive(:has_spree_role?).with("admin").and_return(false) end - context "as a normal user" do + it "gets a single product" do + product.master.images.create!(attachment: image("thinking-cat.jpg")) + product.variants.create!(unit_value: "1", unit_description: "thing") + product.variants.first.images.create!(attachment: image("thinking-cat.jpg")) + product.set_property("spree", "rocks") + api_get :show, id: product.to_param + + expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) + expect(variants_attributes.all?{ |attr| json_response['variants'].first.keys.include? attr }).to eq(true) + end + + context "finds a product by permalink first then by id" do + let!(:other_product) { create(:product, permalink: "these-are-not-the-droids-you-are-looking-for") } + + before do + product.update_attribute(:permalink, "#{other_product.id}-and-1-ways") + end + + specify do + api_get :show, id: product.to_param + + expect(json_response["permalink_live"]).to match(/and-1-ways/) + product.destroy + + api_get :show, id: other_product.id + expect(json_response["permalink_live"]).to match(/droids/) + end + end + + it "cannot see inactive products" do + api_get :show, id: inactive_product.to_param + + expect(json_response["error"]).to eq("The resource you were looking for could not be found.") + expect(response.status).to eq(404) + end + + it "returns a 404 error when it cannot find a product" do + api_get :show, id: "non-existant" + + expect(json_response["error"]).to eq("The resource you were looking for could not be found.") + expect(response.status).to eq(404) + end + + include_examples "modifying product actions are restricted" + end + + context "as an enterprise user" do + let(:current_api_user) do + user = create(:user) + user.enterprise_roles.create(enterprise: supplier) + user + end + + it "soft deletes my products" do + spree_delete :soft_delete, product_id: product.to_param, format: :json + + expect(response.status).to eq(204) + expect { product.reload }.not_to raise_error + expect(product.deleted_at).not_to be_nil + end + + it "is denied access to soft deleting another enterprises' product" do + spree_delete :soft_delete, product_id: product_other_supplier.to_param, format: :json + + assert_unauthorized! + expect { product_other_supplier.reload }.not_to raise_error + expect(product_other_supplier.deleted_at).to be_nil + end + end + + context "as an administrator" do + before do + allow(current_api_user) + .to receive(:has_spree_role?).with("admin").and_return(true) + end + + it "soft deletes a product" do + spree_delete :soft_delete, product_id: product.to_param, format: :json + + expect(response.status).to eq(204) + expect { product.reload }.not_to raise_error + expect(product.deleted_at).not_to be_nil + end + + it "can create a new product" do + api_post :create, product: { name: "The Other Product", + price: 19.99, + shipping_category_id: create(:shipping_category).id, + supplier_id: supplier.id, + primary_taxon_id: FactoryBot.create(:taxon).id, + variant_unit: "items", + variant_unit_name: "things", + unit_description: "things" } + + expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) + expect(response.status).to eq(201) + end + + it "cannot create a new product with invalid attributes" do + api_post :create, product: {} + + expect(response.status).to eq(422) + expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") + errors = json_response["errors"] + expect(errors.keys).to match_array(["name", "price", "primary_taxon", "shipping_category_id", "supplier", "variant_unit"]) + end + + it "can update a product" do + api_put :update, id: product.to_param, product: { name: "New and Improved Product!" } + + expect(response.status).to eq(200) + end + + it "cannot update a product with an invalid attribute" do + api_put :update, id: product.to_param, product: { name: "" } + + expect(response.status).to eq(422) + expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") + expect(json_response["errors"]["name"]).to eq(["can't be blank"]) + end + + it "can delete a product" do + expect(product.deleted_at).to be_nil + api_delete :destroy, id: product.to_param + + expect(response.status).to eq(204) + expect(product.reload.deleted_at).not_to be_nil + end + end + + describe '#clone' do + context 'as a normal user' do before do allow(current_api_user) .to receive(:has_spree_role?).with("admin").and_return(false) end - it "gets a single product" do - product.master.images.create!(attachment: image("thinking-cat.jpg")) - product.variants.create!(unit_value: "1", unit_description: "thing") - product.variants.first.images.create!(attachment: image("thinking-cat.jpg")) - product.set_property("spree", "rocks") - api_get :show, id: product.to_param + it 'denies access' do + spree_post :clone, product_id: product.id, format: :json - expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) - expect(variants_attributes.all?{ |attr| json_response['variants'].first.keys.include? attr }).to eq(true) + assert_unauthorized! end - - context "finds a product by permalink first then by id" do - let!(:other_product) { create(:product, permalink: "these-are-not-the-droids-you-are-looking-for") } - - before do - product.update_attribute(:permalink, "#{other_product.id}-and-1-ways") - end - - specify do - api_get :show, id: product.to_param - - expect(json_response["permalink_live"]).to match(/and-1-ways/) - product.destroy - - api_get :show, id: other_product.id - expect(json_response["permalink_live"]).to match(/droids/) - end - end - - it "cannot see inactive products" do - api_get :show, id: inactive_product.to_param - - expect(json_response["error"]).to eq("The resource you were looking for could not be found.") - expect(response.status).to eq(404) - end - - it "returns a 404 error when it cannot find a product" do - api_get :show, id: "non-existant" - - expect(json_response["error"]).to eq("The resource you were looking for could not be found.") - expect(response.status).to eq(404) - end - - include_examples "modifying product actions are restricted" end - context "as an enterprise user" do + context 'as an enterprise user' do let(:current_api_user) do user = create(:user) user.enterprise_roles.create(enterprise: supplier) user end - it "soft deletes my products" do - spree_delete :soft_delete, product_id: product.to_param, format: :json + it 'responds with a successful response' do + spree_post :clone, product_id: product.id, format: :json - expect(response.status).to eq(204) - expect { product.reload }.not_to raise_error - expect(product.deleted_at).not_to be_nil + expect(response.status).to eq(201) end - it "is denied access to soft deleting another enterprises' product" do - spree_delete :soft_delete, product_id: product_other_supplier.to_param, format: :json + it 'clones the product' do + spree_post :clone, product_id: product.id, format: :json - assert_unauthorized! - expect { product_other_supplier.reload }.not_to raise_error - expect(product_other_supplier.deleted_at).to be_nil + expect(json_response['name']).to eq("COPY OF #{product.name}") + end + + it 'clones a product with image' do + spree_post :clone, product_id: product_with_image.id, format: :json + + expect(response.status).to eq(201) + expect(json_response['name']).to eq("COPY OF #{product_with_image.name}") end end - context "as an administrator" do + context 'as an administrator' do before do allow(current_api_user) .to receive(:has_spree_role?).with("admin").and_return(true) end - it "soft deletes a product" do - spree_delete :soft_delete, product_id: product.to_param, format: :json + it 'responds with a successful response' do + spree_post :clone, product_id: product.id, format: :json - expect(response.status).to eq(204) - expect { product.reload }.not_to raise_error - expect(product.deleted_at).not_to be_nil - end - - it "can create a new product" do - api_post :create, product: { name: "The Other Product", - price: 19.99, - shipping_category_id: create(:shipping_category).id, - supplier_id: supplier.id, - primary_taxon_id: FactoryBot.create(:taxon).id, - variant_unit: "items", - variant_unit_name: "things", - unit_description: "things" } - - expect(all_attributes.all?{ |attr| json_response.keys.include? attr }).to eq(true) expect(response.status).to eq(201) end - it "cannot create a new product with invalid attributes" do - api_post :create, product: {} + it 'clones the product' do + spree_post :clone, product_id: product.id, format: :json - expect(response.status).to eq(422) - expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") - errors = json_response["errors"] - expect(errors.keys).to match_array(["name", "price", "primary_taxon", "shipping_category_id", "supplier", "variant_unit"]) + expect(json_response['name']).to eq("COPY OF #{product.name}") end - it "can update a product" do - api_put :update, id: product.to_param, product: { name: "New and Improved Product!" } + it 'clones a product with image' do + spree_post :clone, product_id: product_with_image.id, format: :json - expect(response.status).to eq(200) - end - - it "cannot update a product with an invalid attribute" do - api_put :update, id: product.to_param, product: { name: "" } - - expect(response.status).to eq(422) - expect(json_response["error"]).to eq("Invalid resource. Please fix errors and try again.") - expect(json_response["errors"]["name"]).to eq(["can't be blank"]) - end - - it "can delete a product" do - expect(product.deleted_at).to be_nil - api_delete :destroy, id: product.to_param - - expect(response.status).to eq(204) - expect(product.reload.deleted_at).not_to be_nil - end - end - - describe '#clone' do - context 'as a normal user' do - before do - allow(current_api_user) - .to receive(:has_spree_role?).with("admin").and_return(false) - end - - it 'denies access' do - spree_post :clone, product_id: product.id, format: :json - - assert_unauthorized! - end - end - - context 'as an enterprise user' do - let(:current_api_user) do - user = create(:user) - user.enterprise_roles.create(enterprise: supplier) - user - end - - it 'responds with a successful response' do - spree_post :clone, product_id: product.id, format: :json - - expect(response.status).to eq(201) - end - - it 'clones the product' do - spree_post :clone, product_id: product.id, format: :json - - expect(json_response['name']).to eq("COPY OF #{product.name}") - end - - it 'clones a product with image' do - spree_post :clone, product_id: product_with_image.id, format: :json - - expect(response.status).to eq(201) - expect(json_response['name']).to eq("COPY OF #{product_with_image.name}") - end - end - - context 'as an administrator' do - before do - allow(current_api_user) - .to receive(:has_spree_role?).with("admin").and_return(true) - end - - it 'responds with a successful response' do - spree_post :clone, product_id: product.id, format: :json - - expect(response.status).to eq(201) - end - - it 'clones the product' do - spree_post :clone, product_id: product.id, format: :json - - expect(json_response['name']).to eq("COPY OF #{product.name}") - end - - it 'clones a product with image' do - spree_post :clone, product_id: product_with_image.id, format: :json - - expect(response.status).to eq(201) - expect(json_response['name']).to eq("COPY OF #{product_with_image.name}") - end + expect(response.status).to eq(201) + expect(json_response['name']).to eq("COPY OF #{product_with_image.name}") end end end diff --git a/spec/controllers/api/variants_controller_spec.rb b/spec/controllers/api/variants_controller_spec.rb index 2e5fef6cd2..9964d40c1b 100644 --- a/spec/controllers/api/variants_controller_spec.rb +++ b/spec/controllers/api/variants_controller_spec.rb @@ -1,199 +1,197 @@ require 'spec_helper' -module Spree - describe Spree::Api::VariantsController, type: :controller do - render_views +describe Api::VariantsController, type: :controller do + render_views - let(:supplier) { FactoryBot.create(:supplier_enterprise) } - let!(:variant1) { FactoryBot.create(:variant) } - let!(:variant2) { FactoryBot.create(:variant) } - let!(:variant3) { FactoryBot.create(:variant) } - let(:attributes) { [:id, :options_text, :price, :on_hand, :unit_value, :unit_description, :on_demand, :display_as, :display_name] } + let(:supplier) { FactoryBot.create(:supplier_enterprise) } + let!(:variant1) { FactoryBot.create(:variant) } + let!(:variant2) { FactoryBot.create(:variant) } + let!(:variant3) { FactoryBot.create(:variant) } + let(:attributes) { [:id, :options_text, :price, :on_hand, :unit_value, :unit_description, :on_demand, :display_as, :display_name] } - before do - allow(controller).to receive(:spree_current_user) { current_api_user } + before do + allow(controller).to receive(:spree_current_user) { current_api_user } + end + + context "as a normal user" do + sign_in_as_user! + + let!(:product) { create(:product) } + let!(:variant) do + variant = product.master + variant.option_values << create(:option_value) + variant end - context "as a normal user" do - sign_in_as_user! + it "retrieves a list of variants with appropriate attributes" do + spree_get :index, format: :json - let!(:product) { create(:product) } - let!(:variant) do - variant = product.master - variant.option_values << create(:option_value) - variant + keys = json_response.first.keys.map(&:to_sym) + expect(attributes.all?{ |attr| keys.include? attr }).to eq(true) + end + + it "is denied access when trying to delete a variant" do + product = create(:product) + variant = product.master + spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json + + assert_unauthorized! + expect { variant.reload }.not_to raise_error + expect(variant.deleted_at).to be_nil + end + + it 'can query the results through a parameter' do + expected_result = create(:variant, sku: 'FOOBAR') + api_get :index, q: { sku_cont: 'FOO' } + + expect(json_response.size).to eq(1) + expect(json_response.first['sku']).to eq expected_result.sku + end + + # Regression test for spree#2141 + context "a deleted variant" do + before do + variant.update_column(:deleted_at, Time.zone.now) end - it "retrieves a list of variants with appropriate attributes" do - spree_get :index, format: :json - - keys = json_response.first.keys.map(&:to_sym) - expect(attributes.all?{ |attr| keys.include? attr }).to eq(true) + it "is not returned in the results" do + api_get :index + expect(json_response.count).to eq(10) # there are 11 variants end - it "is denied access when trying to delete a variant" do - product = create(:product) - variant = product.master - spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json - - assert_unauthorized! - expect { variant.reload }.not_to raise_error - expect(variant.deleted_at).to be_nil - end - - it 'can query the results through a parameter' do - expected_result = create(:variant, sku: 'FOOBAR') - api_get :index, q: { sku_cont: 'FOO' } - - expect(json_response.size).to eq(1) - expect(json_response.first['sku']).to eq expected_result.sku - end - - # Regression test for spree#2141 - context "a deleted variant" do - before do - variant.update_column(:deleted_at, Time.zone.now) - end - - it "is not returned in the results" do - api_get :index - expect(json_response.count).to eq(10) # there are 11 variants - end - - it "is not returned even when show_deleted is passed" do - api_get :index, show_deleted: true - expect(json_response.count).to eq(10) # there are 11 variants - end - end - - it "can see a single variant" do - api_get :show, id: variant.to_param - - keys = json_response.keys.map(&:to_sym) - expect((attributes).all?{ |attr| keys.include? attr }).to eq(true) - end - - it "cannot create a new variant if not an admin" do - api_post :create, variant: { sku: "12345" } - - assert_unauthorized! - end - - it "cannot update a variant" do - api_put :update, id: variant.to_param, variant: { sku: "12345" } - - assert_unauthorized! - end - - it "cannot delete a variant" do - api_delete :destroy, id: variant.to_param - - assert_unauthorized! - expect { variant.reload }.not_to raise_error + it "is not returned even when show_deleted is passed" do + api_get :index, show_deleted: true + expect(json_response.count).to eq(10) # there are 11 variants end end - context "as an enterprise user" do - sign_in_as_enterprise_user! [:supplier] - let(:supplier_other) { create(:supplier_enterprise) } - let(:product) { create(:product, supplier: supplier) } - let(:variant) { product.master } - let(:product_other) { create(:product, supplier: supplier_other) } - let(:variant_other) { product_other.master } + it "can see a single variant" do + api_get :show, id: variant.to_param - it "soft deletes a variant" do - spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json - - expect(response.status).to eq(204) - expect { variant.reload }.not_to raise_error - expect(variant.deleted_at).to be_present - end - - it "is denied access to soft deleting another enterprises' variant" do - spree_delete :soft_delete, variant_id: variant_other.to_param, product_id: product_other.to_param, format: :json - - assert_unauthorized! - expect { variant.reload }.not_to raise_error - expect(variant.deleted_at).to be_nil - end - - context 'when the variant is not the master' do - before { variant.update_attribute(:is_master, false) } - - it 'refreshes the cache' do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - spree_delete :soft_delete, variant_id: variant.id, product_id: variant.product.permalink, format: :json - end - end + keys = json_response.keys.map(&:to_sym) + expect((attributes).all?{ |attr| keys.include? attr }).to eq(true) end - context "as an administrator" do - sign_in_as_admin! + it "cannot create a new variant if not an admin" do + api_post :create, variant: { sku: "12345" } - let(:product) { create(:product) } - let(:variant) { product.master } - let(:resource_scoping) { { product_id: variant.product.to_param } } + assert_unauthorized! + end - it "soft deletes a variant" do - spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json + it "cannot update a variant" do + api_put :update, id: variant.to_param, variant: { sku: "12345" } - expect(response.status).to eq(204) - expect { variant.reload }.not_to raise_error - expect(variant.deleted_at).not_to be_nil - end + assert_unauthorized! + end - it "doesn't delete the only variant of the product" do - product = create(:product) - variant = product.variants.first - spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json + it "cannot delete a variant" do + api_delete :destroy, id: variant.to_param - expect(variant.reload).to_not be_deleted - expect(assigns(:variant).errors[:product]).to include "must have at least one variant" - end + assert_unauthorized! + expect { variant.reload }.not_to raise_error + end + end - context 'when the variant is not the master' do - before { variant.update_attribute(:is_master, false) } + context "as an enterprise user" do + sign_in_as_enterprise_user! [:supplier] + let(:supplier_other) { create(:supplier_enterprise) } + let(:product) { create(:product, supplier: supplier) } + let(:variant) { product.master } + let(:product_other) { create(:product, supplier: supplier_other) } + let(:variant_other) { product_other.master } - it 'refreshes the cache' do - expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) - spree_delete :soft_delete, variant_id: variant.id, product_id: variant.product.permalink, format: :json - end - end + it "soft deletes a variant" do + spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json - context "deleted variants" do - before do - variant.update_column(:deleted_at, Time.zone.now) - end + expect(response.status).to eq(204) + expect { variant.reload }.not_to raise_error + expect(variant.deleted_at).to be_present + end - it "are visible by admin" do - api_get :index, show_deleted: 1 + it "is denied access to soft deleting another enterprises' variant" do + spree_delete :soft_delete, variant_id: variant_other.to_param, product_id: product_other.to_param, format: :json - expect(json_response.count).to eq(2) - end - end + assert_unauthorized! + expect { variant.reload }.not_to raise_error + expect(variant.deleted_at).to be_nil + end - it "can create a new variant" do - original_number_of_variants = variant.product.variants.count - api_post :create, variant: { sku: "12345", unit_value: "weight", unit_description: "L" } + context 'when the variant is not the master' do + before { variant.update_attribute(:is_master, false) } - expect(attributes.all?{ |attr| json_response.include? attr.to_s }).to eq(true) - expect(response.status).to eq(201) - expect(json_response["sku"]).to eq("12345") - expect(variant.product.variants.count).to eq(original_number_of_variants + 1) - end - - it "can update a variant" do - api_put :update, id: variant.to_param, variant: { sku: "12345" } - - expect(response.status).to eq(200) - end - - it "can delete a variant" do - api_delete :destroy, id: variant.to_param - - expect(response.status).to eq(204) - expect { Spree::Variant.find(variant.id) }.to raise_error(ActiveRecord::RecordNotFound) + it 'refreshes the cache' do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) + spree_delete :soft_delete, variant_id: variant.id, product_id: variant.product.permalink, format: :json end end end + + context "as an administrator" do + sign_in_as_admin! + + let(:product) { create(:product) } + let(:variant) { product.master } + let(:resource_scoping) { { product_id: variant.product.to_param } } + + it "soft deletes a variant" do + spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json + + expect(response.status).to eq(204) + expect { variant.reload }.not_to raise_error + expect(variant.deleted_at).not_to be_nil + end + + it "doesn't delete the only variant of the product" do + product = create(:product) + variant = product.variants.first + spree_delete :soft_delete, variant_id: variant.to_param, product_id: product.to_param, format: :json + + expect(variant.reload).to_not be_deleted + expect(assigns(:variant).errors[:product]).to include "must have at least one variant" + end + + context 'when the variant is not the master' do + before { variant.update_attribute(:is_master, false) } + + it 'refreshes the cache' do + expect(OpenFoodNetwork::ProductsCache).to receive(:variant_destroyed).with(variant) + spree_delete :soft_delete, variant_id: variant.id, product_id: variant.product.permalink, format: :json + end + end + + context "deleted variants" do + before do + variant.update_column(:deleted_at, Time.zone.now) + end + + it "are visible by admin" do + api_get :index, show_deleted: 1 + + expect(json_response.count).to eq(2) + end + end + + it "can create a new variant" do + original_number_of_variants = variant.product.variants.count + api_post :create, variant: { sku: "12345", unit_value: "weight", unit_description: "L" } + + expect(attributes.all?{ |attr| json_response.include? attr.to_s }).to eq(true) + expect(response.status).to eq(201) + expect(json_response["sku"]).to eq("12345") + expect(variant.product.variants.count).to eq(original_number_of_variants + 1) + end + + it "can update a variant" do + api_put :update, id: variant.to_param, variant: { sku: "12345" } + + expect(response.status).to eq(200) + end + + it "can delete a variant" do + api_delete :destroy, id: variant.to_param + + expect(response.status).to eq(204) + expect { Spree::Variant.find(variant.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + end end From 4aa6c673ff8f6bf874da73de1dbd7fe218159993 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Thu, 1 Aug 2019 18:34:19 +0100 Subject: [PATCH 13/13] Adapt api products and variants controllers to new namespace outside of Spree --- app/controllers/api/products_controller.rb | 26 +++++++++++----------- app/controllers/api/variants_controller.rb | 24 ++++++++++---------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/app/controllers/api/products_controller.rb b/app/controllers/api/products_controller.rb index 6efa38806c..2c04c52697 100644 --- a/app/controllers/api/products_controller.rb +++ b/app/controllers/api/products_controller.rb @@ -1,23 +1,23 @@ require 'open_food_network/permissions' module Api - class ProductsController < ::Api::BaseController + class ProductsController < Api::BaseController respond_to :json skip_authorization_check only: [:show, :bulk_products, :overridable] def show @product = find_product(params[:id]) - render json: @product, serializer: ::Api::Admin::ProductSerializer + render json: @product, serializer: Api::Admin::ProductSerializer end def create - authorize! :create, Product + authorize! :create, Spree::Product params[:product][:available_on] ||= Time.zone.now - @product = Product.new(params[:product]) + @product = Spree::Product.new(params[:product]) begin if @product.save - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 + render json: @product, serializer: Api::Admin::ProductSerializer, status: 201 else invalid_resource!(@product) end @@ -28,21 +28,21 @@ module Api end def update - authorize! :update, Product + authorize! :update, Spree::Product @product = find_product(params[:id]) if @product.update_attributes(params[:product]) - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 200 + render json: @product, serializer: Api::Admin::ProductSerializer, status: 200 else invalid_resource!(@product) end end def destroy - authorize! :delete, Product + authorize! :delete, Spree::Product @product = find_product(params[:id]) @product.update_attribute(:deleted_at, Time.zone.now) @product.variants_including_master.update_all(deleted_at: Time.zone.now) - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 + render json: @product, serializer: Api::Admin::ProductSerializer, status: 204 end # TODO: This should be named 'managed'. Is the action above used? Maybe we should remove it. @@ -70,7 +70,7 @@ module Api @product = find_product(params[:product_id]) authorize! :delete, @product @product.destroy - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 204 + render json: @product, serializer: Api::Admin::ProductSerializer, status: 204 end # POST /api/products/:product_id/clone @@ -82,12 +82,12 @@ module Api @product = original_product.duplicate - render json: @product, serializer: ::Api::Admin::ProductSerializer, status: 201 + render json: @product, serializer: Api::Admin::ProductSerializer, status: 201 end private - # Copied and modified from Spree::Api::BaseController to allow + # Copied and modified from SpreeApi::BaseController to allow # enterprise users to access inactive products def product_scope # This line modified @@ -115,7 +115,7 @@ module Api def render_paged_products(products) serializer = ActiveModel::ArraySerializer.new( products, - each_serializer: ::Api::Admin::ProductSerializer + each_serializer: Api::Admin::ProductSerializer ) render text: { products: serializer, pages: products.num_pages }.to_json diff --git a/app/controllers/api/variants_controller.rb b/app/controllers/api/variants_controller.rb index 8c5ddafc39..b196270927 100644 --- a/app/controllers/api/variants_controller.rb +++ b/app/controllers/api/variants_controller.rb @@ -1,5 +1,5 @@ module Api - class VariantsController < ::Api::BaseController + class VariantsController < Api::BaseController respond_to :json skip_authorization_check only: [:index, :show] @@ -7,29 +7,29 @@ module Api def index @variants = scope.includes(:option_values).ransack(params[:q]).result - render json: @variants, each_serializer: ::Api::VariantSerializer + render json: @variants, each_serializer: Api::VariantSerializer end def show @variant = scope.includes(:option_values).find(params[:id]) - render json: @variant, serializer: ::Api::VariantSerializer + render json: @variant, serializer: Api::VariantSerializer end def create - authorize! :create, Variant + authorize! :create, Spree::Variant @variant = scope.new(params[:variant]) if @variant.save - render json: @variant, serializer: ::Api::VariantSerializer, status: 201 + render json: @variant, serializer: Api::VariantSerializer, status: 201 else invalid_resource!(@variant) end end def update - authorize! :update, Variant + authorize! :update, Spree::Variant @variant = scope.find(params[:id]) if @variant.update_attributes(params[:variant]) - render json: @variant, serializer: ::Api::VariantSerializer, status: 200 + render json: @variant, serializer: Api::VariantSerializer, status: 200 else invalid_resource!(@product) end @@ -40,14 +40,14 @@ module Api authorize! :delete, @variant VariantDeleter.new.delete(@variant) - render json: @variant, serializer: ::Api::VariantSerializer, status: 204 + render json: @variant, serializer: Api::VariantSerializer, status: 204 end def destroy - authorize! :delete, Variant + authorize! :delete, Spree::Variant @variant = scope.find(params[:id]) @variant.destroy - render json: @variant, serializer: ::Api::VariantSerializer, status: 204 + render json: @variant, serializer: Api::VariantSerializer, status: 204 end private @@ -64,10 +64,10 @@ module Api variants = @product.variants_including_master.with_deleted end else - variants = Variant.scoped + variants = Spree::Variant.scoped if current_api_user.has_spree_role?("admin") unless params[:show_deleted] - variants = Variant.active + variants = Spree::Variant.active end else variants = variants.active