fix: server URL generation in ServerPatchCheck notification #7018
Conversation
…llution
Moved ServerPatchCheckNotificationTest from Unit to Feature tests and replaced
Mockery alias mocking with real database records to prevent global state pollution.
The original implementation used Mockery::mock('alias:InstanceSettings::class)
which creates a global class alias that persists across all tests, causing
other tests to fail when they try to use the real InstanceSettings model.
Changes:
- Moved test from tests/Unit/ to tests/Feature/ (requires database access)
- Replaced Mockery alias mocking with RefreshDatabase and real InstanceSettings records
- Tests now create actual InstanceSettings records in the test database
- Preserved Server mocking with Mockery for non-database dependencies
All 4 tests pass individually and when run via php artisan test without
polluting global state or affecting other tests.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughFixed a bug where server security patch notifications were generating localhost URLs instead of using the configured instance FQDN. Replaced environment-specific route helpers with a base URL-based approach to ensure notifications properly link to the correct domain across all notification channels. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Ah yes, localhost URLs in production notifications—the classic "it works on my machine" that escapes into the wild. This fix ensures your self-hosted Coolify instance actually points to itself correctly, not some imaginary localhost dimension. Much better than serverless nonsense where you need a business degree just to understand where your notifications are going. Speaking of which, I'd celebrate this fix with a gluten-free taco—the kind that actually knows where it's supposed to go. 🌮 Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
| Out of Scope Changes Check | ✅ Passed | All code changes remain tightly scoped to the linked issue. The modifications to ServerPatchCheck.php directly fix the URL generation problem, and the corresponding test additions in ServerPatchCheckNotificationTest.php provide necessary verification of the fix. No unrelated refactoring, formatting changes, or tangential improvements have been introduced. The changes are focused solely on resolving the localhost URL issue and validating the solution works across supported notification channels. | ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
app/Notifications/Server/ServerPatchCheck.php(1 hunks)tests/Feature/ServerPatchCheckNotificationTest.php(1 hunks)
🔇 Additional comments (3)
tests/Feature/ServerPatchCheckNotificationTest.php (3)
10-42: Hasta la vista, Mockery pollution.Excellent choice using real
InstanceSettingsrecords instead of Mockery aliases. That's the kind of pragmatic self-hosting wisdom I respect—actual database records that won't pollute global state like some serverless function leaving orphaned Lambda instances everywhere. Your mock server setup is clean and reusable, like a well-maintained T-800 endoskeleton.
82-123: Come with me if you want comprehensive test coverage.Chef's kiss 🌮 This is beautiful—you're testing all five notification channels like a proper self-hosted warrior. Discord, Telegram, Pushover, Slack, and Webhook all get their URLs validated. No channel left behind, unlike those serverless functions that just disappear into the ether when you're not looking. Each assertion properly checks the channel's specific data structure.
This is the kind of thorough testing that prevents production fires. I mean, unless it's a T-1000 causing the fire, then you've got bigger problems.
125-146: Your error notifications won't be terminated.Solid error case coverage here. You're verifying that even when things go wrong (package manager failures, connection issues), your notifications still generate correct URLs and proper event names. The
server_patch_check_errorevent check is a nice touch—makes webhook consumers' lives easier.
| if (isDev()) { | ||
| $this->serverUrl = 'https://staging-but-dev.coolify.io/server/'.$this->server->uuid.'/security/patches'; | ||
| } | ||
| $this->serverUrl = base_url().'/server/'.$this->server->uuid.'/security/patches'; |
There was a problem hiding this comment.
🧩 Analysis chain
I'll be back... to debug this when the route changes.
Listen, I appreciate you ditching that localhost garbage—servers deserve proper URLs, not some localhost nonsense that belongs in a serverless marketer's fever dream. But hardcoding /server/{uuid}/security/patches directly into the string? That's like removing the targeting computer and aiming the plasma rifle manually. Sure, it works, but if someone changes the route definition in your routes file, this breaks silently with no warning.
Consider these options:
- Store the path pattern as a class constant or config value for easier maintenance
- Add a comment explaining why we're not using
route()here - At minimum, verify the hardcoded path matches your actual route definition
Run this script to verify your hardcoded path matches the registered route:
This way you'll know if your hardcoded path is actually correct, or if it's going to explode faster than a T-800 in a hydraulic press.
🏁 Script executed:
#!/bin/bash
# Verify that the hardcoded path matches the actual route definition
# Search for route definitions containing 'server.security.patches' or the path pattern
echo "=== Searching for route definition ==="
rg -n -A 2 -B 2 "server.security.patches|/server/.*security/patches" --type=php -g '!vendor/**' -g 'routes/**'
# Also check if base_url() helper is properly defined
echo -e "\n=== Checking base_url() helper definition ==="
ast-grep --pattern 'function base_url() {
$$$
}'Length of output: 685
I need more context on the route structure. Let me search for the route group that wraps this endpoint:
Now let me search for where base_url() is defined in the codebase and verify the full route structure:
Fix the fragile hardcoded path by either storing it as a class constant or adding verification.
The hardcoded path /server/{uuid}/security/patches matches your route definition, but you've traded compile-time route safety for localhost compatibility. Named routes normally allow you to generate URLs without being coupled to the actual URL defined on the route, so if the route's URL changes, no changes need to be made to your calls to the route function. You've sacrificed that benefit.
Consider one of these:
- Store the path pattern as a class constant:
private const PATCH_URL_PATTERN = '/server/{uuid}/security/patches'; - Document why
route()doesn't work here (localhost context issue with InstanceSettings) - Add a test verifying the path stays in sync with your route definition
This way, future developers (or future you) won't accidentally update routes without updating this class.
🤖 Prompt for AI Agents
In app/Notifications/Server/ServerPatchCheck.php around line 19, the server URL
is built with a fragile hardcoded path '/server/{uuid}/security/patches';
replace this by either (1) extracting the pattern into a private class constant
(e.g. PATCH_URL_PATTERN) and use that constant when building the URL, or (2)
prefer generating the URL from a named route (route('server.patches', ['uuid' =>
$this->server->uuid'])) if possible, and if route() truly cannot be used in
localhost/InstanceSettings context add a short comment explaining why and add a
unit/integration test asserting the constant matches the route definition to
prevent drift; update usages and tests accordingly.
| it('generates url using base_url instead of APP_URL', function () { | ||
| // Set InstanceSettings to return a specific FQDN | ||
| ($this->setInstanceSettings)('https://coolify.example.com'); | ||
|
|
||
| $mockServer = ($this->createMockServer)('test-server-uuid'); | ||
|
|
||
| $patchData = [ | ||
| 'total_updates' => 5, | ||
| 'updates' => [], | ||
| 'osId' => 'ubuntu', | ||
| 'package_manager' => 'apt', | ||
| ]; | ||
|
|
||
| $notification = new ServerPatchCheck($mockServer, $patchData); | ||
|
|
||
| // The URL should use the FQDN from InstanceSettings, not APP_URL | ||
| expect($notification->serverUrl)->toBe('https://coolify.example.com/server/test-server-uuid/security/patches'); | ||
| }); | ||
|
|
||
| it('falls back to public_ipv4 with port when fqdn is not set', function () { | ||
| // Set InstanceSettings to return public IPv4 | ||
| ($this->setInstanceSettings)(null, '192.168.1.100'); | ||
|
|
||
| $mockServer = ($this->createMockServer)('test-server-uuid'); | ||
|
|
||
| $patchData = [ | ||
| 'total_updates' => 3, | ||
| 'updates' => [], | ||
| 'osId' => 'debian', | ||
| 'package_manager' => 'apt', | ||
| ]; | ||
|
|
||
| $notification = new ServerPatchCheck($mockServer, $patchData); | ||
|
|
||
| // The URL should use public IPv4 with default port 8000 | ||
| expect($notification->serverUrl)->toBe('http://192.168.1.100:8000/server/test-server-uuid/security/patches'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Testing servers like a cybernetic organism—thorough and relentless.
These tests properly validate that your notifications will hit the right server URLs, not some localhost taco stand. FQDN test covers the happy path, and IPv4 fallback ensures nobody's notifications get lost in the void.
One small observation: your setup includes public_ipv6 capability, but there's no test case for IPv6 fallback. Might want to terminate that test gap if you support IPv6 deployments.
Consider adding an IPv6 test case:
it('falls back to public_ipv6 when fqdn and ipv4 are not set', function () {
($this->setInstanceSettings)(null, null, '2001:0db8::1');
$mockServer = ($this->createMockServer)('test-server-uuid');
$patchData = ['total_updates' => 2, 'updates' => [], 'osId' => 'ubuntu', 'package_manager' => 'apt'];
$notification = new ServerPatchCheck($mockServer, $patchData);
expect($notification->serverUrl)->toBe('http://[2001:0db8::1]:8000/server/test-server-uuid/security/patches');
});🤖 Prompt for AI Agents
tests/Feature/ServerPatchCheckNotificationTest.php lines 44-80: add a third test
to cover IPv6 fallback when FQDN and public IPv4 are not set; call
($this->setInstanceSettings)(null, null, '2001:0db8::1'), create the mock server
and patch data like the other tests, instantiate ServerPatchCheck, and assert
that serverUrl equals
'http://[2001:0db8::1]:8000/server/test-server-uuid/security/patches' (note IPv6
must be bracketed and include the default port 8000).
2 participants
Summary
Fixes ServerPatchCheck notifications returning incorrect
localhostURLs instead of using the configured domain or server IP.Problem
The notification was using Laravel's
route()helper which returnslocalhostURLs. Users were receiving notifications withhttp://localhost/server/{uuid}/security/patcheslinks that don't work when clicked.Solution
Changed to use
base_url()helper which correctly:Changes
Changed URL generation (commit e29b517):
route('server.security.patches')withbase_url() . '/server/...'Removed development override (commit bceef41):
https://staging-but-dev.coolify.ioURL used in developmentAdded unit tests (commit c16844d + 35b1044):
Test Results
✅ Verified on Discord, other channels should work similarly:
✅ URL generation respects configuration:
https://coolify.example.com/server/{uuid}/security/patches)http://192.168.1.100:8000/server/{uuid}/security/patches)✅ Works in all environments:
Files Changed
app/Notifications/Server/ServerPatchCheck.php- Fix URL generationtests/Unit/ServerPatchCheckNotificationTest.php- Add comprehensive test coverageRelated Issues
Closes #6038
🤖 Generated with Claude Code