Allow non-authenticated users to star photo (by admin option)#4082
Allow non-authenticated users to star photo (by admin option)#4082tkulev wants to merge 12 commits intoLycheeOrg:masterfrom
Conversation
Added translations for Bulgarian language.
… for photos selection. Added migration for config. - Added functionality to copy to clipboard names of selected images.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds photo "star" support: new PhotoVisibilityType enum and config migration; album/photo policies and authorization trait; request update; rights and config exposed via API; middleware update; frontend UI/events/state for starring, filtering and copying names; photos store filtering; many i18n additions; minor date-format change. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
resources/js/stores/PhotosState.ts (2)
18-22:⚠️ Potential issue | 🟡 Minor
reset()doesn't clearallPhotosorstarredPhotosCount.After
reset(),allPhotosandstarredPhotosCountbecome stale. They should be reset alongsidephotos.Suggested fix
reset() { this.photos = []; + this.allPhotos = []; this.photosTimeline = undefined; this.photoRatingFilter = null; + this.starredPhotosCount = 0; },
67-109:⚠️ Potential issue | 🟠 Major
appendPhotosdoesn't updateallPhotosorstarredPhotosCount.When additional pages of photos are loaded via
appendPhotos, theallPhotoscache andstarredPhotosCountare not updated. This means:
- Toggling the starred filter after paginating will lose the newly appended photos.
- The starred count in the header badge will be wrong.
Suggested fix (add at the end of `appendPhotos`)
appendPhotos(photos: App.Http.Resources.Models.PhotoResource[], isTimeline: boolean) { // ... existing code ... + this.allPhotos = this.photos; + this.starredPhotosCount = this.photos.filter((p) => p.is_starred).length; },
🧹 Nitpick comments (5)
lang/cz/all_settings.php (1)
17-17: Minor grammar nit in translation string."set star flag to photo" reads more naturally as "set the star flag on a photo". This likely affects all locale files that share the same English fallback.
Suggested wording
- 'photos_star_visibility' => 'Who can see and set star flag to photo', + 'photos_star_visibility' => 'Who can see and set the star flag on a photo',lang/cz/dialogs.php (1)
253-255: Array keys use camelCase, inconsistent with the rest of the file.Every other top-level key in this file uses
snake_case(e.g.,share_album,move_photo,photo_delete), but the new group uses camelCase (selectedImages→namesCopied). To stay consistent:Proposed fix
- 'selectedImages' => [ - 'namesCopied' => 'The names of the selected images have been copied!', + 'selected_images' => [ + 'names_copied' => 'The names of the selected images have been copied!', ],Ensure all references to these keys (in Blade/Vue templates) are updated accordingly.
#!/bin/bash # Find all references to the camelCase keys in the codebase rg -rn 'selectedImages' --type-add 'vue:*.vue' --type-add 'blade:*.blade.php' -g '!lang/' rg -rn 'namesCopied' --type-add 'vue:*.vue' --type-add 'blade:*.blade.php' -g '!lang/' # Also check other lang files for consistency rg -rn 'selectedImages' lang/lang/en/dialogs.php (1)
256-258: Inconsistent key naming: camelCase vs snake_case.All other keys in this file use
snake_case(e.g.,share_album,url_copied,move_album,photo_delete). These new keys use camelCase, which breaks the established convention. Consider renaming toselected_imagesandnames_copiedfor consistency across all locale files.♻️ Suggested fix
- 'selectedImages' => [ - 'namesCopied' => 'The names of the selected images have been copied!', + 'selected_images' => [ + 'names_copied' => 'The names of the selected images have been copied!', ],resources/js/components/headers/AlbumHeader.vue (1)
29-36: "Copy starred names" button semantics are unclear.The
pi-cloneicon andcan_editguard don't clearly convey that this button copies starred photo names to clipboard. Consider:
- Using
pi-copyorpi-clipboardicon instead.- Adding a
:titletooltip explaining the action.- Reviewing whether
can_editis the right permission gate (the action reads data, it doesn't edit anything).database/migrations/2026_02_11_170000_add_star_photo_visibility_config.php (1)
9-10: Missinguseimport for theDBfacade.
DBis referenced on lines 44 and 55 but is not explicitly imported. While Laravel's global alias typically resolves this, adding the import is consistent with PSR-4 and avoids surprises if aliases are ever disabled.Proposed fix
use Illuminate\Database\Migrations\Migration; +use Illuminate\Support\Facades\DB;
| /** | ||
| * Checks whether the photo can be starred by the current user. | ||
| * | ||
| * A photo is called _starred_ if the current user is allowed to star | ||
| * the photo. | ||
| * A photo is _starred_ if any of the following conditions hold | ||
| * (OR-clause) | ||
| * | ||
| * - the user is an admin | ||
| * - the user is the owner of the photo | ||
| * | ||
| * @param User|null $user | ||
| * @param Photo $photo | ||
| * | ||
| * @return bool | ||
| */ |
There was a problem hiding this comment.
Docblock is inaccurate — it omits the album-level visibility conditions.
The PHPDoc states only "the user is an admin" and "the user is the owner" as conditions, but the implementation also delegates to AlbumPolicy::canStar which grants access based on photos_star_visibility config for authenticated and anonymous users. Update the docblock to reflect the full set of conditions.
Suggested docblock update
/**
* Checks whether the photo can be starred by the current user.
*
- * A photo is called _starred_ if the current user is allowed to star
- * the photo.
- * A photo is _starred_ if any of the following conditions hold
+ * A photo can be starred if any of the following conditions hold
* (OR-clause)
*
* - the user is an admin
* - the user is the owner of the photo
+ * - the photo belongs to an album where starring is allowed per the album policy
+ * (based on photos_star_visibility config)
*
* `@param` User|null $user
* `@param` Photo $photo
*
* `@return` bool
*/- Fix for update of starred photos.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/js/stores/PhotosState.ts (1)
43-111:⚠️ Potential issue | 🟠 MajorPreserve
allPhotoswhen a filter is active.When
activeFilteris set,setPhotos/appendPhotosoverwriteallPhotoswiththis.photos, which may already be filtered. This collapses the full list and makes the filter non-reversible (e.g., pagination in starred-only view permanently drops unstarred photos). Consider updatingallPhotosfrom the unfiltered dataset and re-runningperformFilterso filtering stays view-only.💡 Suggested fix (keep full list intact when filter is active)
setPhotos(photos: App.Http.Resources.Models.PhotoResource[], isTimeline: boolean) { if (isTimeline) { this.photosTimeline = spliter( photos, (p: App.Http.Resources.Models.PhotoResource) => p.timeline?.time_date ?? "", (p: App.Http.Resources.Models.PhotoResource) => p.timeline?.format ?? "Others", ); this.photos = merge(this.photosTimeline); // Rebuild navigation links after timeline merge since photos were reordered this.rebuildNavigationLinks(); } else { // We are not using the timeline, so we can just use the photos as is. this.photos = photos; this.photosTimeline = undefined; } this.allPhotos = this.photos; + if (this.activeFilter !== null) { + this.performFilter(); + } }, /** * Append new photos to the existing collection. * Handles both timeline and non-timeline modes. * * Critical: Fixes navigation links (next_photo_id/previous_photo_id) between * the last photo of the existing collection and the first photo of the new batch. * Without this fix, navigating between photos would break at page boundaries. */ appendPhotos(photos: App.Http.Resources.Models.PhotoResource[], isTimeline: boolean) { + if (this.activeFilter !== null) { + this.allPhotos = [...this.allPhotos, ...photos]; + this.performFilter(); + return; + } if (isTimeline) { // Append new photos to timeline and re-merge const newTimelinePhotos = spliter( photos, (p: App.Http.Resources.Models.PhotoResource) => p.timeline?.time_date ?? "", (p: App.Http.Resources.Models.PhotoResource) => p.timeline?.format ?? "Others", ); // Merge existing timeline with new timeline data if (this.photosTimeline) { // Append new timeline groups or merge into existing ones for (const newGroup of newTimelinePhotos) { const existingGroup = this.photosTimeline.find((g) => g.header === newGroup.header); if (existingGroup) { existingGroup.data = [...existingGroup.data, ...newGroup.data]; } else { this.photosTimeline.push(newGroup); } } } else { this.photosTimeline = newTimelinePhotos; } this.photos = merge(this.photosTimeline); // Rebuild all navigation links after timeline merge since photos were reordered this.rebuildNavigationLinks(); } else { // Remember where the old photos end so we can fix the boundary link const oldPhotoCount = this.photos.length; // Simply append photos to the existing array this.photos = [...this.photos, ...photos]; // Fix navigation links at the boundary between old and new photos if (oldPhotoCount > 0 && oldPhotoCount < this.photos.length) { const lastOldPhoto = this.photos[oldPhotoCount - 1]; const firstNewPhoto = this.photos[oldPhotoCount]; // Connect the last old photo to the first new photo lastOldPhoto.next_photo_id = firstNewPhoto.id; // Connect the first new photo back to the last old photo firstNewPhoto.previous_photo_id = lastOldPhoto.id; } } this.allPhotos = this.photos; },
🧹 Nitpick comments (13)
lang/no/dialogs.php (1)
253-255: New translation string is in English, not Norwegian.The
names_copiedvalue is not translated to Norwegian. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files, so this is a minor nit.lang/pt/gallery.php (1)
99-100: New translation strings are in English, not Portuguese.Both
show_starredandcopy_starred_namesremain untranslated. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/pl/dialogs.php (1)
254-256: New translation string is in English, not Polish.The
names_copiedvalue is not translated to Polish. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/vi/dialogs.php (1)
253-255: New translation string is in English, not Vietnamese.The
names_copiedvalue is not translated to Vietnamese. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/zh_TW/gallery.php (1)
99-100: New translation strings are in English, not Traditional Chinese.Both
show_starredandcopy_starred_namesremain untranslated. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/de/gallery.php (1)
98-99: New translation strings are in English, not German.Both
show_starredandcopy_starred_namesremain untranslated. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/fr/gallery.php (1)
98-99: New translation strings are in English, not French.Both
show_starredandcopy_starred_namesremain untranslated. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/ja/gallery.php (1)
99-100: New translation strings are in English, not Japanese.Both
show_starredandcopy_starred_namesremain untranslated. Based on learnings, the maintainer does not prioritize strict localization consistency for new entries in language files.lang/es/gallery.php (1)
99-100: New strings are not translated to Spanish.Both
show_starredandcopy_starred_namescontain English text instead of Spanish translations. This is consistent with some other untranslated strings in this file, but flagging for awareness.Based on learnings, the maintainer does not prioritize strict localization consistency for new items in language files.
lang/ru/dialogs.php (1)
252-254: New string is not translated to Russian.The
names_copiedvalue remains in English while the rest of this file is in Russian.Based on learnings, the maintainer does not prioritize strict localization consistency for new items in language files.
lang/de/dialogs.php (1)
255-257: New string is not translated to German.The
names_copiedvalue remains in English while the rest of this file is in German.Based on learnings, the maintainer does not prioritize strict localization consistency for new items in language files.
lang/pt/dialogs.php (1)
253-255: New string is not translated to Portuguese.Based on learnings, the maintainer does not prioritize strict localization consistency for new items in language files.
lang/ja/dialogs.php (1)
253-255: New string is not translated to Japanese.Based on learnings, the maintainer does not prioritize strict localization consistency for new items in language files.
| toast.add({ | ||
| severity: "info", | ||
| summary: "Info", | ||
| detail: trans("dialogs.selected_images.names_copied") + ". " + selectedNames, | ||
| life: 3000, | ||
| }), | ||
| ) | ||
| .catch(() => | ||
| toast.add({ | ||
| severity: "error", | ||
| summary: "Error", | ||
| detail: "Failed to copy to clipboard", | ||
| life: 3000, |
There was a problem hiding this comment.
Localize toast summary/error strings.
The success/error toasts use hard-coded English ("Info", "Error", "Failed to copy to clipboard"). Please route these through i18n so the UI respects the user’s locale.
💡 Example update (add corresponding translation keys as needed)
.then(() =>
toast.add({
severity: "info",
- summary: "Info",
- detail: trans("dialogs.selected_images.names_copied") + ". " + selectedNames,
+ summary: trans("toasts.info"),
+ detail: `${trans("dialogs.selected_images.names_copied")} ${selectedNames}`,
life: 3000,
}),
)
.catch(() =>
toast.add({
severity: "error",
- summary: "Error",
- detail: "Failed to copy to clipboard",
+ summary: trans("toasts.error"),
+ detail: trans("dialogs.selected_images.copy_failed"),
life: 3000,
}),
);Fixed images links after filtering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
resources/js/stores/PhotosState.ts (1)
11-14: Consider stronger typing foractiveFilterandfilterPhotosparameter.
Record<string, unknown>allows arbitrary keys that may not exist onPhotoResource, and the cast at Line 132 bypasses compile-time checks. UsingPartial<App.Http.Resources.Models.PhotoResource>would constrain the filter to valid photo properties and eliminate the need for theRecord<string, unknown>cast inside the loop.Proposed diff
- activeFilter: null as Record<string, unknown> | null, + activeFilter: null as Partial<App.Http.Resources.Models.PhotoResource> | null,- filterPhotos(filter: Record<string, unknown> | null) { + filterPhotos(filter: Partial<App.Http.Resources.Models.PhotoResource> | null) {- if ((photo as Record<string, unknown>)[key] !== filter[key]) { + if (photo[key as keyof App.Http.Resources.Models.PhotoResource] !== filter[key as keyof App.Http.Resources.Models.PhotoResource]) {Also applies to: 120-123
| $this->can_pasword_protect = !request()->configs()->getValueAsBool('cache_enabled'); | ||
| $this->can_import_from_server = Gate::check(AlbumPolicy::CAN_IMPORT_FROM_SERVER, [AbstractAlbum::class]); | ||
| $this->can_make_purchasable = $this->canMakePurchasable($abstract_album); | ||
| $this->can_star = Gate::check(AlbumPolicy::CAN_STAR, [AbstractAlbum::class, $abstract_album]); |
There was a problem hiding this comment.
As this is set in InitConfig, this is not needed here. It does not depend of the specific of the album. Unless you plan to add this later?
| $this->can_edit = Gate::check(AlbumPolicy::CAN_EDIT, [AbstractAlbum::class, $album]); | ||
| $this->can_download = Gate::check(AlbumPolicy::CAN_DOWNLOAD, [AbstractAlbum::class, $album]); | ||
| $this->can_access_full_photo = Gate::check(AlbumPolicy::CAN_ACCESS_FULL_PHOTO, [AbstractAlbum::class, $album]); | ||
| $this->can_star = Gate::check(AlbumPolicy::CAN_STAR, [AbstractAlbum::class, $album]); |
| */ | ||
| public function canSee(?User $user, BaseSmartAlbum $smart_album): bool | ||
| { | ||
| // We do not require upload rights for all albums |
| if ($user !== null && $visibility === PhotoVisibilityType::AUTHENTICATED) { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
You are missing the condition where the album is shared with another user and PhotoVisibilityType is set to Annonymous. this should also return true.
| use Illuminate\Database\Migrations\Migration; | ||
| use Illuminate\Support\Facades\DB; | ||
|
|
||
| return new class() extends Migration { |
There was a problem hiding this comment.
use App\Models\Extensions\BaseConfigMigration;
return new class() extends BaseConfigMigration {
takes care of the boiler plate, you just to define need getConfigs()
3 participants
Summary by CodeRabbit
New Features
Permissions
Documentation