Improve sample data for the returned/reimbursed order #3495
Conversation
kennyadsl
force-pushed
the
kennyadsl/improve-sample-returns
branch
from
1399e21 to
8127dc3
Compare
There was a problem hiding this comment.
@kennyadsl 👍 thanks for the improvement, I left a non-blocking suggestion
sample/db/samples/reimbursements.rb
Outdated
| customer_return = Spree::CustomerReturn.create( | ||
| stock_location: stock_location, | ||
| return_items: [return_item] | ||
| order.return_authorizations.create( |
There was a problem hiding this comment.
If a new validation is added to the model then #create may fail silently, so what about using #create!?
There was a problem hiding this comment.
Done, and added the ! to some other methods as well, thanks!
The current code is not creating a valid reimbursement since it's created when the associated order is still not in the riht state (paid and shipped). The amount of the reimbursement was also wrong, since the return item created had an exchange variant associated, making the total of the calculated reimboursement 0. This was mainly used to allow seeing the reimbursement email in the preview (with solidusio#1146), and it was working fine for that purpose but it is creating an inconsistent order in the backend running samples. Retruns are already complex and messed up, we need to avoid showing wrong examples of code and order states to users.
The sample order's reimbursement is created correctly but `perform!` is never called on it. This means that the instance of Spree::Reimbursement is persisted, but no action is taken on the reimbursment for this return (no payment refund is made). This commit change how we create the reimbursement, the provided method #build_from_customer_return already accept return items, create the reimbursement taking what is needed by the customer return instance and also perform the acutal refund. To perform a reimbursement we need to pass the user that is taking this action. When solidus_auth_devise is there we can assume we have an admin user, at least this is what happens in the sandbox envirnoment. When it's not used we can just try to fetch/create a dummy user.
kennyadsl
force-pushed
the
kennyadsl/improve-sample-returns
branch
from
8127dc3 to
7686078
Compare
| order = Spree::Order.last | ||
| inventory_unit = order.inventory_units.first | ||
| order = Spree::Order.last | ||
| inventory_unit = order.inventory_units.take! |
There was a problem hiding this comment.
Hey @kennyadsl I think we're not handling exceptions when this takes! method fails, should we still use the bang method?
There was a problem hiding this comment.
Ah nevermind, I just saw spaghetticode's comment and that solved my doubt here
| stock_location = inventory_unit.find_stock_item.stock_location | ||
| return_reason = Spree::ReturnReason.active.take! | ||
| preferred_reimbursement_type = Spree::ReimbursementType.where(name: 'Original').take! |
There was a problem hiding this comment.
As far as I remember the take method expects the collection to contain at least one element, this will probably break if we don't have any Spree::ReimbursementType in the database
| customer_return: customer_return, | ||
| return_items: [return_item] | ||
| ) | ||
| reimbursement = Spree::Reimbursement.build_from_customer_return(customer_return) |
There was a problem hiding this comment.
Thanks for the commit description! That made me know what the build_from_customer_return method does without having to do a lot of research
| admin_user = if defined?(Spree::Auth) | ||
| Spree.user_class.admin.take! | ||
| else | ||
| Spree.user_class.find_or_create_by!(email: '[email protected]') | ||
| end |
There was a problem hiding this comment.
This is a clever way to set an user, but if we need an actual admin user, shouldn't we create it with more information than just the email?
There was a problem hiding this comment.
This will be executed when Auth::Devise is not present, so it will create a LegacyUser which does not need any more information to be valid.
Description
In our sample data script, we are creating two orders. One of them is also returned and refunded in order to be able to see the reimbursement mailer preview, which requires an actual reimbursement in the database to work.
The order is set in a way that allows the email preview to work but it's actually a bit messed up. In a real UX flow, it's not possible to have the order in that state, so I tried to adjust it to be as similar as possible with how a real refunded order looks like.
This also improves the documentation since the sample code flow used can be a guideline to understand how the complex returns flow works.
Ref: #1146
Checklist:
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)