Skip to content

Commit

Permalink
Fix star sync
Browse files Browse the repository at this point in the history 
Star sync was based on repo full_name
But this value can change during the sync (for example when a repo change its name) so it become inaccurate.
  • Loading branch information
@j0k3r
j0k3r committed Mar 20, 2017
1 parent b76aca9 commit bc852dc
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 71 deletions.
23 changes: 10 additions & 13 deletions src/AppBundle/Consumer/SyncStarredRepos.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,6 @@ private function doSyncRepo(User $user)
]);

foreach ($starredRepos as $starredRepo) {
$newStars[] = $starredRepo['full_name'];

$repo = $this->repoRepository->find($starredRepo['id']);

// if repo doesn't exist
Expand All @@ -116,7 +114,11 @@ private function doSyncRepo(User $user)
$em->persist($repo);
}

if (false === in_array($repo->getFullName(), $currentStars, true)) {
// store current repo id to compare it later when we'll sync removed star
// using `id` instead of `full_name` to be more accurated (full_name can change)
$newStars[] = $repo->getId();

if (false === in_array($repo->getId(), $currentStars, true)) {
$star = new Star($user, $repo);

$em->persist($star);
Expand All @@ -139,27 +141,22 @@ private function doSyncRepo(User $user)
* When user unstar a repo we also need to remove that association.
*
* @param User $user
* @param array $newStars Current stars of the user
* @param array $newStars Current starred repos Id of the user
*
* @return mixed
*/
private function doCleanOldStar(User $user, array $newStars)
{
$currentStars = $this->starRepository->findAllByUser($user->getId());

$starsToRemove = array_diff($currentStars, $newStars);
$repoIdsToRemove = array_diff($currentStars, $newStars);

if (empty($starsToRemove)) {
if (empty($repoIdsToRemove)) {
return;
}

$starIds = [];
foreach ($starsToRemove as $starToRemove) {
$starIds[] = $this->repoRepository->findOneBy(['fullName' => $starToRemove])->getId();
}

$this->logger->notice('Removed stars: ' . count($starIds), ['user' => $user->getUsername()]);
$this->logger->notice('Removed stars: ' . count($repoIdsToRemove), ['user' => $user->getUsername()]);

return $this->starRepository->removeFromUser($starIds, $user->getId());
return $this->starRepository->removeFromUser($repoIdsToRemove, $user->getId());
}
}
10 changes: 5 additions & 5 deletions src/AppBundle/Repository/StarRepository.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ class StarRepository extends \Doctrine\ORM\EntityRepository
public function findAllByUser($userId)
{
$repos = $this->createQueryBuilder('s')
->select('r.fullName')
->select('r.id')
->leftJoin('s.repo', 'r')
->where('s.user = ' . $userId)
->getQuery()
->getArrayResult();

$res = [];
foreach ($repos as $repo) {
$res[] = $repo['fullName'];
$res[] = $repo['id'];
}

return $res;
Expand All @@ -37,16 +37,16 @@ public function findAllByUser($userId)
/**
* Remove stars for a user.
*
* @param array $starIds
* @param array $repoIds
* @param int $userId
*
* @return mixed
*/
public function removeFromUser(array $starIds, $userId)
public function removeFromUser(array $repoIds, $userId)
{
return $this->createQueryBuilder('s')
->delete()
->where('s.repo IN (:ids)')->setParameter('ids', $starIds)
->where('s.repo IN (:ids)')->setParameter('ids', $repoIds)
->andWhere('s.user = :userId')->setParameter('userId', $userId)
->getQuery()
->execute();
Expand Down
86 changes: 33 additions & 53 deletions tests/AppBundle/Consumer/SyncStarredReposTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace Tests\AppBundle\Consumer;

use AppBundle\Consumer\SyncStarredRepos;
use AppBundle\Entity\Repo;
use AppBundle\Entity\User;
use Github\Client as GithubClient;
use Github\HttpClient\Builder;
Expand Down Expand Up @@ -98,25 +99,26 @@ public function testProcessSuccessfulMessage()
$starRepository->expects($this->exactly(2))
->method('findAllByUser')
->with(123)
->willReturn(['removed/star-repo']);
->willReturn([666, 777]);

$star = $this->getMockBuilder('AppBundle\Entity\Star')
->disableOriginalConstructor()
->getMock();
$starRepository->expects($this->once())
->method('removeFromUser')
->with([1 => 777], 123)
->willReturn(true);

$star->expects($this->once())
->method('getId')
->with()
->willReturn(1);
$repo = new Repo();
$repo->setId(666);
$repo->setFullName('j0k3r/banditore');
$repo->setUpdatedAt((new \DateTime())->setTimestamp(time() - 3600 * 72));

$repoRepository = $this->getMockBuilder('AppBundle\Repository\RepoRepository')
->disableOriginalConstructor()
->getMock();

$repoRepository->expects($this->once())
->method('findOneBy')
->with(['fullName' => 'removed/star-repo'])
->willReturn($star);
->method('find')
->with(666)
->willReturn($repo);

$responses = new MockHandler([
// first /user/starred
Expand All @@ -133,20 +135,6 @@ public function testProcessSuccessfulMessage()
]])),
// /rate_limit
new Response(200, ['Content-Type' => 'application/json'], json_encode(['resources' => ['core' => ['remaining' => 10]]])),
// second /user/starred
new Response(200, ['Content-Type' => 'application/json'], json_encode([[
'description' => 'banditore',
'homepage' => 'http://banditore.io',
'language' => 'PHP',
'name' => 'banditore',
'full_name' => 'j0k3r/banditore',
'id' => 666,
'owner' => [
'avatar_url' => 'http://avatar.api/banditore.jpg',
],
]])),
// /rate_limit
new Response(200, ['Content-Type' => 'application/json'], json_encode(['resources' => ['core' => ['remaining' => 10]]])),
// third /user/starred will return empty response which means, we reached the last page
new Response(200, ['Content-Type' => 'application/json'], json_encode([])),
]);
Expand All @@ -172,9 +160,8 @@ public function testProcessSuccessfulMessage()

$this->assertSame('Consume banditore.sync_starred_repos message', $records[0]['message']);
$this->assertSame(' sync 1 starred repos', $records[1]['message']);
$this->assertSame(' sync 1 starred repos', $records[2]['message']);
$this->assertSame('Removed stars: 1', $records[3]['message']);
$this->assertSame('Synced repos: 2', $records[4]['message']);
$this->assertSame('Removed stars: 1', $records[2]['message']);
$this->assertSame('Synced repos: 1', $records[3]['message']);
}

/**
Expand Down Expand Up @@ -215,25 +202,18 @@ public function testProcessUnexpectedError()
->disableOriginalConstructor()
->getMock();

$starRepository->expects($this->exactly(2))
$starRepository->expects($this->once())
->method('findAllByUser')
->with(123)
->willReturn(['removed/star-repo']);

$star = $this->getMockBuilder('AppBundle\Entity\Star')
->disableOriginalConstructor()
->getMock();

$star->expects($this->never())
->method('getId');
->willReturn([666]);

$repoRepository = $this->getMockBuilder('AppBundle\Repository\RepoRepository')
->disableOriginalConstructor()
->getMock();

$repoRepository->expects($this->once())
->method('findOneBy')
->with(['fullName' => 'removed/star-repo'])
->method('find')
->with(666)
->will($this->throwException(new \Exception('booboo')));

$responses = new MockHandler([
Expand Down Expand Up @@ -312,21 +292,21 @@ public function testProcessSuccessfulMessageNoStarToRemove()
$starRepository->expects($this->exactly(2))
->method('findAllByUser')
->with(123)
->willReturn(['j0k3r/banditore']);
->willReturn([123]);

$star = $this->getMockBuilder('AppBundle\Entity\Star')
->disableOriginalConstructor()
->getMock();

$star->expects($this->never())
->method('getId');
$repo = new Repo();
$repo->setId(123);
$repo->setFullName('j0k3r/banditore');
$repo->setUpdatedAt((new \DateTime())->setTimestamp(time() - 3600 * 72));

$repoRepository = $this->getMockBuilder('AppBundle\Repository\RepoRepository')
->disableOriginalConstructor()
->getMock();

$repoRepository->expects($this->never())
->method('findOneBy');
$repoRepository->expects($this->once())
->method('find')
->with(123)
->willReturn($repo);

$responses = new MockHandler([
// first /user/starred
Expand Down Expand Up @@ -396,7 +376,7 @@ public function testProcessWithBadClient()
->getMock();

$repoRepository->expects($this->never())
->method('findOneBy');
->method('find');

$logger = new Logger('foo');
$logHandler = new TestHandler();
Expand Down Expand Up @@ -466,8 +446,8 @@ public function testFunctionalConsumer()
// before import
$stars = $container->get('banditore.repository.star')->findAllByUser(123);
$this->assertCount(2, $stars, 'User 123 has 2 starred repos');
$this->assertSame('symfony/symfony', $stars[0], 'User 123 has "symfony/symfony" starred repo');
$this->assertSame('test/test', $stars[1], 'User 123 has "test/test" starred repo');
$this->assertSame(555, $stars[0], 'User 123 has "symfony/symfony" starred repo');
$this->assertSame(666, $stars[1], 'User 123 has "test/test" starred repo');

$processor->process(new Message(json_encode(['user_id' => 123])), []);

Expand All @@ -478,8 +458,8 @@ public function testFunctionalConsumer()
// validate that `test/test` association got removed
$stars = $container->get('banditore.repository.star')->findAllByUser(123);
$this->assertCount(2, $stars, 'User 123 has 2 starred repos');
$this->assertSame('test/test', $stars[0], 'User 123 has "test/test" starred repo');
$this->assertSame('j0k3r/banditore', $stars[1], 'User 123 has "j0k3r/banditore" starred repo');
$this->assertSame(666, $stars[0], 'User 123 has "test/test" starred repo');
$this->assertSame(777, $stars[1], 'User 123 has "j0k3r/banditore" starred repo');
}

private function getMockClient($responses)
Expand Down

0 comments on commit bc852dc

Please sign in to comment.