From a867f7c5437b40bb4731c2f290701e59f10a29c1 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Sat, 22 Jul 2023 21:58:11 +0900 Subject: [PATCH 1/5] Add Product Scope Query --- .rubocop_todo.yml | 1 - app/controllers/api/v0/products_controller.rb | 65 ++--------------- app/models/spree/product.rb | 11 +++ app/queries/product_scope_query.rb | 69 +++++++++++++++++++ 4 files changed, 86 insertions(+), 60 deletions(-) create mode 100644 app/queries/product_scope_query.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 2a88db9bfe..b874419ffa 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -194,7 +194,6 @@ Metrics/ClassLength: - 'app/controllers/admin/resource_controller.rb' - 'app/controllers/admin/schedules_controller.rb' - 'app/controllers/admin/subscriptions_controller.rb' - - 'app/controllers/api/v0/products_controller.rb' - 'app/controllers/application_controller.rb' - 'app/controllers/payment_gateways/paypal_controller.rb' - 'app/controllers/spree/admin/orders_controller.rb' diff --git a/app/controllers/api/v0/products_controller.rb b/app/controllers/api/v0/products_controller.rb index 6b6baa311a..e94a1e3832 100644 --- a/app/controllers/api/v0/products_controller.rb +++ b/app/controllers/api/v0/products_controller.rb @@ -13,7 +13,7 @@ module Api skip_authorization_check only: [:show, :bulk_products, :overridable] def show - @product = find_product(params[:id]) + @product = ProductScopeQuery.new(current_api_user, params).find_product render json: @product, serializer: Api::Admin::ProductSerializer end @@ -30,7 +30,7 @@ module Api def update authorize! :update, Spree::Product - @product = find_product(params[:id]) + @product = ProductScopeQuery.new(current_api_user, params).find_product if @product.update(product_params) render json: @product, serializer: Api::Admin::ProductSerializer, status: :ok else @@ -40,36 +40,20 @@ module Api def destroy authorize! :delete, Spree::Product - @product = find_product(params[:id]) + @product = ProductScopeQuery.new(current_api_user, params).find_product authorize! :delete, @product @product.destroy render json: @product, serializer: Api::Admin::ProductSerializer, status: :no_content end def bulk_products - product_query = OpenFoodNetwork::Permissions. - new(current_api_user). - editable_products. - merge(product_scope) - - if params[:import_date].present? - product_query = product_query. - imported_on(params[:import_date]). - group_by_products_id - end - - @products = product_query. - ransack(query_params_with_defaults). - result + @products = Spree::Product.bulk_products(current_api_user, params) render_paged_products @products end def overridable - producer_ids = OpenFoodNetwork::Permissions.new(current_api_user). - variant_override_producers.by_name.select('enterprises.id') - - @products = paged_products_for_producers producer_ids + @products = Spree::Product.paged_products_for_producers(current_api_user, params) render_paged_products @products, ::Api::Admin::ProductSimpleSerializer end @@ -78,7 +62,7 @@ module Api # def clone authorize! :create, Spree::Product - original_product = find_product(params[:product_id]) + original_product = ProductScopeQuery.new(current_api_user, params).find_product_to_be_cloned authorize! :update, original_product @product = original_product.duplicate @@ -88,39 +72,6 @@ module Api private - def find_product(id) - product_scope.find(id) - end - - def product_scope - 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 - end - - scope.includes(product_query_includes) - end - - def product_query_includes - [ - image: { attachment_attachment: :blob }, - variants: [:default_price, :stock_locations, :stock_items, :variant_overrides] - ] - end - - def paged_products_for_producers(producer_ids) - Spree::Product.where(nil). - merge(product_scope). - includes(variants: [:product, :default_price, :stock_items]). - where(supplier_id: producer_ids). - by_producer.by_name. - ransack(params[:q]).result - end - def render_paged_products(products, product_serializer = ::Api::Admin::ProductSerializer) @pagy, products = pagy(products, items: params[:per_page] || DEFAULT_PER_PAGE) @@ -135,10 +86,6 @@ module Api } end - def query_params_with_defaults - (params[:q] || {}).reverse_merge(s: 'created_at desc') - end - def product_params @product_params ||= params.permit(product: PermittedAttributes::Product.attributes)[:product].to_h diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 0378df7322..76e3b8745c 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -189,6 +189,17 @@ module Spree scope :active, lambda { where("spree_products.deleted_at IS NULL") } + scope :bulk_products, lambda { |user, params| + ProductScopeQuery.new(user, params).bulk_products + } + + scope :paged_products_for_producers, + lambda { |user, params| ProductScopeQuery.new(user, params).paged_products_for_producers } + + scope :product_scope, lambda { |user, params| + ProductScopeQuery.new(user, params).product_scope + } + def self.group_by_products_id group(column_names.map { |col_name| "#{table_name}.#{col_name}" }) end diff --git a/app/queries/product_scope_query.rb b/app/queries/product_scope_query.rb new file mode 100644 index 0000000000..bc8b6953b2 --- /dev/null +++ b/app/queries/product_scope_query.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +class ProductScopeQuery + def initialize(user, params) + @user = user + @params = params + end + + def bulk_products + product_query = OpenFoodNetwork::Permissions. + new(@user). + editable_products. + merge(product_scope) + + if @params[:import_date].present? + product_query = product_query. + imported_on(@params[:import_date]). + group_by_products_id + end + + product_query. + ransack(query_params_with_defaults). + result + end + + def find_product + product_scope.find(@params[:id]) + end + + def find_product_to_be_cloned + product_scope.find(@params[:product_id]) + end + + def paged_products_for_producers + producer_ids = OpenFoodNetwork::Permissions.new(@user). + variant_override_producers.by_name.select('enterprises.id') + + Spree::Product.where(nil). + merge(product_scope). + includes(variants: [:product, :default_price, :stock_items]). + where(supplier_id: producer_ids). + by_producer.by_name. + ransack(@params[:q]).result + end + + def product_scope + if @user.has_spree_role?("admin") || @user.enterprises.present? + scope = Spree::Product + if @params[:show_deleted] + scope = scope.with_deleted + end + else + scope = Spree::Product.active + end + + scope.includes(product_query_includes) + end + + def product_query_includes + [ + image: { attachment_attachment: :blob }, + variants: [:default_price, :stock_locations, :stock_items, :variant_overrides] + ] + end + + def query_params_with_defaults + (@params[:q] || {}).reverse_merge(s: 'created_at desc') + end +end From c125c42a6d09e4e1645b5e2ab2303e5b985f0957 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Wed, 26 Jul 2023 23:06:43 +0900 Subject: [PATCH 2/5] Add Tests for Product Scope Query --- spec/queries/product_scope_query_spec.rb | 76 ++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100755 spec/queries/product_scope_query_spec.rb diff --git a/spec/queries/product_scope_query_spec.rb b/spec/queries/product_scope_query_spec.rb new file mode 100755 index 0000000000..87926b847f --- /dev/null +++ b/spec/queries/product_scope_query_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ProductScopeQuery do + let!(:taxon) { create(:taxon) } + let(:supplier) { create(:supplier_enterprise) } + let(:supplier2) { create(:supplier_enterprise) } + let!(:product) { create(:product, supplier:, primary_taxon: taxon) } + let!(:product2) { create(:product, supplier: supplier2, primary_taxon: taxon) } + let!(:current_api_user) { supplier_enterprise_user(supplier) } + + before { current_api_user.enterprise_roles.create(enterprise: supplier2) } + + describe 'bulk update' do + let!(:product3) { create(:product, supplier: supplier2) } + + it "returns a list of products" do + expect(ProductScopeQuery.new(current_api_user, {}).bulk_products) + .to include(product, product2, product3) + end + + it "filters results by supplier" do + subject = ProductScopeQuery + .new(current_api_user, { q: { supplier_id_eq: supplier.id } }).bulk_products + + expect(subject).to include(product) + expect(subject).not_to include(product2, product3) + end + + it "filters results by product category" do + subject = ProductScopeQuery + .new(current_api_user, { q: { primary_taxon_id_eq: taxon.id } }).bulk_products + + expect(subject).to include(product, product2) + expect(subject).not_to include(product3) + end + + it "filters results by import_date" do + product.variants.first.update_attribute :import_date, 1.day.ago + product2.variants.first.update_attribute :import_date, 2.days.ago + product3.variants.first.update_attribute :import_date, 1.day.ago + + subject = ProductScopeQuery + .new(current_api_user, { import_date: 1.day.ago.to_date.to_s }).bulk_products + + expect(subject).to include(product, product3) + expect(subject).not_to include(product2) + end + end + + describe 'finder functions' do + let!(:query) { + ProductScopeQuery.new(current_api_user, { id: product.id, product_id: product2.id }) + } + describe 'find_product' do + it 'finds product' do + expect(query.find_product).to eq(product) + end + end + + describe 'find_product_to_be_cloned' do + it 'finds product to be cloned' do + expect(query.find_product_to_be_cloned).to eq(product2) + end + end + end + + private + + def supplier_enterprise_user(enterprise) + user = create(:user) + user.enterprise_roles.create(enterprise:) + user + end +end From 9e82ab8a0f7efebf9a7865a1223b45a2ff74f926 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Wed, 26 Jul 2023 23:25:43 +0900 Subject: [PATCH 3/5] Remove query scopes from product model --- app/models/spree/product.rb | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 76e3b8745c..0378df7322 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -189,17 +189,6 @@ module Spree scope :active, lambda { where("spree_products.deleted_at IS NULL") } - scope :bulk_products, lambda { |user, params| - ProductScopeQuery.new(user, params).bulk_products - } - - scope :paged_products_for_producers, - lambda { |user, params| ProductScopeQuery.new(user, params).paged_products_for_producers } - - scope :product_scope, lambda { |user, params| - ProductScopeQuery.new(user, params).product_scope - } - def self.group_by_products_id group(column_names.map { |col_name| "#{table_name}.#{col_name}" }) end From fc47c57603817688dea15833e7c034cc33a87cc6 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Wed, 26 Jul 2023 23:26:11 +0900 Subject: [PATCH 4/5] Add helper method to products controller and Use query object --- app/controllers/api/v0/products_controller.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/v0/products_controller.rb b/app/controllers/api/v0/products_controller.rb index e94a1e3832..eead37ca49 100644 --- a/app/controllers/api/v0/products_controller.rb +++ b/app/controllers/api/v0/products_controller.rb @@ -13,7 +13,7 @@ module Api skip_authorization_check only: [:show, :bulk_products, :overridable] def show - @product = ProductScopeQuery.new(current_api_user, params).find_product + @product = product_finder.find_product render json: @product, serializer: Api::Admin::ProductSerializer end @@ -30,7 +30,7 @@ module Api def update authorize! :update, Spree::Product - @product = ProductScopeQuery.new(current_api_user, params).find_product + @product = product_finder.find_product if @product.update(product_params) render json: @product, serializer: Api::Admin::ProductSerializer, status: :ok else @@ -40,20 +40,20 @@ module Api def destroy authorize! :delete, Spree::Product - @product = ProductScopeQuery.new(current_api_user, params).find_product + @product = product_finder.find_product authorize! :delete, @product @product.destroy render json: @product, serializer: Api::Admin::ProductSerializer, status: :no_content end def bulk_products - @products = Spree::Product.bulk_products(current_api_user, params) + @products = product_finder.bulk_products render_paged_products @products end def overridable - @products = Spree::Product.paged_products_for_producers(current_api_user, params) + @products = product_finder.paged_products_for_producers render_paged_products @products, ::Api::Admin::ProductSimpleSerializer end @@ -62,7 +62,7 @@ module Api # def clone authorize! :create, Spree::Product - original_product = ProductScopeQuery.new(current_api_user, params).find_product_to_be_cloned + original_product = product_finder.find_product_to_be_cloned authorize! :update, original_product @product = original_product.duplicate @@ -72,6 +72,10 @@ module Api private + def product_finder + ProductScopeQuery.new(current_api_user, params) + end + def render_paged_products(products, product_serializer = ::Api::Admin::ProductSerializer) @pagy, products = pagy(products, items: params[:per_page] || DEFAULT_PER_PAGE) From 0b6b750706557277f33a4450e045e8fca495f2d7 Mon Sep 17 00:00:00 2001 From: Neal Chambers Date: Mon, 31 Jul 2023 09:32:22 +0900 Subject: [PATCH 5/5] Clarify Product Scope Query Spec --- spec/queries/product_scope_query_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/queries/product_scope_query_spec.rb b/spec/queries/product_scope_query_spec.rb index 87926b847f..f8bde2c8da 100755 --- a/spec/queries/product_scope_query_spec.rb +++ b/spec/queries/product_scope_query_spec.rb @@ -50,16 +50,15 @@ describe ProductScopeQuery do end describe 'finder functions' do - let!(:query) { - ProductScopeQuery.new(current_api_user, { id: product.id, product_id: product2.id }) - } describe 'find_product' do + subject(:query) { ProductScopeQuery.new(current_api_user, { id: product.id }) } it 'finds product' do expect(query.find_product).to eq(product) end end describe 'find_product_to_be_cloned' do + subject(:query) { ProductScopeQuery.new(current_api_user, { product_id: product2.id }) } it 'finds product to be cloned' do expect(query.find_product_to_be_cloned).to eq(product2) end