From 3ab7993d04baf82cc2d16cfe07e5de41d29a9a6b Mon Sep 17 00:00:00 2001 From: Alexey Skobkin Date: Tue, 17 Jan 2017 04:07:38 +0300 Subject: [PATCH] User removal support. --- .../Version20170116224555.php | 34 +++++++++++++ .../Command/ImportUsersCommand.php | 10 +--- .../Command/UpdateSubscriptionsCommand.php | 20 ++++++-- .../DataFixtures/ORM/LoadUserData.php | 4 +- .../Bundle/PointToolsBundle/Entity/User.php | 50 +++++++++++-------- .../Repository/UserRepository.php | 13 +++++ .../Service/Api/AbstractApi.php | 38 ++++++++------ .../Service/Factory/UserFactory.php | 12 ++--- .../Service/SubscriptionsManager.php | 1 + .../Event/UserSubscribersUpdatedEventTest.php | 8 +-- .../Event/UsersRenamedEventTest.php | 2 +- 11 files changed, 126 insertions(+), 66 deletions(-) create mode 100644 app/DoctrineMigrations/Version20170116224555.php diff --git a/app/DoctrineMigrations/Version20170116224555.php b/app/DoctrineMigrations/Version20170116224555.php new file mode 100644 index 0000000..f2fb05a --- /dev/null +++ b/app/DoctrineMigrations/Version20170116224555.php @@ -0,0 +1,34 @@ +abortIf($this->connection->getDatabasePlatform()->getName() !== 'postgresql', 'Migration can only be executed safely on \'postgresql\'.'); + + $this->addSql('ALTER TABLE users.users ADD is_removed BOOLEAN DEFAULT FALSE NOT NULL'); + } + + /** + * @param Schema $schema + */ + public function down(Schema $schema) + { + // this down() migration is auto-generated, please modify it to your needs + $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'postgresql', 'Migration can only be executed safely on \'postgresql\'.'); + + $this->addSql('ALTER TABLE users.users DROP is_removed'); + } +} diff --git a/src/Skobkin/Bundle/PointToolsBundle/Command/ImportUsersCommand.php b/src/Skobkin/Bundle/PointToolsBundle/Command/ImportUsersCommand.php index 87de45b..a5bd800 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Command/ImportUsersCommand.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Command/ImportUsersCommand.php @@ -75,15 +75,9 @@ class ImportUsersCommand extends ContainerAwareCommand continue; } - $createdAt = \DateTime::createFromFormat('Y-m-d_H:i:s', $row[3]); + $createdAt = \DateTime::createFromFormat('Y-m-d_H:i:s', $row[3]) ?: new \DateTime(); - if (!$createdAt) { - $createdAt = new \DateTime(); - } - - $user = (new User($row[0], $row[1], $row[2])) - ->setCreatedAt($createdAt) - ; + $user = new User($row[0], $createdAt, $row[1], $row[2]); if (!$input->getOption('check-only')) { $em->persist($user); diff --git a/src/Skobkin/Bundle/PointToolsBundle/Command/UpdateSubscriptionsCommand.php b/src/Skobkin/Bundle/PointToolsBundle/Command/UpdateSubscriptionsCommand.php index 001ca97..feb87f1 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Command/UpdateSubscriptionsCommand.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Command/UpdateSubscriptionsCommand.php @@ -6,6 +6,7 @@ use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; use Skobkin\Bundle\PointToolsBundle\Entity\Subscription; use Skobkin\Bundle\PointToolsBundle\Entity\User; +use Skobkin\Bundle\PointToolsBundle\Exception\Api\UserNotFoundException; use Skobkin\Bundle\PointToolsBundle\Repository\UserRepository; use Skobkin\Bundle\PointToolsBundle\Service\SubscriptionsManager; use Skobkin\Bundle\PointToolsBundle\Service\Api\UserApi; @@ -185,6 +186,11 @@ class UpdateSubscriptionsCommand extends ContainerAwareCommand try { $userCurrentSubscribers = $this->api->getUserSubscribersById($user->getId()); + } catch (UserNotFoundException $e) { + $this->logger->warning('User not found. Marking as removed', ['login' => $user->getLogin(), 'user_id' => $user->getId()]); + $user->markAsRemoved(); + + continue; } catch (\Exception $e) { $this->logger->error( 'Error while getting subscribers. Skipping.', @@ -222,14 +228,16 @@ class UpdateSubscriptionsCommand extends ContainerAwareCommand private function getUsersForUpdate(int $appUserId): array { + $usersForUpdate = []; + if ($this->input->getOption('all-users')) { - $usersForUpdate = $this->userRepo->findAll(); + $usersForUpdate = $this->userRepo->findBy(['removed' => false]); } else { /** @var User $serviceUser */ - $serviceUser = $this->userRepo->find($appUserId); + $serviceUser = $this->userRepo->findActiveUserWithSubscribers($appUserId); if (!$serviceUser) { - $this->logger->info('Service user not found'); + $this->logger->critical('Service user not found'); // @todo Retrieving user throw new \RuntimeException('Service user not found in the database'); @@ -239,6 +247,10 @@ class UpdateSubscriptionsCommand extends ContainerAwareCommand try { $usersForUpdate = $this->api->getUserSubscribersById($appUserId); + } catch (UserNotFoundException $e) { + $this->logger->critical('Service user deleted or API response is invalid'); + + throw $e; } catch (\Exception $e) { $this->logger->warning( 'Error while getting service subscribers. Fallback to local list.', @@ -251,8 +263,6 @@ class UpdateSubscriptionsCommand extends ContainerAwareCommand ] ); - $usersForUpdate = []; - /** @var Subscription $subscription */ foreach ((array) $serviceUser->getSubscribers() as $subscription) { $usersForUpdate[] = $subscription->getSubscriber(); diff --git a/src/Skobkin/Bundle/PointToolsBundle/DataFixtures/ORM/LoadUserData.php b/src/Skobkin/Bundle/PointToolsBundle/DataFixtures/ORM/LoadUserData.php index 3e5ae0f..db6a431 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/DataFixtures/ORM/LoadUserData.php +++ b/src/Skobkin/Bundle/PointToolsBundle/DataFixtures/ORM/LoadUserData.php @@ -27,9 +27,7 @@ class LoadUserData extends AbstractFixture implements OrderedFixtureInterface $userId = 99999; foreach ($this->users as $userData) { - $user = (new User($userId--, $userData['login'], $userData['name'])) - ->setCreatedAt(new \DateTime()) - ; + $user = new User($userId--, new \DateTime(), $userData['login'], $userData['name']); $om->persist($user); diff --git a/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php b/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php index 69adb10..7ed3ccc 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php @@ -68,17 +68,25 @@ class User /** * @var ArrayCollection|SubscriptionEvent[] + * * @ORM\OneToMany(targetEntity="SubscriptionEvent", mappedBy="author", fetch="EXTRA_LAZY") */ private $newSubscriberEvents; + /** + * @var bool + * + * @ORM\Column(name="is_removed", type="boolean", options={"default": false}) + */ + private $removed = false; - public function __construct(int $id, string $login = null, string $name = null) + + public function __construct(int $id, \DateTime $createdAt = null, string $login = null, string $name = null) { $this->id = $id; $this->login = $login; $this->name = $name; - $this->createdAt = new \DateTime(); + $this->createdAt = $createdAt ?: new \DateTime(); $this->subscribers = new ArrayCollection(); $this->subscriptions = new ArrayCollection(); @@ -98,30 +106,25 @@ class User return $this->id; } - public function setLogin(string $login): self - { - $this->login = $login; - - return $this; - } public function getLogin(): string { return $this->login; } - public function setName(?string $name): self - { - $this->name = $name; - - return $this; - } - public function getName(): ?string { return $this->name; } + public function updateLoginAndName(string $login, string $name): self + { + $this->login = $login; + $this->name = $name; + + return $this; + } + public function addSubscriber(Subscription $subscribers): self { $this->subscribers[] = $subscribers; @@ -170,15 +173,18 @@ class User return $this->createdAt; } - public function setCreatedAt(\DateTime $createdAt): self - { - $this->createdAt = $createdAt; - - return $this; - } - public function getUpdatedAt(): ?\DateTime { return $this->updatedAt; } + + public function isRemoved(): bool + { + return $this->isRemoved(); + } + + public function markAsRemoved(): void + { + $this->removed = true; + } } diff --git a/src/Skobkin/Bundle/PointToolsBundle/Repository/UserRepository.php b/src/Skobkin/Bundle/PointToolsBundle/Repository/UserRepository.php index 8c1890a..2ee26f1 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Repository/UserRepository.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Repository/UserRepository.php @@ -13,6 +13,19 @@ class UserRepository extends EntityRepository $this->getEntityManager()->persist($entity); } + public function findActiveUserWithSubscribers(int $id): ?User + { + $qb = $this->createQueryBuilder('u'); + + return $qb + ->select(['u', '']) + ->innerJoin('u.subscribers', 's') + ->innerJoin('s.subscriber', 'us') + ->where('u.removed = FALSE') + ->getQuery()->getOneOrNullResult() + ; + } + /** * Case-insensitive user search */ diff --git a/src/Skobkin/Bundle/PointToolsBundle/Service/Api/AbstractApi.php b/src/Skobkin/Bundle/PointToolsBundle/Service/Api/AbstractApi.php index 3970db7..eec91f7 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Service/Api/AbstractApi.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Service/Api/AbstractApi.php @@ -51,22 +51,6 @@ class AbstractApi $this->logger = $logger; } - /** - * Make GET request and return response body - */ - public function getGetResponseBody($path, array $parameters = []): StreamInterface - { - return $this->sendGetRequest($path, $parameters)->getBody(); - } - - /** - * Make POST request and return response body - */ - public function getPostResponseBody(string $path, array $parameters = []): StreamInterface - { - return $this->sendPostRequest($path, $parameters)->getBody(); - } - /** * Make GET request and return DTO objects * @@ -97,6 +81,22 @@ class AbstractApi ); } + /** + * Make GET request and return response body + */ + public function getGetResponseBody($path, array $parameters = []): StreamInterface + { + return $this->sendGetRequest($path, $parameters)->getBody(); + } + + /** + * Make POST request and return response body + */ + public function getPostResponseBody(string $path, array $parameters = []): StreamInterface + { + return $this->sendPostRequest($path, $parameters)->getBody(); + } + /** * @param string $path Request path * @param array $parameters Key => Value array of query parameters @@ -151,6 +151,12 @@ class AbstractApi $code = $response->getStatusCode(); $reason = $response->getReasonPhrase(); + // @todo remove after fix + // Temporary fix until @arts fixes this bug + if ('{"error": "UserNotFound"}' === (string) $response->getBody()) { + throw new NotFoundException($reason, $code); + } + switch ($code) { case SymfonyResponse::HTTP_UNAUTHORIZED: throw new UnauthorizedException($reason, $code); diff --git a/src/Skobkin/Bundle/PointToolsBundle/Service/Factory/UserFactory.php b/src/Skobkin/Bundle/PointToolsBundle/Service/Factory/UserFactory.php index 2c7fc2e..d9ff639 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Service/Factory/UserFactory.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Service/Factory/UserFactory.php @@ -39,16 +39,14 @@ class UserFactory extends AbstractFactory /** @var User $user */ if (null === ($user = $this->userRepository->find($userData->getId()))) { - // Creating new user - $user = new User($userData->getId()); + $user = new User( + $userData->getId(), + \DateTime::createFromFormat('Y-m-d_H:i:s', $userData->getCreated()) ?: new \DateTime() + ); $this->userRepository->add($user); } - // Updating data - $user - ->setLogin($userData->getLogin()) - ->setName($userData->getName()) - ; + $user->updateLoginAndName($userData->getLogin(), $userData->getName()); return $user; } diff --git a/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php b/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php index 79312a5..fc839ea 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php @@ -52,6 +52,7 @@ class SubscriptionsManager */ public function updateUserSubscribers(User $user, $newSubscribersList = []): void { + // @todo optimize $tmpOldSubscribers = $user->getSubscribers(); $oldSubscribersList = []; diff --git a/tests/Skobkin/PointToolsBundle/Event/UserSubscribersUpdatedEventTest.php b/tests/Skobkin/PointToolsBundle/Event/UserSubscribersUpdatedEventTest.php index 56a6ab3..3853c04 100644 --- a/tests/Skobkin/PointToolsBundle/Event/UserSubscribersUpdatedEventTest.php +++ b/tests/Skobkin/PointToolsBundle/Event/UserSubscribersUpdatedEventTest.php @@ -14,15 +14,15 @@ class UserSubscribersUpdatedEventTest extends \PHPUnit_Framework_TestCase public function testCreate() { - $user = new User(99999, 'testuser', 'Test User 1'); + $user = new User(99999, new \DateTime(), 'testuser', 'Test User 1'); $subscribed = [ - new User(99998, 'testuser2', 'Test User 2'), + new User(99998, new \DateTime(), 'testuser2', 'Test User 2'), ]; $unsubscribed = [ - new User(99997, 'testuser3', 'Test User 3'), - new User(99996, 'testuser4', 'Test User 4'), + new User(99997, new \DateTime(), 'testuser3', 'Test User 3'), + new User(99996, new \DateTime(), 'testuser4', 'Test User 4'), ]; return new UserSubscribersUpdatedEvent($user, $subscribed, $unsubscribed); diff --git a/tests/Skobkin/PointToolsBundle/Event/UsersRenamedEventTest.php b/tests/Skobkin/PointToolsBundle/Event/UsersRenamedEventTest.php index 50fef8a..b4464c4 100644 --- a/tests/Skobkin/PointToolsBundle/Event/UsersRenamedEventTest.php +++ b/tests/Skobkin/PointToolsBundle/Event/UsersRenamedEventTest.php @@ -15,7 +15,7 @@ class UsersRenamedEventTest extends \PHPUnit_Framework_TestCase public function testGetRenames() { - $user = new User(99999, 'testuser', 'Test User 1'); + $user = new User(99999, new \DateTime(), 'testuser', 'Test User 1'); $renameRecords = [ new UserRenameEvent($user, 'testuser_old1'), new UserRenameEvent($user, 'testuser_old2'),