analytics pair programming review #444
Conversation
| sources: PageViewSource[]; | ||
| }; | ||
|
|
||
| type DailyStatsValues = { |
There was a problem hiding this comment.
Hm, it has daily and weekly stats, so maybe DailyStatsValues is not the best name?
| context.entities.DailyStats.findFirst(statsQuery), | ||
| context.entities.DailyStats.findMany({ ...statsQuery, take: 7 }), |
There was a problem hiding this comment.
We can probably just do findMany and get first one from it.
| ]); | ||
|
|
||
| if (!dailyStats) { | ||
| console.log('\x1b[34mNote: No daily stats have been generated by the dailyStatsJob yet. \x1b[0m'); |
There was a problem hiding this comment.
This assumes that dailyStatsJob genreates the daily stats and that it is called that way. Might be better if the message here is less specific, or if it didn't happen here at all.
| return undefined; | ||
| } | ||
|
|
||
| return { dailyStats, weeklyStats }; |
There was a problem hiding this comment.
Ok so this function says getDailyStats but returns both daily Stats and weeklyStats, look into that.
| import { HttpError, prisma } from 'wasp/server'; | ||
| import { type GetDailyStats } from 'wasp/server/operations'; | ||
|
|
||
| type DailyStatsWithSources = DailyStats & { |
There was a problem hiding this comment.
Is it clear enough here that sources are PageViewSources? Mayee it is , but if not we can make it clearer.
There was a problem hiding this comment.
Consider renaming to jobs.ts and creating an index.ts entrypoint file.
| import { stripe } from '../payment/stripe/stripeClient'; | ||
| import { listOrders } from '@lemonsqueezy/lemonsqueezy.js'; | ||
| import { getDailyPageViews, getSources } from './providers/plausibleAnalyticsUtils'; | ||
| // import { getDailyPageViews, getSources } from './providers/googleAnalyticsUtils'; |
There was a problem hiding this comment.
This often gets overlooked. Maybe create an interface in index.ts that's explicit about which provider is being used.
| import { paymentProcessor } from '../payment/paymentProcessor'; | ||
| import { SubscriptionStatus } from '../payment/plans'; | ||
|
|
||
| export type DailyStatsProps = { dailyStats?: DailyStats; weeklyStats?: DailyStats[]; isLoading?: boolean }; |
There was a problem hiding this comment.
This type is only being used by the admin dash components. Consider moving it to a central place there.
| const nowUTC = new Date(Date.now()); | ||
| nowUTC.setUTCHours(0, 0, 0, 0); | ||
|
|
||
| const yesterdayUTC = new Date(nowUTC); | ||
| yesterdayUTC.setUTCDate(yesterdayUTC.getUTCDate() - 1); |
There was a problem hiding this comment.
Let's extract this to a function or make it a one liner with a descriptive variable name.
| const yesterdaysStats = await context.entities.DailyStats.findFirst({ | ||
| where: { | ||
| date: { | ||
| equals: yesterdayUTC, |
There was a problem hiding this comment.
Investigate whether this needs to be equals or it could be, .e.g. the most recent entity from yesterday, or >= or <= or something like that.
|
|
||
| export type DailyStatsProps = { dailyStats?: DailyStats; weeklyStats?: DailyStats[]; isLoading?: boolean }; | ||
|
|
||
| export const calculateDailyStats: DailyStatsJob<never, void> = async (_args, context) => { |
There was a problem hiding this comment.
This whole function would benefit a lot by extracting the main parts to helper functions and organizing them logically.
| let dailyStats = await context.entities.DailyStats.findUnique({ | ||
| where: { | ||
| date: nowUTC, | ||
| }, | ||
| }); | ||
|
|
||
| if (!dailyStats) { | ||
| console.log('No daily stat found for today, creating one...'); | ||
| dailyStats = await context.entities.DailyStats.create({ | ||
| data: { | ||
| date: nowUTC, | ||
| totalViews, | ||
| prevDayViewsChangePercent, | ||
| userCount, | ||
| paidUserCount, | ||
| userDelta, | ||
| paidUserDelta, | ||
| totalRevenue, | ||
| }, | ||
| }); | ||
| } else { | ||
| console.log('Daily stat found for today, updating it...'); | ||
| dailyStats = await context.entities.DailyStats.update({ | ||
| where: { | ||
| id: dailyStats.id, | ||
| }, | ||
| data: { | ||
| totalViews, | ||
| prevDayViewsChangePercent, | ||
| userCount, | ||
| paidUserCount, | ||
| userDelta, | ||
| paidUserDelta, | ||
| totalRevenue, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Replace with upsert.
| }, | ||
| }); | ||
| } | ||
| const sources = await getSources(); |
There was a problem hiding this comment.
| const sources = await getSources(); | |
| const sources = await getSources(); |
There was a problem hiding this comment.
Rename to pageViewSources.
| const sources = await getSources(); | ||
|
|
||
| for (const source of sources) { | ||
| let visitors = source.visitors; | ||
| if (typeof source.visitors !== 'number') { | ||
| visitors = parseInt(source.visitors); | ||
| } | ||
| await context.entities.PageViewSource.upsert({ | ||
| where: { | ||
| date_name: { | ||
| date: nowUTC, | ||
| name: source.source, | ||
| }, | ||
| }, | ||
| create: { | ||
| date: nowUTC, | ||
| name: source.source, | ||
| visitors, | ||
| dailyStatsId: dailyStats.id, | ||
| }, | ||
| update: { | ||
| visitors, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can we make the user more aware of why pageViewSources are here and what's going on? Can we make it more clear what the connection between dailyStats and pageViewSources is here?
Also, can we extract this upsert logic to a helper function because it feels too specific?
| console.table({ dailyStats }); | ||
| } catch (error: any) { | ||
| console.error('Error calculating daily stats: ', error); | ||
| await context.entities.Logs.create({ |
There was a problem hiding this comment.
This could be extracted to a log provider/helper.
Description
Contributor Checklist