Bugfix: Make sure amount displayed on campaign page is dynamically calculated #1008
Conversation
mkue
requested review from
andrashee,
ssandino and
socialincome-dev
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces changes to campaign-related configurations and data handling across multiple files. The modifications primarily involve renaming properties from Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
shared/src/types/contribution.ts (1)
33-34: Consider enhancing fee documentation.While the comments are clear, consider documenting:
- Types of fees included
- Whether fees are percentage-based or fixed
admin/src/collections/Campaigns.ts (1)
Line range hint
37-37: Fix typo in property nameThere's a typo in "Thrird Description Title" that should be corrected.
- name: 'Thrird Description Title', + name: 'Third Description Title',website/src/app/[lang]/[region]/(website)/campaign/[campaign]/page.tsx (2)
93-93: Simplify 'amountCollected' calculation and handle potential undefined valuesThe
?? 0is unnecessary since thereducefunction starts with an initial value of0. Additionally, consider handling cases whereamount_chfmight beundefined.You can simplify the calculation:
- const amountCollected = (contributions.docs.reduce((sum, c) => sum + c.data().amount_chf, 0) ?? 0) * exchangeRate; + const amountCollected = contributions.docs.reduce((sum, c) => sum + (c.data().amount_chf || 0), 0) * exchangeRate;
129-129: Ensure consistent use of 'contributions.size' for countsYou have updated the
countparameter to usecontributions.size. Verify that all other instances wherecontributionsrepresents the count are also usingcontributions.sizeto prevent any inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
admin/src/collections/Campaigns.ts(2 hunks)shared/src/stripe/StripeEventHandler.ts(0 hunks)shared/src/types/campaign.ts(0 hunks)shared/src/types/contribution.ts(1 hunks)website/src/app/[lang]/[region]/(website)/campaign/[campaign]/page.tsx(3 hunks)
💤 Files with no reviewable changes (2)
- shared/src/types/campaign.ts
- shared/src/stripe/StripeEventHandler.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test admin
- GitHub Check: Test shared code
- GitHub Check: Test website
- GitHub Check: Test functions
- GitHub Check: Security checks (typescript)
- GitHub Check: Prettify
🔇 Additional comments (6)
shared/src/types/contribution.ts (2)
42-42: LGTM! Helpful API documentation link.The Stripe API reference link improves code maintainability.
25-27: LGTM! Clear documentation of the contribution model.The BaseContribution type documentation clearly explains how the net amount is calculated.
Let's verify this calculation is used consistently:
✅ Verification successful
Documentation accurately reflects the contribution model
The fields
amount_chfandfees_chfare consistently used across the codebase, with proper handling in payment processing, statistics calculation, and test cases.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for places where amount_chf and fees_chf are used together rg -l 'amount_chf.*fees_chf|fees_chf.*amount_chf'Length of output: 84
Script:
#!/bin/bash # Search for actual calculations or assignments involving these fields ast-grep --pattern 'amount_chf - fees_chf' ast-grep --pattern 'fees_chf + $_' ast-grep --pattern '$_ = amount_chf' # Also search for any references to these fields in function bodies rg -A 2 -B 2 '\b(amount_chf|fees_chf)\b' --type tsLength of output: 11014
admin/src/collections/Campaigns.ts (3)
72-72: LGTM: Property renames improve consistencyThe renaming of properties from 'title' to 'name' for social media links standardizes the naming convention.
Also applies to: 78-78, 84-84, 90-90, 96-96
145-145: LGTM: Validation message property updateThe update from
matchMessagetomatchesMessagealigns with the API requirements.
Line range hint
1-164: Verify dynamic amount calculation implementationThe removal of
amount_collected_chfandcontributionsproperties suggests these values will be calculated dynamically. However, I don't see the implementation for this dynamic calculation in the provided changes.Let's verify where these calculations are implemented:
✅ Verification successful
Dynamic calculation implementation verified
The removed properties
amount_collected_chfandcontributionsare now dynamically calculated through Firestore queries, summing theamount_chffrom all related contributions and applying current exchange rates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files that might contain the dynamic amount calculation implementation rg -l "amount.*collected" --type ts rg -l "contribution.*total" --type tsLength of output: 713
Script:
#!/bin/bash # Search for campaign-related calculation implementations rg -l "campaign.*contribution" --type ts rg -l "campaign.*amount" --type ts # Look for potential API endpoints or database queries ast-grep --pattern 'export async function $FUNC($$$) { $$$ campaign$$$ $$$ }'Length of output: 236
Script:
#!/bin/bash # Check campaign page implementation cat website/src/app/[lang]/[region]/\(website\)/campaign/[campaign]/page.tsx # Search for Firestore/database related files fd -e ts firestore rg -l "getContributions|getCampaign" --type tsLength of output: 13540
website/src/app/[lang]/[region]/(website)/campaign/[campaign]/page.tsx (1)
9-9: Import statement is correctThe import of the
Contributiontype is appropriate.
website/src/app/[lang]/[region]/(website)/campaign/[campaign]/page.tsx
Outdated
Show resolved
Hide resolved
|
Visit the preview URL for this PR (updated for commit 12dd4d6): https://si-admin-staging--pr1008-michael-campaign-sta-ursb8pkw.web.app (expires Sun, 19 Jan 2025 20:29:49 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/src/app/[lang]/[region]/(website)/campaign/[campaign]/page.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CodeQL
- GitHub Check: Test website
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (2)
website/src/app/[lang]/[region]/(website)/campaign/[campaign]/page.tsx (2)
9-9: LGTM: Import additions are appropriateThe new imports support the dynamic contribution calculation implementation.
89-91: Verify campaign_path field comparisonThe Firestore query compares 'campaign_path' with a DocumentReference. Ensure the field is stored as a DocumentReference in Firestore.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
amount_collected_chfandcontributionsproperties from campaign data model.Documentation
Chores