diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index 1f79ebf95c..7df8c7b357 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -29,12 +29,10 @@ module Spree acts_as_paranoid - searchable_attributes :supplier_id, :meta_keywords, :sku - searchable_associations :supplier, :properties, :variants + searchable_attributes :meta_keywords, :sku + searchable_associations :properties, :variants searchable_scopes :active, :with_properties - belongs_to :supplier, class_name: 'Enterprise', optional: false, touch: true - has_one :image, class_name: "Spree::Image", as: :viewable, dependent: :destroy has_many :product_properties, dependent: :destroy @@ -45,7 +43,6 @@ module Spree has_many :prices, -> { order('spree_variants.id, currency') }, through: :variants has_many :stock_items, through: :variants - has_many :supplier_properties, through: :supplier, source: :properties has_many :variant_images, -> { order(:position) }, source: :images, through: :variants @@ -79,14 +76,11 @@ module Spree around_destroy :destruction after_save :update_units + # -- Scopes scope :with_properties, ->(*property_ids) { left_outer_joins(:product_properties). - left_outer_joins(:supplier_properties). where(inherits_properties: true). - where(producer_properties: { property_id: property_ids }). - or( - where(spree_product_properties: { property_id: property_ids }) - ) + where(spree_product_properties: { property_id: property_ids }) } scope :with_order_cycles_outer, -> { @@ -126,8 +120,9 @@ module Spree distinct } - # -- Scopes - scope :in_supplier, lambda { |supplier| where(supplier_id: supplier) } + scope :in_supplier, lambda { |supplier| + joins(:variants).where(spree_variants: { supplier: }) + } # Products distributed via the given distributor through an OC scope :in_distributor, lambda { |distributor| @@ -149,10 +144,10 @@ module Spree enterprise = enterprise.respond_to?(:id) ? enterprise.id : enterprise.to_i with_order_cycles_outer. - where(" - spree_products.supplier_id = ? - OR (o_exchanges.incoming = ? AND o_exchanges.receiver_id = ?) - ", enterprise, false, enterprise). + in_supplier(enterprise). + or( + where(o_exchanges: { incoming: false, receiver_id: enterprise }) + ). select('distinct spree_products.*') } @@ -170,14 +165,14 @@ module Spree where.not(order_cycles: { id: nil }) } - scope :by_producer, -> { joins(:supplier).order('enterprises.name') } - scope :by_name, -> { order('name') } + scope :by_producer, -> { joins(variants: :supplier).order('enterprises.name') } + scope :by_name, -> { order('spree_products.name') } scope :managed_by, lambda { |user| if user.has_spree_role?('admin') where(nil) else - where(supplier_id: user.enterprises.select("enterprises.id")) + in_supplier(user.enterprises) end } @@ -188,7 +183,8 @@ module Spree .with_permission(:add_to_order_cycle) .where(enterprises: { is_primary_producer: true }) .pluck(:parent_id) - where(spree_products: { supplier_id: [enterprise.id] | permitted_producer_ids }) + + in_supplier([enterprise.id].union(permitted_producer_ids)) } scope :active, lambda { where(spree_products: { deleted_at: nil }) } @@ -236,7 +232,10 @@ module Spree ps = product_properties.all if inherits_properties - ps = OpenFoodNetwork::PropertyMerge.merge(ps, supplier.producer_properties) + # NOTE: Set the supplier as the first variant supplier. If variants have different supplier, + # result might not be correct + supplier = variants.first.supplier + ps = OpenFoodNetwork::PropertyMerge.merge(ps, supplier&.producer_properties || []) end ps. diff --git a/db/migrate/20240221060301_add_supplier_to_variants.rb b/db/migrate/20240221060301_add_supplier_to_variants.rb new file mode 100644 index 0000000000..84eea57fd2 --- /dev/null +++ b/db/migrate/20240221060301_add_supplier_to_variants.rb @@ -0,0 +1,5 @@ +class AddSupplierToVariants < ActiveRecord::Migration[7.0] + def change + add_reference :spree_variants, :supplier, foreign_key: { to_table: :enterprises } + end +end diff --git a/db/schema.rb b/db/schema.rb index ba68c5c4b6..e8b49d884d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -977,10 +977,12 @@ ActiveRecord::Schema[7.0].define(version: 2024_06_25_024328) do t.bigint "tax_category_id" t.bigint "shipping_category_id" t.bigint "primary_taxon_id" + t.bigint "supplier_id" t.index ["primary_taxon_id"], name: "index_spree_variants_on_primary_taxon_id" t.index ["product_id"], name: "index_variants_on_product_id" t.index ["shipping_category_id"], name: "index_spree_variants_on_shipping_category_id" t.index ["sku"], name: "index_spree_variants_on_sku" + t.index ["supplier_id"], name: "index_spree_variants_on_supplier_id" t.index ["tax_category_id"], name: "index_spree_variants_on_tax_category_id" t.check_constraint "unit_value > 0::double precision", name: "positive_unit_value" end @@ -1221,6 +1223,7 @@ ActiveRecord::Schema[7.0].define(version: 2024_06_25_024328) do add_foreign_key "spree_taxons", "spree_taxons", column: "parent_id", name: "spree_taxons_parent_id_fk" add_foreign_key "spree_users", "spree_addresses", column: "bill_address_id", name: "spree_users_bill_address_id_fk" add_foreign_key "spree_users", "spree_addresses", column: "ship_address_id", name: "spree_users_ship_address_id_fk" + add_foreign_key "spree_variants", "enterprises", column: "supplier_id" add_foreign_key "spree_variants", "spree_products", column: "product_id", name: "spree_variants_product_id_fk" add_foreign_key "spree_variants", "spree_shipping_categories", column: "shipping_category_id" add_foreign_key "spree_variants", "spree_tax_categories", column: "tax_category_id" diff --git a/spec/factories/product_factory.rb b/spec/factories/product_factory.rb index 8e1d5e64c7..454a1bd689 100644 --- a/spec/factories/product_factory.rb +++ b/spec/factories/product_factory.rb @@ -14,8 +14,6 @@ FactoryBot.define do sku { 'ABC' } deleted_at { nil } - supplier { Enterprise.is_primary_producer.first || FactoryBot.create(:supplier_enterprise) } - unit_value { 1 } unit_description { '' } diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 26733c8759..0293b27888 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -10,6 +10,10 @@ module Spree context '#duplicate' do it 'duplicates product' do + variant = product.variants.first + variant.supplier = create(:supplier_enterprise) + variant.save! + clone = product.duplicate expect(clone).to be_persisted @@ -84,65 +88,6 @@ module Spree end end - describe "supplier properties" do - subject { create(:product) } - - it "has no supplier properties to start with" do - expect(subject.supplier_properties).to eq [] - end - - it "doesn't include product properties" do - subject.set_property("certified", "organic") - expect(subject.supplier_properties).to eq [] - end - - it "includes the supplier's properties" do - subject.supplier.set_producer_property("certified", "yes") - expect(subject.supplier_properties.map(&:presentation)).to eq ["certified"] - end - end - - describe ".with_properties scope" do - let!(:product_without_wanted_property_on_supplier) { - create(:product, supplier: supplier_without_wanted_property) - } - let!(:product_with_wanted_property_on_supplier) { - create(:product, supplier: supplier_with_wanted_property) - } - let!(:product_with_wanted_property) { create(:product, properties: [wanted_property]) } - let!(:product_without_wanted_property_property) { - create(:product, properties: [unwanted_property]) - } - let!(:product_with_wanted_property_and_on_supplier) { - create(:product, properties: [wanted_property], supplier: supplier_with_wanted_property) - } - let!(:product_ignoring_property) { - create(:product, supplier: supplier_with_wanted_property, inherits_properties: false) - } - let(:supplier_with_wanted_property) { - create(:supplier_enterprise, properties: [wanted_property]) - } - let(:supplier_without_wanted_property) { - create(:supplier_enterprise, properties: [unwanted_property]) - } - let(:wanted_property) { create(:property, presentation: 'Certified Organic') } - let(:unwanted_property) { create(:property, presentation: 'Latest Hype') } - - it "returns no products without a property id" do - expect(Spree::Product.with_properties([])).to eq [] - end - - it "returns only products with the wanted property set both on supplier & product itself" do - expect( - Spree::Product.with_properties([wanted_property.id]) - ).to match_array [ - product_with_wanted_property_on_supplier, - product_with_wanted_property, - product_with_wanted_property_and_on_supplier - ] - end - end - describe '#total_on_hand' do it 'returns sum of stock items count_on_hand' do product = build(:product) @@ -162,10 +107,6 @@ module Spree end end - describe "associations" do - it { is_expected.to belong_to(:supplier).required } - end - describe "validations and defaults" do it "is valid when built from factory" do expect(build(:product)).to be_valid @@ -196,10 +137,6 @@ module Spree end end - it "requires a supplier" do - expect(build(:simple_product, supplier: nil)).not_to be_valid - end - context "when the product has variants" do let(:product) do product = create(:simple_product) @@ -238,7 +175,6 @@ module Spree before do create(:stock_location) product.primary_taxon_id = taxon.id - product.supplier = create(:supplier_enterprise) product.name = "Product1" product.variant_unit = "weight" product.variant_unit_scale = 1000 @@ -330,13 +266,16 @@ module Spree let(:product) { create(:simple_product) } describe "touching affected enterprises when the product is deleted" do - let(:product) { create(:simple_product, supplier: distributor) } - let(:supplier) { product.supplier } + let(:product) { create(:simple_product) } + let(:supplier) { create(:supplier_enterprise) } let(:distributor) { create(:distributor_enterprise) } let!(:oc) { create(:simple_order_cycle, distributors: [distributor], variants: [product.variants.first]) } + before do + create(:variant, product:, supplier:) + end it "touches the supplier" do expect { product.destroy }.to change { supplier.reload.updated_at } @@ -377,12 +316,37 @@ module Spree end describe "scopes" do + describe ".with_properties" do + let!(:product_with_wanted_property) { create(:product, properties: [wanted_property]) } + let!(:product_without_wanted_property_property) { + create(:product, properties: [unwanted_property]) + } + let!(:product_ignoring_property) { + create(:product, inherits_properties: false) + } + let(:wanted_property) { create(:property, presentation: 'Certified Organic') } + let(:unwanted_property) { create(:property, presentation: 'Latest Hype') } + + it "returns no products without a property id" do + expect(Spree::Product.with_properties([])).to eq [] + end + + it "returns only products with the wanted property set both on supplier & product itself" do + expect( + Spree::Product.with_properties([wanted_property.id]) + ).to match_array [product_with_wanted_property] + end + end + describe "in_supplier" do it "shows products in supplier" do s1 = create(:supplier_enterprise) + p1 = create(:product, supplier_id: s1.id) + create(:variant, product: p1, supplier: s1) s2 = create(:supplier_enterprise) - p1 = create(:product, supplier: s1) - p2 = create(:product, supplier: s2) + p2 = create(:product, supplier_id: s2.id) + create(:variant, product: p2, supplier: s2) + expect(Product.in_supplier(s1)).to eq([p1]) end end @@ -461,9 +425,12 @@ module Spree describe "in_supplier_or_distributor" do it "shows products in supplier" do s1 = create(:supplier_enterprise) + p1 = create(:product, supplier_id: s1.id) + create(:variant, product: p1, supplier: s1) s2 = create(:supplier_enterprise) - p1 = create(:product, supplier: s1) - p2 = create(:product, supplier: s2) + p2 = create(:product, supplier_id: s2.id) + create(:variant, product: p2, supplier: s2) + expect(Product.in_supplier_or_distributor(s1)).to eq([p1]) end @@ -483,7 +450,9 @@ module Spree it "shows products in all three without duplicates" do s = create(:supplier_enterprise) d = create(:distributor_enterprise) - p = create(:product, supplier: s) + p = create(:product) + create(:variant, product: p, supplier: s) + create(:simple_order_cycle, suppliers: [s], distributors: [d], variants: [p.variants.first]) [s, d].each { |e| expect(Product.in_supplier_or_distributor(e)).to eq([p]) } @@ -523,31 +492,57 @@ module Spree end end - describe "access roles" do + describe "by_producer" do + it "orders by name" do + producer_z = create(:enterprise, name: "z_cooperative") + producer_a = create(:enterprise, name: "a_cooperative") + producer_g = create(:enterprise, name: "g_cooperative") + + product_1 = create(:product) + create(:variant, product: product_1, supplier: producer_z) + product_2 = create(:product) + create(:variant, product: product_2, supplier: producer_a) + product_3 = create(:product) + create(:variant, product: product_3, supplier: producer_g) + + expect(Product.by_producer).to match_array([ + product_2, + product_3, + product_1, + ]) + end + end + + describe "managed_by" do + let!(:e1) { create(:enterprise) } + let!(:e2) { create(:enterprise) } + let!(:p1) { create(:product) } + let!(:p2) { create(:product) } + before(:each) do - @e1 = create(:enterprise) - @e2 = create(:enterprise) - @p1 = create(:product, supplier: @e1) - @p2 = create(:product, supplier: @e2) + create(:variant, product: p1, supplier: e1) + create(:variant, product: p1, supplier: e2) end it "shows only products for given user" do user = create(:user) user.spree_roles = [] - @e1.enterprise_roles.build(user:).save + e1.enterprise_roles.build(user:).save product = Product.managed_by user + expect(product.count).to eq(1) - expect(product).to include @p1 + expect(product).to include p1 end it "shows all products for admin user" do user = create(:admin_user) product = Product.managed_by user + expect(product.count).to eq(2) - expect(product).to include @p1 - expect(product).to include @p2 + expect(product).to include p1 + expect(product).to include p2 end end @@ -583,11 +578,15 @@ module Spree let(:shop) { create(:distributor_enterprise) } let(:add_to_oc_producer) { create(:supplier_enterprise) } let(:other_producer) { create(:supplier_enterprise) } - let!(:p1) { create(:simple_product, supplier: shop ) } - let!(:p2) { create(:simple_product, supplier: add_to_oc_producer ) } - let!(:p3) { create(:simple_product, supplier: other_producer ) } + let(:p1) { create(:simple_product) } + let(:p2) { create(:simple_product) } + let(:p3) { create(:simple_product) } before do + create(:variant, product: p1, supplier: shop) + create(:variant, product: p2, supplier: add_to_oc_producer) + create(:variant, product: p3, supplier: other_producer) + create(:enterprise_relationship, parent: add_to_oc_producer, child: shop, permissions_list: [:add_to_order_cycle]) create(:enterprise_relationship, parent: other_producer, child: shop, @@ -613,59 +612,54 @@ module Spree end end - describe "properties" do + describe "#properties_including_inherited" do + let(:product) { create(:simple_product) } + let(:supplier) { create(:supplier_enterprise) } + + before do + product.variants = [] + product.variants << create(:variant, product:, supplier:) + end + it "returns product properties as a hash" do - product = create(:simple_product) product.set_property 'Organic Certified', 'NASAA 12345' property = product.properties.last expect(product.properties_including_inherited) - .to eq([{ id: property.id, name: "Organic Certified", - value: 'NASAA 12345' }]) + .to eq([{ id: property.id, name: "Organic Certified", value: 'NASAA 12345' }]) end it "returns producer properties as a hash" do - supplier = create(:supplier_enterprise) - product = create(:simple_product, supplier:) - supplier.set_producer_property 'Organic Certified', 'NASAA 54321' property = supplier.properties.last expect(product.properties_including_inherited) - .to eq([{ id: property.id, name: "Organic Certified", - value: 'NASAA 54321' }]) + .to eq([{ id: property.id, name: "Organic Certified", value: 'NASAA 54321' }]) end it "overrides producer properties with product properties" do - supplier = create(:supplier_enterprise) - product = create(:simple_product, supplier:) - product.set_property 'Organic Certified', 'NASAA 12345' supplier.set_producer_property 'Organic Certified', 'NASAA 54321' property = product.properties.last expect(product.properties_including_inherited) - .to eq([{ id: property.id, name: "Organic Certified", - value: 'NASAA 12345' }]) + .to eq([{ id: property.id, name: "Organic Certified", value: 'NASAA 12345' }]) end context "when product has an inherit_properties value set to true" do - let(:supplier) { create(:supplier_enterprise) } - let(:product) { create(:simple_product, supplier:, inherits_properties: true) } + let(:product) { create(:simple_product, inherits_properties: true) } it "inherits producer properties" do supplier.set_producer_property 'Organic Certified', 'NASAA 54321' property = supplier.properties.last expect(product.properties_including_inherited) - .to eq([{ id: property.id, name: "Organic Certified", - value: 'NASAA 54321' }]) + .to eq([{ id: property.id, name: "Organic Certified", value: 'NASAA 54321' }]) end end context "when product has an inherit_properties value set to false" do - let(:supplier) { create(:supplier_enterprise) } - let(:product) { create(:simple_product, supplier:, inherits_properties: false) } + let(:product) { create(:simple_product, inherits_properties: false) } it "does not inherit producer properties" do supplier.set_producer_property 'Organic Certified', 'NASAA 54321' @@ -675,9 +669,6 @@ module Spree end it "sorts by position" do - supplier = create(:supplier_enterprise) - product = create(:simple_product, supplier:) - pa = Spree::Property.create! name: 'A', presentation: 'A' pb = Spree::Property.create! name: 'B', presentation: 'B' pc = Spree::Property.create! name: 'C', presentation: 'C' @@ -743,18 +734,25 @@ module Spree end describe "deletion" do - let(:p) { create(:simple_product) } - let(:v) { create(:variant, product: p) } - let(:oc) { create(:simple_order_cycle) } - let(:s) { create(:supplier_enterprise) } - let(:e) { - create(:exchange, order_cycle: oc, incoming: true, sender: s, receiver: oc.coordinator) + let(:product) { create(:simple_product) } + let(:variant) { create(:variant, product:) } + let(:order_cycle) { create(:simple_order_cycle) } + let(:supplier) { create(:supplier_enterprise) } + let(:exchange) { + create( + :exchange, + order_cycle:, + incoming: true, + sender: supplier, + receiver: order_cycle.coordinator + ) } it "removes all variants from order cycles" do - e.variants << v - p.destroy - expect(e.variants.reload).to be_empty + exchange.variants << variant + + product.destroy + expect(exchange.variants.reload).to be_empty end end