mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 00:11:37 -06:00
fix(stats): count completed attempts in the activity goal
The activity overview derived completions from reading sessions that reached 99 percent, while the
dashboard's reading-goal widget counted completed reading attempts. The two disagreed on iOS, where
Home reads the widget and the Annual Goal card reads the overview: a book marked read by hand, one
finished on Kobo or KOReader, an audiobook whose last session stops short of the end, and a re-read
collapsed into its first finish were all invisible to the session heuristic.
Commit 3de365a0 already made completed attempts canonical for the web timeline, goal trajectory and
completion latency. The activity-overview route arrived later with its own session-based query and
reintroduced the divergence on the one surface the web never calls.
Drop the timezone conversion with it. endedOn is a calendar date rather than an instant, so it has
no zone to convert and AT TIME ZONE would only shift buckets across year boundaries.
Fixes #1474
This commit is contained in:
@@ -128,6 +128,63 @@ describe('UserStatisticsRepository', () => {
|
||||
expect(calls[0]!.text).not.toContain('"reading_sessions"."end_progress"');
|
||||
});
|
||||
|
||||
it('counts completed reading attempts for the activity completion timeline', async () => {
|
||||
const calls: Array<{ text: string; params: unknown[] }> = [];
|
||||
const fakeClient = {
|
||||
query: vi.fn().mockImplementation((cfg: { text: string }, params: unknown[]) => {
|
||||
calls.push({ text: cfg.text, params });
|
||||
const count = cfg.text.includes('"reading_attempts"') ? 5 : 2;
|
||||
return Promise.resolve({ rows: [[2026, 9, count]] });
|
||||
}),
|
||||
};
|
||||
const db = drizzle({ client: fakeClient as never, schema });
|
||||
const repo = new UserStatisticsRepository(db as never);
|
||||
|
||||
await expect(repo.getActivityCompletionTimeline(5, [2])).resolves.toEqual([{ year: 2026, month: 9, count: 5 }]);
|
||||
|
||||
expect(calls).toHaveLength(1);
|
||||
expect(calls[0]!.text).toContain('"reading_attempts"');
|
||||
expect(calls[0]!.text).not.toContain('"reading_sessions"."end_progress"');
|
||||
expect(calls[0]!.text).not.toContain('AT TIME ZONE');
|
||||
});
|
||||
|
||||
it('scopes and filters the activity completion timeline the way the reading-goal widget does', async () => {
|
||||
const calls: Array<{ text: string; params: unknown[] }> = [];
|
||||
const fakeClient = {
|
||||
query: vi.fn().mockImplementation((cfg: { text: string }, params: unknown[]) => {
|
||||
calls.push({ text: cfg.text, params });
|
||||
return Promise.resolve({ rows: [] });
|
||||
}),
|
||||
};
|
||||
const db = drizzle({ client: fakeClient as never, schema });
|
||||
const repo = new UserStatisticsRepository(db as never);
|
||||
|
||||
await repo.getActivityCompletionTimeline(5, [2, 7]);
|
||||
|
||||
const { text, params } = calls[0]!;
|
||||
expect(text).toContain('"reading_attempts"."outcome" =');
|
||||
expect(text).toContain('"reading_attempts"."ended_on" is not null');
|
||||
expect(text).toContain('"reading_attempts"."deleted_at" is null');
|
||||
expect(text).toContain('"books"."library_id" in');
|
||||
expect(params).toContain('completed');
|
||||
expect(params).toEqual(expect.arrayContaining([5, 2, 7]));
|
||||
});
|
||||
|
||||
it('returns nothing for the activity completion timeline when no library is in scope', async () => {
|
||||
const calls: Array<{ text: string }> = [];
|
||||
const fakeClient = {
|
||||
query: vi.fn().mockImplementation((cfg: { text: string }) => {
|
||||
calls.push({ text: cfg.text });
|
||||
return Promise.resolve({ rows: [] });
|
||||
}),
|
||||
};
|
||||
const db = drizzle({ client: fakeClient as never, schema });
|
||||
const repo = new UserStatisticsRepository(db as never);
|
||||
|
||||
await expect(repo.getActivityCompletionTimeline(5, [])).resolves.toEqual([]);
|
||||
expect(calls[0]!.text).toContain('false');
|
||||
});
|
||||
|
||||
it('uses completed reading attempt dates for completion latency', async () => {
|
||||
const calls: Array<{ text: string; params: unknown[] }> = [];
|
||||
const fakeClient = {
|
||||
|
||||
@@ -211,20 +211,19 @@ export class UserStatisticsRepository {
|
||||
return rows.map((row) => row.day);
|
||||
}
|
||||
|
||||
async getActivityCompletionTimeline(userId: number, libraryIds: number[] | null, timeZone: string): Promise<UserCompletionTimelinePoint[]> {
|
||||
const firstCompletion = this.db
|
||||
.select({
|
||||
bookId: readingSessions.bookId,
|
||||
firstCompletedAt: sql<Date>`min(${readingSessions.endedAt})`.as('first_completed_at'),
|
||||
})
|
||||
.from(readingSessions)
|
||||
.innerJoin(books, eq(books.id, readingSessions.bookId))
|
||||
.where(and(eq(readingSessions.userId, userId), gte(readingSessions.endProgress, 99), this.libraryFilter(libraryIds)))
|
||||
.groupBy(readingSessions.bookId)
|
||||
.as('activity_first_completion');
|
||||
const localCompletion = sql`${firstCompletion.firstCompletedAt} AT TIME ZONE ${timeZone}`;
|
||||
const yearExpr = sql<number>`extract(year from ${localCompletion})::int`;
|
||||
const monthExpr = sql<number>`extract(month from ${localCompletion})::int`;
|
||||
/**
|
||||
* Completed books per month for the activity overview.
|
||||
*
|
||||
* Completed reading attempts are the canonical record, the same source the dashboard's
|
||||
* reading-goal widget counts, so the Home tile and the activity goal card cannot disagree.
|
||||
* Deriving completions from sessions that reached 99 percent instead drops every book marked
|
||||
* read by hand or finished on Kobo, KOReader or an audiobook that stops short of the end, and
|
||||
* collapses a re-read into the year of its first finish. `endedOn` is a calendar date rather
|
||||
* than an instant, so it carries no zone to convert and is bucketed as stored.
|
||||
*/
|
||||
async getActivityCompletionTimeline(userId: number, libraryIds: number[] | null): Promise<UserCompletionTimelinePoint[]> {
|
||||
const yearExpr = sql<number>`extract(year from ${readingAttempts.endedOn})::int`;
|
||||
const monthExpr = sql<number>`extract(month from ${readingAttempts.endedOn})::int`;
|
||||
|
||||
return this.db
|
||||
.select({
|
||||
@@ -232,9 +231,19 @@ export class UserStatisticsRepository {
|
||||
month: monthExpr,
|
||||
count: sql<number>`count(*)::int`,
|
||||
})
|
||||
.from(firstCompletion)
|
||||
.groupBy(sql`1`, sql`2`)
|
||||
.orderBy(sql`1`, sql`2`);
|
||||
.from(readingAttempts)
|
||||
.innerJoin(books, eq(books.id, readingAttempts.bookId))
|
||||
.where(
|
||||
and(
|
||||
eq(readingAttempts.userId, userId),
|
||||
eq(readingAttempts.outcome, 'completed'),
|
||||
isNotNull(readingAttempts.endedOn),
|
||||
isNull(readingAttempts.deletedAt),
|
||||
this.libraryFilter(libraryIds),
|
||||
),
|
||||
)
|
||||
.groupBy(yearExpr, monthExpr)
|
||||
.orderBy(yearExpr, monthExpr);
|
||||
}
|
||||
|
||||
async getActivityPaceSummary(userId: number, libraryIds: number[] | null, days = 1825): Promise<ActivityPaceSummary> {
|
||||
|
||||
@@ -309,7 +309,7 @@ export class UserStatisticsService {
|
||||
|
||||
const [availableYearsRaw, completionRows, funnel, pace] = await Promise.all([
|
||||
this.repo.getActivityAvailableYears(user.id, libraryIds, timeZone),
|
||||
this.repo.getActivityCompletionTimeline(user.id, libraryIds, timeZone),
|
||||
this.repo.getActivityCompletionTimeline(user.id, libraryIds),
|
||||
this.getProgressFunnel(user, { libraryIds: query.libraryIds, days: 365, comparePrevious: true }),
|
||||
this.repo.getActivityPaceSummary(user.id, libraryIds),
|
||||
]);
|
||||
|
||||
@@ -804,4 +804,149 @@ describe('Reading attempts main-flow simulation (docker e2e)', { timeout: TIMEOU
|
||||
expect(latencyResponse.statusCode).toBe(200);
|
||||
expect(latencyResponse.json()).toMatchObject({ totalCompletions: 3, medianDays: 2 });
|
||||
});
|
||||
|
||||
it('32. reports the same completed-book count on the reading-goal widget and the activity overview', async () => {
|
||||
const username = `activity-goal-${randomUUID()}`;
|
||||
const password = 'ActivityGoal123!';
|
||||
const [user] = await ctx.db
|
||||
.insert(schema.users)
|
||||
.values({
|
||||
username,
|
||||
name: 'Activity Goal User',
|
||||
passwordHash: await hash(password, 4),
|
||||
isDefaultPassword: false,
|
||||
provisioningMethod: 'local',
|
||||
})
|
||||
.returning({ id: schema.users.id });
|
||||
await ctx.db.insert(schema.userLibraryAccess).values({ userId: user.id, libraryId, accessLevel: 'viewer' });
|
||||
|
||||
const attempts = ctx.app.get(ReadingAttemptService);
|
||||
const thisYear = new Date().getUTCFullYear();
|
||||
const todayKey = new Date().toISOString().slice(0, 10);
|
||||
// Every seeded date has to be in the current year and not in the future, on Jan 1 as much as
|
||||
// in December, so a fixed day in January is clamped to today rather than landing ahead of it.
|
||||
const dayThisYear = (monthDay: string) => {
|
||||
const candidate = `${thisYear}-${monthDay}`;
|
||||
return candidate > todayKey ? todayKey : candidate;
|
||||
};
|
||||
|
||||
async function addSession(book: BookFixture, attemptId: number, endProgress: number, day: string) {
|
||||
const startedAt = new Date(`${day}T10:00:00.000Z`);
|
||||
await ctx.db.insert(schema.readingSessions).values({
|
||||
userId: user.id,
|
||||
bookFileId: book.fileId,
|
||||
bookId: book.bookId,
|
||||
attemptId,
|
||||
sessionId: `activity-goal-${randomUUID()}`,
|
||||
source: 'koreader',
|
||||
startedAt,
|
||||
endedAt: new Date(startedAt.getTime() + 60 * 60 * 1000),
|
||||
durationSeconds: 3600,
|
||||
progressDelta: 10,
|
||||
endProgress,
|
||||
});
|
||||
}
|
||||
|
||||
// Finished in the app, with a session that ran to the end. Both sources always agreed here.
|
||||
const finishedInApp = await createBook();
|
||||
const finishedInAppAttempt = await attempts.createHistorical(user.id, finishedInApp.bookId, {
|
||||
startedOn: dayThisYear('02-01'),
|
||||
endedOn: dayThisYear('02-10'),
|
||||
outcome: 'completed',
|
||||
});
|
||||
await addSession(finishedInApp, finishedInAppAttempt.id, 100, dayThisYear('02-10'));
|
||||
|
||||
// Marked read by hand or imported from another client: no session exists at all.
|
||||
const markedRead = await createBook();
|
||||
await attempts.createHistorical(user.id, markedRead.bookId, {
|
||||
startedOn: dayThisYear('03-01'),
|
||||
endedOn: dayThisYear('03-05'),
|
||||
outcome: 'completed',
|
||||
});
|
||||
|
||||
// Finished elsewhere, so the last session this server saw stops short of the end.
|
||||
const finishedElsewhere = await createBook();
|
||||
const finishedElsewhereAttempt = await attempts.createHistorical(user.id, finishedElsewhere.bookId, {
|
||||
startedOn: dayThisYear('04-01'),
|
||||
endedOn: dayThisYear('04-08'),
|
||||
outcome: 'completed',
|
||||
});
|
||||
await addSession(finishedElsewhere, finishedElsewhereAttempt.id, 97.4, dayThisYear('04-08'));
|
||||
|
||||
// Read twice this year. Each finish counts, rather than collapsing into the first one.
|
||||
const reRead = await createBook();
|
||||
const firstRead = await attempts.createHistorical(user.id, reRead.bookId, {
|
||||
startedOn: dayThisYear('05-01'),
|
||||
endedOn: dayThisYear('05-10'),
|
||||
outcome: 'completed',
|
||||
});
|
||||
await addSession(reRead, firstRead.id, 100, dayThisYear('05-10'));
|
||||
const secondRead = await attempts.createHistorical(user.id, reRead.bookId, {
|
||||
startedOn: dayThisYear('06-01'),
|
||||
endedOn: dayThisYear('06-10'),
|
||||
outcome: 'completed',
|
||||
});
|
||||
await addSession(reRead, secondRead.id, 100, dayThisYear('06-10'));
|
||||
|
||||
// Finished last year, so it belongs to last year's total and must not reach this one.
|
||||
const lastYearBook = await createBook();
|
||||
const lastYearAttempt = await attempts.createHistorical(user.id, lastYearBook.bookId, {
|
||||
startedOn: `${thisYear - 1}-11-01`,
|
||||
endedOn: `${thisYear - 1}-11-20`,
|
||||
outcome: 'completed',
|
||||
});
|
||||
await addSession(lastYearBook, lastYearAttempt.id, 100, `${thisYear - 1}-11-20`);
|
||||
|
||||
// Deleted, skimmed and undated attempts stay out of every count.
|
||||
const excludedBook = await createBook();
|
||||
const deleted = await attempts.createHistorical(user.id, excludedBook.bookId, {
|
||||
startedOn: dayThisYear('07-01'),
|
||||
endedOn: dayThisYear('07-02'),
|
||||
outcome: 'completed',
|
||||
});
|
||||
await attempts.delete(user.id, excludedBook.bookId, deleted.id);
|
||||
await attempts.createHistorical(user.id, excludedBook.bookId, {
|
||||
startedOn: dayThisYear('07-03'),
|
||||
endedOn: dayThisYear('07-04'),
|
||||
outcome: 'skimmed',
|
||||
});
|
||||
await attempts.createHistorical(user.id, excludedBook.bookId, { startedOn: null, endedOn: null, outcome: 'completed' });
|
||||
|
||||
const login = await ctx.app.inject({ method: 'POST', url: '/api/v1/auth/login', payload: { username, password } });
|
||||
expect(login.statusCode).toBe(200);
|
||||
const token = (login.json() as { accessToken: string }).accessToken;
|
||||
|
||||
const widgetResponse = await ctx.app.inject({
|
||||
method: 'GET',
|
||||
url: '/api/v1/dashboard/widgets/reading-goal',
|
||||
headers: auth(token),
|
||||
});
|
||||
expect(widgetResponse.statusCode).toBe(200);
|
||||
const widget = widgetResponse.json() as { completedBooks: number; year: number };
|
||||
|
||||
const overviewResponse = await ctx.app.inject({
|
||||
method: 'GET',
|
||||
url: '/api/v1/user-statistics/activity-overview',
|
||||
headers: auth(token),
|
||||
});
|
||||
expect(overviewResponse.statusCode).toBe(200);
|
||||
const overview = overviewResponse.json() as {
|
||||
snapshot: { completedBooksYtd: number };
|
||||
goal: { year: number; completedBooks: number; points: Array<{ actualCumulative: number }> };
|
||||
completion: { months: Array<{ year: number; month: number; count: number }> };
|
||||
};
|
||||
|
||||
// Four books finished this year, one of them twice: the two surfaces have to say the same
|
||||
// thing, and say 5, rather than the 2 a session-derived count would have reported.
|
||||
expect(widget.completedBooks).toBe(5);
|
||||
expect(overview.goal.completedBooks).toBe(widget.completedBooks);
|
||||
expect(overview.snapshot.completedBooksYtd).toBe(widget.completedBooks);
|
||||
expect(overview.goal.points.at(-1)?.actualCumulative).toBe(5);
|
||||
expect(overview.goal.year).toBe(widget.year);
|
||||
|
||||
const monthsThisYear = overview.completion.months.filter((month) => month.year === thisYear);
|
||||
expect(monthsThisYear.reduce((sum, month) => sum + month.count, 0)).toBe(5);
|
||||
const monthsLastYear = overview.completion.months.filter((month) => month.year === thisYear - 1);
|
||||
expect(monthsLastYear.reduce((sum, month) => sum + month.count, 0)).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user