Copy all prices from master over children variants

This commit changes functionalities in three ways:

- Backend: Before this change, when a user created a new variant the
  form was pre-populated only with a single price field for the default
  currency. In the case the user removed the amount field and submitted
  the form, the created price had an amount equal to 0. Now, all the
  prices created for a given product (for a given master variant, in
  fact) get pre-populated in a nested form. In the case one of the
  amounts is removed by the user, the form becomes invalid on submission
  and forces the user to set an amount for every price.

- API: Before this change, the user was able to create a variant 1)
  manually assigning a single price in the default currency through the
  `price` attribute, and 2) letting it inherit the default price from
  the master variant when `price` attribute was not present. Now, when
  not overridden all master prices get inherited. Otherwise, the user
  can specify both through `price` (for the default currency) and
  `prices_attributes` attributes.

- Product duplicator: Before this change, only the default master price
  was copied. Now, all prices are taken into account.

Internally, the `set_price` callback on `Variant` has been removed in
order to allow that flexibility. The behavior has been moved to a layer
one step closer than where it should actually be: A service layer. It
will help when we tackle this long-term goal. It also means that when we
manually create a variant we must specify the default price for it.

Having three rails associations between `Variant` and `Price` is a
source of inconsistencies, as when we add a `Price` in-memory to one of
them the others don't get updated. It should be changed as follow-up
work. For now, `Variant#any_price?` hack has been added. In the same
way, `Variant#find_or_build_default_price` didn't find it when it still
was in-memory. `Variant#default_price_from_memory` have been added to
fix it.

api/app/controllers/spree/api/variants_controller.rb

@@ -8,6 +8,7 @@ class VariantsController < Spree::Api::BaseController
       def create
         authorize! :create, Variant
         @variant = scope.new(variant_params)
+        @variant.inherit_prices unless @variant.any_price?
         if @variant.save
           respond_with(@variant, status: 201, default_template: :show)
         else

api/spec/requests/spree/api/products_controller_spec.rb

@@ -126,7 +126,7 @@ module Spree
 
       it "gets a single product" do
         product.master.images.create!(attachment: image("blank.jpg"))
-        product.variants.create!
+        product.variants.create!(price: 50)
         product.variants.first.images.create!(attachment: image("blank.jpg"))
         product.set_property("spree", "rocks")
         product.taxons << create(:taxon)

api/spec/requests/spree/api/variants_controller_spec.rb

@@ -308,6 +308,33 @@ module Spree
         expect(variant.product.variants.count).to eq(1)
       end
 
+      it "inherites prices from master" do
+        product = create(:product, price: 50)
+
+        post spree.api_product_variants_path(product), params: { variant: { sku: "12345" } }
+
+        variant = Spree::Variant.find(json_response["id"])
+        expect(variant.price).to eq(50)
+      end
+
+      it "can override prices inherited from master through nested attributes" do
+        product = create(:product, price: 50)
+
+        post spree.api_product_variants_path(product), params: { variant: { sku: "12345", prices_attributes: [amount: 25, currency: 'USD'] } }
+
+        variant = Spree::Variant.find(json_response["id"])
+        expect(variant.price).to eq(25)
+      end
+
+      it "can override prices inherited from master through default price attribute" do
+        product = create(:product, price: 50)
+
+        post spree.api_product_variants_path(product), params: { variant: { sku: "12345", price: 25 } }
+
+        variant = Spree::Variant.find(json_response["id"])
+        expect(variant.price).to eq(25)
+      end
+
       it "creates new variants with nested option values" do
         option_values = create_list(:option_value, 2)
         expect do

backend/app/assets/stylesheets/spree/backend/components/_number_with_currency.scss

@@ -42,4 +42,9 @@
       }
     }
   }
+
+  .field_with_errors {
+    flex: 1 1 auto;
+    width: 1%;
+  }
 }

backend/app/controllers/spree/admin/variants_controller.rb

@@ -15,8 +15,7 @@ class VariantsController < ResourceController
       def new_before
         @object.attributes = @object.product.master.attributes.except('id', 'created_at', 'deleted_at',
                                                                       'sku', 'is_master')
-        # Shallow Clone of the default price to populate the price field.
-        @object.default_price = @object.product.master.default_price.clone
+        @object.inherit_prices
       end
 
       def collection

backend/app/views/spree/admin/shared/_number_with_currency.html.erb

@@ -2,12 +2,13 @@
 <% currency_attr ||= :currency %>
 <% currency ||= nil %>
 <% required ||= nil %>
+<% extra_attrs ||= {} %>
 
 <div class="input-group number-with-currency <%= "number-with-currency-with-select" unless currency %> js-number-with-currency">
   <div class="input-group-prepend">
     <span class="input-group-text number-with-currency-symbol"></span>
   </div>
-  <%= f.text_field amount_attr, value: number_to_currency(f.object.public_send(amount_attr), unit: '', delimiter: ''), class: 'form-control number-with-currency-amount', required: required %>
+  <%= f.text_field amount_attr, extra_attrs.merge(value: number_to_currency(f.object.public_send(amount_attr), unit: '', delimiter: ''), class: 'form-control number-with-currency-amount', required: required) %>
   <% if currency %>
     <div class="input-group-append">
       <span class="input-group-text number-with-currency-addon" data-currency="<%= currency %>">

backend/app/views/spree/admin/variants/_form.html.erb

@@ -63,9 +63,13 @@
 
     <div class="row">
       <div class="col-3">
-        <div class="field" data-hook="price">
-          <%= f.label :price %>
-          <%= render "spree/admin/shared/number_with_currency", f: f, amount_attr: :price, currency: @variant.find_or_build_default_price.currency %>
+        <div class="field">
+          <fieldset>
+            <legend><%= t("spree.helpers.prices.legend") %></legend>
+            <%= f.fields_for :prices do |p_form| %>
+              <%= render "prices_form", f: p_form, currency: p_form.object.currency %>
+            <% end %>
+          </fieldset>
         </div>
       </div>
 

backend/app/views/spree/admin/variants/_prices_form.html.erb

@@ -0,0 +1,4 @@
+<div class="mb-3">
+  <%= render "spree/admin/shared/number_with_currency", f: f, amount_attr: :amount, currency: currency, extra_attrs: { "aria-label" => t("spree.helpers.prices.amount_in", currency: currency) } %>
+  <%= f.hidden_field :currency, value: currency %>
+</div>

backend/spec/features/admin/products/pricing_spec.rb

@@ -70,6 +70,7 @@
       let!(:other_price) { product.master.prices.create(amount: 34.56, currency: "EUR") }
 
       it 'has a working edit page' do
+        product.reload
         within "#spree_price_#{product.master.prices.first.id}" do
           click_icon :edit
         end

backend/spec/features/admin/products/variant_spec.rb

@@ -12,13 +12,15 @@
       product.options.each do |option|
         create(:option_value, option_type: option.option_type)
       end
+      create(:price, amount: 1.75, currency: 'EUR', variant: product.master)
 
       visit spree.admin_path
       click_nav "Products"
       within_row(1) { click_icon :edit }
       click_link "Variants"
       click_on "New Variant"
-      expect(page).to have_field('variant_price', with: "1.99")
+      expect(page).to have_selector('input[value="1.99"][aria-label="Price in USD"]')
+      expect(page).to have_selector('input[value="1.75"][aria-label="Price in EUR"]')
       expect(page).to have_field('variant_cost_price', with: "1.00")
       expect(page).to have_field('variant_weight', with: "2.50")
       expect(page).to have_field('variant_height', with: "3.00")

core/app/models/concerns/spree/default_price.rb

@@ -5,16 +5,28 @@ module DefaultPrice
     extend ActiveSupport::Concern
 
     included do
+      # TODO: Treat as a regular method to avoid in-memory inconsistencies with
+      # `prices`. e.g.:
+      #
+      # ```
+      # Variant.new(price: 25).prices.any? # => false
+      # ```
       has_one :default_price,
         -> { with_discarded.currently_valid.with_default_attributes },
         class_name: 'Spree::Price',
         inverse_of: :variant,
         dependent: :destroy,
         autosave: true
+
+      def self.default_pricing
+        Spree::Config.default_pricing_options.desired_attributes
+      end
     end
 
     def find_or_build_default_price
-      default_price || build_default_price(Spree::Config.default_pricing_options.desired_attributes)
+      default_price ||
+        default_price_from_memory ||
+        build_default_price(self.class.default_pricing)
     end
 
     delegate :display_price, :display_amount, :price, to: :find_or_build_default_price
@@ -23,5 +35,19 @@ def find_or_build_default_price
     def has_default_price?
       default_price.present? && !default_price.discarded?
     end
+
+    private
+
+    # TODO: Remove when {Spree::Price.default_price} is no longer an
+    # association.
+    def default_price_from_memory
+      prices.to_a.filter(&:new_record?).find do |price|
+        price.
+          attributes.
+          values_at(
+            *self.class.default_pricing.keys.map(&:to_s)
+          ) == self.class.default_pricing.values
+      end
+    end
   end
 end

core/app/models/spree/variant.rb

@@ -58,6 +58,13 @@ class Variant < Spree::Base
       inverse_of: :variant,
       autosave: true
 
+    # TODO: Treat as a regular scope to avoid in-memory inconsistencies with
+    # `prices`. e.g.:
+    #
+    # ```
+    # Variant.new.tap { |v| v.currently_valid_prices << Price.new }.prices.any?
+    # # => false
+    # ```
     has_many :currently_valid_prices,
       -> { currently_valid },
       class_name: 'Spree::Price',
@@ -66,7 +73,6 @@ class Variant < Spree::Base
       autosave: true
 
     before_validation :set_cost_currency
-    before_validation :set_price, if: -> { product && product.master }
     before_validation :build_vat_prices, if: -> { rebuild_vat_prices? || new_record? && product }
 
     validates :product, presence: true
@@ -85,6 +91,8 @@ class Variant < Spree::Base
 
     after_destroy :destroy_option_values_variants
 
+    accepts_nested_attributes_for :prices
+
     # Returns variants that are in stock. When stock locations are provided as
     # a parameter, the scope is limited to variants that are in stock in the
     # provided stock locations.
@@ -354,6 +362,30 @@ def gallery
       @gallery ||= Spree::Config.variant_gallery_class.new(self)
     end
 
+    # Creates a shallow copy of every price from the master variant of the
+    # product, and sets them as the variant's prices
+    #
+    # @return [Array<Spree::Price>]
+    # @raise [RuntimeError] if self has already been persisted to the DB
+    def inherit_prices
+      raise 'Prices from master can only be copied over non-persisted variants' if persisted?
+
+      self.prices = product.master.prices.map(&:dup)
+    end
+
+    # TODO: Treat {Spree::Variant.default_price} as a method, and
+    # {currently_valid_prices} as a scope and remove this method.
+    #
+    # Returns whether self is associated to {Spree::Price} through any of their
+    # associations.
+    #
+    # @return [Boolean]
+    def any_price?
+      prices.any? ||
+        currently_valid_prices.any? ||
+        default_price.present?
+    end
+
     private
 
     def rebuild_vat_prices?
@@ -367,11 +399,6 @@ def set_master_out_of_stock
       end
     end
 
-    # Ensures a new variant takes the product master price when price is not supplied
-    def set_price
-      self.price = product.master.price if price.nil? && Spree::Config[:require_master_price] && !is_master?
-    end
-
     def check_price
       if price.nil? && Spree::Config[:require_master_price] && is_master?
         errors.add :price, 'Must supply price for variant or master.price for product.'

core/config/locales/en.yml

@@ -1383,6 +1383,9 @@ en:
       products:
         price_diff_add_html: "(Add: %{amount_html})"
         price_diff_subtract_html: "(Subtract: %{amount_html})"
+      prices:
+        legend: Prices
+        amount_in: "Price in %{currency}"
     hidden: hidden
     hide_out_of_stock: Hide out of stock
     hints:

core/lib/spree/core/importer/product.rb

@@ -18,7 +18,9 @@ def create
           if product.save
             variants_attrs.each do |variant_attribute|
               # make sure the product is assigned before the options=
-              product.variants.create({ product: product }.merge(variant_attribute))
+              variant = product.variants.new({ product: product }.merge(variant_attribute))
+              variant.inherit_prices unless variant.prices.any? || variant.persisted?
+              variant.save
             end
 
             set_up_options

core/lib/spree/core/product_duplicator.rb

@@ -54,6 +54,7 @@ def duplicate_variant(variant)
       new_variant.sku = "COPY OF #{new_variant.sku}"
       new_variant.deleted_at = nil
       new_variant.option_values = variant.option_values.map { |option_value| option_value }
+      new_variant.prices = variant.prices.map(&:dup)
       new_variant
     end
 

core/lib/spree/permitted_attributes.rb

@@ -126,7 +126,8 @@ module PermittedAttributes
       :name, :presentation, :cost_price, :lock_version,
       :position, :track_inventory,
       :product_id, :product, :option_values_attributes, :price,
-      :weight, :height, :width, :depth, :sku, :cost_currency, option_value_ids: [], options: [:name, :value]
+      :weight, :height, :width, :depth, :sku, :cost_currency, option_value_ids: [], options: [:name, :value],
+      prices_attributes: [:id, :amount, :currency]
     ]
 
     @@checkout_address_attributes = [

core/spec/models/spree/product_duplicator_spec.rb

@@ -79,11 +79,15 @@ module Spree
       let!(:variant1) { create(:variant, product: product, option_values: [option_value1]) }
       let!(:variant2) { create(:variant, product: product, option_values: [option_value2]) }
 
+      # will change the count by 3, since there will be a master variant as well
       it "will duplciate the variants" do
-        # will change the count by 3, since there will be a master variant as well
         expect{ duplicator.duplicate }.to change{ Spree::Variant.count }.by(3)
       end
 
+      it "will duplciate the prices" do
+        expect{ duplicator.duplicate }.to change{ Spree::Price.count }.by(3)
+      end
+
       it "will not duplicate the option values" do
         expect{ duplicator.duplicate }.to change{ Spree::OptionValue.count }.by(0)
       end