Solidus: Core: OrderContents#add quantity parameter not set to default when passed nil

[vfonic]

Steps to reproduce

I had code similar to orders_controller#populate:

# Adds a new item to the order (creating a new order if none already exists)
    def populate
      @order   = current_order(create_order_if_necessary: true)
      variant  = Spree::Variant.find(params[:variant_id])
      quantity = params[:quantity].to_i

      # 2,147,483,647 is crazy. See issue https://github.com/spree/spree/issues/2695.
      if !quantity.between?(1, 2_147_483_647)
        @order.errors.add(:base, Spree.t(:please_enter_reasonable_quantity))
      end

      begin
        @line_item = @order.contents.add(variant, quantity) # this line
      rescue ActiveRecord::RecordInvalid => e
        @order.errors.add(:base, e.record.errors.full_messages.join(", "))
      end

      respond_with(@order) do |format|
        format.html do
          if @order.errors.any?
            flash[:error] = @order.errors.full_messages.join(", ")
            redirect_back_or_default(spree.root_path)
            return
          else
            redirect_to cart_path
          end
        end
      end
    end

The difference is that:

1. I don't have reasonable quantity check (I think that's unnecessary code)
2. I have this line slightly different:
   Solidus core code:

@line_item = @order.contents.add(variant, quantity)

Mine:

@line_item = @order.contents.add(variant, params[:quantity])

I looked at OrderContents and saw the default parameter value for quantity to be one and assumed that it would work with these params only:

Parameters: {"variant_id"=>"2"}

Expected behavior

The quantity defaults to 1 when it's: nil or an empty string

Actual behavior

The quantity defaults to 0.
The result of this is a new line_item added to order with a quantity of 0.

System configuration

Solidus Version:
master: 4cfd1a2b64482329f2e36452b4b2470659b3c8a9

I tried to resolve this issue and ended up overcomplicating things. Should we just do something like this:

    def add(variant, quantity = 1, options = {})
      quantity = quantity.to_i
      quantity = 1 if quantity <= 0
      line_item = add_to_line_item(variant, , options)
      after_add_or_remove(line_item, options)
    end

    def remove(variant, quantity = 1, options = {})
      quantity = quantity.to_i
      quantity = 1 if quantity <= 0
      line_item = remove_from_line_item(variant, quantity.to_i, options)
      after_add_or_remove(line_item, options)
    end

Or, what I was doing was:

• Create a new method on LineItem called #adjust_quantity! and then take care of line_item.quantity inside of LineItem class.

I've created a WIP PR on my solidus fork here:
https://github.com/vfonic/solidus/pull/1

[mtomov]

Maybe in the #populate, one can just do

quantity = params[:quantity].present? ? params[:quantity].to_i : 1

I'd avoid going for anything too involved. I think it is the responsibility of the controller to filter out the bad data.

[vfonic]

Since solidus' users are developers and there's no documentation of solidus...at all, I think the filtering of the bad data should happen at any user-entry point aka developer API aka any public method. In this case, that would be the OrderContents. Whether it would be a default of 1 or a validation error, that's another discussion, but it should not be possible to create invalid OrderContents (that would be OrderContents with quantity of zero.