Add single receipt platform tip setting - #11792
Conversation
|
Warning Review limit reached
More reviews will be available in 6 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates getConsolidatedInvoicesData to fetch transaction kind and TransactionGroup, build a contribution-by-group lookup, and when iterating transactions skip PLATFORM_TIP entries if the related contribution’s host has settings.singleReceiptPlatformTip enabled. It also refactors invoice slug generation to read host.slug via a cached host lookup. Tests were added to create a platform-tip transaction and assert the contributor can download its receipt while the host admin cannot. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d90f889 to
64db4fb
Compare
64db4fb to
69f21cc
Compare
… of a feature Replace the SINGLE_RECEIPT_PLATFORM_TIP feature (allowed-features + CollectiveFeatures GraphQL type) with a plain host setting, settings.singleReceiptPlatformTip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15bc718b36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/lib/pdf.ts (1)
67-85: ⚡ Quick winBatch-load contribution hosts instead of querying them one-by-one in the invoice loop.
Each
PLATFORM_TIPentry does its ownCollective.findByPkand awaits it serially. For accounts with many historical tips, this turns consolidated receipt generation into a linear sequence of extra DB round-trips. Preloading the uniquerelatedContribution.HostCollectiveIdvalues alongsidecontributionByGroup(or extending the existing host cache to includesettings) keeps the loop in-memory.♻️ One way to preload the host settings once
const platformTipGroups = transactions .filter(t => t.kind === TransactionKind.PLATFORM_TIP) .map(t => t.TransactionGroup); let contributionByGroup = {}; + let contributionHostsById = {}; if (platformTipGroups.length) { const contributions = await models.Transaction.findAll({ attributes: ['HostCollectiveId', 'TransactionGroup'], where: { TransactionGroup: platformTipGroups, kind: TransactionKind.CONTRIBUTION, type: 'CREDIT', }, }); contributionByGroup = contributions.reduce((result, contribution) => { result[contribution.TransactionGroup] = contribution; return result; }, {}); + + const contributionHostIds = [...new Set(contributions.map(c => c.HostCollectiveId).filter(Boolean))]; + const contributionHosts = await models.Collective.findAll({ + attributes: ['id', 'settings'], + where: { id: contributionHostIds }, + }); + contributionHostsById = contributionHosts.reduce((result, host) => { + result[host.id] = host; + return result; + }, {}); } for (const transaction of transactions) { if (transaction.kind === TransactionKind.PLATFORM_TIP) { const relatedContribution = contributionByGroup[transaction.TransactionGroup]; if (relatedContribution?.HostCollectiveId) { - const contributionHost = await models.Collective.findByPk(relatedContribution.HostCollectiveId); + const contributionHost = contributionHostsById[relatedContribution.HostCollectiveId]; if (contributionHost && get(contributionHost, 'settings.singleReceiptPlatformTip') === true) { continue; } } }Also applies to: 100-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/lib/pdf.ts` around lines 67 - 85, The code currently collects PLATFORM_TIP TransactionGroup values into platformTipGroups and builds contributionByGroup via models.Transaction.findAll, but later still calls Collective.findByPk inside the invoice loop for each relatedContribution.HostCollectiveId; change this to batch-load all unique HostCollectiveId values after building contributionByGroup: extract unique HostCollectiveId from the contributions (contribution.HostCollectiveId), call models.Collective.findAll({ where: { id: hostIds }, attributes: ['id','settings', ...] }) once, build a map keyed by id (hostById) and use that map in place of Collective.findByPk inside the loop so the invoice loop uses the in-memory host objects (and their settings) instead of serial DB calls; ensure you reference platformTipGroups, contributionByGroup, models.Transaction.findAll, contribution.HostCollectiveId and replace Collective.findByPk usages accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/lib/pdf.ts`:
- Around line 67-85: The code currently collects PLATFORM_TIP TransactionGroup
values into platformTipGroups and builds contributionByGroup via
models.Transaction.findAll, but later still calls Collective.findByPk inside the
invoice loop for each relatedContribution.HostCollectiveId; change this to
batch-load all unique HostCollectiveId values after building
contributionByGroup: extract unique HostCollectiveId from the contributions
(contribution.HostCollectiveId), call models.Collective.findAll({ where: { id:
hostIds }, attributes: ['id','settings', ...] }) once, build a map keyed by id
(hostById) and use that map in place of Collective.findByPk inside the loop so
the invoice loop uses the in-memory host objects (and their settings) instead of
serial DB calls; ensure you reference platformTipGroups, contributionByGroup,
models.Transaction.findAll, contribution.HostCollectiveId and replace
Collective.findByPk usages accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca6179c3-bfde-49c7-b1af-3f1ffce0a8b6
📒 Files selected for processing (2)
server/lib/pdf.tstest/server/graphql/common/transactions.test.js
…ount Transaction.createPlatformTipTransactions sets a platform tip's CollectiveId and HostCollectiveId to the platform account, not the contribution's host. The fixture pointed them at the contributed collective and its host, so the host-admin assertion passed for the wrong reason. Host the fixture on the platform account and assert that a contribution-host admin cannot download the standalone platform tip receipt (only the payer can). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
PDF: opencollective/opencollective-pdf#1234