Add host-report script - #11715
Open
kewitz wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughA new TypeScript CLI script is added to generate aggregated host transaction report nodes. The script accepts period type (month, quarter, year), host ID, and date range parameters. It constructs a SQL query using CTEs to bucket transactions by configurable date fields, calculates platform, host, and payment-processor fees alongside tax amounts converted to host currency, and determines host-related activity status using direct host matching and parent-collective relationships. The script validates inputs, fetches host currency information, executes the query, transforms results into structured report nodes, and logs the output before closing the database connection. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
envargument is declared in the CLI signature (generate <period> <hostId> <from> <to> [dateField] [env]) but never used in the action handler, which may confuse users; either handle it explicitly or remove it from the interface. - The
dateFieldparameter only has an implicit fallback tocreatedAt; adding an explicit validation (e.g., asserting it is eithercreatedAtoreffective) would make the behavior clearer and prevent unexpected values from silently being treated ascreatedAt. - Consider wrapping the main command logic in a try/finally block and
awaitingsequelize.close()to ensure the DB connection is closed even if an error occurs during query execution or post-processing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `env` argument is declared in the CLI signature (`generate <period> <hostId> <from> <to> [dateField] [env]`) but never used in the action handler, which may confuse users; either handle it explicitly or remove it from the interface.
- The `dateField` parameter only has an implicit fallback to `createdAt`; adding an explicit validation (e.g., asserting it is either `createdAt` or `effective`) would make the behavior clearer and prevent unexpected values from silently being treated as `createdAt`.
- Consider wrapping the main command logic in a try/finally block and `await`ing `sequelize.close()` to ensure the DB connection is closed even if an error occurs during query execution or post-processing.
## Individual Comments
### Comment 1
<location path="scripts/accounting/generate-host-report.ts" line_range="49-52" />
<code_context>
+ e."type" AS "expenseType",
+ NOW() AS "refreshedAt"
+ FROM "Transactions" t
+ LEFT JOIN LATERAL (
+ SELECT e2."type" FROM "Expenses" e2 WHERE e2.id = t."ExpenseId"
+ ) AS e ON t."ExpenseId" IS NOT NULL
+ WHERE t."deletedAt" IS NULL
+ AND t."HostCollectiveId" = :hostCollectiveId
</code_context>
<issue_to_address>
**suggestion:** The lateral join for expenses looks more complex than necessary.
Given the correlation `WHERE e2.id = t."ExpenseId"`, the `LATERAL` isn’t necessary here, and `ON t."ExpenseId" IS NOT NULL` is unusual. You can keep the same semantics with a simpler left join:
```sql
LEFT JOIN "Expenses" e ON e.id = t."ExpenseId"
```
This preserves rows with a null `ExpenseId` while avoiding the lateral subquery and custom `ON` clause.
```suggestion
FROM "Transactions" t
LEFT JOIN "Expenses" e ON e.id = t."ExpenseId"
```
</issue_to_address>
### Comment 2
<location path="scripts/accounting/generate-host-report.ts" line_range="105" />
<code_context>
+ .action(async (period, hostId, from, to, dateField = 'createdAt') => {
+ console.log('Generating report for host', hostId, 'from', from, 'to', to, 'dateField', dateField);
+ const host = await models.Collective.findByPk(toNumber(hostId));
+ assert(['month', 'quarter', 'year'].includes(period), 'Invalid period, must be month, quarter or year');
+ assert(host, 'Host not found');
+
</code_context>
<issue_to_address>
**suggestion:** Period validation is case-sensitive and may cause avoidable failures.
Because the allowed `period` values are lowercase, inputs like `Month` or `YEAR` will fail even though they’re reasonable for a CLI. If you want more user-friendly behavior, normalize `period` (e.g., `period = period.toLowerCase()`) before the `assert` and reuse that normalized value for validation and `timeUnit`.
Suggested implementation:
```typescript
.command('generate <period> <hostId> <from> <to> [dateField] [env]')
.action(async (period, hostId, from, to, dateField = 'createdAt') => {
const normalizedPeriod = period.toLowerCase();
console.log('Generating report for host', hostId, 'from', from, 'to', to, 'dateField', dateField);
const host = await models.Collective.findByPk(toNumber(hostId));
```
```typescript
const host = await models.Collective.findByPk(toNumber(hostId));
assert(['month', 'quarter', 'year'].includes(normalizedPeriod), 'Invalid period, must be month, quarter or year');
```
Anywhere later in this file where `period` is used as the time unit (e.g. `const timeUnit = period;` or in date calculations), replace that usage with `normalizedPeriod` so that all logic relies on the normalized value while still accepting case-insensitive CLI input.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+49
to
+52
| FROM "Transactions" t | ||
| LEFT JOIN LATERAL ( | ||
| SELECT e2."type" FROM "Expenses" e2 WHERE e2.id = t."ExpenseId" | ||
| ) AS e ON t."ExpenseId" IS NOT NULL |
There was a problem hiding this comment.
suggestion: The lateral join for expenses looks more complex than necessary.
Given the correlation WHERE e2.id = t."ExpenseId", the LATERAL isn’t necessary here, and ON t."ExpenseId" IS NOT NULL is unusual. You can keep the same semantics with a simpler left join:
LEFT JOIN "Expenses" e ON e.id = t."ExpenseId"This preserves rows with a null ExpenseId while avoiding the lateral subquery and custom ON clause.
Suggested change
| FROM "Transactions" t | |
| LEFT JOIN LATERAL ( | |
| SELECT e2."type" FROM "Expenses" e2 WHERE e2.id = t."ExpenseId" | |
| ) AS e ON t."ExpenseId" IS NOT NULL | |
| FROM "Transactions" t | |
| LEFT JOIN "Expenses" e ON e.id = t."ExpenseId" |
| .action(async (period, hostId, from, to, dateField = 'createdAt') => { | ||
| console.log('Generating report for host', hostId, 'from', from, 'to', to, 'dateField', dateField); | ||
| const host = await models.Collective.findByPk(toNumber(hostId)); | ||
| assert(['month', 'quarter', 'year'].includes(period), 'Invalid period, must be month, quarter or year'); |
There was a problem hiding this comment.
suggestion: Period validation is case-sensitive and may cause avoidable failures.
Because the allowed period values are lowercase, inputs like Month or YEAR will fail even though they’re reasonable for a CLI. If you want more user-friendly behavior, normalize period (e.g., period = period.toLowerCase()) before the assert and reuse that normalized value for validation and timeUnit.
Suggested implementation:
.command('generate <period> <hostId> <from> <to> [dateField] [env]')
.action(async (period, hostId, from, to, dateField = 'createdAt') => {
const normalizedPeriod = period.toLowerCase();
console.log('Generating report for host', hostId, 'from', from, 'to', to, 'dateField', dateField);
const host = await models.Collective.findByPk(toNumber(hostId)); const host = await models.Collective.findByPk(toNumber(hostId));
assert(['month', 'quarter', 'year'].includes(normalizedPeriod), 'Invalid period, must be month, quarter or year');Anywhere later in this file where period is used as the time unit (e.g. const timeUnit = period; or in date calculations), replace that usage with normalizedPeriod so that all logic relies on the normalized value while still accepting case-insensitive CLI input.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/accounting/generate-host-report.ts`:
- Around line 15-17: The current query function silently treats any
non-'effective' dateField as 'createdAt'; change it to validate dateField
explicitly and throw a clear Error for invalid values. In the query(dateField)
function (and the similar block around the other occurrence), accept only the
allowed tokens (e.g., 'effective' and 'created' or the exact two permitted
strings used across the file), and if dateField is not one of them, throw new
Error('Invalid dateField: must be "effective" or "created"') so callers cannot
silently get the wrong date basis; then map the valid tokens to the SQL
expressions (use COALESCE(...) for 'effective', t."createdAt" for 'created').
- Around line 73-74: The SQL WHERE clause is missing the lower bound so the
CLI's "from" parameter isn't applied; update the queries that filter by
"HostCollectiveId" and "date <= :dateTo" to also include "AND date >= :from" (or
"AND date >= :fromDate" if that is the CLI flag name used) and ensure the query
parameter binding for :from matches the CLI param; apply this change to the two
affected queries (the block using hostCollectiveId/dateTo around the lines shown
and the second occurrence at lines 109-113).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81f74bef-f6c8-4005-9db7-7acab5e0fb1c
📒 Files selected for processing (1)
scripts/accounting/generate-host-report.ts
Comment on lines
+15
to
+17
| const query = dateField => { | ||
| dateField = dateField === 'effective' ? `COALESCE(t."clearedAt", t."createdAt")` : `t."createdAt"`; | ||
| return ` |
There was a problem hiding this comment.
Reject invalid dateField instead of silently defaulting.
Any unexpected dateField currently falls back to createdAt, which can silently produce the wrong report basis.
Proposed fix
.action(async (period, hostId, from, to, dateField = 'createdAt') => {
console.log('Generating report for host', hostId, 'from', from, 'to', to, 'dateField', dateField);
const host = await models.Collective.findByPk(toNumber(hostId));
assert(['month', 'quarter', 'year'].includes(period), 'Invalid period, must be month, quarter or year');
+ assert(['createdAt', 'effective'].includes(dateField), 'Invalid dateField, must be createdAt or effective');
assert(host, 'Host not found');Also applies to: 102-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/accounting/generate-host-report.ts` around lines 15 - 17, The current
query function silently treats any non-'effective' dateField as 'createdAt';
change it to validate dateField explicitly and throw a clear Error for invalid
values. In the query(dateField) function (and the similar block around the other
occurrence), accept only the allowed tokens (e.g., 'effective' and 'created' or
the exact two permitted strings used across the file), and if dateField is not
one of them, throw new Error('Invalid dateField: must be "effective" or
"created"') so callers cannot silently get the wrong date basis; then map the
valid tokens to the SQL expressions (use COALESCE(...) for 'effective',
t."createdAt" for 'created').
Comment on lines
+73
to
+74
| WHERE "HostCollectiveId" = :hostCollectiveId | ||
| AND date <= :dateTo) |
There was a problem hiding this comment.
Apply the from bound in SQL to keep the query scoped.
<from> is accepted by the CLI but never applied in the SQL filter. That makes the query unbounded on the lower side and can scan far more history than needed.
Proposed fix
FROM "HostMonthlyTransactions"
WHERE "HostCollectiveId" = :hostCollectiveId
+ AND date >= :dateFrom
AND date <= :dateTo) const queryResult = await sequelize.query(query(dateField), {
replacements: {
hostCollectiveId: toNumber(hostId),
timeUnit: period,
+ dateFrom: moment(from).utc().toISOString(),
dateTo: moment(to).utc().toISOString(),
},Also applies to: 109-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/accounting/generate-host-report.ts` around lines 73 - 74, The SQL
WHERE clause is missing the lower bound so the CLI's "from" parameter isn't
applied; update the queries that filter by "HostCollectiveId" and "date <=
:dateTo" to also include "AND date >= :from" (or "AND date >= :fromDate" if that
is the CLI flag name used) and ensure the query parameter binding for :from
matches the CLI param; apply this change to the two affected queries (the block
using hostCollectiveId/dateTo around the lines shown and the second occurrence
at lines 109-113).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.