production_work: Show cost of production works

issue11001

[reviewbot, 2021-11-30 17:00:00]

flake8 OK
URL: https://codereview.tryton.org/345051002

[pokoli, 2021-11-30 17:02:10]

We need a changelog entry

Also it will be good to complete the test scenario to test that the value is
correctly computed.

https://codereview.tryton.org/345051002/diff/379701002/view/work_form.xml
File view/work_form.xml (right):

https://codereview.tryton.org/345051002/diff/379701002/view/work_form.xml#new...
view/work_form.xml:22: <label id="empty" colspan="2"/>
The empty label can be removed. So cost will be shown before state.

https://codereview.tryton.org/345051002/diff/379701002/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/379701002/work.py#newcode139
work.py:139: cost = fields.Function(fields.Numeric('Cost', digits=price_digits,
Please use double quotes for human readable strings.

https://codereview.tryton.org/345051002/diff/379701002/work.py#newcode140
work.py:140: readonly=True), 'get_cost')
No need to set readonly of Function field.

https://codereview.tryton.org/345051002/diff/379701002/work.py#newcode253
work.py:253: def get_cost(self, name):
As we are showing the field on the list view I think it will be more performant
to use a classmethod to compute the values using an sql query. 

You should follow the same value structure as the balance computation of account
module: 

https://hg.tryton.org/modules/account/file/e453761ac7c5/account.py#l913

But adapting the query to just sum the values of the cycles grouped for each
work.

[ced, 2021-11-30 23:09:56]

https://codereview.tryton.org/345051002/diff/379701002/view/work_list.xml
File view/work_list.xml (right):

https://codereview.tryton.org/345051002/diff/379701002/view/work_list.xml#new...
view/work_list.xml:8: <field name="cost"/>
I'm not sure it is need on default list but only on the view inside production
order.

https://codereview.tryton.org/345051002/diff/379701002/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/379701002/work.py#newcode253
work.py:253: def get_cost(self, name):
On 2021/11/30 17:02:10, pokoli wrote:
> As we are showing the field on the list view I think it will be more
performant
> to use a classmethod to compute the values using an sql query. 
> 
> You should follow the same value structure as the balance computation of
account
> module: 
> 
> https://hg.tryton.org/modules/account/file/e453761ac7c5/account.py#l913
> 
> But adapting the query to just sum the values of the cycles grouped for each
> work. 

I guess the Production.get_cost is a better example.

Also we must count only cycle in state done.

[acaubet, 2021-12-01 08:59:28]

Use query, Fix ced and pokoli remarks

[reviewbot, 2021-12-01 09:32:45]

flake8 OK
URL: https://codereview.tryton.org/345051002

[ced, 2021-12-13 00:03:51]

https://codereview.tryton.org/345051002/diff/367921003/CHANGELOG
File CHANGELOG (right):

https://codereview.tryton.org/345051002/diff/367921003/CHANGELOG#newcode1
CHANGELOG:1: * Show cost of production works
I think it is better to say: Display cost of work

https://codereview.tryton.org/345051002/diff/367921003/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/367921003/work.py#newcode263
work.py:263: ids = [w.id for w in works]
I think it is better to create the list of ids inside the grouped_slice because
it will be a smaller list.

https://codereview.tryton.org/345051002/diff/367921003/work.py#newcode264
work.py:264: costs = dict((i, Decimal(0)) for i in ids)
It is better to use a defaultdict(Decimal)

https://codereview.tryton.org/345051002/diff/367921003/work.py#newcode269
work.py:269: cycle, condition=(cycle.work == work.id)).select(
It is more readable to put '.select(' on a new line.

https://codereview.tryton.org/345051002/diff/367921003/work.py#newcode273
work.py:273: costs.update(dict(cursor))
no need for dict call

https://codereview.tryton.org/345051002/diff/367921003/work.py#newcode274
work.py:274: for work in works:
I would loop only on costs keys.

[acaubet, 2022-01-21 12:49:00]

Fix ced remarks to improve query

[acaubet, 2022-01-21 12:53:17]

Apply isort

[reviewbot, 2022-01-21 13:13:23]

checks OK
URL: https://codereview.tryton.org/345051002

[acaubet, 2022-01-26 14:48:09]

When costs[cost] is None I recieve the following:

Traceback (most recent call last):
  File "/trytond/wsgi.py", line 117, in dispatch_request
    return endpoint(request, **request.view_args)
  File "/trytond/protocols/dispatcher.py", line 47, in rpc
    return methods.get(request.rpc_method, _dispatch)(
  File "/trytond/wsgi.py", line 84, in auth_required
    return wrapped(*args, **kwargs)
  File "/trytond/protocols/wrappers.py", line 156, in wrapper
    return func(request, pool, *args, **kwargs)
  File "/trytond/protocols/dispatcher.py", line 181, in _dispatch
    result = rpc.result(meth(*c_args, **c_kwargs))
  File "/trytond/model/modelsql.py", line 867, in read
    getter_results = field.get(
  File "/trytond/model/fields/function.py", line 105, in get
    return dict((name, call(name)) for name in names)
  File "/trytond/model/fields/function.py", line 105, in <genexpr>
    return dict((name, call(name)) for name in names)
  File "/trytond/model/fields/function.py", line 98, in call
    return method(records, name)
  File "/production.py", line 544, in get_cost
    costs[cost] = Decimal(str(costs[cost]))
decimal.InvalidOperation: [<class 'decimal.ConversionSyntax'>]

Fault: [<class 'decimal.ConversionSyntax'>]


Don't know which is the best solution but the following fix does the job:
costs[cost] = Decimal(str(costs[cost]))

[pokoli, 2022-01-26 14:50:23]

https://codereview.tryton.org/345051002/diff/376541002/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/376541002/work.py#newcode271
work.py:271: work.id, Sum(cycle.cost),
you should Coalesce the cost to 0 to avoid summing null values: 

Sum(Coalesce(cycle.cost, 0))

[acaubet, 2022-02-03 16:01:11]

Use Coalesce to prevent sum of null values

[acaubet, 2022-02-03 16:03:31]

Reorder view fields

[reviewbot, 2022-02-03 16:14:03]

checks OK
URL: https://codereview.tryton.org/345051002

[ced, 2022-02-16 23:23:27]

I think Production.get_cost should be updated to sum the new cost of its works.

https://codereview.tryton.org/345051002/diff/367921003/CHANGELOG
File CHANGELOG (right):

https://codereview.tryton.org/345051002/diff/367921003/CHANGELOG#newcode1
CHANGELOG:1: * Show cost of production works
On 2021/12/13 00:03:50, ced wrote:
> I think it is better to say: Display cost of work

I still think it is better "Display".

https://codereview.tryton.org/345051002/diff/355981002/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/355981002/work.py#newcode265
work.py:265: costs = defaultdict(lambda: Decimal(0))
could be just: defaultdict(Decimal)

https://codereview.tryton.org/345051002/diff/355981002/work.py#newcode269
work.py:269: cursor.execute(*work.join(
we prefer to put '.join' at the beginning of the line.

https://codereview.tryton.org/345051002/diff/355981002/work.py#newcode273
work.py:273: where=red_sql,
for me it should be limited to cycle done.

[acaubet, 2022-03-22 16:11:12]

Fix ced remarks and update Production.get_cost

[pokoli, 2022-03-22 16:16:48]

https://codereview.tryton.org/345051002/diff/368211002/production.py
File production.py (right):

https://codereview.tryton.org/345051002/diff/368211002/production.py#newcode44
production.py:44: costs = Work.get_cost(self.works)
I think its easier to do:

for work in self.works:
   cost += work.cost

return cost

[reviewbot, 2022-03-22 16:27:04]

checks OK
URL: https://codereview.tryton.org/345051002

[acaubet, 2022-03-23 08:34:04]

Simplify Production.get_cost method

[reviewbot, 2022-03-23 08:37:44]

checks OK
URL: https://codereview.tryton.org/345051002

[pokoli, 2022-03-23 08:40:27]

https://codereview.tryton.org/345051002/diff/368211002/production.py
File production.py (right):

https://codereview.tryton.org/345051002/diff/368211002/production.py#newcode43
production.py:43: cost = super(Production, self).get_cost(name)
super call should not be removed!
Otherwise other modules cost is lost

https://codereview.tryton.org/345051002/diff/399161002/production.py
File production.py (right):

https://codereview.tryton.org/345051002/diff/399161002/production.py#newcode41
production.py:41: cost = Decimal(0)
cost = super().get_cost(name)

[acaubet, 2022-03-23 08:50:21]

Add missed super() call

[reviewbot, 2022-03-23 08:59:48]

checks OK
URL: https://codereview.tryton.org/345051002

[ced, 2022-04-03 17:44:58]

https://codereview.tryton.org/345051002/diff/403091003/view/work_list_sequenc...
File view/work_list_sequence.xml (right):

https://codereview.tryton.org/345051002/diff/403091003/view/work_list_sequenc...
view/work_list_sequence.xml:8: <field name="cost"/>
I would define it as optional="0"

https://codereview.tryton.org/345051002/diff/403091003/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/403091003/work.py#newcode259
work.py:259: def get_cost(cls, works, name=None):
name can not be optional.

https://codereview.tryton.org/345051002/diff/403091003/work.py#newcode271
work.py:271: cycle, condition=(cycle.work == work.id))
could be on the same line as .join

https://codereview.tryton.org/345051002/diff/403091003/work.py#newcode272
work.py:272: .select(
should have the same indentation as .join

[acaubet, 2022-04-05 11:58:20]

Fix identation and optionals

[reviewbot, 2022-04-05 12:07:51]

checks OK
URL: https://codereview.tryton.org/345051002

[ced, 2022-07-16 16:45:51]

It will be good to update the scenario to test the cost computation on work.

[acaubet, 2022-08-11 11:29:58]

On 2022/07/16 16:45:51, ced wrote:
> It will be good to update the scenario to test the cost computation on work.

Sorry I don't understand. Is not this
https://codereview.tryton.org/345051002/diff/390501002/tests/scenario_product...
what you are asking for?

[acaubet, 2022-08-11 11:42:43]

Update to tip

[reviewbot, 2022-08-11 11:46:36]

checks OK
URL: https://codereview.tryton.org/345051002

[ced, 2022-11-27 22:40:24]

On 2022/08/11 11:29:58, acaubet wrote:
> On 2022/07/16 16:45:51, ced wrote:
> > It will be good to update the scenario to test the cost computation on work.
> 
> Sorry I don't understand. Is not this
>
https://codereview.tryton.org/345051002/diff/390501002/tests/scenario_product...
> what you are asking for?

I mean testing on different step when adding cycles.

[ced, 2022-11-27 22:40:29]

https://codereview.tryton.org/345051002/diff/443461003/work.py
File work.py (right):

https://codereview.tryton.org/345051002/diff/443461003/work.py#newcode260
work.py:260: for sub_ids in grouped_slice([w.id for w in works]):
It will be better to slice the instances and after that build the list of ids.