Move supplier to variant part 1

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.

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

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"

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 { '' }
 

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