Merge pull request #1095 from ivantodorovich/9.0-fix

[9.0][kpi] FIX #1093

kpi/README.rst

@@ -60,6 +60,7 @@ Contributors
 * Loic Lacroix <loic.lacroix@savoirfairelinux.com>
 * Sandy Carter <sandy.carter@savoirfairelinux.com>
 * Gervais Naoussi <gervaisnaoussi@gmail.com>
+* Iván Todorovich <ivan.todorovich@gmail.com>
 
 Maintainer
 ----------

kpi/__openerp__.py

@@ -4,7 +4,7 @@
 
 {
     "name": "Key Performance Indicator",
-    "version": "9.0.1.0.0",
+    "version": "9.0.1.1.0",
     "author": "Savoir-faire Linux,Odoo Community Association (OCA)",
     "website": "http://www.savoirfairelinux.com",
     "license": "AGPL-3",

kpi/models/kpi_threshold.py

@@ -13,27 +13,25 @@ class KPIThreshold(models.Model):
 
     @api.multi
     def _compute_is_valid_threshold(self):
-        result = {}
         for obj in self:
             # check if ranges overlap
             # TODO: This code can be done better
+            obj.valid = True
             for range1 in obj.range_ids:
-                for range2 in obj.range_ids:
-                    if (range1.valid and range2.valid and
-                            range1.min_value < range2.min_value):
-                        result[obj.id] = range1.max_value <= range2.min_value
-        return result
-
-    @api.multi
-    def _compute_generate_invalid_message(self):
-        result = {}
-        for obj in self:
+                if not range1.valid:
+                    obj.valid = False
+                    break
+                for range2 in (obj.range_ids-range1):
+                    if (range1.max_value >= range2.min_value and
+                            range1.min_value <= range2.max_value):
+                        obj.valid = False
+                        break
             if obj.valid:
-                result[obj.id] = ""
+                obj.invalid_message = None
             else:
-                result[obj.id] = ("Two of your ranges are overlapping. Please "
-                                  "make sure your ranges do not overlap.")
-        return result
+                obj.invalid_message = (
+                    "Some ranges are invalid or overlapping. "
+                    "Please make sure your ranges do not overlap.")
 
     name = fields.Char('Name', size=50, required=True)
     range_ids = fields.Many2many(
@@ -46,7 +44,7 @@ class KPIThreshold(models.Model):
     valid = fields.Boolean(string='Valid', required=True,
                            compute="_compute_is_valid_threshold", default=True)
     invalid_message = fields.Char(string='Message', size=100,
-                                  compute="_compute_generate_invalid_message")
+                                  compute="_compute_is_valid_threshold")
     kpi_ids = fields.One2many('kpi', 'threshold_id', 'KPIs')
     company_id = fields.Many2one(
         'res.company', 'Company',

kpi/models/kpi_threshold_range.py

@@ -46,7 +46,7 @@ class KPIThresholdRange(models.Model):
     valid = fields.Boolean(string='Valid', required=True,
                            compute="_compute_is_valid_range", default=True)
     invalid_message = fields.Char(string='Message', size=100,
-                                  compute="_compute_generate_invalid_message")
+                                  compute="_compute_is_valid_range")
     min_type = fields.Selection((
         ('static', 'Fixed value'),
         ('python', 'Python Code'),
@@ -56,6 +56,7 @@ class KPIThresholdRange(models.Model):
     min_value = fields.Float(string='Minimum', compute="_compute_min_value")
     min_fixed_value = fields.Float('Minimum')
     min_code = fields.Text('Minimum Computation Code')
+    min_error = fields.Char('Error', compute="_compute_min_value")
     min_dbsource_id = fields.Many2one(
         'base.external.dbsource',
         'External DB Source',
@@ -69,6 +70,7 @@ class KPIThresholdRange(models.Model):
     max_value = fields.Float(string='Maximum', compute="_compute_max_value")
     max_fixed_value = fields.Float('Maximum')
     max_code = fields.Text('Maximum Computation Code')
+    max_error = fields.Char('Error', compute="_compute_max_value")
     max_dbsource_id = fields.Many2one(
         'base.external.dbsource',
         'External DB Source',
@@ -92,9 +94,10 @@ class KPIThresholdRange(models.Model):
 
     @api.multi
     def _compute_min_value(self):
-        result = {}
         for obj in self:
             value = None
+            error = None
+            try:
                 if obj.min_type == 'local' and is_sql_or_ddl_statement(
                         obj.min_code):
                     self.env.cr.execute(obj.min_code)
@@ -111,51 +114,53 @@ class KPIThresholdRange(models.Model):
                     value = safe_eval(obj.min_code)
                 else:
                     value = obj.min_fixed_value
+            except Exception as e:
+                value = None
+                error = str(e)
             obj.min_value = value
-        return result
+            obj.min_error = error
 
     @api.multi
     def _compute_max_value(self):
-        result = {}
         for obj in self:
             value = None
+            error = None
+            try:
                 if obj.max_type == 'local' and is_sql_or_ddl_statement(
                         obj.max_code):
                     self.env.cr.execute(obj.max_code)
                     dic = self.env.cr.dictfetchall()
                     if is_one_value(dic):
                         value = dic[0]['value']
-            elif obj.max_type == 'python':
-                value = safe_eval(obj.max_code)
                 elif (obj.max_type == 'external' and obj.max_dbsource_id.id and
                       is_sql_or_ddl_statement(obj.max_code)):
                     dbsrc_obj = obj.max_dbsource_id
                     res = dbsrc_obj.execute(obj.max_code)
                     if is_one_value(res):
                         value = res[0]['value']
+                elif obj.max_type == 'python':
+                    value = safe_eval(obj.max_code)
                 else:
                     value = obj.max_fixed_value
+            except Exception as e:
+                value = None
+                error = str(e)
             obj.max_value = value
-        return result
+            obj.max_error = error
 
     @api.multi
     def _compute_is_valid_range(self):
-        result = {}
         for obj in self:
-            if obj.max_value < obj.min_value:
+            if obj.min_error or obj.max_error:
+                obj.valid = False
+                obj.invalid_message = (
+                    "Either minimum or maximum value has "
+                    "computation errors. Please fix them.")
+            elif obj.max_value < obj.min_value:
                 obj.valid = False
-            else:
-                obj.valid = True
-        return result
-
-    @api.multi
-    def _compute_generate_invalid_message(self):
-        result = {}
-        for obj in self:
-            if obj.valid:
-                obj.invalid_message = ""
-            else:
                 obj.invalid_message = (
                     "Minimum value is greater than the maximum "
                     "value! Please adjust them.")
-        return result
+            else:
+                obj.valid = True
+                obj.invalid_message = ""

kpi/tests/__init__.py

@@ -0,0 +1,4 @@
+# -*- coding: utf-8 -*-
+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+
+from . import test_kpi

kpi/tests/test_kpi.py

@@ -0,0 +1,87 @@
+# -*- coding: utf-8 -*-
+# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
+
+from openerp.tests.common import TransactionCase
+
+
+class TestKPI(TransactionCase):
+
+    def setUp(self):
+        super(TestKPI, self).setUp()
+
+    def test_invalid_threshold_range(self):
+        range1 = self.env['kpi.threshold.range'].create({
+            'name': 'Range1',
+            'min_type': 'static',
+            'max_type': 'static',
+            'min_fixed_value': 3,
+            'max_fixed_value': 1,
+        })
+        range2 = self.env['kpi.threshold.range'].create({
+            'name': 'Range2',
+            'min_type': 'static',
+            'max_type': 'static',
+            'min_fixed_value': 4,
+            'max_fixed_value': 10,
+        })
+        self.assertFalse(range1.valid)
+        self.assertTrue(range2.valid)
+
+    def test_invalid_threshold(self):
+        range1 = self.env['kpi.threshold.range'].create({
+            'name': 'Range1',
+            'min_type': 'static',
+            'max_type': 'static',
+            'min_fixed_value': 1,
+            'max_fixed_value': 4,
+        })
+        range2 = self.env['kpi.threshold.range'].create({
+            'name': 'Range2',
+            'min_type': 'static',
+            'max_type': 'static',
+            'min_fixed_value': 4,
+            'max_fixed_value': 10,
+        })
+        range3 = self.env['kpi.threshold.range'].create({
+            'name': 'Range3',
+            'min_type': 'static',
+            'max_type': 'static',
+            'min_fixed_value': 1,
+            'max_fixed_value': 3,
+        })
+        range_invalid = self.env['kpi.threshold.range'].create({
+            'name': 'RangeInvalid',
+            'min_type': 'static',
+            'max_type': 'static',
+            'min_fixed_value': 3,
+            'max_fixed_value': 1,
+        })
+
+        threshold1 = self.env['kpi.threshold'].create({
+            'name': 'Threshold1',
+            'range_ids': [(6, 0, [range1.id, range2.id])],
+        })
+
+        threshold2 = self.env['kpi.threshold'].create({
+            'name': 'Threshold1',
+            'range_ids': [(6, 0, [range3.id, range2.id])],
+        })
+
+        threshold3 = self.env['kpi.threshold'].create({
+            'name': 'Threshold1',
+            'range_ids': [(6, 0, [range_invalid.id, range2.id])],
+        })
+
+        self.assertFalse(threshold1.valid)
+        self.assertTrue(threshold2.valid)
+        self.assertFalse(threshold3.valid)
+
+    def test_invalid_threshold_range_exception(self):
+        range_error = self.env['kpi.threshold.range'].create({
+            'name': 'RangeError',
+            'min_type': 'python',
+            'min_code': '<Not a valid python expression>',
+            'max_type': 'static',
+            'max_fixed_value': 1,
+        })
+        self.assertFalse(range_error.valid)

kpi/views/kpi.xml

@@ -48,6 +48,7 @@
             <field name="model">kpi</field>
             <field name="arch" type="xml">
                 <form string="Key Performance Indicator">
+                    <sheet>
                         <group col="6" colspan="6">
                             <field name="name" colspan="2"/>
                             <field name="threshold_id" colspan="2"/>
@@ -79,6 +80,7 @@
                                 <field name="description" nolabel="1"/>
                             </page>
                         </notebook>
+                    </sheet>
                 </form>
             </field>
         </record>

kpi/views/kpi_category.xml

@@ -23,11 +23,13 @@
             <field name="model">kpi.category</field>
             <field name="arch" type="xml">
                 <form string="Category">
+                    <sheet>
                         <group col="2" colspan="2">
                             <field name="name" colspan="2"/>
                             <newline/>
                             <field name="description" colspan="2"/>
                         </group>
+                    </sheet>
                 </form>
             </field>
         </record>

kpi/views/kpi_history.xml

@@ -23,6 +23,7 @@
             <field name="model">kpi.history</field>
             <field name="arch" type="xml">
                 <form string="KPI History">
+                    <sheet>
                         <group col="4" colspan="4">
                             <field name="kpi_id"/>
                             <field name="name"/>
@@ -31,6 +32,7 @@
                             <field name="color" widget="color"/>
                             <field name="company_id" groups="base.group_multi_company"/>
                         </group>
+                    </sheet>
                 </form>
             </field>
         </record>

kpi/views/kpi_threshold.xml

@@ -9,7 +9,7 @@
             <field name="name">kpi.threshold.tree</field>
             <field name="model">kpi.threshold</field>
             <field name="arch" type="xml">
-                <tree string="Thresholds">
+                <tree string="Thresholds" decoration-danger="invalid_message">
                     <field name="name"/>
                     <field name="invalid_message"/>
                     <field name="company_id" groups="base.group_multi_company"/>
@@ -22,6 +22,7 @@
             <field name="model">kpi.threshold</field>
             <field name="arch" type="xml">
                 <form string="Threshold">
+                    <sheet>
                         <group col="6" colspan="6">
                             <field name="name" colspan="2"/>
                             <field name="company_id" groups="base.group_multi_company" colspan="2"/>
@@ -34,9 +35,11 @@
                             <newline/>
                             <field name="kpi_ids" nolabel="1" colspan="6"/>
                             <newline/>
-                    <field name="invalid_message" nolabel="1" attrs="{'invisible' : [('invalid_message', '=', '')]}" colspan="2"/>
+                            <separator string="Errors" attrs="{'invisible' : [('invalid_message', '=', False)]}" colspan="4"/>
+                            <field name="invalid_message" nolabel="1" attrs="{'invisible' : [('invalid_message', '=', False)]}" colspan="4"/>
                             <newline/>
                         </group>
+                    </sheet>
                 </form>
             </field>
         </record>

kpi/views/kpi_threshold_range.xml

@@ -9,7 +9,7 @@
             <field name="name">kpi.threshold.range.tree</field>
             <field name="model">kpi.threshold.range</field>
             <field name="arch" type="xml">
-                <tree string="Ranges">
+                <tree string="Ranges" decoration-danger="invalid_message">
                     <field name="name"/>
                     <field name="min_value"/>
                     <field name="max_value"/>
@@ -25,6 +25,7 @@
             <field name="model">kpi.threshold.range</field>
             <field name="arch" type="xml">
                 <form string="Range">
+                    <sheet>
                         <group col="6" colspan="6">
                             <field name="name"/>
                             <field name="color"/>
@@ -37,7 +38,9 @@
                             <field name="min_fixed_value" colspan="2" attrs="{'invisible' : [('min_type', '!=', 'static')]}"/>
                             <field name="min_dbsource_id" colspan="2" attrs="{'invisible' : [('min_type', '!=', 'external')]}"/>
                             <newline/>
-                        <field name="min_code" colspan="4" attrs="{'invisible' : [('min_type', 'NOT IN', ('local','external','python'))]}"/>
+                            <field name="min_code" colspan="6" attrs="{'invisible' : [('min_type', 'NOT IN', ('local','external','python'))]}"/>
+                            <newline/>
+                            <field name="min_error" colspan="6" attrs="{'invisible': [('min_error', '=', False)]}" />
                             <newline/>
                             <separator string="Maximum" colspan="4"/>
                             <newline/>
@@ -45,12 +48,16 @@
                             <field name="max_fixed_value" attrs="{'invisible' : [('max_type', '!=', 'static')]}"/>
                             <field name="max_dbsource_id" attrs="{'invisible' : [('max_type', '!=', 'external')]}"/>
                             <newline/>
-                        <field name="max_code" colspan="4" attrs="{'invisible' : [('max_type', 'NOT IN', ('local','external','python'))]}"/>
+                            <field name="max_code" colspan="6" attrs="{'invisible' : [('max_type', 'NOT IN', ('local','external','python'))]}"/>
+                            <newline/>
+                            <field name="max_error" colspan="6" attrs="{'invisible': [('max_error', '=', False)]}" />
                             <newline/>
                             <separator string="Thresholds" colspan="4"/>
                             <field name="threshold_ids" nolabel="1" colspan="4"/>
-                        <field name="invalid_message" nolabel="1" attrs="{'invisible' : [('invalid_message', '=', '')]}" colspan="4"/>
+                            <separator string="Errors" attrs="{'invisible' : [('invalid_message', '=', False)]}" colspan="4"/>
+                            <field name="invalid_message" nolabel="1" attrs="{'invisible' : [('invalid_message', '=', False)]}" colspan="4"/>
                         </group>
+                    </sheet>
                 </form>
             </field>
         </record>