fix(teams): gate empty-state and add membership check to overview stats
Whole-branch review findings: 1. balanceHistory/monthlyFlow always returned 12 entries, even for a brand-new team with zero transactions, so the frontend's empty-state (gated on .length === 0) could never fire for a real "no movements yet" team. Now returns empty arrays when there are no relevant movements at all (not just none in the last 12 months, so a team with older-but-real history still gets a flat chart). Also added the same defensive `?? []` guard on players/transactions that getOverview already has, so a team with no players/relations loaded doesn't throw. 2. GET :id/overview/stats had no team-membership check -- any logged-in user (RoleEnum.user is the default role) could read any other team's financial stats by iterating ids. Injected TeamAccessService into TeamsService (already a sibling provider in TeamsModule, no module wiring needed) and call assertMember(actorUserId, teamId) as the first line of getOverviewStats, threaded from the controller via @Req(). Read access only (assertMember, not assertManager), matching who can already view the overview page. Sibling routes with the same pre-existing gap were left untouched, per review scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -40,13 +40,14 @@ describe('TeamsController', () => {
|
|||||||
).toEqual(Reflect.getMetadata('roles', TeamsController.prototype.findOne));
|
).toEqual(Reflect.getMetadata('roles', TeamsController.prototype.findOne));
|
||||||
});
|
});
|
||||||
|
|
||||||
it('delegates to service.getOverviewStats with the route id', () => {
|
it('delegates to service.getOverviewStats with the route id and the acting user id', () => {
|
||||||
const stats = { balanceHistory: [], monthlyFlow: [], topOutstanding: [] };
|
const stats = { balanceHistory: [], monthlyFlow: [], topOutstanding: [] };
|
||||||
service.getOverviewStats.mockReturnValue(stats);
|
service.getOverviewStats.mockReturnValue(stats);
|
||||||
|
const req = { user: { id: 42 } };
|
||||||
|
|
||||||
const result = controller.getOverviewStats('7');
|
const result = controller.getOverviewStats(req as any, '7');
|
||||||
|
|
||||||
expect(service.getOverviewStats).toHaveBeenCalledWith('7');
|
expect(service.getOverviewStats).toHaveBeenCalledWith('7', 42);
|
||||||
expect(result).toBe(stats);
|
expect(result).toBe(stats);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -91,8 +91,9 @@ export class TeamsController {
|
|||||||
@UseGuards(AuthGuard('jwt'), RolesGuard)
|
@UseGuards(AuthGuard('jwt'), RolesGuard)
|
||||||
@Get(':id/overview/stats')
|
@Get(':id/overview/stats')
|
||||||
@HttpCode(HttpStatus.OK)
|
@HttpCode(HttpStatus.OK)
|
||||||
getOverviewStats(@Param('id') id: string) {
|
getOverviewStats(@Req() req, @Param('id') id: string) {
|
||||||
return this.service.getOverviewStats(id);
|
const userId = req.user?.id;
|
||||||
|
return this.service.getOverviewStats(id, userId);
|
||||||
}
|
}
|
||||||
|
|
||||||
@ApiOperation({
|
@ApiOperation({
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
import { ForbiddenException } from '@nestjs/common';
|
||||||
import { TeamsService } from './teams.service';
|
import { TeamsService } from './teams.service';
|
||||||
|
|
||||||
describe('TeamsService', () => {
|
describe('TeamsService', () => {
|
||||||
@@ -8,11 +9,13 @@ describe('TeamsService', () => {
|
|||||||
const settingsRepository = {};
|
const settingsRepository = {};
|
||||||
const teamWalletTransactionRepository = {};
|
const teamWalletTransactionRepository = {};
|
||||||
const logger = { info: jest.fn(), debug: jest.fn() };
|
const logger = { info: jest.fn(), debug: jest.fn() };
|
||||||
|
const access = { assertMember: jest.fn(), assertManager: jest.fn() };
|
||||||
|
|
||||||
let service: TeamsService;
|
let service: TeamsService;
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
jest.resetAllMocks();
|
jest.resetAllMocks();
|
||||||
|
access.assertMember.mockResolvedValue(undefined);
|
||||||
service = new TeamsService(
|
service = new TeamsService(
|
||||||
repository as any,
|
repository as any,
|
||||||
playerRepository as any,
|
playerRepository as any,
|
||||||
@@ -21,6 +24,7 @@ describe('TeamsService', () => {
|
|||||||
settingsRepository as any,
|
settingsRepository as any,
|
||||||
teamWalletTransactionRepository as any,
|
teamWalletTransactionRepository as any,
|
||||||
logger as any,
|
logger as any,
|
||||||
|
access as any,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -146,7 +150,7 @@ describe('TeamsService', () => {
|
|||||||
it('anchors balanceHistory to team.balance and reconstructs earlier months backward, with carry-forward and correct sign handling', async () => {
|
it('anchors balanceHistory to team.balance and reconstructs earlier months backward, with carry-forward and correct sign handling', async () => {
|
||||||
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
||||||
|
|
||||||
const result = await service.getOverviewStats('7');
|
const result = await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
// Same relative shape as the movements alone would produce
|
// Same relative shape as the movements alone would produce
|
||||||
// (100, 150, 150, 150, 150, 110, 130, 130, 130, 130, 190, 190), but
|
// (100, 150, 150, 150, 150, 110, 130, 130, 130, 130, 190, 190), but
|
||||||
@@ -173,7 +177,7 @@ describe('TeamsService', () => {
|
|||||||
const team = buildTeam();
|
const team = buildTeam();
|
||||||
repository.findOneOrFail.mockResolvedValue(team);
|
repository.findOneOrFail.mockResolvedValue(team);
|
||||||
|
|
||||||
const result = await service.getOverviewStats('7');
|
const result = await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
// Sum of this fixture's movements is 190, but team.balance is 490 —
|
// Sum of this fixture's movements is 190, but team.balance is 490 —
|
||||||
// if the sanity check passes, the implementation is anchored to
|
// if the sanity check passes, the implementation is anchored to
|
||||||
@@ -185,7 +189,7 @@ describe('TeamsService', () => {
|
|||||||
it('groups monthlyFlow into income (payment+credit) and expense, excluding fine/levy/fee', async () => {
|
it('groups monthlyFlow into income (payment+credit) and expense, excluding fine/levy/fee', async () => {
|
||||||
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
||||||
|
|
||||||
const result = await service.getOverviewStats('7');
|
const result = await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
expect(result.monthlyFlow).toEqual([
|
expect(result.monthlyFlow).toEqual([
|
||||||
{ month: '2025-09', income: 100, expense: 0 },
|
{ month: '2025-09', income: 100, expense: 0 },
|
||||||
@@ -206,7 +210,7 @@ describe('TeamsService', () => {
|
|||||||
it('limits topOutstanding to active players with negative balance, sorted by debt descending', async () => {
|
it('limits topOutstanding to active players with negative balance, sorted by debt descending', async () => {
|
||||||
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
||||||
|
|
||||||
const result = await service.getOverviewStats('7');
|
const result = await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
expect(result.topOutstanding).toEqual([
|
expect(result.topOutstanding).toEqual([
|
||||||
{ playerId: 102, playerName: 'Bea Berg', balance: 75 },
|
{ playerId: 102, playerName: 'Bea Berg', balance: 75 },
|
||||||
@@ -226,7 +230,7 @@ describe('TeamsService', () => {
|
|||||||
}));
|
}));
|
||||||
repository.findOneOrFail.mockResolvedValue(team);
|
repository.findOneOrFail.mockResolvedValue(team);
|
||||||
|
|
||||||
const result = await service.getOverviewStats('7');
|
const result = await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
expect(result.topOutstanding).toHaveLength(10);
|
expect(result.topOutstanding).toHaveLength(10);
|
||||||
// highest debt first
|
// highest debt first
|
||||||
@@ -240,12 +244,79 @@ describe('TeamsService', () => {
|
|||||||
it('loads the team with the same relations used by getTeamTransactions', async () => {
|
it('loads the team with the same relations used by getTeamTransactions', async () => {
|
||||||
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
||||||
|
|
||||||
await service.getOverviewStats('7');
|
await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
expect(repository.findOneOrFail).toHaveBeenCalledWith({
|
expect(repository.findOneOrFail).toHaveBeenCalledWith({
|
||||||
where: { id: 7 },
|
where: { id: 7 },
|
||||||
relations: ['players', 'players.transactions', 'transactions'],
|
relations: ['players', 'players.transactions', 'transactions'],
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('checks team membership via TeamAccessService.assertMember before loading the team', async () => {
|
||||||
|
repository.findOneOrFail.mockResolvedValue(buildTeam());
|
||||||
|
|
||||||
|
await service.getOverviewStats('7', 42);
|
||||||
|
|
||||||
|
expect(access.assertMember).toHaveBeenCalledWith(42, 7);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('propagates a ForbiddenException from assertMember without loading the team', async () => {
|
||||||
|
const forbidden = new ForbiddenException('Keine Berechtigung für dieses Team.');
|
||||||
|
access.assertMember.mockRejectedValue(forbidden);
|
||||||
|
|
||||||
|
await expect(service.getOverviewStats('7', 42)).rejects.toBe(forbidden);
|
||||||
|
expect(repository.findOneOrFail).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns empty balanceHistory/monthlyFlow/topOutstanding for a brand-new team with no transactions and no players, without throwing', async () => {
|
||||||
|
repository.findOneOrFail.mockResolvedValue({
|
||||||
|
id: 9,
|
||||||
|
balance: 0,
|
||||||
|
transactions: [],
|
||||||
|
players: [],
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await service.getOverviewStats('9', 42);
|
||||||
|
|
||||||
|
expect(result).toEqual({ balanceHistory: [], monthlyFlow: [], topOutstanding: [] });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not throw when the players/transactions relations come back undefined (defensive guard, matches getOverview)', async () => {
|
||||||
|
repository.findOneOrFail.mockResolvedValue({
|
||||||
|
id: 9,
|
||||||
|
balance: 0,
|
||||||
|
transactions: undefined,
|
||||||
|
players: undefined,
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await service.getOverviewStats('9', 42);
|
||||||
|
|
||||||
|
expect(result).toEqual({ balanceHistory: [], monthlyFlow: [], topOutstanding: [] });
|
||||||
|
});
|
||||||
|
|
||||||
|
it('still returns a real (flat) chart, not an empty-state, when the last movement is outside the 12-month window', async () => {
|
||||||
|
// Only relevant movement is dated ~2 years ago -> outside the 12-month
|
||||||
|
// window (2025-09..2026-08), but movements.length > 0, so this must
|
||||||
|
// NOT trigger the empty-state — it's a real (if flat) history.
|
||||||
|
repository.findOneOrFail.mockResolvedValue({
|
||||||
|
id: 9,
|
||||||
|
balance: 250,
|
||||||
|
transactions: [
|
||||||
|
{
|
||||||
|
id: 1,
|
||||||
|
date: '2024-01-15T10:00:00.000Z',
|
||||||
|
amount: '250.00',
|
||||||
|
type: { name: 'credit' },
|
||||||
|
},
|
||||||
|
],
|
||||||
|
players: [],
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await service.getOverviewStats('9', 42);
|
||||||
|
|
||||||
|
expect(result.balanceHistory).toHaveLength(12);
|
||||||
|
expect(result.balanceHistory.every((entry) => entry.balance === 250)).toBe(true);
|
||||||
|
expect(result.monthlyFlow).toHaveLength(12);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ import { Repository } from 'typeorm';
|
|||||||
import { CreateTeamDTO } from './dto/create-team.dto';
|
import { CreateTeamDTO } from './dto/create-team.dto';
|
||||||
import { UpdatePlayerProfileDto } from './dto/update-player-profile.dto';
|
import { UpdatePlayerProfileDto } from './dto/update-player-profile.dto';
|
||||||
import { Team } from './entities/team.entity';
|
import { Team } from './entities/team.entity';
|
||||||
|
import { TeamAccessService } from './team-access.service';
|
||||||
|
|
||||||
@Injectable()
|
@Injectable()
|
||||||
export class TeamsService {
|
export class TeamsService {
|
||||||
@@ -28,6 +29,7 @@ export class TeamsService {
|
|||||||
@InjectRepository(TeamWalletTransaction)
|
@InjectRepository(TeamWalletTransaction)
|
||||||
private teamWalletTransactionRepository: Repository<TeamWalletTransaction>,
|
private teamWalletTransactionRepository: Repository<TeamWalletTransaction>,
|
||||||
private logger: LoggingService,
|
private logger: LoggingService,
|
||||||
|
private access: TeamAccessService,
|
||||||
) {}
|
) {}
|
||||||
|
|
||||||
async getOverview(teamId: string) {
|
async getOverview(teamId: string) {
|
||||||
@@ -218,30 +220,40 @@ export class TeamsService {
|
|||||||
return result;
|
return result;
|
||||||
}
|
}
|
||||||
|
|
||||||
async getOverviewStats(teamId: string | number): Promise<{
|
async getOverviewStats(
|
||||||
|
teamId: string | number,
|
||||||
|
actorUserId: string | number,
|
||||||
|
): Promise<{
|
||||||
balanceHistory: { month: string; balance: number }[];
|
balanceHistory: { month: string; balance: number }[];
|
||||||
monthlyFlow: { month: string; income: number; expense: number }[];
|
monthlyFlow: { month: string; income: number; expense: number }[];
|
||||||
topOutstanding: { playerId: number; playerName: string; balance: number }[];
|
topOutstanding: { playerId: number; playerName: string; balance: number }[];
|
||||||
}> {
|
}> {
|
||||||
const id = Number(teamId);
|
const id = Number(teamId);
|
||||||
|
|
||||||
|
// Read access: any active team member may view the KPI charts (same
|
||||||
|
// audience as the existing :id/overview page), not just managers.
|
||||||
|
await this.access.assertMember(Number(actorUserId), id);
|
||||||
|
|
||||||
const team = await this.repository.findOneOrFail({
|
const team = await this.repository.findOneOrFail({
|
||||||
where: { id },
|
where: { id },
|
||||||
relations: ['players', 'players.transactions', 'transactions'],
|
relations: ['players', 'players.transactions', 'transactions'],
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const players = team.players ?? [];
|
||||||
|
const teamTransactions = team.transactions ?? [];
|
||||||
|
|
||||||
// Only payment/credit/expense movements represent actual cash flow and
|
// Only payment/credit/expense movements represent actual cash flow and
|
||||||
// are the only types that touch team.balance (see setBalance() on
|
// are the only types that touch team.balance (see setBalance() on
|
||||||
// TeamWalletTransaction/Transaction) — fine/levy/fee raise a player's
|
// TeamWalletTransaction/Transaction) — fine/levy/fee raise a player's
|
||||||
// debt but never move money, so they are excluded entirely.
|
// debt but never move money, so they are excluded entirely.
|
||||||
const movements: { date: string; amount: number; type: string }[] = [];
|
const movements: { date: string; amount: number; type: string }[] = [];
|
||||||
|
|
||||||
for (const t of team.transactions) {
|
for (const t of teamTransactions) {
|
||||||
movements.push({ date: t.date, amount: Number(t.amount), type: t.type.name });
|
movements.push({ date: t.date, amount: Number(t.amount), type: t.type.name });
|
||||||
}
|
}
|
||||||
|
|
||||||
for (const p of team.players) {
|
for (const p of players) {
|
||||||
for (const t of p.transactions) {
|
for (const t of p.transactions ?? []) {
|
||||||
if (t.type.name === 'payment') {
|
if (t.type.name === 'payment') {
|
||||||
movements.push({ date: t.date, amount: Number(t.amount), type: t.type.name });
|
movements.push({ date: t.date, amount: Number(t.amount), type: t.type.name });
|
||||||
}
|
}
|
||||||
@@ -250,6 +262,23 @@ export class TeamsService {
|
|||||||
|
|
||||||
movements.sort((a, b) => (a.date < b.date ? -1 : a.date > b.date ? 1 : 0));
|
movements.sort((a, b) => (a.date < b.date ? -1 : a.date > b.date ? 1 : 0));
|
||||||
|
|
||||||
|
const topOutstanding = players
|
||||||
|
.filter((p) => p.active && Number(p.balance) < 0)
|
||||||
|
.sort((a, b) => Number(a.balance) - Number(b.balance))
|
||||||
|
.slice(0, 10)
|
||||||
|
.map((p) => ({
|
||||||
|
playerId: p.id,
|
||||||
|
playerName: p.firstName + ' ' + p.lastName,
|
||||||
|
balance: this.round(Math.abs(Number(p.balance))),
|
||||||
|
}));
|
||||||
|
|
||||||
|
// A brand-new team with no relevant movements at all (ever, not just in
|
||||||
|
// the last 12 months) has nothing to chart — return empty arrays so the
|
||||||
|
// frontend's empty-state fires instead of rendering 12 flat zero points.
|
||||||
|
if (movements.length === 0) {
|
||||||
|
return { balanceHistory: [], monthlyFlow: [], topOutstanding };
|
||||||
|
}
|
||||||
|
|
||||||
const months = this.getLast12Months();
|
const months = this.getLast12Months();
|
||||||
|
|
||||||
// team.balance is the one authoritative, current value — it can include
|
// team.balance is the one authoritative, current value — it can include
|
||||||
@@ -293,16 +322,6 @@ export class TeamsService {
|
|||||||
return { month, income: this.round(income), expense: this.round(expense) };
|
return { month, income: this.round(income), expense: this.round(expense) };
|
||||||
});
|
});
|
||||||
|
|
||||||
const topOutstanding = team.players
|
|
||||||
.filter((p) => p.active && Number(p.balance) < 0)
|
|
||||||
.sort((a, b) => Number(a.balance) - Number(b.balance))
|
|
||||||
.slice(0, 10)
|
|
||||||
.map((p) => ({
|
|
||||||
playerId: p.id,
|
|
||||||
playerName: p.firstName + ' ' + p.lastName,
|
|
||||||
balance: this.round(Math.abs(Number(p.balance))),
|
|
||||||
}));
|
|
||||||
|
|
||||||
return { balanceHistory, monthlyFlow, topOutstanding };
|
return { balanceHistory, monthlyFlow, topOutstanding };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user