Add check for EpisodeAction properties

- also add type hints and phpdocs
- add additional unit tests
- set integer types in EpisodeActionEntity
- correctly call static methods
- improve exception handling in EpisodeActionSaver
- remove unused method EpisodeActionWriter::purge()

lib/Controller/EpisodeActionController.php

@@ -3,9 +3,6 @@ declare(strict_types=1);
 
 namespace OCA\GPodderSync\Controller;
 
-use DateTime;
-use GuzzleHttp\Psr7\Response;
-use OCA\GPodderSync\Core\EpisodeAction\EpisodeAction;
 use OCA\GPodderSync\Core\EpisodeAction\EpisodeActionSaver;
 use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionRepository;
 use OCP\AppFramework\Controller;
@@ -14,12 +11,8 @@ use OCP\IRequest;
 
 class EpisodeActionController extends Controller {
 
-	/**
-	 * @var EpisodeActionRepository
-	 */
 	private EpisodeActionRepository $episodeActionRepository;
-
+	private string $userId;
-	private $userId;
 	private EpisodeActionSaver $episodeActionSaver;
 
 	protected $request;
@@ -48,14 +41,12 @@ class EpisodeActionController extends Controller {
 	public function create(): JSONResponse {
 
 		$episodeActionsArray = $this->filterEpisodesFromRequestParams($this->request->getParams());
-		
 		$this->episodeActionSaver->saveEpisodeActions($episodeActionsArray, $this->userId);
 
 		return new JSONResponse(["timestamp" => time()]);
 	}
 
 	/**
-	 *
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
 	 *
@@ -77,8 +68,7 @@ class EpisodeActionController extends Controller {
 	}
 
 	/**
-	 * @param array $requestParams
+	 * @param array $data
-	 * 
 	 * @return array $episodeActionsArray
 	 */
 	public function filterEpisodesFromRequestParams(array $data): array {

lib/Controller/SubscriptionChangeController.php

@@ -13,15 +13,9 @@ use OCP\IRequest;
 
 class SubscriptionChangeController extends Controller {
 
-	/**
-	 * @var SubscriptionChangeSaver
-	 */
 	private SubscriptionChangeSaver $subscriptionChangeSaver;
-	/**
-	 * @var SubscriptionChangeRepository
-	 */
 	private SubscriptionChangeRepository $subscriptionChangeRepository;
-	private $userId;
+	private string $userId;
 
 	public function __construct(
 		string $AppName,
@@ -42,9 +36,11 @@ class SubscriptionChangeController extends Controller {
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
 	 *
+	 * @param array $add
+	 * @param array $remove
 	 * @return JSONResponse
 	 */
-	public function create($add, $remove): JSONResponse {
+	public function create(array $add, array $remove): JSONResponse {
 		$this->subscriptionChangeSaver->saveSubscriptionChanges($add, $remove, $this->userId);
 
 		return new JSONResponse(["timestamp" => time()]);
@@ -55,7 +51,7 @@ class SubscriptionChangeController extends Controller {
 	 * @NoAdminRequired
 	 * @NoCSRFRequired
 	 *
-	 * @param int $since
+	 * @param int|null $since
 	 * @return JSONResponse
 	 * @throws \Exception
 	 */
@@ -75,8 +71,8 @@ class SubscriptionChangeController extends Controller {
 	 */
 	private function createDateTimeFromTimestamp(?int $since): DateTime {
 		return ($since !== null)
-			? (new \DateTime)->setTimestamp($since)
+			? (new DateTime)->setTimestamp($since)
-			: (new \DateTime('-1 week'));
+			: (new DateTime('-1 week'));
 	}
 
 	/**
@@ -85,7 +81,7 @@ class SubscriptionChangeController extends Controller {
 	 * @return mixed
 	 */
 	private function extractUrlList(array $allSubscribed): array {
-		return array_map(function (SubscriptionChangeEntity $subscription) {
+		return array_map(static function (SubscriptionChangeEntity $subscription) {
 			return $subscription->getUrl();
 		}, $allSubscribed);
 	}

lib/Core/EpisodeAction/EpisodeAction.php

@@ -99,8 +99,7 @@ class EpisodeAction {
 		return $this->id;
 	}
 
-	public function toArray()
+	public function toArray(): array {
-	{
 		return
 		[
 			'podcast' => $this->getPodcast(),

lib/Core/EpisodeAction/EpisodeActionReader.php

@@ -3,17 +3,20 @@ declare(strict_types=1);
 
 namespace OCA\GPodderSync\Core\EpisodeAction;
 
-class EpisodeActionReader
+class EpisodeActionReader {
-{
+	private array $requiredProperties = ['podcast', 'episode', 'action', 'timestamp'];
+
 	/**
-	 * @param $episodeActionsArray[]
+	 * @param array $episodeActionsArray []
 	 * @return EpisodeAction[]
 	 */
-	public function fromArray(array $episodeActionsArray): array
+	public function fromArray(array $episodeActionsArray): array {
-	{
 		$episodeActions = [];
 
-		foreach($episodeActionsArray as $episodeAction) {
+		foreach ($episodeActionsArray as $episodeAction) {
+			if ($this->hasRequiredProperties($episodeAction) === false) {
+				continue;
+			}
 			$episodeActions[] = new EpisodeAction(
 				$episodeAction["podcast"],
 				$episodeAction["episode"],
@@ -29,4 +32,12 @@ class EpisodeActionReader
 
 		return $episodeActions;
 	}
+
+	/**
+	 * @param array $episodeAction
+	 * @return bool
+	 */
+	private function hasRequiredProperties(array $episodeAction): bool {
+		return (count(array_intersect($this->requiredProperties, array_keys($episodeAction))) === count($this->requiredProperties));
+	}
 }

lib/Core/EpisodeAction/EpisodeActionSaver.php

@@ -3,8 +3,7 @@ declare(strict_types=1);
 
 namespace OCA\GPodderSync\Core\EpisodeAction;
 
-use DateTimeZone;
+use DateTime;
-use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionEntity;
 use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionRepository;
 use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionWriter;
@@ -17,7 +16,7 @@ class EpisodeActionSaver
 	private EpisodeActionWriter $episodeActionWriter;
 	private EpisodeActionReader $episodeActionReader;
 
-	const DATETIME_FORMAT = 'Y-m-d\TH:i:s';
+	private const DATETIME_FORMAT = 'Y-m-d\TH:i:s';
 
 	public function __construct(
 		EpisodeActionRepository $episodeActionRepository,
@@ -32,10 +31,10 @@ class EpisodeActionSaver
 
 	/**
 	 * @param array $episodeActionsArray
-	 *
+	 * @param string $userId
 	 * @return EpisodeActionEntity[]
 	 */
-	public function saveEpisodeActions($episodeActionsArray, string $userId): array
+	public function saveEpisodeActions(array $episodeActionsArray, string $userId): array
 	{
 		$episodeActions = $this->episodeActionReader->fromArray($episodeActionsArray);
 
@@ -46,12 +45,12 @@ class EpisodeActionSaver
 
 			try {
                 $episodeActionEntities[] = $this->episodeActionWriter->save($episodeActionEntity);
-            } catch (UniqueConstraintViolationException $uniqueConstraintViolationException) {
-                $episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId);
             } catch (Exception $exception) {
                 if ($exception->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
-                    $episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId);
+					try {
-                }
+						$episodeActionEntities[] = $this->updateEpisodeAction($episodeActionEntity, $userId);
+					} catch (Exception $exception) {}
+				}
             }
         }
 		return $episodeActionEntities;
@@ -59,10 +58,13 @@ class EpisodeActionSaver
 
 	private function convertTimestampToUnixEpoch(string $timestamp): string
 	{
-		return \DateTime::createFromFormat('Y-m-d\TH:i:s', $timestamp)
+		return DateTime::createFromFormat(self::DATETIME_FORMAT, $timestamp)
 			->format("U");
 	}
 
+	/**
+	 * @throws Exception
+	 */
 	private function updateEpisodeAction(
 		EpisodeActionEntity $episodeActionEntity,
 		string $userId
@@ -96,7 +98,7 @@ class EpisodeActionSaver
 	private function ensureGuidDoesNotGetNulledWithOldData(EpisodeAction $episodeActionToUpdate, EpisodeActionEntity $episodeActionEntity): void
 	{
 		$existingGuid = $episodeActionToUpdate->getGuid();
-		if ($existingGuid !== null && $episodeActionEntity->getGuid() == null) {
+		if ($existingGuid !== null && $episodeActionEntity->getGuid() === null) {
 			$episodeActionEntity->setGuid($existingGuid);
 		}
 	}

lib/Core/SubscriptionChange/SubscriptionChangeRequestParser.php

@@ -21,10 +21,10 @@ class SubscriptionChangeRequestParser {
 	 * @return SubscriptionChange[]
 	 */
 	public function createSubscriptionChangeList(array $urlsSubscribed, array $urlsUnsubscribed): array {
-		$urlsToSubscribe = $this->subscriptionChangeReader->mapToSubscriptionsChanges($urlsSubscribed, true);
+		$urlsToSubscribe = $this->subscriptionChangeReader::mapToSubscriptionsChanges($urlsSubscribed, true);
-		$urlsToDelete = $this->subscriptionChangeReader->mapToSubscriptionsChanges($urlsUnsubscribed, false);
+		$urlsToDelete = $this->subscriptionChangeReader::mapToSubscriptionsChanges($urlsUnsubscribed, false);
 
-		/** @var \OCA\GPodderSync\Core\SubscriptionChange\SubscriptionChange[] $subscriptionChanges */
+		/** @var SubscriptionChange[] $subscriptionChanges */
 		return array_merge($urlsToSubscribe, $urlsToDelete);
 	}
 }

lib/Db/EpisodeAction/EpisodeActionEntity.php

@@ -21,9 +21,13 @@ class EpisodeActionEntity extends Entity implements JsonSerializable {
 
 	public function __construct() {
 		$this->addType('id','integer');
+		$this->addType('started','integer');
+		$this->addType('position','integer');
+		$this->addType('total','integer');
+		$this->addType('timestampEpoch','integer');
 	}
 
-	public function jsonSerialize() {
+	public function jsonSerialize(): array {
 		return [
 			'id' => $this->id,
 			'podcast' => $this->podcast,
@@ -37,22 +41,4 @@ class EpisodeActionEntity extends Entity implements JsonSerializable {
 		];
 	}
 
-	public function getTimestampEpoch() : int
-	{
-		return (int) $this->timestampEpoch;
-	}
-
-	public function getStarted() : int {
-		return (int) $this->started;
-	}
-
-	public function getPosition(): int
-	{
-		return (int) $this->position;
-	}
-
-	public function getTotal(): int
-	{
-		return (int) $this->total;
-	}
 }

lib/Db/EpisodeAction/EpisodeActionMapper.php

@@ -3,20 +3,23 @@ declare(strict_types=1);
 
 namespace OCA\GPodderSync\Db\EpisodeAction;
 
-use OCA\GPodderSync\Core\EpisodeAction\EpisodeAction;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
+use OCP\AppFramework\Db\QBMapper;
+use OCP\DB\Exception;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
-use Safe\DateTime;
 
-class EpisodeActionMapper extends \OCP\AppFramework\Db\QBMapper
+class EpisodeActionMapper extends QBMapper
 {
 	public function __construct(IDBConnection $db)
 	{
 		parent::__construct($db, 'gpodder_episode_action', EpisodeActionEntity::class);
 	}
 
+	/**
+	 * @throws Exception
+	 */
 	public function findAll(int $sinceTimestamp, string $userId): array
 	{
 		$qb = $this->db->getQueryBuilder();
@@ -35,6 +38,11 @@ class EpisodeActionMapper extends \OCP\AppFramework\Db\QBMapper
 
 	}
 
+	/**
+	 * @param string $episodeIdentifier
+	 * @param string $userId
+	 * @return EpisodeActionEntity|null
+	 */
 	public function findByEpisodeIdentifier(string $episodeIdentifier, string $userId) : ?EpisodeActionEntity
 	{
 		$qb = $this->db->getQueryBuilder();
@@ -52,14 +60,10 @@ class EpisodeActionMapper extends \OCP\AppFramework\Db\QBMapper
 
 		try {
 			/** @var EpisodeActionEntity $episodeActionEntity */
-			$episodeActionEntity = $this->findEntity($qb);
+			return $this->findEntity($qb);
-
+		} catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) {
-			return $episodeActionEntity;
+			return null;
-		} catch (DoesNotExistException $e) {
-		} catch (MultipleObjectsReturnedException $e) {
 		}
-
-		return null;
 	}
 
 

lib/Db/EpisodeAction/EpisodeActionRepository.php

@@ -3,6 +3,7 @@ declare(strict_types=1);
 
 namespace OCA\GPodderSync\Db\EpisodeAction;
 
+use DateTime;
 use OCA\GPodderSync\Core\EpisodeAction\EpisodeAction;
 
 class EpisodeActionRepository {
@@ -20,7 +21,6 @@ class EpisodeActionRepository {
 	 * @param string $userId
 	 *
 	 * @return EpisodeAction[]
-	 * @throws \Safe\Exceptions\DatetimeException
 	 */
 	public function findAll(int $sinceEpoch, string $userId) : array {
 		$episodeActions = [];
@@ -30,6 +30,11 @@ class EpisodeActionRepository {
 		return $episodeActions;
 	}
 
+	/**
+	 * @param string $identifier
+	 * @param string $userId
+	 * @return EpisodeAction|null
+	 */
 	public function findByEpisodeIdentifier(string $identifier, string $userId): ?EpisodeAction {
 		$episodeActionEntity = $this->episodeActionMapper->findByEpisodeIdentifier($identifier, $userId);
 
@@ -45,8 +50,6 @@ class EpisodeActionRepository {
 	/**
 	 * @param EpisodeActionEntity $episodeActionEntity
 	 * @return EpisodeAction
-	 * @throws \Safe\Exceptions\DatetimeException
-	 *
 	 */
 	private function mapEntityToEpisodeAction(EpisodeActionEntity $episodeActionEntity): EpisodeAction
 	{
@@ -54,7 +57,7 @@ class EpisodeActionRepository {
 			$episodeActionEntity->getPodcast(),
 			$episodeActionEntity->getEpisode(),
 			$episodeActionEntity->getAction(),
-			\DateTime::createFromFormat(
+			DateTime::createFromFormat(
 				"U",
 				(string)$episodeActionEntity->getTimestampEpoch())
 				->format("Y-m-d\TH:i:s"),

lib/Db/EpisodeAction/EpisodeActionWriter.php

@@ -3,6 +3,8 @@ declare(strict_types=1);
 
 namespace OCA\GPodderSync\Db\EpisodeAction;
 
+use OCP\DB\Exception;
+
 class EpisodeActionWriter {
 
 	/**
@@ -14,18 +16,18 @@ class EpisodeActionWriter {
 		$this->episodeActionMapper = $episodeActionMapper;
 	}
 
+	/**
+	 * @throws Exception
+	 */
 	public function save(EpisodeActionEntity $episodeActionEntity): EpisodeActionEntity {
 		return $this->episodeActionMapper->insert($episodeActionEntity);
 	}
 
+	/**
+	 * @throws Exception
+	 */
 	public function update(EpisodeActionEntity $episodeActionEntity) {
 		return $this->episodeActionMapper->update($episodeActionEntity);
 
 	}
-
-	public function purge() {
-		foreach ($this->episodeActionMapper->findAll() as $entity) {
-			$this->episodeActionMapper->delete($entity);
-		}
-	}
 }

tests/Unit/Core/EpisodeAction/EpisodeActionReaderTest.php

@@ -56,4 +56,12 @@ class EpisodeActionReaderTest extends TestCase {
 		$this->assertSame(450, $episodeActions[2]->getTotal());
 	}
 
+	public function testCreateWithFaultyData(): void {
+		$episodeActions = (new EpisodeActionReader())->fromArray([
+			["podcast" => "https://example.org/feed.xml", "action" => "download", "timestamp" => "2021-10-03T12:03:17"],
+			["podcast" => "https://example.org/feed.xml", "episode" => "https://example.org/episode2.mp3", "guid" => "episode2", "action" => "download", "timestamp" => "2021-10-03T12:03:17"],
+		]);
+		$this->assertCount(1, $episodeActions);
+	}
+
 }

tests/Unit/Core/EpisodeAction/EpisodeActionSaverTest.php

@@ -0,0 +1,82 @@
+<?php
+declare(strict_types=1);
+
+namespace OCA\GPodderSync\Tests\Unit\Core\EpisodeAction;
+
+use OCA\GPodderSync\Core\EpisodeAction\EpisodeAction;
+use OCA\GPodderSync\Core\EpisodeAction\EpisodeActionReader;
+use OCA\GPodderSync\Core\EpisodeAction\EpisodeActionSaver;
+use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionEntity;
+use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionRepository;
+use OCA\GPodderSync\Db\EpisodeAction\EpisodeActionWriter;
+use OCP\DB\Exception;
+use Test\TestCase;
+
+class EpisodeActionSaverTest extends TestCase {
+
+	public function testSaveEpisodeActions(): void {
+		$episodeAction1 = new EpisodeAction('podcast1', 'episode1', 'PLAY', '2021-10-07T13:27:14', 15, 120, 500, 'podcast1guid', null);
+		$episodeAction2 = new EpisodeAction('podcast1', 'episode2', 'PLAY', '2021-10-07T13:27:14', -1, -1, -1, 'podcast1guid', null);
+		$repository = $this->createMock(EpisodeActionRepository::class);
+		$writer = $this->createMock(EpisodeActionWriter::class);
+		$writer->expects($this->exactly(2))->method('save')->withConsecutive(
+			[$this->isInstanceOf(EpisodeActionEntity::class)], [$this->isInstanceOf(EpisodeActionEntity::class)]
+		);
+		$reader = $this->createMock(EpisodeActionReader::class);
+		$reader->method('fromArray')->willReturn([$episodeAction1, $episodeAction2]);
+		$saver = new EpisodeActionSaver($repository, $writer, $reader);
+		$actions = [];
+		$userId = 'paul';
+		$result = $saver->saveEpisodeActions($actions, $userId);
+		$this->assertCount(2, $result);
+		$this->assertInstanceOf(EpisodeActionEntity::class, $result[0]);
+		$this->assertInstanceOf(EpisodeActionEntity::class, $result[1]);
+	}
+
+	public function testUpdateEpisodeActions(): void {
+		$userId = 'paul';
+		$updateGuid = 'podcast2guid';
+		$episodeAction1 = new EpisodeAction('podcast1', 'episode1', 'PLAY', '2021-10-07T13:27:14', 15, 120, 500, 'podcast1guid', null);
+		$episodeAction2 = new EpisodeAction('podcast1', 'episode2', 'PLAY', '2021-10-07T13:27:14', 120, 200, 500, $updateGuid, null);
+		$existingEpisodeAction = new EpisodeAction('podcast1', 'episode2', 'PLAY', '2021-10-07T13:27:14', 120, 200, 500, $updateGuid, 1234);
+		$repository = $this->createMock(EpisodeActionRepository::class);
+		$repository->expects($this->once())->method('findByEpisodeIdentifier')->with($updateGuid, $userId)->willReturn($existingEpisodeAction);
+		$writer = $this->createMock(EpisodeActionWriter::class);
+		$mockException = $this->createMock(Exception::class);
+		$mockException->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION);
+		$writer->method('save')
+			->willReturnOnConsecutiveCalls(
+				$this->createMock(EpisodeActionEntity::class),
+				$this->throwException($mockException)
+			);
+		$writer->expects($this->once())->method('update')->with($this->isInstanceOf(EpisodeActionEntity::class))->willReturn($this->createMock(EpisodeActionEntity::class));
+		$reader = $this->createMock(EpisodeActionReader::class);
+		$reader->method('fromArray')->willReturn([$episodeAction1, $episodeAction2]);
+		$saver = new EpisodeActionSaver($repository, $writer, $reader);
+		$actions = [];
+		$result = $saver->saveEpisodeActions($actions, $userId);
+		$this->assertCount(2, $result);
+		$this->assertInstanceOf(EpisodeActionEntity::class, $result[0]);
+		$this->assertInstanceOf(EpisodeActionEntity::class, $result[1]);
+	}
+
+	public function testUpdateEpisodeActionsFailure(): void {
+		$userId = 'paul';
+		$updateGuid = 'podcast2guid';
+		$episodeAction2 = new EpisodeAction('podcast1', 'episode2', 'PLAY', '2021-10-07T13:27:14', 120, 200, 500, $updateGuid, null);
+		$existingEpisodeAction = new EpisodeAction('podcast1', 'episode2', 'PLAY', '2021-10-07T13:27:14', 120, 200, 500, $updateGuid, 1234);
+		$repository = $this->createMock(EpisodeActionRepository::class);
+		$repository->expects($this->once())->method('findByEpisodeIdentifier')->with($updateGuid, $userId)->willReturn($existingEpisodeAction);
+		$writer = $this->createMock(EpisodeActionWriter::class);
+		$mockException = $this->createMock(Exception::class);
+		$mockException->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION);
+		$writer->method('save')->willThrowException($mockException);
+		$writer->expects($this->once())->method('update')->with($this->isInstanceOf(EpisodeActionEntity::class))->willThrowException($mockException);
+		$reader = $this->createMock(EpisodeActionReader::class);
+		$reader->method('fromArray')->willReturn([$episodeAction2]);
+		$saver = new EpisodeActionSaver($repository, $writer, $reader);
+		$actions = [];
+		$result = $saver->saveEpisodeActions($actions, $userId);
+		$this->assertCount(0, $result);
+	}
+}

tests/Unit/Core/EpisodeAction/EpisodeActionTest.php

@@ -0,0 +1,24 @@
+<?php
+declare(strict_types=1);
+
+namespace OCA\GPodderSync\Tests\Unit\Core\EpisodeAction;
+
+use OCA\GPodderSync\Core\EpisodeAction\EpisodeAction;
+use Test\TestCase;
+
+class EpisodeActionTest extends TestCase {
+	public function testToArray(): void {
+		$episodeAction = new EpisodeAction('podcast1', 'episode1', 'PLAY', '2021-10-07T13:27:14', 15, 120, 500, 'podcast1guid', null);
+		$expected = [
+			'podcast' => 'podcast1',
+			'episode' => 'episode1',
+			'timestamp' => '2021-10-07T13:27:14',
+			'guid' => 'podcast1guid',
+			'position' => 120,
+			'started' => 15,
+			'total' => 500,
+			'action' => 'PLAY',
+		];
+		$this->assertSame($expected, $episodeAction->toArray());
+	}
+}