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

[16.0][ADD] stock_picking_status_notification: Add new module to notify user after change picking states #1680

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

GabbasovDinar
Copy link
Member

@GabbasovDinar GabbasovDinar commented Aug 26, 2024

This module allows to send notifications to selected backend users when the state of stock picking changes . For this configurable notification rules are used. Rules are checked using flexible patterns. The first one rule matching the pattern is triggered.

Task: 3874

@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch 6 times, most recently from 6ebcefc to 8627894 Compare August 26, 2024 18:29
@rvalyi
Copy link
Member

rvalyi commented Aug 26, 2024

Just an observation: some modules similar modules with mail templates like edi_notification or account_bank_payment_notification use the notification suffix instead of notify. In the OCA the notify suffix has been used in web_notify for server push. So I wonder if it might not be better to user notification here for consistency. I don't have a strong opinion about this; it's just an observation.

cc @pedrobaeza @sbidoul

@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch 2 times, most recently from 7543b61 to 32255c4 Compare August 26, 2024 18:47
@GabbasovDinar GabbasovDinar marked this pull request as ready for review August 26, 2024 18:47
@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch from 32255c4 to 5a7da37 Compare August 26, 2024 19:04
@pedrobaeza pedrobaeza added this to the 16.0 milestone Aug 27, 2024
@pedrobaeza
Copy link
Member

About the formal aspect, the tag of the commit should be [ADD], not [NEW]. About the module name and feature, stock_picking_state_tracking can be more appropriated if the implementation is with a mail subtype and directly subscribers of the thread receive the notifications using standard options. You can put that subtype inheritable in picking types.

@GabbasovDinar GabbasovDinar changed the title [16.0][NEW] stock_picking_notify: Add new module to notify user after change picking states [16.0][ADD] stock_picking_notify: Add new module to notify user after change picking states Aug 29, 2024
@ivs-cetmix
Copy link
Member

About the formal aspect, the tag of the commit should be [ADD], not [NEW]. About the module name and feature, stock_picking_state_tracking can be more appropriated if the implementation is with a mail subtype and directly subscribers of the thread receive the notifications using standard options. You can put that subtype inheritable in picking types.

Hello @pedrobaeza thank you for your feedback!
Actually this module has nothing to do with the standard tracking mechanism. Moreover people who will get notified can be even not the followers of the document they are notified about.
I have put the general idea into the CONTEXT section, here is some further elaboration:
We have a warehouse with around 30 people working there. Odoo is installed on the laptop which is located on a table in the middle. So once a new SO is completed in the shop someone should take care of it.
Actually we are going to use sound notification because people are not just sitting at the table and waiting.
Here is a PR for that: OCA/web#2923

@pedrobaeza
Copy link
Member

Well, I think that should be a pro-active action, checking a list of "completed orders" more than a reactive one, having a notification per order (even with sound), but anyone takes the approach they consider...

stock_picking_status_notification then for the current approach of the module.

@ivs-cetmix
Copy link
Member

Well, I think that should be a pro-active action, checking a list of "completed orders" more than a reactive one, having a notification per order (even with sound), but anyone takes the approach they consider...

stock_picking_status_notification then for the current approach of the module.

This is not the case here. This will require someone stop his work, go to the laptop and check the orders any eg 10 minutes.
Which will lead to the excessive time loss. Because sometimes there can be one order in 2-3 minutes, and sometimes there can be no orders for an hour.

@pedrobaeza
Copy link
Member

I see something weird in that flows, but I'm not blocking anyway. I don't see normal that someone has to stop their work to do "something" on that case. Automate that "something" or batch it somehow.

@ivs-cetmix
Copy link
Member

I see something weird in that flows, but I'm not blocking anyway. I don't see normal that someone has to stop their work to do "something" on that case. Automate that "something" or batch it somehow.

Well this happens. Let's say you are packing the website orders which are queued and planned. But when there is an offline sale and the customer is here you to serve him right now. And then continue with your website orders.

@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 29, 2024

Uhm, OK, that seems a real need, although missing the notification/sound seems a real possibility, so you should consider a fallback.

@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch 2 times, most recently from 82e2100 to 76f9bd2 Compare August 29, 2024 11:48
@GabbasovDinar GabbasovDinar changed the title [16.0][ADD] stock_picking_notify: Add new module to notify user after change picking states [16.0][ADD] stock_picking_status_notification: Add new module to notify user after change picking states Aug 29, 2024
@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch 2 times, most recently from 9e417fa to 2ead2ec Compare August 30, 2024 07:25
@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch 4 times, most recently from 50dfa78 to a14dcc5 Compare September 4, 2024 07:58
Copy link
Member

@ivs-cetmix ivs-cetmix left a comment

Choose a reason for hiding this comment

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

Functional review LGTM

Copy link

@hoangtrann hoangtrann left a comment

Choose a reason for hiding this comment

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

Code LGTM

…fy user

This module adds the ability to notify the user after the picking state changes

Task: 3874
@GabbasovDinar GabbasovDinar force-pushed the 16.0-t3874-stock_notify_picking-new-module branch from a14dcc5 to 61dddf5 Compare October 21, 2024 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants