From c1b5b53a16e8e4c2a53448bf2eede5e4e1e0c627 Mon Sep 17 00:00:00 2001 From: Duncan Sommerville Date: Wed, 10 Jun 2015 17:11:42 -0400 Subject: [PATCH] Fix merge errors; use Celery status messages + handle fail case better --- .../services/SoundcloudService.php | 16 ++++++++--- .../services/ThirdPartyService.php | 27 +++++++++++-------- airtime_mvc/application/upgrade/Upgrades.php | 10 +++---- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/airtime_mvc/application/services/SoundcloudService.php b/airtime_mvc/application/services/SoundcloudService.php index d2f0f0dc8..27003cac7 100644 --- a/airtime_mvc/application/services/SoundcloudService.php +++ b/airtime_mvc/application/services/SoundcloudService.php @@ -83,15 +83,16 @@ class SoundcloudService extends ThirdPartyService { /** * Update a ThirdPartyTrackReferences object for a completed upload + * TODO: should we have a database layer class to handle Propel operations? * * @param $fileId int local CcFiles identifier * @param $track object third-party service track object + * @param $status string Celery task status * * @throws Exception * @throws PropelException */ - protected function _addOrUpdateTrackReference($fileId, $track) { - // First, check if the track already has an entry in the database + protected function _addOrUpdateTrackReference($fileId, $track, $status) { $ref = ThirdPartyTrackReferencesQuery::create() ->filterByDbService($this->_SERVICE_NAME) ->findOneByDbFileId($fileId); @@ -99,10 +100,17 @@ class SoundcloudService extends ThirdPartyService { $ref = new ThirdPartyTrackReferences(); } $ref->setDbService($this->_SERVICE_NAME); - $ref->setDbForeignId($track->id); + // Only set the SoundCloud fields if the task was successful + if ($status == $this->_SUCCESS_STATUS) { + // TODO: fetch any additional SoundCloud parameters we want to store + $ref->setDbForeignId($track->id); // SoundCloud identifier + } $ref->setDbFileId($fileId); - $ref->setDbStatus($track->state); + $ref->setDbStatus($status); // Null the broker task fields because we no longer need them + // We use NULL over an empty string/object here because we have + // a unique constraint on the task ID and it's easier to filter + // and query against NULLs $ref->setDbBrokerTaskId(NULL); $ref->setDbBrokerTaskName(NULL); $ref->setDbBrokerTaskDispatchTime(NULL); diff --git a/airtime_mvc/application/services/ThirdPartyService.php b/airtime_mvc/application/services/ThirdPartyService.php index dc7497116..7ab668db4 100644 --- a/airtime_mvc/application/services/ThirdPartyService.php +++ b/airtime_mvc/application/services/ThirdPartyService.php @@ -35,6 +35,11 @@ abstract class ThirdPartyService { */ protected $_PENDING_STATUS = 'PENDING'; + /** + * @var string status string for successful tasks + */ + protected $_SUCCESS_STATUS = 'SUCCESS'; + /** * @var string status string for failed tasks */ @@ -68,6 +73,7 @@ abstract class ThirdPartyService { /** * Create a ThirdPartyTrackReferences object for a pending task + * TODO: should we have a database layer class to handle Propel operations? * * @param $fileId int local CcFiles identifier * @param $brokerTaskId int broker task identifier to so we can asynchronously @@ -151,9 +157,8 @@ abstract class ThirdPartyService { $pendingTasks = $this->_getPendingTasks($taskName); foreach ($pendingTasks as $task) { try { - $result = $this->_getTaskResult($task); - Logging::info(json_decode($result)); - $this->_addOrUpdateTrackReference($task->getDbFileId(), json_decode($result)); + $message = $this->_getTaskMessage($task); + $this->_addOrUpdateTrackReference($task->getDbFileId(), json_decode($message->result), $message->status); } catch(CeleryException $e) { Logging::info("Couldn't retrieve task message for task " . $task->getDbBrokerTaskName() . " with ID " . $task->getDbBrokerTaskId() . ": " . $e->getMessage()); @@ -185,19 +190,18 @@ abstract class ThirdPartyService { } /** - * Get the result from a celery task message in the results queue - * If the task message no longer exists, remove it from the track references table + * Get a Celery task message from the results queue * * @param $task ThirdPartyTrackReferences the track reference object * - * @return array the results from the task message + * @return object the task message object * * @throws CeleryException when the result message for this task no longer exists */ - protected function _getTaskResult($task) { - $message = Application_Model_RabbitMq::getAsyncResultMessage($task->getDbBrokerTaskName(), - $task->getDbBrokerTaskId()); - return json_decode($message['body'])->result; // The actual result message from the service + protected function _getTaskMessage($task) { + $message = Application_Model_RabbitMq::getAsyncResultMessage($task->getDbBrokerTaskName(), + $task->getDbBrokerTaskId()); + return json_decode($message['body']); } /** @@ -231,11 +235,12 @@ abstract class ThirdPartyService { * * @param $fileId int local CcFiles identifier * @param $track object third-party service track object + * @param $status string Celery task status * * @throws Exception * @throws PropelException */ - abstract protected function _addOrUpdateTrackReference($fileId, $track); + abstract protected function _addOrUpdateTrackReference($fileId, $track, $status); /** * Check whether an OAuth access token exists for the third-party client diff --git a/airtime_mvc/application/upgrade/Upgrades.php b/airtime_mvc/application/upgrade/Upgrades.php index 774b9cff8..d5eb4e7c2 100644 --- a/airtime_mvc/application/upgrade/Upgrades.php +++ b/airtime_mvc/application/upgrade/Upgrades.php @@ -35,7 +35,7 @@ class UpgradeManager { $schemaVersion = Application_Model_Preference::GetSchemaVersion(); $supportedSchemaVersions = self::getSupportedSchemaVersions(); - $upgradeNeeded = !in_array($schemaVersion, $supportedSchemaVersions); + return !in_array($schemaVersion, $supportedSchemaVersions); // We shouldn't run the upgrade as a side-effect of this function! /* if ($upgradeNeeded) { @@ -46,7 +46,6 @@ class UpgradeManager public function doUpgrade() { - $didWePerformAnUpgrade = false; // Get all upgrades dynamically (in declaration order!) so we don't have to add them explicitly each time // TODO: explicitly sort classnames by ascending version suffix for safety $upgraders = getUpgrades(); @@ -69,7 +68,7 @@ class UpgradeManager { // pass the given directory to the upgrades, since __DIR__ returns parent dir of file, not executor $upgrader->upgrade($dir); //This will throw an exception if the upgrade fails. - $didWePerformAnUpgrade = true; + $upgradePerformed = true; } } return $upgradePerformed; @@ -512,7 +511,7 @@ class AirtimeUpgrader2512 extends AirtimeUpgrader class AirtimeUpgrader2513 extends AirtimeUpgrader { - protected function getSupportedVersions() { + protected function getSupportedSchemaVersions() { return array ( '2.5.12' ); @@ -544,7 +543,7 @@ class AirtimeUpgrader2513 extends AirtimeUpgrader passthru("export PGPASSWORD=$password && psql -h $host -U $username -q -f $dir/upgrade_sql/airtime_" .$newVersion."/upgrade.sql $database 2>&1 | grep -v -E \"will create implicit sequence|will create implicit index\""); - Application_Model_Preference::SetAirtimeVersion($newVersion); + Application_Model_Preference::SetSchemaVersion($newVersion); Cache::clear(); $this->toggleMaintenanceScreen(false); @@ -553,5 +552,6 @@ class AirtimeUpgrader2513 extends AirtimeUpgrader throw $e; } } + }