[FIX] mrp: dont compute oee with intermediary rounding

[ethanrobv]

Current behavior:
The form view for a workcenter has an OEE smart button which can display a different value from the real OEE displayed by the mrp_workcenter_productivity_report_oee displayed when actually clicking the button and looking at the report.

Expected behavior:
Same values

Steps to reproduce:

1. Make a workcenter and a BoM with an operation performed at the workcenter

2. Use the BoM in an MO such that there is some un-productive time (e.g., recorded production duration takes longer than expected duration)

• example: 0:20 expected, 1:01 actual

3. Go to the workcenter list view -> click on the created workcenter -> look at OEE smart button display value -> click on it to see report -> report values are different

Cause of the issue:
the oee field on the workcenter is computed with rounded intermediary blocked_time and productive_time values, the actual report uses the raw values.

Fix:
Don't use the rounded intermediary values in computing oee.

Post-this-diff, we actually do one less _read_group (along with computing a more accurate field value).

opw-4795463

[robodoo]

[lase-odoo]

Thanks for the fix and test !

I left a few comments concerning details of the technical aspect of the fix :)

I totally agree with the direction of the fix since it only improves the accuracy of the computed value. However, I am wondering if such a change is really suitable for 16.0 which we will not support in 3 months. My issues are that:

1. The rounding issue only ever happen in cases that the cumulative duration of the mrp.workcenter.productivity is not suitable to be rounded in hours (e.g. is two smalls like close to the minutes and not to the hour), the use cases were this improved computation is really relevant might be a bit niche.
2. Since the oee value is only used in the kanban view and on that smart button for display purposes, the issue does not seem blocking/too impacting.

I personally think it is still worth fixing as the change does not seem that risky but I leave the decision to the stock team oif they want to retarget higher versions !

[lase-odoo]

The computations of productive_time and blocked_time do not need to sum as they are both fields are floats fields (and hence will be aggregated using the sum by the read_group:

odoo/odoo/fields.py

Line 1515 in fd5fe84

group_operator = 'sum'

I therefore guess that you wrote this sum with an if condition to filter out the record with the appropriate loss_type. Wouldn't it be clearer to access the value directly define the blocked_time as such (just as in the compute method of blocked_time) ?

It would also highlight that your on "hand" computation of the productive_time and of the blocked_time does the exact same as the regular one except that it keeps the raw value for your computation of the oee.

[ethanrobv]

No, we need to sum because we get both the productive and blocked (non-productive) time in the same _read_group()

[clesgow]

Suggested change

	time_data = self.env['mrp.workcenter.productivity'].read_group(
	domain=[
	('date_start', '>=', fields.Datetime.to_string(datetime.now() - relativedelta.relativedelta(months=1))),
	('workcenter_id', 'in', self.ids),
	('date_end', '!=', False),
	],
	fields=['workcenter_id', 'loss_type', 'duration'],
	groupby=['workcenter_id', 'loss_type'],
	lazy=False,
	)
	time_data = self.env['mrp.workcenter.productivity']._read_group(
	domain=[
	('date_start', '>=', fields.Datetime.to_string(datetime.now() - relativedelta.relativedelta(months=1))),
	('workcenter_id', 'in', self.ids),
	('date_end', '!=', False),
	],
	groupby=['workcenter_id', 'loss_type'],
	aggregates=['duration:sum'],
	)

As a rule of thumb, always prefer _read_group. More details on the why here.

[clesgow]

Suggested change

	for order in self:
	for workcenter in self:

Pet peeve of mine, but since we're rewriting the compute anyway... Using order used here is misleading.

[clesgow]

Thanks for the fix!
@robodoo r+

[robodoo]

@ethanrobv @clesgow 'ci/runbot' failed on this reviewed PR.

[clesgow]

@robodoo r+