fix: team permissions, transactions, form sentinels and review polish

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

app/Http/Controllers/ProductController.php

@@ -8,6 +8,7 @@ use App\Models\Product;
 use App\Models\Tag;
 use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Auth;
+use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Facades\Gate;
 use Illuminate\Support\Facades\Storage;
 use Illuminate\Validation\Rule;
@@ -121,9 +122,15 @@ class ProductController extends Controller
         $validated['user_id'] = Auth::id();
         $validated['status'] = 'pending';
 
+        $created = [];
+        $obsolete = [];
+
+        try {
+            $product = DB::transaction(function () use ($request, $validated, &$created, &$obsolete) {
                 // Handle thumbnail upload
                 if ($request->hasFile('thumbnail')) {
                     $path = $request->file('thumbnail')->store('thumbnails', 'public');
+                    $created[] = $path;
                     $validated['thumbnail'] = $path;
                 }
 
@@ -134,7 +141,14 @@ class ProductController extends Controller
                     $product->tags()->attach($request->tags);
                 }
 
-        $this->syncDetails($request, $product);
+                $this->syncDetails($request, $product, true, $created, $obsolete);
+
+                return $product;
+            });
+        } catch (\Throwable $e) {
+            Storage::disk('public')->delete($created);
+            throw $e;
+        }
 
         return redirect()->route('products.show', $product)
             ->with('success', 'Product submitted successfully! It will be reviewed by our moderators.');
@@ -193,23 +207,39 @@ class ProductController extends Controller
             $validated = array_diff_key($validated, Product::BADGES);
         }
 
+        // Only the submitter and admins manage the team
+        $manageMembers = Auth::id() === $product->user_id || Auth::user()->isAdmin();
+
+        $created = [];
+        $obsolete = [];
+
+        try {
+            DB::transaction(function () use ($request, $product, $validated, $manageMembers, &$created, &$obsolete) {
                 // Handle thumbnail upload
                 if ($request->hasFile('thumbnail')) {
-            // Delete old thumbnail if exists
                     if ($product->thumbnail) {
-                Storage::disk('public')->delete($product->thumbnail);
+                        $obsolete[] = $product->thumbnail;
                     }
 
                     $path = $request->file('thumbnail')->store('thumbnails', 'public');
+                    $created[] = $path;
                     $validated['thumbnail'] = $path;
                 }
 
                 $product->update($validated);
 
-        $this->syncDetails($request, $product);
+                $this->syncDetails($request, $product, $manageMembers, $created, $obsolete);
 
                 // Sync tags (will remove old tags and add new ones)
                 $product->tags()->sync($request->tags ?? []);
+            });
+        } catch (\Throwable $e) {
+            Storage::disk('public')->delete($created);
+            throw $e;
+        }
+
+        // Old files are only removed once the changes are committed
+        Storage::disk('public')->delete($obsolete);
 
         return redirect()->route('products.show', $product)
             ->with('success', 'Product updated successfully!');
@@ -231,7 +261,7 @@ class ProductController extends Controller
             'remove_screenshots.*' => 'integer',
             'plans' => 'nullable|array',
             'plans.*.name' => 'required|string|max:100',
-            'plans.*.price' => 'required|numeric|min:0|max:99999',
+            'plans.*.price' => 'required|numeric|min:0|max:99999|decimal:0,2',
             'plans.*.period' => ['required', Rule::in(PricingPlan::PERIODS)],
             'plans.*.description' => 'nullable|string|max:255',
             'members' => 'nullable|array',
@@ -242,10 +272,13 @@ class ProductController extends Controller
 
     /**
      * Sync related product detail data (badges are stored on the product itself).
+     * Must run inside a transaction: newly stored files are added to $created,
+     * files to remove after commit are added to $obsolete.
      */
-    protected function syncDetails(Request $request, Product $product): void
+    protected function syncDetails(Request $request, Product $product, bool $manageMembers, array &$created, array &$obsolete): void
     {
-        // The submitted plan list replaces all existing plans
+        // The submitted plan list replaces all existing plans (a missing sentinel means the form could not send one)
+        if ($request->has('plans_present') || $product->wasRecentlyCreated) {
             $product->pricingPlans()->delete();
             foreach (array_values((array) $request->input('plans', [])) as $i => $plan) {
                 $product->pricingPlans()->create([
@@ -256,19 +289,22 @@ class ProductController extends Controller
                     'position' => $i,
                 ]);
             }
+        }
 
         // The submitted member list replaces all existing members
+        if ($manageMembers && ($request->has('members_present') || $product->wasRecentlyCreated)) {
             $members = [];
             foreach ((array) $request->input('members', []) as $member) {
                 $members[$member['user_id']] = ['role' => $member['role']];
             }
             $product->members()->sync($members);
+        }
 
         // Only rows belonging to this product may be removed
         $ids = array_filter((array) $request->input('remove_screenshots', []));
         if ($ids) {
-            $product->screenshots()->whereIn('id', $ids)->get()->each(function ($shot) {
+            $product->screenshots()->whereIn('id', $ids)->get()->each(function ($shot) use (&$obsolete) {
-                Storage::disk('public')->delete($shot->path);
+                $obsolete[] = $shot->path;
                 $shot->delete();
             });
         }
@@ -276,8 +312,10 @@ class ProductController extends Controller
         if ($request->hasFile('screenshots')) {
             $position = ($product->screenshots()->max('position') ?? -1) + 1;
             foreach ($request->file('screenshots') as $file) {
+                $path = $file->store('screenshots', 'public');
+                $created[] = $path;
                 $product->screenshots()->create([
-                    'path' => $file->store('screenshots', 'public'),
+                    'path' => $path,
                     'position' => $position++,
                 ]);
             }
@@ -291,17 +329,16 @@ class ProductController extends Controller
     {
         Gate::authorize('delete-product', $product);
 
-        // Delete thumbnail if exists
+        $files = $product->screenshots->pluck('path')->all();
         if ($product->thumbnail) {
-            Storage::disk('public')->delete($product->thumbnail);
+            $files[] = $product->thumbnail;
-        }
-
-        foreach ($product->screenshots as $shot) {
-            Storage::disk('public')->delete($shot->path);
         }
 
         $product->delete();
 
+        // Files go only after the row is gone
+        Storage::disk('public')->delete($files);
+
         return redirect()->route('products.index')
             ->with('success', 'Product deleted successfully!');
     }

resources/views/components/product/gallery.blade.php

@@ -18,7 +18,7 @@
          @keydown.arrow-left.window="open && prev()"
          {{ $attributes }}>
         <div class="grid grid-cols-1 sm:grid-cols-3 gap-2">
-            <button type="button" @click="show(0)" class="sm:col-span-2 sm:row-span-3 block overflow-hidden rounded-lg border border-gray-200 dark:border-gray-700">
+            <button type="button" @click="show(0)" class="{{ $tiles->isEmpty() ? 'sm:col-span-3' : 'sm:col-span-2 sm:row-span-3' }} block overflow-hidden rounded-lg border border-gray-200 dark:border-gray-700">
                 <img src="{{ $urls[0] }}" alt="{{ $product->title }} screenshot 1" class="w-full h-full object-cover aspect-video sm:aspect-auto">
             </button>
             @foreach($tiles as $i => $shot)

resources/views/components/product/plans-form.blade.php

@@ -1,7 +1,7 @@
 @props(['plans' => []])
 
 @php
-    $initial = collect(old('plans', $plans))->map(fn ($p) => [
+    $initial = collect(old('plans_present') ? old('plans', []) : $plans)->map(fn ($p) => [
         'name' => $p['name'] ?? '',
         'price' => $p['price'] ?? '',
         'period' => $p['period'] ?? 'monthly',
@@ -12,6 +12,7 @@
 
 <!-- Pricing plans -->
 <div x-data="{ plans: @js($initial) }">
+    <input type="hidden" name="plans_present" value="1">
     <label class="block text-sm font-medium text-gray-700">Pricing plans</label>
     <div class="mt-2 space-y-3">
         <template x-for="(plan, i) in plans" :key="i">

resources/views/components/product/team-form.blade.php

@@ -1,7 +1,7 @@
-@props(['members' => []])
+@props(['members' => [], 'readonly' => false])
 
 @php
-    $initial = collect(old('members', collect($members)->map(fn ($m) => [
+    $initial = collect(old('members_present') ? old('members', []) : (collect($members)->map(fn ($m) => [
         'user_id' => $m->id,
         'name' => $m->name,
         'role' => $m->pivot->role,
@@ -14,6 +14,19 @@
 @endphp
 
 <!-- Team members -->
+@if($readonly)
+<div>
+    <label class="block text-sm font-medium text-gray-700">Team members</label>
+    <p class="mt-1 text-sm text-gray-500">Only the submitter or an admin can change the team.</p>
+    <ul class="mt-2 space-y-1">
+        @forelse($initial as $m)
+            <li class="text-sm text-gray-900">{{ $m['name'] }} <span class="text-gray-500">- {{ $m['role'] }}</span></li>
+        @empty
+            <li class="text-sm text-gray-500">No team members.</li>
+        @endforelse
+    </ul>
+</div>
+@else
 <div x-data="{
         members: @js($initial),
         q: '',
@@ -32,6 +45,7 @@
             this.q = ''; this.results = [];
         }
     }">
+    <input type="hidden" name="members_present" value="1">
     <label class="block text-sm font-medium text-gray-700">Team members</label>
     <div class="relative mt-2">
         <input type="text" x-model="q" @input="search()" placeholder="Search users by name" autocomplete="off" class="{{ $input }}">
@@ -45,6 +59,7 @@
         <template x-for="(member, i) in members" :key="member.user_id">
             <div class="grid grid-cols-1 sm:grid-cols-12 gap-2 p-3 border border-gray-200 rounded-md items-center">
                 <input type="hidden" :name="`members[${i}][user_id]`" :value="member.user_id">
+                <input type="hidden" :name="`members[${i}][name]`" :value="member.name">
                 <span class="sm:col-span-4 text-sm text-gray-900" x-text="member.name"></span>
                 <input type="text" :name="`members[${i}][role]`" x-model="member.role" placeholder="Role (e.g. Designer)" maxlength="100" class="{{ $input }} sm:col-span-7">
                 <button type="button" @click="members.splice(i, 1)" class="sm:col-span-1 text-sm text-red-600 hover:text-red-800">Remove</button>
@@ -58,3 +73,4 @@
         <p class="mt-2 text-sm text-red-600">{{ $message }}</p>
     @enderror
 </div>
+@endif

resources/views/products/edit.blade.php

@@ -253,7 +253,7 @@
 
                 <x-product.plans-form :plans="$product->pricingPlans->toArray()" />
 
-                <x-product.team-form :members="$product->members" />
+                <x-product.team-form :members="$product->members" :readonly="! (auth()->id() === $product->user_id || auth()->user()->isAdmin())" />
 
                 <!-- Info Alert -->
                 <div class="rounded-md bg-blue-50 p-4">

tests/Feature/ProductIntegrityTest.php

@@ -0,0 +1,229 @@
+<?php
+
+use App\Models\PricingPlan;
+use App\Models\Product;
+use App\Models\ProductScreenshot;
+use App\Models\User;
+use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Http\UploadedFile;
+use Illuminate\Support\Facades\Storage;
+
+uses(RefreshDatabase::class);
+
+function integrityPayload(Product $product, array $overrides = []): array
+{
+    return array_merge([
+        'title' => 'Integrity Product',
+        'description' => 'A product description',
+        'website_url' => 'https://example.com',
+        'category_id' => $product->category_id,
+    ], $overrides);
+}
+
+function productWithTeam(User $owner, string $status = 'approved'): array
+{
+    $member = User::factory()->create();
+    $product = Product::factory()->{$status}()->create(['user_id' => $owner->id]);
+    $product->members()->attach($member->id, ['role' => 'Designer']);
+
+    return [$product, $member];
+}
+
+test('moderator update leaves members untouched', function () {
+    $owner = userWithRole('user');
+    [$product, $member] = productWithTeam($owner);
+    $other = User::factory()->create();
+
+    $moderator = userWithRole('moderator');
+    $this->actingAs($moderator)->put(route('products.update', $product), integrityPayload($product, [
+        'members_present' => 1,
+        'members' => [['user_id' => $other->id, 'role' => 'Hacker']],
+    ]))->assertRedirect()->assertSessionHasNoErrors();
+
+    expect($product->members()->pluck('users.id')->all())->toBe([$member->id]);
+
+    $this->actingAs($moderator)->put(route('products.update', $product->fresh()), integrityPayload($product, [
+        'members_present' => 1,
+    ]))->assertRedirect();
+    expect($product->members()->count())->toBe(1);
+});
+
+test('admin can change members on another users product', function () {
+    $owner = userWithRole('user');
+    [$product] = productWithTeam($owner);
+    $other = User::factory()->create();
+
+    $this->actingAs(userWithRole('admin'))->put(route('products.update', $product), integrityPayload($product, [
+        'members_present' => 1,
+        'members' => [['user_id' => $other->id, 'role' => 'Lead']],
+    ]))->assertRedirect()->assertSessionHasNoErrors();
+
+    expect($product->members()->pluck('users.id')->all())->toBe([$other->id]);
+});
+
+test('owner can change members', function () {
+    $owner = userWithRole('user');
+    [$product] = productWithTeam($owner, 'pending');
+    $other = User::factory()->create();
+
+    $this->actingAs($owner)->put(route('products.update', $product), integrityPayload($product, [
+        'members_present' => 1,
+        'members' => [['user_id' => $other->id, 'role' => 'Lead']],
+    ]))->assertRedirect();
+
+    expect($product->members()->pluck('users.id')->all())->toBe([$other->id]);
+});
+
+test('moderator sees a read-only team on the edit form, owner sees inputs', function () {
+    $owner = userWithRole('user');
+    [$product, $member] = productWithTeam($owner);
+
+    $this->actingAs(userWithRole('moderator'))->get(route('products.edit', $product))
+        ->assertOk()->assertSee($member->name)
+        ->assertDontSee('members_present')
+        ->assertDontSee('Search users by name');
+
+    $this->actingAs($owner)->get(route('products.edit', $product))
+        ->assertOk()->assertSee('members_present')->assertSee('Search users by name');
+});
+
+test('update without sentinels keeps existing plans and members', function () {
+    $owner = userWithRole('user');
+    [$product, $member] = productWithTeam($owner, 'pending');
+    $product->pricingPlans()->create(['name' => 'Keep', 'price' => 1, 'period' => 'monthly', 'position' => 0]);
+
+    $this->actingAs($owner)->put(route('products.update', $product), integrityPayload($product))->assertRedirect();
+
+    expect($product->pricingPlans()->pluck('name')->all())->toBe(['Keep']);
+    expect($product->members()->pluck('users.id')->all())->toBe([$member->id]);
+});
+
+test('update with sentinels and no rows clears plans and members', function () {
+    $owner = userWithRole('user');
+    [$product] = productWithTeam($owner, 'pending');
+    $product->pricingPlans()->create(['name' => 'Gone', 'price' => 1, 'period' => 'monthly', 'position' => 0]);
+
+    $this->actingAs($owner)->put(route('products.update', $product), integrityPayload($product, [
+        'plans_present' => 1,
+        'members_present' => 1,
+    ]))->assertRedirect();
+
+    expect($product->pricingPlans()->count())->toBe(0);
+    expect($product->members()->count())->toBe(0);
+});
+
+test('forms post the sentinels and honour them after a validation error', function () {
+    $owner = userWithRole('user');
+    $product = Product::factory()->pending()->create(['user_id' => $owner->id]);
+    $product->pricingPlans()->create(['name' => 'Stored', 'price' => 1, 'period' => 'monthly', 'position' => 0]);
+
+    $this->actingAs($owner)->get(route('products.edit', $product))
+        ->assertOk()->assertSee('plans_present')->assertSee('Stored');
+
+    // User removed every plan, then validation failed: the plan must not come back.
+    $this->actingAs($owner)->from(route('products.edit', $product))
+        ->put(route('products.update', $product), integrityPayload($product, ['title' => '', 'plans_present' => 1]));
+    $this->actingAs($owner)->get(route('products.edit', $product))->assertOk();
+    $html = $this->actingAs($owner)->followingRedirects()->from(route('products.edit', $product))
+        ->put(route('products.update', $product), integrityPayload($product, ['title' => '', 'plans_present' => 1]))
+        ->getContent();
+    expect($html)->not->toContain('Stored');
+});
+
+test('restored members show their names after a validation error', function () {
+    $owner = userWithRole('user');
+    $product = Product::factory()->pending()->create(['user_id' => $owner->id]);
+    $m = User::factory()->create(['name' => 'Zed Zephyr']);
+
+    $html = $this->actingAs($owner)->followingRedirects()->from(route('products.edit', $product))
+        ->put(route('products.update', $product), integrityPayload($product, [
+            'title' => '',
+            'members_present' => 1,
+            'members' => [['user_id' => $m->id, 'role' => 'Dev', 'name' => 'Zed Zephyr']],
+        ]))->getContent();
+
+    expect($html)->toContain('Zed Zephyr')->not->toContain('User #')->toContain('members[${i}][name]');
+});
+
+test('failure part-way through update rolls back and removes new files', function () {
+    Storage::fake('public');
+    $owner = userWithRole('user');
+    $product = Product::factory()->pending()->create(['user_id' => $owner->id, 'thumbnail' => 'thumbnails/old.jpg']);
+    Storage::disk('public')->put('thumbnails/old.jpg', 'x');
+    $product->pricingPlans()->create(['name' => 'Old', 'price' => 1, 'period' => 'monthly', 'position' => 0]);
+    $shot = $product->screenshots()->create(['path' => 'screenshots/old.jpg']);
+    Storage::disk('public')->put('screenshots/old.jpg', 'x');
+
+    ProductScreenshot::creating(fn () => throw new RuntimeException('boom'));
+
+    $this->withoutExceptionHandling();
+    expect(fn () => $this->actingAs($owner)->put(route('products.update', $product), integrityPayload($product, [
+        'title' => 'Changed',
+        'plans_present' => 1,
+        'plans' => [['name' => 'New', 'price' => '3', 'period' => 'yearly']],
+        'remove_screenshots' => [$shot->id],
+        'thumbnail' => UploadedFile::fake()->image('t.jpg'),
+        'screenshots' => [UploadedFile::fake()->image('s.jpg')],
+    ])))->toThrow(RuntimeException::class);
+
+    expect($product->fresh()->title)->not->toBe('Changed');
+    expect($product->pricingPlans()->pluck('name')->all())->toBe(['Old']);
+    expect($product->screenshots()->count())->toBe(1);
+    expect(Storage::disk('public')->allFiles())->toEqualCanonicalizing(['thumbnails/old.jpg', 'screenshots/old.jpg']);
+});
+
+test('failure during store leaves no product and no files', function () {
+    Storage::fake('public');
+    $owner = userWithRole('user');
+    ProductScreenshot::creating(fn () => throw new RuntimeException('boom'));
+
+    $this->withoutExceptionHandling();
+    expect(fn () => $this->actingAs($owner)->post(route('products.store'), [
+        'title' => 'X', 'description' => 'D', 'website_url' => 'https://example.com',
+        'category_id' => makeCategory()->id,
+        'thumbnail' => UploadedFile::fake()->image('t.jpg'),
+        'screenshots' => [UploadedFile::fake()->image('s.jpg')],
+    ]))->toThrow(RuntimeException::class);
+
+    expect(Product::count())->toBe(0);
+    expect(Storage::disk('public')->allFiles())->toBe([]);
+});
+
+test('price allows at most two decimals', function () {
+    $owner = userWithRole('user');
+    $product = Product::factory()->pending()->create(['user_id' => $owner->id]);
+
+    $this->actingAs($owner)->put(route('products.update', $product), integrityPayload($product, [
+        'plans_present' => 1,
+        'plans' => [['name' => 'P', 'price' => '9.999', 'period' => 'monthly']],
+    ]))->assertSessionHasErrors('plans.0.price');
+});
+
+test('admin can change badges on an approved product', function () {
+    $product = Product::factory()->approved()->create(['eu_hosted' => false]);
+
+    $this->actingAs(userWithRole('admin'))->put(route('products.update', $product), integrityPayload($product, [
+        'eu_hosted' => 1,
+    ]))->assertRedirect();
+
+    expect($product->fresh()->eu_hosted)->toBeTrue();
+});
+
+test('submitter of a rejected product can change badges', function () {
+    $owner = userWithRole('user');
+    $product = Product::factory()->rejected()->create(['user_id' => $owner->id, 'eu_made' => false]);
+
+    $this->actingAs($owner)->put(route('products.update', $product), integrityPayload($product, [
+        'eu_made' => 1,
+    ]))->assertRedirect();
+
+    expect($product->fresh()->eu_made)->toBeTrue();
+});
+
+test('single screenshot gallery tile spans the full width', function () {
+    $product = Product::factory()->approved()->create();
+    $product->screenshots()->create(['path' => 'screenshots/a.jpg']);
+
+    $html = view('components.product.gallery', ['product' => $product->load('screenshots')])->render();
+    expect($html)->toContain('sm:col-span-3')->not->toContain('sm:col-span-2');
+});

tests/Feature/ProductPricingTest.php

@@ -56,6 +56,7 @@ test('update replaces the plan list', function () {
 
     $this->actingAs($user)->put(route('products.update', $product), pricingPayload([
         'category_id' => $product->category_id,
+        'plans_present' => 1,
         'plans' => [['name' => 'New', 'price' => '3', 'period' => 'one-time']],
     ]))->assertRedirect();
 

tests/Feature/ProductTeamTest.php

@@ -63,12 +63,14 @@ test('update replaces members and uses the product owner for the owner rule', fu
 
     $this->actingAs($admin)->put(route('products.update', $product), teamPayload([
         'category_id' => $product->category_id,
+        'members_present' => 1,
         'members' => [['user_id' => $owner->id, 'role' => 'Founder']],
     ]))->assertSessionHasErrors('members.0.user_id');
     expect($product->members()->pluck('users.id')->all())->toBe([$a->id]);
 
     $this->actingAs($admin)->put(route('products.update', $product), teamPayload([
         'category_id' => $product->category_id,
+        'members_present' => 1,
         'members' => [['user_id' => $admin->id, 'role' => 'Helper'], ['user_id' => $b->id, 'role' => 'New']],
     ]))->assertRedirect()->assertSessionHasNoErrors();
     expect($product->members()->pluck('users.id')->all())->toEqualCanonicalizing([$admin->id, $b->id]);