Skip to content

Resolved issues #1113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
k-deejah wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Resolved issues #1113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 167 additions & 9 deletions src/gamification/points/points.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,34 +4,60 @@ import { EventEmitter2 } from '@nestjs/event-emitter';
import { PointsService } from './points.service';
import { UserProgress } from '../entities/user-progress.entity';
import { PointTransaction } from '../entities/point-transaction.entity';
import { TiersService } from '../tiers/tiers.service';
import { TiersService, TIER_THRESHOLDS } from '../tiers/tiers.service';
import { Tier } from '../enums/tier.enum';
import { PointActivityType } from '../enums/point-activity.enum';
import { GAMIFICATION_EVENTS } from '../events/gamification.events';

// ---------------------------------------------------------------------------
// Factory helpers
// ---------------------------------------------------------------------------

const mockRepo = () => ({
findOne: jest.fn(),
find: jest.fn(),
create: jest.fn((v) => v),
save: jest.fn((v) => Promise.resolve(v)),
// create() merges the provided value so callers get back the same shape
create: jest.fn((v) => ({ ...v })),
// save() resolves with exactly the object passed in so the persisted state
// is what the service assigned BEFORE calling save (Issue #1000).
save: jest.fn((v) => Promise.resolve({ ...v })),
});

// Real getTierForPoints logic, based on TIER_THRESHOLDS, so tier-boundary
// tests exercise the actual computation path.
const realGetTierForPoints = (totalPoints: number): Tier => {
const order: Tier[] = [Tier.BRONZE, Tier.SILVER, Tier.GOLD, Tier.PLATINUM, Tier.DIAMOND];
let tier = Tier.BRONZE;
for (const t of order) {
if (totalPoints >= TIER_THRESHOLDS[t]) tier = t;
}
return tier;
};

const mockTiersService = () => ({
getTierForPoints: jest.fn().mockReturnValue(Tier.BRONZE),
getTierForPoints: jest.fn().mockImplementation(realGetTierForPoints),
});

// ---------------------------------------------------------------------------
// Shared setup
// ---------------------------------------------------------------------------

describe('PointsService', () => {
let service: PointsService;
let progressRepo: ReturnType<typeof mockRepo>;
let txRepo: ReturnType<typeof mockRepo>;
let tiersService: ReturnType<typeof mockTiersService>;
let emitter: { emit: jest.Mock };

beforeEach(async () => {
emitter = { emit: jest.fn() };

const module: TestingModule = await Test.createTestingModule({
providers: [
PointsService,
{ provide: getRepositoryToken(UserProgress), useFactory: mockRepo },
{ provide: getRepositoryToken(PointTransaction), useFactory: mockRepo },
{ provide: EventEmitter2, useValue: { emit: jest.fn() } },
{ provide: EventEmitter2, useValue: emitter },
{ provide: TiersService, useFactory: mockTiersService },
],
}).compile();
Expand All @@ -42,6 +68,10 @@ describe('PointsService', () => {
tiersService = module.get(TiersService);
});

// -------------------------------------------------------------------------
// addPoints — basic behaviour
// -------------------------------------------------------------------------

describe('addPoints', () => {
it('creates a transaction and updates progress', async () => {
progressRepo.findOne.mockResolvedValue(null);
Expand Down Expand Up @@ -75,33 +105,157 @@ describe('PointsService', () => {
expect(progress.level).toBe(2); // floor(1100/1000)+1 = 2
});

it('detects tier promotion', async () => {
it('detects tier promotion when tier changes', async () => {
const existing: Partial<UserProgress> = {
totalPoints: 900,
xp: 900,
level: 1,
tier: Tier.BRONZE,
};
progressRepo.findOne.mockResolvedValue(existing);
tiersService.getTierForPoints.mockReturnValue(Tier.SILVER);
// 900 + 200 = 1100 → SILVER threshold is 1000
const { tierPromoted } = await service.addPoints('user-1', 200, 'TEST');
expect(tierPromoted).toBe(true);
});

it('does not flag promotion when tier unchanged', async () => {
it('does not flag promotion when tier is unchanged', async () => {
const existing: Partial<UserProgress> = {
totalPoints: 100,
xp: 100,
level: 1,
tier: Tier.BRONZE,
};
progressRepo.findOne.mockResolvedValue(existing);
tiersService.getTierForPoints.mockReturnValue(Tier.BRONZE);
// 100 + 50 = 150 — still BRONZE
const { tierPromoted } = await service.addPoints('user-1', 50, 'TEST');
expect(tierPromoted).toBe(false);
});

// -------------------------------------------------------------------------
// Issue #1000 — tier must be persisted in the same save call
// -------------------------------------------------------------------------

it('persists the new tier inside the save call (Issue #1000)', async () => {
const existing: Partial<UserProgress> = {
totalPoints: 900,
xp: 900,
level: 1,
tier: Tier.BRONZE,
};
progressRepo.findOne.mockResolvedValue(existing);

// 900 + 200 = 1100 → SILVER
await service.addPoints('user-1', 200, 'TEST');

// The argument that was passed to save() must already carry the new tier,
// not the old one — no second save should be needed.
const savedArg: Partial<UserProgress> = progressRepo.save.mock.calls[0][0];
expect(savedArg.tier).toBe(Tier.SILVER);
});

it('returned progress.tier matches what was persisted (Issue #1000)', async () => {
progressRepo.findOne.mockResolvedValue(null);

// Starting from 0, award enough to reach SILVER (1000 pts)
const { progress } = await service.addPoints('user-1', 1000, 'TEST');

expect(progress.tier).toBe(Tier.SILVER);
});

it('emits POINTS_AWARDED only after save resolves (Issue #1000)', async () => {
progressRepo.findOne.mockResolvedValue(null);

// Make save() resolve asynchronously but controllably
let resolveSave!: (v: any) => void;
progressRepo.save.mockReturnValueOnce(
new Promise((r) => {
resolveSave = r;
}),
);

const addPromise = service.addPoints('user-1', 100, 'TEST');

// Event must NOT fire before save resolves
expect(emitter.emit).not.toHaveBeenCalled();

resolveSave({ totalPoints: 100, xp: 100, level: 1, tier: Tier.BRONZE });
await addPromise;

expect(emitter.emit).toHaveBeenCalledWith(
GAMIFICATION_EVENTS.POINTS_AWARDED,
expect.any(Object),
);
});

it('does not emit if save rejects (Issue #1000)', async () => {
progressRepo.findOne.mockResolvedValue(null);
progressRepo.save.mockRejectedValueOnce(new Error('DB write failed'));

await expect(service.addPoints('user-1', 100, 'TEST')).rejects.toThrow('DB write failed');

expect(emitter.emit).not.toHaveBeenCalled();
});
});

// -------------------------------------------------------------------------
// Issue #1000 — tier boundary crossing test: promotion fires exactly once
// -------------------------------------------------------------------------

describe('tier boundary crossing across two awards', () => {
it('promoton event fires once and persisted tier is correct after crossing SILVER boundary', async () => {
// First award: stays in BRONZE (900 pts)
progressRepo.findOne.mockResolvedValueOnce(null);
const first = await service.addPoints('user-1', 900, 'TEST');

expect(first.tierPromoted).toBe(false);
expect(first.progress.tier).toBe(Tier.BRONZE);

// Simulate what the DB would return on the next read — BRONZE with 900pts
const afterFirstSave = { ...progressRepo.save.mock.results[0].value };

// Second award: crosses into SILVER (900 + 200 = 1100)
progressRepo.findOne.mockResolvedValueOnce(afterFirstSave);
const second = await service.addPoints('user-1', 200, 'TEST');

expect(second.tierPromoted).toBe(true);
expect(second.progress.tier).toBe(Tier.SILVER);

// The saved argument on the second save must carry SILVER
const secondSaveArg: Partial<UserProgress> =
progressRepo.save.mock.calls[progressRepo.save.mock.calls.length - 1][0];
expect(secondSaveArg.tier).toBe(Tier.SILVER);

// Total POINTS_AWARDED emissions across both calls
const promotionEmits = emitter.emit.mock.calls.filter(
([event]) => event === GAMIFICATION_EVENTS.POINTS_AWARDED,
);
expect(promotionEmits).toHaveLength(2); // one per award, regardless of tier change

// tierPromoted was false on first and true on second — exactly one boundary crossing
expect(first.tierPromoted).toBe(false);
expect(second.tierPromoted).toBe(true);
});

it('does not fire a second promotion when the same boundary is re-approached', async () => {
// User already at SILVER (1000 pts), award more without crossing GOLD (5000 pts)
const existing: Partial<UserProgress> = {
totalPoints: 1000,
xp: 1000,
level: 2,
tier: Tier.SILVER,
};
progressRepo.findOne.mockResolvedValue(existing);

const { tierPromoted } = await service.addPoints('user-1', 100, 'TEST');

expect(tierPromoted).toBe(false);
});
});

// -------------------------------------------------------------------------
// awardActivity
// -------------------------------------------------------------------------

describe('awardActivity', () => {
it('uses POINT_RULES for known activity types', async () => {
progressRepo.findOne.mockResolvedValue(null);
Expand All @@ -110,6 +264,10 @@ describe('PointsService', () => {
});
});

// -------------------------------------------------------------------------
// getUserProgress
// -------------------------------------------------------------------------

describe('getUserProgress', () => {
it('returns null when no progress exists', async () => {
progressRepo.findOne.mockResolvedValue(null);
Expand Down
29 changes: 23 additions & 6 deletions src/gamification/points/points.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { User } from '../../users/entities/user.entity';
import { GAMIFICATION_EVENTS, PointsAwardedEvent } from '../events/gamification.events';
import { TiersService } from '../tiers/tiers.service';
import { PointActivityType, POINT_RULES } from '../enums/point-activity.enum';
import { Tier } from '../enums/tier.enum';

// /** Points per XP level (level = floor(xp / XP_PER_LEVEL) + 1) */
// const XP_PER_LEVEL = 1000;
Expand Down Expand Up @@ -38,6 +39,14 @@ export class PointsService {
/**
* Award an arbitrary number of points for a custom activity string.
* Returns the updated progress and whether a tier promotion occurred.
*
* Issue #1000 — tier computation and persistence ordering fix:
* 1. `previousTier` is captured from the loaded entity before any mutation.
* 2. `newTier` is derived from the projected `totalPoints` BEFORE save,
* then assigned to `progress.tier` so a single `save()` call persists
* both the new point total and the correct tier atomically.
* 3. The POINTS_AWARDED event is emitted only after `save()` resolves, so
* a rolled-back or failed write never publishes a phantom promotion.
*/
async addPoints(
userId: string,
Expand All @@ -63,19 +72,27 @@ export class PointsService {
});
}

// Capture the tier currently on the record before any mutation so we can
// detect a real boundary crossing after the save commits.
const previousTier = progress.tier ?? Tier.BRONZE;

progress.totalPoints += points;
progress.xp += points;
progress.level = Math.floor(progress.xp / 1000) + 1;

const newLevel = Math.floor(progress.xp / 1000) + 1;
progress.level = newLevel;
// Derive the new tier from the projected total and assign it BEFORE save
// so the single repository call persists both points and tier together.
const newTier = this.tiersService.getTierForPoints(progress.totalPoints);
progress.tier = newTier;

const previousTier = progress.tier;
const saved = await this.userProgressRepository.save(progress);
const newTier = this.tiersService.getTierForPoints(saved.totalPoints);
saved.tier = newTier;

// tierPromoted is true only when the boundary is actually crossed and the
// value is now durable in the database.
const tierPromoted = newTier !== previousTier;

// Emit event so BadgesService can react
// Emit only after the DB write succeeds so a save failure does not
// publish a promotion that never actually committed.
this.eventEmitter.emit(
GAMIFICATION_EVENTS.POINTS_AWARDED,
new PointsAwardedEvent(userId, saved.totalPoints, saved.level),
Expand Down
Loading
Loading