Permitted attributes improvements

api/app/controllers/spree/api/base_controller.rb

@@ -11,9 +11,6 @@ class BaseController < ActionController::Base
       include Spree::Core::ControllerHelpers::Pricing
       include Spree::Core::ControllerHelpers::StrongParameters
 
-      class_attribute :admin_line_item_attributes
-      self.admin_line_item_attributes = [:price, :variant_id, :sku]
-
       attr_accessor :current_api_user
 
       class_attribute :error_notifier
@@ -34,11 +31,7 @@ class BaseController < ActionController::Base
 
       # users should be able to set price when importing orders via api
       def permitted_line_item_attributes
-        if can?(:admin, Spree::LineItem)
-          super + admin_line_item_attributes
-        else
-          super
-        end
+        can?(:admin, Spree::LineItem) ? permitted_admin_line_item_attributes : super
       end
 
       def load_user

api/app/controllers/spree/api/line_items_controller.rb

@@ -1,10 +1,6 @@
 module Spree
   module Api
     class LineItemsController < Spree::Api::BaseController
-      class_attribute :line_item_options
-
-      self.line_item_options = []
-
       before_filter :load_order, only: [:create, :update, :destroy]
       around_filter :lock_order, only: [:create, :update, :destroy]
 
@@ -66,11 +62,7 @@ def line_items_attributes
       end
 
       def line_item_params
-        params.require(:line_item).permit(
-          :quantity,
-            :variant_id,
-            options: line_item_options
-        )
+        params.require(:line_item).permit(permitted_line_item_attributes)
       end
     end
   end

api/app/controllers/spree/api/orders_controller.rb

@@ -1,12 +1,6 @@
 module Spree
   module Api
     class OrdersController < Spree::Api::BaseController
-      class_attribute :admin_shipment_attributes
-      self.admin_shipment_attributes = [:shipping_method, :stock_location, inventory_units: [:variant_id, :sku]]
-
-      class_attribute :admin_order_attributes
-      self.admin_order_attributes = [:import, :number, :completed_at, :locked_at, :channel, :user_id, :created_at]
-
       skip_before_action :authenticate_user, only: :apply_coupon_code
 
       before_action :find_order, except: [:create, :mine, :current, :index]
@@ -119,15 +113,11 @@ def determine_order_user
       end
 
       def permitted_order_attributes
-        can?(:admin, Spree::Order) ? (super + admin_order_attributes) : super
+        can?(:admin, Spree::Order) ? permitted_admin_order_attributes : super
       end
 
       def permitted_shipment_attributes
-        if can?(:admin, Spree::Shipment)
-          super + admin_shipment_attributes
-        else
-          super
-        end
+        can?(:admin, Spree::Shipment) ? permitted_admin_shipment_attributes : super
       end
 
       def find_order(_lock = false)

api/spec/controllers/spree/api/line_items_controller_spec.rb

@@ -2,16 +2,13 @@
 
 module Spree
   PermittedAttributes.module_eval do
-    mattr_writer :line_item_attributes
+    mattr_writer :line_item_option_attributes
   end
 
-  unless PermittedAttributes.line_item_attributes.include? :some_option
-    PermittedAttributes.line_item_attributes += [:some_option]
+  unless PermittedAttributes.line_item_option_attributes.include? :some_option
+    PermittedAttributes.line_item_option_attributes += [:some_option]
   end
 
-  # This should go in an initializer
-  Spree::Api::LineItemsController.line_item_options += [:some_option]
-
   describe Api::LineItemsController, type: :controller do
     render_views
 

api/spec/controllers/spree/api/orders_controller_spec.rb

@@ -2,6 +2,14 @@
 require 'spree/testing_support/bar_ability'
 
 module Spree
+  PermittedAttributes.module_eval do
+    mattr_writer :line_item_option_attributes
+  end
+
+  unless PermittedAttributes.line_item_option_attributes.include? :some_option
+    PermittedAttributes.line_item_option_attributes += [:some_option]
+  end
+
   describe Api::OrdersController, type: :controller do
     render_views
     let!(:order) { create(:order) }
@@ -121,7 +129,7 @@ module Spree
         end
 
         it "updates the otherwise forbidden attributes" do
-          expect{ subject }.to change{ order.reload.number }.
+          expect { subject }.to change { order.reload.number }.
             to("anothernumber")
         end
       end
@@ -133,7 +141,7 @@ module Spree
         end
 
         it "does not change forbidden attributes" do
-          expect{ subject }.to_not change{ order.reload.number }
+          expect { subject }.to_not change { order.reload.number }
         end
       end
     end
@@ -353,17 +361,12 @@ module Spree
     end
 
     # Regression test for https://github.com/spree/spree/issues/3404
-    it "can specify additional parameters for a line item" do
-      expect(Order).to receive(:create!).and_return(order = Spree::Order.new)
-      allow(order).to receive(:associate_user!)
-      allow(order).to receive_message_chain(:contents, :add).and_return(line_item = double('LineItem'))
-      expect(line_item).to receive(:update_attributes!).with("special" => true)
-
-      allow(controller).to receive_messages(permitted_line_item_attributes: [:id, :variant_id, :quantity, :special])
+    it "can specify additional parameters via options for a line item" do
+      expect_any_instance_of(LineItem).to receive(:some_option=).with(4)
       api_post :create, order: {
         line_items: {
           "0" => {
-            variant_id: variant.to_param, quantity: 5, special: true
+            variant_id: variant.to_param, quantity: 5, options: { some_option: 4 }
           }
         }
       }
@@ -374,7 +377,7 @@ module Spree
       api_post :create, order: {
         line_items: {
           "0" => {
-            price: 33.0, variant_id: variant.to_param, quantity: 5
+            variant_id: variant.to_param, quantity: 5, options: { price: 33.0 }
           }
         }
       }
@@ -407,13 +410,13 @@ module Spree
       let(:address_params) { { country_id: country.id } }
       let(:billing_address) {
         { firstname: "Tiago", lastname: "Motta", address1: "Av Paulista",
-                                city: "Sao Paulo", zipcode: "01310-300", phone: "12345678",
-                                country_id: country.id }
+          city: "Sao Paulo", zipcode: "01310-300", phone: "12345678",
+          country_id: country.id }
       }
       let(:shipping_address) {
         { firstname: "Tiago", lastname: "Motta", address1: "Av Paulista",
-                                 city: "Sao Paulo", zipcode: "01310-300", phone: "12345678",
-                                 country_id: country.id }
+          city: "Sao Paulo", zipcode: "01310-300", phone: "12345678",
+          country_id: country.id }
       }
       let(:country) { create(:country, { name: "Brazil", iso_name: "BRAZIL", iso: "BR", iso3: "BRA", numcode: 76 }) }
 
@@ -450,7 +453,7 @@ module Spree
       it "cannot change the price of an existing line item" do
         api_put :update, id: order.to_param, order: {
           line_items: {
-            0 => { id: line_item.id, price: 0 }
+            0 => { id: line_item.id, options: { price: 0 } }
           }
         }
 
@@ -728,7 +731,7 @@ module Spree
           api_post :create, order: {
             line_items: {
               "0" => {
-                price: 33.0, variant_id: variant.to_param, quantity: 5
+                variant_id: variant.to_param, quantity: 5, options: { price: 33.0 }
               }
             }
           }

core/app/models/spree/order_contents.rb

@@ -86,7 +86,7 @@ def add_to_line_item(variant, quantity, options = {})
       )
 
       line_item.quantity += quantity.to_i
-      line_item.options = ActionController::Parameters.new(options).permit(PermittedAttributes.line_item_attributes)
+      line_item.options = options.except(:stock_location_quantities, :shipment)
 
       if line_item.new_record?
         create_order_stock_locations(line_item, options[:stock_location_quantities])

core/lib/spree/core/controller_helpers/strong_parameters.rb

@@ -2,34 +2,58 @@ module Spree
   module Core
     module ControllerHelpers
       module StrongParameters
-        def permitted_attributes
+        def base_attributes
           Spree::PermittedAttributes
         end
 
         delegate(*Spree::PermittedAttributes::ATTRIBUTES,
-                 to: :permitted_attributes,
+                 to: :base_attributes,
                  prefix: :permitted)
 
+        def admin_attributes
+          Spree::PermittedAttributes::Admin
+        end
+
+        delegate(*Spree::PermittedAttributes::Admin::ATTRIBUTES,
+                 to: :admin_attributes,
+                 prefix: :permitted_admin)
+
         def permitted_credit_card_update_attributes
-          permitted_attributes.credit_card_update_attributes + [
+          base_attributes.credit_card_update_attributes + [
             address_attributes: permitted_address_attributes
           ]
         end
 
+        def permitted_line_item_attributes
+          base_attributes.line_item_attributes + [
+            options: base_attributes.line_item_option_attributes
+          ]
+        end
+
+        def permitted_admin_line_item_attributes
+          base_attributes.line_item_attributes + admin_attributes.line_item_attributes + [
+<<<<<<< HEAD
+            options: base_attributes.line_item_option_attributes + admin_attributes.line_item_option_attributes
+=======
+            options: base_attributes.line_item_option_attributes +  admin_attributes.line_item_option_attributes
+>>>>>>> 3862505... Simplify order_contents line_item add
+          ]
+        end
+
         def permitted_payment_attributes
-          permitted_attributes.payment_attributes + [
+          base_attributes.payment_attributes + [
             source_attributes: permitted_source_attributes
           ]
         end
 
         def permitted_source_attributes
-          permitted_attributes.source_attributes + [
+          base_attributes.source_attributes + [
             address_attributes: permitted_address_attributes
           ]
         end
 
         def permitted_checkout_attributes
-          permitted_attributes.checkout_attributes + [
+          base_attributes.checkout_attributes + [
             bill_address_attributes: permitted_address_attributes,
             ship_address_attributes: permitted_address_attributes,
             payments_attributes: permitted_payment_attributes,
@@ -43,14 +67,20 @@ def permitted_order_attributes
           ]
         end
 
+        def permitted_admin_order_attributes
+          permitted_checkout_attributes + admin_attributes.order_attributes + [
+            line_items_attributes: permitted_admin_line_item_attributes
+          ]
+        end
+
         def permitted_product_attributes
-          permitted_attributes.product_attributes + [
+          base_attributes.product_attributes + [
             product_properties_attributes: permitted_product_properties_attributes
           ]
         end
 
         def permitted_user_attributes
-          permitted_attributes.user_attributes + [
+          base_attributes.user_attributes + [
             bill_address_attributes: permitted_address_attributes,
             ship_address_attributes: permitted_address_attributes
           ]

core/lib/spree/core/importer/order.rb

@@ -97,15 +97,8 @@ def self.create_shipments_from_params(shipments_hash, order)
         def self.create_line_items_from_params(line_items_hash, order)
           return {} unless line_items_hash
           line_items_hash.each_key do |k|
-            extra_params = line_items_hash[k].except(:variant_id, :quantity, :sku)
             line_item = ensure_variant_id_from_params(line_items_hash[k])
-            line_item = order.contents.add(Spree::Variant.find(line_item[:variant_id]), line_item[:quantity])
-            # Raise any errors with saving to prevent import succeeding with line items failing silently.
-            if extra_params.present?
-              line_item.update_attributes!(extra_params)
-            else
-              line_item.save!
-            end
+            order.contents.add(Spree::Variant.find(line_item[:variant_id]), line_item[:quantity], line_item[:options] || {})
           end
         end
 

core/lib/spree/permitted_attributes.rb

@@ -12,6 +12,7 @@ module PermittedAttributes
       :image_attributes,
       :inventory_unit_attributes,
       :line_item_attributes,
+      :line_item_option_attributes,
       :option_type_attributes,
       :option_value_attributes,
       :payment_attributes,
@@ -60,6 +61,8 @@ module PermittedAttributes
 
     @@line_item_attributes = [:id, :variant_id, :quantity]
 
+    @@line_item_option_attributes = []
+
     @@option_type_attributes = [:name, :presentation, :option_values_attributes]
 
     @@option_value_attributes = [:name, :presentation]
@@ -101,8 +104,9 @@ module PermittedAttributes
     @@stock_movement_attributes = [
       :quantity, :stock_item, :stock_item_id, :originator, :action]
 
-    @@store_attributes = [:name, :url, :seo_title, :meta_keywords,
-                          :meta_description, :default_currency, :mail_from_address]
+    @@store_attributes = [
+      :name, :url, :seo_title, :meta_keywords,
+      :meta_description, :default_currency, :mail_from_address]
 
     @@taxonomy_attributes = [:name]
 
@@ -125,3 +129,5 @@ module PermittedAttributes
       :weight, :height, :width, :depth, :sku, :cost_currency, option_value_ids: [], options: [:name, :value]]
   end
 end
+
+require 'spree/permitted_attributes/admin'

core/lib/spree/permitted_attributes/admin.rb

@@ -0,0 +1,24 @@
+module Spree
+  module PermittedAttributes
+    module Admin
+      ATTRIBUTES = [
+        :line_item_attributes,
+        :line_item_option_attributes,
+        :order_attributes,
+        :shipment_attributes
+      ]
+
+      mattr_reader(*ATTRIBUTES)
+
+      @@line_item_attributes = [:sku]
+
+      @@line_item_option_attributes = [:price]
+
+      @@line_item_option_attributes = []
+
+      @@order_attributes = [:import, :number, :completed_at, :locked_at, :channel, :user_id, :created_at]
+
+      @@shipment_attributes = [:shipping_method, :stock_location, inventory_units: [:variant_id, :sku]] + PermittedAttributes.shipment_attributes
+    end
+  end
+end