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

Canceled orders considered as 'sold' in many parts of spree #729

Closed
zecke opened this Issue Nov 2, 2011 · 12 comments

Comments

Projects
None yet
3 participants
@zecke
Contributor

zecke commented Nov 2, 2011

A canceled order will still be considered as complete by various parts of spree. This includes the order listing when (Only show completed orders is ticked), it shows on the Overview page of the admin page, but most annoyingly it also affects the InventoryUnit and the "On Hand" calculation of items.

The easiest way to reproduce is to:
1.) set variant on-hand to 1
2.) purchase
3.) cancel the order
4.) verify on-hand is calculated as 1.

So right now the on-hand value is adjusted on payment but not when the order is canceled.

@tomash

This comment has been minimized.

Show comment
Hide comment
@tomash

tomash Nov 2, 2011

Contributor

and a small plug into this issue: in stores with tracking stock levels, cancelling an order should result in inventory levels being re-added ("back on shelf").

Contributor

tomash commented Nov 2, 2011

and a small plug into this issue: in stores with tracking stock levels, cancelling an order should result in inventory levels being re-added ("back on shelf").

@zecke

This comment has been minimized.

Show comment
Hide comment
@zecke

zecke Nov 3, 2011

Contributor

Yes and :track_inventory_levels is on by default. The only workaround is to manipulate the line_item's of a canceled order (and losing information about it) or to re-add items to the stock by hand.

Contributor

zecke commented Nov 3, 2011

Yes and :track_inventory_levels is on by default. The only workaround is to manipulate the line_item's of a canceled order (and losing information about it) or to re-add items to the stock by hand.

@tomash

This comment has been minimized.

Show comment
Hide comment
@tomash

tomash Nov 3, 2011

Contributor

I think I know which code to edit (and write some specs), are you in for some collaboration @zecke ? :)

Contributor

tomash commented Nov 3, 2011

I think I know which code to edit (and write some specs), are you in for some collaboration @zecke ? :)

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Nov 24, 2011

Member

Any update on this issue guys?

Member

radar commented Nov 24, 2011

Any update on this issue guys?

@zecke

This comment has been minimized.

Show comment
Hide comment
@zecke

zecke Nov 24, 2011

Contributor

No, was too busy with my main tasks. It will probably take a week or two until I have some to look into this.

Contributor

zecke commented Nov 24, 2011

No, was too busy with my main tasks. It will probably take a week or two until I have some to look into this.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Nov 25, 2011

Member

There's actually a pending test for this in core/spec/models/order_spec.rb now. Let me look at fixing this now.

Member

radar commented Nov 25, 2011

There's actually a pending test for this in core/spec/models/order_spec.rb now. Let me look at fixing this now.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Nov 25, 2011

Member

Um: https://github.com/spree/spree/blob/master/core/app/models/spree/order.rb#L516-517.

So there's your problem. Just fixing it now. Ideally I'd like this to be in 0.60, 0.70 and master as a fix.

Member

radar commented Nov 25, 2011

Um: https://github.com/spree/spree/blob/master/core/app/models/spree/order.rb#L516-517.

So there's your problem. Just fixing it now. Ideally I'd like this to be in 0.60, 0.70 and master as a fix.

radar added a commit that referenced this issue Nov 25, 2011

WIP for #729. When an order is cancelled, restock the products
Next thing is to check that when an order is resumed that the products are re-added
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Nov 25, 2011

Member

Right, so that's fixed. Next steps are:

  1. Ensure that when an order is resumed the products are "destocked", i.e. variant count on hand is decremented by same amount.
  2. Backport this fix + related tests to 0.70 and 0.60.
Member

radar commented Nov 25, 2011

Right, so that's fixed. Next steps are:

  1. Ensure that when an order is resumed the products are "destocked", i.e. variant count on hand is decremented by same amount.
  2. Backport this fix + related tests to 0.70 and 0.60.
@zecke

This comment has been minimized.

Show comment
Hide comment
@zecke

zecke Nov 25, 2011

Contributor

I used 'many' in the subject because I was mostly too lazy to create individual bug reports (sorry) and I don't know if github allows to have a hierarchy of bugs. So besides the stocking issue I think there are some more issues and it is around some inconsistencies of the order state. Some parts of spree check for @order.state.. some use the @order.completed_at but for a canceled order the completed_at will not be NULL but the state will be set to cancel.

core admin/orders:
The controller is using 'params[:search][:completed_at_is_not_null] ||= '1' if Spree::Config[:show_only_complete_orders_by_default]' this includes canceled orders (because they have a completed_at but then got canceled). Normally I use the state list to filter for 'state == complete'

dash
def last_five_orders
orders = Order.includes(:line_items).where("completed_at IS NOT NULL").order("completed_at DESC").limit(5)

(arguable if I want to see canceled orders or not)

def biggest_spenders
spenders = Order.sum(:total, :group => :user_id, :limit => 5, :order => "sum(total) desc", :conditions => "completed_at is not null and user_id is not null")

(I probably don't want to see canceled orders as their was no flow of cash)

Contributor

zecke commented Nov 25, 2011

I used 'many' in the subject because I was mostly too lazy to create individual bug reports (sorry) and I don't know if github allows to have a hierarchy of bugs. So besides the stocking issue I think there are some more issues and it is around some inconsistencies of the order state. Some parts of spree check for @order.state.. some use the @order.completed_at but for a canceled order the completed_at will not be NULL but the state will be set to cancel.

core admin/orders:
The controller is using 'params[:search][:completed_at_is_not_null] ||= '1' if Spree::Config[:show_only_complete_orders_by_default]' this includes canceled orders (because they have a completed_at but then got canceled). Normally I use the state list to filter for 'state == complete'

dash
def last_five_orders
orders = Order.includes(:line_items).where("completed_at IS NOT NULL").order("completed_at DESC").limit(5)

(arguable if I want to see canceled orders or not)

def biggest_spenders
spenders = Order.sum(:total, :group => :user_id, :limit => 5, :order => "sum(total) desc", :conditions => "completed_at is not null and user_id is not null")

(I probably don't want to see canceled orders as their was no flow of cash)

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 1, 2011

Member

@zecke: Please open another issue for that other bug. I think this is only to do with the re-stocking of items.

I think this problem has now been dealt with, and so I'm closing the issue.

Member

radar commented Dec 1, 2011

@zecke: Please open another issue for that other bug. I think this is only to do with the re-stocking of items.

I think this problem has now been dealt with, and so I'm closing the issue.

@radar radar closed this Dec 1, 2011

@zecke

This comment has been minimized.

Show comment
Hide comment
@zecke

zecke Dec 1, 2011

Contributor
  • So shops with canceled orders will continue to have broken stock? Does it make sense to provide a migration?
Contributor

zecke commented Dec 1, 2011

  • So shops with canceled orders will continue to have broken stock? Does it make sense to provide a migration?
@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Dec 1, 2011

Member

Well yes, they will have broken stock still. I am not confident at a way to fix this. If you have one I'd be more than welcome to review it.

Member

radar commented Dec 1, 2011

Well yes, they will have broken stock still. I am not confident at a way to fix this. If you have one I'd be more than welcome to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment