Always pass the new order number on unreturned exchange failures #388
Conversation
Currently, anything that's not specifically a `ChargeFailure` will not include the new order number. That's a pity, because something like an out of stock error shows up as an exception that is not a `ChargeFailure` and then it's really hard to even find the new error. This always makes the new order available to the outside so that we don't have to specifically read it from the exception.
| @@ -53,12 +53,12 @@ def charge_for_items | |||
|
|
|||
| end | |||
|
|
|||
| private | |||
|
|
|||
| def new_order | |||
There was a problem hiding this comment.
I feel like this should get its own special unit test now lol
|
Jordan, I think you are right. Updated. |
| @@ -19,6 +19,8 @@ def initialize(shipment, return_items) | |||
| end | |||
|
|
|||
| def charge_for_items | |||
| self.new_order = Spree::Order.create!(exchange_order_attributes) | |||
There was a problem hiding this comment.
I would just do @new_order = ..., but this is a cool style!
|
looks okay |
|
👍 |
| @@ -20,12 +20,13 @@ namespace :exchanges do | |||
| begin | |||
| item_charger.charge_for_items | |||
| rescue Spree::UnreturnedItemCharger::ChargeFailure => e | |||
| failure = {message: e.message, new_order: e.new_order.try(:number)} | |||
| failure = { message: e.message } | |||
| rescue Exception => e | |||
There was a problem hiding this comment.
While you're here, if you could get rid of this, I'd really appreciate it.
Instead of
rescue Exception => e
just
rescue => e
Rescuing Exception will rescue syntax errors, signal exceptions and such. Things that you shouldn't be rescuing unless you're re-raising them.
There was a problem hiding this comment.
Yeah good point; done.
|
@jhawthorn good to merge? |
|
lgtm 👍 |
| failure = {message: e.message, new_order: e.new_order.try(:number)} | ||
| rescue Exception => e | ||
| failure = {message: "#{e.class}: #{e.message}"} | ||
| failure = { message: e.message } |
There was a problem hiding this comment.
We can now just collapse this into the rescue => e right?
|
I would have prefered we return new_order from Are we just waiting on the re-raise of the exception to finish this one off? |
|
Reading through this again, I would like to merge this PR as is. We always talk about keeping the surface area of changes as low as we can. Well, this is the minimal change: Always pass the new order. If we want to change the way we handle unexpected exceptions that's cool with me but I'd like to address it in a separate PR. I wouldn't even collapse the two @jordan-brough @cbrunsdon Is that ok with you? |
|
👍 |
|
Sure, 👍 by me |
Always pass the new order number on unreturned exchange failures
Currently, anything that's not specifically a
ChargeFailurewill notinclude the new order number. That's a pity, because something like an
out of stock error shows up as an exception that is not a
ChargeFailureand then it's really hard to even find the new error.This always makes the new order available to the outside so that we
don't have to specifically read it from the exception.