Allow customer returns to reference existing ReturnItems on create through API

[forkata]

Description
This change fixes an issue in the API customer returns controller create action when referencing existing return items from a return authorization. This works on update but Rails has a limitation where it cannot set the customer_return_id on the ReturnItem records when they are passed as nested attributes, because that record doesn't exist. This will fail with an ActiveRecord::RecordNotFound error. This behaviour is documented, though slightly buried deep in the documentation here.

We already allow this in the CustomerReturnsController in the backend, however it requires a bit of parameter mangling to get the exact behaviour we want. In this change we have pulled the build_return_items_from_params before action from the backend controller and modified it to better fit the API use case.

The main changes required to the before_action were to

• remove the code to exclude items with the returned flag, which is specific to the backend form.
• allowed the nested return_items_attributes to be passed as a hash of hashes (default for nested forms) as well as an Array. I've also added a deprecation when providing these as a hash of hashes.
• remove the custom errors the backend adds when a reception_status_event is not provided, this is not required by the API controller, but required in the backend.

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)
• I have attached screenshots to this PR for visual changes (if needed)

[kennyadsl]

@forkata Thanks for the PR!

For anyone else reviewing, here's a handy link to the backend counterpart for this feature.

remove the code to exclude items with the returned flag, which is specific to the backend form

👍

remove support for passing the return_items_attributes as a hash of hashes (default for nested forms) and instead expects that field to be an Array. We could add this back as it may be a breaking change if anyone is passing these attributes in that way through the API which the create call on an ActiveRecord object should handle. I don't think this is a documented feature in our API docs as the type is specified as array here).

Do you think there's a way to deprecate passing the hash, just to be sure we are not breaking existing applications? I'm afraid that a lot of people are looking at the source code to understand how to use our APIs, also because at the moment the doc is not super reliable as you saw.

add custom errors to the CustomerReturn and fail the create call if a reception_status_event is not provided in each the return_items_attributes. This is the behaviour in the backend controller, but I now realize reception_status_event is not required in our API docs 🤔, so I am happy to remove this change or update the docs if the field is actually required.

I'd avoid adding this validation, maybe just because I don't understand why a reception_status_event is needed. I'm open to change my mind if having a record built that way can create data inconsistencies. Did you get that?

[forkata]

Thank you for your quick feedback @kennyadsl!

remove support for passing the return_items_attributes as a hash of hashes (default for nested forms) and instead expects that field to be an Array. We could add this back as it may be a breaking change if anyone is passing these attributes in that way through the API which the create call on an ActiveRecord object should handle. I don't think this is a documented feature in our API docs as the type is specified as array here).

Do you think there's a way to deprecate passing the hash, just to be sure we are not breaking existing applications? I'm afraid that a lot of people are looking at the source code to understand how to use our APIs, also because at the moment the doc is not super reliable as you saw.

That is a great suggestion! I was contemplating handling both formats to ensure backwards compatibility. What I can do is add a change and corresponding tests to handle return_items_attributes as a hash of hashes and add a deprecation warning if we go down that code path.

add custom errors to the CustomerReturn and fail the create call if a reception_status_event is not provided in each the return_items_attributes. This is the behaviour in the backend controller, but I now realize reception_status_event is not required in our API docs 🤔, so I am happy to remove this change or update the docs if the field is actually required.

I'd avoid adding this validation, maybe just because I don't understand why a reception_status_event is needed. I'm open to change my mind if having a record built that way can create data inconsistencies. Did you get that?

I was also unsure if this was a backend specific requirement, but assumed it was not because of this being a feature of the state machine which is defined in the model. I am going to do some more testing on this and remove if it isn't required to create or update the CustomerReturn record or when creating new return items through this call. It would be also good to have this behaviour tested in the specs for this controller so I will try to add to that.

[forkata]

Hi @kennyadsl,
After some more testing here I've updated the PR and it should be now ready for review. I made the following changes based on our conversation

• the reception_status_event field appears to be optional and we can create the CR without providing that. I've added a test to show usage with and without specifying that in the payload and removed the custom error when this field was omitted.
• i've added a test for passing return_items_attributes as a hash of hashes and ensured my change is backwards compatible for this, as well as added a deprecation warning in case this code path is reached. It is relatively simple to support both, but I feel like this was undocumented and probably not something we should support through the API.

[kennyadsl]

Thanks! Going to merge this after we release 3.0 (this will be present in 3.1).

[spaghetticode]

@forkata thanks for this fix, I really appreciated being guided in the review by the very detailed commits ❤️

I left a couple of comments, let me know what you think!

[spaghetticode]

I'm not sure why we need compact after the map cycle, I suppose we're always returning a Spree::ReturnItem with each iteration, and when a return item cannot be found from item_params[:id] an exception is raised... but maybe I'm overlooking some scenario?

Also, I think the naming of the method reflects only part of what this method does here. What about a more general build_customer_return, since it's actually building the instance stored in@customer_return?

[forkata]

I'm not sure why we need compact after the map cycle

I agree, upon closer inspection I don't think the .compact is necessary here.

I think the naming of the method reflects only part of what this method does here.

That's a great point, I think build_customer_return is a better name here, I will update that!

Thanks for the feedback, I will update this change to reflect the above suggestions!

[forkata]

@spaghetticode I've addressed your comments here and this should be ready for another review 🙏🏼

[spaghetticode]

@forkata thank you, LGTM 👍