diff --git a/src/bot/commands/reconnect.test.ts b/src/bot/commands/reconnect.test.ts index 00317e3..60dc45a 100644 --- a/src/bot/commands/reconnect.test.ts +++ b/src/bot/commands/reconnect.test.ts @@ -146,13 +146,17 @@ mock.module('../../services/currency/converter', () => ({ formatAmount: (x: number, c: string) => `${x} ${c}`, })); -const importExpensesFromSheetMock = mock(async (..._args: unknown[]): Promise => 0); - +const importExpensesFromSheetMock = mock( + async (..._args: unknown[]): Promise<{ inserted: number; skippedNoOwner: number }> => ({ + inserted: 0, + skippedNoOwner: 0, + }), +); mock.module('./sync', () => ({ importExpensesFromSheet: importExpensesFromSheetMock, })); -const { handleReconnectCommand } = await import('./reconnect'); +const { handleReconnectCommand, formatFullSyncReport } = await import('./reconnect'); let db: SqliteDb; let groups: GroupRepository; @@ -577,3 +581,28 @@ describe('/reconnect budget import from sheet', () => { expect(hasChanged).toBe(false); }); }); + +describe('/reconnect full sync report', () => { + // #184: sheet rows skipped for a userless group must be reported, not silently dropped. + test('skipped-no-owner rows are listed in the user-facing report', () => { + const report = formatFullSyncReport({ + snapshotId: null, + snapshotExpenses: 0, + snapshotBudgets: 0, + sheetBackupUrl: null, + yearCreated: null, + sheetToDbExpenses: 0, + sheetRowsSkippedNoOwner: 3, + dbToSheetExpenses: 0, + sheetToBudgets: 0, + budgetTabsCreated: [], + budgetRowsWritten: 0, + audit: [], + recreated: [], + }); + expect(report).toContain('пропущено 3 строки'); + expect(report).toContain('в группе нет ни одного участника'); + // The skip IS the outcome: the generic «синхронизированы» must not mask it (#184). + expect(report).not.toContain('расходы синхронизированы'); + }); +}); diff --git a/src/bot/commands/reconnect.ts b/src/bot/commands/reconnect.ts index d2b7298..a49f60b 100644 --- a/src/bot/commands/reconnect.ts +++ b/src/bot/commands/reconnect.ts @@ -102,6 +102,8 @@ interface FullSyncReport { sheetBackupUrl: string | null; yearCreated: number | null; sheetToDbExpenses: number; + /** Sheet rows skipped because the group had no user to own them (#184). */ + sheetRowsSkippedNoOwner: number; dbToSheetExpenses: number; sheetToBudgets: number; budgetTabsCreated: string[]; @@ -205,6 +207,7 @@ export async function fullSyncAfterReconnect(groupId: number): Promise { sheetBackupUrl: null, yearCreated: null, sheetToDbExpenses: 0, + sheetRowsSkippedNoOwner: 0, dbToSheetExpenses: 0, sheetToBudgets: 0, budgetTabsCreated: [], @@ -278,11 +281,13 @@ export async function fullSyncAfterReconnect(groupId: number): Promise { report.yearCreated = await ensureCurrentYearSpreadsheet(freshGroup); // Step 3: Sheet → DB expenses (add-only, never deletes) - report.sheetToDbExpenses = await importExpensesFromSheet( + const importResult = await importExpensesFromSheet( freshGroup.id, conn, freshGroup.spreadsheet_id, ); + report.sheetToDbExpenses = importResult.inserted; + report.sheetRowsSkippedNoOwner = importResult.skippedNoOwner; // Step 4: DB → Sheet expenses (push missing) report.dbToSheetExpenses = await pushMissingExpensesToSheet(freshGroup); @@ -674,6 +679,12 @@ function formatExpenseSyncSection(report: FullSyncReport): string[] { lines.push( `\n📥 Таблица → БД: +${report.sheetToDbExpenses} ${pluralize(report.sheetToDbExpenses, 'расход добавлен', 'расхода добавлено', 'расходов добавлено')}`, ); + } else if (report.sheetRowsSkippedNoOwner > 0) { + // The primary #184 case (0 imported, N skipped): the skip IS the outcome — saying + // «синхронизированы» would obscure that nothing was imported. + lines.push( + `⏭️ Таблица → БД: пропущено ${report.sheetRowsSkippedNoOwner} ${pluralize(report.sheetRowsSkippedNoOwner, 'строка', 'строки', 'строк')} — в группе нет ни одного участника; они добавятся после того, как кто-нибудь сделает /start`, + ); } else { lines.push('\n📥 Таблица → БД: расходы синхронизированы'); } @@ -709,7 +720,7 @@ function formatBudgetSyncSection(report: FullSyncReport): string[] { return lines; } -function formatFullSyncReport(report: FullSyncReport): string { +export function formatFullSyncReport(report: FullSyncReport): string { const lines: string[] = ['✅ Синхронизация завершена!\n']; lines.push(...formatBackupSection(report)); lines.push(...formatRecreatedSection(report)); diff --git a/src/bot/commands/sync-expenses.test.ts b/src/bot/commands/sync-expenses.test.ts index 5a3667b..7ae7c1d 100644 --- a/src/bot/commands/sync-expenses.test.ts +++ b/src/bot/commands/sync-expenses.test.ts @@ -37,7 +37,8 @@ const { DatabaseService } = await import('../../database/index'); const service = new DatabaseService(db); mock.module('../../database', () => ({ database: service, _budgetWriter: () => service.budgets })); -const { ensureFreshExpenses, handleSyncCommand, syncExpenses } = await import('./sync'); +const { ensureFreshExpenses, handleSyncCommand, importExpensesFromSheet, syncExpenses } = + await import('./sync'); const { executeTool } = await import('../../services/ai/tool-executor'); afterAll(() => db.close()); @@ -204,3 +205,29 @@ describe('sync reports name the skipped rows', () => { expect(result.output?.length).toBeLessThan(500); }); }); + +describe('importExpensesFromSheet ownership fallback', () => { + // users.id is global and the FK does not check group_id: a fallback to user 1 would hand the + // imported expense to another group's user whenever one exists (same hazard as /sync). + test("a userless group imports nothing, never rows owned by another group's user 1", async () => { + db.exec("DELETE FROM sqlite_sequence WHERE name = 'users'"); + const otherGroupId = connectedGroup(); + const otherUser = service.users.create({ telegram_id: Date.now(), group_id: otherGroupId }); + expect(otherUser.id).toBe(1); + const groupId = connectedGroup(); + sheetRows = [sheetRow('imported row')]; + + const result = await importExpensesFromSheet( + groupId, + { refreshToken: 'tok', oauthClient: 'current' } as never, + 'SHEET', + ); + + expect(result.inserted).toBe(0); + // The caller (the /reconnect report) shows the skipped count to the user (#184). + expect(result.skippedNoOwner).toBe(1); + expect(service.expenses.findByGroupId(groupId, 100)).toEqual([]); + expect(logMock.warn).toHaveBeenCalled(); + expect(logMock.error).not.toHaveBeenCalled(); + }); +}); diff --git a/src/bot/commands/sync.ts b/src/bot/commands/sync.ts index f7d5c26..3c47822 100644 --- a/src/bot/commands/sync.ts +++ b/src/bot/commands/sync.ts @@ -394,24 +394,40 @@ function appendResultSection( if (items.length > 10) lines.push(` ...и ещё ${items.length - 10}`); } +export interface SheetImportResult { + /** Rows added to the DB. */ + inserted: number; + /** Rows skipped because the group has no user to own them — never a global fallback id (#184). */ + skippedNoOwner: number; +} + /** * One-directional import: read expenses from an arbitrary spreadsheet and add any - * that are missing from the DB. Never deletes. Returns the number of inserted rows. + * that are missing from the DB. Never deletes. Rows are skipped (and counted) when + * the group has no user to own them, so callers can report it (#184). * Safe to call repeatedly — deduplicates by date|category|amount|currency key. */ export async function importExpensesFromSheet( groupId: number, conn: GoogleConn, spreadsheetId: string, -): Promise { +): Promise { const { expenses: sheetExpenses } = await readExpensesFromSheet(conn, spreadsheetId); - if (sheetExpenses.length === 0) return 0; + if (sheetExpenses.length === 0) return { inserted: 0, skippedNoOwner: 0 }; const dbExpenses = database.expenses.findByGroupId(groupId, 100000); const existingKeys = new Set(dbExpenses.map((e) => dbExpenseToKey(e))); const users = database.users.findByGroupId ? database.users.findByGroupId(groupId) : []; - const defaultUserId = users[0]?.id ?? 1; + // No fallback id: users.id is global, so any constant would hand rows to another group's user. + const defaultUserId = users[0]?.id; + if (defaultUserId === undefined) { + logger.warn( + { groupId, rows: sheetExpenses.length }, + '[SYNC] Sheet import skipped: the group has no user to own the rows', + ); + return { inserted: 0, skippedNoOwner: sheetExpenses.length }; + } let inserted = 0; for (const row of sheetExpenses) { @@ -432,7 +448,7 @@ export async function importExpensesFromSheet( existingKeys.add(parsed.key); // guard against within-loop duplicates inserted++; } - return inserted; + return { inserted, skippedNoOwner: 0 }; } // ── Auto-sync with cooldown ── diff --git a/src/bot/cron.test.ts b/src/bot/cron.test.ts index 4b28a67..fae6798 100644 --- a/src/bot/cron.test.ts +++ b/src/bot/cron.test.ts @@ -123,7 +123,11 @@ mock.module('./services/budget-sync', () => ({ })); const importExpensesFromSheetMock = mock( - async (_token: string, _groupId: number, _spreadsheetId: string): Promise => 0, + async ( + _token: string, + _groupId: number, + _spreadsheetId: string, + ): Promise<{ inserted: number; skippedNoOwner: number }> => ({ inserted: 0, skippedNoOwner: 0 }), ); mock.module('./commands/sync', () => ({ importExpensesFromSheet: importExpensesFromSheetMock, @@ -686,7 +690,7 @@ describe('recoverPriorYearExpenses', () => { beforeEach(() => { mockGroupsRepo.findAll.mockReset().mockReturnValue([]); mockGroupSpreadsheets.listAll.mockReset().mockReturnValue([]); - importExpensesFromSheetMock.mockReset().mockResolvedValue(0); + importExpensesFromSheetMock.mockReset().mockResolvedValue({ inserted: 0, skippedNoOwner: 0 }); logMock.error.mockReset(); logMock.info.mockReset(); }); @@ -728,7 +732,7 @@ describe('recoverPriorYearExpenses', () => { mockGroupSpreadsheets.listAll.mockReturnValue([ { year: new Date().getFullYear() - 1, spreadsheetId: 'sid-prev' }, ]); - importExpensesFromSheetMock.mockResolvedValue(7); + importExpensesFromSheetMock.mockResolvedValue({ inserted: 7, skippedNoOwner: 0 }); await recoverPriorYearExpenses(); @@ -736,6 +740,28 @@ describe('recoverPriorYearExpenses', () => { expect(infoCalls.some((c) => c.includes('Restored 7'))).toBe(true); }); + it('warns when a userless group forced the import to skip rows (#184)', async () => { + mockGroupsRepo.findAll.mockReturnValue([ + { + id: 1, + telegram_group_id: -100, + google_refresh_token: 'tok', + default_currency: 'EUR', + enabled_currencies: ['EUR'], + active_topic_id: null, + }, + ]); + mockGroupSpreadsheets.listAll.mockReturnValue([ + { year: new Date().getFullYear() - 1, spreadsheetId: 'sid-prev' }, + ]); + importExpensesFromSheetMock.mockResolvedValue({ inserted: 0, skippedNoOwner: 4 }); + + await recoverPriorYearExpenses(); + + const warnCalls = logMock.warn.mock.calls.map((c) => JSON.stringify(c)); + expect(warnCalls.some((c) => c.includes('skipped 4 row(s)'))).toBe(true); + }); + it('logs and continues on per-sheet import failure', async () => { mockGroupsRepo.findAll.mockReturnValue([ { @@ -753,7 +779,7 @@ describe('recoverPriorYearExpenses', () => { ]); importExpensesFromSheetMock .mockRejectedValueOnce(new Error('OAuth expired')) - .mockResolvedValueOnce(3); + .mockResolvedValueOnce({ inserted: 3, skippedNoOwner: 0 }); await recoverPriorYearExpenses(); diff --git a/src/bot/cron.ts b/src/bot/cron.ts index 21ba9f3..cb4cbbc 100644 --- a/src/bot/cron.ts +++ b/src/bot/cron.ts @@ -241,11 +241,20 @@ export async function recoverPriorYearExpenses(): Promise { for (const { year, spreadsheetId } of priorSheets) { try { - const inserted = await importExpensesFromSheet(group.id, googleConn(group), spreadsheetId); + const { inserted, skippedNoOwner } = await importExpensesFromSheet( + group.id, + googleConn(group), + spreadsheetId, + ); if (inserted > 0) { logger.info( `[RECOVER] Restored ${inserted} expense(s) for group ${group.id}, year ${year}`, ); + } else if (skippedNoOwner > 0) { + // No user to report to and none to own the rows: the operator trace (#184). + logger.warn( + `[RECOVER] group ${group.id}, year ${year}: skipped ${skippedNoOwner} row(s) — no user in the group`, + ); } else { logger.info(`[RECOVER] group ${group.id}, year ${year}: nothing missing`); }