[ADD] maintenance_plan_meter_mode by lmiguens-solvos · Pull Request #4 · solvosci/slv-maintenance

lmiguens-solvos · 2020-06-12T11:19:13Z

Maintenance plan by self-increasing meter.
Adds new mode: meter mode.
When meter mode is true you have to configure autoincrement params.

dalonsod

Only code review. Some minor changes and a bug overriding a method.
When comments attended, I'll functionally test it.

Looks great!!

maintenance_plan_meter_mode/models/maintenance.py

maintenance_plan_meter_mode/views/maintenance_views.xml

maintenance_plan_meter_mode/models/maintenance.py

maintenance_plan_meter_mode/views/maintenance_views.xml

maintenance_plan_meter_mode/models/maintenance.py

dalonsod · 2020-06-15T17:07:05Z

Made first functionally test, I've found the following problem. Steps to reproduce:

Go to an equipment with plans (e.g. "HP Inkjet printer").
By the way, the maintenance plan grid view inside "Maintenance" tab hasn't any meter mode fields.
Add a self-increase mode plan
Save => an error is fired

File "c:\users\david\solvos\desarrollo\pycharmprojects\odoo-12-iti\addons-gmao2\maintenance\maintenance_plan\models\maintenance_equipment.py", line 127, in _compute_next_maintenance
    next_unplanned_dates)
TypeError: unorderable types: bool() < datetime.date()

This error underlies an unexpected problem: since we're defining a kind of plan that does not actually require a next_maintenance_date, that field is not filled (False). When _compute_next_maintenance is fired (https://github.com/OCA/maintenance/blob/12.0/maintenance_plan/models/maintenance_equipment.py#L111) in order to fill next_action_date for the related equipment, the above error is thrown (fails comparing existing next_maintenance_date of other plans with the False one). So, we should always fill next_maintenance_date for this kind of plans, with a convienient value (an estimated one, or such as 12/31/9999, if necessary).

lmiguens-solvos · 2020-06-16T15:27:51Z

I think the problem is in:
https://github.com/OCA/maintenance/blob/12.0/maintenance_plan/models/maintenance_plan.py#L106
Because this only processes the plans with interval> 0

dalonsod · 2020-06-16T15:41:10Z

I don´t understand in:
https://github.com/OCA/maintenance/blob/12.0/maintenance_plan/models/maintenance_plan.py#L106
it´s a different code of _compute_next_maintenance.
This is the code that I used in my addon, although the error you indicate also occurs.

They are different methods, fired at different events on different models. Unfortunately, both methods have the same name:

You refer _compute_next_maintenance for maintenance.plan, that is the compute method for next_maintenance_date field of a plan. This field show the next date with a planned maintenance request related to the current plan.
I refer _compute_next_maintenance for maintenance.equipment, that is the compute method for next_action_date field of an equipment. This field shows, for an equipment, the next date with a planned maintenance request, plan independent. This method is originally defined in the maintenance Odoo addon (https://github.com/odoo/odoo/blob/12.0/addons/maintenance/models/maintenance.py#L153) and later completely redefined in OCA maintenance_plan addon (the link I've provided).

dalonsod · 2020-06-18T17:35:03Z

@lmiguens-solvos I've made some changes in order to get our own _compute_next_maintenance method for a plan.
Could you test? I made a little test and it appears to work.

And don't forget adding some missing fields to the maintenance plan grid view inside "Maintenance" tab!

dalonsod · 2020-08-19T11:44:25Z

@lmiguens-solvos I've added demo data and tests to the module. Maybe you could take a look at the test code and run it.

For achieving it, you'd need to run Odoo updating the module and adding the proper flag (-u maintenance_plan_meter_mode --test-enable). When started running these lines or similar should be displayed on the console:

2020-08-19 13:11:07,946 14100 INFO Slv_O12_da_gmao2_03 odoo.modules.module: odoo.addons.maintenance_plan_meter_mode.tests.test_maintenance_plan_meter_mode running tests. 
2020-08-19 13:11:07,948 14100 INFO Slv_O12_da_gmao2_03 odoo.addons.maintenance_plan_meter_mode.tests.test_maintenance_plan_meter_mode: test_meter_mode (odoo.addons.maintenance_plan_meter_mode.tests.test_maintenance_plan_meter_mode.TestMaintenancePlan) 
2020-08-19 13:13:50,383 14100 INFO Slv_O12_da_gmao2_03 odoo.addons.maintenance_plan_meter_mode.tests.test_maintenance_plan_meter_mode: Ran 1 test in 162.436s 
2020-08-19 13:13:50,383 14100 INFO Slv_O12_da_gmao2_03 odoo.addons.maintenance_plan_meter_mode.tests.test_maintenance_plan_meter_mode: OK 
2020-08-19 13:13:50,383 14100 INFO Slv_O12_da_gmao2_03 odoo.modules.module: odoo.addons.maintenance_plan_meter_mode.tests.test_maintenance_plan_meter_mode tested in 162.44s, 801 queries

, or the detailed errors if test fails.

lmiguens-solvos · 2020-08-21T11:52:37Z

@dalonsod Demo data and tests looks good to me.
I ran the update with test-enable and they worked fine. I also forced a bug and saw it.

dalonsod · 2020-08-21T13:04:27Z

@lmiguens-solvos great!

@lromero-solvos we need your review!

dalonsod

See comments

dalonsod · 2020-09-08T14:59:36Z

maintenance_plan_meter_mode/models/maintenance.py

+    @api.onchange('meter_autoinc_mode')
+    def _onchange_meter_mode(self):
+        if self.meter_autoinc_mode:
+            self.meter_autoinc_value = None


The suggested code is actually the same, but maybe clearer (coherent with cron recordset selection domain)

Suggested change

self.meter_autoinc_value = None

self.meter_autoinc_value = 0

dalonsod · 2020-09-08T15:00:42Z

maintenance_plan_meter_mode/models/maintenance.py

            values['maintenance_plan_horizon'] = None
            values['planning_step'] = None
+            if meter_autoinc_mode:
+                values['meter_autoinc_value'] = None


The same as above

Suggested change

values['meter_autoinc_value'] = None

values['meter_autoinc_value'] = 0

dalonsod · 2020-09-08T15:16:41Z

maintenance_plan_meter_mode/models/maintenance.py

+    meter_autoinc_mode = fields.Boolean(
+        string='Manual or External auto increment')


Since it's not a selection field, I think it's better a short description with the True (checked) option. In the frontend it's a little bit confusing "Manual or External" for a check, actually is "Self-increased vs Only Manual" (and "Manual" can be through frontend or from and external access e.g. RPC):

Suggested change

meter_autoinc_mode = fields.Boolean(

string='Manual or External auto increment')

meter_autoinc_mode = fields.Boolean(

string='Auto incremental mode',

help='If checked, Current value is periodically incremented according a self-increase value; otherwise only manual changes of Current value are allowed')

dalonsod · 2020-09-08T15:19:11Z

maintenance_plan_meter_mode/views/maintenance_views.xml

-                <field name="meter_autoinc_lastvalue"/>
+            </xpath>
+            <xpath expr="//notebook" position="inside">
+                <page string="Maintenance Meter Mode">


what do you think? In fact, I don't like "Maintenance" for the other tab...

Suggested change

<page string="Maintenance Meter Mode">

<page string="Meter Mode Plans">

dalonsod · 2020-09-08T15:19:55Z

maintenance_plan_meter_mode/views/maintenance_views.xml

+                            <tree string="Meter mode Plans">
+                                <field name="maintenance_kind_id"/>
+                                <field name="name"/>
+                                <field name="meter_mode"/>


This field could be removed from the view, only meter mode plans are actually shown

dalonsod · 2020-09-08T15:22:41Z

maintenance_plan_meter_mode/views/maintenance_views.xml

+                               attrs="{'required':[('meter_mode', '=', True),('meter_autoinc_mode', '=', False)]
+                               ,'readonly':[('meter_mode', '=', False)]}"/>


I think better invisible than readonly, the same for the other field:

Suggested change

attrs="{'required':[('meter_mode', '=', True),('meter_autoinc_mode', '=', False)]

,'readonly':[('meter_mode', '=', False)]}"/>

attrs="{'required':[('meter_mode', '=', True),('meter_autoinc_mode', '=', False)]

,'invisible':[('meter_mode', '=', False)]}"/>

dalonsod · 2020-09-08T15:59:54Z

The new meter_autoinc_mode adds an issue to cron management:

        for plan in self.env['maintenance.plan'].search(
                [('meter_mode', '=', True), ('meter_autoinc_value', '>', 0)]):

The meter_autoinc_value condition should be removed: if we have a meter_autoinc_mode=False plan, no requests will be generated.

lmiguens-solvos requested review from dalonsod and lromero-solvos June 12, 2020 11:19

dalonsod requested changes Jun 12, 2020

View reviewed changes

dalonsod force-pushed the 12.0-add-maintenance_plan_meter_mode branch from e39ac50 to a4a7819 Compare August 19, 2020 11:40

dalonsod approved these changes Aug 21, 2020

View reviewed changes

dalonsod requested changes Sep 8, 2020

View reviewed changes

[ADD] maintenance_plan_meter_mode

251b33d

dalonsod force-pushed the 12.0-add-maintenance_plan_meter_mode branch from 9d7f03a to 251b33d Compare September 14, 2020 11:03

	values['meter_autoinc_value'] = None
	values['meter_autoinc_value'] = 0

		meter_autoinc_mode = fields.Boolean(
		string='Manual or External auto increment')

	<page string="Maintenance Meter Mode">
	<page string="Meter Mode Plans">

		attrs="{'required':[('meter_mode', '=', True),('meter_autoinc_mode', '=', False)]
		,'readonly':[('meter_mode', '=', False)]}"/>

Conversation

lmiguens-solvos commented Jun 12, 2020

Uh oh!

dalonsod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dalonsod commented Jun 15, 2020

Uh oh!

lmiguens-solvos commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dalonsod commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dalonsod commented Jun 18, 2020

Uh oh!

dalonsod commented Aug 19, 2020

Uh oh!

lmiguens-solvos commented Aug 21, 2020

Uh oh!

dalonsod commented Aug 21, 2020

Uh oh!

dalonsod left a comment

Choose a reason for hiding this comment

Uh oh!

dalonsod Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dalonsod Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dalonsod Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dalonsod Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dalonsod Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

dalonsod Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalonsod commented Sep 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lmiguens-solvos commented Jun 16, 2020 •

edited

Loading

dalonsod commented Jun 16, 2020 •

edited

Loading

dalonsod Sep 8, 2020 •

edited

Loading