Handle webhook add and remove errors for better UX (#396)

repman-io · Jan 29, 2021 · 14460be · 14460be
1 parent 32e4fec
commit 14460be
Show file tree

Hide file tree

Showing 20 changed files with 180 additions and 101 deletions.
diff --git a/config/packages/prod/sentry.yaml b/config/packages/prod/sentry.yaml
@@ -4,5 +4,6 @@ sentry:
         release: '%env(string:default:kernel_version:APP_VERSION)%'
         excluded_exceptions:
             - 'Symfony\Component\Security\Core\Exception\AccessDeniedException'
+            - 'Symfony\Component\Security\Core\Exception\AuthenticationCredentialsNotFoundException'
             - 'Symfony\Component\HttpKernel\Exception\NotFoundHttpException'
             - 'Symfony\Component\HttpKernel\Exception\MethodNotAllowedHttpException'
diff --git a/src/Controller/OrganizationController.php b/src/Controller/OrganizationController.php
@@ -33,7 +33,6 @@
 use Buddy\Repman\Query\User\PackageQuery;
 use Buddy\Repman\Query\User\PackageQuery\Filter as PackageFilter;
 use Buddy\Repman\Security\Model\User;
-use Buddy\Repman\Service\ExceptionHandler;
 use Sensio\Bundle\FrameworkExtraBundle\Configuration\IsGranted;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\HttpFoundation\JsonResponse;
@@ -46,13 +45,11 @@ final class OrganizationController extends AbstractController
 {
     private PackageQuery $packageQuery;
     private OrganizationQuery $organizationQuery;
-    private ExceptionHandler $exceptionHandler;
 
-    public function __construct(PackageQuery $packageQuery, OrganizationQuery $organizationQuery, ExceptionHandler $exceptionHandler)
+    public function __construct(PackageQuery $packageQuery, OrganizationQuery $organizationQuery)
     {
         $this->packageQuery = $packageQuery;
         $this->organizationQuery = $organizationQuery;
-        $this->exceptionHandler = $exceptionHandler;
     }
 
     /**
@@ -362,7 +359,11 @@ private function tryToRemoveWebhook(Package $package): void
                         break;
                 }
             } catch (HandlerFailedException $exception) {
-                $this->exceptionHandler->handle($exception);
+                $reason = current($exception->getNestedExceptions());
+                $this->addFlash('danger', sprintf(
+                    'Webhook removal failed due to "%s". Please remove it manually.',
+                    $reason !== false ? $reason->getMessage() : $exception->getMessage()
+                ));
             }
         }
     }

diff --git a/src/Entity/Organization/Package.php b/src/Entity/Organization/Package.php
@@ -75,6 +75,11 @@ class Package
      */
     private ?\DateTimeImmutable $webhookCreatedAt = null;
 
+    /**
+     * @ORM\Column(type="string", nullable=true)
+     */
+    private ?string $webhookCreatedError = null;
+
     /**
      * @ORM\Column(type="text", nullable=true)
      */
@@ -223,6 +228,12 @@ public function hasOAuthToken(): bool
     public function webhookWasCreated(): void
     {
         $this->webhookCreatedAt = new \DateTimeImmutable();
+        $this->webhookCreatedError = null;
+    }
+
+    public function webhookWasNotCreated(string $error): void
+    {
+        $this->webhookCreatedError = substr($error, 0, 255);
     }
 
     public function webhookWasRemoved(): void

diff --git a/src/MessageHandler/Organization/Package/AddBitbucketHookHandler.php b/src/MessageHandler/Organization/Package/AddBitbucketHookHandler.php
@@ -19,11 +19,15 @@ public function __invoke(AddBitbucketHook $message): void
             return;
         }
 
-        $this->integrations->bitbucketApi()->addHook(
-            $package->oauthToken()->accessToken($this->oauth),
-            $package->metadata(Metadata::BITBUCKET_REPO_NAME),
-            $this->router->generate('package_webhook', ['package' => $package->id()->toString()], UrlGeneratorInterface::ABSOLUTE_URL)
-        );
-        $package->webhookWasCreated();
+        try {
+            $this->integrations->bitbucketApi()->addHook(
+                $package->oauthToken()->accessToken($this->oauth),
+                $package->metadata(Metadata::BITBUCKET_REPO_NAME),
+                $this->router->generate('package_webhook', ['package' => $package->id()->toString()], UrlGeneratorInterface::ABSOLUTE_URL)
+            );
+            $package->webhookWasCreated();
+        } catch (\Throwable $exception) {
+            $package->webhookWasNotCreated($exception->getMessage());
+        }
     }
 }
diff --git a/src/MessageHandler/Organization/Package/AddGitHubHookHandler.php b/src/MessageHandler/Organization/Package/AddGitHubHookHandler.php
@@ -19,12 +19,16 @@ public function __invoke(AddGitHubHook $message): void
             return;
         }
 
-        $this->integrations->gitHubApi()->addHook(
-            $package->oauthToken()->accessToken($this->oauth),
-            $package->metadata(Metadata::GITHUB_REPO_NAME),
-            $this->router->generate('package_webhook', ['package' => $package->id()->toString()], UrlGeneratorInterface::ABSOLUTE_URL)
-        );
+        try {
+            $this->integrations->gitHubApi()->addHook(
+                $package->oauthToken()->accessToken($this->oauth),
+                $package->metadata(Metadata::GITHUB_REPO_NAME),
+                $this->router->generate('package_webhook', ['package' => $package->id()->toString()], UrlGeneratorInterface::ABSOLUTE_URL)
+            );
 
-        $package->webhookWasCreated();
+            $package->webhookWasCreated();
+        } catch (\Throwable $exception) {
+            $package->webhookWasNotCreated($exception->getMessage());
+        }
     }
 }
diff --git a/src/MessageHandler/Organization/Package/AddGitLabHookHandler.php b/src/MessageHandler/Organization/Package/AddGitLabHookHandler.php
@@ -19,11 +19,15 @@ public function __invoke(AddGitLabHook $message): void
             return;
         }
 
-        $this->integrations->gitLabApi()->addHook(
-            $package->oauthToken()->accessToken($this->oauth),
-            $package->metadata(Metadata::GITLAB_PROJECT_ID),
-            $this->router->generate('package_webhook', ['package' => $package->id()->toString()], UrlGeneratorInterface::ABSOLUTE_URL)
-        );
-        $package->webhookWasCreated();
+        try {
+            $this->integrations->gitLabApi()->addHook(
+                $package->oauthToken()->accessToken($this->oauth),
+                $package->metadata(Metadata::GITLAB_PROJECT_ID),
+                $this->router->generate('package_webhook', ['package' => $package->id()->toString()], UrlGeneratorInterface::ABSOLUTE_URL)
+            );
+            $package->webhookWasCreated();
+        } catch (\Throwable $exception) {
+            $package->webhookWasNotCreated($exception->getMessage());
+        }
     }
 }
diff --git a/src/Migrations/Version20210129111835.php b/src/Migrations/Version20210129111835.php
@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Buddy\Repman\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * Auto-generated Migration: Please modify to your needs!
+ */
+final class Version20210129111835 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return '';
+    }
+
+    public function up(Schema $schema): void
+    {
+        // this up() migration is auto-generated, please modify it to your needs
+        $this->addSql('ALTER TABLE organization_package ADD webhook_created_error VARCHAR(255) DEFAULT NULL');
+    }
+
+    public function down(Schema $schema): void
+    {
+        // this down() migration is auto-generated, please modify it to your needs
+        $this->addSql('ALTER TABLE organization_package DROP webhook_created_error');
+    }
+}
diff --git a/src/Query/User/Model/Package.php b/src/Query/User/Model/Package.php
@@ -19,6 +19,7 @@ final class Package
     private ?\DateTimeImmutable $lastSyncAt;
     private ?string $lastSyncError;
     private ?\DateTimeImmutable $webhookCreatedAt;
+    private ?string $webhookCreatedError;
     private int $keepLastReleases;
 
     public function __construct(
@@ -33,6 +34,7 @@ public function __construct(
         ?\DateTimeImmutable $lastSyncAt = null,
         ?string $lastSyncError = null,
         ?\DateTimeImmutable $webhookCreatedAt = null,
+        ?string $webhookCreatedError = null,
         ?ScanResult $scanResult = null,
         int $keepLastReleases = 0
     ) {
@@ -47,6 +49,7 @@ public function __construct(
         $this->lastSyncAt = $lastSyncAt;
         $this->lastSyncError = $lastSyncError;
         $this->webhookCreatedAt = $webhookCreatedAt;
+        $this->webhookCreatedError = $webhookCreatedError;
         $this->scanResult = $scanResult ?? null;
         $this->keepLastReleases = $keepLastReleases;
     }
@@ -106,6 +109,11 @@ public function webhookCreatedAt(): ?\DateTimeImmutable
         return $this->webhookCreatedAt;
     }
 
+    public function webhookCreatedError(): ?string
+    {
+        return $this->webhookCreatedError;
+    }
+
     public function allowToAutoAddWebhook(): bool
     {
         return in_array($this->type, ['github-oauth', 'gitlab-oauth', 'bitbucket-oauth'], true);

diff --git a/src/Query/User/PackageQuery/DbalPackageQuery.php b/src/Query/User/PackageQuery/DbalPackageQuery.php
@@ -72,9 +72,11 @@ function (array $data): Package {
                 last_sync_at,
                 last_sync_error,
                 webhook_created_at,
+                webhook_created_error,
                 last_scan_date,
                 last_scan_status,
-                last_scan_result
+                last_scan_result,
+                keep_last_releases
             FROM organization_package
             WHERE organization_id = :organization_id
             '.$filterSQL.'
@@ -141,6 +143,7 @@ public function getById(string $id): Option
                 last_sync_at,
                 last_sync_error,
                 webhook_created_at,
+                webhook_created_error,
                 last_scan_date,
                 last_scan_status,
                 last_scan_result,
@@ -370,6 +373,7 @@ private function hydratePackage(array $data): Package
             $data['last_sync_at'] !== null ? new \DateTimeImmutable($data['last_sync_at']) : null,
             $data['last_sync_error'],
             $data['webhook_created_at'] !== null ? new \DateTimeImmutable($data['webhook_created_at']) : null,
+            $data['webhook_created_error'],
             $scanResult,
             $data['keep_last_releases'] ?? 0
         );

diff --git a/src/Service/ExceptionHandler.php b/src/Service/ExceptionHandler.php
diff --git a/src/Service/Symfony/SentryExceptionHandler.php b/src/Service/Symfony/SentryExceptionHandler.php
diff --git a/templates/organization/package/webhook.html.twig b/templates/organization/package/webhook.html.twig
@@ -39,6 +39,12 @@
                     data-action="{{ path('organization_package_webhook', {organization: organization.alias, package: package.id }) }}"
                     data-method="POST"
             >Synchronize webhook</a>
+            {% if package.webhookCreatedError is not null %}
+                <div class="alert alert-danger small">
+                    The following error occurred while adding a webhook:<br />
+                    {{ package.webhookCreatedError }}
+                </div>
+            {% endif %}
         {% else %}
             unable to automatically add a webhook, please add it manually
         {% endif %}

diff --git a/tests/Doubles/FakeBitbucketApi.php b/tests/Doubles/FakeBitbucketApi.php
@@ -14,9 +14,7 @@ final class FakeBitbucketApi implements BitbucketApi
 
     public function primaryEmail(string $accessToken): string
     {
-        if ($this->exception !== null) {
-            throw $this->exception;
-        }
+        $this->throwExceptionIfSet();
 
         return $this->primaryEmail;
     }
@@ -33,9 +31,7 @@ public function setExceptionOnNextCall(?\Throwable $exception): void
 
     public function repositories(string $accessToken): Repositories
     {
-        if ($this->exception !== null) {
-            throw $this->exception;
-        }
+        $this->throwExceptionIfSet();
 
         return new Repositories([
             new BitbucketApi\Repository('{0f6dc6fe-f8ab-4a53-bb63-03042b80056f}', 'buddy-works/repman', 'https://bitbucket.org/buddy-works/repman.git'),
@@ -44,11 +40,18 @@ public function repositories(string $accessToken): Repositories
 
     public function addHook(string $accessToken, string $fullName, string $hookUrl): void
     {
-        // TODO: Implement addHook() method.
+        $this->throwExceptionIfSet();
     }
 
     public function removeHook(string $accessToken, string $fullName, string $hookUrl): void
     {
-        // TODO: Implement removeHook() method.
+        $this->throwExceptionIfSet();
+    }
+
+    private function throwExceptionIfSet(): void
+    {
+        if ($this->exception !== null) {
+            throw $this->exception;
+        }
     }
 }
diff --git a/tests/Doubles/FakeGitLabApi.php b/tests/Doubles/FakeGitLabApi.php
@@ -9,20 +9,36 @@
 
 final class FakeGitLabApi implements GitLabApi
 {
+    private ?\Throwable $exception = null;
+
+    public function setExceptionOnNextCall(?\Throwable $exception): void
+    {
+        $this->exception = $exception;
+    }
+
     public function projects(string $accessToken): Projects
     {
+        $this->throwExceptionIfSet();
+
         return new Projects([
             new GitLabApi\Project(123456, 'buddy-works/repman', 'https://gitlab.com/buddy-works/repman'),
         ]);
     }
 
     public function addHook(string $accessToken, int $projectId, string $hookUrl): void
     {
-        // TODO: Implement addHook() method.
+        $this->throwExceptionIfSet();
     }
 
     public function removeHook(string $accessToken, int $projectId, string $hookUrl): void
     {
-        // TODO: Implement removeHook() method.
+        $this->throwExceptionIfSet();
+    }
+
+    private function throwExceptionIfSet(): void
+    {
+        if ($this->exception !== null) {
+            throw $this->exception;
+        }
     }
 }
diff --git a/tests/Doubles/InMemoryExceptionHandler.php b/tests/Doubles/InMemoryExceptionHandler.php