fix: Improved support for group authorization for Album delete & edit

[ildyria]

Summary by CodeRabbit

• Bug Fixes
  • Permission checks for album deletion and editing now recognize group-based permissions in addition to direct user permissions, enabling authorized actions through group membership.
  • Fixed bulk edit and delete operations to correctly process all albums when users have multiple permission grants.

[coderabbitai]

📝 Walkthrough

Walkthrough

Permission checks in AlbumPolicy.php now evaluate both direct user grants and group-inherited permissions. Single-album deletion validation was relaxed to require at least one matching permission. Bulk operations use distinct album ID selection for accurate counting across multiple permission rows.

Changes

Cohort / File(s)	Summary
Permission Logic Updates app/Policies/AlbumPolicy.php	Modified delete and edit-by-IDs permission checks to include group-based grants via APC::USER_GROUP_ID. Relaxed single-album delete validation from requiring exactly one match to at least one match. Enhanced bulk operation queries with distinct album ID selection for accurate unique album counting.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hop along, permissions bloom,
Groups now granted in the room,
Distinct albums, counted clear,
Access logic, now sincere! 🔐

🚥 Pre-merge checks | ✅ 1

✅ Passed checks (1 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

app/Policies/AlbumPolicy.php (1)

382-394: ⚠️ Potential issue | 🔴 Critical

distinct()->count() does not produce COUNT(DISTINCT base_album_id) — group-inherited grants will be wrongly denied.

The intent (per the PR/AI summary) of adding ->select(APC::BASE_ALBUM_ID) and ->distinct() is to count unique albums when a user matches via both a direct USER_ID grant and a USER_GROUP_ID grant. However, Laravel's Builder::aggregate() clones the query without the columns property and invokes the aggregate with ['*'], so compileAggregate produces SELECT COUNT(*) … — the elseif ($query->distinct && $column !== '*') branch is skipped because $column === '*'. The ->select(...) column is dropped and distinct() has no effect on the aggregate.

Concrete impact: if a user has both a direct permission row and a group-inherited permission row for the same album, count() returns 2 per album, and === $num_albums fails → access incorrectly denied. This is exactly the scenario the PR is meant to enable.

Fix by passing the column to count() (or to distinct()), which hits the correct branch in compileAggregate:

🐛 Proposed fix for canEditById (lines 382–394)

 		if (
 			AccessPermission::query()
-			->select(APC::BASE_ALBUM_ID)
 			->whereIn(APC::BASE_ALBUM_ID, $album_ids)
 			->where(fn ($query) => $query->where(APC::USER_ID, '=', $user->id)
 					->orWhereIn(APC::USER_GROUP_ID, $user->user_groups->pluck('id'))
 			)
 			->where(APC::GRANTS_EDIT, '=', true)
-			->distinct()
-			->count() === $num_albums
+			->distinct()
+			->count(APC::BASE_ALBUM_ID) === $num_albums
 		) {
 			return true;
 		}

The same issue applies to canDeleteById at lines 437–449 — please mirror the fix there (using APC::GRANTS_DELETE / count(APC::BASE_ALBUM_ID)).

Run this script to confirm the Laravel version in use and the exact aggregate / compileAggregate behavior in this vendor tree:

#!/bin/bash
# Confirm Laravel framework version
cat composer.lock 2>/dev/null | python3 -c "
import json, sys
d = json.load(sys.stdin)
for p in d.get('packages', []):
    if p['name'] in ('laravel/framework', 'illuminate/database'):
        print(p['name'], p['version'])
"

# Inspect Query\Builder::aggregate to confirm it clones-without columns
fd -t f 'Builder.php' vendor/laravel/framework/src/Illuminate/Database/Query 2>/dev/null \
  | xargs -I{} awk '/function aggregate\(/,/^    \}/' {}

# Inspect compileAggregate in the base Grammar
fd -t f 'Grammar.php' vendor/laravel/framework/src/Illuminate/Database/Query/Grammars 2>/dev/null \
  | xargs -I{} awk '/function compileAggregate\(/,/^    \}/' {}

🧹 Nitpick comments (1)

app/Policies/AlbumPolicy.php (1)

285-295: Prefer exists() over count() >= 1 for an existence check.

Behaviorally the change from === 1 to >= 1 is correct (the USER_ID/USER_GROUP_ID OR can yield multiple rows per album). But since you only care whether any matching row exists, ->exists() is clearer and lets the DB short-circuit (typically a SELECT EXISTS(...) / LIMIT 1 instead of counting all matches).

♻️ Proposed refactor

 		if (
 			AccessPermission::query()
 			->where(APC::BASE_ALBUM_ID, '=', $abstract_album->parent_id)
 			->where(fn ($query) => $query->where(APC::USER_ID, '=', $user->id)
 					->orWhereIn(APC::USER_GROUP_ID, $user->user_groups->pluck('id'))
 			)
 			->where(APC::GRANTS_DELETE, '=', true)
-			->count() >= 1
+			->exists()
 		) {
 			return true;
 		}

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 191dfcd5-0e8e-48a2-a52f-8af30a34df0c

📥 Commits

Reviewing files that changed from the base of the PR and between 2a99024 and 73c5fcb.

📒 Files selected for processing (1)

• app/Policies/AlbumPolicy.php

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.34%. Comparing base (2a99024) to head (73c5fcb).
⚠️ Report is 2 commits behind head on master.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.