Vier fehlschlagende Tests behoben
1. AgentRepository::updateActivity() schrieb current_ticket_id ungeprueft. Eine
unbekannte Ticket-ID lief in die FOREIGN-KEY-Constraint und kam als
ungefangene PDOException samt Stacktrace und Dateipfaden beim Client an.
Jetzt wird die Existenz vorab geprueft (422). Zusaetzlich faengt
Application::handle() unerwartete Throwables ab und antwortet mit einem
generischen 500 - Details gehen nur noch ins Log.
2. Die Normalisierung der Codeberg-Label-Farbe ("#" davor) lag im
CurlCodebergClient statt im CodebergSyncService. Damit haetten andere
Client-Implementierungen unterschiedliche Farbformate ins Ticket geschrieben.
Verschoben in die Domaenenschicht, leere Farben ergeben jetzt null.
3. Config::get() griff selbst auf getenv() zurueck, wodurch ein in der Umgebung
gesetzter CODEBERG_TOKEN in eine explizit gebaute Config durchschlug - im
Testlauf landete der Token im Klartext in der PHPUnit-Ausgabe. Die Umgebung
wird jetzt ausschliesslich in fromEnvironment() eingesammelt.
Regressionstests fuer alle drei Punkte ergaenzt: 104 Tests, 181 Assertions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
93a1586289
commit
09a3b928ab
11 changed files with 131 additions and 11 deletions
|
|
@ -24,6 +24,7 @@ use Monitor\Api\Repository\KpiRepository;
|
||||||
use Monitor\Api\Repository\LegacyStatusFileReader;
|
use Monitor\Api\Repository\LegacyStatusFileReader;
|
||||||
use Monitor\Api\Repository\SystemRepository;
|
use Monitor\Api\Repository\SystemRepository;
|
||||||
use Monitor\Api\Repository\TicketRepository;
|
use Monitor\Api\Repository\TicketRepository;
|
||||||
|
use Throwable;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Verdrahtet Router, Controller und Auth fuer alle /api/v1/*-Endpunkte aus
|
* Verdrahtet Router, Controller und Auth fuer alle /api/v1/*-Endpunkte aus
|
||||||
|
|
@ -108,6 +109,12 @@ final class Application
|
||||||
return Response::error($e->getMessage(), 404);
|
return Response::error($e->getMessage(), 404);
|
||||||
} catch (InvalidArgumentException $e) {
|
} catch (InvalidArgumentException $e) {
|
||||||
return Response::error($e->getMessage(), 422);
|
return Response::error($e->getMessage(), 422);
|
||||||
|
} catch (Throwable $e) {
|
||||||
|
// Unerwartete Fehler (z.B. PDOException) duerfen nie als PHP-Fatal-Error mit
|
||||||
|
// Stacktrace und Dateipfaden im Response-Body landen - Details nur ins Log.
|
||||||
|
error_log(sprintf('[monitor-api] %s: %s in %s:%d', $e::class, $e->getMessage(), $e->getFile(), $e->getLine()));
|
||||||
|
|
||||||
|
return Response::error('Interner Serverfehler.', 500);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,11 @@ namespace Monitor\Api;
|
||||||
/**
|
/**
|
||||||
* Liest die Konfiguration ausschliesslich aus Umgebungsvariablen (12-factor-Stil),
|
* Liest die Konfiguration ausschliesslich aus Umgebungsvariablen (12-factor-Stil),
|
||||||
* damit die Docker-Compose-Umgebung ohne zusaetzliche Config-Dateien auskommt.
|
* damit die Docker-Compose-Umgebung ohne zusaetzliche Config-Dateien auskommt.
|
||||||
|
*
|
||||||
|
* Die Umgebung wird ausschliesslich in fromEnvironment() eingesammelt; get() greift nie
|
||||||
|
* selbst auf getenv() zu. Damit ist eine mit explizitem Array gebaute Config in Tests
|
||||||
|
* vollstaendig gekapselt und uebernimmt keine Werte (z.B. CODEBERG_TOKEN) aus der
|
||||||
|
* Umgebung des laufenden Prozesses.
|
||||||
*/
|
*/
|
||||||
final class Config
|
final class Config
|
||||||
{
|
{
|
||||||
|
|
@ -21,7 +26,7 @@ final class Config
|
||||||
|
|
||||||
private function get(string $key, ?string $default = null): ?string
|
private function get(string $key, ?string $default = null): ?string
|
||||||
{
|
{
|
||||||
$value = $this->env[$key] ?? getenv($key);
|
$value = $this->env[$key] ?? null;
|
||||||
if ($value === false || $value === null || $value === '') {
|
if ($value === false || $value === null || $value === '') {
|
||||||
return $default;
|
return $default;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -100,9 +100,19 @@ final class AgentRepository
|
||||||
return array_map(fn (array $row) => $this->hidingKey($row), $stmt->fetchAll());
|
return array_map(fn (array $row) => $this->hidingKey($row), $stmt->fetchAll());
|
||||||
}
|
}
|
||||||
|
|
||||||
/** @return array<string, mixed> */
|
/**
|
||||||
|
* Ein unbekanntes Ticket wuerde sonst erst in der FOREIGN-KEY-Constraint auflaufen und
|
||||||
|
* als ungefangene PDOException beim Client landen - deshalb hier pruefen und als
|
||||||
|
* regulaeren Validierungsfehler (422) melden.
|
||||||
|
*
|
||||||
|
* @return array<string, mixed>
|
||||||
|
*/
|
||||||
public function updateActivity(int $id, string $state, ?int $currentTicketId): array
|
public function updateActivity(int $id, string $state, ?int $currentTicketId): array
|
||||||
{
|
{
|
||||||
|
if ($currentTicketId !== null && !$this->ticketExists($currentTicketId)) {
|
||||||
|
throw new InvalidArgumentException('Unbekanntes Ticket: ' . $currentTicketId);
|
||||||
|
}
|
||||||
|
|
||||||
$stmt = $this->db->pdo()->prepare(
|
$stmt = $this->db->pdo()->prepare(
|
||||||
'UPDATE agents SET state = :state, current_ticket_id = :ticket_id, last_seen_at = :now WHERE id = :id'
|
'UPDATE agents SET state = :state, current_ticket_id = :ticket_id, last_seen_at = :now WHERE id = :id'
|
||||||
);
|
);
|
||||||
|
|
@ -121,6 +131,14 @@ final class AgentRepository
|
||||||
return $agent;
|
return $agent;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private function ticketExists(int $ticketId): bool
|
||||||
|
{
|
||||||
|
$stmt = $this->db->pdo()->prepare('SELECT 1 FROM tickets WHERE id = :id');
|
||||||
|
$stmt->execute(['id' => $ticketId]);
|
||||||
|
|
||||||
|
return $stmt->fetchColumn() !== false;
|
||||||
|
}
|
||||||
|
|
||||||
/** @param array<string, mixed> $row @return array<string, mixed> */
|
/** @param array<string, mixed> $row @return array<string, mixed> */
|
||||||
private function hidingKey(array $row): array
|
private function hidingKey(array $row): array
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -47,7 +47,19 @@ final class CodebergSyncService
|
||||||
'description' => $issue['body'],
|
'description' => $issue['body'],
|
||||||
'column' => $column,
|
'column' => $column,
|
||||||
'type' => $firstLabel['name'] ?? null,
|
'type' => $firstLabel['name'] ?? null,
|
||||||
'color' => $firstLabel['color'] ?? null,
|
'color' => self::normalizeColor($firstLabel['color'] ?? null),
|
||||||
];
|
];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Codeberg liefert Label-Farben mal mit, mal ohne fuehrendes "#". Die Normalisierung
|
||||||
|
* gehoert hierher und nicht in den Client, damit jede CodebergClient-Implementierung
|
||||||
|
* dieselbe CSS-taugliche Farbe im Ticket ablegt.
|
||||||
|
*/
|
||||||
|
private static function normalizeColor(?string $color): ?string
|
||||||
|
{
|
||||||
|
$color = trim((string) $color);
|
||||||
|
|
||||||
|
return $color === '' ? null : '#' . ltrim($color, '#');
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -45,7 +45,7 @@ final class CurlCodebergClient implements CodebergClient
|
||||||
'labels' => array_map(
|
'labels' => array_map(
|
||||||
static fn (array $label): array => [
|
static fn (array $label): array => [
|
||||||
'name' => (string) $label['name'],
|
'name' => (string) $label['name'],
|
||||||
'color' => '#' . ltrim((string) $label['color'], '#'),
|
'color' => (string) $label['color'],
|
||||||
],
|
],
|
||||||
$issue['labels'] ?? []
|
$issue['labels'] ?? []
|
||||||
),
|
),
|
||||||
|
|
|
||||||
|
|
@ -73,6 +73,32 @@ final class ApplicationTest extends TestCase
|
||||||
self::assertCount(1, $listResponse->body()['tickets']);
|
self::assertCount(1, $listResponse->body()['tickets']);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Regression: Ein unbekanntes Ticket lief bis in die FOREIGN-KEY-Constraint und kam als
|
||||||
|
* ungefangene PDOException samt Stacktrace beim Client an.
|
||||||
|
*/
|
||||||
|
public function testActivityWithUnknownTicketReturns422WithoutInternals(): void
|
||||||
|
{
|
||||||
|
$registerResponse = $this->app->handle(Request::create(
|
||||||
|
'POST',
|
||||||
|
'/api/v1/agents',
|
||||||
|
body: ['display_name' => 'Alice', 'role' => 'entwickler'],
|
||||||
|
headers: ['X-Admin-Key' => 'admin-secret']
|
||||||
|
));
|
||||||
|
$apiKey = $registerResponse->body()['api_key'];
|
||||||
|
$agentId = $registerResponse->body()['agent']['id'];
|
||||||
|
|
||||||
|
$response = $this->app->handle(Request::create(
|
||||||
|
'POST',
|
||||||
|
'/api/v1/agents/' . $agentId . '/activity',
|
||||||
|
body: ['state' => 'working', 'current_ticket' => 999],
|
||||||
|
headers: ['X-Api-Key' => $apiKey]
|
||||||
|
));
|
||||||
|
|
||||||
|
self::assertSame(422, $response->status);
|
||||||
|
self::assertStringNotContainsString('PDOException', json_encode($response->body()));
|
||||||
|
}
|
||||||
|
|
||||||
public function testPatchUnknownTicketReturns404(): void
|
public function testPatchUnknownTicketReturns404(): void
|
||||||
{
|
{
|
||||||
$registerResponse = $this->app->handle(Request::create(
|
$registerResponse = $this->app->handle(Request::create(
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,22 @@ final class ConfigTest extends TestCase
|
||||||
self::assertSame(8, $config->deskCount());
|
self::assertSame(8, $config->deskCount());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Regression: get() griff frueher selbst auf getenv() zurueck, wodurch ein in der
|
||||||
|
* Umgebung gesetzter CODEBERG_TOKEN in eine explizit gebaute Config (und damit in
|
||||||
|
* Testausgaben) durchschlug.
|
||||||
|
*/
|
||||||
|
public function testExplicitEnvArrayDoesNotFallBackToProcessEnvironment(): void
|
||||||
|
{
|
||||||
|
putenv('CODEBERG_TOKEN=darf-nicht-durchsickern');
|
||||||
|
|
||||||
|
try {
|
||||||
|
self::assertNull((new Config([]))->codebergToken());
|
||||||
|
} finally {
|
||||||
|
putenv('CODEBERG_TOKEN');
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
public function testValuesCanBeOverridden(): void
|
public function testValuesCanBeOverridden(): void
|
||||||
{
|
{
|
||||||
$config = new Config([
|
$config = new Config([
|
||||||
|
|
|
||||||
|
|
@ -8,16 +8,20 @@ use Monitor\Api\Controller\AgentController;
|
||||||
use Monitor\Api\Db\Database;
|
use Monitor\Api\Db\Database;
|
||||||
use Monitor\Api\Http\Request;
|
use Monitor\Api\Http\Request;
|
||||||
use Monitor\Api\Repository\AgentRepository;
|
use Monitor\Api\Repository\AgentRepository;
|
||||||
|
use Monitor\Api\Repository\TicketRepository;
|
||||||
use PHPUnit\Framework\TestCase;
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
final class AgentControllerTest extends TestCase
|
final class AgentControllerTest extends TestCase
|
||||||
{
|
{
|
||||||
private AgentController $controller;
|
private AgentController $controller;
|
||||||
private AgentRepository $repository;
|
private AgentRepository $repository;
|
||||||
|
private TicketRepository $tickets;
|
||||||
|
|
||||||
protected function setUp(): void
|
protected function setUp(): void
|
||||||
{
|
{
|
||||||
$this->repository = new AgentRepository(Database::inMemory());
|
$db = Database::inMemory();
|
||||||
|
$this->repository = new AgentRepository($db);
|
||||||
|
$this->tickets = new TicketRepository($db);
|
||||||
$this->controller = new AgentController($this->repository);
|
$this->controller = new AgentController($this->repository);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -78,16 +82,17 @@ final class AgentControllerTest extends TestCase
|
||||||
public function testActivityUpdatesOwnState(): void
|
public function testActivityUpdatesOwnState(): void
|
||||||
{
|
{
|
||||||
$registration = $this->repository->register('Alice', 'entwickler');
|
$registration = $this->repository->register('Alice', 'entwickler');
|
||||||
|
$ticketId = (int) $this->tickets->create(['title' => 'Testticket'])['id'];
|
||||||
|
|
||||||
$response = $this->controller->activity(
|
$response = $this->controller->activity(
|
||||||
Request::create('POST', '/x', body: ['state' => 'working', 'current_ticket' => 7]),
|
Request::create('POST', '/x', body: ['state' => 'working', 'current_ticket' => $ticketId]),
|
||||||
['id' => (string) $registration['agent']['id']],
|
['id' => (string) $registration['agent']['id']],
|
||||||
$registration['agent']
|
$registration['agent']
|
||||||
);
|
);
|
||||||
|
|
||||||
self::assertSame(200, $response->status);
|
self::assertSame(200, $response->status);
|
||||||
self::assertSame('working', $response->body()['agent']['state']);
|
self::assertSame('working', $response->body()['agent']['state']);
|
||||||
self::assertSame(7, $response->body()['agent']['current_ticket_id']);
|
self::assertSame($ticketId, $response->body()['agent']['current_ticket_id']);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testIndexListsAgents(): void
|
public function testIndexListsAgents(): void
|
||||||
|
|
|
||||||
|
|
@ -8,15 +8,19 @@ use InvalidArgumentException;
|
||||||
use Monitor\Api\Db\Database;
|
use Monitor\Api\Db\Database;
|
||||||
use Monitor\Api\Http\NotFoundException;
|
use Monitor\Api\Http\NotFoundException;
|
||||||
use Monitor\Api\Repository\AgentRepository;
|
use Monitor\Api\Repository\AgentRepository;
|
||||||
|
use Monitor\Api\Repository\TicketRepository;
|
||||||
use PHPUnit\Framework\TestCase;
|
use PHPUnit\Framework\TestCase;
|
||||||
|
|
||||||
final class AgentRepositoryTest extends TestCase
|
final class AgentRepositoryTest extends TestCase
|
||||||
{
|
{
|
||||||
private AgentRepository $repository;
|
private AgentRepository $repository;
|
||||||
|
private TicketRepository $tickets;
|
||||||
|
|
||||||
protected function setUp(): void
|
protected function setUp(): void
|
||||||
{
|
{
|
||||||
$this->repository = new AgentRepository(Database::inMemory(), deskCount: 2);
|
$db = Database::inMemory();
|
||||||
|
$this->repository = new AgentRepository($db, deskCount: 2);
|
||||||
|
$this->tickets = new TicketRepository($db);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testRegisterAssignsLowestFreeDesk(): void
|
public function testRegisterAssignsLowestFreeDesk(): void
|
||||||
|
|
@ -72,11 +76,21 @@ final class AgentRepositoryTest extends TestCase
|
||||||
{
|
{
|
||||||
$result = $this->repository->register('Alice', 'entwickler');
|
$result = $this->repository->register('Alice', 'entwickler');
|
||||||
$id = (int) $result['agent']['id'];
|
$id = (int) $result['agent']['id'];
|
||||||
|
$ticketId = (int) $this->tickets->create(['title' => 'Testticket'])['id'];
|
||||||
|
|
||||||
$updated = $this->repository->updateActivity($id, 'working', 42);
|
$updated = $this->repository->updateActivity($id, 'working', $ticketId);
|
||||||
|
|
||||||
self::assertSame('working', $updated['state']);
|
self::assertSame('working', $updated['state']);
|
||||||
self::assertSame(42, $updated['current_ticket_id']);
|
self::assertSame($ticketId, $updated['current_ticket_id']);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function testUpdateActivityRejectsUnknownTicket(): void
|
||||||
|
{
|
||||||
|
$result = $this->repository->register('Alice', 'entwickler');
|
||||||
|
|
||||||
|
$this->expectException(InvalidArgumentException::class);
|
||||||
|
|
||||||
|
$this->repository->updateActivity((int) $result['agent']['id'], 'working', 999);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testUpdateActivityThrowsForUnknownAgent(): void
|
public function testUpdateActivityThrowsForUnknownAgent(): void
|
||||||
|
|
|
||||||
|
|
@ -51,6 +51,22 @@ final class CodebergSyncServiceTest extends TestCase
|
||||||
self::assertCount(1, $done);
|
self::assertCount(1, $done);
|
||||||
self::assertSame('Geschlossenes Issue', $done[0]['title']);
|
self::assertSame('Geschlossenes Issue', $done[0]['title']);
|
||||||
self::assertNull($done[0]['type']);
|
self::assertNull($done[0]['type']);
|
||||||
|
self::assertNull($done[0]['color']);
|
||||||
|
}
|
||||||
|
|
||||||
|
public function testSyncNormalizesLabelColorRegardlessOfLeadingHash(): void
|
||||||
|
{
|
||||||
|
$tickets = new TicketRepository(Database::inMemory());
|
||||||
|
$client = new FakeCodebergClient([
|
||||||
|
'open' => [
|
||||||
|
['number' => 1, 'title' => 'Mit Raute', 'body' => null, 'labels' => [['name' => 'bug', 'color' => '#00ff00']]],
|
||||||
|
],
|
||||||
|
'closed' => [],
|
||||||
|
]);
|
||||||
|
|
||||||
|
(new CodebergSyncService($client, $tickets, 'fds2610', 'monitor'))->sync();
|
||||||
|
|
||||||
|
self::assertSame('#00ff00', $tickets->all('backlog')[0]['color']);
|
||||||
}
|
}
|
||||||
|
|
||||||
public function testSyncDoesNotResetManuallyMovedTicket(): void
|
public function testSyncDoesNotResetManuallyMovedTicket(): void
|
||||||
|
|
|
||||||
|
|
@ -82,7 +82,8 @@ namespace Monitor\Api\Tests\Sync {
|
||||||
|
|
||||||
self::assertCount(1, $issues);
|
self::assertCount(1, $issues);
|
||||||
self::assertSame(1, $issues[0]['number']);
|
self::assertSame(1, $issues[0]['number']);
|
||||||
self::assertSame('#ff0000', $issues[0]['labels'][0]['color']);
|
// Der Client reicht die Label-Farbe roh durch; das "#" setzt CodebergSyncService.
|
||||||
|
self::assertSame('ff0000', $issues[0]['labels'][0]['color']);
|
||||||
self::assertStringContainsString('state=open', CurlStub::$requestedUrls[0]);
|
self::assertStringContainsString('state=open', CurlStub::$requestedUrls[0]);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue