diff --git a/backend/__tests__/utils/feedbackRatingRemoval.test.js b/backend/__tests__/utils/feedbackRatingRemoval.test.js new file mode 100644 index 00000000..56722570 --- /dev/null +++ b/backend/__tests__/utils/feedbackRatingRemoval.test.js @@ -0,0 +1,177 @@ +/** + * Unit tests for rating removal (#884). + * + * Pins the contract of `feedbackService.submitFeedback` for + * `feedback_type: 'rating'` with `rating: 0` ("clear my rating"): + * - An existing rating row is DELETED (not updated to 0 — a stored 0 + * would drag the photo's average down and still count in totals). + * - Photo stats (average_rating) are recalculated after the delete. + * - Rating 0 with no existing rating is a no-op that never inserts a row. + * - Removal is guest-scoped: clearing guest A's rating leaves guest B's + * rating (and the resulting average) intact. + * - Regular re-rating (3 → 5) still updates in place. + */ + +const path = require('path'); +const fs = require('fs'); +const os = require('os'); + +process.env.NODE_ENV = 'test'; +process.env.TEST_DATABASE_PATH = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-rating-removal-')), 'db.sqlite', +); +process.env.JWT_SECRET = process.env.JWT_SECRET || 'rating-removal-test-secret'; + +const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb'); + +const feedbackService = require('../../src/services/feedbackService'); + +const EVENT_SLUG = 'rating-removal-event'; +const GUEST_A = 'guest-a-identifier'; +const GUEST_B = 'guest-b-identifier'; + +let db; +let cleanup; +let eventId; +let photoId; + +async function rate(rating, guestIdentifier = GUEST_A) { + return feedbackService.submitFeedback(photoId, eventId, { + feedback_type: 'rating', + rating, + ip_address: '127.0.0.1', + user_agent: 'jest', + }, guestIdentifier); +} + +async function ratingRows(guestIdentifier) { + const q = db('photo_feedback').where({ + photo_id: photoId, + feedback_type: 'rating', + }); + if (guestIdentifier) q.where('guest_identifier', guestIdentifier); + return q.select('*'); +} + +async function photoAverage() { + const photo = await db('photos').where('id', photoId).first(); + return Number(photo.average_rating); +} + +beforeAll(async () => { + ({ db, cleanup } = await bootCrmDb()); + await seedMinimal(db); + const inserted = await db('events').insert({ + slug: EVENT_SLUG, + event_type: 'wedding', + event_name: 'Rating Removal Test', + event_date: '2026-06-22', + host_email: 'host@example.com', + admin_email: 'admin@example.com', + password_hash: 'x', + share_link: `/gallery/${EVENT_SLUG}/share`, + share_token: 'rating-removal-share', + expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(), + is_active: 1, + is_archived: 0, + is_draft: 0, + created_at: new Date().toISOString(), + }).returning('id'); + eventId = inserted[0]?.id ?? inserted[0]; + + const r = await db('photos').insert({ + event_id: eventId, + filename: 'photo-1.jpg', + path: 'events/rating-removal/1.jpg', + type: 'individual', + uploaded_at: new Date().toISOString(), + }).returning('id'); + photoId = r[0]?.id ?? r[0]; +}, 30000); + +afterAll(async () => { if (cleanup) await cleanup(); }); + +beforeEach(async () => { + await db('photo_feedback').where('event_id', eventId).del(); + await db('photos').where('id', photoId).update({ average_rating: 0, feedback_count: 0 }); +}); + +describe('rating removal (#884)', () => { + test('rating 0 deletes the existing rating row and resets the average', async () => { + const created = await rate(4); + expect(created.created).toBe(true); + expect(await photoAverage()).toBe(4); + + const removed = await rate(0); + expect(removed.removed).toBe(true); + expect(await ratingRows(GUEST_A)).toHaveLength(0); + expect(await photoAverage()).toBe(0); + }); + + test('rating 0 without an existing rating is a no-op (no 0-row inserted)', async () => { + const r = await rate(0); + expect(r.removed).toBe(true); + expect(await ratingRows()).toHaveLength(0); + expect(await photoAverage()).toBe(0); + }); + + test('removal is guest-scoped: guest B keeps their rating and the average', async () => { + await rate(2, GUEST_A); + await rate(4, GUEST_B); + expect(await photoAverage()).toBe(3); + + const removed = await rate(0, GUEST_A); + expect(removed.removed).toBe(true); + expect(await ratingRows(GUEST_A)).toHaveLength(0); + expect(await ratingRows(GUEST_B)).toHaveLength(1); + expect(await photoAverage()).toBe(4); + }); + + test('numeric string "0" also clears (truthy-string bypass guard)', async () => { + await rate(4); + const removed = await rate('0'); + expect(removed.removed).toBe(true); + expect(await ratingRows(GUEST_A)).toHaveLength(0); + expect(await photoAverage()).toBe(0); + }); + + test('malformed rating input never clears an existing rating', async () => { + await rate(4); + for (const bad of [undefined, null, 'bad', NaN]) { + const r = await rate(bad); + expect(r.removed).toBeFalsy(); + } + expect(await ratingRows(GUEST_A)).toHaveLength(1); + }); + + test('clearing deletes racy duplicate rating rows, not just the first', async () => { + // Simulate the check-then-insert race: two rating rows for one guest. + const row = { + photo_id: photoId, + event_id: eventId, + feedback_type: 'rating', + guest_identifier: GUEST_A, + is_approved: 1, + created_at: new Date().toISOString(), + updated_at: new Date().toISOString(), + }; + await db('photo_feedback').insert({ ...row, rating: 3 }); + await db('photo_feedback').insert({ ...row, rating: 5 }); + expect(await ratingRows(GUEST_A)).toHaveLength(2); + + const removed = await rate(0); + expect(removed.removed).toBe(true); + expect(await ratingRows(GUEST_A)).toHaveLength(0); + expect(await photoAverage()).toBe(0); + }); + + test('re-rating with a different value still updates in place', async () => { + await rate(3); + const updated = await rate(5); + expect(updated.updated).toBe(true); + const rows = await ratingRows(GUEST_A); + expect(rows).toHaveLength(1); + expect(rows[0].rating).toBe(5); + expect(await photoAverage()).toBe(5); + }); +}); diff --git a/backend/src/services/feedbackService.js b/backend/src/services/feedbackService.js index 5647945b..0502e08b 100644 --- a/backend/src/services/feedbackService.js +++ b/backend/src/services/feedbackService.js @@ -117,6 +117,12 @@ class FeedbackService { throw new Error('Invalid reaction'); } + // Rating 0 clears the guest's rating (#884). Only the explicit zero + // sentinel (0, or "0" from callers that skip the route validator's + // toInt) triggers the destructive path — malformed input (undefined, + // NaN, null) must never delete an existing rating. + const isRatingClear = feedback_type === 'rating' && (rating === 0 || rating === '0'); + // Check if similar feedback already exists (prevent duplicates). // When a per-person guest_id is present, scope the check to that guest // so two guests on the same device can independently like a photo. @@ -135,6 +141,26 @@ class FeedbackService { const existing = await duplicateQuery.first(); if (existing) { + // Rating 0 clears the guest's rating (#884) — delete rather + // than store 0, which would drag the photo's average down and + // still count in total_ratings. Delete the full guest-scoped + // set, not existing.id: the check-then-insert above can race + // into duplicate rows (same defense as the reaction path), and + // clearing must not leave a stray duplicate in the average. + if (isRatingClear) { + const clearScope = db('photo_feedback').where({ + photo_id: photoId, + event_id: eventId, + feedback_type: 'rating', + }); + if (guest_id) clearScope.where('guest_id', guest_id); + else clearScope.where('guest_identifier', guestIdentifier); + await clearScope.delete(); + + await this.updatePhotoFeedbackStats(photoId); + return { removed: true }; + } + if (feedback_type === 'rating' && rating !== existing.rating) { // Update existing rating await db('photo_feedback') @@ -199,6 +225,12 @@ class FeedbackService { } } + // Rating 0 with no existing rating: nothing to clear — never insert a + // 0-rating row (#884). + if (isRatingClear) { + return { removed: true }; + } + // Per-guest cap enforcement (#655). Only checked on ADD; toggle-off is // always allowed. NULL or 0 stored in the column means "unlimited" — // the photographer hasn't opted in to a cap for this event. diff --git a/backend/src/utils/feedbackValidation.js b/backend/src/utils/feedbackValidation.js index ac21c3ee..bfcd6163 100644 --- a/backend/src/utils/feedbackValidation.js +++ b/backend/src/utils/feedbackValidation.js @@ -137,11 +137,14 @@ const validateFeedbackSubmission = [ .isIn(['rating', 'like', 'comment', 'favorite', 'reaction']) .withMessage('Invalid feedback type'), - // Conditional validation based on feedback type + // Conditional validation based on feedback type. 0 clears the guest's + // existing rating (#884). toInt so a numeric string "0" reaches the + // service as a real 0 and hits the removal path. body('rating') .if(body('feedback_type').equals('rating')) - .isInt({ min: 1, max: 5 }) - .withMessage('Rating must be between 1 and 5'), + .isInt({ min: 0, max: 5 }) + .withMessage('Rating must be between 0 and 5') + .toInt(), // Reactions (#839): fixed curated set only — no free-form emoji. body('reaction') diff --git a/frontend/src/components/gallery/GalleryView.tsx b/frontend/src/components/gallery/GalleryView.tsx index e50d62b2..9826956d 100644 --- a/frontend/src/components/gallery/GalleryView.tsx +++ b/frontend/src/components/gallery/GalleryView.tsx @@ -783,7 +783,13 @@ export const GalleryView: React.FC = ({ slug, event }) => { photos={filteredPhotos} slug={slug} categoryId={selectedCategoryId} - onFeedbackChange={() => refetch()} + onFeedbackChange={() => { + refetch(); + // Guest-mode Rated/Liked filter membership + chip counts come + // from my-feedback, not the photo rows (#538) — refresh it too + // so e.g. a cleared rating (#884) leaves the Rated filter. + queryClient.invalidateQueries({ queryKey: ['my-feedback', slug] }); + }} heroPhotoOverride={staticHeroPhoto} feedbackEnabled={feedbackEnabled} feedbackOptions={{ @@ -1030,7 +1036,13 @@ export const GalleryView: React.FC = ({ slug, event }) => { photos={filteredPhotos} slug={slug} categoryId={selectedCategoryId} - onFeedbackChange={() => refetch()} + onFeedbackChange={() => { + refetch(); + // Guest-mode Rated/Liked filter membership + chip counts come + // from my-feedback, not the photo rows (#538) — refresh it too + // so e.g. a cleared rating (#884) leaves the Rated filter. + queryClient.invalidateQueries({ queryKey: ['my-feedback', slug] }); + }} heroPhotoOverride={staticHeroPhoto} feedbackEnabled={feedbackEnabled} feedbackOptions={{ diff --git a/frontend/src/components/gallery/PhotoLightbox.tsx b/frontend/src/components/gallery/PhotoLightbox.tsx index d079c564..01c5ff2e 100644 --- a/frontend/src/components/gallery/PhotoLightbox.tsx +++ b/frontend/src/components/gallery/PhotoLightbox.tsx @@ -118,6 +118,16 @@ export const PhotoLightbox: React.FC = ({ return () => observer.disconnect(); }, []); + // Keep the index valid when the photo list shrinks while open (see + // currentPhoto fallback below). + useEffect(() => { + if (photos.length === 0) { + onClose(); + } else if (currentIndex > photos.length - 1) { + setCurrentIndex(photos.length - 1); + } + }, [photos.length, currentIndex]); + // Save-aware download. On mobile (where Web Share + files is supported) // this opens the OS share sheet so "Save to Photos" actually lands in @@ -125,7 +135,12 @@ export const PhotoLightbox: React.FC = ({ // otherwise have to chain Files → unzip → save (#531). Desktop and // unsupported browsers fall through to a regular . const downloadPhotoMutation = useSavePhotoToDevice(); - const currentPhoto = photos[currentIndex]; + // Fall back to the last photo when the list shrinks under us: clearing + // your rating under the "Rated" feedback filter (#884) — like unliking + // under "Likes" — refetches the gallery and can drop the current photo, + // leaving currentIndex past the end. The effect below re-syncs the + // index (or closes the lightbox when nothing is left). + const currentPhoto = photos[currentIndex] ?? photos[photos.length - 1]; // Per-category download permission (#640). AND'd with the event-level // allowDownloads — disabling at either level hides the download button. // Defaults true for uncategorised photos and pre-migration-135 categories. @@ -241,7 +256,7 @@ export const PhotoLightbox: React.FC = ({ let mounted = true; (async () => { try { - if (!feedbackSettings?.feedback_enabled) return; + if (!feedbackSettings?.feedback_enabled || !currentPhoto) return; const data = await feedbackService.getPhotoFeedback(slug, String(currentPhoto.id)); if (!mounted) return; setMyLiked(!!data.my_feedback.liked); @@ -254,7 +269,7 @@ export const PhotoLightbox: React.FC = ({ } })(); return () => { mounted = false; }; - }, [slug, currentPhoto.id, feedbackSettings?.feedback_enabled]); + }, [slug, currentPhoto?.id, feedbackSettings?.feedback_enabled]); const submitLike = async () => { // Guest identity mode: ensure we have a per-person guest token. The @@ -629,6 +644,12 @@ export const PhotoLightbox: React.FC = ({ `fixed inset-0 bg-black z-50 flex items-center justify-center protected-image protection-${protectionLevel}` : 'fixed inset-0 bg-black z-50 flex items-center justify-center'; + // Empty list (last photo dropped out of the current filter): the effect + // above is about to close the lightbox — render nothing meanwhile. + if (!currentPhoto) { + return null; + } + const desktopFeedbackWidth = 416; // 26rem; keep in sync with panel width const isDesktopFeedback = showFeedback && !isSmallScreen; @@ -777,10 +798,12 @@ export const PhotoLightbox: React.FC = ({ {[1,2,3,4,5].map((i) => ( @@ -1041,7 +1064,9 @@ export const PhotoLightbox: React.FC = ({ // Per-guest cap reached (#655) on the post-identity-modal submit. if (!handleLimitError(err)) throw err; } - } else if (pendingAction?.type === 'rating' && pendingAction.rating) { + } else if (pendingAction?.type === 'rating' && typeof pendingAction.rating === 'number') { + // Explicit number check above: a pending rating of 0 (= clear + // my rating, #884) must still be submitted. await feedbackService.submitFeedback(slug, String(currentPhoto.id), { feedback_type: 'rating', rating: pendingAction.rating, @@ -1049,6 +1074,14 @@ export const PhotoLightbox: React.FC = ({ guest_email: email, }); setMyRating(pendingAction.rating); + // Refresh the visible average/count — parity with the direct + // submit paths, and required for a clear (#884) so the old + // average doesn't linger until the photo is reopened. + try { + const fresh = await feedbackService.getPhotoFeedback(slug, String(currentPhoto.id)); + setAvgRating(Number(fresh.summary?.average_rating) || 0); + setTotalRatings(Number(fresh.summary?.total_ratings) || 0); + } catch {} } // Sync gallery photo list (feedback filter chips) — parity with // the direct submit paths. diff --git a/frontend/src/components/gallery/PhotoRating.tsx b/frontend/src/components/gallery/PhotoRating.tsx index 3aad8d4d..aa5a11a0 100644 --- a/frontend/src/components/gallery/PhotoRating.tsx +++ b/frontend/src/components/gallery/PhotoRating.tsx @@ -56,6 +56,12 @@ export const PhotoRating: React.FC = ({ }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: ['photo-feedback', gallerySlug, photoId] }); + // The parent refetch fired optimistically in onMutate, i.e. possibly + // before the server applied the change — refresh gallery-level state + // again now that it has, so the average/Rated-filter membership + // reflect the accepted value (matters most for a clear, #884). + queryClient.invalidateQueries({ queryKey: ['gallery-photos', gallerySlug] }); + queryClient.invalidateQueries({ queryKey: ['my-feedback', gallerySlug] }); toast.success(t('feedback.ratingSubmitted', 'Rating submitted')); }, onError: (error: any) => { diff --git a/frontend/src/components/gallery/layouts/GalleryStoryLayout.tsx b/frontend/src/components/gallery/layouts/GalleryStoryLayout.tsx index ce0fc157..9f4f0e1e 100644 --- a/frontend/src/components/gallery/layouts/GalleryStoryLayout.tsx +++ b/frontend/src/components/gallery/layouts/GalleryStoryLayout.tsx @@ -207,15 +207,21 @@ export const GalleryStoryLayout: React.FC = ({ const handleRate = useCallback(async (rating: number) => { if (!selectedPhotoForFeedback) return; + // Clicking the star you already gave clears the rating (#884) — + // 0 tells the backend to delete it. Session-local `ratings` is the + // source of truth for "my rating" here, never the photo's average. + const current = ratings[selectedPhotoForFeedback.id] || 0; + const next = rating === current ? 0 : rating; + setRatings(prev => ({ ...prev, - [selectedPhotoForFeedback.id]: rating + [selectedPhotoForFeedback.id]: next })); try { await feedbackService.submitFeedback(slug, String(selectedPhotoForFeedback.id), { feedback_type: 'rating', - rating: rating, + rating: next, guest_name: savedIdentity?.name, guest_email: savedIdentity?.email, }); @@ -223,7 +229,7 @@ export const GalleryStoryLayout: React.FC = ({ } catch (err) { console.warn('Rating submit failed', err); } - }, [selectedPhotoForFeedback, slug, savedIdentity, onFeedbackChange]); + }, [selectedPhotoForFeedback, ratings, slug, savedIdentity, onFeedbackChange]); const handleDownloadAll = useCallback(async () => { toast.info(t('gallery.downloading', { count: photos.length })); @@ -388,7 +394,7 @@ export const GalleryStoryLayout: React.FC = ({ onClose={handleCloseFeedback} photo={selectedPhotoForFeedback} comments={selectedPhotoForFeedback ? (comments[selectedPhotoForFeedback.id] || []) : []} - rating={selectedPhotoForFeedback ? (ratings[selectedPhotoForFeedback.id] || selectedPhotoForFeedback.average_rating || 0) : 0} + rating={selectedPhotoForFeedback ? (ratings[selectedPhotoForFeedback.id] ?? (selectedPhotoForFeedback.average_rating || 0)) : 0} onAddComment={handleAddComment} onRate={handleRate} requireNameEmail={feedbackOptions?.requireNameEmail}