From 38d0ab50be8811b61aed89088cfa761893272be3 Mon Sep 17 00:00:00 2001 From: Alexey Skobkin Date: Tue, 17 Jan 2017 04:36:35 +0300 Subject: [PATCH] Revert: User removal support. --- .../Version20170116224555.php | 34 ------------- .../Command/ImportUsersCommand.php | 10 +++- .../Command/UpdateSubscriptionsCommand.php | 20 ++------ .../DataFixtures/ORM/LoadUserData.php | 4 +- .../Bundle/PointToolsBundle/Entity/User.php | 48 ++++++++----------- .../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, 65 insertions(+), 125 deletions(-) delete mode 100644 app/DoctrineMigrations/Version20170116224555.php diff --git a/app/DoctrineMigrations/Version20170116224555.php b/app/DoctrineMigrations/Version20170116224555.php deleted file mode 100644 index f2fb05a..0000000 --- a/app/DoctrineMigrations/Version20170116224555.php +++ /dev/null @@ -1,34 +0,0 @@ -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 a5bd800..87de45b 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Command/ImportUsersCommand.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Command/ImportUsersCommand.php @@ -75,9 +75,15 @@ class ImportUsersCommand extends ContainerAwareCommand continue; } - $createdAt = \DateTime::createFromFormat('Y-m-d_H:i:s', $row[3]) ?: new \DateTime(); + $createdAt = \DateTime::createFromFormat('Y-m-d_H:i:s', $row[3]); - $user = new User($row[0], $createdAt, $row[1], $row[2]); + if (!$createdAt) { + $createdAt = new \DateTime(); + } + + $user = (new User($row[0], $row[1], $row[2])) + ->setCreatedAt($createdAt) + ; 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 feb87f1..001ca97 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Command/UpdateSubscriptionsCommand.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Command/UpdateSubscriptionsCommand.php @@ -6,7 +6,6 @@ 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; @@ -186,11 +185,6 @@ 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.', @@ -228,16 +222,14 @@ class UpdateSubscriptionsCommand extends ContainerAwareCommand private function getUsersForUpdate(int $appUserId): array { - $usersForUpdate = []; - if ($this->input->getOption('all-users')) { - $usersForUpdate = $this->userRepo->findBy(['removed' => false]); + $usersForUpdate = $this->userRepo->findAll(); } else { /** @var User $serviceUser */ - $serviceUser = $this->userRepo->findActiveUserWithSubscribers($appUserId); + $serviceUser = $this->userRepo->find($appUserId); if (!$serviceUser) { - $this->logger->critical('Service user not found'); + $this->logger->info('Service user not found'); // @todo Retrieving user throw new \RuntimeException('Service user not found in the database'); @@ -247,10 +239,6 @@ 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.', @@ -263,6 +251,8 @@ 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 db6a431..3e5ae0f 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/DataFixtures/ORM/LoadUserData.php +++ b/src/Skobkin/Bundle/PointToolsBundle/DataFixtures/ORM/LoadUserData.php @@ -27,7 +27,9 @@ class LoadUserData extends AbstractFixture implements OrderedFixtureInterface $userId = 99999; foreach ($this->users as $userData) { - $user = new User($userId--, new \DateTime(), $userData['login'], $userData['name']); + $user = (new User($userId--, $userData['login'], $userData['name'])) + ->setCreatedAt(new \DateTime()) + ; $om->persist($user); diff --git a/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php b/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php index 7ed3ccc..69adb10 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Entity/User.php @@ -68,25 +68,17 @@ 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, \DateTime $createdAt = null, string $login = null, string $name = null) + public function __construct(int $id, string $login = null, string $name = null) { $this->id = $id; $this->login = $login; $this->name = $name; - $this->createdAt = $createdAt ?: new \DateTime(); + $this->createdAt = new \DateTime(); $this->subscribers = new ArrayCollection(); $this->subscriptions = new ArrayCollection(); @@ -106,25 +98,30 @@ 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 getName(): ?string + public function setName(?string $name): self { - return $this->name; - } - - public function updateLoginAndName(string $login, string $name): self - { - $this->login = $login; $this->name = $name; return $this; } + public function getName(): ?string + { + return $this->name; + } + public function addSubscriber(Subscription $subscribers): self { $this->subscribers[] = $subscribers; @@ -173,18 +170,15 @@ 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 2ee26f1..8c1890a 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Repository/UserRepository.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Repository/UserRepository.php @@ -13,19 +13,6 @@ 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 eec91f7..3970db7 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Service/Api/AbstractApi.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Service/Api/AbstractApi.php @@ -51,6 +51,22 @@ 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 * @@ -81,22 +97,6 @@ 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,12 +151,6 @@ 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 d9ff639..2c7fc2e 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Service/Factory/UserFactory.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Service/Factory/UserFactory.php @@ -39,14 +39,16 @@ class UserFactory extends AbstractFactory /** @var User $user */ if (null === ($user = $this->userRepository->find($userData->getId()))) { - $user = new User( - $userData->getId(), - \DateTime::createFromFormat('Y-m-d_H:i:s', $userData->getCreated()) ?: new \DateTime() - ); + // Creating new user + $user = new User($userData->getId()); $this->userRepository->add($user); } - $user->updateLoginAndName($userData->getLogin(), $userData->getName()); + // Updating data + $user + ->setLogin($userData->getLogin()) + ->setName($userData->getName()) + ; return $user; } diff --git a/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php b/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php index fc839ea..79312a5 100644 --- a/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php +++ b/src/Skobkin/Bundle/PointToolsBundle/Service/SubscriptionsManager.php @@ -52,7 +52,6 @@ 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 3853c04..56a6ab3 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, new \DateTime(), 'testuser', 'Test User 1'); + $user = new User(99999, 'testuser', 'Test User 1'); $subscribed = [ - new User(99998, new \DateTime(), 'testuser2', 'Test User 2'), + new User(99998, 'testuser2', 'Test User 2'), ]; $unsubscribed = [ - new User(99997, new \DateTime(), 'testuser3', 'Test User 3'), - new User(99996, new \DateTime(), 'testuser4', 'Test User 4'), + new User(99997, 'testuser3', 'Test User 3'), + new User(99996, '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 b4464c4..50fef8a 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, new \DateTime(), 'testuser', 'Test User 1'); + $user = new User(99999, 'testuser', 'Test User 1'); $renameRecords = [ new UserRenameEvent($user, 'testuser_old1'), new UserRenameEvent($user, 'testuser_old2'),