fix: address cashbox-export whole-branch review findings
- Reject dates that are not strict YYYY-MM-DD (was accepting full ISO datetimes, which silently produced empty exports instead of a 400) and reject from > to with a 400 before touching the team/DB. - Emit the cashbox_export_download and cashbox_export_subscription_update audit log events that were declared but never fired, matching the audit trail every sibling feature already has. - Restore full type checking on the pdfkit import via `import = require()` instead of an untyped require() with an eslint-disable. - Tighten a cashbox.spec.ts assertion to check the exact dialog class instead of expect.anything(), so it can't pass with the wrong dialog wired to the Export button. - Style and announce the export dialogs' error messages using this codebase's established error-message/role=alert pattern.
This commit is contained in:
@@ -6,11 +6,12 @@ import { CashboxExportSubscriptionService } from './cashbox-export-subscription.
|
|||||||
describe('CashboxExportSubscriptionService', () => {
|
describe('CashboxExportSubscriptionService', () => {
|
||||||
const repository = { findOne: jest.fn(), create: jest.fn((v) => v), save: jest.fn(async (v) => v) };
|
const repository = { findOne: jest.fn(), create: jest.fn((v) => v), save: jest.fn(async (v) => v) };
|
||||||
const access = { assertAtLeast: jest.fn() };
|
const access = { assertAtLeast: jest.fn() };
|
||||||
|
const logger = { info: jest.fn() };
|
||||||
let service: CashboxExportSubscriptionService;
|
let service: CashboxExportSubscriptionService;
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
jest.clearAllMocks();
|
jest.clearAllMocks();
|
||||||
service = new CashboxExportSubscriptionService(repository as any, access as any);
|
service = new CashboxExportSubscriptionService(repository as any, access as any, logger as any);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('returns a paused default when no subscription exists yet', async () => {
|
it('returns a paused default when no subscription exists yet', async () => {
|
||||||
@@ -78,6 +79,11 @@ describe('CashboxExportSubscriptionService', () => {
|
|||||||
nextRunDate: '2026-09-01T00:00:00.000Z',
|
nextRunDate: '2026-09-01T00:00:00.000Z',
|
||||||
}),
|
}),
|
||||||
);
|
);
|
||||||
|
expect(logger.info).toHaveBeenCalledWith({
|
||||||
|
event: 'cashbox_export_subscription_update',
|
||||||
|
details: 'teamId=5 active=true interval=monthly recipients=1',
|
||||||
|
userId: 42,
|
||||||
|
});
|
||||||
jest.useRealTimers();
|
jest.useRealTimers();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { BadRequestException, Injectable } from '@nestjs/common';
|
import { BadRequestException, Injectable } from '@nestjs/common';
|
||||||
import { InjectRepository } from '@nestjs/typeorm';
|
import { InjectRepository } from '@nestjs/typeorm';
|
||||||
|
import { LoggingService } from 'src/database/logging/logging.service';
|
||||||
import { TeamRolesEnum } from 'src/team-roles/team-roles.enum';
|
import { TeamRolesEnum } from 'src/team-roles/team-roles.enum';
|
||||||
import { TeamAccessService } from 'src/teams/team-access.service';
|
import { TeamAccessService } from 'src/teams/team-access.service';
|
||||||
import { Repository } from 'typeorm';
|
import { Repository } from 'typeorm';
|
||||||
@@ -20,6 +21,7 @@ export class CashboxExportSubscriptionService {
|
|||||||
@InjectRepository(CashboxExportSubscription)
|
@InjectRepository(CashboxExportSubscription)
|
||||||
private readonly repository: Repository<CashboxExportSubscription>,
|
private readonly repository: Repository<CashboxExportSubscription>,
|
||||||
private readonly access: TeamAccessService,
|
private readonly access: TeamAccessService,
|
||||||
|
private readonly logger: LoggingService,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
async getSubscription(
|
async getSubscription(
|
||||||
@@ -67,6 +69,11 @@ export class CashboxExportSubscriptionService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const saved = await this.repository.save(entity);
|
const saved = await this.repository.save(entity);
|
||||||
|
await this.logger.info({
|
||||||
|
event: 'cashbox_export_subscription_update',
|
||||||
|
details: `teamId=${teamId} active=${dto.active} interval=${dto.interval} recipients=${dto.recipients.length}`,
|
||||||
|
userId,
|
||||||
|
});
|
||||||
return this.toResponse(saved);
|
return this.toResponse(saved);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -67,6 +67,24 @@ describe('cashbox export HTTP boundary', () => {
|
|||||||
expect(service.exportForUser).not.toHaveBeenCalled();
|
expect(service.exportForUser).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('rejects a full ISO datetime instead of a plain YYYY-MM-DD date for from', async () => {
|
||||||
|
await request(app.getHttpServer())
|
||||||
|
.get(
|
||||||
|
'/api/v1/cashbox-export/5?from=2026-08-01T12:00:00Z&to=2026-08-31&format=csv',
|
||||||
|
)
|
||||||
|
.set('Authorization', 'Bearer user')
|
||||||
|
.expect(422);
|
||||||
|
expect(service.exportForUser).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('rejects a malformed date string for to', async () => {
|
||||||
|
await request(app.getHttpServer())
|
||||||
|
.get('/api/v1/cashbox-export/5?from=2026-08-01&to=not-a-date&format=csv')
|
||||||
|
.set('Authorization', 'Bearer user')
|
||||||
|
.expect(422);
|
||||||
|
expect(service.exportForUser).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
it('returns a CSV file with correct headers and content', async () => {
|
it('returns a CSV file with correct headers and content', async () => {
|
||||||
const csvBuffer = Buffer.from('Datum;Typ;Wer;Notiz;Betrag;Periodensaldo', 'utf-8');
|
const csvBuffer = Buffer.from('Datum;Typ;Wer;Notiz;Betrag;Periodensaldo', 'utf-8');
|
||||||
service.exportForUser.mockResolvedValue({
|
service.exportForUser.mockResolvedValue({
|
||||||
|
|||||||
@@ -1,10 +1,11 @@
|
|||||||
import { ForbiddenException, NotFoundException } from '@nestjs/common';
|
import { BadRequestException, ForbiddenException, NotFoundException } from '@nestjs/common';
|
||||||
import { TeamRolesEnum } from '../team-roles/team-roles.enum';
|
import { TeamRolesEnum } from '../team-roles/team-roles.enum';
|
||||||
import { CashboxExportService } from './cashbox-export.service';
|
import { CashboxExportService } from './cashbox-export.service';
|
||||||
|
|
||||||
describe('CashboxExportService', () => {
|
describe('CashboxExportService', () => {
|
||||||
const teamRepository = { findOne: jest.fn() };
|
const teamRepository = { findOne: jest.fn() };
|
||||||
const access = { assertAtLeast: jest.fn() };
|
const access = { assertAtLeast: jest.fn() };
|
||||||
|
const logger = { info: jest.fn() };
|
||||||
let service: CashboxExportService;
|
let service: CashboxExportService;
|
||||||
let callOrder: string[];
|
let callOrder: string[];
|
||||||
|
|
||||||
@@ -31,7 +32,7 @@ describe('CashboxExportService', () => {
|
|||||||
return team;
|
return team;
|
||||||
});
|
});
|
||||||
|
|
||||||
service = new CashboxExportService(teamRepository as any, access as any);
|
service = new CashboxExportService(teamRepository as any, access as any, logger as any);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('checks permission before loading data', async () => {
|
it('checks permission before loading data', async () => {
|
||||||
@@ -69,6 +70,15 @@ describe('CashboxExportService', () => {
|
|||||||
).rejects.toBeInstanceOf(NotFoundException);
|
).rejects.toBeInstanceOf(NotFoundException);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('rejects a range where from is after to without querying the team', async () => {
|
||||||
|
await expect(
|
||||||
|
service.exportForUser(5, 42, '2026-08-31', '2026-08-01', 'csv'),
|
||||||
|
).rejects.toBeInstanceOf(BadRequestException);
|
||||||
|
|
||||||
|
expect(access.assertAtLeast).not.toHaveBeenCalled();
|
||||||
|
expect(teamRepository.findOne).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
it('builds a CSV buffer with the correct content type and filename', async () => {
|
it('builds a CSV buffer with the correct content type and filename', async () => {
|
||||||
const result = await service.exportForUser(5, 42, '2026-08-01', '2026-08-31', 'csv');
|
const result = await service.exportForUser(5, 42, '2026-08-01', '2026-08-31', 'csv');
|
||||||
|
|
||||||
@@ -84,4 +94,14 @@ describe('CashboxExportService', () => {
|
|||||||
expect(result.filename).toBe('kassenbuch_team-a_2026-08-01_2026-08-31.pdf');
|
expect(result.filename).toBe('kassenbuch_team-a_2026-08-01_2026-08-31.pdf');
|
||||||
expect(result.buffer.subarray(0, 5).toString('utf-8')).toBe('%PDF-');
|
expect(result.buffer.subarray(0, 5).toString('utf-8')).toBe('%PDF-');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('logs a cashbox_export_download event after a successful export', async () => {
|
||||||
|
await service.exportForUser(5, 42, '2026-08-01', '2026-08-31', 'csv');
|
||||||
|
|
||||||
|
expect(logger.info).toHaveBeenCalledWith({
|
||||||
|
event: 'cashbox_export_download',
|
||||||
|
details: 'teamId=5 format=csv from=2026-08-01 to=2026-08-31',
|
||||||
|
userId: 42,
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import { Injectable, NotFoundException } from '@nestjs/common';
|
import { BadRequestException, Injectable, NotFoundException } from '@nestjs/common';
|
||||||
import { InjectRepository } from '@nestjs/typeorm';
|
import { InjectRepository } from '@nestjs/typeorm';
|
||||||
|
import { LoggingService } from 'src/database/logging/logging.service';
|
||||||
import { TeamRolesEnum } from 'src/team-roles/team-roles.enum';
|
import { TeamRolesEnum } from 'src/team-roles/team-roles.enum';
|
||||||
import { Team } from 'src/teams/entities/team.entity';
|
import { Team } from 'src/teams/entities/team.entity';
|
||||||
import { TeamAccessService } from 'src/teams/team-access.service';
|
import { TeamAccessService } from 'src/teams/team-access.service';
|
||||||
@@ -12,6 +13,7 @@ export class CashboxExportService {
|
|||||||
@InjectRepository(Team)
|
@InjectRepository(Team)
|
||||||
private readonly teamRepository: Repository<Team>,
|
private readonly teamRepository: Repository<Team>,
|
||||||
private readonly access: TeamAccessService,
|
private readonly access: TeamAccessService,
|
||||||
|
private readonly logger: LoggingService,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
async exportForUser(
|
async exportForUser(
|
||||||
@@ -21,6 +23,11 @@ export class CashboxExportService {
|
|||||||
to: string,
|
to: string,
|
||||||
format: 'csv' | 'pdf',
|
format: 'csv' | 'pdf',
|
||||||
): Promise<{ buffer: Buffer; contentType: string; filename: string }> {
|
): Promise<{ buffer: Buffer; contentType: string; filename: string }> {
|
||||||
|
if (from > to) {
|
||||||
|
throw new BadRequestException(
|
||||||
|
'Der Startzeitraum darf nicht nach dem Endzeitraum liegen.',
|
||||||
|
);
|
||||||
|
}
|
||||||
await this.access.assertAtLeast(
|
await this.access.assertAtLeast(
|
||||||
userId,
|
userId,
|
||||||
teamId,
|
teamId,
|
||||||
@@ -36,16 +43,28 @@ export class CashboxExportService {
|
|||||||
const rows = buildRows(team, from, to);
|
const rows = buildRows(team, from, to);
|
||||||
|
|
||||||
if (format === 'csv') {
|
if (format === 'csv') {
|
||||||
return {
|
const result = {
|
||||||
buffer: Buffer.from(buildCsv(rows), 'utf-8'),
|
buffer: Buffer.from(buildCsv(rows), 'utf-8'),
|
||||||
contentType: 'text/csv; charset=utf-8',
|
contentType: 'text/csv; charset=utf-8',
|
||||||
filename: `kassenbuch_${team.alias}_${from}_${to}.csv`,
|
filename: `kassenbuch_${team.alias}_${from}_${to}.csv`,
|
||||||
};
|
};
|
||||||
|
await this.logger.info({
|
||||||
|
event: 'cashbox_export_download',
|
||||||
|
details: `teamId=${teamId} format=${format} from=${from} to=${to}`,
|
||||||
|
userId,
|
||||||
|
});
|
||||||
|
return result;
|
||||||
}
|
}
|
||||||
return {
|
const result = {
|
||||||
buffer: await buildPdf(team, rows, from, to),
|
buffer: await buildPdf(team, rows, from, to),
|
||||||
contentType: 'application/pdf',
|
contentType: 'application/pdf',
|
||||||
filename: `kassenbuch_${team.alias}_${from}_${to}.pdf`,
|
filename: `kassenbuch_${team.alias}_${from}_${to}.pdf`,
|
||||||
};
|
};
|
||||||
|
await this.logger.info({
|
||||||
|
event: 'cashbox_export_download',
|
||||||
|
details: `teamId=${teamId} format=${format} from=${from} to=${to}`,
|
||||||
|
userId,
|
||||||
|
});
|
||||||
|
return result;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,6 +1,5 @@
|
|||||||
import { Team } from 'src/teams/entities/team.entity';
|
import { Team } from 'src/teams/entities/team.entity';
|
||||||
// eslint-disable-next-line @typescript-eslint/no-var-requires
|
import PDFDocument = require('pdfkit');
|
||||||
const PDFDocument = require('pdfkit');
|
|
||||||
|
|
||||||
export interface CashboxExportRow {
|
export interface CashboxExportRow {
|
||||||
date: string;
|
date: string;
|
||||||
|
|||||||
@@ -1,10 +1,10 @@
|
|||||||
import { IsDateString, IsIn } from 'class-validator';
|
import { IsIn, Matches } from 'class-validator';
|
||||||
|
|
||||||
export class CashboxExportQueryDto {
|
export class CashboxExportQueryDto {
|
||||||
@IsDateString()
|
@Matches(/^\d{4}-\d{2}-\d{2}$/)
|
||||||
from: string;
|
from: string;
|
||||||
|
|
||||||
@IsDateString()
|
@Matches(/^\d{4}-\d{2}-\d{2}$/)
|
||||||
to: string;
|
to: string;
|
||||||
|
|
||||||
@IsIn(['csv', 'pdf'])
|
@IsIn(['csv', 'pdf'])
|
||||||
|
|||||||
@@ -30,7 +30,7 @@ const rangeValid: ValidatorFn = (group): ValidationErrors | null => {
|
|||||||
<form [formGroup]="form" (ngSubmit)="download()">
|
<form [formGroup]="form" (ngSubmit)="download()">
|
||||||
<mat-dialog-content>
|
<mat-dialog-content>
|
||||||
@if (downloadError()) {
|
@if (downloadError()) {
|
||||||
<p class="error">{{ downloadError() }}</p>
|
<p class="error-message" role="alert">{{ downloadError() }}</p>
|
||||||
}
|
}
|
||||||
<mat-form-field appearance="outline">
|
<mat-form-field appearance="outline">
|
||||||
<mat-label>Von</mat-label>
|
<mat-label>Von</mat-label>
|
||||||
@@ -54,6 +54,14 @@ const rangeValid: ValidatorFn = (group): ValidationErrors | null => {
|
|||||||
</mat-dialog-actions>
|
</mat-dialog-actions>
|
||||||
</form>
|
</form>
|
||||||
`,
|
`,
|
||||||
|
styles: `
|
||||||
|
.error-message {
|
||||||
|
padding: 12px 16px;
|
||||||
|
border-radius: 12px;
|
||||||
|
color: var(--mat-sys-error);
|
||||||
|
background: var(--mat-sys-error-container);
|
||||||
|
}
|
||||||
|
`,
|
||||||
})
|
})
|
||||||
export class CashboxExportDialog {
|
export class CashboxExportDialog {
|
||||||
protected readonly dialogRef = inject(MatDialogRef<CashboxExportDialog>);
|
protected readonly dialogRef = inject(MatDialogRef<CashboxExportDialog>);
|
||||||
|
|||||||
@@ -1,10 +1,10 @@
|
|||||||
<h2 mat-dialog-title>Automatischen Versand einrichten</h2>
|
<h2 mat-dialog-title>Automatischen Versand einrichten</h2>
|
||||||
<mat-dialog-content>
|
<mat-dialog-content>
|
||||||
@if (loadError()) {
|
@if (loadError()) {
|
||||||
<p class="error">{{ loadError() }}</p>
|
<p class="error-message" role="alert">{{ loadError() }}</p>
|
||||||
}
|
}
|
||||||
@if (saveError()) {
|
@if (saveError()) {
|
||||||
<p class="error">{{ saveError() }}</p>
|
<p class="error-message" role="alert">{{ saveError() }}</p>
|
||||||
}
|
}
|
||||||
<mat-form-field appearance="outline">
|
<mat-form-field appearance="outline">
|
||||||
<mat-label>E-Mail-Adresse hinzufügen</mat-label>
|
<mat-label>E-Mail-Adresse hinzufügen</mat-label>
|
||||||
|
|||||||
@@ -0,0 +1,6 @@
|
|||||||
|
.error-message {
|
||||||
|
padding: 12px 16px;
|
||||||
|
border-radius: 12px;
|
||||||
|
color: var(--mat-sys-error);
|
||||||
|
background: var(--mat-sys-error-container);
|
||||||
|
}
|
||||||
@@ -27,6 +27,7 @@ const EMAIL_PATTERN = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
|
|||||||
MatSlideToggleModule,
|
MatSlideToggleModule,
|
||||||
],
|
],
|
||||||
templateUrl: './cashbox-export-subscription-dialog.html',
|
templateUrl: './cashbox-export-subscription-dialog.html',
|
||||||
|
styleUrl: './cashbox-export-subscription-dialog.scss',
|
||||||
})
|
})
|
||||||
export class CashboxExportSubscriptionDialog {
|
export class CashboxExportSubscriptionDialog {
|
||||||
protected readonly dialogRef = inject(MatDialogRef<CashboxExportSubscriptionDialog>);
|
protected readonly dialogRef = inject(MatDialogRef<CashboxExportSubscriptionDialog>);
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import { PenaltyApi } from '../../../core/team/penalty-api';
|
|||||||
import { TeamStore } from '../../../core/team/team-store';
|
import { TeamStore } from '../../../core/team/team-store';
|
||||||
import { TransactionsApi } from '../../../core/team/transactions-api';
|
import { TransactionsApi } from '../../../core/team/transactions-api';
|
||||||
import { Cashbox } from './cashbox';
|
import { Cashbox } from './cashbox';
|
||||||
|
import { CashboxExportDialog } from './cashbox-export-dialog/cashbox-export-dialog';
|
||||||
|
|
||||||
describe('Cashbox', () => {
|
describe('Cashbox', () => {
|
||||||
const team = {
|
const team = {
|
||||||
@@ -292,7 +293,7 @@ describe('Cashbox', () => {
|
|||||||
exportButton?.click();
|
exportButton?.click();
|
||||||
|
|
||||||
expect(dialog.open).toHaveBeenCalledWith(
|
expect(dialog.open).toHaveBeenCalledWith(
|
||||||
expect.anything(),
|
CashboxExportDialog,
|
||||||
expect.objectContaining({ data: { teamId: expect.any(Number) } }),
|
expect.objectContaining({ data: { teamId: expect.any(Number) } }),
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user