Issue 353701002: tryton-env: Add warehouses selection to supply stock wizard (Closed)
DescriptionCOLLABORATOR=sergi@koolpi.com
issue10532
Patch Set 1 #Patch Set 2 : stock_supply: Add warehouses selection to supply stock wizard #
Total comments: 4
Patch Set 3 : Set the warehouses into the purchase_parameters, break line and remove warehouses label #Patch Set 4 : Add whitespace after : #
Total comments: 2
Patch Set 5 : Set warehouses on StockSupply.execute #
Total comments: 2
Patch Set 6 : stock_supply: Make supply stock wizard able to work without values and add colspan for warehouses f… #
Total comments: 2
Patch Set 7 : Test if the StartView has a warehouse value and not pass data where not needed #Patch Set 8 : Test if the start StateView has a warehouse value and not pass data where not needed #
Total comments: 6
Patch Set 9 : Not set warehouses required to False and comment why getattr() is used #
Total comments: 1
Patch Set 10 : Also update stock_supply_production #
Total comments: 1
Patch Set 11 : Remove breakpoint #
Total comments: 3
Patch Set 12 : Start is empty on cron -> start is empty when run by cron and add help message in warehouses field #
Total comments: 1
Patch Set 13 : Put help as last keyword in warehouses field #
Total comments: 1
Patch Set 14 : Filter order point by warehouses #Patch Set 15 : Filter order point by warehouses #
Total comments: 6
Patch Set 16 : Append new parameters in case someone was using positional, use a set instead of a list, filter onl… #
Total comments: 2
Patch Set 17 : Use warehouse instances instead of ids and create a new list of order_points #
Total comments: 3
Patch Set 18 : Fill used_order_points list properly and call generate_internal_shipment just once #
Total comments: 3
Patch Set 19 : Remove useless declaration, break line at opening [ and overwrite order_points #Patch Set 20 : Update to tip #
MessagesTotal messages: 59
stock_supply: Add warehouses selection to supply stock wizard
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/342601004/stock.py File stock.py (left): https://codereview.tryton.org/353701002/diff/342601004/stock.py#oldcode106 stock.py:106: PurchaseRequest.generate_requests(**self._purchase_parameters) Why not setting the warehouses into the purchase_parameters?
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/342601004/stock.py File stock.py (left): https://codereview.tryton.org/353701002/diff/342601004/stock.py#oldcode106 stock.py:106: PurchaseRequest.generate_requests(**self._purchase_parameters) On 2021/06/28 10:30:47, pokoli wrote: > Why not setting the warehouses into the purchase_parameters? But mainly _purchase_parameters must still be used to not break code that would extend it. https://codereview.tryton.org/353701002/diff/342601004/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/342601004/stock.py#newcode118 stock.py:118: warehouses = fields.Many2Many('stock.location', The style prefer to break like on opening '(' https://codereview.tryton.org/353701002/diff/342601004/view/supply_start_form... File view/supply_start_form.xml (right): https://codereview.tryton.org/353701002/diff/342601004/view/supply_start_form... view/supply_start_form.xml:8: <label name="warehouses"/> warehouses is a Many2Many so no need for a label.
Sign in to reply to this message.
Set the warehouses into the purchase_parameters, break line and remove warehouses label
Sign in to reply to this message.
Add whitespace after :
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/358341002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/358341002/stock.py#newcode118 stock.py:118: 'stock.location', None, None, 'Warehouses', required=True, Warehouses should be in double quotes (as it's an human readable string). https://codereview.tryton.org/353701002/diff/358341002/stock.py#newcode122 stock.py:122: help="The warehouses for which the stock will be supplied.") I think there is no need for help text. It is clear the usage of the field.
Sign in to reply to this message.
Set warehouses on StockSupply.execute
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/362101002/order_point.py File order_point.py (right): https://codereview.tryton.org/353701002/diff/362101002/order_point.py#newcode263 order_point.py:263: StockSupply.execute(session_id, data, 'create_') I think it will be better flexible if the wizard could work without values by having default fallback. https://codereview.tryton.org/353701002/diff/362101002/view/supply_start_form... File view/supply_start_form.xml (right): https://codereview.tryton.org/353701002/diff/362101002/view/supply_start_form... view/supply_start_form.xml:8: <field name="warehouses"/> I think it should colspan="2"
Sign in to reply to this message.
stock_supply: Make supply stock wizard able to work without values and add colspan for warehouses field
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
stock_supply_production should be also updated. https://codereview.tryton.org/353701002/diff/338831016/order_point.py File order_point.py (right): https://codereview.tryton.org/353701002/diff/338831016/order_point.py#newcode262 order_point.py:262: data = {'start': {'warehouses': None}} We do not need to pass data here. https://codereview.tryton.org/353701002/diff/338831016/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/338831016/stock.py#newcode101 stock.py:101: return {'warehouses': self.start.warehouses} You should test if the start StateView has a warehouse value and return an empty dict when none. Something like: parameters = {} if getattr(self.start, 'warehouses', None): parameters['warehouses'] = self.start.warehouses
Sign in to reply to this message.
Test if the StartView has a warehouse value and not pass data where not needed
Sign in to reply to this message.
Test if the start StateView has a warehouse value and not pass data where not needed
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
I agree that stock_supply_production should also be updated to pass warehouses to Production.generate_requests. https://codereview.tryton.org/353701002/diff/334991002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/334991002/stock.py#newcode102 stock.py:102: if getattr(self.start, 'warehouses', None): I do not think we need to use getattr with default. https://codereview.tryton.org/353701002/diff/334991002/stock.py#newcode121 stock.py:121: 'stock.location', None, None, "Warehouses", required=False, no need to set required to False.
Sign in to reply to this message.
If I do it with "if self.start.warehouses:", I receive AttributeError: 'stock.supply.start' Model has no attribute 'warehouses': None https://codereview.tryton.org/353701002/diff/334991002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/334991002/stock.py#newcode102 stock.py:102: if getattr(self.start, 'warehouses', None): On 2021/09/15 22:07:26, ced wrote: > I do not think we need to use getattr with default. If I do it with "if self.start.warehouses:", I receive AttributeError: 'stock.supply.start' Model has no attribute 'warehouses': None
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/334991002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/334991002/stock.py#newcode102 stock.py:102: if getattr(self.start, 'warehouses', None): On 2021/09/16 07:38:07, Davidoff wrote: > On 2021/09/15 22:07:26, ced wrote: > > I do not think we need to use getattr with default. > > If I do it with "if self.start.warehouses:", I receive AttributeError: > 'stock.supply.start' Model has no attribute 'warehouses': None This should not happen and must be investigate because it may show that indeed the warehouses are never filled.
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/334991002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/334991002/stock.py#newcode102 stock.py:102: if getattr(self.start, 'warehouses', None): On 2021/09/16 07:47:48, ced wrote: > On 2021/09/16 07:38:07, Davidoff wrote: > > On 2021/09/15 22:07:26, ced wrote: > > > I do not think we need to use getattr with default. > > > > If I do it with "if self.start.warehouses:", I receive AttributeError: > > 'stock.supply.start' Model has no attribute 'warehouses': None > > This should not happen and must be investigate because it may show that indeed > the warehouses are never filled. It is not filled when running from the cron job. We have two options here: 1. use getattr( for supporting cron 2. Set all the parameters on the cron job. What do you prefer?
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/334991002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/334991002/stock.py#newcode102 stock.py:102: if getattr(self.start, 'warehouses', None): On 2021/09/16 08:38:43, pokoli wrote: > On 2021/09/16 07:47:48, ced wrote: > > On 2021/09/16 07:38:07, Davidoff wrote: > > > On 2021/09/15 22:07:26, ced wrote: > > > > I do not think we need to use getattr with default. > > > > > > If I do it with "if self.start.warehouses:", I receive AttributeError: > > > 'stock.supply.start' Model has no attribute 'warehouses': None > > > > This should not happen and must be investigate because it may show that indeed > > the warehouses are never filled. > > It is not filled when running from the cron job. We have two options here: > > 1. use getattr( for supporting cron > 2. Set all the parameters on the cron job. > > What do you prefer? OK let's keep the getattr but then we need a comment that explain why.
Sign in to reply to this message.
Not set warehouses required to False and comment why getattr() is used
Sign in to reply to this message.
Still missing stock_supply_production https://codereview.tryton.org/353701002/diff/344741002/stock.py File stock.py (right): https://codereview.tryton.org/353701002/diff/344741002/stock.py#newcode102 stock.py:102: # We use getattr() for supporting cron, because warehouses field I think it can be more consice with something like: # Use getattr because start is empty on cron
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
Also update stock_supply_production
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/336921006/modules/stock_supply_p... File modules/stock_supply_production/stock.py (right): https://codereview.tryton.org/353701002/diff/336921006/modules/stock_supply_p... modules/stock_supply_production/stock.py:101: breakpoint() breakpoint should be removed
Sign in to reply to this message.
Remove breakpoint
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/373591002/modules/stock_supply/s... File modules/stock_supply/stock.py (right): https://codereview.tryton.org/353701002/diff/373591002/modules/stock_supply/s... modules/stock_supply/stock.py:102: # Use getattr because start is empty on cron ... start is empty when run by cron https://codereview.tryton.org/353701002/diff/373591002/modules/stock_supply/s... modules/stock_supply/stock.py:125: ]) I think we need an help which say: If empty all warehouses are used. https://codereview.tryton.org/353701002/diff/373591002/modules/stock_supply_p... File modules/stock_supply_production/stock.py (right): https://codereview.tryton.org/353701002/diff/373591002/modules/stock_supply_p... modules/stock_supply_production/stock.py:99: # Use getattr because start is empty on cron idem
Sign in to reply to this message.
Start is empty on cron -> start is empty when run by cron and add help message in warehouses field
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/373711002/modules/stock_supply/s... File modules/stock_supply/stock.py (right): https://codereview.tryton.org/353701002/diff/373711002/modules/stock_supply/s... modules/stock_supply/stock.py:123: help="If empty all warehouses are used.", Guidelines are to put help as last keyword.
Sign in to reply to this message.
Put help as last keyword in warehouses field
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/381141002/modules/stock_supply/s... File modules/stock_supply/stock.py (right): https://codereview.tryton.org/353701002/diff/381141002/modules/stock_supply/s... modules/stock_supply/stock.py:94: return ShipmentInternal.generate_internal_shipment(clean=clean) I think we must also support warehouses here. We can filter for order point with a provisioning_location or overflowing_location in the warehouses and location in the warehouses.
Sign in to reply to this message.
Filter order point by warehouses
Sign in to reply to this message.
Filter order point by warehouses
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... File modules/stock_supply/shipment.py (right): https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... modules/stock_supply/shipment.py:14: def generate_internal_shipment(cls, warehouses=None, clean=True): Better to append new parameters in case someone was using positional. https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... modules/stock_supply/shipment.py:31: # fetch warehouses: This comment does not add any value. https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... modules/stock_supply/shipment.py:35: warehouse_ids = [w.id for w in warehouses] It will be faster to use a set https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... modules/stock_supply/shipment.py:52: for op in order_points: There is no need to filter if warehouses was None. https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... modules/stock_supply/shipment.py:54: and op.warehouse_location.warehouse.id warehouse_location is always empty for internal type. Indeed you must use op.location.warehouse https://codereview.tryton.org/353701002/diff/355621002/modules/stock_supply/s... modules/stock_supply/shipment.py:56: order_points.remove(op) It will be faster to use a list comprehension with an if test than removing one by one the order point.
Sign in to reply to this message.
Append new parameters in case someone was using positional, use a set instead of a list, filter only if warehouses is not None, use location.warehouse instead of warehouse_location.warehouse and use a list comprehension
Sign in to reply to this message.
flake8 OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/360281002/modules/stock_supply/s... File modules/stock_supply/shipment.py (right): https://codereview.tryton.org/353701002/diff/360281002/modules/stock_supply/s... modules/stock_supply/shipment.py:55: and op.location.warehouse.id not in warehouse_ids] The comment about list comprehension is to create a new list of order_points. https://codereview.tryton.org/353701002/diff/360281002/modules/stock_supply/s... modules/stock_supply/shipment.py:55: and op.location.warehouse.id not in warehouse_ids] There is no point to use id for comparison, this can be done with the warehouse instances.
Sign in to reply to this message.
Use warehouse instances instead of ids and create a new list of order_points
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/376231003/modules/stock_supply/s... File modules/stock_supply/shipment.py (right): https://codereview.tryton.org/353701002/diff/376231003/modules/stock_supply/s... modules/stock_supply/shipment.py:52: if warehouses: There is no need to test this as it can no be None. https://codereview.tryton.org/353701002/diff/376231003/modules/stock_supply/s... modules/stock_supply/shipment.py:56: and op.location.warehouse in warehouses] This is a strange way to fill a list. Why not: used_order_points = [op for op in order_points if op.location.warehouse in warehouses] https://codereview.tryton.org/353701002/diff/376231003/modules/stock_supply/s... File modules/stock_supply/stock.py (right): https://codereview.tryton.org/353701002/diff/376231003/modules/stock_supply/s... modules/stock_supply/stock.py:99: return ShipmentInternal.generate_internal_shipment(clean=clean) It will be better to have only one call to generate_internal_shipment and just test to fill warehouses. It will probably good to have the same design as _purchase_parameters.
Sign in to reply to this message.
Fill used_order_points list properly and call generate_internal_shipment just once
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
https://codereview.tryton.org/353701002/diff/360921002/modules/stock_supply/s... File modules/stock_supply/shipment.py (right): https://codereview.tryton.org/353701002/diff/360921002/modules/stock_supply/s... modules/stock_supply/shipment.py:51: used_order_points = [] useless https://codereview.tryton.org/353701002/diff/360921002/modules/stock_supply/s... modules/stock_supply/shipment.py:52: used_order_points = [op for op in order_points break like at opening [ https://codereview.tryton.org/353701002/diff/360921002/modules/stock_supply/s... modules/stock_supply/shipment.py:52: used_order_points = [op for op in order_points I think there is no need to use a new name for the order_points list.
Sign in to reply to this message.
Remove useless declaration, break line at opening [ and overwrite order_points
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
Update to tip
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/353701002
Sign in to reply to this message.
New changeset 0fb7b188fb26 by David Blanco Bautista in branch 'default': Add warehouses selection to supply stock wizard https://hg.tryton.org/modules/stock_supply/rev/0fb7b188fb26
Sign in to reply to this message.
New changeset 6edaaba16b00 by David Blanco Bautista in branch 'default': Add warehouses selection to supply stock wizard https://hg.tryton.org/modules/stock_supply_production/rev/6edaaba16b00
Sign in to reply to this message.
New changeset 9c3595e62be2 by David Blanco Bautista in branch 'default': Add warehouses selection to supply stock wizard https://hg.tryton.org/tryton-env/rev/9c3595e62be2
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
