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

shopfloor: ensure that confirmation email is sent when moves are validated #53

Merged
merged 3 commits into from
Aug 27, 2020

Conversation

sebalix
Copy link
Member

@sebalix sebalix commented Aug 27, 2020

I let some FIXME in the code regarding the existing calls to move._action_done without the _sf_no_backorder context key, I don't know if it was what we wanted when this code has been written or if it needs to be updated/fixed now?

Issue 1215

ping @guewen

@sebalix sebalix force-pushed the 13.0-shopfloor-1215 branch from 012496a to f4b058e Compare August 27, 2020 08:20
@guewen
Copy link

guewen commented Aug 27, 2020

I'd not have taken this approach that needs to pass a context key: almost sure we'll forget to do it on new calls to _action_done().
I'd have added a context key _action_done_from_picking=True in StockPicking.action_done(), then run the check for _send_confirmation_mail only when we don't have this key.
I may miss a valid reason to use your approach though.

@@ -207,7 +207,8 @@ def _set_destination_and_done(self, move, scanned_location):
# when writing the destination on the package level, it writes
# on the move lines
move.move_line_ids.package_level_id.location_dest_id = scanned_location
move._action_done()
# FIXME: '_sf_no_backorder' was not passed in the context, is it expected?
Copy link

Choose a reason for hiding this comment

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

Perhaps. Maybe. I don't know.
Originally, this scenario was meant to move a picking with a single package, the behavior for other uses cases are undefined (to my knowledge).

@sebalix
Copy link
Member Author

sebalix commented Aug 27, 2020

I'll do as you suggest, it's safer.

@sebalix
Copy link
Member Author

sebalix commented Aug 27, 2020

@guewen New approach pushed as fixup

Copy link

@guewen guewen left a comment

Choose a reason for hiding this comment

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

Nice, great to have added checks in the tests

@guewen guewen merged commit 033b919 into camptocamp:13.0-shopfloor Aug 27, 2020
simahawk pushed a commit to simahawk/wms that referenced this pull request Oct 12, 2020
sebalix pushed a commit that referenced this pull request Nov 4, 2020
Signed-off-by jgrandguillaume
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