-
-
Notifications
You must be signed in to change notification settings - Fork 781
[16.0][IMP] product_lot_sequence: product sequence exceptions #2109
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
base: 16.0
Are you sure you want to change the base?
[16.0][IMP] product_lot_sequence: product sequence exceptions #2109
Conversation
66704b3 to
aceb352
Compare
| disallow_lot_sequence = fields.Boolean( | ||
| help="Don't use an specific lot sequence for this product" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to have this variable inversed in meaning with a migration script.
Seems more readable and less confusing
| disallow_lot_sequence = fields.Boolean( | |
| help="Don't use an specific lot sequence for this product" | |
| ) | |
| use_specific_lot_sequence = fields.Boolean( | |
| help="Use an specific lot sequence for this product", | |
| default=True, | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
yajo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO the fact that checking (or unchecking, as per https://github.com/OCA/product-attribute/pull/2109/files#r2451880343) a checkbox removes or creates a record is a bit surprising.
I think that we can do a better UI:
When removing, add a confirm="Are you sure?" or similar to the button.
This way, the effect and usability is nearly the same, but we avoid surprises.
There might be cases where we don't want to assign specific sequences to our product and keep the core behavior. For that we've created a flag that allows to do so. - The flag *Use specific sequence* is on by default. - Set it off and the product will behave as in Odoo core. - Once you do so, you can remove the product's lot sequence as well. MT-12305
aceb352 to
325ec93
Compare
fcvalgar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional Review,
LGTM, thank you @chienandalu
yajo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When creating a new product, I think this is inconsistent:
Then, if you do this:
- Select "by lots"
- Create sequence
- Uncheck "use specific lot sequence".
You get another inconsistency:
I think the problem is that the check box is now unnecessary. See comments below. If we remove the checkbox, it's gonna be less code and simpler UX.
Thanks!
| use_specific_lot_sequence = fields.Boolean( | ||
| help="Use an specific lot sequence for this product", | ||
| default=True, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field can be removed now, and all other uses of it.
Since the creation or removal is done by buttons, then the presence or absence of a sequence is enough to make the module work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because there's an automation that creates the sequence on every write if it isn't one and we want to avoid that (and I don't want to change that behavior that might affect other users of the module)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even in that case, the behavior is inconsistent if we have 2 ways to do the same.
However, if that's the goal, there's an easier way to achieve it. Just add support for a new system parameter that disables the automatic creation of sequences, so if the user wants one, they just have to click on the create button.
| @api.depends("tracking") # For products being created (before saved). | ||
| @api.depends("tracking", "use_specific_lot_sequence") | ||
| def _compute_display_lot_sequence_fields(self): | ||
| self.display_lot_sequence_fields = ( | ||
| product_sequence_policy = ( | ||
| self.env["stock.lot"]._get_sequence_policy() == "product" | ||
| ) | ||
| for product in self: | ||
| product.display_lot_sequence_fields = ( | ||
| product_sequence_policy and product.use_specific_lot_sequence | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should start by checking that if tracking == "none", then product.display_lot_sequence_fields = False.
Otherwise, just use the default value from config, since the checkbox should be removed.
| product_sequence_policy and product.use_specific_lot_sequence | ||
| ) | ||
|
|
||
| @api.model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @api.model |
There might be cases where we don't want to assign specific sequences to our
product and keep the core behavior. For that we've created a flag that allows
to do so.
(TODO: remove this vídeo, as it's an old version of the change)
https://www.loom.com/share/4edde17a26ef4d2f9d909e59ea538f7b?sid=86bbaefb-926b-46a8-982f-c9b1a800e364
cc @moduon MT-12305
please review @rafaelbn @fcvalgar @Shide