feat(setup-checks): let ExApps surface setup checks in the admin overview#908
feat(setup-checks): let ExApps surface setup checks in the admin overview#908oleksandr-nc wants to merge 1 commit into
Conversation
oleksandr-nc
force-pushed
the
enh/exapp-setup-checks
branch
4 times, most recently
from
1942754 to
6f8c0a5
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds ExApp setup-check support end to end. It introduces state and refresh services, timed and queued background jobs, setup-check implementations for error and warning severities, OCS endpoints for register and unregister actions, OpenAPI updates, and PHPUnit tests covering refresh, persistence, rendering, and API flows. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
tests/php/Service/ExAppSetupCheckServiceTest.php (1)
33-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the exact 64-byte key boundary too.
This only exercises a clearly invalid app id. If
isValidAppId()regressed from<=to<, the suite would still stay green while rejecting the longest valid id (strlen('setup_checks_') + 51 === 64).Suggested test addition
public function testOptInIgnoresOverlongAppId(): void { // KEY_PREFIX (13) + appId must stay within IAppConfig's 64-char key limit. $this->appConfig->expects(self::never())->method('setValueString'); $this->service->optIn(str_repeat('a', 60)); } + +public function testOptInAllowsMaxLengthAppId(): void { + $appId = str_repeat('a', 51); + $this->appConfig->expects(self::once()) + ->method('setValueString') + ->with('app_api', 'setup_checks_' . $appId, '1'); + $this->service->optIn($appId); +}tests/php/Service/ExAppSetupCheckRefreshServiceTest.php (1)
83-92: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the
/setup_checksrequest contract in the helper.The callback only keys on
$exApp->getAppid(), so these tests would still pass ifrefresh()hit the wrong path, used the wrong HTTP verb, or stopped sending the timeout option.Suggested tightening
$this->exAppService->method('getExApp')->willReturnCallback(fn (string $id): ?ExApp => $appsById[$id] ?? null); $this->appAPIService->method('requestToExApp')->willReturnCallback( - fn (ExApp $exApp) => $resultsByAppId[$exApp->getAppid()] + function (ExApp $exApp, string $path, mixed $body, string $method, array $headers, array $options) use ($resultsByAppId) { + self::assertSame('/setup_checks', $path); + self::assertSame('GET', $method); + self::assertArrayHasKey('timeout', $options); + return $resultsByAppId[$exApp->getAppid()]; + } );tests/php/SetupChecks/ExAppsSetupCheckTest.php (1)
111-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a numeric-looking app-id case for the stored-state filter.
buildFromState()explicitly casts decoded state keys back tostringbecause JSON can turn"123"into123. The current suite only uses alphabetic ids, so that guard can regress silently.Suggested test addition
public function testNotOptedInAppInStateIsIgnored(): void { $result = $this->runCheck( ['a'], ['gone' => [$this->issue('error', 'stale', 'App gone')]], ['gone' => $this->makeExApp('gone')], ); self::assertSame('success', $result->getSeverity()); } + +public function testNumericLookingAppIdFromStoredStateIsHandled(): void { + $result = $this->runCheck( + ['123'], + ['123' => [$this->issue('warning', 'needs attention', 'App 123')]], + ['123' => $this->makeExApp('123')], + ); + self::assertSame('warning', $result->getSeverity()); + self::assertStringContainsString('App 123: needs attention', (string)$result->getDescription()); +}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9673faf8-d2df-46ae-aeb5-ec5fc64b45b3
📒 Files selected for processing (15)
appinfo/info.xmlappinfo/routes.phplib/AppInfo/Application.phplib/BackgroundJob/ExAppSetupChecksRefreshJob.phplib/BackgroundJob/ExAppSetupChecksRefreshOnceJob.phplib/Controller/SetupCheckController.phplib/Service/ExAppService.phplib/Service/ExAppSetupCheckRefreshService.phplib/Service/ExAppSetupCheckService.phplib/SetupChecks/ExAppsSetupCheck.phpopenapi-full.jsonopenapi.jsontests/php/Service/ExAppSetupCheckRefreshServiceTest.phptests/php/Service/ExAppSetupCheckServiceTest.phptests/php/SetupChecks/ExAppsSetupCheckTest.php
oleksandr-nc
force-pushed
the
enh/exapp-setup-checks
branch
from
6f8c0a5 to
120467f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/php/Service/ExAppSetupCheckRefreshServiceTest.php (1)
56-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the header stub depend on the requested header name.
getHeader()currently returns the same value for any header, so the size-limit tests still pass even if the production code reads the wrong header or stops checkingContent-Lengthentirely. Restrict the stub toContent-Lengthso these cases actually validate the bound-checking path.Suggested tightening
private function response(int $statusCode, string $body, ?int $contentLength = null): IResponse&MockObject { $response = $this->createMock(IResponse::class); $response->method('getStatusCode')->willReturn($statusCode); $response->method('getBody')->willReturn($body); - // default to a realistic Content-Length matching the body - $response->method('getHeader')->willReturn((string)($contentLength ?? strlen($body))); + $response->method('getHeader')->willReturnCallback( + static fn (string $name): string => $name === 'Content-Length' + ? (string)($contentLength ?? strlen($body)) + : '', + ); return $response; } private function responseNoContentLength(int $statusCode, string $body): IResponse&MockObject { $response = $this->createMock(IResponse::class); $response->method('getStatusCode')->willReturn($statusCode); $response->method('getBody')->willReturn($body); - $response->method('getHeader')->willReturn(''); // no Content-Length header + $response->method('getHeader')->willReturnCallback( + static fn (string $name): string => '', + ); // no Content-Length header return $response; }tests/php/Service/ExAppSetupCheckServiceTest.php (1)
58-71: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the
IAppConfignamespace and state key in the round-trip test.These callbacks ignore
$aand$k, so the test still goes green if the service persists state under the wrong app or key. Since this feature lives entirely on theIAppConfigcontract, lock the expectation to the same namespace/key pair used elsewhere.Suggested tightening
public function testStoreAndGetStateRoundTrip(): void { $stored = ''; - $this->appConfig->method('setValueString')->willReturnCallback(function (string $a, string $k, string $v) use (&$stored): bool { + $this->appConfig->expects(self::once()) + ->method('setValueString') + ->with('app_api', 'setupchecks_state', self::isType('string')) + ->willReturnCallback(function (string $a, string $k, string $v) use (&$stored): bool { $stored = $v; return true; }); - $this->appConfig->method('getValueString')->willReturnCallback(function (string $a, string $k, string $d = '') use (&$stored): string { + $this->appConfig->expects(self::once()) + ->method('getValueString') + ->with('app_api', 'setupchecks_state', '') + ->willReturnCallback(function (string $a, string $k, string $d = '') use (&$stored): string { return $stored !== '' ? $stored : $d; });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2698c646-2da4-4a8d-ac9f-4c4b6d23039d
📒 Files selected for processing (18)
appinfo/info.xmlappinfo/routes.phplib/AppInfo/Application.phplib/BackgroundJob/ExAppSetupChecksRefreshJob.phplib/BackgroundJob/ExAppSetupChecksRefreshOnceJob.phplib/Controller/SetupCheckController.phplib/Service/ExAppService.phplib/Service/ExAppSetupCheckRefreshService.phplib/Service/ExAppSetupCheckService.phplib/SetupChecks/AbstractExAppsSetupCheck.phplib/SetupChecks/ExAppsErrorSetupCheck.phplib/SetupChecks/ExAppsInfoSetupCheck.phplib/SetupChecks/ExAppsWarningSetupCheck.phpopenapi-full.jsonopenapi.jsontests/php/Service/ExAppSetupCheckRefreshServiceTest.phptests/php/Service/ExAppSetupCheckServiceTest.phptests/php/SetupChecks/ExAppsSetupCheckTest.php
✅ Files skipped from review due to trivial changes (2)
- lib/SetupChecks/ExAppsInfoSetupCheck.php
- appinfo/info.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- lib/Service/ExAppService.php
- openapi.json
- lib/Controller/SetupCheckController.php
- appinfo/routes.php
- lib/Service/ExAppSetupCheckService.php
- lib/Service/ExAppSetupCheckRefreshService.php
- openapi-full.json
oleksandr-nc
force-pushed
the
enh/exapp-setup-checks
branch
from
120467f to
d5cd289
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/php/SetupChecks/ExAppsSetupCheckTest.php (1)
68-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression case for numeric-looking app IDs.
AbstractExAppsSetupCheck::collectIssues()deliberately casts decoded state keys back to string before comparing them with the opted-in ids. This suite never exercises that path, so a future cleanup could remove the cast and silently drop valid checks for app ids like"123"from the Overview.Suggested test
+ public function testNumericLookingAppIdFromStoredStateIsStillMatched(): void { + $r = $this->runCheck( + ExAppsErrorSetupCheck::class, + ['123'], + [123 => [$this->issue('error', 'numeric id issue', 'App 123')]], + ['123' => $this->makeExApp('123')], + ); + self::assertSame('error', $r->getSeverity()); + self::assertStringContainsString('numeric id issue', (string)$r->getDescription()); + }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fdee7d6-f198-41b5-91ae-c9f0991a9338
📒 Files selected for processing (17)
appinfo/info.xmlappinfo/routes.phplib/AppInfo/Application.phplib/BackgroundJob/ExAppSetupChecksRefreshJob.phplib/BackgroundJob/ExAppSetupChecksRefreshOnceJob.phplib/Controller/SetupCheckController.phplib/Service/ExAppService.phplib/Service/ExAppSetupCheckRefreshService.phplib/Service/ExAppSetupCheckService.phplib/SetupChecks/AbstractExAppsSetupCheck.phplib/SetupChecks/ExAppsErrorSetupCheck.phplib/SetupChecks/ExAppsWarningSetupCheck.phpopenapi-full.jsonopenapi.jsontests/php/Service/ExAppSetupCheckRefreshServiceTest.phptests/php/Service/ExAppSetupCheckServiceTest.phptests/php/SetupChecks/ExAppsSetupCheckTest.php
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/SetupChecks/ExAppsErrorSetupCheck.php
- lib/AppInfo/Application.php
- lib/SetupChecks/ExAppsWarningSetupCheck.php
- appinfo/routes.php
- lib/Service/ExAppService.php
- appinfo/info.xml
- openapi-full.json
- lib/Service/ExAppSetupCheckService.php
- lib/Controller/SetupCheckController.php
- openapi.json
- lib/Service/ExAppSetupCheckRefreshService.php
- lib/SetupChecks/AbstractExAppsSetupCheck.php
oleksandr-nc
force-pushed
the
enh/exapp-setup-checks
branch
from
d5cd289 to
a2cb56d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d23c60c1-af40-49f9-8168-a496095ce956
📒 Files selected for processing (17)
appinfo/info.xmlappinfo/routes.phplib/AppInfo/Application.phplib/BackgroundJob/ExAppSetupChecksRefreshJob.phplib/BackgroundJob/ExAppSetupChecksRefreshOnceJob.phplib/Controller/SetupCheckController.phplib/Service/ExAppService.phplib/Service/ExAppSetupCheckRefreshService.phplib/Service/ExAppSetupCheckService.phplib/SetupChecks/AbstractExAppsSetupCheck.phplib/SetupChecks/ExAppsErrorSetupCheck.phplib/SetupChecks/ExAppsWarningSetupCheck.phpopenapi-full.jsonopenapi.jsontests/php/Service/ExAppSetupCheckRefreshServiceTest.phptests/php/Service/ExAppSetupCheckServiceTest.phptests/php/SetupChecks/ExAppsSetupCheckTest.php
✅ Files skipped from review due to trivial changes (1)
- appinfo/info.xml
🚧 Files skipped from review as they are similar to previous changes (11)
- lib/Controller/SetupCheckController.php
- lib/SetupChecks/ExAppsWarningSetupCheck.php
- appinfo/routes.php
- lib/AppInfo/Application.php
- openapi.json
- lib/SetupChecks/ExAppsErrorSetupCheck.php
- lib/Service/ExAppService.php
- openapi-full.json
- lib/Service/ExAppSetupCheckService.php
- lib/Service/ExAppSetupCheckRefreshService.php
- lib/SetupChecks/AbstractExAppsSetupCheck.php
…view Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
oleksandr-nc
force-pushed
the
enh/exapp-setup-checks
branch
from
a2cb56d to
078c023
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: de26f816-efaa-4cec-910f-3da9f52fca50
📒 Files selected for processing (17)
appinfo/info.xmlappinfo/routes.phplib/AppInfo/Application.phplib/BackgroundJob/ExAppSetupChecksRefreshJob.phplib/BackgroundJob/ExAppSetupChecksRefreshOnceJob.phplib/Controller/SetupCheckController.phplib/Service/ExAppService.phplib/Service/ExAppSetupCheckRefreshService.phplib/Service/ExAppSetupCheckService.phplib/SetupChecks/AbstractExAppsSetupCheck.phplib/SetupChecks/ExAppsErrorSetupCheck.phplib/SetupChecks/ExAppsWarningSetupCheck.phpopenapi-full.jsonopenapi.jsontests/php/Service/ExAppSetupCheckRefreshServiceTest.phptests/php/Service/ExAppSetupCheckServiceTest.phptests/php/SetupChecks/ExAppsSetupCheckTest.php
🚧 Files skipped from review as they are similar to previous changes (12)
- lib/Service/ExAppService.php
- lib/SetupChecks/ExAppsWarningSetupCheck.php
- lib/SetupChecks/ExAppsErrorSetupCheck.php
- appinfo/info.xml
- lib/Controller/SetupCheckController.php
- lib/AppInfo/Application.php
- appinfo/routes.php
- lib/Service/ExAppSetupCheckService.php
- lib/Service/ExAppSetupCheckRefreshService.php
- openapi.json
- lib/SetupChecks/AbstractExAppsSetupCheck.php
- openapi-full.json
1 participant
Implements #888.
ExApps can now surface admin-facing setup checks in Settings > Administration > Overview, next to the
core ones (e.g. an ExApp warning that it isn't configured correctly). An ExApp opts in with
POST /api/v1/setup_check(andDELETEto opt out); app_api registers a single aggregate "ExternalApps"
ISetupCheck, and the results come from each opted-in ExApp's/setup_checksendpoint. A downor slow ExApp shows "not responding".
The SetupCheck page itself does no HTTP. The fan-out to ExApps runs entirely in the background (a
10-min TimedJob plus a one-off job that an admin's page visit enqueues, stale-while-revalidate) and
stores the results in
IAppConfig; the page just reads those, filtered to the currently opted-in andenabled ExApps. So a slow, down, or errored ExApp can never slow down the admin page.
Notes:
IAppConfig.Draft: the
nc_py_apihelper (a@setup_checkdecorator) and docs are a follow-up; this is theserver/app_api side. NC35-only, not backported.