* fix(analytics): make per-photo view/download counters actually count (#895) Three stacked defects behind 'per-image stats stay 0': - photos.view_count had NO writer anywhere — the admin IMAGES table and photo viewer display it, so it was permanently 0. It now increments when the full-size photo or its preview tier is served, excluding the slideshow kiosk (migration 138 design) and follow-up video Range requests (seeks are not views). Fire-and-forget so analytics can never fail the byte-serving path. - Zip downloads (download-all, presigned download-all, download-selected) never incremented per-photo download_count — only single-photo downloads did, so zip-heavy galleries showed 0 forever. The zip routes now bump exactly the photos that went into the archive (the prebuilt-zip path mirrors the archive builders' category filter). - Every admin surface used a different definition of 'downloads', which is the reporter's 46 vs 45 vs 0: event details counted only action='download' (no zips at all), the dashboard counted download+download_all but silently EXCLUDED download_selected and download_all_presigned. All queries now share one action set: download, download_all, download_all_presigned, download_selected. New photoEngagementCounters suite pins all of it (7 tests). * fix(analytics): count views via an explicit lightbox beacon (#895 review round) External review flagged that request-level view counting is wrong in both directions: the lightbox preloads prev/next neighbours (3 fetches per open) while a preloaded neighbour promoted by a swipe is never re-fetched (#505 keeps the DOM node), and enhanced/maximum galleries never hit /photo at all (bytes come from /api/secure-images). - Views now count via POST /:slug/photo/:photoId/view, fired by the lightbox exactly when a photo becomes the visible slide; the serving-route increments are removed. Covers protected galleries and the preview tier uniformly; slideshow kiosk stays excluded. - bumpEventDownloadCounts mirrors downloadZipService._build (ALL event photos) — the category filter mismatched the prebuilt zip's actual contents. (That the builder ignores per-category allow_downloads is a separate pre-existing issue.) - Zip loops count only successfully appended entries, with a pre-append storage stat: a lazy stream's async error bypassed the per-photo catch and hung the whole response — pre-existing bug, now fixed. Suite extended to 9 tests (beacon semantics, serve-does-not-count, skipped-entry exclusion). * fix(analytics): fire the view beacon from the premium lightbox too (#895 review round 2) gallery-premium events use yet-another-react-lightbox inside GalleryPremiumLayout instead of PhotoLightbox, so the layout never counted views. yarl's on.view fires on open and on every slide change — identical semantics to the PhotoLightbox beacon. Also documents the accepted prebuilt-zip approximation: _build can skip entries whose watermark step fails and still publish the archive; counting those exactly would need a persisted zip manifest. * perf(analytics): skip the per-entry zip preflight on S3 (#895 review round 3) The pre-append source check exists for LocalFs's lazy createReadStream (async error would kill the whole zip response). S3's get() awaits GetObject and rejects inside the loop's try/catch on a missing key, so a HEAD per entry was a redundant serial round trip — 500 extra HEADs on a 500-photo zip. --------- Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
Paul Nothaft
co-authored by
Paul Nothaft
Paul Nothaft
parent
5171105938
commit
78116e2e8b
@@ -69,7 +69,7 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req,
|
||||
|
||||
// Get total downloads (last 30 days) - include both single and bulk downloads
|
||||
const totalDownloads = await db('access_logs')
|
||||
.whereIn('action', ['download', 'download_all'])
|
||||
.whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected'])
|
||||
.where('timestamp', '>=', thirtyDaysAgo.toISOString())
|
||||
.count('id as count')
|
||||
.first();
|
||||
@@ -99,7 +99,7 @@ router.get('/stats', adminAuth, requirePermission('analytics.view'), async (req,
|
||||
.first();
|
||||
|
||||
const previousDownloads = await db('access_logs')
|
||||
.whereIn('action', ['download', 'download_all'])
|
||||
.whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected'])
|
||||
.where('timestamp', '>=', sixtyDaysAgo.toISOString())
|
||||
.where('timestamp', '<', thirtyDaysAgo.toISOString())
|
||||
.count('id as count')
|
||||
@@ -271,7 +271,7 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (
|
||||
// Get downloads per day - include both single and bulk downloads
|
||||
const downloadsData = await db('access_logs')
|
||||
.select(db.raw('DATE(timestamp) as date'), db.raw('COUNT(*) as count'))
|
||||
.whereIn('action', ['download', 'download_all'])
|
||||
.whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected'])
|
||||
.where('timestamp', '>=', startDateStr)
|
||||
.groupByRaw('DATE(timestamp)');
|
||||
|
||||
@@ -307,7 +307,7 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (
|
||||
.select('events.id', 'events.event_name', 'events.slug')
|
||||
.select(db.raw('COUNT(CASE WHEN action = \'view\' THEN 1 END) as views'))
|
||||
.select(db.raw('COUNT(DISTINCT CASE WHEN action = \'view\' THEN ip_address END) as uniqueVisitors'))
|
||||
.select(db.raw('COUNT(CASE WHEN action IN (\'download\', \'download_all\') THEN 1 END) as downloads'))
|
||||
.select(db.raw('COUNT(CASE WHEN action IN (\'download\', \'download_all\', \'download_all_presigned\', \'download_selected\') THEN 1 END) as downloads'))
|
||||
.join('events', 'access_logs.event_id', 'events.id')
|
||||
.where('access_logs.timestamp', '>=', startDateStr)
|
||||
.groupBy('events.id', 'events.event_name', 'events.slug')
|
||||
@@ -377,7 +377,7 @@ router.get('/analytics', adminAuth, requirePermission('analytics.view'), async (
|
||||
.first();
|
||||
|
||||
const totalDownloadsCount = await db('access_logs')
|
||||
.whereIn('action', ['download', 'download_all'])
|
||||
.whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected'])
|
||||
.where('timestamp', '>=', startDateStr)
|
||||
.count('id as count')
|
||||
.first();
|
||||
|
||||
@@ -773,9 +773,11 @@ module.exports = (router) => {
|
||||
.where('action', 'view')
|
||||
.count('* as totalViews');
|
||||
|
||||
// One row per download event: singles AND zips (#895). Must stay in
|
||||
// sync with adminDashboard's definition or the two surfaces disagree.
|
||||
const [{ totalDownloads }] = await db('access_logs')
|
||||
.where('event_id', id)
|
||||
.where('action', 'download')
|
||||
.whereIn('action', ['download', 'download_all', 'download_all_presigned', 'download_selected'])
|
||||
.count('* as totalDownloads');
|
||||
|
||||
const [{ uniqueVisitors }] = await db('access_logs')
|
||||
|
||||
@@ -1155,6 +1155,22 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken,
|
||||
});
|
||||
|
||||
// Download all photos as ZIP
|
||||
// Zip downloads count toward each contained photo's download_count (#895)
|
||||
// — previously only single-photo downloads did, so galleries whose guests
|
||||
// grab the zip showed 0 per-photo downloads forever. Used by the
|
||||
// pre-generated-zip branches only: it mirrors downloadZipService._build,
|
||||
// which zips EVERY event photo with no per-category allow_downloads
|
||||
// filter — the counter has to reflect what actually shipped. (That the
|
||||
// prebuilt zip ignores per-category download opt-outs is a separate,
|
||||
// pre-existing issue.) Known approximation: _build skips entries whose
|
||||
// WATERMARK step fails and still publishes the zip; counting those
|
||||
// would need a persisted archive manifest, which isn't worth it for
|
||||
// that tail case. Fire-and-forget at the call sites: counters must
|
||||
// never fail a download.
|
||||
async function bumpEventDownloadCounts(eventId) {
|
||||
await db('photos').where('event_id', eventId).increment('download_count', 1);
|
||||
}
|
||||
|
||||
router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, blockHiddenGallery, async (req, res) => {
|
||||
try {
|
||||
// Check if downloads are allowed for this event
|
||||
@@ -1184,6 +1200,7 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block
|
||||
user_agent: req.headers['user-agent'],
|
||||
action: 'download_all_presigned'
|
||||
}).catch(() => {});
|
||||
bumpEventDownloadCounts(req.event.id).catch(() => {});
|
||||
// Surface in the admin notification bell (#746).
|
||||
logActivity('gallery_downloaded', { scope: 'all' }, req.event.id, galleryActor(req));
|
||||
res.redirect(302, url);
|
||||
@@ -1209,6 +1226,7 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block
|
||||
user_agent: req.headers['user-agent'],
|
||||
action: 'download_all'
|
||||
}).catch(() => {});
|
||||
bumpEventDownloadCounts(req.event.id).catch(() => {});
|
||||
// Surface in the admin notification bell (#746) — only once the
|
||||
// stream actually finished; logging at pipe-time would report
|
||||
// downloads that then broke mid-transfer (codex review of #849).
|
||||
@@ -1274,6 +1292,10 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block
|
||||
// get a deterministic `_1` suffix before the entries hit the archive.
|
||||
const useOriginalBulk = await getUseOriginalFilenames();
|
||||
const bulkEntryNames = getZipEntryNames(photos, useOriginalBulk);
|
||||
// Only photos whose append succeeded count as downloaded (#895) — the
|
||||
// catch below deliberately skips missing/corrupt sources, and those
|
||||
// never make it into the archive.
|
||||
const appendedIds = [];
|
||||
for (let i = 0; i < photos.length; i += 1) {
|
||||
const photo = photos[i];
|
||||
const storageKey = resolvePhotoStorageKey(req.event, photo);
|
||||
@@ -1287,6 +1309,22 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block
|
||||
}
|
||||
|
||||
try {
|
||||
// Verify the source exists BEFORE appending — but only for local
|
||||
// sources: fs.createReadStream is lazy, so its error fires outside
|
||||
// this try/catch and the archive 'error' handler then kills the
|
||||
// whole response instead of skipping one photo (#895 review). S3's
|
||||
// get() awaits GetObject and rejects right here on a missing key,
|
||||
// so a preflight HEAD per entry would just be a redundant serial
|
||||
// round trip (500-photo zip = 500 extra HEADs).
|
||||
if (storageKey && storage.kind() === 'local') {
|
||||
const srcStat = await storage.stat(storageKey);
|
||||
if (!srcStat) {
|
||||
throw new Error(`Photo missing in storage: ${storageKey}`);
|
||||
}
|
||||
} else if (!storageKey && !fs.existsSync(resolvePhotoFilePath(req.event, photo))) {
|
||||
throw new Error('Photo file missing on disk');
|
||||
}
|
||||
|
||||
if (shouldApplyWatermark && effectiveSettings) {
|
||||
// Watermark service operates on a local path. For managed photos in
|
||||
// S3 mode, materialize a tmp local copy first.
|
||||
@@ -1306,9 +1344,9 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block
|
||||
const stream = await storage.get(storageKey);
|
||||
archive.append(stream, { name: archiveName });
|
||||
} else {
|
||||
const filePath = resolvePhotoFilePath(req.event, photo);
|
||||
archive.file(filePath, { name: archiveName });
|
||||
archive.file(resolvePhotoFilePath(req.event, photo), { name: archiveName });
|
||||
}
|
||||
appendedIds.push(photo.id);
|
||||
} catch (err) {
|
||||
logger.warn('Skipping photo in bulk download due to error', {
|
||||
slug: req.params.slug,
|
||||
@@ -1334,6 +1372,12 @@ router.get('/:slug/download-all', verifyGalleryAccess, denySlideshowToken, block
|
||||
user_agent: req.headers['user-agent'],
|
||||
action: 'download_all'
|
||||
});
|
||||
// Exactly the photos that made it into this archive (#895) — skipped
|
||||
// (missing/corrupt) sources don't count.
|
||||
if (appendedIds.length > 0) {
|
||||
db('photos').whereIn('id', appendedIds)
|
||||
.increment('download_count', 1).catch(() => {});
|
||||
}
|
||||
} catch (error) {
|
||||
errorResponse(res, error, 500, 'Failed to create download archive');
|
||||
}
|
||||
@@ -1415,11 +1459,26 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken,
|
||||
// #493: same display-name resolution as bulk download, with dedup.
|
||||
const useOriginalSelected = await getUseOriginalFilenames();
|
||||
const selectedEntryNames = getZipEntryNames(photos, useOriginalSelected);
|
||||
// Only photos whose append succeeded count as downloaded (#895).
|
||||
const appendedIds = [];
|
||||
for (let i = 0; i < photos.length; i += 1) {
|
||||
const photo = photos[i];
|
||||
const name = selectedEntryNames[i] || `photo-${photo.id}.jpg`;
|
||||
const storageKey = resolveSelectedKey(req.event, photo);
|
||||
try {
|
||||
// Same pre-append source check as download-all (#895 review),
|
||||
// local backend only: a lazy fs stream's async error would kill
|
||||
// the response instead of skipping the photo; S3's get() rejects
|
||||
// at the await below, so no redundant per-entry HEAD there.
|
||||
if (storageKey && selectedStorage.kind() === 'local') {
|
||||
const srcStat = await selectedStorage.stat(storageKey);
|
||||
if (!srcStat) {
|
||||
throw new Error(`Photo missing in storage: ${storageKey}`);
|
||||
}
|
||||
} else if (!storageKey && !fs.existsSync(resolvePhotoFilePath(req.event, photo))) {
|
||||
throw new Error('Photo file missing on disk');
|
||||
}
|
||||
|
||||
if (shouldApplyWatermark && effectiveSettings) {
|
||||
const buf = storageKey
|
||||
? await withSelectedLocalCopy(storageKey, (lp) =>
|
||||
@@ -1433,6 +1492,7 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken,
|
||||
} else {
|
||||
archive.file(resolvePhotoFilePath(req.event, photo), { name });
|
||||
}
|
||||
appendedIds.push(photo.id);
|
||||
} catch (err) {
|
||||
logger.warn('Skipping selected photo due to error', {
|
||||
slug: req.params.slug,
|
||||
@@ -1455,12 +1515,50 @@ router.post('/:slug/download-selected', verifyGalleryAccess, denySlideshowToken,
|
||||
user_agent: req.headers['user-agent'],
|
||||
action: 'download_selected'
|
||||
});
|
||||
// Exactly the photos that made it into this archive (#895) — skipped
|
||||
// (missing/corrupt) sources don't count.
|
||||
if (appendedIds.length > 0) {
|
||||
db('photos').whereIn('id', appendedIds)
|
||||
.increment('download_count', 1).catch(() => {});
|
||||
}
|
||||
} catch (error) {
|
||||
errorResponse(res, error, 500, 'Failed to download selected photos');
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
// Explicit per-photo view beacon (#895). Counting views on the image-
|
||||
// serving routes is wrong in both directions: the lightbox preloads the
|
||||
// prev/next neighbours (three fetches per open), while a preloaded
|
||||
// neighbour that becomes the current slide is never re-fetched (#505
|
||||
// keeps the DOM node alive across the swipe) — so request-level counters
|
||||
// overcount preloads AND undercount swipe-throughs. Instead the lightbox
|
||||
// pings this endpoint exactly when a photo becomes the visible slide.
|
||||
// This also covers enhanced/maximum-protection galleries, whose bytes
|
||||
// are served by /api/secure-images and never pass the routes below.
|
||||
// The slideshow kiosk is excluded (denySlideshowToken; migration 138).
|
||||
router.post('/:slug/photo/:photoId/view',
|
||||
verifyGalleryAccess,
|
||||
denySlideshowToken,
|
||||
blockHiddenGallery,
|
||||
async (req, res) => {
|
||||
try {
|
||||
const photo = await db('photos')
|
||||
.where({ id: req.params.photoId, event_id: req.event.id })
|
||||
.first('id', 'visibility');
|
||||
if (!photo) {
|
||||
return res.status(404).json({ error: 'Photo not found' });
|
||||
}
|
||||
if (photo.visibility === 'hidden' && req.accessLevel !== 'client') {
|
||||
return res.status(403).json({ error: 'Photo not available' });
|
||||
}
|
||||
await db('photos').where('id', photo.id).increment('view_count', 1);
|
||||
res.status(204).end();
|
||||
} catch (error) {
|
||||
errorResponse(res, error, 500, 'Failed to record view');
|
||||
}
|
||||
});
|
||||
|
||||
// View single photo (with watermark if enabled)
|
||||
router.get('/:slug/photo/:photoId',
|
||||
verifyGalleryAccess,
|
||||
|
||||
Reference in New Issue
Block a user