Fix silent newsletter validation drops, surface skipped-sync visibility, consolidate duplicated helpers

Three fixes from the earlier review, plus cleanup:

- submissions.service.ts: a newsletter opt-in present but failing
  validation (e.g. malformed email) was silently dropped with no signal
  to the client - the rest of the submission saved, but the visitor had
  no way to know their newsletter signup didn't go through. Added
  newsletterDropped to the submit response so the frontend can tell them.

- reports.admin.service.ts: the newsletter summary tracked
  total/sent/pending/failed but silently omitted SKIPPED (stub-mode)
  signups from any bucket - every current signup showed total>0 with
  every bucket reading 0, indistinguishable from "we don't know what
  happened". Added a skipped count.

- Consolidated two things duplicated across the module: sendServerError
  (reimplemented ~11 times, three of those as identical local copies of
  the same function) into feedback.errors.ts, and formatDatetime/
  toMysqlDatetime (the same local-time formatting logic under two names,
  in csv.service.ts and events.admin.service.ts respectively) into
  feedback.dates.ts.

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

src/models/feedback/Feedback.router.ts

@@ -2,10 +2,9 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../middleware/logger';
 import {publicRouter} from './public/public.router';
 import {adminRouter} from './admin/admin.router';
+import {sendServerError} from './feedback.errors';
 
 /**
  * Router Definition
@@ -52,12 +51,6 @@ feedbackRouter.get('/', async (req: Request, res: Response) => {
 	try {
 		res.status(200).send('Nachklang e.V. Feedback API Endpoint');
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 });

src/models/feedback/admin/admin.router.ts

@@ -2,9 +2,8 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../../middleware/logger';
 import {requireAdminAuth} from '../feedback.auth';
+import {sendServerError} from '../feedback.errors';
 import {eventsAdminRouter} from './events.admin.router';
 import {songsAdminRouter} from './songs.admin.router';
 import {questionsAdminRouter} from './questions.admin.router';
@@ -81,13 +80,7 @@ adminRouter.delete('/submissions/:submissionId', async (req: Request, res: Respo
 		}
 		res.status(204).send();
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			status: 'PROCESSING_ERROR',
-			message: 'Internal Server Error. Try again later.',
-			reference: errorGuid
-		});
 	}
 });
 

src/models/feedback/admin/csv.service.ts

@@ -1,4 +1,5 @@
 import {NachklangFeedbackDB} from '../Feedback.db';
+import {formatDatetime} from '../feedback.dates';
 
 const CSV_SEPARATOR = ';';
 const UTF8_BOM = '';
@@ -23,15 +24,6 @@ export const escapeCsvField = (value: string | number | null | undefined): strin
 	return str;
 };
 
-/** mariadb returns DATETIME columns as JS Date objects - format explicitly,
- * otherwise String(date) falls back to the verbose Date.toString() format. */
-export const formatDatetime = (value: Date | string | null): string => {
-	if (!value) return '';
-	const d = value instanceof Date ? value : new Date(value);
-	const pad = (n: number) => String(n).padStart(2, '0');
-	return `${d.getFullYear()}-${pad(d.getMonth() + 1)}-${pad(d.getDate())} ${pad(d.getHours())}:${pad(d.getMinutes())}:${pad(d.getSeconds())}`;
-};
-
 const buildCsv = (headers: string[], rows: (string | number | null | undefined)[][]): string => {
 	const lines = [headers.map(escapeCsvField).join(CSV_SEPARATOR)];
 	for (const row of rows) {

src/models/feedback/admin/events.admin.router.ts

@@ -2,26 +2,15 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../../middleware/logger';
 import * as EventsAdminService from './events.admin.service';
 import * as SongsAdminService from './songs.admin.service';
+import {sendServerError} from '../feedback.errors';
 
 /**
  * Router Definition
  */
 export const eventsAdminRouter = express.Router();
 
-const sendServerError = (res: Response, e: any) => {
-	let errorGuid = Guid.create().toString();
-	logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-	res.status(500).send({
-		'status': 'PROCESSING_ERROR',
-		'message': 'Internal Server Error. Try again later.',
-		'reference': errorGuid
-	});
-};
-
 /**
  * @swagger
  * /feedback/admin/events:

src/models/feedback/admin/events.admin.service.ts

@@ -1,6 +1,7 @@
 import {NachklangFeedbackDB} from '../Feedback.db';
 import {Song} from '../feedback.interface';
 import {CreateEventInput, EventAdminDetail, EventAdminQuestionAssignment, EventAdminSummary, UpdateEventInput} from './admin.interface';
+import {formatDatetime} from '../feedback.dates';
 
 const UMLAUT_MAP: Record<string, string> = {
 	'ä': 'ae', 'ö': 'oe', 'ü': 'ue', 'ß': 'ss',
@@ -83,11 +84,6 @@ const generateUniqueSlug = async (conn: any, name: string, eventDate: string): P
 	}
 };
 
-const toMysqlDatetime = (d: Date): string => {
-	const pad = (n: number) => String(n).padStart(2, '0');
-	return `${d.getFullYear()}-${pad(d.getMonth() + 1)}-${pad(d.getDate())} ${pad(d.getHours())}:${pad(d.getMinutes())}:${pad(d.getSeconds())}`;
-};
-
 export const createEvent = async (input: CreateEventInput, createdByEmail: string): Promise<number> => {
 	let conn = await NachklangFeedbackDB.getConnection();
 	try {
@@ -101,7 +97,7 @@ export const createEvent = async (input: CreateEventInput, createdByEmail: strin
 			INSERT INTO events (slug, name, subtitle, event_date, feedback_deadline, intro_text, created_by_email)
 			VALUES (?,?,?,?,?,?,?) RETURNING event_id`;
 		const res = await conn.query(query, [
-			slug, input.name, input.subtitle || null, input.eventDate, toMysqlDatetime(deadline),
+			slug, input.name, input.subtitle || null, input.eventDate, formatDatetime(deadline),
 			input.introText || null, createdByEmail
 		]);
 		await conn.commit();

src/models/feedback/admin/questions.admin.router.ts

@@ -2,25 +2,14 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../../middleware/logger';
 import * as QuestionsAdminService from './questions.admin.service';
+import {sendServerError} from '../feedback.errors';
 
 /**
  * Router Definition
  */
 export const questionsAdminRouter = express.Router();
 
-const sendServerError = (res: Response, e: any) => {
-	let errorGuid = Guid.create().toString();
-	logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-	res.status(500).send({
-		'status': 'PROCESSING_ERROR',
-		'message': 'Internal Server Error. Try again later.',
-		'reference': errorGuid
-	});
-};
-
 /**
  * @swagger
  * /feedback/admin/questions:

src/models/feedback/admin/reports.admin.interface.ts

@@ -58,6 +58,7 @@ export interface EventReport {
 		sent: number;
 		pending: number;
 		failed: number;
+		skipped: number;
 	};
 }
 

src/models/feedback/admin/reports.admin.router.ts

@@ -2,27 +2,16 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../../middleware/logger';
 import * as ReportsAdminService from './reports.admin.service';
 import * as CsvService from './csv.service';
 import * as EventsAdminService from './events.admin.service';
+import {sendServerError} from '../feedback.errors';
 
 /**
  * Router Definition
  */
 export const reportsAdminRouter = express.Router();
 
-const sendServerError = (res: Response, e: any) => {
-	let errorGuid = Guid.create().toString();
-	logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-	res.status(500).send({
-		'status': 'PROCESSING_ERROR',
-		'message': 'Internal Server Error. Try again later.',
-		'reference': errorGuid
-	});
-};
-
 /**
  * @swagger
  * /feedback/admin/events/{eventId}/report:

src/models/feedback/admin/reports.admin.service.ts

@@ -16,7 +16,7 @@ export const aggregateReport = (
 	submissionStats: {totalSubmissions: number; firstSubmissionAt: string | null; lastSubmissionAt: string | null},
 	answerRows: AnswerRow[],
 	guestBookCount: number,
-	newsletterCounts: {total: number; sent: number; pending: number; failed: number}
+	newsletterCounts: {total: number; sent: number; pending: number; failed: number; skipped: number}
 ): EventReport => {
 	const groupKey = (row: AnswerRow) => `${row.questionId ?? 'null'}::${row.questionLabel}`;
 
@@ -137,13 +137,14 @@ export const getReport = async (eventId: number): Promise<EventReport | null> =>
 			`SELECT sync_status, COUNT(*) as cnt FROM newsletter_signups WHERE event_id = ? GROUP BY sync_status`,
 			[eventId]
 		);
-		const newsletterCounts = {total: 0, sent: 0, pending: 0, failed: 0};
+		const newsletterCounts = {total: 0, sent: 0, pending: 0, failed: 0, skipped: 0};
 		for (const row of newsletterRows) {
 			const cnt = Number(row.cnt);
 			newsletterCounts.total += cnt;
 			if (row.sync_status === 'SENT') newsletterCounts.sent = cnt;
 			else if (row.sync_status === 'PENDING') newsletterCounts.pending = cnt;
 			else if (row.sync_status === 'FAILED') newsletterCounts.failed = cnt;
+			else if (row.sync_status === 'SKIPPED') newsletterCounts.skipped = cnt;
 		}
 
 		return aggregateReport(

src/models/feedback/admin/songs.admin.router.ts

@@ -2,9 +2,8 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../../middleware/logger';
 import * as SongsAdminService from './songs.admin.service';
+import {sendServerError} from '../feedback.errors';
 
 /**
  * Router Definition
@@ -80,13 +79,7 @@ songsAdminRouter.put('/:songId', async (req: Request, res: Response) => {
 		}
 		res.status(200).send({status: 'OK'});
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 });
 
@@ -99,12 +92,6 @@ songsAdminRouter.delete('/:songId', async (req: Request, res: Response) => {
 		}
 		res.status(204).send();
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 });

src/models/feedback/feedback.auth.ts

@@ -1,7 +1,6 @@
 import express from 'express';
-import {Guid} from 'guid-typescript';
-import logger from '../../middleware/logger';
 import * as UserService from '../calendar/users/users.service';
+import {sendServerError} from './feedback.errors';
 
 /**
  * This file is the ONLY place in the feedback module that knows how admin
@@ -74,12 +73,6 @@ export const requireAdminAuth: express.RequestHandler = async (req, res, next) =
 		res.locals.admin = identity;
 		next();
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 };

src/models/feedback/feedback.dates.ts

@@ -0,0 +1,13 @@
+/**
+ * Formats a Date using its local getters (not toISOString/UTC), so the
+ * wall-clock time the server is running in is what gets stored/displayed -
+ * never silently shifted by a UTC conversion. Used both for MySQL DATETIME
+ * literals (events.admin.service.ts) and CSV export (csv.service.ts): same
+ * requirement, same format, in either context.
+ */
+export const formatDatetime = (value: Date | string | null): string => {
+	if (!value) return '';
+	const d = value instanceof Date ? value : new Date(value);
+	const pad = (n: number) => String(n).padStart(2, '0');
+	return `${d.getFullYear()}-${pad(d.getMonth() + 1)}-${pad(d.getDate())} ${pad(d.getHours())}:${pad(d.getMinutes())}:${pad(d.getSeconds())}`;
+};

src/models/feedback/feedback.errors.ts

@@ -0,0 +1,18 @@
+import {Response} from 'express';
+import {Guid} from 'guid-typescript';
+import logger from '../../middleware/logger';
+
+/**
+ * The feedback module's standard catch-block response: log with a
+ * reference guid, never leak the real error message to the client. Every
+ * router in this module follows this exact convention (see CLAUDE.md).
+ */
+export const sendServerError = (res: Response, e: any): void => {
+	const errorGuid = Guid.create().toString();
+	logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
+	res.status(500).send({
+		status: 'PROCESSING_ERROR',
+		message: 'Internal Server Error. Try again later.',
+		reference: errorGuid
+	});
+};

src/models/feedback/public/public.router.ts

@@ -2,11 +2,11 @@
  * Required External Modules and Interfaces
  */
 import express, {Request, Response} from 'express';
-import {Guid} from 'guid-typescript';
 import logger from '../../../middleware/logger';
 import {getEligibleEvents, getEventConfigBySlug} from './events.public.service';
 import {submitFeedback} from './submissions.service';
 import {hashIp, isRateLimited, recordSubmission} from '../feedback.ratelimit';
+import {sendServerError} from '../feedback.errors';
 
 /**
  * Router Definition
@@ -51,13 +51,7 @@ publicRouter.get('/events', async (req: Request, res: Response) => {
 		const events = await getEligibleEvents();
 		res.status(200).send(events);
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 });
 
@@ -106,13 +100,7 @@ publicRouter.get('/events/:slug', async (req: Request, res: Response) => {
 		}
 		res.status(200).send(result.event);
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 });
 
@@ -166,7 +154,7 @@ publicRouter.post('/events/:slug/submissions', async (req: Request, res: Respons
 		// nothing, stay silent about it having failed.
 		if (isHoneypotTriggered(body)) {
 			logger.info('Feedback honeypot triggered', {slug: req.params.slug});
-			res.status(201).send({submissionId: -1});
+			res.status(201).send({submissionId: -1, newsletterDropped: false});
 			return;
 		}
 
@@ -196,16 +184,10 @@ publicRouter.post('/events/:slug/submissions', async (req: Request, res: Respons
 				res.status(400).send({status: 'EMPTY_SUBMISSION'});
 				return;
 			case 'OK':
-				res.status(201).send({submissionId: result.submissionId});
+				res.status(201).send({submissionId: result.submissionId, newsletterDropped: result.newsletterDropped});
 				return;
 		}
 	} catch (e: any) {
-		let errorGuid = Guid.create().toString();
+		sendServerError(res, e);
-		logger.error('Error handling a request: ' + e.message, {reference: errorGuid});
-		res.status(500).send({
-			'status': 'PROCESSING_ERROR',
-			'message': 'Internal Server Error. Try again later.',
-			'reference': errorGuid
-		});
 	}
 });

src/models/feedback/public/submission.interface.ts

@@ -62,6 +62,9 @@
  *         submissionId:
  *           type: integer
  *           example: 91
+ *         newsletterDropped:
+ *           type: boolean
+ *           description: True if the newsletter opt-in was present but failed validation (e.g. a malformed email) - the rest of the submission still saved.
  */
 
 export interface RatingInput {

src/models/feedback/public/submissions.service.ts

@@ -137,7 +137,7 @@ export const validateNewsletter = (input?: NewsletterInput): ValidatedNewsletter
 };
 
 export type SubmitResult =
-	| { status: 'OK'; submissionId: number }
+	| { status: 'OK'; submissionId: number; newsletterDropped: boolean }
 	| { status: 'NOT_FOUND' }
 	| { status: 'CLOSED' }
 	| { status: 'EMPTY' };
@@ -160,6 +160,12 @@ export const submitFeedback = async (slug: string, body: SubmissionRequestBody,
 	const answerRows = validateAnswers(body.answers || [], questionsById, songTitleById);
 	const guestBook = validateGuestBook(body.guestBook);
 	const newsletter = validateNewsletter(body.newsletter);
+	// body.newsletter is only sent at all when the visitor had the opt-in
+	// checkbox on (see FeedbackForm.tsx), so a present-but-invalid block
+	// (e.g. a mistyped email) is distinguishable from "didn't opt in" - the
+	// rest of the submission still saves, but the client can tell the
+	// visitor their newsletter signup specifically didn't go through.
+	const newsletterDropped = !!body.newsletter && !newsletter;
 
 	if (answerRows.length === 0 && !guestBook && !newsletter) {
 		return {status: 'EMPTY'};
@@ -213,7 +219,7 @@ export const submitFeedback = async (slug: string, body: SubmissionRequestBody,
 			});
 		}
 
-		return {status: 'OK', submissionId};
+		return {status: 'OK', submissionId, newsletterDropped};
 	} catch (err) {
 		await conn.rollback();
 		throw err;

test/feedback/csv.service.test.ts

@@ -1,4 +1,5 @@
-import {escapeCsvField, formatDatetime} from '../../src/models/feedback/admin/csv.service';
+import {escapeCsvField} from '../../src/models/feedback/admin/csv.service';
+import {formatDatetime} from '../../src/models/feedback/feedback.dates';
 
 describe('escapeCsvField', () => {
 	it('passes plain text through unchanged', () => {

test/feedback/reports.admin.service.test.ts

@@ -2,7 +2,7 @@ import {aggregateReport} from '../../src/models/feedback/admin/reports.admin.ser
 import {AnswerRow} from '../../src/models/feedback/admin/reports.admin.interface';
 
 const eventMeta = {eventId: 1, name: 'Sommerkonzert', eventDate: '2026-08-01', feedbackDeadline: '2026-08-15T23:59:59'};
-const emptyNewsletter = {total: 0, sent: 0, pending: 0, failed: 0};
+const emptyNewsletter = {total: 0, sent: 0, pending: 0, failed: 0, skipped: 0};
 
 const row = (overrides: Partial<AnswerRow>): AnswerRow => ({
 	submissionId: 1,
@@ -90,13 +90,13 @@ describe('aggregateReport - top-level fields', () => {
 			{totalSubmissions: 42, firstSubmissionAt: '2026-08-02T10:00:00.000Z', lastSubmissionAt: '2026-08-10T18:00:00.000Z'},
 			[],
 			7,
-			{total: 10, sent: 6, pending: 2, failed: 2}
+			{total: 10, sent: 6, pending: 2, failed: 1, skipped: 1}
 		);
 		expect(report.totalSubmissions).toBe(42);
 		expect(report.firstSubmissionAt).toBe('2026-08-02T10:00:00.000Z');
 		expect(report.lastSubmissionAt).toBe('2026-08-10T18:00:00.000Z');
 		expect(report.guestBookCount).toBe(7);
-		expect(report.newsletter).toEqual({total: 10, sent: 6, pending: 2, failed: 2});
+		expect(report.newsletter).toEqual({total: 10, sent: 6, pending: 2, failed: 1, skipped: 1});
 		expect(report.event).toEqual(eventMeta);
 	});