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.

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 option to automatically finish an order #495

Merged
merged 1 commit into from
Oct 12, 2017

Conversation

paroga
Copy link
Member

@paroga paroga commented Aug 20, 2017

This change allows us to implement automatic notifications
to the supplier in the next step.

@paroga paroga force-pushed the feature/auto_finish_order branch from 757cfed to 5474569 Compare August 20, 2017 12:58
@wvengen
Copy link
Member

wvengen commented Aug 21, 2017

Looking good!
What is your use-case for this?

This idea has come up for me in the past several times, and I decided not to implement it, because there was always a manual step after the order was closed (sending the order to the supplier). So why bother closing automatically when it really depends on the next manual step? That gives people just a bit more time to order until it is really time. Having automated mails would make this more useful (but many foodcoops want to add manual notes to it about pickup arrangement etc.).

Anyway, if there's use, let's include it. I would be careful to add many checkboxes for options like this, because it requires making more decisions in the normal flow. Anyway, I think it's fine now.

@wvengen
Copy link
Member

wvengen commented Aug 21, 2017

Ah, this ties in to #488.

@paroga
Copy link
Member Author

paroga commented Aug 21, 2017 via email

@wvengen
Copy link
Member

wvengen commented Aug 21, 2017

That sounds really good!

@paroga paroga force-pushed the feature/auto_finish_order branch from 5474569 to 6459c85 Compare October 11, 2017 22:23
@paroga
Copy link
Member Author

paroga commented Oct 11, 2017

i've adopted this change

Copy link
Member

@wvengen wvengen left a comment

Choose a reason for hiding this comment

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

I love this feature :) Some small remarks, otherwise fine.

elsif auto_close_and_send_min_quantity?
finish! created_by
send_to_supplier created_by if order.sum >= order.supplier.min_order_quantity
end
Copy link
Member

Choose a reason for hiding this comment

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

In many other places we use finish!(user), maybe here as well? (Also send_to_supplier.)

update!(last_sent_mail: Time.now)
end

def do_end_action
Copy link
Member

Choose a reason for hiding this comment

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

Does this warrant an exclamation mark? (Since it calls finish!, which changes the order).

@@ -263,6 +265,35 @@ def close_direct!(user)
update_attributes! state: 'closed', updated_by: user
end

def send_to_supplier(user)
Copy link
Member

Choose a reason for hiding this comment

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

Does this warrant an exclamation mark? (Since it sends a mail and calls update!, which changes the order).

end
end

def self.finish_ended
Copy link
Member

Choose a reason for hiding this comment

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

same here

# Finish ended orders
every 1.minute do
rake "multicoops:run TASK=foodsoft:finish_ended_orders"
end
Copy link
Member

Choose a reason for hiding this comment

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

Could it be that this task takes more than a minute to run? Would this interfere? Just to check.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's very unlikely that it takes so long (but i'll address this problem with the new time zone aware scheduler anyway later). it it really takes that long finish! will fail, because the order is already finished and nothing happens

@wvengen wvengen added this to the 4.6 milestone Oct 12, 2017
@paroga paroga force-pushed the feature/auto_finish_order branch from 6459c85 to af35d2d Compare October 12, 2017 18:43
@@ -117,8 +117,7 @@ def finish
# Send a order to the supplier.
def send_result_to_supplier
order = Order.find(params[:id])
Mailer.order_result_supplier(@current_user, order).deliver_now
order.update!(last_sent_mail: Time.now)
order.send_to_supplier(@current_user)
Copy link
Member

Choose a reason for hiding this comment

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

don't forget to change this!

@paroga paroga merged commit 25553e3 into master Oct 12, 2017
@paroga paroga deleted the feature/auto_finish_order branch October 12, 2017 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants