Confusing parameter usage in Spree::Api::StockItemsController

[stewart]

The usage of parameters in Spree::Api::StockItemsController is strange and hard to follow.

More specifically, the count_on_hand and force parameters are both expected to be set on params[:stock_item], but neither are permitted attributes. This results in confusing logic, including the following:

• #create relies on stock_item[count_on_hand] being nil (as it is an unpermitted attribute) and coercing to 0 as a result
• #update directly manipulates and deletes the stock_item[count_on_hand] and stock_item[force] attributes
• #stock_item_params is impure, prematurely deleting the stock_item[force] attribute if called early

I'm unsure if these issues can be addressed while maintaining the existing parameter API, or if it's worth breaking compatibility of these endpoints for better logic and a cleaner external interface, but this code as it stands is confusing, unclear, and just doesn't feel good.

[jarednorman]

I'm going to quote @stewart's comment but rewrite the links to point to a commit (5ed1162) that was likely what was on master at the time this issue was opened.

This is so that I and anyone trying to understand this issue can make sense of what it is referring to and to help us validate how much of this is still relevant.

@stewart's comment with the links rewritten:

The usage of parameters in Spree::Api::StockItemsController is strange and hard to follow.

More specifically, the count_on_hand and force parameters are both expected to be set on params[:stock_item], but neither are permitted attributes. This results in confusing logic, including the following:

• #create relies on stock_item[count_on_hand] being nil (as it is an unpermitted attribute) and coercing to 0 as a result
• #update directly manipulates and deletes the stock_item[count_on_hand] and stock_item[force] attributes
• #stock_item_params is impure, prematurely deleting the stock_item[force] attribute if called early

I'm unsure if these issues can be addressed while maintaining the existing parameter API, or if it's worth breaking compatibility of these endpoints for better logic and a cleaner external interface, but this code as it stands is confusing, unclear, and just doesn't feel good.

These issues now link to lines that make sense to me. I haven't yet evaluated how much of this is relevant to the current state of this API controller yet, though.