diff --git a/api/spec/integration-db.ts b/api/spec/integration-db.ts new file mode 100644 index 00000000..9e3dc128 --- /dev/null +++ b/api/spec/integration-db.ts @@ -0,0 +1,132 @@ +import dotenv from 'dotenv'; +import { Knex } from 'knex'; + +// `knexfile` loads this same file, but these helpers read DB_* directly to decide whether +// they are allowed to write, so they cannot rely on that import having happened first. +// jest's cwd is `api/`, so `../.env` is the repository root env the API itself uses. +dotenv.config({ path: '../.env' }); + +/** + * Whether a spec may create and delete fixture rows in the configured database. + * + * `DB_HOST`/`DB_DATABASE` only prove that *a* database is reachable, and the API's + * ordinary environment defines both -- pointed at a shared or production schema where + * fixture INSERTs and cleanup DELETEs must never run. Writing therefore additionally + * requires `CTR_INTEGRATION_TEST_DB` to name the configured database exactly: an + * explicit, per-environment statement that this specific schema is disposable, rather + * than an inference drawn from configuration that happens to be present. + */ +export function integrationDbAuthorized(env: NodeJS.ProcessEnv = process.env): boolean { + return Boolean( + env.DB_HOST + && env.DB_DATABASE + && env.CTR_INTEGRATION_TEST_DB + && env.CTR_INTEGRATION_TEST_DB === env.DB_DATABASE, + ); +} + +/** + * `describe` for a block that writes to the database, downgraded to `describe.skip` + * when the opt-in above has not been given. Skipped blocks report as skipped, never as + * a silent pass. + */ +export const describeWithDb = integrationDbAuthorized() ? describe : describe.skip; + +/** Marks every row this run creates, so cleanup can never reach a row it did not make. */ +export const FIXTURE_TAG = 'b1itest'; + +let sequence = 0; + +/** A name unique to this process and call, prefixed so cleanup can find it again. */ +export function fixtureName(label: string): string { + sequence += 1; + return `${FIXTURE_TAG}-${label}-${process.pid}-${sequence}`; +} + +/** Fields of a fixture role that a test actually cares about. */ +export interface RoleFixture { + name: string; + income_cc: number; + income_xp: number; +} + +/** A fixture member, with the wallet the payout paths credit. */ +export interface MemberFixture { + id: number; + walletId: number; + username: string; +} + +/** Inserts a role and returns its id. */ +export async function createRole(knex: Knex, role: RoleFixture): Promise { + const [id] = await knex('role').insert(role); + return id; +} + +/** + * Inserts a member and its wallet. + * @param knex connection to write through + * @param overrides member columns to set, most usefully the two credit timestamps + */ +export async function createMember( + knex: Knex, + overrides: Record = {}, +): Promise { + const username = fixtureName('member'); + const [walletId] = await knex('wallet').insert({ balance: 1000 }); + const [id] = await knex('member').insert({ + username, + email: `${username}@example.invalid`, + password: 'not-a-real-hash', + wallet_id: walletId, + ...overrides, + }); + return { id, walletId, username }; +} + +/** Assigns a role to a member, optionally scoped to a place. */ +export async function assignRole( + knex: Knex, + memberId: number, + roleId: number, + placeId: number = null, +): Promise { + await knex('role_assignment').insert({ + member_id: memberId, + role_id: roleId, + place_id: placeId, + }); +} + +/** A timestamp `days` days before now, for setting a credit timestamp out of its window. */ +export function daysAgo(days: number): Date { + const when = new Date(); + when.setDate(when.getDate() - days); + return when; +} + +/** + * Deletes every row created by `createMember`/`createRole` in this schema. + * + * Keyed off {@link FIXTURE_TAG} rather than off ids collected during the run, so a spec + * that died part way through still cleans up after itself on the next run. + */ +export async function cleanUpFixtures(knex: Knex): Promise { + const members = await knex('member') + .select('id', 'wallet_id') + .where('username', 'like', `${FIXTURE_TAG}-%`); + const memberIds = members.map(member => member.id); + const walletIds = members.map(member => member.wallet_id); + if (memberIds.length) { + await knex('role_assignment').whereIn('member_id', memberIds).del(); + await knex('member').whereIn('id', memberIds).del(); + } + if (walletIds.length) { + await knex('transaction').whereIn('recipient_wallet_id', walletIds).del(); + await knex('wallet').whereIn('id', walletIds).del(); + } + await knex('role_assignment') + .whereIn('role_id', knex('role').select('id').where('name', 'like', `${FIXTURE_TAG}-%`)) + .del(); + await knex('role').where('name', 'like', `${FIXTURE_TAG}-%`).del(); +} diff --git a/api/src/controllers/member.controller.ts b/api/src/controllers/member.controller.ts index 29dd0e8c..9839ecdc 100644 --- a/api/src/controllers/member.controller.ts +++ b/api/src/controllers/member.controller.ts @@ -7,7 +7,6 @@ import * as badwords from 'badwords-list'; import { sendPasswordResetEmail, sendPasswordResetUnknownEmail } from '../libs'; import { MemberService, HomeService, PlaceService } from '../services'; -import { SessionInfo } from 'session-info.interface'; import {parseInt} from 'lodash'; class MemberController { @@ -98,7 +97,7 @@ class MemberController { } } - public async check3d(request: Request, response: Response): Promise { + public async check3d(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if(!session) return; try { @@ -110,7 +109,7 @@ class MemberController { } } - public async getActivePlaces(request: Request, response: Response): Promise { + public async getActivePlaces(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if(!session) return; try { @@ -313,7 +312,7 @@ class MemberController { const token = await this.memberService.getMemberToken(session.id); const { banned, banInfo } = await this.memberService.isBanned(session.id); if (!banned) { - await this.memberService.maybeGiveDailyCredits(session.id); + await this.memberService.giveDailyCreditsForLogin(session.id); const homeInfo = await this.homeService.getHome(session.id); const chatdefault = await this.memberService.getMemberChat(session.id); session.hasHome = !!homeInfo; @@ -462,7 +461,7 @@ class MemberController { } } - public async getOnlineUsers(request: Request, response: Response): Promise { + public async getOnlineUsers(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if (!session) return; try { @@ -491,7 +490,7 @@ class MemberController { } } - public async getStorage(request: Request, response: Response): Promise { + public async getStorage(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if (!session) return; const member_id = parseInt(request.body.member_id); @@ -504,7 +503,7 @@ class MemberController { } } - public async updateStorage(request: Request, response: Response): Promise { + public async updateStorage(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if(!session) return; try { diff --git a/api/src/cron/role-credit.integration.spec.ts b/api/src/cron/role-credit.integration.spec.ts new file mode 100644 index 00000000..8d5ddc42 --- /dev/null +++ b/api/src/cron/role-credit.integration.spec.ts @@ -0,0 +1,375 @@ +import { Container } from 'typedi'; + +import { Db } from '../db/db.class'; +import { RoleAssignmentService } from '../services'; +import { + assignRole, + cleanUpFixtures, + createMember, + createRole, + daysAgo, + describeWithDb, + fixtureName, + MemberFixture, +} from '@spec/integration-db'; + +/** + * Weekly job pay, exercised end to end through the cron that actually runs it, against a + * real MySQL. + * + * Everything under test here is about a database's behaviour rather than a service's: + * which rows a predicate selects, whether two writes land in one transaction, and what + * two workers racing each other leave behind. A mocked query builder can assert that a + * predicate was *written*; only a database can say what it *selects*, and no mock can + * fail a transaction half way through and show what survived. + * + * Requires the explicit disposable-database opt-in described in `spec/integration-db.ts`, + * and `--runInBand`: these specs read and write the shared `member` table. + */ +describeWithDb('weekly role credit (real database)', () => { + const db = Container.get(Db); + const knex = db.knex; + /** The cron module is the production entry point, and takes no arguments. */ + const runRoleCreditCron: () => Promise = jest.requireActual('./role-credit'); + + /** A member who is due weekly pay: paid last week, and active within the last seven days. */ + async function createDueMember(): Promise { + return createMember(knex, { + last_weekly_role_credit: daysAgo(7), + last_daily_login_credit: new Date(), + xp: 0, + }); + } + + async function balanceOf(member: MemberFixture): Promise { + const wallet = await knex('wallet').where({ id: member.walletId }).first(); + return wallet.balance; + } + + async function memberRow(member: MemberFixture) { + return knex('member').where({ id: member.id }).first(); + } + + async function weeklyLedgerRows(member: MemberFixture) { + return knex('transaction') + .where({ recipient_wallet_id: member.walletId }) + .andWhere('reason', 'like', 'weekly-role-credit%'); + } + + /** True when the member's eligibility timestamp has been moved to today. */ + async function stampedToday(member: MemberFixture): Promise { + const [rows] = await knex.raw( + 'SELECT DATE(last_weekly_role_credit) = DATE(NOW()) AS paid_today FROM member WHERE id = ?', + [member.id], + ); + return Boolean(rows[0].paid_today); + } + + beforeEach(async () => { + await cleanUpFixtures(knex); + }); + + afterEach(async () => { + await cleanUpFixtures(knex); + }); + + afterAll(async () => { + await knex.destroy(); + }); + + describe('eligibility', () => { + it('does not pay, or stamp, a member whose only role pays nothing', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(0); + expect(await weeklyLedgerRows(member)).toHaveLength(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('pays a role that grants CityCash but no XP', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('CcOnly'), + income_cc: 40, + income_xp: 0, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1040); + expect((await memberRow(member)).xp).toBe(0); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + expect(await stampedToday(member)).toBe(true); + }); + + it('pays a role that grants XP but no CityCash', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('XpOnly'), + income_cc: 0, + income_xp: 7, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(7); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + expect(await stampedToday(member)).toBe(true); + }); + + it('leaves a member who was already paid today alone', async () => { + const member = await createMember(knex, { + last_weekly_role_credit: new Date(), + last_daily_login_credit: new Date(), + xp: 0, + }); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1000); + expect(await weeklyLedgerRows(member)).toHaveLength(0); + }); + }); + + describe('single-pay role selection', () => { + it('pays the earning role, not the one that pays nothing', async () => { + const member = await createDueMember(); + const worker = await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + }); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + })); + await assignRole(knex, member.id, worker); + + await runRoleCreditCron(); + + const ledger = await weeklyLedgerRows(member); + expect(ledger).toHaveLength(1); + expect(ledger[0].reason).toBe(`weekly-role-credit for ${worker}`); + expect(await balanceOf(member)).toBe(1050); + expect((await memberRow(member)).xp).toBe(5); + }); + + it('breaks a CityCash tie on XP, and still pays only once', async () => { + const member = await createDueMember(); + const lowXp = await createRole(knex, { + name: fixtureName('EqualCcLowXp'), + income_cc: 60, + income_xp: 1, + }); + const highXp = await createRole(knex, { + name: fixtureName('EqualCcHighXp'), + income_cc: 60, + income_xp: 9, + }); + await assignRole(knex, member.id, lowXp); + await assignRole(knex, member.id, highXp); + + await runRoleCreditCron(); + + const ledger = await weeklyLedgerRows(member); + expect(ledger).toHaveLength(1); + expect(ledger[0].reason).toBe(`weekly-role-credit for ${highXp}`); + expect(await balanceOf(member)).toBe(1060); + expect((await memberRow(member)).xp).toBe(9); + }); + + it('pays a place-scoped assignment exactly as it pays an unscoped one', async () => { + const unscoped = await createDueMember(); + const scoped = await createDueMember(); + const role = await createRole(knex, { + name: fixtureName('PlaceScoped'), + income_cc: 50, + income_xp: 5, + }); + await assignRole(knex, unscoped.id, role, null); + await assignRole(knex, scoped.id, role, 1); + + await runRoleCreditCron(); + + expect(await balanceOf(unscoped)).toBe(1050); + expect(await balanceOf(scoped)).toBe(1050); + expect((await memberRow(scoped)).xp).toBe(5); + }); + }); + + describe('batch limit', () => { + it('fills the batch with earning members rather than with non-earning ones', async () => { + const nothing = await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + }); + const worker = await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + }); + const nonEarners: MemberFixture[] = []; + for (let index = 0; index < 3; index += 1) { + const member = await createDueMember(); + await assignRole(knex, member.id, nothing); + nonEarners.push(member); + } + const earners: MemberFixture[] = []; + for (let index = 0; index < 2; index += 1) { + const member = await createDueMember(); + await assignRole(knex, member.id, worker); + earners.push(member); + } + + const selected = await Container.get(RoleAssignmentService).getMembersDueRoleCredit(2); + + expect(selected).toHaveLength(2); + expect(selected.sort()).toEqual(earners.map(member => member.id).sort()); + for (const member of nonEarners) { + expect(selected).not.toContain(member.id); + } + }); + }); + + describe('concurrency', () => { + /** + * Slows every wallet update down, so that two payouts starting together are + * guaranteed to overlap rather than merely likely to. Without it the race is real + * but timing-dependent, and a green run would prove nothing. + */ + beforeEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + await knex.raw( + 'CREATE TRIGGER b1_slow_wallet_update BEFORE UPDATE ON wallet ' + + 'FOR EACH ROW SET @b1_slow = SLEEP(0.4)', + ); + }); + + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + }); + + it('pays exactly once when two cron executions overlap', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + + await Promise.all([runRoleCreditCron(), runRoleCreditCron()]); + + expect(await balanceOf(member)).toBe(1050); + expect((await memberRow(member)).xp).toBe(5); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + }, 30000); + }); + + describe('rollback', () => { + /** + * Fails the member half of the payout at the database, leaving the wallet and ledger + * half untouched by the injection. If the two halves commit independently, the money + * moves and the member stays eligible to be paid again. + */ + async function failMemberUpdatesFor(memberId: number): Promise { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw( + `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW + BEGIN + IF NEW.id = ${memberId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; + END IF; + END`, + ); + } + + /** + * Fails the ledger half instead, so the two halves are proven to depend on each + * other in both directions rather than only in the one the implementation happens + * to write first. + */ + async function failLedgerWritesFor(walletId: number): Promise { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_ledger_insert'); + await knex.raw( + `CREATE TRIGGER b1_block_ledger_insert BEFORE INSERT ON transaction FOR EACH ROW + BEGIN + IF NEW.recipient_wallet_id = ${walletId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected ledger failure'; + END IF; + END`, + ); + } + + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw('DROP TRIGGER IF EXISTS b1_block_ledger_insert'); + }); + + it('leaves the member untouched when the ledger half of the payout fails', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + await failLedgerWritesFor(member.walletId); + + await expect(runRoleCreditCron()).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('leaves no money moved when the member half of the payout fails', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + await failMemberUpdatesFor(member.id); + + await expect(runRoleCreditCron()).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect(await weeklyLedgerRows(member)).toHaveLength(0); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('does not pay twice when the run is retried after that failure', async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: 50, + income_xp: 5, + })); + await failMemberUpdatesFor(member.id); + await expect(runRoleCreditCron()).rejects.toThrow(); + + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await runRoleCreditCron(); + + expect(await balanceOf(member)).toBe(1050); + expect(await weeklyLedgerRows(member)).toHaveLength(1); + expect((await memberRow(member)).xp).toBe(5); + }); + }); +}); diff --git a/api/src/cron/role-credit.ts b/api/src/cron/role-credit.ts index e31c055e..ca550603 100644 --- a/api/src/cron/role-credit.ts +++ b/api/src/cron/role-credit.ts @@ -6,14 +6,7 @@ module.exports = async () => { const roleAssignmentService = Container.get(RoleAssignmentService); const batch = await roleAssignmentService.getMembersDueRoleCredit(20); console.log(`CRON[role-credit]: ${ batch.length } to process...`); - for(const row of batch) { - await roleAssignmentService.giveWeeklyRoleCredit( - row.member_id, - row.xp, - row.wallet_id, - row.income_xp, - row.income_cc, - row.role_id, - ); + for(const memberId of batch) { + await roleAssignmentService.giveWeeklyRoleCredit(memberId); } }; diff --git a/api/src/knexfile.ts b/api/src/knexfile.ts index 9cebb342..6b39f5f6 100644 --- a/api/src/knexfile.ts +++ b/api/src/knexfile.ts @@ -28,6 +28,41 @@ const config: { [key: string]: Knex.Config } = { directory: './../db/seed', }, }, + /** + * Used when NODE_ENV=test, which jest sets for us. + * + * Without this key `config[process.env.NODE_ENV]` is undefined, and because `Db`'s + * constructor calls `knex(...)` at import time, every suite that transitively imports a + * repository dies while loading rather than running a single assertion. The + * database-backed specs need the key for a second reason: it is the connection they + * actually talk to. Point DB_DATABASE at a disposable schema before running those -- + * see `spec/integration-db.ts`, which refuses to write without an explicit opt-in. + */ + test: { + client: 'mysql', + connection: { + host: process.env.DB_HOST, + port: Number.parseInt(process.env.DB_PORT), + user: process.env.DB_USER, + password: process.env.DB_PASS, + database: process.env.DB_DATABASE, + charset: 'utf8mb4', + }, + // min 0, unlike the other environments: a minimum of 2 opens connections nothing asks + // for, and jest then hangs at the end of a run waiting on handles nothing will close. + pool: { + min: 0, + max: 5, + }, + migrations: { + directory: '../db/migrations', + extension: 'ts', + tableName: 'migrations', + }, + seeds: { + directory: './../db/seed', + }, + }, production: { client: 'mysql', connection: { diff --git a/api/src/repositories/credit/credit.repository.ts b/api/src/repositories/credit/credit.repository.ts new file mode 100644 index 00000000..2e6b9260 --- /dev/null +++ b/api/src/repositories/credit/credit.repository.ts @@ -0,0 +1,242 @@ +import { Knex } from 'knex'; +import { Service } from 'typedi'; + +import { Db } from '../../db/db.class'; +import { Member, Transaction, TransactionReason, Wallet } from '../../types/models'; + +/** CityCash and experience granted by a single credit. */ +export interface CreditAmount { + /** CityCash added to the member's wallet. */ + cc: number; + /** Experience points added to the member. */ + xp: number; +} + +/** What a payout attempt did. */ +export interface CreditOutcome { + /** + * `true` when this call is the one that paid. `false` means the member was not + * eligible when the row was actually held - already credited, or holding no earning + * role - and nothing was written. + */ + credited: boolean; + /** Amounts paid. Only present when `credited`. */ + amount?: CreditAmount; + /** Role the weekly payment was made for. Only present on a weekly credit. */ + roleId?: number; +} + +/** + * Restricts a role join to roles that actually earn: ones paying CityCash, XP, or both. + * + * A role paying neither is not a job. Payroll's inner join used to exist only to ask + * "does this member hold any role at all", so an Admin assignment - seeded at 0 CityCash + * and 0 XP - qualified its holder, took a slot in the capped batch, produced a + * zero-value ledger row, and got the eligibility timestamp stamped. + * + * The two income columns are independent, so the predicate is "pays CityCash OR pays XP", + * never CityCash alone: filtering on CityCash would stop an XP-only role ever accruing + * its XP. Applied to eligibility and to role selection alike, so a member cannot be + * selected on one role and paid for another. + * + * Shared with RoleAssignmentRepository, which asks the same question when it picks the + * batch. + * @param builder grouped-where builder supplied by knex + */ +export function wherePayingRole(builder: Knex.QueryBuilder): void { + builder.where('role.income_cc', '>', 0).orWhere('role.income_xp', '>', 0); +} + +/** + * Whether a member has already had their daily login bonus since the beginning + * (00:00:00) of the current day. + * + * Lives here rather than on MemberService because the check that decides whether money + * moves has to run inside the transaction that moves it. MemberService keeps a method + * of the same name, delegating here, so both answer identically. + */ +export function hasReceivedDailyCreditToday(lastCredit: Date, now: Date = new Date()): boolean { + return lastCredit.getTime() >= new Date(now).setHours(0, 0, 0, 0); +} + +/** + * Repository owning the two job-credit payouts, each as a single database transaction. + * + * Both payouts move four things that have to agree: a wallet balance, a ledger row, the + * member's XP, and the eligibility timestamp that decides whether the payout may happen + * again. Splitting them across transactions is what let a failed second half leave money + * moved with the member still eligible, and reading eligibility outside the transaction + * that pays is what let two callers both decide to pay. + * + * So every method here follows the same shape: + * + * 1. lock the member row with SELECT ... FOR UPDATE; + * 2. re-evaluate eligibility against the locked row, not against anything a caller + * passed in or a batch query read earlier; + * 3. write wallet, ledger, XP and timestamp; + * 4. commit. + * + * A second caller arriving concurrently blocks at step 1, and a locking read returns the + * latest committed row rather than the transaction's snapshot, so it sees the first + * caller's timestamp at step 2 and becomes a no-op. Wallet balances are moved with + * `balance = balance + ?` rather than a read followed by a write of the total, so a + * credit landing on a wallet another transaction is also crediting cannot overwrite it. + * + * The member row is always the first row locked, so these paths cannot deadlock against + * each other. + */ +@Service() +export class CreditRepository { + constructor(private db: Db) {} + + /** + * Gives the member their daily login bonus, unless they have already had it today. + * @param memberId id of the member to credit + * @param amounts amounts to award, by whether the member holds any role + * @returns what was paid, or `credited: false` if the bonus had already been given + */ + public async giveDailyCredit( + memberId: number, + amounts: { unemployed: CreditAmount; employed: CreditAmount }, + ): Promise { + return this.db.knex.transaction(async trx => { + const member = await this.lockMember(trx, memberId); + if (!member) return { credited: false }; + if (hasReceivedDailyCreditToday(member.last_daily_login_credit)) { + return { credited: false }; + } + + // Holding any role at all is what counts as employed for the daily bonus, + // including a role that pays nothing. That is existing policy, unchanged here - + // the earning-role filter belongs to weekly payroll, not to this. + const [{ roles }] = await trx('role_assignment') + .where('member_id', memberId) + .count({ roles: '*' }); + const amount = Number(roles) > 0 ? amounts.employed : amounts.unemployed; + + await this.pay(trx, member.wallet_id, amount.cc, TransactionReason.DailyCredit); + await trx('member') + .where({ id: memberId }) + .update({ + xp: trx.raw('xp + ?', [amount.xp]), + last_daily_login_credit: new Date(), + }); + + return { credited: true, amount }; + }); + } + + /** + * Pays a member for one week of the highest-paying role they hold, unless they have + * already been paid today or hold no earning role. + * + * Takes only an id: the batch query that produced it read the member's XP, wallet and + * roles before any lock was held, and none of that is authoritative by the time the + * money moves. Everything the payout needs is re-read here, under the lock. + * @param memberId id of the member to pay + * @returns what was paid, or `credited: false` if the member was not due pay + */ + public async giveWeeklyRoleCredit(memberId: number): Promise { + return this.db.knex.transaction(async trx => { + const member = await this.lockMemberDueWeeklyCredit(trx, memberId); + if (!member || !member.due) return { credited: false }; + + // One role, not every role held: paying a citizen for a single job is the + // behaviour established when single pay landed, and is preserved here. + const role = await trx('role_assignment') + .select('role_assignment.role_id', 'role.income_cc', 'role.income_xp') + .innerJoin('role', 'role_assignment.role_id', 'role.id') + .where('role_assignment.member_id', memberId) + .where(wherePayingRole) + // Highest CityCash wins; among roles paying the same CityCash, the one granting + // more XP wins rather than whichever row the database happened to return first. + .orderBy('role.income_cc', 'desc') + .orderBy('role.income_xp', 'desc') + .first(); + if (!role) return { credited: false }; + + const amount: CreditAmount = { cc: role.income_cc, xp: role.income_xp }; + await this.pay( + trx, + member.wallet_id, + amount.cc, + `${TransactionReason.WeeklyCredit} for ${role.role_id}`, + ); + await trx('member') + .where({ id: memberId }) + .update({ + xp: trx.raw('xp + ?', [amount.xp]), + // Stamped from the database clock, because the predicate that reads it back is + // also evaluated there. A JS timestamp would open a double-pay window whenever + // the API and the database disagree about the date. + last_weekly_role_credit: trx.fn.now(), + }); + + return { credited: true, amount, roleId: role.role_id }; + }); + } + + /** + * Locks a member row and answers, in the same statement, whether they are due weekly + * pay. + * + * One statement rather than a lock followed by a separate check, so that the answer + * cannot come from a different point in time than the lock. Under REPEATABLE READ a + * plain SELECT reads the transaction's snapshot rather than the latest committed row, + * which is exactly the mistake this method exists to avoid making. + * + * The three conditions are deliberately the ones the batch query selects on, evaluated + * by the database so that "today" and "within the last seven days" mean the same thing + * in both places. + */ + private async lockMemberDueWeeklyCredit( + trx: Knex.Transaction, + memberId: number, + ): Promise<{ wallet_id: number; due: number }> { + return trx('member') + .select('wallet_id') + .select(trx.raw( + `status = 1 + AND DATE(last_weekly_role_credit) <> DATE(NOW()) + AND DATE(last_daily_login_credit) >= DATE(NOW() - INTERVAL 7 DAY) AS due`, + )) + .where({ id: memberId }) + .forUpdate() + .first(); + } + + /** + * Locks a member row for the life of the transaction. + * + * A locking read, so it returns the latest committed row rather than the snapshot the + * transaction started with - which is what makes the eligibility recheck that follows + * it see a concurrent payout that has already committed. + */ + private async lockMember(trx: Knex.Transaction, memberId: number): Promise { + return trx('member').where({ id: memberId }).forUpdate().first(); + } + + /** + * Moves CityCash into a wallet and records it in the ledger. + * + * `increment` emits `balance = balance + ?`, so the new balance is computed by the + * database from whatever is committed at that moment. Reading the balance and writing + * back `read + amount` instead is what let one of two concurrent credits vanish. + * + * The ledger row is written even when the amount is zero: an XP-only role still earns + * a weekly credit, and the row is the only record that it happened. + */ + private async pay( + trx: Knex.Transaction, + walletId: number, + amount: number, + reason: string, + ): Promise { + await trx('wallet').where({ id: walletId }).increment('balance', amount); + await trx('transaction').insert({ + amount, + reason, + recipient_wallet_id: walletId, + }); + } +} diff --git a/api/src/repositories/index.ts b/api/src/repositories/index.ts index 7936f133..f7799bc7 100644 --- a/api/src/repositories/index.ts +++ b/api/src/repositories/index.ts @@ -2,6 +2,7 @@ export * from './avatar/avatar.repository'; export * from './ban/ban.repository'; export * from './block/block.repository'; export * from './club-member/club-member.repository'; +export * from './credit/credit.repository'; export * from './colony/colony.repository'; export * from './home/home.repository'; export * from './home-design/home-design.repository'; diff --git a/api/src/repositories/role-assignment/role-assignment.repository.ts b/api/src/repositories/role-assignment/role-assignment.repository.ts index eac9abf0..62d6f632 100644 --- a/api/src/repositories/role-assignment/role-assignment.repository.ts +++ b/api/src/repositories/role-assignment/role-assignment.repository.ts @@ -1,15 +1,52 @@ import { Service } from 'typedi'; import { Db } from '../../db/db.class'; -import { knex } from 'knex'; import { RoleAssignment } from '../../types/models'; +import { wherePayingRole } from '../credit/credit.repository'; + +/** The donor role ids, and which of them the member should end up holding. */ +interface DonorRoleIds { + supporter: number; + advocate: number; + devotee: number; + champion: number; + donorLevel?: number; +} + +/** A member id, as the access-rights queries select it. */ +interface MemberIdRow { + member_id: number; +} + +/** A username, as the access-rights and roster queries select it. */ +interface UsernameRow { + username: string; +} + +/** A recent hire: who was given which role. */ +interface LatestAssignmentRow extends UsernameRow { + roleName: string; +} + +/** A role a member holds, with the place it is scoped to if any. */ +interface MemberRoleRow { + id: number; + place_id: number; + name: string; + place: string; +} + +/** A `count(id)` result, as knex returns it: a single row holding the total. */ +export interface AssignmentCount { + count: number; +} /** Repository for fetching/interacting with role assignment data in the database. */ @Service() export class RoleAssignmentRepository { constructor(private db: Db) {} - public async addDonor(member_id: number, roleId: any): Promise { + public async addDonor(member_id: number, roleId: DonorRoleIds): Promise { try{ await this.db.knex('role_assignment') .where('member_id', member_id) @@ -34,7 +71,7 @@ export class RoleAssignmentRepository { placeId: number, memberId: number, roleId: number, - ): Promise { + ): Promise { return this.db.knex('role_assignment') .insert( { @@ -48,15 +85,15 @@ export class RoleAssignmentRepository { public async getAccessInfoByID( placeId, ownerCode, - deputyCode): Promise<{ owner: any[]; deputies: any[] }> { - const owner: any[] = await this.db.knex + deputyCode): Promise<{ owner: MemberIdRow[]; deputies: MemberIdRow[] }> { + const owner: MemberIdRow[] = await this.db.knex .select( 'member_id', ) .from('role_assignment') .where('place_id', placeId) .where('role_id', ownerCode); - const deputies: any[] = await this.db.knex + const deputies: MemberIdRow[] = await this.db.knex .select( 'member_id', ) @@ -69,8 +106,8 @@ export class RoleAssignmentRepository { public async getAccessInfoByUsername( placeId, ownerCode, - deputyCode): Promise<{ owner: any[]; deputies: any[] }> { - const owner: any[] = await this.db.knex + deputyCode): Promise<{ owner: UsernameRow[]; deputies: UsernameRow[] }> { + const owner: UsernameRow[] = await this.db.knex .select( 'member.username', ) @@ -78,7 +115,7 @@ export class RoleAssignmentRepository { .where('role_assignment.place_id', placeId) .where('role_assignment.role_id', ownerCode) .innerJoin('member', 'role_assignment.member_id', 'member.id'); - const deputies: any[] = await this.db.knex + const deputies: UsernameRow[] = await this.db.knex .select( 'member.username', ) @@ -94,26 +131,26 @@ export class RoleAssignmentRepository { return roleResults; } - public async removeRoleAssignment(id: number): Promise { + public async removeRoleAssignment(id: number): Promise { await this.db.knex('role_assignment') .where('place_id', id) .del(); } - public async removeAllByUserId(id: number): Promise { + public async removeAllByUserId(id: number): Promise { await this.db.knex('role_assignment') .where('member_id', id) .del(); } - public async getUsernamesByRoleId(roleId: number): Promise { + public async getUsernamesByRoleId(roleId: number): Promise { return this.db.knex('role_assignment') .select('member.username') .where('role_assignment.role_id', '=', roleId) .leftJoin('member', 'role_assignment.member_id', 'member.id'); } - public async getLatest(): Promise { + public async getLatest(): Promise { return this.db.knex('role_assignment') .select('member.username', 'role.name as roleName') .leftJoin('member', 'role_assignment.member_id', 'member.id') @@ -122,7 +159,7 @@ export class RoleAssignmentRepository { .orderBy('role_assignment.id', 'desc'); } - public async getDonor(memberId: number, roleId: any): Promise { + public async getDonor(memberId: number, roleId: DonorRoleIds): Promise { return this.db.knex .select('role.name') .from('role_assignment') @@ -138,7 +175,7 @@ export class RoleAssignmentRepository { .first(); } - public async getRoleNameAndIdByMemberId(memberId: number): Promise { + public async getRoleNameAndIdByMemberId(memberId: number): Promise { return this.db.knex .distinct( 'role_assignment.role_id as id', @@ -153,61 +190,38 @@ export class RoleAssignmentRepository { } /** - * query finds all users with job who meet pay requirements - * the inner join is just there to check for holding a job - * then the for function will gather the highest paying role information per user - * the for function also packages all the information for the return - * @param limit - * @returns list of users with jobs that earned pay + * Finds members who are due weekly job pay. + * + * Returns ids and nothing else. Everything the payout needs - which role pays, what it + * pays, the member's XP and wallet - is re-read by CreditRepository inside the + * transaction that moves the money, because anything read here is already stale by the + * time a worker gets to it, and a second worker may have paid in between. + * + * The join to `role` is what makes the batch worth its size: without the earning-role + * filter, a member whose only role pays nothing qualifies, takes one of `limit` slots, + * and displaces someone who actually earned pay. + * @param limit maximum number of members to return + * @returns ids of members eligible for weekly pay, at most `limit` of them */ - public async getMembersDueRoleCredit(limit: number): Promise { - const query = await this.db.knex - .select( - 'member.id', - 'member.wallet_id', - 'member.xp', - ) + public async getMembersDueRoleCredit(limit: number): Promise { + const rows = await this.db.knex + .distinct('member.id') .from('member') .innerJoin('role_assignment', 'member.id', 'role_assignment.member_id') + .innerJoin('role', 'role_assignment.role_id', 'role.id') .where('member.status', 1) + .where(wherePayingRole) .whereRaw('DATE(member.last_weekly_role_credit) != DATE(NOW())') .whereRaw('DATE(member.last_daily_login_credit) >= DATE(NOW() - INTERVAL 7 DAY)') - .limit(limit) - .distinct('member.id'); - - const results = []; - for (const index in query) { - const member_info = query[index]; - const role_info = await this.db.knex - .select( - 'role_assignment.role_id', - 'role.income_cc', - 'role.income_xp', - ) - .from('role_assignment') - .innerJoin('role', 'role_assignment.role_id', 'role.id') - .where('role_assignment.member_id', member_info.id) - .orderBy('role.income_cc','desc') - .first(); - if (role_info) { - results[index] = { - member_id: member_info.id, - role_id: role_info.role_id, - wallet_id: member_info.wallet_id, - xp: member_info.xp, - income_cc: role_info.income_cc, - income_xp: role_info.income_xp, - }; - } - } - return results; + .limit(limit); + return rows.map(row => row.id); } public async removeIdFromAssignment( placeId: number, memberId: number, roleId: number, - ): Promise { + ): Promise { return await this.db.knex('role_assignment') .where('place_id', placeId) .where('member_id', memberId) @@ -215,9 +229,12 @@ export class RoleAssignmentRepository { .del(); } - public async countByAssigned(id: number): Promise { - return this.db.knex('role_assignment') + public async countByAssigned(id: number): Promise { + // knex types an untyped `count` as a dictionary of unnamed columns; the alias makes + // the shape known here in a way the builder's own types cannot express. + const rows = await this.db.knex('role_assignment') .count('id as count') .where('role_id', id); + return rows; } } diff --git a/api/src/repositories/transaction/transaction.repository.ts b/api/src/repositories/transaction/transaction.repository.ts index ad718a7d..b85fe328 100644 --- a/api/src/repositories/transaction/transaction.repository.ts +++ b/api/src/repositories/transaction/transaction.repository.ts @@ -3,36 +3,26 @@ import { Service } from 'typedi'; import { Db } from '../../db/db.class'; import { Transaction, TransactionReason, Wallet } from '../../types/models'; +/** A `count(id)` result, as knex returns it: a single row holding the total. */ +export interface TransactionCount { + count: number; +} + +/** + * A transaction as the admin listings hand it back: the stored columns, plus the two + * username fields `AdminService` resolves onto each row from the wallet ids once it has + * them. Optional because the repository never sets them itself. + */ +export interface TransactionListRow extends Transaction { + recipient_username?: { username: string }[]; + sender_username?: { username: string }[]; +} + /** Repository for creating/interacting with transaction/wallet data in the database. */ @Service() export class TransactionRepository { constructor(private db: Db) {} - /** - * Applies the given amount to the balance for the wallet with the given id, and creates - * a transaction record. - * @param walletId id of recipient wallet - * @param amount amount transacted - * @returns promise resolving in the created transaction object, or rejecting on error - */ - public async createDailyCreditTransaction( - walletId: number, - amount: number, - ): Promise { - return await this.db.knex.transaction(async trx => { - const wallet = await trx('wallet').where({ id: walletId }).first(); - await trx('wallet') - .where({ id: walletId }) - .update({ balance: wallet.balance + amount }); - const [transactionId] = await trx('transaction').insert({ - amount, - reason: TransactionReason.DailyCredit, - recipient_wallet_id: walletId, - }); - return this.find({ id: transactionId }); - }); - } - /** * Finds a transaction with the given search parameters if one exists. * @param transactionSearchParams object containing properties of a transaction for searching on @@ -89,24 +79,7 @@ export class TransactionRepository { return this.find({ id: transactionId }); }); } - public async createWeeklyRoleCreditTransaction( - walletId: number, - amount: number, - roleId: number, - ): Promise { - return await this.db.knex.transaction(async trx => { - const wallet = await trx('wallet').where({ id: walletId }).first(); - await trx('wallet') - .where({ id: walletId }) - .update({ balance: wallet.balance + amount }); - const [transactionId] = await trx('transaction').insert({ - amount, - reason: `${TransactionReason.WeeklyCredit} for ${roleId}`, - recipient_wallet_id: walletId, - }); - return this.find({ id: transactionId }); - }); - } + public async createSystemCreditTransaction( walletId: number, amount: number, @@ -257,7 +230,11 @@ export class TransactionRepository { }); } - public async getTransactions(type: string, limit: number, offset: number): Promise { + public async getTransactions( + type: string, + limit: number, + offset: number, + ): Promise { return this.db.knex .select( 'id', @@ -272,10 +249,13 @@ export class TransactionRepository { .limit(limit) .offset(offset) .orderBy('id', 'DESC'); - ; } - public async getTransactionsByWalletId(id: number, limit: number, offset: number): Promise { + public async getTransactionsByWalletId( + id: number, + limit: number, + offset: number, + ): Promise { return this.db.knex .select( 'id', @@ -291,35 +271,37 @@ export class TransactionRepository { .limit(limit) .offset(offset) .orderBy('id', 'DESC'); - ; } - public async getLatestTransactions(time: Date): Promise { + public async getLatestTransactions(time: Date): Promise { return this.db.knex .select('transaction.*') .from('transaction') .where('created_at', '>=', time) .limit(30) .orderBy('transaction.id', 'DESC'); - ; } - public async getTotal( type: string): Promise { - return this.db.knex + public async getTotal( type: string): Promise { + // knex types an untyped `count` as a dictionary of unnamed columns; the alias makes + // the shape known here in a way the builder's own types cannot express. + const rows = await this.db.knex .count('id as count') .from('transaction') .where('reason', type); + return rows; } - public async getWalletTotal( id: number): Promise { - return this.db.knex + public async getWalletTotal( id: number): Promise { + const rows = await this.db.knex .count('id as count') .from('transaction') .where('recipient_wallet_id', id) .orWhere('sender_wallet_id', id); + return rows; } - public async removeAllByWalletId(id: number): Promise { + public async removeAllByWalletId(id: number): Promise { await this.db.knex('transaction') .where('recipient_wallet_id', id) .orWhere('sender_wallet_id', id) diff --git a/api/src/services/member/daily-credit.integration.spec.ts b/api/src/services/member/daily-credit.integration.spec.ts new file mode 100644 index 00000000..02563dda --- /dev/null +++ b/api/src/services/member/daily-credit.integration.spec.ts @@ -0,0 +1,288 @@ +import bcrypt from 'bcrypt'; +import { Container } from 'typedi'; + +import { Db } from '../../db/db.class'; +import { MemberService } from './member.service'; +import { + assignRole, + cleanUpFixtures, + createMember, + createRole, + daysAgo, + describeWithDb, + fixtureName, + MemberFixture, +} from '@spec/integration-db'; + +/** + * The daily login credit, against a real MySQL. + * + * The interesting questions are all about what the database ends up holding: whether two + * logins arriving together pay once or twice, whether a failure part way through leaves + * money moved, and whether a credit landing at the same moment as the weekly payroll can + * overwrite it. None of those can be answered by a mocked query builder. + * + * Requires the explicit disposable-database opt-in described in `spec/integration-db.ts`, + * and `--runInBand`: these specs read and write the shared `member` table. + */ +describeWithDb('daily login credit (real database)', () => { + const knex = Container.get(Db).knex; + const service = Container.get(MemberService); + + /** The amounts are economy policy and deliberately hard-coded: B1 must not move them. */ + const UNEMPLOYED_CC = 50; + const UNEMPLOYED_XP = 5; + const EMPLOYED_CC = 100; + const EMPLOYED_XP = 10; + + /** A member who has not been credited today, and so is due the bonus. */ + async function createDueMember(overrides: Record = {}) { + return createMember(knex, { + last_daily_login_credit: daysAgo(1), + last_weekly_role_credit: new Date(), + xp: 0, + ...overrides, + }); + } + + async function balanceOf(member: MemberFixture): Promise { + const wallet = await knex('wallet').where({ id: member.walletId }).first(); + return wallet.balance; + } + + async function memberRow(member: MemberFixture) { + return knex('member').where({ id: member.id }).first(); + } + + async function dailyLedgerRows(member: MemberFixture) { + return knex('transaction') + .where({ recipient_wallet_id: member.walletId, reason: 'daily-credit' }); + } + + /** True when the member's eligibility timestamp has been moved to today. */ + async function stampedToday(member: MemberFixture): Promise { + const [rows] = await knex.raw( + 'SELECT DATE(last_daily_login_credit) = DATE(NOW()) AS credited_today ' + + 'FROM member WHERE id = ?', + [member.id], + ); + return Boolean(rows[0].credited_today); + } + + beforeEach(async () => { + await cleanUpFixtures(knex); + }); + + afterEach(async () => { + await cleanUpFixtures(knex); + }); + + afterAll(async () => { + await knex.destroy(); + }); + + describe('amounts', () => { + it('credits an unemployed member the standard amount, once', async () => { + const member = await createDueMember(); + + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + expect((await memberRow(member)).xp).toBe(UNEMPLOYED_XP); + expect(await dailyLedgerRows(member)).toHaveLength(1); + expect(await stampedToday(member)).toBe(true); + }); + + it('credits the employed amount to anyone holding a role, including one that pays nothing', + async () => { + const member = await createDueMember(); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('AdminOnly'), + income_cc: 0, + income_xp: 0, + })); + + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000 + EMPLOYED_CC); + expect((await memberRow(member)).xp).toBe(EMPLOYED_XP); + }); + + it('is a no-op for a member already credited today', async () => { + const member = await createDueMember({ last_daily_login_credit: new Date() }); + + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000); + expect(await dailyLedgerRows(member)).toHaveLength(0); + }); + }); + + describe('login', () => { + const password = 'b1-correct-horse'; + + async function createLoginMember(): Promise { + return createDueMember({ password: await bcrypt.hash(password, 10) }); + } + + it('has already applied the credit by the time it returns a token', async () => { + const member = await createLoginMember(); + + const token = await service.login(member.username, password); + + expect(typeof token).toBe('string'); + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + }); + + describe('when the credit cannot be applied', () => { + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + }); + + async function failMemberUpdatesFor(memberId: number): Promise { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw( + `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW + BEGIN + IF NEW.id = ${memberId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; + END IF; + END`, + ); + } + + it('still logs the member in', async () => { + const member = await createLoginMember(); + await failMemberUpdatesFor(member.id); + + const token = await service.login(member.username, password); + + expect(typeof token).toBe('string'); + expect(token.length).toBeGreaterThan(0); + }); + + it('leaves no half-applied credit behind', async () => { + const member = await createLoginMember(); + await failMemberUpdatesFor(member.id); + + await service.login(member.username, password); + // Long enough for a credit that was never awaited to land anyway. + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(await balanceOf(member)).toBe(1000); + expect(await dailyLedgerRows(member)).toHaveLength(0); + expect(await stampedToday(member)).toBe(false); + }); + }); + }); + + describe('concurrency', () => { + /** See the equivalent note in `role-credit.integration.spec.ts`. */ + beforeEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + await knex.raw( + 'CREATE TRIGGER b1_slow_wallet_update BEFORE UPDATE ON wallet ' + + 'FOR EACH ROW SET @b1_slow = SLEEP(0.4)', + ); + }); + + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_slow_wallet_update'); + }); + + it('credits once when two logins arrive together', async () => { + const member = await createDueMember(); + + await Promise.all([ + service.maybeGiveDailyCredits(member.id), + service.maybeGiveDailyCredits(member.id), + ]); + + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + expect((await memberRow(member)).xp).toBe(UNEMPLOYED_XP); + expect(await dailyLedgerRows(member)).toHaveLength(1); + }, 30000); + + it('does not lose a wallet update when the weekly payroll credits the same wallet', + async () => { + const runRoleCreditCron: () => Promise = jest.requireActual('../../cron/role-credit'); + const weeklyCc = 50; + const weeklyXp = 5; + const member = await createDueMember({ last_weekly_role_credit: daysAgo(7) }); + await assignRole(knex, member.id, await createRole(knex, { + name: fixtureName('RealWorker'), + income_cc: weeklyCc, + income_xp: weeklyXp, + })); + + await Promise.all([ + service.maybeGiveDailyCredits(member.id), + runRoleCreditCron(), + ]); + + expect(await balanceOf(member)).toBe(1000 + EMPLOYED_CC + weeklyCc); + expect((await memberRow(member)).xp).toBe(EMPLOYED_XP + weeklyXp); + }, 30000); + }); + + describe('rollback', () => { + afterEach(async () => { + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await knex.raw('DROP TRIGGER IF EXISTS b1_block_ledger_insert'); + }); + + async function failMemberUpdatesFor(memberId: number): Promise { + await knex.raw( + `CREATE TRIGGER b1_block_member_update BEFORE UPDATE ON member FOR EACH ROW + BEGIN + IF NEW.id = ${memberId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected member update failure'; + END IF; + END`, + ); + } + + it('moves no money when the member half of the credit fails', async () => { + const member = await createDueMember(); + await failMemberUpdatesFor(member.id); + + await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect(await dailyLedgerRows(member)).toHaveLength(0); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('leaves the member untouched when the ledger half of the credit fails', async () => { + const member = await createDueMember(); + await knex.raw( + `CREATE TRIGGER b1_block_ledger_insert BEFORE INSERT ON transaction FOR EACH ROW + BEGIN + IF NEW.recipient_wallet_id = ${member.walletId} THEN + SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'b1 injected ledger failure'; + END IF; + END`, + ); + + await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); + + expect(await balanceOf(member)).toBe(1000); + expect((await memberRow(member)).xp).toBe(0); + expect(await stampedToday(member)).toBe(false); + }); + + it('credits exactly once when the attempt is retried after that failure', async () => { + const member = await createDueMember(); + await failMemberUpdatesFor(member.id); + await expect(service.maybeGiveDailyCredits(member.id)).rejects.toThrow(); + + await knex.raw('DROP TRIGGER IF EXISTS b1_block_member_update'); + await service.maybeGiveDailyCredits(member.id); + + expect(await balanceOf(member)).toBe(1000 + UNEMPLOYED_CC); + expect(await dailyLedgerRows(member)).toHaveLength(1); + expect((await memberRow(member)).xp).toBe(UNEMPLOYED_XP); + }); + }); +}); diff --git a/api/src/services/member/member.service.spec.ts b/api/src/services/member/member.service.spec.ts index bb151a0b..bbf91abf 100644 --- a/api/src/services/member/member.service.spec.ts +++ b/api/src/services/member/member.service.spec.ts @@ -9,6 +9,7 @@ import { } from 'models'; import { AvatarRepository, + CreditRepository, MemberRepository, TransactionRepository, WalletRepository, @@ -26,6 +27,7 @@ describe('MemberService', () => { email: 'foo@foo.com', }; let avatarRepository: jest.Mocked; + let creditRepository: jest.Mocked; let memberRepository: jest.Mocked; let transactionRepository: jest.Mocked; let walletRepository: jest.Mocked; @@ -34,6 +36,8 @@ describe('MemberService', () => { beforeEach(() => { avatarRepository = createSpyObj(AvatarRepository); avatarRepository.find.mockResolvedValue(fakeAvatar as Avatar); + creditRepository = createSpyObj(CreditRepository); + creditRepository.giveDailyCredit.mockResolvedValue({ credited: true }); memberRepository = createSpyObj(MemberRepository); memberRepository.create.mockResolvedValue(fakeMember.id); memberRepository.find.mockResolvedValue(fakeMember as Member); @@ -42,6 +46,7 @@ describe('MemberService', () => { walletRepository = createSpyObj(WalletRepository); Container.reset(); Container.set(AvatarRepository, avatarRepository); + Container.set(CreditRepository, creditRepository); Container.set(MemberRepository, memberRepository); Container.set(TransactionRepository, transactionRepository); Container.set(WalletRepository, walletRepository); @@ -140,17 +145,28 @@ describe('MemberService', () => { beforeEach(async () => { await service.login(fakeMember.username, fakeMember.password); }); - it('gives daily xp to the member', () => { - expect(memberRepository.update).toHaveBeenCalledWith( - fakeMember.id, - expect.objectContaining({ xp: expect.any(Number) }), - ); + it('gives the member their daily citycash and xp', () => { + expect(creditRepository.giveDailyCredit).toHaveBeenCalledWith(fakeMember.id, { + unemployed: { + cc: MemberService.DAILY_CC_AMOUNT, + xp: MemberService.DAILY_XP_AMOUNT, + }, + employed: { + cc: MemberService.DAILY_CC_EMPLOYED_AMOUNT, + xp: MemberService.DAILY_XP_EMPLOYED_AMOUNT, + }, + }); }); - it('updates the timestamp of when the user last received login credit', () => { - expect(memberRepository.update).toHaveBeenCalledWith( - fakeMember.id, - expect.objectContaining({ last_daily_login_credit: expect.any(Date) }), - ); + }); + describe('when the daily credit cannot be given', () => { + // Refusing someone their account over a missed bonus would be the worse outcome, + // and the payout is all-or-nothing, so there is no partial credit to undo. + beforeEach(() => { + creditRepository.giveDailyCredit.mockRejectedValue(new Error('database is down')); + }); + it('still logs the member in', async () => { + const token = await service.login(fakeMember.username, fakeMember.password); + expect(token).toBe(await service.getMemberToken(fakeMember.id)); }); }); }); diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 8f033d45..798ed3a5 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -7,6 +7,8 @@ import { Service } from 'typedi'; import { AvatarRepository, BanRepository, + CreditRepository, + hasReceivedDailyCreditToday, MapLocationRepository, MemberRepository, PlaceRepository, @@ -17,11 +19,78 @@ import { ObjectInstanceRepository, VoteRepository, } from '../../repositories'; -import { Member } from '../../types/models'; +import { Member, ObjectInstance, Place } from '../../types/models'; import { MemberInfoView, MemberAdminView } from '../../types/views'; import { SessionInfo } from 'session-info.interface'; import { Request, Response } from 'express'; +/** A role a member holds, as the member views list it. */ +interface MemberRoleSummary { + id: number; + place_id: number; + name: string; + place: string; +} + +/** Whether a member is banned, and the ban that says so. */ +interface BanStatus { + banned: boolean; + banInfo?: { end_date?: string; reason?: string }; +} + +/** A place with people in it right now, decorated with its name and headcount. */ +interface ActivePlaceSummary { + place_id: number; + name?: string; + slug?: string; + type?: number; + username?: string; + count?: number; +} + +/** + * A member who has been active recently, plus the two fields the member controller + * resolves onto each row after this returns. Optional because the service never sets + * them itself. + */ +interface OnlineMember { + id: number; + username: string; + hasHome?: boolean; + security?: boolean; +} + +/** A storage unit belonging to a member, with the number of objects it holds. */ +interface StorageUnitSummary { + id: number; + count?: number; +} + +/** + * A member identified from their wallet, as `findByWalletId` actually returns them. + * + * The query behind it selects `username` alone, so a full `Member` is not available + * here however much the name suggests otherwise. + */ +interface WalletMemberSummary { + username: string; +} + +/** + * The value `MemberService.getAccessLevel()` resolves to. + * + * At runtime this is always a `string[]` of the access tags the member holds + * ('admin', 'security', 'leader'). The `'admin'` and `'security'` scalar members + * exist purely for compatibility: some legacy callers still compare the whole + * return value to a bare string (`accessLevel === 'admin'` in + * admin.controller.ts, `accessLevel === 'security'` in member.controller.ts) + * instead of testing membership. Those comparisons are an existing authority + * bug: they can never be true against the array this method actually returns. + * Repairing them changes who can reach admin functionality, so it is + * deliberately out of scope here and is tracked as separate authority work. + */ +type LegacyAccessLevel = string[] | 'admin' | 'security'; + /** Service for dealing with members */ @Service() export class MemberService { @@ -42,6 +111,7 @@ export class MemberService { private avatarRepository: AvatarRepository, private banRepository: BanRepository, private memberRepository: MemberRepository, + private creditRepository: CreditRepository, private transactionRepository: TransactionRepository, private walletRepository: WalletRepository, private placeRepository: PlaceRepository, @@ -101,7 +171,7 @@ export class MemberService { }); } - public async getAccessLevel(memberId: number): Promise { + public async getAccessLevel(memberId: number): Promise { const security = await this.canAdmin(memberId); const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); const leader = await this.canLeader(memberId); @@ -141,7 +211,7 @@ export class MemberService { username, password: hashedPassword, }); - await this.maybeGiveDailyCredits(memberId); + await this.giveDailyCreditsForLogin(memberId); return this.getMemberToken(memberId); } @@ -301,7 +371,7 @@ export class MemberService { return this.memberRepository.getPrimaryRoleName(memberId); } - public async getRoles(memberId: number): Promise { + public async getRoles(memberId: number): Promise { const roles = await this.roleAssignmentRepository.getRoleNameAndIdByMemberId(memberId); return roles; } @@ -313,8 +383,7 @@ export class MemberService { * @returns `true` if the member has received their daily login bonus today, `false` otherwise */ public hasReceivedLoginCreditToday(member: Member): boolean { - const today = new Date().setHours(0, 0, 0, 0); - return member.last_daily_login_credit.getTime() >= today; + return hasReceivedDailyCreditToday(member.last_daily_login_credit); } /** @@ -332,7 +401,7 @@ export class MemberService { * @param memberId * @return banned boolean true if banned */ - public async isBanned(memberId: number): Promise { + public async isBanned(memberId: number): Promise { let banned = false; const member = await this.memberRepository.findById(memberId); const banInfo = await this.banRepository.getBanMaxDate(memberId); @@ -360,33 +429,51 @@ export class MemberService { const validPassword = await bcrypt.compare(password, member.password); if (!validPassword) throw new Error('Incorrect login details.'); if (member.status === 0) throw new Error('banned'); - this.maybeGiveDailyCredits(member.id); + await this.giveDailyCreditsForLogin(member.id); return this.encodeMemberToken(member); } /** * Distributes daily credits (citycash, xp) to the member with the given id if they haven't * already received any today. + * + * Eligibility is rechecked inside the transaction that pays, so calling this twice at + * once credits once. Rejects if the credit could not be applied - in which case nothing + * was applied, not even partly. Callers that are logging someone in should treat that + * rejection as a lost bonus, not as a failed login. * @param memberId id of member to receive daily credits * @returns promise resolving when complete, rejecting on error */ public async maybeGiveDailyCredits(memberId: number): Promise { - const member = await this.memberRepository.findById(memberId); - if (!this.hasReceivedLoginCreditToday(member)) { - let ccIncrease = MemberService.DAILY_CC_AMOUNT; - let xpIncrease = MemberService.DAILY_XP_AMOUNT; - - const roles = await this.roleAssignmentRepository.getByMemberId(memberId); - if (roles.length > 0) { - ccIncrease = MemberService.DAILY_CC_EMPLOYED_AMOUNT; - xpIncrease = MemberService.DAILY_XP_EMPLOYED_AMOUNT; - } + await this.creditRepository.giveDailyCredit(memberId, { + unemployed: { + cc: MemberService.DAILY_CC_AMOUNT, + xp: MemberService.DAILY_XP_AMOUNT, + }, + employed: { + cc: MemberService.DAILY_CC_EMPLOYED_AMOUNT, + xp: MemberService.DAILY_XP_EMPLOYED_AMOUNT, + }, + }); + } - await this.transactionRepository.createDailyCreditTransaction(member.wallet_id, ccIncrease); - await this.memberRepository.update(memberId, { - last_daily_login_credit: new Date(), - xp: member.xp + xpIncrease, - }); + /** + * Gives daily credits as part of logging someone in, and swallows the failure if it + * cannot. + * + * Awaited, so the credit has landed before the caller gets a token: unawaited, the + * caller could read its own balance and not see it yet, and nothing kept the process + * interested in a write that outlived the response. Caught, because refusing someone + * their account over a missed bonus is the worse outcome of the two - and the payout is + * all-or-nothing, so a failure here leaves no half-applied credit to clean up. + * @param memberId id of member being logged in + * @returns promise resolving when the credit has been applied, or has failed + */ + public async giveDailyCreditsForLogin(memberId: number): Promise { + try { + await this.maybeGiveDailyCredits(memberId); + } catch (error) { + console.error(`Failed to give daily credits to member ${memberId}:`, error); } } @@ -514,7 +601,7 @@ export class MemberService { }); } - public async getActivePlaces(): Promise { + public async getActivePlaces(): Promise { const returnPlaces = []; const placeIds = []; const activeTime = new Date(Date.now() - 5 * 60000); @@ -575,13 +662,13 @@ export class MemberService { } } - public async getOnlineUsers(): Promise { + public async getOnlineUsers(): Promise { const activeTime = new Date(Date.now() - 5 * 60000); const users = await this.memberRepository.findOnlineUsers(activeTime); return users; } - public async getBackpack(username: string): Promise { + public async getBackpack(username: string): Promise { let memberId = null; let userId = null; try { @@ -595,7 +682,7 @@ export class MemberService { } } - public async getStorage(memberId: number): Promise { + public async getStorage(memberId: number): Promise { const units = []; const unit = await this.placeRepository.findStorageByUserID(memberId); for (const storage of unit) { @@ -606,17 +693,19 @@ export class MemberService { return units; } - public async getStorageById(placeId: number): Promise { + public async getStorageById(placeId: number): Promise { const unit = await this.placeRepository.findById(placeId); return unit; } - public async getMemberByWalletId(walletId: number): Promise { + public async getMemberByWalletId( + walletId: number, + ): Promise { const user = await this.memberRepository.findByWalletId(walletId); return user; } - public async removeAccount(id: number): Promise { + public async removeAccount(id: number): Promise { const user = await this.memberRepository.findById(id); await this.roleAssignmentRepository.removeAllByUserId(id); await this.banRepository.removeAllByUserId(id); diff --git a/api/src/services/role-assignment/role-assignment.service.ts b/api/src/services/role-assignment/role-assignment.service.ts index 954e62e6..d7e03f5d 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -2,9 +2,9 @@ import { Service } from 'typedi'; import { RoleAssignment } from '../../types/models'; import { + AssignmentCount, + CreditRepository, RoleAssignmentRepository, - MemberRepository, - TransactionRepository, } from '../../repositories'; /** Service for interacting with roles */ @@ -12,8 +12,7 @@ import { export class RoleAssignmentService { constructor( private roleAssignmentRepository: RoleAssignmentRepository, - private memberRepository: MemberRepository, - private transactionRepository: TransactionRepository, + private creditRepository: CreditRepository, ) {} public async getMembersRoles(memberId: number): Promise { @@ -22,36 +21,33 @@ export class RoleAssignmentService { } /** - * Grabs all payments due to users from database 50 at a time and - * places them in response array sorts respone into highest cc payout - * then drops all other payouts to the same user + * Finds up to `limit` members who are due weekly job pay. + * @param limit maximum number of members to return + * @returns ids of members eligible for weekly pay */ - public async getMembersDueRoleCredit(limit: number): Promise { - const response = await this.roleAssignmentRepository.getMembersDueRoleCredit(limit); - return response; + public async getMembersDueRoleCredit(limit: number): Promise { + return this.roleAssignmentRepository.getMembersDueRoleCredit(limit); } - public async giveWeeklyRoleCredit( - memberId: number, - memberXp: number, - walletId: number, - incomeXp: number, - incomeCc: number, - roleId: number, - ): Promise { - await this.transactionRepository.createWeeklyRoleCreditTransaction( - walletId, - incomeCc, - roleId, - ); - await this.memberRepository.update(memberId, { - last_weekly_role_credit: new Date(), - xp: memberXp + incomeXp, - - }); + /** + * Pays a member for one week of the highest-paying role they hold. + * + * Eligibility is rechecked, and the paying role re-resolved, inside the transaction + * that moves the money, so a member already paid by another worker is a no-op rather + * than a second payment. + * @param memberId id of the member to pay + * @returns promise resolving when the payout has been decided, rejecting on error + */ + public async giveWeeklyRoleCredit(memberId: number): Promise { + await this.creditRepository.giveWeeklyRoleCredit(memberId); } - public async countByAssigned(id: number): Promise { + /** + * Counts how many members hold the role with the given id. + * @param id id of the role to count assignments of + * @returns a single-row count, as knex returns it + */ + public async countByAssigned(id: number): Promise { return await this.roleAssignmentRepository.countByAssigned(id); } }