Issue 419231003: sale_stock_quantity: Apply check_stock_quantity per warehouse line (Closed)
Descriptionissue11531
COLLABORATOR=cedric.krier@b2ck.com
Patch Set 1 #
Total comments: 1
Patch Set 2 : Do test inside check_stock_quantity #
Total comments: 4
Patch Set 3 : Apply remarks #
Total comments: 6
Patch Set 4 : Apply Ced's remrks #
Total comments: 6
Patch Set 5 : Apply remarks #
Total comments: 14
Patch Set 6 : Apply remarks #
Total comments: 6
Patch Set 7 : Apply Ced's remarks #
Total comments: 2
Patch Set 8 : Apply remark and update to tip #
Total comments: 4
Patch Set 9 : Apply remark and update to tip #
Total comments: 1
Patch Set 10 : Compute quantity per warehouse #MessagesTotal messages: 38
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/439161003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/439161003/sale.py#newcode38 sale.py:38: if sale.warehouse: I think it's better to do the test nside check_stock_quantity. This way if the method is called from another place we do not need to care if the warehouse is set or not.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/435231003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/435231003/sale.py#newcode87 sale.py:87: def filter_line(line): I think it is here that the line without warehouse should be filtered out. https://codereview.tryton.org/419231003/diff/435231003/sale.py#newcode136 sale.py:136: locations=[self.warehouse.id], Indeed we should group products by warehouse from the line.
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/435231003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/435231003/sale.py#newcode87 sale.py:87: def filter_line(line): On 2022/06/01 21:22:02, ced wrote: > I think it is here that the line without warehouse should be filtered out. Done. https://codereview.tryton.org/419231003/diff/435231003/sale.py#newcode136 sale.py:136: locations=[self.warehouse.id], On 2022/06/01 21:22:02, ced wrote: > Indeed we should group products by warehouse from the line. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/411221005/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/411221005/sale.py#newcode142 sale.py:142: quantities = {p: p.forecast_quantity for p in products} quantities must be conserved between warehouse. https://codereview.tryton.org/419231003/diff/411221005/sale.py#newcode142 sale.py:142: quantities = {p: p.forecast_quantity for p in products} I think the quantities must be stored per warehouse, product. https://codereview.tryton.org/419231003/diff/411221005/sale.py#newcode183 sale.py:183: quantities[product] -= quantity The quantity must be taken from the right product and warehouse.
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/411221005/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/411221005/sale.py#newcode142 sale.py:142: quantities = {p: p.forecast_quantity for p in products} On 2022/06/02 09:26:13, ced wrote: > I think the quantities must be stored per warehouse, product. Done. https://codereview.tryton.org/419231003/diff/411221005/sale.py#newcode142 sale.py:142: quantities = {p: p.forecast_quantity for p in products} On 2022/06/02 09:26:13, ced wrote: > quantities must be conserved between warehouse. Done. https://codereview.tryton.org/419231003/diff/411221005/sale.py#newcode183 sale.py:183: quantities[product] -= quantity On 2022/06/02 09:26:13, ced wrote: > The quantity must be taken from the right product and warehouse. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/427331003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/427331003/sale.py#newcode143 sale.py:143: quantities[warehouse.id] = { could be done outside the with statement https://codereview.tryton.org/419231003/diff/427331003/sale.py#newcode150 sale.py:150: ('sale.warehouse', '=', warehouse.id), We need to search on the warehouse of the line. But as it is a Function field, I guess we could just test it in the lines loop. https://codereview.tryton.org/419231003/diff/427331003/sale.py#newcode167 sale.py:167: quantities[warehouse.id][product] -= quantity I think you could use the warehouse instead instead of id.
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/427331003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/427331003/sale.py#newcode143 sale.py:143: quantities[warehouse.id] = { On 2022/06/07 21:29:22, ced wrote: > could be done outside the with statement Done. https://codereview.tryton.org/419231003/diff/427331003/sale.py#newcode150 sale.py:150: ('sale.warehouse', '=', warehouse.id), On 2022/06/07 21:29:22, ced wrote: > We need to search on the warehouse of the line. > But as it is a Function field, I guess we could just test it in the lines loop. Done. https://codereview.tryton.org/419231003/diff/427331003/sale.py#newcode167 sale.py:167: quantities[warehouse.id][product] -= quantity On 2022/06/07 21:29:22, ced wrote: > I think you could use the warehouse instead instead of id. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/435301003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode134 sale.py:134: for warehouse, lines in groupby(lines, key=lambda l: l.warehouse): you must sort lines by the same grouping key for better optimization. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode134 sale.py:134: for warehouse, lines in groupby(lines, key=lambda l: l.warehouse): It may be good to rename lines to avoid clash with the filtered lines. Maybe w_lines https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode137 sale.py:137: product_ids = list(product_ids) the 2 lines could be merged https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode142 sale.py:142: products = Product.browse(product_ids) Indeed browse convert to int and take any iterable so it could be: with ...: products = Product.browse({l.product for l in lines}) https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode146 sale.py:146: for sub_product_ids in grouped_slice(product_ids): could reuse the products instances https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode147 sale.py:147: lines = Line.search([ It may be good to rename this as other_lines to not confused with the grouped lines https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode168 sale.py:168: for line in filter(filter_line, self.lines): could reuse the previously filtered lines if we convert it to a list.
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/435301003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode134 sale.py:134: for warehouse, lines in groupby(lines, key=lambda l: l.warehouse): On 2022/06/17 09:39:50, ced wrote: > you must sort lines by the same grouping key for better optimization. Done. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode134 sale.py:134: for warehouse, lines in groupby(lines, key=lambda l: l.warehouse): On 2022/06/17 09:39:51, ced wrote: > It may be good to rename lines to avoid clash with the filtered lines. > Maybe w_lines Done. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode137 sale.py:137: product_ids = list(product_ids) On 2022/06/17 09:39:50, ced wrote: > the 2 lines could be merged Done. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode142 sale.py:142: products = Product.browse(product_ids) On 2022/06/17 09:39:51, ced wrote: > Indeed browse convert to int and take any iterable so it could be: > > with ...: > products = Product.browse({l.product for l in lines}) Using this, i had to do some changes in method get_delta (line 103). Done. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode146 sale.py:146: for sub_product_ids in grouped_slice(product_ids): On 2022/06/17 09:39:50, ced wrote: > could reuse the products instances Done. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode147 sale.py:147: lines = Line.search([ On 2022/06/17 09:39:50, ced wrote: > It may be good to rename this as other_lines to not confused with the grouped > lines Done. https://codereview.tryton.org/419231003/diff/435301003/sale.py#newcode168 sale.py:168: for line in filter(filter_line, self.lines): On 2022/06/17 09:39:51, ced wrote: > could reuse the previously filtered lines if we convert it to a list. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/419391003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/419391003/sale.py#newcode104 sale.py:104: product_ids = [p.id for p in products] better to convert into list after the grouping. https://codereview.tryton.org/419231003/diff/419391003/sale.py#newcode148 sale.py:148: for sub_product_ids in grouped_slice(products): It should be named sub_products https://codereview.tryton.org/419231003/diff/419391003/sale.py#newcode170 sale.py:170: for line in list(lines): Why calling list?
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/419391003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/419391003/sale.py#newcode104 sale.py:104: product_ids = [p.id for p in products] On 2022/06/21 21:46:01, ced wrote: > better to convert into list after the grouping. Done. https://codereview.tryton.org/419231003/diff/419391003/sale.py#newcode148 sale.py:148: for sub_product_ids in grouped_slice(products): On 2022/06/21 21:46:01, ced wrote: > It should be named sub_products Done. https://codereview.tryton.org/419231003/diff/419391003/sale.py#newcode170 sale.py:170: for line in list(lines): On 2022/06/21 21:46:01, ced wrote: > Why calling list? Removed.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/441211003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/441211003/sale.py#newcode95 sale.py:95: def get_delta(date): products must be passed now as it is no more a global variable.
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/441211003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/441211003/sale.py#newcode95 sale.py:95: def get_delta(date): On 2022/07/18 20:11:12, ced wrote: > products must be passed now as it is no more a global variable. Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
I think the title should be changed to reflect the actual change. https://codereview.tryton.org/419231003/diff/419521003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/419521003/sale.py#newcode138 sale.py:138: for warehouse, w_lines in groupby(lines, keyfunc): Indeed we do not need to group lines by warehouse. We just need to have all the warehouses. So it will be simpler to just construct a warehouse set like {l.warehouse for l in lines}
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/419521003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/419521003/sale.py#newcode138 sale.py:138: for warehouse, w_lines in groupby(lines, keyfunc): On 2022/09/08 06:25:24, ced wrote: > Indeed we do not need to group lines by warehouse. We just need to have all the > warehouses. > So it will be simpler to just construct a warehouse set like {l.warehouse for l > in lines} Done.
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
https://codereview.tryton.org/419231003/diff/419521003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/419521003/sale.py#newcode138 sale.py:138: for warehouse, w_lines in groupby(lines, keyfunc): On 2022/09/08 06:25:24, ced wrote: > Indeed we do not need to group lines by warehouse. We just need to have all the > warehouses. > So it will be simpler to just construct a warehouse set like {l.warehouse for l > in lines} This is wrong because the point is to check quantity per line warehouse. https://codereview.tryton.org/419231003/diff/419521003/sale.py#newcode195 sale.py:195: delta = get_delta(date, products) this should be called for the warehouse line https://codereview.tryton.org/419231003/diff/418001003/sale.py File sale.py (right): https://codereview.tryton.org/419231003/diff/418001003/sale.py#newcode105 sale.py:105: [self.warehouse.id], This should be per line warehouse
Sign in to reply to this message.
checks OK URL: https://codereview.tryton.org/419231003
Sign in to reply to this message.
|
