From a1893061ce028e65f4fb9445de1c0bf46d9a09ac Mon Sep 17 00:00:00 2001 From: thrillfall Date: Fri, 24 Feb 2023 16:48:16 +0100 Subject: [PATCH] fix #109 search for episode action by guid only (retry with episode url if nothing found) thus avoid multiple results --- lib/Core/EpisodeAction/EpisodeActionSaver.php | 161 +++++++++--------- lib/Db/EpisodeAction/EpisodeActionMapper.php | 33 +++- .../EpisodeAction/EpisodeActionRepository.php | 30 +++- .../EpisodeActionControllerTest.php | 5 +- .../EpisodeActionGuidMigrationTest.php | 18 +- .../EpisodeActionRepositoryTest.php | 2 +- ...ionSaverGuidBackwardCompatibilityTest.php} | 2 +- .../Migration/TimestampMigrationTest.php | 6 +- 8 files changed, 141 insertions(+), 116 deletions(-) rename tests/Integration/{EpisodeActionSaverGuidBackwardCompatbilityTest.php => EpisodeActionSaverGuidBackwardCompatibilityTest.php} (95%) diff --git a/lib/Core/EpisodeAction/EpisodeActionSaver.php b/lib/Core/EpisodeAction/EpisodeActionSaver.php index 9095a7d..cfac4b9 100644 --- a/lib/Core/EpisodeAction/EpisodeActionSaver.php +++ b/lib/Core/EpisodeAction/EpisodeActionSaver.php @@ -13,107 +13,106 @@ use OCP\DB\Exception; class EpisodeActionSaver { - private EpisodeActionRepository $episodeActionRepository; - private EpisodeActionWriter $episodeActionWriter; - private EpisodeActionReader $episodeActionReader; + private EpisodeActionRepository $episodeActionRepository; + private EpisodeActionWriter $episodeActionWriter; + private EpisodeActionReader $episodeActionReader; - private const DATETIME_FORMAT = 'Y-m-d\TH:i:s'; + private const DATETIME_FORMAT = 'Y-m-d\TH:i:s'; - public function __construct( - EpisodeActionRepository $episodeActionRepository, - EpisodeActionWriter $episodeActionWriter, - EpisodeActionReader $episodeActionReader - ) - { - $this->episodeActionRepository = $episodeActionRepository; - $this->episodeActionWriter = $episodeActionWriter; - $this->episodeActionReader = $episodeActionReader; - } + public function __construct( + EpisodeActionRepository $episodeActionRepository, + EpisodeActionWriter $episodeActionWriter, + EpisodeActionReader $episodeActionReader + ) + { + $this->episodeActionRepository = $episodeActionRepository; + $this->episodeActionWriter = $episodeActionWriter; + $this->episodeActionReader = $episodeActionReader; + } - /** - * @param array $episodeActionsArray - * @param string $userId - * @return EpisodeActionEntity[] - */ - public function saveEpisodeActions(array $episodeActionsArray, string $userId): array - { - $episodeActions = $this->episodeActionReader->fromArray($episodeActionsArray); + public function saveEpisodeActions(array $episodeActionsArray, string $userId): array + { + $episodeActions = $this->episodeActionReader->fromArray($episodeActionsArray); - $episodeActionEntities = []; + $episodeActionEntities = []; foreach ($episodeActions as $episodeAction) { - $episodeActionEntity = $this->hydrateEpisodeActionEntity($episodeAction, $userId); + $episodeActionEntity = $this->hydrateEpisodeActionEntity($episodeAction, $userId); - try { + try { $episodeActionEntities[] = $this->episodeActionWriter->save($episodeActionEntity); - } catch (UniqueConstraintViolationException $uniqueConstraintViolationException) { - $episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId); + } catch (UniqueConstraintViolationException $uniqueConstraintViolationException) { + $episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId); } catch (Exception $exception) { if ($exception->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - $episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId); - } + $episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId); + } } } - return $episodeActionEntities; - } + return $episodeActionEntities; + } - private function convertTimestampToUnixEpoch(string $timestamp): string - { - return DateTime::createFromFormat(self::DATETIME_FORMAT, $timestamp) - ->format("U"); - } + private function convertTimestampToUnixEpoch(string $timestamp): string + { + return DateTime::createFromFormat(self::DATETIME_FORMAT, $timestamp) + ->format("U"); + } - private function updateEpisodeAction( - EpisodeActionEntity $episodeActionEntity, - string $userId - ): EpisodeActionEntity - { - $identifier = $episodeActionEntity->getGuid() ?? $episodeActionEntity->getEpisode(); - $episodeActionToUpdate = $this->episodeActionRepository->findByEpisodeIdentifier( - $identifier, - $userId - ); + private function updateEpisodeAction( + EpisodeActionEntity $episodeActionEntity, + string $userId + ): EpisodeActionEntity + { + $episodeActionToUpdate = $this->findEpisodeActionToUpdate($episodeActionEntity, $userId); - if ($episodeActionToUpdate === null && $episodeActionEntity->getGuid() !== null) { - $episodeActionToUpdate = $this->getOldEpisodeActionByEpisodeUrl($episodeActionEntity->getEpisode(), $userId); - } + $episodeActionEntity->setId($episodeActionToUpdate->getId()); - $episodeActionEntity->setId($episodeActionToUpdate->getId()); + $this->ensureGuidDoesNotGetNulledWithOldData($episodeActionToUpdate, $episodeActionEntity); - $this->ensureGuidDoesNotGetNulledWithOldData($episodeActionToUpdate, $episodeActionEntity); + return $this->episodeActionWriter->update($episodeActionEntity); + } - return $this->episodeActionWriter->update($episodeActionEntity); - } + private function ensureGuidDoesNotGetNulledWithOldData(EpisodeAction $episodeActionToUpdate, EpisodeActionEntity $episodeActionEntity): void + { + $existingGuid = $episodeActionToUpdate->getGuid(); + if ($existingGuid !== null && $episodeActionEntity->getGuid() === null) { + $episodeActionEntity->setGuid($existingGuid); + } + } - private function getOldEpisodeActionByEpisodeUrl(string $episodeUrl, string $userId): ?EpisodeAction - { - return $this->episodeActionRepository->findByEpisodeIdentifier( - $episodeUrl, - $userId - ); - } + private function hydrateEpisodeActionEntity(EpisodeAction $episodeAction, string $userId): EpisodeActionEntity + { + $episodeActionEntity = new EpisodeActionEntity(); + $episodeActionEntity->setPodcast($episodeAction->getPodcast()); + $episodeActionEntity->setEpisode($episodeAction->getEpisode()); + $episodeActionEntity->setGuid($episodeAction->getGuid()); + $episodeActionEntity->setAction($episodeAction->getAction()); + $episodeActionEntity->setPosition($episodeAction->getPosition()); + $episodeActionEntity->setStarted($episodeAction->getStarted()); + $episodeActionEntity->setTotal($episodeAction->getTotal()); + $episodeActionEntity->setTimestampEpoch($this->convertTimestampToUnixEpoch($episodeAction->getTimestamp())); + $episodeActionEntity->setUserId($userId); - private function ensureGuidDoesNotGetNulledWithOldData(EpisodeAction $episodeActionToUpdate, EpisodeActionEntity $episodeActionEntity): void - { - $existingGuid = $episodeActionToUpdate->getGuid(); - if ($existingGuid !== null && $episodeActionEntity->getGuid() === null) { - $episodeActionEntity->setGuid($existingGuid); - } - } + return $episodeActionEntity; + } - private function hydrateEpisodeActionEntity(EpisodeAction $episodeAction, string $userId): EpisodeActionEntity - { - $episodeActionEntity = new EpisodeActionEntity(); - $episodeActionEntity->setPodcast($episodeAction->getPodcast()); - $episodeActionEntity->setEpisode($episodeAction->getEpisode()); - $episodeActionEntity->setGuid($episodeAction->getGuid()); - $episodeActionEntity->setAction($episodeAction->getAction()); - $episodeActionEntity->setPosition($episodeAction->getPosition()); - $episodeActionEntity->setStarted($episodeAction->getStarted()); - $episodeActionEntity->setTotal($episodeAction->getTotal()); - $episodeActionEntity->setTimestampEpoch($this->convertTimestampToUnixEpoch($episodeAction->getTimestamp())); - $episodeActionEntity->setUserId($userId); + private function findEpisodeActionToUpdate(EpisodeActionEntity $episodeActionEntity, string $userId): ?EpisodeAction + { + $episodeAction = null; + if ($episodeActionEntity->getGuid() !== null) { + $episodeAction = $this->episodeActionRepository->findByGuid( + $episodeActionEntity->getGuid(), + $userId + ); + } - return $episodeActionEntity; - } + if ($episodeAction === null) { + $episodeAction = $this->episodeActionRepository->findByEpisodeUrl( + $episodeActionEntity->getEpisode(), + $userId + ); + } + + return $episodeAction; + } } diff --git a/lib/Db/EpisodeAction/EpisodeActionMapper.php b/lib/Db/EpisodeAction/EpisodeActionMapper.php index 53a95d4..668f954 100644 --- a/lib/Db/EpisodeAction/EpisodeActionMapper.php +++ b/lib/Db/EpisodeAction/EpisodeActionMapper.php @@ -43,20 +43,18 @@ class EpisodeActionMapper extends QBMapper * @param string $userId * @return EpisodeActionEntity|null */ - public function findByEpisodeIdentifier(string $episodeIdentifier, string $userId) : ?EpisodeActionEntity + public function findByEpisodeUrl(string $episodeIdentifier, string $userId) : ?EpisodeActionEntity { $qb = $this->db->getQueryBuilder(); $qb->select('*') ->from($this->getTableName()) ->where( - $qb->expr()->orX( - $qb->expr()->eq('episode', $qb->createNamedParameter($episodeIdentifier)), - $qb->expr()->eq('guid', $qb->createNamedParameter($episodeIdentifier))) - ) + $qb->expr()->eq('episode', $qb->createNamedParameter($episodeIdentifier)) + ) ->andWhere( - $qb->expr()->eq('user_id', $qb->createNamedParameter($userId)) - ); + $qb->expr()->eq('user_id', $qb->createNamedParameter($userId)) + ); try { /** @var EpisodeActionEntity $episodeActionEntity */ @@ -66,5 +64,26 @@ class EpisodeActionMapper extends QBMapper } } + public function findByGuid(string $guid, string $userId) : ?EpisodeActionEntity + { + $qb = $this->db->getQueryBuilder(); + + $qb->select('*') + ->from($this->getTableName()) + ->where( + $qb->expr()->eq('guid', $qb->createNamedParameter($guid)) + ) + ->andWhere( + $qb->expr()->eq('user_id', $qb->createNamedParameter($userId)) + ); + + try { + /** @var EpisodeActionEntity $episodeActionEntity */ + return $this->findEntity($qb); + } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) { + return null; + } + } + } diff --git a/lib/Db/EpisodeAction/EpisodeActionRepository.php b/lib/Db/EpisodeAction/EpisodeActionRepository.php index 0836c14..ff60e8b 100644 --- a/lib/Db/EpisodeAction/EpisodeActionRepository.php +++ b/lib/Db/EpisodeAction/EpisodeActionRepository.php @@ -28,17 +28,29 @@ class EpisodeActionRepository { return $episodeActions; } - public function findByEpisodeIdentifier(string $identifier, string $userId): ?EpisodeAction { - $episodeActionEntity = $this->episodeActionMapper->findByEpisodeIdentifier($identifier, $userId); + public function findByEpisodeUrl(string $episodeUrl, string $userId): ?EpisodeAction { + $episodeActionEntity = $this->episodeActionMapper->findByEpisodeUrl($episodeUrl, $userId); - if ($episodeActionEntity === null) { - return null; - } + if ($episodeActionEntity === null) { + return null; + } - return $this->mapEntityToEpisodeAction( - $episodeActionEntity - ); - } + return $this->mapEntityToEpisodeAction( + $episodeActionEntity + ); + } + + public function findByGuid(string $guid, string $userId): ?EpisodeAction { + $episodeActionEntity = $this->episodeActionMapper->findByGuid($guid, $userId); + + if ($episodeActionEntity === null) { + return null; + } + + return $this->mapEntityToEpisodeAction( + $episodeActionEntity + ); + } private function mapEntityToEpisodeAction(EpisodeActionEntity $episodeActionEntity): EpisodeAction { diff --git a/tests/Integration/Controller/EpisodeActionControllerTest.php b/tests/Integration/Controller/EpisodeActionControllerTest.php index 73788a4..9e06446 100644 --- a/tests/Integration/Controller/EpisodeActionControllerTest.php +++ b/tests/Integration/Controller/EpisodeActionControllerTest.php @@ -3,7 +3,9 @@ declare(strict_types=1); namespace tests\Integration\Controller; -use OC\Security\SecureRandom; +require_once __DIR__ . '/../../Helper/DatabaseTransaction.php'; +require_once __DIR__ . '/../../Helper/Writer/TestWriter.php'; + use OCA\GPodderSync\Controller\EpisodeActionController; use OCA\GPodderSync\Core\EpisodeAction\EpisodeActionSaver; use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionEntity; @@ -12,7 +14,6 @@ use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionRepository; use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionWriter; use OCP\AppFramework\App; use OCP\AppFramework\IAppContainer; -use OCP\IConfig; use OCP\IRequest; use Test\TestCase; use tests\Helper\DatabaseTransaction; diff --git a/tests/Integration/EpisodeActionGuidMigrationTest.php b/tests/Integration/EpisodeActionGuidMigrationTest.php index 1d77929..f15a6e1 100644 --- a/tests/Integration/EpisodeActionGuidMigrationTest.php +++ b/tests/Integration/EpisodeActionGuidMigrationTest.php @@ -22,19 +22,13 @@ class EpisodeActionGuidMigrationTest extends TestCase private IAppContainer $container; - /** - * @var EpisodeActionWriter - */ - private $episodeActionWriter; + private EpisodeActionWriter $episodeActionWriter; - /** - * @var EpisodeActionRepository - */ - private $episodeActionRepository; + private EpisodeActionRepository $episodeActionRepository; public function setUp(): void { parent::setUp(); - $app = new App('gpoddersync'); + $app = new App('nextcloud-gpodder'); $this->container = $app->getContainer(); $this->episodeActionWriter = $this->container->get(EpisodeActionWriter::class); $this->episodeActionRepository = $this->container->get(EpisodeActionRepository::class); @@ -85,7 +79,7 @@ class EpisodeActionGuidMigrationTest extends TestCase self::assertSame( $savedEpisodeActionEntity->getId(), - $this->episodeActionRepository->findByEpisodeIdentifier($episodeActionEntity->getEpisode(), self::USER_ID_0)->getId() + $this->episodeActionRepository->findByEpisodeUrl($episodeActionEntity->getEpisode(), self::USER_ID_0)->getId() ); //update same episode action again this time with guid @@ -96,12 +90,12 @@ class EpisodeActionGuidMigrationTest extends TestCase self::assertSame( $savedEpisodeActionEntityWithGuid->getId(), - $this->episodeActionRepository->findByEpisodeIdentifier($episodeActionEntityWithGuid->getEpisode(), self::USER_ID_0)->getId() + $this->episodeActionRepository->findByEpisodeUrl($episodeActionEntityWithGuid->getEpisode(), self::USER_ID_0)->getId() ); self::assertSame( $savedEpisodeActionEntityWithGuid->getId(), - $this->episodeActionRepository->findByEpisodeIdentifier($episodeActionEntityWithGuid->getGuid(), self::USER_ID_0)->getId() + $this->episodeActionRepository->findByGuid($episodeActionEntityWithGuid->getGuid(), self::USER_ID_0)->getId() ); } diff --git a/tests/Integration/EpisodeActionRepositoryTest.php b/tests/Integration/EpisodeActionRepositoryTest.php index a3b14e4..a04a471 100644 --- a/tests/Integration/EpisodeActionRepositoryTest.php +++ b/tests/Integration/EpisodeActionRepositoryTest.php @@ -49,7 +49,7 @@ class EpisodeActionRepositoryTest extends \Test\TestCase /** @var $episodeActionRepository EpisodeActionRepository */ $episodeActionRepository = $this->container->get(EpisodeActionRepository::class); - $retrievedEpisodeActionEntity = $episodeActionRepository->findByEpisodeIdentifier($guid, self::USER_ID_0); + $retrievedEpisodeActionEntity = $episodeActionRepository->findByGuid($guid, self::USER_ID_0); self::assertSame('2021-08-22T23:58:56', $retrievedEpisodeActionEntity->getTimestamp()); } diff --git a/tests/Integration/EpisodeActionSaverGuidBackwardCompatbilityTest.php b/tests/Integration/EpisodeActionSaverGuidBackwardCompatibilityTest.php similarity index 95% rename from tests/Integration/EpisodeActionSaverGuidBackwardCompatbilityTest.php rename to tests/Integration/EpisodeActionSaverGuidBackwardCompatibilityTest.php index e12b33b..8e72e94 100644 --- a/tests/Integration/EpisodeActionSaverGuidBackwardCompatbilityTest.php +++ b/tests/Integration/EpisodeActionSaverGuidBackwardCompatibilityTest.php @@ -10,7 +10,7 @@ use Test\TestCase; /** * @group DB */ -class EpisodeActionSaverGuidBackwardCompatbilityTest extends TestCase +class EpisodeActionSaverGuidBackwardCompatibilityTest extends TestCase { private const USER_ID_0 = "testuser0"; diff --git a/tests/Integration/Migration/TimestampMigrationTest.php b/tests/Integration/Migration/TimestampMigrationTest.php index d674f26..387c90b 100644 --- a/tests/Integration/Migration/TimestampMigrationTest.php +++ b/tests/Integration/Migration/TimestampMigrationTest.php @@ -79,7 +79,7 @@ class TimestampMigrationTest extends TestCase $trueCrimeEpisodeActionEntity->setGuid(uniqid("self::TEST_GUID_1234")); $this->episodeActionWriter->save($trueCrimeEpisodeActionEntity); - $episodeActionBeforeConversion = $this->episodeActionMapper->findByEpisodeIdentifier( + $episodeActionBeforeConversion = $this->episodeActionMapper->findByGuid( $scienceEpisodeActionEntity->getGuid(), self::ADMIN ); @@ -92,7 +92,7 @@ class TimestampMigrationTest extends TestCase $timestampMigration = new TimestampMigration($this->dbConnection, $this->migrationConfig); $timestampMigration->run($this->createMock(SimpleOutput::class)); - $scienceEpisodeActionAfterConversion = $this->episodeActionMapper->findByEpisodeIdentifier( + $scienceEpisodeActionAfterConversion = $this->episodeActionMapper->findByGuid( $scienceEpisodeActionEntity->getGuid(), self::ADMIN ); @@ -101,7 +101,7 @@ class TimestampMigrationTest extends TestCase $scienceEpisodeActionAfterConversion->getTimestampEpoch() ); - $trueCrimeEpisodeActionAfterConversion = $this->episodeActionMapper->findByEpisodeIdentifier( + $trueCrimeEpisodeActionAfterConversion = $this->episodeActionMapper->findByGuid( $trueCrimeEpisodeActionEntity->getGuid(), self::ADMIN );