Fixing some small Honeybager alerts#54
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
ericvanjohnson
had a problem deploying
to
Testing
GitHub Actions
Failure
WalkthroughThis PR adds support for optional website fields on Sponsor records. It introduces a database migration to make the Changes
Suggested reviewers
Poem
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (2)
tests/Feature/Http/Controllers/SponsorControllerTest.php (1)
29-33: Simplifyattach()call by removing redundant foreign keys.Laravel's
BelongsToMany::attach()automatically handles the foreign keys (sponsor_uuid,conference_uuid) based on the relationship definition. Only pivot-specific data likesponsorship_levelneeds to be passed.♻️ Proposed simplification
$this->conference->sponsors()->attach($sponsor->uuid, [ - 'sponsor_uuid' => $sponsor->uuid, - 'conference_uuid' => $this->conference->uuid, 'sponsorship_level' => 'gold', ]);Apply the same simplification to the other
attach()calls on lines 45-49, 64-68, and 70-74.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Feature/Http/Controllers/SponsorControllerTest.php` around lines 29 - 33, The attach() calls in SponsorControllerTest.php pass redundant pivot foreign keys; update each occurrence of $this->conference->sponsors()->attach(...) (including the other three similar calls) to only pass the related model id/uuid and the pivot-specific data (remove 'sponsor_uuid' and 'conference_uuid'), e.g. call attach($sponsor->uuid, ['sponsorship_level' => 'gold']) so Laravel's BelongsToMany relationship supplies the foreign keys automatically.database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php (1)
7-17: Consider adding adown()method for rollback capability.The migration lacks a
down()method to reverse the schema change. While Laravel allows this, it prevents clean rollbacks during development or if issues arise in production.🔧 Proposed fix to add rollback support
public function up(): void { Schema::table('sponsors', function (Blueprint $table) { $table->string('website')->nullable()->change(); }); } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('sponsors', function (Blueprint $table) { + $table->string('website')->nullable(false)->change(); + }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php` around lines 7 - 17, Add a down() method to the anonymous Migration class to reverse the up() change: in down() call Schema::table('sponsors', function (Blueprint $table) { ... }) and change the 'website' column back to non-nullable (undo the ->nullable()->change() applied in up()). Reference the same migration class and methods (up(), down(), Schema::table, Blueprint) and ensure the change() call mirrors the up() but removes nullability so rollbacks restore the original state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Feature/Http/Controllers/SponsorControllerTest.php`:
- Around line 35-39: The test in SponsorControllerTest is hitting the model's
logoUrl accessor which calls Storage::disk('s3')->url() because the factory
produced a relative logo path; to fix, update the test to either use the
phpArchitect() factory state (so the factory returns a full HTTP logo URL), add
a new factory state that sets a non-S3 or absolute logo path, or mock the
storage by calling Storage::fake() before creating the Sponsor so logoUrl won’t
call the real S3 disk; adjust the test setup accordingly to use phpArchitect()
or Storage::fake() and then assertSee as before.
---
Nitpick comments:
In `@database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php`:
- Around line 7-17: Add a down() method to the anonymous Migration class to
reverse the up() change: in down() call Schema::table('sponsors', function
(Blueprint $table) { ... }) and change the 'website' column back to non-nullable
(undo the ->nullable()->change() applied in up()). Reference the same migration
class and methods (up(), down(), Schema::table, Blueprint) and ensure the
change() call mirrors the up() but removes nullability so rollbacks restore the
original state.
In `@tests/Feature/Http/Controllers/SponsorControllerTest.php`:
- Around line 29-33: The attach() calls in SponsorControllerTest.php pass
redundant pivot foreign keys; update each occurrence of
$this->conference->sponsors()->attach(...) (including the other three similar
calls) to only pass the related model id/uuid and the pivot-specific data
(remove 'sponsor_uuid' and 'conference_uuid'), e.g. call attach($sponsor->uuid,
['sponsorship_level' => 'gold']) so Laravel's BelongsToMany relationship
supplies the foreign keys automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 549df2be-1602-4547-be40-c97bd61fba2f
📒 Files selected for processing (4)
database/factories/SponsorFactory.phpdatabase/migrations/2026_03_31_004613_make_sponsor_website_nullable.phpresources/views/components/partners-section.blade.phptests/Feature/Http/Controllers/SponsorControllerTest.php
| $response = $this->get('/'); | ||
|
|
||
| $response->assertStatus(200); | ||
| $response->assertSee($sponsor->name, false); | ||
| $response->assertSee('example.com', false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other tests handle file storage/S3 dependencies
echo "=== Searching for Storage mocking patterns in existing tests ==="
rg -n "Storage::fake|fake\('s3'\)|mockery.*Storage" tests/ --type php -C 2
echo ""
echo "=== Check if there's a test trait for storage ==="
fd -t f "Storage" tests/ --exec head -30 {}
echo ""
echo "=== Looking at .env.testing or phpunit.xml for filesystem config ==="
cat .env.testing 2>/dev/null || echo ".env.testing not found"
cat phpunit.xml | grep -A2 -B2 "FILESYSTEM\|AWS" || echo "No filesystem env vars in phpunit.xml"Repository: phparch/phptek-2026
Length of output: 309
🏁 Script executed:
#!/bin/bash
# Read the actual test file to understand the S3 issue
echo "=== Reading SponsorControllerTest.php ==="
wc -l tests/Feature/Http/Controllers/SponsorControllerTest.php
head -60 tests/Feature/Http/Controllers/SponsorControllerTest.php
echo ""
echo "=== Checking phpunit.xml for full configuration ==="
cat phpunit.xmlRepository: phparch/phptek-2026
Length of output: 3179
🏁 Script executed:
#!/bin/bash
# Check the Sponsor model and factory for storage/image usage
echo "=== Checking Sponsor model ==="
find app -name "Sponsor.php" -type f | head -1 | xargs cat -n
echo ""
echo "=== Checking Sponsor factory ==="
find database -name "*SponsorFactory*" -type f | head -1 | xargs cat -n
echo ""
echo "=== Searching for file/image/logo fields in migrations ==="
find database/migrations -name "*.php" | xargs rg -n "sponsor|Sponsor" | head -20Repository: phparch/phptek-2026
Length of output: 6828
Use the phpArchitect() factory state or mock Storage to avoid S3 dependency in tests.
The sponsor logo paths in the factory default to relative paths (e.g., 'images/sponsors/acme.png'), which trigger the logoUrl accessor to call Storage::disk('s3')->url(). Since the test environment has no S3 configuration, the tests fail. Either use the existing phpArchitect() factory state which uses an HTTP URL, add a factory state for testing with a non-S3 logo, or mock the storage layer with Storage::fake().
🧰 Tools
🪛 GitHub Actions: tests
[error] 35-40: SponsorControllerTest > partners section renders sponsors with website: expected response status code [200] but received 500. Underlying exception: InvalidArgumentException: The GetObject operation requires non-empty parameter: Bucket (triggered while rendering /resources/views/components/partners-section.blade.php).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Feature/Http/Controllers/SponsorControllerTest.php` around lines 35 -
39, The test in SponsorControllerTest is hitting the model's logoUrl accessor
which calls Storage::disk('s3')->url() because the factory produced a relative
logo path; to fix, update the test to either use the phpArchitect() factory
state (so the factory returns a full HTTP logo URL), add a new factory state
that sets a non-S3 or absolute logo path, or mock the storage by calling
Storage::fake() before creating the Sponsor so logoUrl won’t call the real S3
disk; adjust the test setup accordingly to use phpArchitect() or Storage::fake()
and then assertSee as before.
There was a problem hiding this comment.
Pull request overview
This PR aims to address Honeybadger alerts by making sponsor websites optional end-to-end (DB/schema + factory/test data + UI rendering) so the partners modal doesn’t error when a sponsor has no website.
Changes:
- Updates the partners modal JS to hide the “Website” block when
sponsor.websiteis missing. - Makes
sponsors.websitenullable and adds aSponsorFactory::withoutWebsite()state. - Replaces the placeholder SponsorController test with feature tests covering sponsors with/without websites on the home page.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/Feature/Http/Controllers/SponsorControllerTest.php | Adds feature tests asserting the home page renders sponsors correctly when website is present/absent. |
| resources/views/components/partners-section.blade.php | Prevents JS errors by conditionally rendering/hiding the sponsor website link in the modal. |
| database/migrations/2026_03_31_004613_make_sponsor_website_nullable.php | Alters sponsors.website to be nullable. |
| database/factories/SponsorFactory.php | Adds a factory state for sponsors with a null website. |
| Schema::table('sponsors', function (Blueprint $table) { | ||
| $table->string('website')->nullable()->change(); | ||
| }); |
There was a problem hiding this comment.
This migration uses ->change() to alter an existing column, but doctrine/dbal is not installed (not present in composer.json). On common setups (including SQLite), this can cause migrations to fail at runtime. Consider adding the required dependency or rewriting the migration to avoid change() (e.g., a DB-specific alter statement or table rebuild strategy).
| Schema::table('sponsors', function (Blueprint $table) { | |
| $table->string('website')->nullable()->change(); | |
| }); | |
| $connection = Schema::getConnection(); | |
| $driver = $connection->getDriverName(); | |
| if ($driver === 'mysql') { | |
| $connection->statement('ALTER TABLE sponsors MODIFY website VARCHAR(255) NULL'); | |
| } elseif ($driver === 'pgsql') { | |
| $connection->statement('ALTER TABLE sponsors ALTER COLUMN website DROP NOT NULL'); | |
| } elseif ($driver === 'sqlsrv') { | |
| $connection->statement('ALTER TABLE sponsors ALTER COLUMN website NVARCHAR(255) NULL'); | |
| } |
| return new class extends Migration | ||
| { | ||
| /** | ||
| * Run the migrations. | ||
| */ | ||
| public function up(): void | ||
| { | ||
| Schema::table('sponsors', function (Blueprint $table) { | ||
| $table->string('website')->nullable()->change(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The migration is missing a down() method, which makes rollbacks impossible. Please add a down() that restores the previous NOT NULL constraint (and consider how to handle existing NULL website values before altering back).
| beforeEach(function () { | ||
| $this->conference = Conference::create([ | ||
| 'uuid' => 'test-conference-uuid', | ||
| 'name' => 'PHP Tek 2026', |
There was a problem hiding this comment.
beforeEach assigns $this->conference, which creates a dynamic property on Tests\TestCase. With PHP 8.4, dynamic properties are deprecated and may surface as test noise or failures depending on CI flags. Prefer a file-scoped variable captured by reference, or define a typed $conference property on the base test case if you want to keep using $this->....
| use Illuminate\Foundation\Testing\RefreshDatabase; | ||
|
|
||
| uses(RefreshDatabase::class); | ||
|
|
There was a problem hiding this comment.
uses(RefreshDatabase::class); is redundant for Feature Pest tests in this repo because RefreshDatabase is already applied globally in tests/Pest.php. Removing it will keep the file consistent with the existing test setup.
| use Illuminate\Foundation\Testing\RefreshDatabase; | |
| uses(RefreshDatabase::class); |
| test('partners section renders sponsors with website', function () { | ||
| $sponsor = Sponsor::factory()->create([ |
There was a problem hiding this comment.
This file path/name (Http/Controllers/SponsorControllerTest) doesn’t match what’s being tested (homepage partners section rendering). Renaming/moving the test file to reflect the behavior under test will make it easier to discover and maintain.
| $this->conference->sponsors()->attach($sponsor->uuid, [ | ||
| 'sponsor_uuid' => $sponsor->uuid, | ||
| 'conference_uuid' => $this->conference->uuid, | ||
| 'sponsorship_level' => 'gold', | ||
| ]); |
There was a problem hiding this comment.
When attaching sponsors to the conference, the pivot attributes include sponsor_uuid and conference_uuid, which the belongsToMany relationship will already populate. Keeping only the extra pivot fields (e.g., sponsorship_level) reduces redundancy and avoids accidental mismatches.
fwallen
left a comment
fwallen
left a comment
There was a problem hiding this comment.
Only one question. Not pulling the handbrake.
| websiteEl.href = sponsor.website; | ||
| websiteEl.textContent = sponsor.website; | ||
| websiteEl.parentElement.style.display = ''; | ||
| } else { | ||
| websiteEl.href = '#'; | ||
| websiteEl.textContent = ''; | ||
| websiteEl.parentElement.style.display = 'none'; |
There was a problem hiding this comment.
Should these references be sponsor.website?
There was a problem hiding this comment.
I don't follow. They are.
There was a problem hiding this comment.
In the if we check for sponsor.websiteEl. Assigning the values to the property should be something ilke:
sponsor.websiteEl.href = sponsor.website;
sponsor.websiteEl.textContent = sponsor.website;
sponsor.websiteEl.parentElement.style.display = '';
Summary by CodeRabbit
New Features
Tests