Reconnecting an integration edits it, and a taken name is a form error (#217)

The setup wizard built a new connection every time its Integrations step was saved, so saving it again collided with the unique key on (user, provider, name) and answered with a 500. It now edits the oldest existing connection for that service. The settings page had the same 500 for a second connection that kept the default name; a UniqueEntity constraint now reports it on the name field. Closes #216.

Co-authored-by: Philippe Delodder <philippe@delodder.be>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

CHANGELOG.md

@@ -8,6 +8,14 @@ The published image tags: `latest` follows the most recent release below,
 
 ## Unreleased
 
+**Reconnecting a service in the setup wizard no longer fails.** Opening the Integrations step for
+a service that was already connected and saving it again answered with an error page, because the
+step built a second connection with the same name instead of editing the first. It now edits the
+existing one, and a blank secret still keeps the stored one. The settings page had the same error
+when a second connection to a service (a work Nextcloud next to the home one) kept the default
+name; it now asks for another name instead. No schema change.
+[#216](https://github.com/karatektus/pl_mail/issues/216), by @phdelodder.
+
 **Inline images in a message show up again.** Pictures that a mail carries inside itself, such as
 a logo or the icons in a booking confirmation, rendered as empty boxes: the message body is shown in
 a sandbox that sends no session cookie, so each image was answered with a redirect to the login

src/Entity/Integration/Integration.php

@@ -15,6 +15,7 @@ use App\Infrastructure\Doctrine\Type\EncryptedStringType;
 use App\Repository\Integration\IntegrationRepository;
 use DateTimeImmutable;
 use Doctrine\ORM\Mapping as ORM;
+use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;
 
 /**
  * One user's connection to one external file or photo service.
@@ -37,6 +38,11 @@ use Doctrine\ORM\Mapping as ORM;
 #[ORM\Table(name: 'integration')]
 #[ORM\UniqueConstraint(name: 'uniq_integration_user_provider_name', columns: ['usr_id', 'provider', 'name'])]
 #[ORM\Index(name: 'idx_integration_user', columns: ['usr_id'])]
+// The unique key above, checked before the INSERT so a form can say so. A
+// second Nextcloud that kept the default name used to reach the database and
+// come back as a 500; now it is an error on the name field, which is the one
+// the user can change (#216).
+#[UniqueEntity(fields: ['usr', 'provider', 'name'], message: 'integration.name_taken', errorPath: 'name')]
 #[ORM\HasLifecycleCallbacks]
 class Integration
 {

src/Service/Onboarding/Step/IntegrationsStepHandler.php

@@ -105,9 +105,16 @@ final readonly class IntegrationsStepHandler implements OnboardingStepHandlerInt
 
         $config = $this->connector->requireConnectable($provider);
 
+        // Reconnecting edits the existing connection: (user, provider, name) is
+        // unique, so a fresh entity would collide on save. Someone with two
+        // (home and work, added in settings) gets the oldest, the one the
+        // wizard would have made, and the same one every time.
+        $integration = $this->integrations->findOneBy(['usr' => $user, 'provider' => $provider], ['id' => 'ASC'])
+            ?? new Integration($user, $provider, $provider->label());
+
         return $this->formFactory->create(
             IntegrationConnectType::class,
-            new Integration($user, $provider, $provider->label()),
+            $integration,
             [
                 'integration_provider' => $provider,
                 'url_editable'         => $this->connector->isUrlEditable($provider, $config),

tests/Controller/Integration/IntegrationNameTakenTest.php

@@ -0,0 +1,174 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Controller\Integration;
+
+use App\Domain\Enum\Integration\Provider;
+use App\Entity\Integration\Integration;
+use App\Entity\Integration\IntegrationProviderConfig;
+use App\Entity\User\User;
+use App\Repository\User\UserRepository;
+use Doctrine\ORM\EntityManagerInterface;
+use Symfony\Bundle\FrameworkBundle\KernelBrowser;
+use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
+use Symfony\Component\DomCrawler\Crawler;
+
+/**
+ * A connection with a name the user already has for that service (#216).
+ *
+ * `(usr, provider, name)` is unique, and both ways of connecting start from the
+ * provider's own name ("Nextcloud"). Saving one twice went straight to the
+ * INSERT and came back as a 500:
+ *
+ *   - the setup wizard built a new connection every time its step was saved,
+ *     so going back to it and saving again collided with the first;
+ *   - the settings page allows a second connection to the same service (home
+ *     and work), but one that kept the default name collided the same way.
+ *
+ * The wizard now edits the existing connection; the settings page says the
+ * name is taken, on the name field.
+ *
+ * The server address is a closed port on purpose: every save probes the
+ * service, and a refused connection is recorded on the entity, not thrown.
+ */
+final class IntegrationNameTakenTest extends WebTestCase
+{
+    private const string ADMIN_EMAIL = 'e2e-admin@plmail.test';
+    private const string NOWHERE     = 'http://127.0.0.1:9';
+
+    private KernelBrowser $client;
+    private EntityManagerInterface $em;
+    private User $user;
+
+    protected function setUp(): void
+    {
+        $this->client = static::createClient();
+        $this->client->disableReboot();
+
+        $this->em = static::getContainer()->get(EntityManagerInterface::class);
+        $user     = static::getContainer()->get(UserRepository::class)->findOneBy(['email' => self::ADMIN_EMAIL]);
+
+        if (null === $user) {
+            self::markTestSkipped('run `app:test:seed-user --admin` first');
+        }
+
+        $this->user = $user;
+        $this->em->getConnection()->beginTransaction();
+
+        // Whatever the e2e suite left behind: one connectable Nextcloud, and
+        // no connections of this user's.
+        $this->em->getConnection()->executeStatement("DELETE FROM integration_provider_config WHERE provider = 'nextcloud'");
+        $this->em->getConnection()->executeStatement('DELETE FROM integration WHERE usr_id = :u', ['u' => $user->id]);
+
+        $config            = new IntegrationProviderConfig(Provider::Nextcloud);
+        $config->isEnabled = true;
+        $this->em->persist($config);
+        $this->em->flush();
+
+        $this->client->loginUser($user);
+    }
+
+    protected function tearDown(): void
+    {
+        if (true === $this->em->getConnection()->isTransactionActive()) {
+            $this->em->getConnection()->rollBack();
+        }
+
+        parent::tearDown();
+    }
+
+    public function testASecondConnectionWithTheSameNameIsAFieldErrorNotA500(): void
+    {
+        $this->connection('Nextcloud', 'home');
+
+        $this->submit('/settings/integrations/connect/nextcloud', ['username' => 'work', 'secret' => 'pw']);
+
+        self::assertResponseStatusCodeSame(422);
+        self::assertStringContainsString(
+            'You already have a connection to this service called "Nextcloud"',
+            $this->client->getCrawler()->filter('form[name="integration_connect"]')->text(),
+        );
+        self::assertSame(['Nextcloud' => 'home'], $this->stored());
+    }
+
+    /** Two of the same service is allowed; only the name has to differ. */
+    public function testASecondConnectionWithItsOwnNameIsSaved(): void
+    {
+        $this->connection('Nextcloud', 'home');
+
+        $this->submit('/settings/integrations/connect/nextcloud', ['name' => 'Work', 'username' => 'work', 'secret' => 'pw']);
+
+        self::assertResponseIsSuccessful();
+        self::assertSame(['Nextcloud' => 'home', 'Work' => 'work'], $this->stored());
+    }
+
+    public function testSavingTheWizardStepAgainEditsTheConnection(): void
+    {
+        $this->connection('Nextcloud', 'old');
+
+        $crawler = $this->client->request('GET', '/onboarding/integrations?connect=nextcloud');
+        self::assertSame('old', $this->field($crawler, 'username'), 'the step opens on what is connected');
+
+        $this->submit('/onboarding/integrations?connect=nextcloud', ['username' => 'new']);
+
+        self::assertResponseRedirects();
+        self::assertSame(['Nextcloud' => 'new'], $this->stored());
+    }
+
+    /** Home and work, both added in settings: the wizard edits the older one, every time. */
+    public function testTheWizardEditsTheOldestOfTwo(): void
+    {
+        $this->connection('Nextcloud', 'home');
+        $this->connection('Work', 'work');
+
+        $crawler = $this->client->request('GET', '/onboarding/integrations?connect=nextcloud');
+
+        self::assertSame('Nextcloud', $this->field($crawler, 'name'));
+        self::assertSame('home', $this->field($crawler, 'username'));
+    }
+
+    private function connection(string $name, string $username): void
+    {
+        $integration           = new Integration($this->user, Provider::Nextcloud, $name);
+        $integration->baseUrl  = self::NOWHERE;
+        $integration->username = $username;
+        $integration->secret   = 'stored-secret';
+
+        $this->em->persist($integration);
+        $this->em->flush();
+    }
+
+    /**
+     * Opens the form at $url, fills in the server address and $fields, submits.
+     *
+     * @param array<string, string> $fields field name inside `integration_connect` => value
+     */
+    private function submit(string $url, array $fields): void
+    {
+        $form = $this->client->request('GET', $url)->filter('form[name="integration_connect"]')->form();
+
+        $form['integration_connect[baseUrl]'] = self::NOWHERE;
+
+        foreach ($fields as $field => $value) {
+            $form['integration_connect[' . $field . ']'] = $value;
+        }
+
+        $this->client->submit($form);
+    }
+
+    private function field(Crawler $crawler, string $field): string
+    {
+        return (string) $crawler->filter('[name="integration_connect[' . $field . ']"]')->attr('value');
+    }
+
+    /** @return array<string, string> name => username, for this user's Nextclouds */
+    private function stored(): array
+    {
+        /** @var array<string, string> */
+        return $this->em->getConnection()->fetchAllKeyValue(
+            "SELECT name, username FROM integration WHERE usr_id = :u AND provider = 'nextcloud' ORDER BY name",
+            ['u' => $this->user->id],
+        );
+    }
+}

translations/validators.de.yaml

@@ -20,6 +20,7 @@ caldav.username_required: Gib den Benutzernamen ein, unter dem dich dein CalDAV-
 calendar.name_required: Gib dem Kalender einen Namen — daran erkennst du ihn zwischen den anderen.
 compose.recipient_invalid: "„{{ address }}“ ist keine gültige E-Mail-Adresse."
 compose.recipient_required: Füge mindestens einen Empfänger hinzu, bevor du sendest.
+integration.name_taken: Du hast schon eine Verbindung zu diesem Dienst mit dem Namen {{ value }}. Gib dieser einen anderen Namen — etwa Privat und Arbeit.
 settings.password.current_required: Gib dein aktuelles Passwort ein — es verhindert, dass jemand an deinem offenen Bildschirm es einfach ändert.
 settings.password.current_wrong: Das ist nicht dein aktuelles Passwort.
 settings.password.mismatch: Die beiden neuen Passwörter stimmen nicht überein.

translations/validators.en.yaml

@@ -21,6 +21,7 @@ caldav.username_required: Enter the username your CalDAV server knows you by.
 calendar.name_required: Give the calendar a name — it is what tells it apart from the others.
 compose.recipient_invalid: "“{{ address }}” is not a valid email address."
 compose.recipient_required: Add at least one recipient before sending.
+integration.name_taken: You already have a connection to this service called {{ value }}. Give this one another name — home and work, say.
 settings.password.current_required: Enter your current password — it is what stops somebody at your unlocked screen changing it.
 settings.password.current_wrong: That is not your current password.
 settings.password.mismatch: The two new passwords don't match.

translations/validators.en_PI.yaml

@@ -19,6 +19,7 @@ caldav.username_required: Give the name yer CalDAV port knows ye by.
 calendar.name_required: Name the almanac, else ye'll not tell it from the rest.
 compose.recipient_invalid: "“{{ address }}” be no proper address, matey."
 compose.recipient_required: Name at least one soul afore ye cast off.
+integration.name_taken: Ye already have a line to this port named {{ value }}. Christen this one somethin' else — home an' work, say.
 settings.password.current_required: Say yer current word — 'tis what keeps a passing hand from changing it at yer open desk.
 settings.password.current_wrong: That be not yer current word.
 settings.password.mismatch: Th' two new words don't match.

translations/validators.fr.yaml

@@ -20,6 +20,7 @@ caldav.username_required: Saisissez le nom d’utilisateur sous lequel votre ser
 calendar.name_required: Donnez un nom à l’agenda — c’est ce qui le distingue des autres.
 compose.recipient_invalid: "« {{ address }} » n’est pas une adresse e-mail valide."
 compose.recipient_required: Ajoutez au moins un destinataire avant d’envoyer.
+integration.name_taken: Vous avez déjà une connexion à ce service nommée {{ value }}. Donnez un autre nom à celle-ci — maison et travail, par exemple.
 settings.password.current_required: Saisissez votre mot de passe actuel — c’est ce qui empêche quelqu’un devant votre écran déverrouillé de le changer.
 settings.password.current_wrong: Ce n’est pas votre mot de passe actuel.
 settings.password.mismatch: Les deux nouveaux mots de passe ne correspondent pas.