-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add API endpoint for customer returns #3579
Add API endpoint for customer returns #3579
Conversation
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
629af7a
to
c0c9b9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's great @seand7565, thanks! And also thanks @jarednorman!
I think we need to squash these two commits and add some API documentation as well. Feel free to ask if you need help with that.
c0c9b9c
to
349b45f
Compare
@kennyadsl Squashed & documented! I added to the stoplight documentation, let me know how it looks. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sean!
Thanks @seand7565 for the PR. In admin workflow, in order to create a Until I understand, that is because you have to accept or not the return, and setup the And after generate the So, I think we should receive a Also, we should need to generate a |
Hey @ccarruitero, thanks for the review! On requiring RMAs - I tried to model the API the same way the backend works. Currently, an RMA is NOT required for creating a Customer Return. You can create a customer return with no RMA, and it will create the customer return successfully. However, it will generate an “acceptance error” of While I don’t like the way this is handled (there are a lot of Return Item statuses that don’t make sense with RMAs like “given to customer” or “lost in transit” - you wouldn’t be expecting a return in those situations but might still want a reimbursement), changing the functionality of how this works is another issue - one that I might tackle very shortly, but outside of the scope of this PR. Reimbursements AFAIK are handled by return_item state changes - setting the reception status of the customer return should trigger the reimbursement handler. I’ll take a look at your comments on the code a bit later - thanks for the input! |
Thanks @seand7565 and @ccarruitero for the review! The "RMA required" part is a bit tricky. By default, you need to have a RMA to create valid Return Items and Customer Returns. This depends on a specific validator set here that can be changed depending on the needs. For now, I'll keep it as in admin even if it's not ideal. |
349b45f
to
e1dad17
Compare
e1dad17
to
5f1d4c1
Compare
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
api/spec/requests/spree/api/customer_returns_controller_spec.rb
Outdated
Show resolved
Hide resolved
5f1d4c1
to
90c4571
Compare
THIS PR SPONSORED BY Super Good Software
Description
Adds an API endpoint that allows you to manage customer returns. Also adds a test suite for this new API endpoint, adds associated jbuilder files, adds customer_return.number to ransackable attributes, and creates a connection between order and customer_return for easier referencing.
Just like return_items, when you create the customer_return you'll also need to add attributes for the return_items associated with the return.
Quick note: The way I'm referencing customer returns from the order is a bit convoluted, because there's no order_id on customer_returns. I think there should be, as it would simplify this a bit (and it makes sense that customer returns would be connected to the order like return authorizations are) - but it felt like it was outside of the scope of this PR.
Ref #3566
Checklist: