From 11e811cc4b18fe7b1b6dd1240acc9d6414145ce5 Mon Sep 17 00:00:00 2001 From: Pierre Goiffon Date: Tue, 19 Apr 2022 17:13:18 +0200 Subject: [PATCH] =?UTF-8?q?N=C2=B03717=20Improve=20iTop=20object=20history?= =?UTF-8?q?=20API=20(#192)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a major flaw in the history API that was causing "phantom" CMDBChange records (without any CMDBChangeOp attached). That was happening especially in iProcess impl. For example this lead to the creation of the combodo-cmdbchange-cleaner module in the Mail To Ticket extension. The modifications in detail : - We can now pass a non persisted CMDBChange instance to \CMDBObject::SetCurrentChange - No persistence done in \CMDBObject::CreateChange anymore - Persistence of the attached CMDChange will be done if necessary in CMDBChangeOp::OnInsert - New CMDBObject::SetCurrentChangeFromParams helper method to ease resetting the current change --- core/cmdbchangeop.class.inc.php | 22 ++++-- core/cmdbobject.class.inc.php | 41 ++++++++--- datamodels/2.x/combodo-db-tools/dbtools.php | 4 +- test/core/CMDBObjectTest.php | 75 +++++++++++++++++++++ webservices/cron.php | 7 +- 5 files changed, 124 insertions(+), 25 deletions(-) create mode 100644 test/core/CMDBObjectTest.php diff --git a/core/cmdbchangeop.class.inc.php b/core/cmdbchangeop.class.inc.php index b1b015516..aeb94bf7a 100644 --- a/core/cmdbchangeop.class.inc.php +++ b/core/cmdbchangeop.class.inc.php @@ -63,22 +63,30 @@ class CMDBChangeOp extends DBObject /** * Describe (as a text string) the modifications corresponding to this change - */ + */ public function GetDescription() { return ''; } /** - * Safety net: in case the change is not given, let's guarantee that it will - * be set to the current ongoing change (or create a new one) - */ + * Safety net: + * * if change isn't persisted yet, use the current change and persist it if needed + * * in case the change is not given, let's guarantee that it will be set to the current ongoing change (or create a new one) + * + * @since 2.7.7 3.0.2 3.1.0 N°3717 do persist the current change if needed + */ protected function OnInsert() { - if ($this->Get('change') <= 0) - { - $this->Set('change', CMDBObject::GetCurrentChange()); + $iChange = $this->Get('change'); + if (($iChange <= 0) || (is_null($iChange))) { + $oChange = CMDBObject::GetCurrentChange(); + if ($oChange->IsNew()) { + $oChange->DBWrite(); + } + $this->Set('change', $oChange); } + parent::OnInsert(); } } diff --git a/core/cmdbobject.class.inc.php b/core/cmdbobject.class.inc.php index ce8acf0d6..e5d1a4b2f 100644 --- a/core/cmdbobject.class.inc.php +++ b/core/cmdbobject.class.inc.php @@ -114,6 +114,26 @@ abstract class CMDBObject extends DBObject self::$m_oCurrChange = $oChange; } + /** + * @param string $sUserInfo + * @param string $sOrigin + * @param \DateTime $oDate + * + * @throws \CoreException + * + * @since 2.7.7 3.0.2 3.1.0 N°3717 new method to reset current change + */ + public static function SetCurrentChangeFromParams($sUserInfo, $sOrigin = null, $oDate = null) + { + static::SetTrackInfo($sUserInfo); + static::SetTrackOrigin($sOrigin); + static::CreateChange(); + + if (!is_null($oDate)) { + static::$m_oCurrChange->Set("date", $oDate); + } + } + // // Todo: simplify the APIs and do not pass the current change as an argument anymore // SetTrackInfo to be invoked in very few cases (UI.php, CSV import, Data synchro) @@ -145,6 +165,8 @@ abstract class CMDBObject extends DBObject * $oMyChange->Set("userinfo", 'this is done by ... for ...'); * $iChangeId = $oMyChange->DBInsert(); * + * **warning** : this will do nothing if current change already exists ! + * * @see SetCurrentChange to specify a CMDBObject instance instead * * @param string $sInfo @@ -157,6 +179,8 @@ abstract class CMDBObject extends DBObject /** * Provides information about the origin of the change * + * **warning** : this will do nothing if current change already exists ! + * * @see SetTrackInfo * @see SetCurrentChange to specify a CMDBObject instance instead * @@ -167,18 +191,15 @@ abstract class CMDBObject extends DBObject { self::$m_sOrigin = $sOrigin; } - + /** * Get the additional information (defaulting to user name) - */ - protected static function GetTrackInfo() + */ + public static function GetTrackInfo() { - if (is_null(self::$m_sInfo)) - { + if (is_null(self::$m_sInfo)) { return CMDBChange::GetCurrentUserName(); - } - else - { + } else { return self::$m_sInfo; } } @@ -201,7 +222,8 @@ abstract class CMDBObject extends DBObject /** * Set to {@link $m_oCurrChange} a standard change record (done here 99% of the time, and nearly once per page) * - * The CMDBChange is persisted so that it has a key > 0, and any new CMDBChangeOp can link to it + * @since 2.7.7 3.0.2 3.1.0 N°3717 {@see CMDBChange} **will be persisted later** in {@see \CMDBChangeOp::OnInsert} (was done previously directly here) + * This will avoid creating in DB CMDBChange lines without any corresponding CMDBChangeOp */ protected static function CreateChange() { @@ -209,7 +231,6 @@ abstract class CMDBObject extends DBObject self::$m_oCurrChange->Set("date", time()); self::$m_oCurrChange->Set("userinfo", self::GetTrackInfo()); self::$m_oCurrChange->Set("origin", self::GetTrackOrigin()); - self::$m_oCurrChange->DBInsert(); } /** diff --git a/datamodels/2.x/combodo-db-tools/dbtools.php b/datamodels/2.x/combodo-db-tools/dbtools.php index b0289d4c9..11dddd82e 100644 --- a/datamodels/2.x/combodo-db-tools/dbtools.php +++ b/datamodels/2.x/combodo-db-tools/dbtools.php @@ -301,9 +301,7 @@ function DisplayLostAttachments(iTopWebPage &$oP, ApplicationContext &$oAppConte $sHistoryEntry = Dict::Format('DBTools:LostAttachments:History', $oOrmDocument->GetFileName()); CMDBObject::SetTrackInfo(UserRights::GetUserFriendlyName()); $oChangeOp = MetaModel::NewObject('CMDBChangeOpPlugin'); - /** @var \Change $oChange */ - $oChange = CMDBObject::GetCurrentChange(); - $oChangeOp->Set('change', $oChange->GetKey()); + // CMDBChangeOp.change will be automatically filled $oChangeOp->Set('objclass', $sTargetClass); $oChangeOp->Set('objkey', $sTargetId); $oChangeOp->Set('description', $sHistoryEntry); diff --git a/test/core/CMDBObjectTest.php b/test/core/CMDBObjectTest.php new file mode 100644 index 000000000..237b51ca4 --- /dev/null +++ b/test/core/CMDBObjectTest.php @@ -0,0 +1,75 @@ +Set('name', 'PHPUnit test'); + $oTestObject->Set('org_id', 1); + $oTestObject->Set('url', 'https://www.combodo.com'); + $oTestObject->DBWrite(); + self::assertFalse(CMDBObject::GetCurrentChange()->IsNew(), 'TrackInfo : Current change persisted'); + self::assertEquals($sTrackInfo, CMDBObject::GetCurrentChange()->Get('userinfo'), + 'TrackInfo : current change created with expected trackinfo'); + + //-- new object with non persisted current change + $sTrackInfo2 = $sTrackInfo.'_2'; + /** @var \CMDBChange $oCustomChange */ + $oCustomChange = MetaModel::NewObject('CMDBChange'); + $oCustomChange->Set('date', time()); + $oCustomChange->Set('userinfo', $sTrackInfo2); + CMDBObject::SetCurrentChange($oCustomChange); + $oTestObject->Set('url', 'https://fr.wikipedia.org'); + $oTestObject->DBUpdate(); + self::assertFalse(CMDBObject::GetCurrentChange()->IsNew(), 'SetCurrentChange : Current change persisted'); + self::assertEquals($sTrackInfo2, CMDBObject::GetCurrentChange()->Get('userinfo'), + 'SetCurrentChange : current change created with expected trackinfo'); + + //-- new object with current change init using helper method + $sTrackInfo3 = $sTrackInfo.'_3'; + CMDBObject::SetCurrentChangeFromParams($sTrackInfo3); + $oTestObject->Set('url', 'https://en.wikipedia.org'); + $oTestObject->DBUpdate(); + self::assertFalse(CMDBObject::GetCurrentChange()->IsNew(), 'SetCurrentChangeFromParams : Current change persisted'); + self::assertEquals($sTrackInfo3, CMDBObject::GetCurrentChange()->Get('userinfo'), + 'SetCurrentChangeFromParams : current change created with expected trackinfo'); + + // restore initial conditions + $oTestObject->DBDelete(); + CMDBObject::SetCurrentChange($oInitialCurrentChange); + CMDBObject::SetTrackInfo($sInitialTrackInfo); + } +} \ No newline at end of file diff --git a/webservices/cron.php b/webservices/cron.php index 980085fd5..ea3e45cb6 100644 --- a/webservices/cron.php +++ b/webservices/cron.php @@ -266,12 +266,9 @@ function CronExec($oP, $aProcesses, $bVerbose, $bDebug=false) // N°3219 for each process will use a specific CMDBChange object with a specific track info // Any BackgroundProcess can overrides this as needed - CMDBObject::SetCurrentChange(null); - CMDBObject::SetTrackInfo("Background task ($sTaskClass)"); - CMDBObject::SetTrackOrigin(null); + CMDBObject::SetCurrentChangeFromParams("Background task ($sTaskClass)"); - if (!array_key_exists($sTaskClass, $aTasks)) - { + if (!array_key_exists($sTaskClass, $aTasks)) { // New entry, let's create a new BackgroundTask record, and plan the first execution $oTask = new BackgroundTask(); $oTask->SetDebug($bDebug);