diff --git a/config/services.php b/config/services.php index bd9a6c98e..a82418580 100644 --- a/config/services.php +++ b/config/services.php @@ -89,6 +89,7 @@ ->bind('$gitlabCommentSyncEnabled', '%env(bool:GITLAB_COMMENT_SYNC)%') ->bind('$gitlabApiUrl', '%env(GITLAB_API_URL)%') ->bind('$applicationName', '%env(APP_NAME)%') + ->bind('$appAbsoluteUrl', '%env(APP_ABSOLUTE_URL)%') ->bind('$codeReviewExcludeAuthors', '%env(CODE_REVIEW_EXCLUDE_AUTHORS)%') ->bind(ProviderInterface::class . ' $collectionProvider', service(CollectionProvider::class)); diff --git a/src/MessageHandler/Gitlab/CommentAddedMessageHandler.php b/src/MessageHandler/Gitlab/CommentAddedMessageHandler.php index cefd84b24..76ab6f623 100644 --- a/src/MessageHandler/Gitlab/CommentAddedMessageHandler.php +++ b/src/MessageHandler/Gitlab/CommentAddedMessageHandler.php @@ -55,6 +55,11 @@ public function __invoke(CommentAdded $event): void return; } - $this->commentService->create($api, $comment, $mergeRequestIId); + try { + $this->commentService->create($api, $comment, $mergeRequestIId); + } catch (Throwable $exception) { + $this->commentService->updateExtReferenceId($api, $comment, $mergeRequestIId); + throw $exception; + } } } diff --git a/src/Service/Api/Gitlab/Discussions.php b/src/Service/Api/Gitlab/Discussions.php index 96c913861..cbb8bc007 100644 --- a/src/Service/Api/Gitlab/Discussions.php +++ b/src/Service/Api/Gitlab/Discussions.php @@ -5,6 +5,7 @@ use DR\Review\Model\Api\Gitlab\Position; use DR\Utils\Arrays; +use Generator; use Symfony\Contracts\HttpClient\HttpClientInterface; use Throwable; @@ -14,6 +15,39 @@ public function __construct(private readonly HttpClientInterface $client) { } + /** + * @phpstan-return Generator + * }> + * @throws Throwable + * @link https://docs.gitlab.com/ee/api/discussions.html#merge-requests + * @link https://docs.gitlab.com/ee/api/rest/index.html#pagination-link-header + */ + public function getDiscussions(int $projectId, int $mergeRequestIId, int $perPage = 20): Generator + { + $page = 1; + do { + $response = $this->client->request( + 'GET', + sprintf('projects/%d/merge_requests/%d/discussions', $projectId, $mergeRequestIId), + ['query' => ['per_page' => $perPage, 'page' => $page]] + ); + $page = (int)($response->getHeaders()['x-next-page'][0] ?? -1); + yield from $response->toArray(); + } while ($page > 0); + } + /** * @throws Throwable * @link https://docs.gitlab.com/ee/api/discussions.html#create-a-new-thread-in-the-merge-request-diff diff --git a/src/Service/Api/Gitlab/GitlabCommentFormatter.php b/src/Service/Api/Gitlab/GitlabCommentFormatter.php new file mode 100644 index 000000000..3b9a6d6a0 --- /dev/null +++ b/src/Service/Api/Gitlab/GitlabCommentFormatter.php @@ -0,0 +1,28 @@ +appAbsoluteUrl . $this->urlGenerator->generate(ReviewController::class, ['review' => $comment->getReview()]); + + $message = str_replace("\n", "\n
", $comment->getMessage()); + + return sprintf("%s\n
\n
*123view: %s*", $message, $url); + } +} diff --git a/src/Service/Api/Gitlab/GitlabCommentService.php b/src/Service/Api/Gitlab/GitlabCommentService.php index 5300f662e..a62ca6c79 100644 --- a/src/Service/Api/Gitlab/GitlabCommentService.php +++ b/src/Service/Api/Gitlab/GitlabCommentService.php @@ -15,8 +15,11 @@ class GitlabCommentService implements LoggerAwareInterface { use LoggerAwareTrait; - public function __construct(private readonly PositionFactory $positionFactory, private readonly CommentRepository $commentRepository) - { + public function __construct( + private readonly PositionFactory $positionFactory, + private readonly CommentRepository $commentRepository, + private readonly GitlabCommentFormatter $commentFormatter, + ) { } /** @@ -47,11 +50,42 @@ public function create(GitlabApi $api, Comment $comment, int $mergeRequestIId): 'Adding comment in gitlab: {projectId} {mergeRequestIId} {comment}', ['projectId' => $projectId, 'mergeRequestIId' => $mergeRequestIId, 'comment' => $comment->getMessage()] ); - $referenceId = $api->discussions()->createDiscussion($projectId, $mergeRequestIId, $position, $comment->getMessage()); + + $message = $this->commentFormatter->format($comment); + $referenceId = $api->discussions()->createDiscussion($projectId, $mergeRequestIId, $position, $message); $comment->setExtReferenceId($referenceId); $this->commentRepository->save($comment, true); } + /** + * @throws Throwable + */ + public function updateExtReferenceId(GitlabApi $api, Comment $comment, int $mergeRequestIId): void + { + $projectId = (int)$comment->getReview()->getRepository()->getRepositoryProperty('gitlab-project-id'); + $reference = $comment->getLineReference(); + + foreach ($api->discussions()->getDiscussions($projectId, $mergeRequestIId) as $thread) { + foreach ($thread['notes'] as $note) { + // try to match body + if ($note['body'] !== $comment->getMessage()) { + continue; + } + + // should at least match either new path or old path + if ($note['position']['old_path'] !== $reference->oldPath && $note['position']['new_path'] !== $reference->newPath) { + continue; + } + + // set reference id and save + $referenceId = sprintf('%s:%s:%s', $mergeRequestIId, $thread['id'], $note['id']); + $this->commentRepository->save($comment->setExtReferenceId($referenceId), true); + + return; + } + } + } + /** * @throws Throwable */ @@ -70,7 +104,9 @@ public function update(GitlabApi $api, Comment $comment): void 'Updating comment in gitlab: {projectId} {mergeRequestIId} {discussionId}', ['projectId' => $projectId, 'mergeRequestIId' => $mergeRequestIId, 'discussionId' => $discussionId] ); - $api->discussions()->updateNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId, $comment->getMessage()); + + $message = $this->commentFormatter->format($comment); + $api->discussions()->updateNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId, $message); } /** @@ -107,6 +143,12 @@ public function delete(GitlabApi $api, Repository $repository, string $extRefere 'Deleting comment in gitlab: {projectId} {mergeRequestIId} {discussionId}', ['projectId' => $projectId, 'mergeRequestIId' => $mergeRequestIId, 'discussionId' => $discussionId] ); - $api->discussions()->deleteNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId); + try { + $api->discussions()->deleteNote($projectId, (int)$mergeRequestIId, $discussionId, $noteId); + // @codeCoverageIgnoreStart + } catch (Throwable $exception) { + $this->logger?->notice('Failed to remove note from Gitlab', ['exception' => $exception]); + } + // @codeCoverageIgnoreEnd } } diff --git a/tests/AbstractTestCase.php b/tests/AbstractTestCase.php index ea7530770..72703fe7d 100644 --- a/tests/AbstractTestCase.php +++ b/tests/AbstractTestCase.php @@ -4,6 +4,7 @@ namespace DR\Review\Tests; use DigitalRevolution\AccessorPairConstraint\AccessorPairAsserter; +use Generator; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; @@ -18,9 +19,7 @@ abstract class AbstractTestCase extends TestCase use AccessorPairAsserter; use TestTrait; - /** @var MockObject&LoggerInterface */ - protected LoggerInterface $logger; - + protected MockObject&LoggerInterface $logger; protected Envelope $envelope; protected function setUp(): void @@ -28,4 +27,15 @@ protected function setUp(): void $this->envelope = new Envelope(new stdClass(), []); $this->logger = $this->createMock(LoggerInterface::class); } + + /** + * @template T + * @param T[] $items + * + * @return Generator + */ + protected static function createGeneratorFrom(array $items): Generator + { + yield from $items; + } } diff --git a/tests/Unit/MessageHandler/Gitlab/CommentAddedMessageHandlerTest.php b/tests/Unit/MessageHandler/Gitlab/CommentAddedMessageHandlerTest.php index 8325d3284..9b012e79d 100644 --- a/tests/Unit/MessageHandler/Gitlab/CommentAddedMessageHandlerTest.php +++ b/tests/Unit/MessageHandler/Gitlab/CommentAddedMessageHandlerTest.php @@ -17,6 +17,7 @@ use DR\Review\Tests\AbstractTestCase; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\MockObject\MockObject; +use RuntimeException; use Throwable; #[CoversClass(CommentAddedMessageHandler::class)] @@ -127,7 +128,35 @@ public function testInvokeSuccess(): void $this->apiProvider->expects(self::once())->method('create')->with($repository, $user)->willReturn($api); $this->mergeRequestService->expects(self::once())->method('retrieveMergeRequestIID')->with($api, $review)->willReturn(12345); $this->commentService->expects(self::once())->method('create')->with($api, $comment, 12345); + $this->commentService->expects(self::never())->method('updateExtReferenceId'); ($this->handler)(new CommentAdded(111, 222, 333, 'file', 'message')); } + + /** + * @throws Throwable + */ + public function testInvokeFailureRetry(): void + { + $user = new User(); + $repository = new Repository(); + + $review = new CodeReview(); + $review->setRepository($repository); + + $comment = new Comment(); + $comment->setReview($review); + $comment->setUser($user); + + $api = $this->createMock(GitlabApi::class); + + $this->commentRepository->expects(self::once())->method('find')->with(222)->willReturn($comment); + $this->apiProvider->expects(self::once())->method('create')->with($repository, $user)->willReturn($api); + $this->mergeRequestService->expects(self::once())->method('retrieveMergeRequestIID')->with($api, $review)->willReturn(12345); + $this->commentService->expects(self::once())->method('create')->with($api, $comment, 12345)->willThrowException(new RuntimeException('foo')); + $this->commentService->expects(self::once())->method('updateExtReferenceId')->with($api, $comment, 12345); + + $this->expectException(RuntimeException::class); + ($this->handler)(new CommentAdded(111, 222, 333, 'file', 'message')); + } } diff --git a/tests/Unit/Service/Api/Gitlab/DiscussionsTest.php b/tests/Unit/Service/Api/Gitlab/DiscussionsTest.php index a52f9fd35..08e01cc76 100644 --- a/tests/Unit/Service/Api/Gitlab/DiscussionsTest.php +++ b/tests/Unit/Service/Api/Gitlab/DiscussionsTest.php @@ -11,6 +11,7 @@ use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; use Throwable; +use function DR\PHPUnitExtensions\Mock\consecutive; #[CoversClass(Discussions::class)] class DiscussionsTest extends AbstractTestCase @@ -25,6 +26,42 @@ protected function setUp(): void $this->discussions = new Discussions($this->client); } + /** + * @throws Throwable + */ + public function testGetDiscussions(): void + { + $discussionA = ['id' => 333, 'notes' => [['id' => 444]]]; + $discussionB = ['id' => 555, 'notes' => [['id' => 666]]]; + + $response = $this->createMock(ResponseInterface::class); + $response->method('getHeaders')->willReturn(['x-next-page' => ['2']], ['x-next-page' => []]); + $response->method('toArray')->willReturn([$discussionA], [$discussionB]); + + $this->client->expects(self::exactly(2)) + ->method('request') + ->with( + ...consecutive( + [ + 'GET', + 'projects/111/merge_requests/222/discussions', + ['query' => ['per_page' => 20, 'page' => 1]] + ], + [ + 'GET', + 'projects/111/merge_requests/222/discussions', + ['query' => ['per_page' => 20, 'page' => 2]] + ] + ) + )->willReturn($response); + + $discussions = []; + foreach ($this->discussions->getDiscussions(111, 222) as $discussion) { + $discussions[] = $discussion; + } + static::assertSame([$discussionA, $discussionB], $discussions); + } + /** * @throws Throwable */ diff --git a/tests/Unit/Service/Api/Gitlab/GitlabCommentFormatterTest.php b/tests/Unit/Service/Api/Gitlab/GitlabCommentFormatterTest.php new file mode 100644 index 000000000..999d0f8a5 --- /dev/null +++ b/tests/Unit/Service/Api/Gitlab/GitlabCommentFormatterTest.php @@ -0,0 +1,42 @@ +urlGenerator = $this->createMock(UrlGeneratorInterface::class); + $this->formatter = new GitlabCommentFormatter('https://example.com', $this->urlGenerator); + } + + public function testFormat(): void + { + $review = new CodeReview(); + $comment = new Comment(); + $comment->setMessage("foo\nbar"); + $comment->setReview($review); + + $this->urlGenerator->expects(self::once()) + ->method('generate') + ->with(ReviewController::class, ['review' => $review]) + ->willReturn('/path/to/review'); + + static::assertSame("foo\n
bar\n
\n
*123view: https://example.com/path/to/review*", $this->formatter->format($comment)); + } +} diff --git a/tests/Unit/Service/Api/Gitlab/GitlabCommentServiceTest.php b/tests/Unit/Service/Api/Gitlab/GitlabCommentServiceTest.php index 45126c074..8fdabd8cd 100644 --- a/tests/Unit/Service/Api/Gitlab/GitlabCommentServiceTest.php +++ b/tests/Unit/Service/Api/Gitlab/GitlabCommentServiceTest.php @@ -14,6 +14,7 @@ use DR\Review\Repository\Review\CommentRepository; use DR\Review\Service\Api\Gitlab\Discussions; use DR\Review\Service\Api\Gitlab\GitlabApi; +use DR\Review\Service\Api\Gitlab\GitlabCommentFormatter; use DR\Review\Service\Api\Gitlab\GitlabCommentService; use DR\Review\Service\Api\Gitlab\MergeRequests; use DR\Review\Service\Api\Gitlab\PositionFactory; @@ -25,15 +26,16 @@ #[CoversClass(GitlabCommentService::class)] class GitlabCommentServiceTest extends AbstractTestCase { - private PositionFactory&MockObject $positionFactory; - private CommentRepository&MockObject $commentRepository; - private GitlabApi&MockObject $api; - private MergeRequests&MockObject $mergeRequests; - private Discussions&MockObject $discussions; - private Comment $comment; - private CodeReview $review; - private Repository $repository; - private GitlabCommentService $service; + private PositionFactory&MockObject $positionFactory; + private CommentRepository&MockObject $commentRepository; + private GitlabCommentFormatter&MockObject $commentFormatter; + private GitlabApi&MockObject $api; + private MergeRequests&MockObject $mergeRequests; + private Discussions&MockObject $discussions; + private Comment $comment; + private CodeReview $review; + private Repository $repository; + private GitlabCommentService $service; protected function setUp(): void { @@ -48,7 +50,8 @@ protected function setUp(): void $this->api->method('discussions')->willReturn($this->discussions); $this->positionFactory = $this->createMock(PositionFactory::class); $this->commentRepository = $this->createMock(CommentRepository::class); - $this->service = new GitlabCommentService($this->positionFactory, $this->commentRepository); + $this->commentFormatter = $this->createMock(GitlabCommentFormatter::class); + $this->service = new GitlabCommentService($this->positionFactory, $this->commentRepository, $this->commentFormatter); } /** @@ -92,13 +95,38 @@ public function testCreateWithVersion(): void $this->mergeRequests->expects(self::once())->method('versions')->with(123, 456)->willReturn([$version]); $this->positionFactory->expects(self::once())->method('create')->with($version, $lineReference)->willReturn($position); - $this->discussions->expects(self::once())->method('createDiscussion')->with(123, 456, $position, 'message')->willReturn('1:2:3'); + $this->commentFormatter->expects(self::once())->method('format')->with($this->comment)->willReturn('formatted'); + $this->discussions->expects(self::once())->method('createDiscussion')->with(123, 456, $position, 'formatted')->willReturn('1:2:3'); $this->commentRepository->expects(self::once())->method('save')->with($this->comment, true); $this->service->create($this->api, $this->comment, 456); static::assertSame('1:2:3', $this->comment->getExtReferenceId()); } + /** + * @throws Throwable + */ + public function testUpdateExtReferenceId(): void + { + $lineReference = new LineReference('old', 'new', 1, 2, 3, null, LineReferenceStateEnum::Added); + $this->comment->setReview($this->review); + $this->comment->setLineReference($lineReference); + $this->comment->setMessage('match'); + $this->review->setRepository($this->repository); + $this->repository->setRepositoryProperty(new RepositoryProperty('gitlab-project-id', '123')); + + $threads = [ + ['id' => '1', 'notes' => [['id' => '2', 'body' => 'foobar', 'position' => ['old_path' => 'old', 'new_path' => 'new']]]], + ['id' => '2', 'notes' => [['id' => '2', 'body' => 'match', 'position' => ['old_path' => 'foo', 'new_path' => 'bar']]]], + ['id' => '3', 'notes' => [['id' => '2', 'body' => 'match', 'position' => ['old_path' => 'old', 'new_path' => 'new']]]] + ]; + + $this->discussions->expects(self::once())->method('getDiscussions')->with(123, 456)->willReturn(static::createGeneratorFrom($threads)); + $this->commentRepository->expects(self::once())->method('save')->with($this->comment, true); + + $this->service->updateExtReferenceId($this->api, $this->comment, 456); + } + /** * @throws Throwable */ @@ -121,7 +149,8 @@ public function testUpdate(): void $this->review->setRepository($this->repository); $this->repository->setRepositoryProperty(new RepositoryProperty('gitlab-project-id', '111')); - $this->discussions->expects(self::once())->method('updateNote')->with(111, 222, '333', '444', 'message'); + $this->commentFormatter->expects(self::once())->method('format')->with($this->comment)->willReturn('formatted'); + $this->discussions->expects(self::once())->method('updateNote')->with(111, 222, '333', '444', 'formatted'); $this->service->update($this->api, $this->comment); }