Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Always pass the new order number on unreturned exchange failures #388

Merged
merged 3 commits into from
Dec 23, 2015
Merged

Always pass the new order number on unreturned exchange failures #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2244915
Always pass the new order number on unreturned exchange failures
magnusvk Sep 23, 2015
5ecca06
Safer new_order handling
magnusvk Sep 23, 2015
9bf8206
Don't rescue Exception
magnusvk Sep 28, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/lib/spree/core/unreturned_item_charger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ def initialize(message, new_order)

class_attribute :failure_handler

attr_reader :original_order
attr_reader :original_order, :new_order

def initialize(shipment, return_items)
@shipment = shipment
Expand All @@ -19,6 +19,8 @@ def initialize(shipment, return_items)
end

def charge_for_items
self.new_order = Spree::Order.create!(exchange_order_attributes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just do @new_order = ..., but this is a cool style!


new_order.associate_user!(@original_order.user) if @original_order.user

add_exchange_variants_to_order
Expand Down Expand Up @@ -55,9 +57,7 @@ def charge_for_items

private

def new_order
@new_order ||= Spree::Order.create!(exchange_order_attributes)
end
attr_writer :new_order

def add_exchange_variants_to_order
@return_items.group_by(&:exchange_variant).map do |variant, variant_return_items|
Expand Down
7 changes: 4 additions & 3 deletions core/lib/tasks/exchanges.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)}
rescue Exception => e
failure = {message: "#{e.class}: #{e.message}"}
failure = { message: e.message }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can now just collapse this into the rescue => e right?

rescue => e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@magnusvk @gmacdougall changing this to rescue => e instead of rescue Exception => e does seem like the right move. (I think rescue Exception was a bad habit of mine from the Ruby 1.8 days where Timeout::Error (and maybe some other classes like that?) did not inherit from StandardError and it was often bad that rescue => e did not rescue them.)

But with this update we now have the problem that our failure_handler will never be called for anything in failures if a non-StandardError occurs while failures has something in it. (e.g. if you sigint the rake task to stop it.

We could add:

rescue Exception
  Spree::UnreturnedItemCharger.failure_handler.call(failures) if Spree::UnreturnedItemCharger.failure_handler
  raise
end

but that might not be good, depending on what the non-StandardError is.

Or we could add more rescues like rescue Interrupt => e ... for expected scenarios.
Or we could add signal handling stuff to this rake task specifically so that you can kill the task and run cleanup code.

Or we could change our failure handler so that it was called right away for each error as it occurred. That seems like the most sane option to me.

Or we could just keep this as it is now and not worry about any of this.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with rescuing Exception as long as we re-raise it. It's just not one of those things that should be swallowed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmacdougall yeah but if it's a NoMemoryError or something like that then you're likely to make things worse by trying to call more code. What do you think of calling the handler right away on each exception, and if there is no handler just allowing the exception to raise right then? We might not want to make the assumption that a store will want this code to continue processing exchanges when unexpected exceptions are happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd just collapse these and not think about rescuing non-StandardError exceptions. Like, what do you even do with a NoMemoryError? But I don't feel strongly about that. Doesn't seem clear to me what else to do though.

failure = { message: "#{e.class}: #{e.message}" }
end

if failure
failure[:new_order] = item_charger.new_order.number if item_charger.new_order
failures << failure.merge({
order: item_charger.original_order.number,
shipment: shipment.number,
Expand Down