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.
|
Thank you for the PR! |
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