Merge "Set number of decimal digits in efficacy indicator"

watcher/db/sqlalchemy/alembic/versions/15f7375ca737_change_efficiacy_indicator_decimals.py

@@ -0,0 +1,20 @@
+"""change_efficiacy_indicator_decimals
+
+Revision ID: 15f7375ca737
+Revises: 609bec748f2a
+Create Date: 2025-03-24 10:15:19.269061
+
+"""""
+
+# revision identifiers, used by Alembic.
+revision = '15f7375ca737'
+down_revision = '609bec748f2a'
+
+from alembic import op
+import sqlalchemy as sa
+
+def upgrade():
+    op.add_column('efficacy_indicators',
+                  sa.Column('data', sa.Float())
+                 )
+

watcher/db/sqlalchemy/api.py

@@ -905,15 +905,34 @@ class Connection(api.BaseConnection):
     # ### EFFICACY INDICATORS ### #
 
     def get_efficacy_indicator_list(self, *args, **kwargs):
-        return self._get_model_list(models.EfficacyIndicator,
+        eff_ind_models = self._get_model_list(
-                                    self._add_efficacy_indicators_filters,
+            models.EfficacyIndicator,
-                                    *args, **kwargs)
+            self._add_efficacy_indicators_filters,
+            *args, **kwargs
+            )
+        for indicator in eff_ind_models:
+            if indicator.data is not None:
+                # jgilaber: use the data value since it stores the value
+                # properly, see https://bugs.launchpad.net/watcher/+bug/2103458
+                # for more details
+                indicator.value = indicator.data
+            else:
+                # if data is None, it means that we're reading data from a
+                # database created before the 15f7375ca737 revision, use the
+                # value column
+                indicator.data = indicator.value
+
+        return eff_ind_models
 
     def create_efficacy_indicator(self, values):
         # ensure defaults are present for new efficacy indicators
         if not values.get('uuid'):
             values['uuid'] = utils.generate_uuid()
 
+        # jgilaber: use the data column since it stores the value
+        # properly, see https://bugs.launchpad.net/watcher/+bug/2103458
+        # for more details
+        values['data'] = values.get('value')
         try:
             efficacy_indicator = self._create(models.EfficacyIndicator, values)
         except db_exc.DBDuplicateEntry:
@@ -922,8 +941,22 @@ class Connection(api.BaseConnection):
 
     def _get_efficacy_indicator(self, context, fieldname, value, eager):
         try:
-            return self._get(context, model=models.EfficacyIndicator,
+            efficacy_indicator = self._get(context,
-                             fieldname=fieldname, value=value, eager=eager)
+                                           model=models.EfficacyIndicator,
+                                           fieldname=fieldname,
+                                           value=value, eager=eager)
+
+            if efficacy_indicator.data is not None:
+                # jgilaber: use the data value since it stores the value
+                # properly, see https://bugs.launchpad.net/watcher/+bug/2103458
+                # for more details
+                efficacy_indicator.value = efficacy_indicator.data
+            else:
+                # if data is None, it means that we're reading data from a
+                # database created before the 15f7375ca737 revision, use the
+                # value column
+                efficacy_indicator.data = efficacy_indicator.value
+            return efficacy_indicator
         except exception.ResourceNotFound:
             raise exception.EfficacyIndicatorNotFound(efficacy_indicator=value)
 

watcher/db/sqlalchemy/models.py

@@ -238,7 +238,10 @@ class EfficacyIndicator(Base):
     name = Column(String(63))
     description = Column(String(255), nullable=True)
     unit = Column(String(63), nullable=True)
+    # this column is deprecated due to bug
+    # https://bugs.launchpad.net/watcher/+bug/2103458
     value = Column(Numeric())
+    data = Column(Float())
     action_plan_id = Column(Integer, ForeignKey('action_plans.id'),
                             nullable=False)
 

watcher/tests/db/test_efficacy_indicator.py

@@ -452,12 +452,8 @@ class MySQLDbEfficacyIndicatorTestCase(base.MySQLDbTestCase):
                 action_plan_id=self.action_plan.id, id=3, uuid=None,
                 name="efficacy_indicator3", description="Test Indicator 3")
 
-    def test_efficacy_indicator_value_decimals(self):
+    def test_efficacy_indicator_value(self):
         db_efficacy_indicator = self.dbapi.get_efficacy_indicator_by_id(
             self.context, 1)
-        self.assertAlmostEqual(float(db_efficacy_indicator.value),
+        self.assertAlmostEqual(db_efficacy_indicator.value, 0.012, places=3)
-                               0.00, places=2)
+        self.assertAlmostEqual(db_efficacy_indicator.data, 0.012, places=3)
-        # FIXME: once the database bug is fixed check that the value is stored
-        # correctly
-        # self.assertAlmostEqual(float(db_efficacy_indicator.value),
-        #                        0.01, places=2)

watcher/tests/db/test_migrations.py

@@ -0,0 +1,205 @@
+# Copyright 2025 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+#
+
+""""Tests for database migration for Watcher.
+
+These are "opportunistic" tests which allow testing against all three databases
+(sqlite in memory, mysql, pg) in a properly configured unit test environment.
+
+For the opportunistic testing you need to set up DBs named 'openstack_citest'
+with user 'openstack_citest' and password 'openstack_citest' on localhost. The
+test will then use that DB and username/password combo to run the tests. Refer
+to the 'tools/test-setup.sh' for an example of how to configure this.
+"""
+
+from alembic import command as alembic_api
+from alembic.script import ScriptDirectory
+from oslo_config import cfg
+from oslo_db.sqlalchemy import enginefacade
+from oslo_db.sqlalchemy import test_fixtures
+from oslo_log import log as logging
+import sqlalchemy
+
+from watcher.common import utils as w_utils
+from watcher.db import api as dbapi
+from watcher.db.sqlalchemy import migration
+from watcher.tests import base
+from watcher.tests.db import utils
+
+LOG = logging.getLogger(__name__)
+
+
+class MySQLDbMigrationsTestCase(test_fixtures.OpportunisticDBTestMixin,
+                                base.TestCase):
+
+    FIXTURE = test_fixtures.MySQLOpportunisticFixture
+
+    def setUp(self):
+        conn_str = "mysql+pymysql://root:insecure_slave@127.0.0.1"
+        # to use mysql db
+        cfg.CONF.set_override("connection", conn_str,
+                              group="database")
+        super().setUp()
+        self.engine = enginefacade.writer.get_engine()
+        self.dbapi = dbapi.get_instance()
+        self.alembic_config = migration._alembic_config()
+        self.revisions_tested = set(["15f7375ca737"])
+
+    def _apply_migration(self, connection, revision):
+        if revision not in self.revisions_tested:
+            # if we don't have tests for this version, just upgrade to it
+            alembic_api.upgrade(self.alembic_config, revision)
+            return
+
+        pre_upgrade = getattr(self, f"_pre_upgrade_{revision}", None)
+        if pre_upgrade:
+            pre_upgrade(connection)
+
+        alembic_api.upgrade(self.alembic_config, revision)
+
+        post_upgrade = getattr(self, f"_check_{revision}", None)
+        if post_upgrade:
+            post_upgrade(connection)
+
+    def _pre_upgrade_15f7375ca737(self, connection):
+        inspector = sqlalchemy.inspect(connection)
+        columns = inspector.get_columns("efficacy_indicators")
+        for column in columns:
+            if column['name'] != "value":
+                continue
+            value_type = column['type']
+            self.assertIsInstance(value_type, sqlalchemy.Numeric)
+            self.assertEqual(value_type.scale, 0)
+            self.assertEqual(value_type.precision, 10)
+
+    def _check_15f7375ca737(self, connection):
+        inspector = sqlalchemy.inspect(connection)
+        columns = inspector.get_columns("efficacy_indicators")
+        for column in columns:
+            if column['name'] == "value":
+                value_type = column['type']
+                self.assertIsInstance(value_type, sqlalchemy.Numeric)
+                self.assertEqual(value_type.scale, 0)
+                self.assertEqual(value_type.precision, 10)
+            elif column['name'] == "data":
+                value_type = column['type']
+                self.assertIsInstance(value_type, sqlalchemy.Float)
+
+    def test_migration_revisions(self):
+        with self.engine.begin() as connection:
+            self.alembic_config.attributes["connection"] = connection
+            script = ScriptDirectory.from_config(self.alembic_config)
+            revisions = [x.revision for x in script.walk_revisions()]
+
+            # for some reason, 'walk_revisions' gives us the revisions in
+            # reverse chronological order so we have to invert this
+            revisions.reverse()
+
+            for revision in revisions:
+                LOG.info('Testing revision %s', revision)
+                self._apply_migration(connection, revision)
+
+
+class MySQLDbDataMigrationsTestCase(MySQLDbMigrationsTestCase):
+
+    def _create_manual_efficacy_indicator(self, connection, **kwargs):
+        eff_ind_values = utils.get_test_efficacy_indicator(**kwargs)
+        metadata = sqlalchemy.MetaData()
+        metadata.reflect(bind=self.engine)
+        eff_ind_table = sqlalchemy.Table('efficacy_indicators', metadata)
+        connection.execute(eff_ind_table.insert(), eff_ind_values)
+        connection.commit()
+
+    def _pre_upgrade_15f7375ca737(self, connection):
+        """Add data to the database before applying the 15f7375ca737 revision.
+
+        This data will then be checked after applying the revision to ensure it
+        was not affected by the db upgrade.
+
+        """
+        self.goal = utils.create_test_goal(
+            id=1, uuid=w_utils.generate_uuid(),
+            name="GOAL_1", display_name='Goal 1')
+        self.strategy = utils.create_test_strategy(
+            id=1, uuid=w_utils.generate_uuid(),
+            name="STRATEGY_ID_1", display_name='My Strategy 1')
+        self.audit_template = utils.create_test_audit_template(
+            name="Audit Template", id=1, uuid=None)
+        self.audit = utils.create_test_audit(
+            audit_template_id=self.audit_template.id, id=1, uuid=None,
+            name="AUDIT_1")
+        self.action_plan = utils.create_test_action_plan(
+            audit_id=self.audit.id, id=1, uuid=None)
+
+        self._create_manual_efficacy_indicator(
+            connection,
+            action_plan_id=self.action_plan.id, id=1, uuid=None,
+            name="efficacy_indicator1", description="Test Indicator 1",
+            value=1.01234567912345678)
+
+    def _check_15f7375ca737(self, connection):
+        """Check data integrity after the database migration."""
+        # check that creating a new efficacy_indicator after the migration
+        # works
+        utils.create_test_efficacy_indicator(
+            action_plan_id=self.action_plan.id, id=2, uuid=None,
+            name="efficacy_indicator1", description="Test Indicator 2",
+            value=0.01234567912345678)
+        db_efficacy_indicator = self.dbapi.get_efficacy_indicator_by_id(
+            self.context, 2)
+        self.assertAlmostEqual(db_efficacy_indicator.value, 0.012, places=3)
+        # check that getting the efficacy_indicator using the
+        # get_efficacy_indicator_list method reports the correct values
+        efficacy_indicators = self.dbapi.get_efficacy_indicator_list(
+            self.context)
+        self.assertEqual(len(efficacy_indicators), 2)
+        self.assertEqual(efficacy_indicators[0].id, 1)
+        self.assertAlmostEqual(efficacy_indicators[0].value,
+                               1.00, places=3)
+
+        self.assertEqual(efficacy_indicators[1].id, 2)
+        self.assertAlmostEqual(efficacy_indicators[1].value,
+                               0.012, places=3)
+        # check that the existing data is there after the migration
+        db_goal = self.dbapi.get_goal_by_id(self.context, 1)
+        self.assertEqual(db_goal.name, "GOAL_1")
+        db_strategy = self.dbapi.get_strategy_by_id(self.context, 1)
+        self.assertEqual(db_strategy.name, "STRATEGY_ID_1")
+        db_audit_template = self.dbapi.get_audit_template_by_id(
+            self.context, 1)
+        self.assertEqual(db_audit_template.name, "Audit Template")
+        db_audit = self.dbapi.get_audit_by_id(self.context, 1)
+        self.assertEqual(db_audit.name, "AUDIT_1")
+        db_efficacy_indicator_1 = self.dbapi.get_efficacy_indicator_by_id(
+            self.context, 1)
+        self.assertAlmostEqual(db_efficacy_indicator_1.data,
+                               1.00, places=2)
+        self.assertAlmostEqual(db_efficacy_indicator_1.value,
+                               1.00, places=2)
+        self.assertEqual(db_efficacy_indicator_1.name, "efficacy_indicator1")
+
+    def test_migration_revisions(self):
+        with self.engine.begin() as connection:
+            self.alembic_config.attributes["connection"] = connection
+            script = ScriptDirectory.from_config(self.alembic_config)
+            revisions = [x.revision for x in script.walk_revisions()]
+
+            # for some reason, 'walk_revisions' gives us the revisions in
+            # reverse chronological order so we have to invert this
+            revisions.reverse()
+
+            for revision in revisions:
+                LOG.info('Testing revision %s', revision)
+                self._apply_migration(connection, revision)