Website: Fetch recipient details on partners page and transparency/finance page from firestore #1020
Conversation
📝 WalkthroughWalkthroughThis pull request introduces comprehensive changes across multiple localization files and components, focusing on recipient statistics and NGO representation. The modifications streamline NGO identifiers, remove recipient-specific metrics from partner data, and introduce a new Changes
Sequence DiagramsequenceDiagram
participant FirestoreAdmin
participant RecipientStatsCalculator
participant NgoList
participant TransparencyPage
FirestoreAdmin->>RecipientStatsCalculator: Retrieve recipient data
RecipientStatsCalculator-->>NgoList: Provide organization stats
RecipientStatsCalculator-->>TransparencyPage: Provide total recipient stats
TransparencyPage->>Section1: Render recipient statistics
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (5)
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…o pranav/dynamic-fetch-recipients-stats
|
Visit the preview URL for this PR (updated for commit 9463af0): https://si-admin-staging--pr1020-pranav-dynamic-fetch-jxgk22fg.web.app (expires Fri, 31 Jan 2025 15:13:27 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
…o pranav/dynamic-fetch-recipients-stats
…b.com/socialincome-san/public into pranav/dynamic-fetch-recipients-stats
DarkMenacer
requested review from
andrashee,
mkue,
ssandino and
socialincome-dev
as code owners
![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
🧹 Nitpick comments (11)
shared/src/utils/stats/RecipientStatsCalculator.ts (2)
1-3: Check lodash usage overhead
Using an entire lodash chain for a straightforward array-based approach may elevate complexity. Consider switching to native arrays for minimal overhead.
48-60: Combine multiple filter calls
Performing repeated.filter(...).size()calls can be consolidated into a single iteration, improving performance.+ const counts = { active: 0, former: 0, suspended: 0 }; + const validRecipients = this.recipients.filter((recipient) => !recipient.test_recipient); + validRecipients.forEach((r) => { + if (r.progr_status === RecipientProgramStatus.Active) counts.active++; + else if (r.progr_status === RecipientProgramStatus.Former) counts.former++; + else if (r.progr_status === RecipientProgramStatus.Suspended) counts.suspended++; + }); + + return { + total: validRecipients.size(), + active: counts.active, + former: counts.former, + suspended: counts.suspended, + };website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx (2)
35-57: Check sum logic
Summingactive,former, andsuspendedfortotalRecipientsmay omit potential future statuses. Consider a fallback or a dynamic approach if new statuses appear.
58-71: Maintain numeric consistency
Ensuring consistent formatting (e.g., rounding or commas) on displayed currency amounts can improve readability.website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/page.tsx (2)
40-46: Ensure clarity in naming
Section1Propsmight benefit from a name reflecting its purpose (e.g.,RecipientsAndContributionsProps).
61-61: Leverage previously fetched data
Destructure your stats only once to clarify the boundaries between data retrieval and component consumption.website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx (1)
50-53: Localize badge labels.
Strings liketranslatorBadgeActive, etc., are empty. For consistency, consider populating them with translated text.shared/locales/en/website-finances.json (1)
13-13: Check capitalization consistency.
Consider matching the casing style of other recipient labels.- "activeRecipients": "{{ value }} Active Recipients" + "activeRecipients": "{{ value }} active recipients"shared/locales/de/website-partners.json (3)
99-99: Consider a more specific firestore ID.The current ID "jamil" might be too generic. Consider using "jnjf" to match the organization's short name.
- "firestore-id": "jamil", + "firestore-id": "jnjf",
190-190: Maintain consistent ID format.Other IDs don't use delimiters. Consider using "equalrights" to match the pattern.
- "firestore-id": "equal_rights", + "firestore-id": "equalrights",
231-231: Maintain consistent ID format.Remove the underscore to match other IDs' pattern.
- "firestore-id": "united_polio", + "firestore-id": "unitedpolio",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
shared/locales/de/website-finances.json(1 hunks)shared/locales/de/website-partners.json(6 hunks)shared/locales/en/website-finances.json(1 hunks)shared/locales/en/website-partners.json(6 hunks)shared/locales/fr/website-finances.json(1 hunks)shared/locales/fr/website-partners.json(6 hunks)shared/locales/it/website-finances.json(1 hunks)shared/locales/it/website-partners.json(6 hunks)shared/src/utils/stats/RecipientStatsCalculator.ts(1 hunks)website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx(2 hunks)website/src/app/[lang]/[region]/(website)/partners/(types)/PartnerCards.ts(1 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/page.tsx(3 hunks)website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx(2 hunks)
🔇 Additional comments (40)
shared/src/utils/stats/RecipientStatsCalculator.ts (3)
33-46: Handle potential missing organization document
When fetchingorganisationSnapshot, ensure thatrecipientData.data().organisationis defined and that the referenced document exists. Otherwise, you risk runtime errors or inconsistent data.
61-88: Clarify zero organization case
Returning zeroes for all status counts if noorgIdis provided is logical, but confirm that this placeholder aligns with your business requirements.
90-94: Straightforward aggregation
TheallStatsmethod directly composes statistics by organization or overall recipients. Looks good.website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/section-1.tsx (2)
5-9: Protect against negative amounts
YourroundAmountfunction handles zero but not negative values. If negatives are possible, handle them similarly.
26-34: Review transitions
The hover scale effect is functional. However, confirm that the added UI interactions (e.g., "hover:scale-[101%]") do not conflict with your design language or accessibility.website/src/app/[lang]/[region]/(website)/transparency/finances/[currency]/page.tsx (3)
12-15: Module imports
The new imports look correct and facilitate fetching recipient data.
58-58: Good practice
Returning multiple stats as a single object keeps your code organization flexible.
71-71: Prop alignment
ProvidingrecipientStatsensures the child component can leverage dynamic stats. Seems consistent withSection1Props.website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx (3)
13-13: No issues found on this import.
15-15: Verify server-side usage.
EnsureRecipientStatsCalculatorusage doesn’t cause issues when rendering on the client.
46-46: Confirm synchronous vs asynchronous behavior.
EnsureallStatsis meant to be called synchronously, or addawaitif it’s async.shared/locales/en/website-finances.json (1)
12-12: Revised emphasis on empowerment is clear.shared/locales/de/website-finances.json (2)
12-12: Clarity improved by focusing on support.
13-13: Inclusive language looks good.shared/locales/fr/website-finances.json (2)
12-12: Clear transition from “engagés” to “soutenus.”
13-13: New active recipients key is consistent.shared/locales/it/website-finances.json (1)
12-13: Verify translations across all language files.The Italian translations look accurate. Let's ensure consistency across other language files.
✅ Verification successful
Translations are consistent across all language files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check translations for totalRecipients and activeRecipients across all language files # Expected: Similar semantic meaning across languages echo "Checking translations across language files..." for lang in de en fr it; do echo -e "\n=== $lang ===" rg '"totalRecipients"|"activeRecipients"' "shared/locales/$lang/website-finances.json" doneLength of output: 1252
website/src/app/[lang]/[region]/(website)/partners/(types)/PartnerCards.ts (1)
46-46: Add optional Firestore ID property
This optional property aligns well with Firestore integration. Confirm it's consistently referenced in the codebase, and consider removing recipient fields if they’re no longer used.shared/locales/en/website-partners.json (6)
44-44: Add "firestore-id" for Aurora
Great addition for Firestore reference. Ensure consistency with other locales.
100-100: Add "firestore-id" for Jamil
The unique ID is correct. Confirm matching doc names in Firestore.
144-144: Add "firestore-id" for Reachout
No concerns. Make sure the name is spelled correctly in Firestore.
186-186: Add "firestore-id" for Equal Rights
Looks good. Keep naming consistent across locales.
227-227: Add "firestore-id" for United Polio
Perfectly fine. Mind underscores in Firestore doc IDs if needed.
267-267: Add "firestore-id" for SLAES
Clear addition. Validate correctness in your Firestore setup.shared/locales/it/website-partners.json (6)
44-44: Add "firestore-id": "aurora"
Consistent with the type definition. Looks fine.
99-99: Add "firestore-id": "jamil"
No issues. Confirm Firestore alignment.
143-143: Add "firestore-id": "reachout"
ID naming is consistent. Good job.
185-185: Add "firestore-id": "equal_rights"
Underscore is okay if it matches Firestore doc IDs.
226-226: Add "firestore-id": "united_polio"
Everything seems in order.
266-266: Add "firestore-id": "slaes"
Check that it aligns with Firestore’s actual doc name.shared/locales/fr/website-partners.json (6)
44-44: Add "firestore-id": "aurora"
Clean insertion. Make sure all references are spelled identically.
99-99: Add "firestore-id": "jamil"
Looks consistent. Ensure identical naming across languages.
143-143: Add "firestore-id": "reachout"
All good. Validate Firestore references.
185-185: Add "firestore-id": "equal_rights"
No issues. Confirm underscore usage if needed in Firestore.
226-226: Add "firestore-id": "united_polio"
Looks correct. The underscore is acceptable as long as it’s consistent.
266-266: Add "firestore-id": "slaes"
Works well. Double-check for matching doc name.shared/locales/de/website-partners.json (4)
44-44: LGTM! Well-structured firestore ID.The ID is simple, lowercase, and clearly identifies the organization.
143-143: LGTM! Clear and consistent ID.
271-271: LGTM! Well-chosen ID using the organization's acronym.
Line range hint
44-271: Verify firestore ID uniqueness across all collections.While the IDs are well-chosen, ensure they don't conflict with IDs in other Firestore collections.
✅ Verification successful
Firestore IDs are properly integrated and used in the system
The firestore-ids in website-partners.json are verified to be valid and match actual database usage, as evidenced by their integration in the backend code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for firestore IDs across the codebase to check for uniqueness echo "Searching for firestore IDs..." rg -i "firestore-id.*(?:aurora|jamil|reachout|equal_rights|united_polio|slaes)" --type jsonLength of output: 1844
Script:
#!/bin/bash # Search for Firestore-related files and collection definitions echo "Searching for Firestore collection references..." rg -i "firestore|collection.*partners" --type ts --type js -A 2 # Also check for any configuration files echo -e "\nSearching for Firestore config files..." fd -e ts -e js -e json firestoreLength of output: 66342
Script:
#!/bin/bash # Search for partner organization collection references and ID usage echo "Searching for partner org collections and ID usage..." rg -i "partner.*organization.*collection|partner.*organisation.*collection" --type ts --type js -A 2 echo -e "\nSearching for specific firestore IDs in TypeScript/JavaScript files..." rg -i "aurora|jamil|reachout|equal_rights|united_polio|slaes" --type ts --type js -A 2Length of output: 2764
| const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin); | ||
| const recipientStats = recipientCalculator.allStats(); |
There was a problem hiding this comment.
Add error handling
Consider wrapping the build steps in a try/catch block to handle Firestore or network issues more gracefully.
+ let recipientStats;
+ try {
+ const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin);
+ recipientStats = recipientCalculator.allStats();
+ } catch (err) {
+ // handle or log error
+ recipientStats = { totalRecipients: {}, totalRecipientsByOrganization: {} };
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin); | |
| const recipientStats = recipientCalculator.allStats(); | |
| let recipientStats; | |
| try { | |
| const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin); | |
| recipientStats = recipientCalculator.allStats(); | |
| } catch (err) { | |
| // handle or log error | |
| recipientStats = { totalRecipients: {}, totalRecipientsByOrganization: {} }; | |
| } |
| const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for Firestore.
Consider wrapping this call in a try/catch block to gracefully handle potential Firestore errors.
- const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin);
+ let recipientCalculator;
+ try {
+ recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin);
+ } catch (error) {
+ console.error("Error building recipient calculator:", error);
+ // Handle or rethrow the error as needed
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin); | |
| let recipientCalculator; | |
| try { | |
| recipientCalculator = await RecipientStatsCalculator.build(firestoreAdmin); | |
| } catch (error) { | |
| console.error("Error building recipient calculator:", error); | |
| // Handle or rethrow the error as needed | |
| } |
| @@ -283,6 +268,7 @@ | |||
| "org-photo": "" | |||
| }, | |||
| "sierra-leone-association-of-ebola-survivors": { | |||
| "firestore-id": "slaes", | |||
There was a problem hiding this comment.
A key that is the same in every translation file, seems wrong. This file should be for strings that differ across languages.
Summary by CodeRabbit
Localization Updates
Partner Management
Data Calculation
User Interface