diff --git a/src/Api/Issue/GithubIssueApi.php b/src/Api/Issue/GithubIssueApi.php index bf55f80..5d3c6f6 100644 --- a/src/Api/Issue/GithubIssueApi.php +++ b/src/Api/Issue/GithubIssueApi.php @@ -88,4 +88,25 @@ public function findStaleIssues(Repository $repository, \DateTimeImmutable $noUp 'desc', ]); } + + public function findBotComment(Repository $repository, int $issueNumber, string $search): ?int + { + $allComments = $this->issueCommentApi->all($repository->getVendor(), $repository->getName(), $issueNumber, ['per_page' => 100]); + foreach (array_reverse($allComments) as $comment) { + if ($this->botUsername !== ($comment['user']['login'] ?? null)) { + continue; + } + + if (str_contains($comment['body'] ?? '', $search)) { + return $comment['id']; + } + } + + return null; + } + + public function removeComment(Repository $repository, int $commentId): void + { + $this->issueCommentApi->remove($repository->getVendor(), $repository->getName(), $commentId); + } } diff --git a/src/Api/Issue/IssueApi.php b/src/Api/Issue/IssueApi.php index 9e339b3..a6ddc34 100644 --- a/src/Api/Issue/IssueApi.php +++ b/src/Api/Issue/IssueApi.php @@ -37,4 +37,8 @@ public function findStaleIssues(Repository $repository, \DateTimeImmutable $noUp * Close an issue and mark it as "not_planned". */ public function close(Repository $repository, int $issueNumber): void; + + public function findBotComment(Repository $repository, int $issueNumber, string $search): ?int; + + public function removeComment(Repository $repository, int $commentId): void; } diff --git a/src/Api/Issue/NullIssueApi.php b/src/Api/Issue/NullIssueApi.php index 30b0c13..2abedbd 100644 --- a/src/Api/Issue/NullIssueApi.php +++ b/src/Api/Issue/NullIssueApi.php @@ -32,4 +32,13 @@ public function findStaleIssues(Repository $repository, \DateTimeImmutable $noUp public function close(Repository $repository, int $issueNumber): void { } + + public function findBotComment(Repository $repository, int $issueNumber, string $search): ?int + { + return null; + } + + public function removeComment(Repository $repository, int $commentId): void + { + } } diff --git a/src/Subscriber/AllowEditFromMaintainerSubscriber.php b/src/Subscriber/AllowEditFromMaintainerSubscriber.php index ddcec6e..7fc4da0 100644 --- a/src/Subscriber/AllowEditFromMaintainerSubscriber.php +++ b/src/Subscriber/AllowEditFromMaintainerSubscriber.php @@ -20,20 +20,31 @@ public function __construct( public function onPullRequest(GitHubEvent $event): void { $data = $event->getData(); - if (!in_array($data['action'], ['opened', 'ready_for_review']) || ($data['pull_request']['draft'] ?? false)) { + if (!in_array($data['action'], ['opened', 'ready_for_review', 'edited']) || ($data['pull_request']['draft'] ?? false)) { return; } + if ($data['repository']['full_name'] === $data['pull_request']['head']['repo']['full_name']) { + return; + } + + $repository = $event->getRepository(); + $pullRequestNumber = $data['pull_request']['number']; + $commentId = $this->commentsApi->findBotComment($repository, $pullRequestNumber, 'Allow edits from maintainer'); + if ($data['pull_request']['maintainer_can_modify'] ?? true) { + if ($commentId) { + $this->commentsApi->removeComment($event->getRepository(), $commentId); + } + return; } - if ($data['repository']['full_name'] === $data['pull_request']['head']['repo']['full_name']) { + // Avoid duplicate comments + if ($commentId) { return; } - $repository = $event->getRepository(); - $pullRequestNumber = $data['pull_request']['number']; $this->commentsApi->commentOnIssue($repository, $pullRequestNumber, <<getData(); - if (!in_array($data['action'], ['opened', 'ready_for_review']) || ($data['pull_request']['draft'] ?? false)) { + if (!in_array($data['action'], ['opened', 'ready_for_review', 'edited']) || ($data['pull_request']['draft'] ?? false)) { return; } @@ -38,7 +38,18 @@ public function onPullRequest(GitHubEvent $event): void } $targetBranch = $data['pull_request']['base']['ref']; + $commentId = $this->issueApi->findBotComment($event->getRepository(), $number, 'seems your PR description refers to branch'); + if ($targetBranch === $descriptionBranch) { + if ($commentId) { + $this->issueApi->removeComment($event->getRepository(), $commentId); + } + + return; + } + + // Avoid duplicate comments + if ($commentId) { return; } diff --git a/src/Subscriber/UnsupportedBranchSubscriber.php b/src/Subscriber/UnsupportedBranchSubscriber.php index 1a5639e..0882c38 100644 --- a/src/Subscriber/UnsupportedBranchSubscriber.php +++ b/src/Subscriber/UnsupportedBranchSubscriber.php @@ -27,14 +27,11 @@ public function __construct( public function onPullRequest(GitHubEvent $event): void { $data = $event->getData(); - if (!in_array($data['action'], ['opened', 'ready_for_review']) || ($data['pull_request']['draft'] ?? false)) { + if (!in_array($data['action'], ['opened', 'ready_for_review', 'edited']) || ($data['pull_request']['draft'] ?? false)) { return; } $targetBranch = $data['pull_request']['base']['ref']; - if ($targetBranch === $data['repository']['default_branch']) { - return; - } try { $validBranches = $this->symfonyVersionProvider->getMaintainedVersions(); @@ -44,11 +41,22 @@ public function onPullRequest(GitHubEvent $event): void return; } - if (in_array($targetBranch, $validBranches)) { + $number = $data['pull_request']['number']; + $commentId = $this->issueApi->findBotComment($event->getRepository(), $number, 'target one of these branches instead'); + + if ($targetBranch === $data['repository']['default_branch'] || in_array($targetBranch, $validBranches)) { + if ($commentId) { + $this->issueApi->removeComment($event->getRepository(), $commentId); + } + + return; + } + + // Avoid duplicate comments + if ($commentId) { return; } - $number = $data['pull_request']['number']; $validBranchesString = implode(', ', $validBranches); $this->issueApi->commentOnIssue($event->getRepository(), $number, <<issueApi = $this->createMock(NullIssueApi::class); + $this->repository = new Repository('carsonbot-playground', 'symfony', null); + + $subscriber = new AllowEditFromMaintainerSubscriber($this->issueApi); + + $this->dispatcher = new EventDispatcher(); + $this->dispatcher->addSubscriber($subscriber); + } + + public function testOnPullRequestEditedCleanup() + { + $this->issueApi->expects($this->once()) + ->method('findBotComment') + ->with($this->repository, 1234, 'Allow edits from maintainer') + ->willReturn(666); + + $this->issueApi->expects($this->once()) + ->method('removeComment') + ->with($this->repository, 666); + + $event = new GitHubEvent([ + 'action' => 'edited', + 'pull_request' => [ + 'number' => 1234, + 'maintainer_can_modify' => true, + 'head' => ['repo' => ['full_name' => 'contributor/symfony']], + ], + 'repository' => ['full_name' => 'fabpot/symfony', 'owner' => ['login' => 'fabpot'], 'name' => 'symfony'], + ], $this->repository); + + $this->dispatcher->dispatch($event, GitHubEvents::PULL_REQUEST); + } + + public function testOnPullRequestEditedIdempotency() + { + $this->issueApi->expects($this->once()) + ->method('findBotComment') + ->with($this->repository, 1234, 'Allow edits from maintainer') + ->willReturn(666); + + $this->issueApi->expects($this->never()) + ->method('commentOnIssue'); + + $event = new GitHubEvent([ + 'action' => 'edited', + 'pull_request' => [ + 'number' => 1234, + 'maintainer_can_modify' => false, + 'head' => ['repo' => ['full_name' => 'contributor/symfony']], + ], + 'repository' => ['full_name' => 'fabpot/symfony', 'owner' => ['login' => 'fabpot'], 'name' => 'symfony'], + ], $this->repository); + + $this->dispatcher->dispatch($event, GitHubEvents::PULL_REQUEST); + } +} diff --git a/tests/Subscriber/MismatchBranchDescriptionSubscriberTest.php b/tests/Subscriber/MismatchBranchDescriptionSubscriberTest.php index b13cc00..364dc48 100644 --- a/tests/Subscriber/MismatchBranchDescriptionSubscriberTest.php +++ b/tests/Subscriber/MismatchBranchDescriptionSubscriberTest.php @@ -211,4 +211,61 @@ public function testOnPullRequestOpenBranchNotInTable() $this->assertCount(0, $responseData); } + + public function testOnPullRequestEditedCleanup() + { + $this->issueApi->expects($this->once()) + ->method('findBotComment') + ->with($this->repository, 1234, 'seems your PR description refers to branch') + ->willReturn(777); + + $this->issueApi->expects($this->once()) + ->method('removeComment') + ->with($this->repository, 777); + + $body = << 'edited', + 'pull_request' => [ + 'number' => 1234, + 'body' => $body, + 'base' => ['ref' => '6.2'], + ], + ], $this->repository); + + $this->dispatcher->dispatch($event, GitHubEvents::PULL_REQUEST); + } + + public function testOnPullRequestEditedIdempotency() + { + $this->issueApi->expects($this->once()) + ->method('findBotComment') + ->with($this->repository, 1234, 'seems your PR description refers to branch') + ->willReturn(777); + + $this->issueApi->expects($this->never()) + ->method('commentOnIssue'); + + $body = << 'edited', + 'pull_request' => [ + 'number' => 1234, + 'body' => $body, + 'base' => ['ref' => '5.4'], // Mismatch + ], + ], $this->repository); + + $this->dispatcher->dispatch($event, GitHubEvents::PULL_REQUEST); + } } diff --git a/tests/Subscriber/UnsupportedBranchSubscriberTest.php b/tests/Subscriber/UnsupportedBranchSubscriberTest.php index 1fc3e3e..aad09e1 100644 --- a/tests/Subscriber/UnsupportedBranchSubscriberTest.php +++ b/tests/Subscriber/UnsupportedBranchSubscriberTest.php @@ -104,4 +104,53 @@ public function testOnPullRequestNotOpen() $responseData = $event->getResponseData(); $this->assertEmpty($responseData); } + + public function testOnPullRequestEditedCleanup() + { + $this->issueApi->expects($this->once()) + ->method('findBotComment') + ->with($this->repository, 1234, 'target one of these branches instead') + ->willReturn(888); + + $this->issueApi->expects($this->once()) + ->method('removeComment') + ->with($this->repository, 888); + + $event = new GitHubEvent([ + 'action' => 'edited', + 'pull_request' => [ + 'number' => 1234, + 'base' => ['ref' => '4.4'], + ], + 'repository' => [ + 'default_branch' => '5.x', + ], + ], $this->repository); + + $this->dispatcher->dispatch($event, GitHubEvents::PULL_REQUEST); + } + + public function testOnPullRequestEditedIdempotency() + { + $this->issueApi->expects($this->once()) + ->method('findBotComment') + ->with($this->repository, 1234, 'target one of these branches instead') + ->willReturn(888); + + $this->issueApi->expects($this->never()) + ->method('commentOnIssue'); + + $event = new GitHubEvent([ + 'action' => 'edited', + 'pull_request' => [ + 'number' => 1234, + 'base' => ['ref' => '4.3'], // Unsupported branch + ], + 'repository' => [ + 'default_branch' => '5.x', + ], + ], $this->repository); + + $this->dispatcher->dispatch($event, GitHubEvents::PULL_REQUEST); + } }