fix: admin UI review round 1 (S47) - Art labels, usage only on success, result fallback
- Art column: only lit/media-delete and lit/divi-delete say "löscht", lit/post-trash "Papierkorb", other destructive tools "überschreibt". - ConnectionUsage records only responses with status < 400 (mcp-oauth answers auth failures with WP_REST_Response 401, not WP_Error). - Log result column: translated "unbekannt" instead of the raw value. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -55,7 +55,7 @@ final class ConnectionUsage
|
||||
public function record(mixed $response, array $handler, WP_REST_Request $request): mixed
|
||||
{
|
||||
$route = '/' . ServerRegistrar::NAMESPACE . '/' . ServerRegistrar::ROUTE;
|
||||
if ($request->get_route() !== $route || is_wp_error($response)) {
|
||||
if ($request->get_route() !== $route || ! self::succeeded($response)) {
|
||||
return $response;
|
||||
}
|
||||
|
||||
@@ -78,4 +78,17 @@ final class ConnectionUsage
|
||||
|
||||
return $response;
|
||||
}
|
||||
|
||||
/**
|
||||
* Only successful answers count as use: mcp-oauth answers auth failures with a
|
||||
* WP_REST_Response(401), not a WP_Error (OAuthHttpTransport::build_auth_error_response).
|
||||
*/
|
||||
private static function succeeded(mixed $response): bool
|
||||
{
|
||||
if (is_wp_error($response)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return ! $response instanceof \WP_HTTP_Response || $response->get_status() < 400;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -130,7 +130,7 @@ final class LogListTable extends \WP_List_Table
|
||||
[$icon, $word] = match ($result) {
|
||||
'ok' => ['yes-alt', __('erfolgreich', 'wp-lit-mcp')],
|
||||
'partial' => ['warning', __('teilweise', 'wp-lit-mcp')],
|
||||
default => ['dismiss', $result],
|
||||
default => ['dismiss', __('unbekannt', 'wp-lit-mcp')],
|
||||
};
|
||||
|
||||
return sprintf(
|
||||
|
||||
+16
-2
@@ -22,6 +22,13 @@ final class ToolsTab implements AdminTab
|
||||
/** More direct tools than this cost noticeable context in every claude.ai conversation. */
|
||||
public const DIRECT_WARNING_THRESHOLD = 25;
|
||||
|
||||
/** Tools that remove something: "delete" (for good) or "trash" (to the trash). */
|
||||
private const DELETING = [
|
||||
'lit/media-delete' => 'delete',
|
||||
'lit/divi-delete' => 'delete',
|
||||
'lit/post-trash' => 'trash',
|
||||
];
|
||||
|
||||
public function __construct(private IntegrationRegistry $registry, private ExposurePolicy $policy)
|
||||
{
|
||||
}
|
||||
@@ -336,12 +343,19 @@ final class ToolsTab implements AdminTab
|
||||
echo '</td></tr>';
|
||||
}
|
||||
|
||||
/** "liest" / "schreibt" / "löscht" from the ability's annotations, with a dashicon. */
|
||||
/**
|
||||
* "liest" / "schreibt" / "überschreibt" / "löscht" / "Papierkorb" with a dashicon. The
|
||||
* "destructive" annotation is a broad hint (deleting, replacing, swapping, ruling S47),
|
||||
* so only the tools in DELETING say "löscht"; other destructive tools "überschreiben".
|
||||
*/
|
||||
private function kind(Ability $ability): string
|
||||
{
|
||||
$annotations = $ability->annotations();
|
||||
[$icon, $text] = match (true) {
|
||||
($annotations['destructive'] ?? false) === true => ['trash', __('löscht', 'wp-lit-mcp')],
|
||||
isset(self::DELETING[$ability->name()]) => ['trash', self::DELETING[$ability->name()] === 'trash'
|
||||
? __('Papierkorb', 'wp-lit-mcp')
|
||||
: __('löscht', 'wp-lit-mcp')],
|
||||
($annotations['destructive'] ?? false) === true => ['update', __('überschreibt', 'wp-lit-mcp')],
|
||||
($annotations['readonly'] ?? false) === true => ['visibility', __('liest', 'wp-lit-mcp')],
|
||||
default => ['edit', __('schreibt', 'wp-lit-mcp')],
|
||||
};
|
||||
|
||||
@@ -8,7 +8,11 @@ use Brain\Monkey\Functions;
|
||||
use LIT\MCP\Admin\ConnectionUsage;
|
||||
use LIT\MCP\Tests\Unit\TestCase;
|
||||
use WP_Error;
|
||||
use WP_HTTP_Response;
|
||||
use WP_REST_Request;
|
||||
use WP_REST_Response;
|
||||
|
||||
require_once __DIR__ . '/Fixtures/HttpResponses.php';
|
||||
|
||||
final class ConnectionUsageTest extends TestCase
|
||||
{
|
||||
@@ -64,6 +68,37 @@ final class ConnectionUsageTest extends TestCase
|
||||
self::assertSame([], $this->recorded);
|
||||
}
|
||||
|
||||
/** @dataProvider failedStatuses */
|
||||
public function testDoesNotRecordRequestsAnsweredWithAnErrorStatus(int $status): void
|
||||
{
|
||||
Functions\when('get_current_user_id')->justReturn(7);
|
||||
$claims = ['sub' => 7, 'app_pass_id' => 'uuid-1'];
|
||||
// mcp-oauth answers auth failures with a WP_REST_Response (OAuthHttpTransport::build_auth_error_response).
|
||||
$rest = new WP_REST_Response(['error' => 'invalid_token'], $status);
|
||||
|
||||
self::assertSame($rest, $this->usage($claims)->record($rest, [], $this->request()));
|
||||
$this->usage($claims)->record(new WP_HTTP_Response(null, $status), [], $this->request());
|
||||
|
||||
self::assertSame([], $this->recorded);
|
||||
}
|
||||
|
||||
/** @return list<list<int>> */
|
||||
public static function failedStatuses(): array
|
||||
{
|
||||
return [[401], [403], [500]];
|
||||
}
|
||||
|
||||
public function testRecordsSuccessfulRestResponses(): void
|
||||
{
|
||||
Functions\when('get_current_user_id')->justReturn(7);
|
||||
$claims = ['sub' => 7, 'app_pass_id' => 'uuid-1'];
|
||||
|
||||
$this->usage($claims)->record(new WP_REST_Response([], 200), [], $this->request());
|
||||
$this->usage($claims)->record(new WP_REST_Response([], 202), [], $this->request());
|
||||
|
||||
self::assertSame([[7, 'uuid-1'], [7, 'uuid-1']], $this->recorded);
|
||||
}
|
||||
|
||||
public function testNeverBreaksTheResponseWhenDecodingOrRecordingFails(): void
|
||||
{
|
||||
Functions\when('get_current_user_id')->justReturn(7);
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
// Minimal stand-ins for WordPress' response classes (only the status part), for ConnectionUsageTest.
|
||||
|
||||
if (! class_exists('WP_HTTP_Response')) {
|
||||
class WP_HTTP_Response
|
||||
{
|
||||
public function __construct(public mixed $data = null, public int $status = 200)
|
||||
{
|
||||
}
|
||||
|
||||
public function get_status(): int
|
||||
{
|
||||
return $this->status;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
if (! class_exists('WP_REST_Response')) {
|
||||
class WP_REST_Response extends WP_HTTP_Response
|
||||
{
|
||||
}
|
||||
}
|
||||
@@ -218,6 +218,7 @@ final class LogListTableTest extends TestCase
|
||||
|
||||
self::assertStringContainsString('dashicons-' . $icon, $html);
|
||||
self::assertStringContainsString($word, $html);
|
||||
self::assertStringNotContainsString('error', $html);
|
||||
}
|
||||
|
||||
/** @return list<list<string>> */
|
||||
@@ -226,7 +227,7 @@ final class LogListTableTest extends TestCase
|
||||
return [
|
||||
['ok', 'yes-alt', 'erfolgreich'],
|
||||
['partial', 'warning', 'teilweise'],
|
||||
['error<b>', 'dismiss', 'error<b>'],
|
||||
['error<b>', 'dismiss', 'unbekannt'],
|
||||
];
|
||||
}
|
||||
|
||||
|
||||
@@ -255,16 +255,45 @@ final class ToolsTabTest extends TestCase
|
||||
$registry = new IntegrationRegistry($this->integration('media', [
|
||||
['lit/media-preview', true, true],
|
||||
['lit/media-upload', false, false],
|
||||
['lit/media-delete', false, false, true],
|
||||
]));
|
||||
|
||||
$html = $this->render([], $registry);
|
||||
|
||||
self::assertStringContainsString('<th scope="col" class="column-kind">Art</th>', $html);
|
||||
self::assertStringContainsString('liest', $this->row($html, 'lit/media-preview'));
|
||||
self::assertStringContainsString('schreibt', $this->row($html, 'lit/media-upload'));
|
||||
self::assertStringContainsString('löscht', $this->row($html, 'lit/media-delete'));
|
||||
self::assertStringNotContainsString('schreibt', $this->row($html, 'lit/media-delete'));
|
||||
self::assertStringContainsString('dashicons-visibility', $this->row($html, 'lit/media-preview'));
|
||||
self::assertStringContainsString('>liest<', $this->row($html, 'lit/media-preview'));
|
||||
self::assertStringContainsString('dashicons-edit', $this->row($html, 'lit/media-upload'));
|
||||
self::assertStringContainsString('>schreibt<', $this->row($html, 'lit/media-upload'));
|
||||
}
|
||||
|
||||
/**
|
||||
* "destructive" is a broad hint (deleting, replacing, swapping) – only explicit deletes say "löscht".
|
||||
*
|
||||
* @dataProvider destructiveAbilities
|
||||
*/
|
||||
public function testArtColumnForEveryDestructiveAbility(string $name, string $icon, string $word): void
|
||||
{
|
||||
$registry = new IntegrationRegistry($this->integration('wordpress', [[$name, false, false, true]]));
|
||||
|
||||
$row = $this->row($this->render([], $registry), $name);
|
||||
|
||||
self::assertStringContainsString('dashicons-' . $icon, $row);
|
||||
self::assertStringContainsString('>' . $word . '<', $row);
|
||||
}
|
||||
|
||||
/** @return list<list<string>> */
|
||||
public static function destructiveAbilities(): array
|
||||
{
|
||||
return [
|
||||
['lit/media-delete', 'trash', 'löscht'],
|
||||
['lit/divi-delete', 'trash', 'löscht'],
|
||||
['lit/post-trash', 'trash', 'Papierkorb'],
|
||||
['lit/post-replace-with-copy', 'update', 'überschreibt'],
|
||||
['lit/revision-restore', 'update', 'überschreibt'],
|
||||
['lit/menu-set-items', 'update', 'überschreibt'],
|
||||
['lit/menu-assign-location', 'update', 'überschreibt'],
|
||||
['lit/divi-replace-text', 'update', 'überschreibt'],
|
||||
];
|
||||
}
|
||||
|
||||
public function testToolCellShowsLabelAndTheNameAsMutedTextWithALabelledSelect(): void
|
||||
|
||||
Reference in New Issue
Block a user