fix: address penalty catalog review findings
This commit is contained in:
@@ -26,6 +26,11 @@ type AuthenticatedRequest = { user: { id: number } };
|
|||||||
export class PenaltyController {
|
export class PenaltyController {
|
||||||
constructor(private readonly service: PenaltyService) {}
|
constructor(private readonly service: PenaltyService) {}
|
||||||
|
|
||||||
|
@Get()
|
||||||
|
getUserPenalties(@Request() request: AuthenticatedRequest) {
|
||||||
|
return this.service.getUserPenalties(request.user.id);
|
||||||
|
}
|
||||||
|
|
||||||
@Get(':teamId')
|
@Get(':teamId')
|
||||||
getTeamPenalties(
|
getTeamPenalties(
|
||||||
@Request() request: AuthenticatedRequest,
|
@Request() request: AuthenticatedRequest,
|
||||||
|
|||||||
@@ -20,6 +20,7 @@ describe('penalty catalog HTTP boundary', () => {
|
|||||||
createdAt: '2026-01-02T00:00:00.000Z',
|
createdAt: '2026-01-02T00:00:00.000Z',
|
||||||
};
|
};
|
||||||
const service = {
|
const service = {
|
||||||
|
getUserPenalties: jest.fn(() => [penalty]),
|
||||||
getTeamPenalties: jest.fn(() => [penalty]),
|
getTeamPenalties: jest.fn(() => [penalty]),
|
||||||
createPenalty: jest.fn(() => penalty),
|
createPenalty: jest.fn(() => penalty),
|
||||||
updatePenalty: jest.fn(() => penalty),
|
updatePenalty: jest.fn(() => penalty),
|
||||||
@@ -62,6 +63,15 @@ describe('penalty catalog HTTP boundary', () => {
|
|||||||
expect(service.getTeamPenalties).toHaveBeenCalledWith(42, 5);
|
expect(service.getTeamPenalties).toHaveBeenCalledWith(42, 5);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('keeps the authenticated cross-team catalog route available', async () => {
|
||||||
|
await request(app.getHttpServer()).get('/api/v1/penalty').expect(401);
|
||||||
|
await request(app.getHttpServer())
|
||||||
|
.get('/api/v1/penalty')
|
||||||
|
.set('Authorization', 'Bearer user')
|
||||||
|
.expect(200, [penalty]);
|
||||||
|
expect(service.getUserPenalties).toHaveBeenCalledWith(42);
|
||||||
|
});
|
||||||
|
|
||||||
it('validates and strips create fields before invoking the service', async () => {
|
it('validates and strips create fields before invoking the service', async () => {
|
||||||
await request(app.getHttpServer())
|
await request(app.getHttpServer())
|
||||||
.post('/api/v1/penalty')
|
.post('/api/v1/penalty')
|
||||||
|
|||||||
@@ -55,6 +55,33 @@ describe('PenaltyService catalog management', () => {
|
|||||||
expect(access.assertMember).not.toHaveBeenCalled();
|
expect(access.assertMember).not.toHaveBeenCalled();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('keeps the authenticated cross-team catalog route compatible and safely mapped', async () => {
|
||||||
|
readRepository.find.mockResolvedValue([
|
||||||
|
{
|
||||||
|
id: 8,
|
||||||
|
description: 'Zu spät',
|
||||||
|
amount: '5.50',
|
||||||
|
createdAt: new Date('2026-01-02T00:00:00.000Z'),
|
||||||
|
team: { id: 5, secret: 'hidden' },
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
|
||||||
|
await expect(service.getUserPenalties(42)).resolves.toEqual([
|
||||||
|
{
|
||||||
|
id: 8,
|
||||||
|
description: 'Zu spät',
|
||||||
|
amount: 5.5,
|
||||||
|
createdAt: new Date('2026-01-02T00:00:00.000Z'),
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
expect(readRepository.find).toHaveBeenCalledWith({
|
||||||
|
where: {
|
||||||
|
team: { players: { user: { id: 42 }, active: true } },
|
||||||
|
},
|
||||||
|
order: { description: 'ASC' },
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it('authorizes team reads, sorts them, and maps only safe fields', async () => {
|
it('authorizes team reads, sorts them, and maps only safe fields', async () => {
|
||||||
readRepository.find.mockResolvedValue([
|
readRepository.find.mockResolvedValue([
|
||||||
{
|
{
|
||||||
@@ -136,11 +163,7 @@ describe('PenaltyService catalog management', () => {
|
|||||||
writeRepository.save.mockImplementation(async (value) => value);
|
writeRepository.save.mockImplementation(async (value) => value);
|
||||||
|
|
||||||
await expect(
|
await expect(
|
||||||
service.updatePenalty(
|
service.updatePenalty(8, { description: ' Neu ', amount: 2.5 }, 42),
|
||||||
8,
|
|
||||||
{ description: ' Neu ', amount: 2.5 },
|
|
||||||
42,
|
|
||||||
),
|
|
||||||
).resolves.toMatchObject({ id: 8, description: 'Neu', amount: 2.5 });
|
).resolves.toMatchObject({ id: 8, description: 'Neu', amount: 2.5 });
|
||||||
expect(duplicateQuery.andWhere).toHaveBeenCalledWith(
|
expect(duplicateQuery.andWhere).toHaveBeenCalledWith(
|
||||||
'penalty.id != :penaltyId',
|
'penalty.id != :penaltyId',
|
||||||
@@ -194,10 +217,7 @@ describe('PenaltyService catalog management', () => {
|
|||||||
logger.info.mockRejectedValue(new Error('audit unavailable'));
|
logger.info.mockRejectedValue(new Error('audit unavailable'));
|
||||||
|
|
||||||
await expect(
|
await expect(
|
||||||
service.createPenalty(
|
service.createPenalty({ teamId: 5, description: 'Neu', amount: 2 }, 42),
|
||||||
{ teamId: 5, description: 'Neu', amount: 2 },
|
|
||||||
42,
|
|
||||||
),
|
|
||||||
).rejects.toThrow('audit unavailable');
|
).rejects.toThrow('audit unavailable');
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -25,6 +25,16 @@ export class PenaltyService {
|
|||||||
private readonly logger: LoggingService,
|
private readonly logger: LoggingService,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
|
async getUserPenalties(userId: number): Promise<PenaltyResponseDTO[]> {
|
||||||
|
const penalties = await this.repository.find({
|
||||||
|
where: {
|
||||||
|
team: { players: { user: { id: userId }, active: true } },
|
||||||
|
},
|
||||||
|
order: { description: 'ASC' },
|
||||||
|
});
|
||||||
|
return penalties.map((penalty) => this.toResponse(penalty));
|
||||||
|
}
|
||||||
|
|
||||||
async getTeamPenalties(
|
async getTeamPenalties(
|
||||||
userId: number,
|
userId: number,
|
||||||
teamId: number,
|
teamId: number,
|
||||||
@@ -133,7 +143,10 @@ export class PenaltyService {
|
|||||||
return penalty;
|
return penalty;
|
||||||
}
|
}
|
||||||
|
|
||||||
private async lockTeam(manager: EntityManager, teamId: number): Promise<Team> {
|
private async lockTeam(
|
||||||
|
manager: EntityManager,
|
||||||
|
teamId: number,
|
||||||
|
): Promise<Team> {
|
||||||
const team = await manager
|
const team = await manager
|
||||||
.getRepository(Team)
|
.getRepository(Team)
|
||||||
.createQueryBuilder('team')
|
.createQueryBuilder('team')
|
||||||
|
|||||||
@@ -17,7 +17,7 @@
|
|||||||
<input matInput type="number" min="0.01" max="10000" step="0.01" formControlName="amount" />
|
<input matInput type="number" min="0.01" max="10000" step="0.01" formControlName="amount" />
|
||||||
<span matTextSuffix>€</span>
|
<span matTextSuffix>€</span>
|
||||||
</mat-form-field>
|
</mat-form-field>
|
||||||
<button mat-flat-button type="submit" [disabled]="form.invalid || saving()">
|
<button mat-flat-button type="submit" [disabled]="form.invalid || mutationPending()">
|
||||||
@if (saving()) {
|
@if (saving()) {
|
||||||
<mat-spinner diameter="18" />
|
<mat-spinner diameter="18" />
|
||||||
} @else {
|
} @else {
|
||||||
@@ -128,7 +128,7 @@
|
|||||||
mat-button
|
mat-button
|
||||||
type="button"
|
type="button"
|
||||||
(click)="startEdit(penalty)"
|
(click)="startEdit(penalty)"
|
||||||
[disabled]="pendingPenaltyId() !== null"
|
[disabled]="mutationPending()"
|
||||||
[attr.aria-label]="penalty.description + ' bearbeiten'"
|
[attr.aria-label]="penalty.description + ' bearbeiten'"
|
||||||
>
|
>
|
||||||
<mat-icon>edit</mat-icon>Bearbeiten
|
<mat-icon>edit</mat-icon>Bearbeiten
|
||||||
@@ -137,7 +137,7 @@
|
|||||||
mat-button
|
mat-button
|
||||||
type="button"
|
type="button"
|
||||||
(click)="confirmDelete(penalty)"
|
(click)="confirmDelete(penalty)"
|
||||||
[disabled]="pendingPenaltyId() !== null"
|
[disabled]="mutationPending()"
|
||||||
[attr.aria-label]="penalty.description + ' löschen'"
|
[attr.aria-label]="penalty.description + ' löschen'"
|
||||||
>
|
>
|
||||||
<mat-icon>delete</mat-icon>Löschen
|
<mat-icon>delete</mat-icon>Löschen
|
||||||
|
|||||||
@@ -190,6 +190,70 @@ describe('Penalties', () => {
|
|||||||
expect(text()).toContain('existiert bereits');
|
expect(text()).toContain('existiert bereits');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('distinguishes a successful update from a failed authoritative reload', () => {
|
||||||
|
loadPenalties
|
||||||
|
.mockReturnValueOnce(of([first]))
|
||||||
|
.mockReturnValueOnce(throwError(() => new HttpErrorResponse({ status: 500 })));
|
||||||
|
create();
|
||||||
|
button('Bearbeiten').click();
|
||||||
|
fixture.componentInstance['editForm'].setValue({ description: 'Neu', amount: 6 });
|
||||||
|
|
||||||
|
fixture.componentInstance['savePenalty'](first);
|
||||||
|
fixture.detectChanges();
|
||||||
|
|
||||||
|
expect(fixture.componentInstance['editingPenaltyId']()).toBeNull();
|
||||||
|
expect(fixture.componentInstance['mutationError']()).toBeNull();
|
||||||
|
expect(text()).toContain('Änderung wurde gespeichert');
|
||||||
|
expect(text()).toContain('Erneut versuchen');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('clears the create form after success even when the reload fails', () => {
|
||||||
|
loadPenalties
|
||||||
|
.mockReturnValueOnce(of([first]))
|
||||||
|
.mockReturnValueOnce(throwError(() => new HttpErrorResponse({ status: 500 })));
|
||||||
|
create();
|
||||||
|
fixture.componentInstance['form'].setValue({ description: 'Handy', amount: 3 });
|
||||||
|
|
||||||
|
fixture.componentInstance['createPenalty']();
|
||||||
|
fixture.detectChanges();
|
||||||
|
|
||||||
|
expect(fixture.componentInstance['form'].getRawValue()).toEqual({
|
||||||
|
description: '',
|
||||||
|
amount: 0,
|
||||||
|
});
|
||||||
|
expect(fixture.componentInstance['mutationError']()).toBeNull();
|
||||||
|
expect(text()).toContain('Änderung wurde gespeichert');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('reports a reload problem instead of a delete failure after successful deletion', () => {
|
||||||
|
loadPenalties
|
||||||
|
.mockReturnValueOnce(of([first]))
|
||||||
|
.mockReturnValueOnce(throwError(() => new HttpErrorResponse({ status: 500 })));
|
||||||
|
create();
|
||||||
|
|
||||||
|
button('Löschen').click();
|
||||||
|
dialogClosed.next(true);
|
||||||
|
fixture.detectChanges();
|
||||||
|
|
||||||
|
expect(deletePenalty).toHaveBeenCalledWith(first.id);
|
||||||
|
expect(fixture.componentInstance['mutationError']()).toBeNull();
|
||||||
|
expect(text()).toContain('Änderung wurde gespeichert');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('prevents overlapping catalog mutations', () => {
|
||||||
|
const creation = new Subject<Penalty>();
|
||||||
|
createPenalty.mockReturnValue(creation);
|
||||||
|
create();
|
||||||
|
fixture.componentInstance['form'].setValue({ description: 'Handy', amount: 3 });
|
||||||
|
|
||||||
|
fixture.componentInstance['createPenalty']();
|
||||||
|
fixture.componentInstance['startEdit'](first);
|
||||||
|
fixture.componentInstance['confirmDelete'](first);
|
||||||
|
|
||||||
|
expect(fixture.componentInstance['editingPenaltyId']()).toBeNull();
|
||||||
|
expect(dialog.open).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
it('shows a load error, retries, and distinguishes an empty search result', () => {
|
it('shows a load error, retries, and distinguishes an empty search result', () => {
|
||||||
loadPenalties
|
loadPenalties
|
||||||
.mockReturnValueOnce(throwError(() => new HttpErrorResponse({ status: 500 })))
|
.mockReturnValueOnce(throwError(() => new HttpErrorResponse({ status: 500 })))
|
||||||
|
|||||||
@@ -12,7 +12,7 @@ import { MatIconModule } from '@angular/material/icon';
|
|||||||
import { MatInputModule } from '@angular/material/input';
|
import { MatInputModule } from '@angular/material/input';
|
||||||
import { MatProgressSpinnerModule } from '@angular/material/progress-spinner';
|
import { MatProgressSpinnerModule } from '@angular/material/progress-spinner';
|
||||||
import { RouterLink } from '@angular/router';
|
import { RouterLink } from '@angular/router';
|
||||||
import { finalize, switchMap, take } from 'rxjs';
|
import { EMPTY, Observable, catchError, finalize, switchMap, take, tap } from 'rxjs';
|
||||||
import { AuthStore } from '../../../../core/auth/auth-store';
|
import { AuthStore } from '../../../../core/auth/auth-store';
|
||||||
import { PenaltyApi } from '../../../../core/team/penalty-api';
|
import { PenaltyApi } from '../../../../core/team/penalty-api';
|
||||||
import { TeamStore } from '../../../../core/team/team-store';
|
import { TeamStore } from '../../../../core/team/team-store';
|
||||||
@@ -55,6 +55,9 @@ export class Penalties {
|
|||||||
protected readonly editingPenaltyId = signal<number | null>(null);
|
protected readonly editingPenaltyId = signal<number | null>(null);
|
||||||
protected readonly loadError = signal<string | null>(null);
|
protected readonly loadError = signal<string | null>(null);
|
||||||
protected readonly mutationError = signal<string | null>(null);
|
protected readonly mutationError = signal<string | null>(null);
|
||||||
|
protected readonly mutationPending = computed(
|
||||||
|
() => this.saving() || this.pendingPenaltyId() !== null,
|
||||||
|
);
|
||||||
protected readonly search = signal('');
|
protected readonly search = signal('');
|
||||||
protected readonly form = this.formBuilder.nonNullable.group({
|
protected readonly form = this.formBuilder.nonNullable.group({
|
||||||
description: ['', [Validators.required, Validators.maxLength(120), Validators.pattern(/\S/)]],
|
description: ['', [Validators.required, Validators.maxLength(120), Validators.pattern(/\S/)]],
|
||||||
@@ -109,20 +112,20 @@ export class Penalties {
|
|||||||
|
|
||||||
protected createPenalty(): void {
|
protected createPenalty(): void {
|
||||||
const team = this.team();
|
const team = this.team();
|
||||||
if (!this.canManage() || !team || this.form.invalid || this.saving()) return;
|
if (!this.canManage() || !team || this.form.invalid || this.mutationPending()) return;
|
||||||
this.saving.set(true);
|
this.saving.set(true);
|
||||||
this.mutationError.set(null);
|
this.mutationError.set(null);
|
||||||
this.penaltyApi
|
this.penaltyApi
|
||||||
.createPenalty({ teamId: team.id, ...this.form.getRawValue() })
|
.createPenalty({ teamId: team.id, ...this.form.getRawValue() })
|
||||||
.pipe(
|
.pipe(
|
||||||
switchMap(() => this.penaltyApi.loadPenalties(team.id)),
|
tap(() => this.form.reset({ description: '', amount: 0 })),
|
||||||
|
switchMap(() => this.reloadAfterMutation(team.id)),
|
||||||
finalize(() => this.saving.set(false)),
|
finalize(() => this.saving.set(false)),
|
||||||
takeUntilDestroyed(this.destroyRef),
|
takeUntilDestroyed(this.destroyRef),
|
||||||
)
|
)
|
||||||
.subscribe({
|
.subscribe({
|
||||||
next: (penalties) => {
|
next: (penalties) => {
|
||||||
this.penalties.set(penalties);
|
this.penalties.set(penalties);
|
||||||
this.form.reset({ description: '', amount: 0 });
|
|
||||||
},
|
},
|
||||||
error: (error: HttpErrorResponse) =>
|
error: (error: HttpErrorResponse) =>
|
||||||
this.mutationError.set(this.errorMessage(error, 'Eintrag konnte nicht angelegt werden.')),
|
this.mutationError.set(this.errorMessage(error, 'Eintrag konnte nicht angelegt werden.')),
|
||||||
@@ -130,7 +133,7 @@ export class Penalties {
|
|||||||
}
|
}
|
||||||
|
|
||||||
protected startEdit(penalty: Penalty): void {
|
protected startEdit(penalty: Penalty): void {
|
||||||
if (!this.canManage() || this.pendingPenaltyId() !== null) return;
|
if (!this.canManage() || this.mutationPending()) return;
|
||||||
this.editingPenaltyId.set(penalty.id);
|
this.editingPenaltyId.set(penalty.id);
|
||||||
this.editForm.setValue({ description: penalty.description, amount: penalty.amount });
|
this.editForm.setValue({ description: penalty.description, amount: penalty.amount });
|
||||||
this.mutationError.set(null);
|
this.mutationError.set(null);
|
||||||
@@ -149,7 +152,7 @@ export class Penalties {
|
|||||||
!teamId ||
|
!teamId ||
|
||||||
this.editingPenaltyId() !== penalty.id ||
|
this.editingPenaltyId() !== penalty.id ||
|
||||||
this.editForm.invalid ||
|
this.editForm.invalid ||
|
||||||
this.pendingPenaltyId() !== null
|
this.mutationPending()
|
||||||
) {
|
) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
@@ -158,14 +161,14 @@ export class Penalties {
|
|||||||
this.penaltyApi
|
this.penaltyApi
|
||||||
.updatePenalty(penalty.id, this.editForm.getRawValue())
|
.updatePenalty(penalty.id, this.editForm.getRawValue())
|
||||||
.pipe(
|
.pipe(
|
||||||
switchMap(() => this.penaltyApi.loadPenalties(teamId)),
|
tap(() => this.editingPenaltyId.set(null)),
|
||||||
|
switchMap(() => this.reloadAfterMutation(teamId)),
|
||||||
finalize(() => this.pendingPenaltyId.set(null)),
|
finalize(() => this.pendingPenaltyId.set(null)),
|
||||||
takeUntilDestroyed(this.destroyRef),
|
takeUntilDestroyed(this.destroyRef),
|
||||||
)
|
)
|
||||||
.subscribe({
|
.subscribe({
|
||||||
next: (penalties) => {
|
next: (penalties) => {
|
||||||
this.penalties.set(penalties);
|
this.penalties.set(penalties);
|
||||||
this.editingPenaltyId.set(null);
|
|
||||||
},
|
},
|
||||||
error: (error: HttpErrorResponse) =>
|
error: (error: HttpErrorResponse) =>
|
||||||
this.mutationError.set(
|
this.mutationError.set(
|
||||||
@@ -175,7 +178,7 @@ export class Penalties {
|
|||||||
}
|
}
|
||||||
|
|
||||||
protected confirmDelete(penalty: Penalty): void {
|
protected confirmDelete(penalty: Penalty): void {
|
||||||
if (!this.canManage() || this.pendingPenaltyId() !== null) return;
|
if (!this.canManage() || this.mutationPending()) return;
|
||||||
this.dialog
|
this.dialog
|
||||||
.open(ConfirmDialog, {
|
.open(ConfirmDialog, {
|
||||||
data: {
|
data: {
|
||||||
@@ -205,7 +208,7 @@ export class Penalties {
|
|||||||
this.penaltyApi
|
this.penaltyApi
|
||||||
.deletePenalty(penalty.id)
|
.deletePenalty(penalty.id)
|
||||||
.pipe(
|
.pipe(
|
||||||
switchMap(() => this.penaltyApi.loadPenalties(teamId)),
|
switchMap(() => this.reloadAfterMutation(teamId)),
|
||||||
finalize(() => this.pendingPenaltyId.set(null)),
|
finalize(() => this.pendingPenaltyId.set(null)),
|
||||||
takeUntilDestroyed(this.destroyRef),
|
takeUntilDestroyed(this.destroyRef),
|
||||||
)
|
)
|
||||||
@@ -219,6 +222,17 @@ export class Penalties {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private reloadAfterMutation(teamId: number): Observable<Penalty[]> {
|
||||||
|
return this.penaltyApi.loadPenalties(teamId).pipe(
|
||||||
|
catchError(() => {
|
||||||
|
this.loadError.set(
|
||||||
|
'Änderung wurde gespeichert, aber der Katalog konnte nicht aktualisiert werden.',
|
||||||
|
);
|
||||||
|
return EMPTY;
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
private load(teamId: number): void {
|
private load(teamId: number): void {
|
||||||
this.loading.set(true);
|
this.loading.set(true);
|
||||||
this.loadError.set(null);
|
this.loadError.set(null);
|
||||||
|
|||||||
Reference in New Issue
Block a user