Fix Spree::Api:LineItemsController#create handling of validation errors

[RyanofWoods]

Spree::Api:LineItemsController#create calls #add on an OrderContents
instance which eventually call #save!. This raises errors if validation
fails. #create was not handling these validation errors. In fact, it had
an if statement checking for errors on a LineItem, but this could never
be returned if errors were raised.

---

I can include changes to the documentation if the code looks good 👍

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)

[spaghetticode]

@RyanofWoods thanks for this PR! I think it makes sense, although I would like to discuss some doubts that come to my mind.

The first one is about the fact that the focus changes from the line item to the order. Also, I'm wondering if we should add another action (i.e. batch_create) for creating multiple line items and leave this one for a single one.

Then, I wonder if changing the implementation of OrderContents#add to call save instead of save! could fix the issue here without much change to the controller action. I don't see the failure case tested on OrderContents specs, so I guess we're underspecifying the behavior and there may be an opportunity to fix that. WDYT?

[RyanofWoods]

Hi @spaghetticode, thanks for the review! 🙂

Having another action for adding multiple items would probably be best 👍 If a method gets added to the OrderContents class then the API and Frontend can both utilize it.

Having the OrdersContent use #save instead of #save! would allow more simple logic when adding a variant, instead of having to catch the error and then insert it back in like here. If the change gets done for #add_to_line_item then I think we should do it here for #remove_from_line_item to have consistency.

Do you think there's more of a risk of developers not handling the errors if it doesn't raise? Or it is something we shouldn't worry about.

Note, this change would mean updating the frontend code, however, I currently have a PR open to move this logic out 🤔 #4102

[spaghetticode]

@RyanofWoods thanks for the detailed explanation ❤️

Well, given that the frontend is leveraging the current behavior I think that changing the way OrderContents#add works becomes less desirable, dealing with the side effects there would complicate things, also considering that AFAIK we tend to limit breaking changes on the frontend.

On the other hand, I think it's still possible to avoid answering with the order in favor of the line item (thus keeping the existing interface also in case of error) with something like this:

def create
  variant = Spree::Variant.find(params[:line_item][:variant_id])

  @line_item = begin
    @order.contents.add(
      variant,
      params[:line_item][:quantity] || 1,
      options: line_item_params[:options].to_h
    )
  rescue ActiveRecord::RecordInvalid => error
    error.record
  end

  if @line_item.persisted?
    respond_with(@line_item, status: 201, default_template: :show)
  else
    invalid_resource!(@line_item)
  end
end

Disclaimer: I didn't try this code thoroughly, so there may be something I'm overlooking

[RyanofWoods]

Hi @spaghetticode. Sorry for the very late response. I have been thinking about this more deeply.

I am thinking that in the long-term it would be more preferable and be more in line with Solidus to have #save rather than #save!. Do you agree?

If so, perhaps there's a way to achieve this change by using the solidus_starter_frontend.

[spaghetticode]

@RyanofWoods yes, in general I think using #save would be better so we don't have to resort to error handling, but it would mean much more work, and I'm not sure I'm getting your reference to the solidus_starter_frontend. Also, if we decide to go that way, I think it would be better done in another PR so we don't broaden the scope of this fix too much.

[waiting-for-dev]

The idea is to deprecate solidus_frontend eventually, so maybe we should not worry too much about it. However, changing #add to return false would be a breaking change in itself, so we should manage it accordingly.

I agree that it'd be best to keep the interface for the current end-point and create a new one for the batch action.

[waiting-for-dev]

BTW, we should make sure that the raising on #add is not used to abort a transaction somewhere else.

[waiting-for-dev]

Hey, @RyanofWoods! Would you still like to work on it? As a minimal change, we could fix rendering the validation errors to the end-user instead of raising them.

[waiting-for-dev]

Hey, @RyanofWoods, pinging you again, in case you'd like to complete it 🙂

[RyanofWoods]

I will look into this again soon (friday or after my two weeks vacation) @waiting-for-dev

[codecov]

Codecov Report

Merging #4177 (5946a34) into main (afa4d89) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #4177   +/-   ##
=======================================
  Coverage   88.67%   88.67%           
=======================================
  Files         564      564           
  Lines       13894    13894           
=======================================
+ Hits        12320    12321    +1     
+ Misses       1574     1573    -1     

Impacted Files	Coverage Δ
...app/controllers/spree/api/line_items_controller.rb	96.96% <100.00%> (+3.03%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[waiting-for-dev]

I left a suggestion about the test, but the application code is perfect 👍

[RyanofWoods]

Rebased

[rainerdema]

👍

[elia]

Good catch! 🙌

[elia]

I'm wondering ig this could/should be turned into a rescue_from in the API::BaseController