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
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 ↗︎ 1 Skipped Deployment
|
…o pranav/dynamic-fetch-recipients-stats
|
Visit the preview URL for this PR (updated for commit efe8dc3): https://si-admin-staging--pr1020-pranav-dynamic-fetch-jxgk22fg.web.app (expires Sat, 08 Feb 2025 11:04:12 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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
shared/src/utils/stats/RecipientStatsCalculator.test.ts (2)
14-16: Consider using dynamic project IDs for test isolation.The hardcoded project ID could cause conflicts in parallel test runs. Consider generating unique IDs per test run.
-const projectId = 'contribution-stats-calculator-test'; +const projectId = `contribution-stats-calculator-test-${Date.now()}`;
75-87: Add error handling to test data insertion.The insertion helper should handle potential Firestore errors and verify successful insertion.
const insertTestData = async () => { + try { await firestoreAdmin .collection<PartnerOrganisation>(PARTNER_ORGANISATION_FIRESTORE_PATH) .doc('socialincome') .set(org1); + const orgDoc = await firestoreAdmin + .collection(PARTNER_ORGANISATION_FIRESTORE_PATH) + .doc('socialincome') + .get(); + if (!orgDoc.exists) { + throw new Error('Failed to insert organization'); + } // ... rest of the function + } catch (error) { + console.error('Failed to insert test data:', error); + throw error; + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shared/src/utils/stats/RecipientStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/RecipientStatsCalculator.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- shared/src/utils/stats/RecipientStatsCalculator.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test admin
- GitHub Check: Test functions
- GitHub Check: Test website
- GitHub Check: Test shared code
- GitHub Check: Security checks (typescript)
- GitHub Check: Prettify
| test('totalRecipients(): Calculate total recipients', async () => { | ||
| expect(calculator.allStats().totalRecipients.total).toEqual(1); | ||
| }); | ||
|
|
||
| test('totalRecipients(): Calculate active recipients', async () => { | ||
| expect(calculator.allStats().totalRecipients.active).toEqual(1); | ||
| }); | ||
|
|
||
| test('totalRecipientsByOrganization(): Calculate total recipients for a particular organisation', async () => { | ||
| expect(calculator.allStats('socialincome').totalRecipientsByOrganization.total).toEqual(1); | ||
| }); | ||
|
|
||
| test('totalRecipientsByOrganization(): Calculate active recipients for a particular organisation', async () => { | ||
| expect(calculator.allStats('socialincome').totalRecipientsByOrganization.active).toEqual(1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for edge scenarios.
Current tests only cover the happy path. Consider adding tests for:
- Empty database
- Multiple organizations
- Invalid organization IDs
- Inactive recipients
| beforeAll(async () => { | ||
| await testEnv.firestore.clearFirestoreData({ projectId: projectId }); | ||
| await insertTestData(); | ||
| calculator = await RecipientStatsCalculator.build(firestoreAdmin); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup after tests.
Add an afterAll hook to clean up the test data and ensure proper test isolation.
+afterAll(async () => {
+ await testEnv.firestore.clearFirestoreData({ projectId: projectId });
+ await testEnv.cleanup();
+});📝 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.
| beforeAll(async () => { | |
| await testEnv.firestore.clearFirestoreData({ projectId: projectId }); | |
| await insertTestData(); | |
| calculator = await RecipientStatsCalculator.build(firestoreAdmin); | |
| }); | |
| beforeAll(async () => { | |
| await testEnv.firestore.clearFirestoreData({ projectId: projectId }); | |
| await insertTestData(); | |
| calculator = await RecipientStatsCalculator.build(firestoreAdmin); | |
| }); | |
| afterAll(async () => { | |
| await testEnv.firestore.clearFirestoreData({ projectId: projectId }); | |
| await testEnv.cleanup(); | |
| }); |
| const recipient1: Recipient = { | ||
| birth_date: new Date('1990-05-15'), | ||
| calling_name: 'John', | ||
| communication_mobile_phone: { | ||
| phone: 1234567890, | ||
| has_whatsapp: true, | ||
| whatsapp_activated: true, | ||
| }, | ||
| email: '[email protected]', | ||
| first_name: 'John', | ||
| gender: 'male', | ||
| insta_handle: '@john_doe', | ||
| last_name: 'Doe', | ||
| main_language: RecipientMainLanguage.English, | ||
| mobile_money_phone: { | ||
| phone: 9876543210, | ||
| has_whatsapp: true, | ||
| }, | ||
| organisation: { id: 'socialincome' } as DocumentReference<PartnerOrganisation>, | ||
| om_uid: 12345, | ||
| profession: 'Software Engineer', | ||
| progr_status: RecipientProgramStatus.Active, | ||
| si_start_date: new Timestamp(1609459200, 0), | ||
| test_recipient: false, | ||
| twitter_handle: '@john_doe_tech', | ||
| successor: 'Jane Doe', | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using PII in test data.
Replace personal information with clearly fake test data to avoid any potential PII concerns.
- email: '[email protected]',
+ email: '[email protected]',
- communication_mobile_phone: {
- phone: 1234567890,
+ communication_mobile_phone: {
+ phone: 5555555555,Also, consider using relative timestamps instead of hardcoded ones for better test maintainability.
Committable suggestion skipped: line range outside the PR's diff.
| */ | ||
| static async build(firestoreAdmin: FirestoreAdmin): Promise<RecipientStatsCalculator> { | ||
| const completeRecipientsData = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | ||
| const recipientStatsEntries = await Promise.all( |
There was a problem hiding this comment.
please filter out the test_recipients. This way we have less complex filtering.
| return new RecipientStatsCalculator(_(recipientStatsEntries)); | ||
| } | ||
|
|
||
| totalRecipients = (): TotalRecipientsByStatus => ({ |
There was a problem hiding this comment.
please use _.groupBy here to simplify the grouping of the recipientProgramStatus
| "sierra-leone-association-of-ebola-survivors" | ||
| ], | ||
| "aurora-foundation": { | ||
| "ngos": ["aurora", "jamil", "reachout", "equal-rights", "united-polio", "slaes"], |
There was a problem hiding this comment.
Please move this to a dedicated .ts file (shared module) as an enum.
| }; | ||
| }; | ||
|
|
||
| allStats = (orgName?: string): RecipientStats => ({ |
There was a problem hiding this comment.
Please make all the calculations of the organisation on this class. Then you can return a dictionary with the results already calculated to the calling component.
| @@ -0,0 +1,87 @@ | |||
| import { DocumentReference, Timestamp } from 'firebase-admin/firestore'; | |||
There was a problem hiding this comment.
Please enrich this text with extra recipients and some edge cases.
…o pranav/dynamic-fetch-recipients-stats
…b.com/socialincome-san/public into pranav/dynamic-fetch-recipients-stats
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
shared/src/types/recipient.ts (1)
49-49: Consider using an enum for NGO identifiers.Replace the string array with an enum to provide type safety and prevent typos.
-export const recipientNGOs = ['aurora', 'jamil', 'reachout', 'equal-rights', 'united-polio', 'slaes']; +export enum RecipientNGO { + Aurora = 'aurora', + Jamil = 'jamil', + Reachout = 'reachout', + EqualRights = 'equal-rights', + UnitedPolio = 'united-polio', + Slaes = 'slaes' +} +export const recipientNGOs = Object.values(RecipientNGO);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
shared/locales/de/website-partners.json(6 hunks)shared/locales/en/website-partners.json(6 hunks)shared/locales/fr/website-partners.json(6 hunks)shared/locales/it/website-partners.json(6 hunks)shared/src/types/recipient.ts(1 hunks)shared/src/utils/stats/RecipientStatsCalculator.test.ts(1 hunks)shared/src/utils/stats/RecipientStatsCalculator.ts(1 hunks)website/src/app/[lang]/[region]/(website)/partners/(components)/PartnerBadges.tsx(1 hunks)website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- shared/src/utils/stats/RecipientStatsCalculator.test.ts
- shared/locales/de/website-partners.json
- shared/locales/fr/website-partners.json
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test functions
- GitHub Check: Test website
- GitHub Check: Test admin
- GitHub Check: Test shared code
- GitHub Check: Security checks (typescript)
- GitHub Check: Prettify
🔇 Additional comments (13)
website/src/app/[lang]/[region]/(website)/partners/(components)/PartnerBadges.tsx (1)
103-107: LGTM!The simplification of FundraiserBadge by removing the HoverCard improves component clarity.
shared/locales/en/website-partners.json (6)
Line range hint
35-89: NGO 'aurora' Block Update
The "aurora" block now uses a simplified identifier and omits recipient metrics, which aligns with the updated data model. Ensure that all references to the old keys (such as detailed recipient statistics) are updated in the application.
Line range hint
90-132: NGO 'jamil' Block Update
The "jamil" block has been updated with a shortened identifier and now focuses solely on core organizational data. The removal of recipient-specific fields is consistent with the new design.
Line range hint
133-173: NGO 'reachout' Block Update
The "reachout" block reflects the streamlined naming convention and reduced data set by removing recipient-related metrics. Confirm that the partners page fetches the updated keys correctly.
Line range hint
174-213: NGO 'equal-rights' Block Update
The "equal-rights" block follows the new simplified structure, dropping non-essential recipient statistics. Verify that any component relying on these fields is now aligned with the updated data model.
Line range hint
214-252: NGO 'united-polio' Block Update
The "united-polio" block is updated to use a concise identifier and focus on essential organizational details only. This change supports the overall objective of streamlining NGO representations.
Line range hint
253-277: NGO 'slaes' Block Update
The "slaes" block now adheres to the new naming convention and has eliminated recipient metrics. Ensure any downstream usage (such as in dynamic listing components) is updated accordingly.shared/locales/it/website-partners.json (6)
Line range hint
35-88: Aggiornamento Blocco NGO 'aurora'
Il blocco "aurora" utilizza ora un identificatore semplificato e non include i campi relativi ai beneficiari, in linea con il nuovo modello dati. Verifica che i riferimenti precedenti siano aggiornati in tutta l’applicazione.
Line range hint
89-131: Aggiornamento Blocco NGO 'jamil'
Il blocco "jamil" presenta il nuovo identificatore abbreviato e contiene solo le informazioni essenziali sull’organizzazione. La rimozione dei dettagli sui beneficiari è coerente con l’obiettivo di semplificazione.
Line range hint
132-172: Aggiornamento Blocco NGO 'reachout'
Il blocco "reachout" rispetta la nuova struttura dati, eliminando i campi non essenziali relativi ai beneficiari. Assicurati che il front-end utilizzi i nuovi identificatori aggiornati.
Line range hint
173-212: Aggiornamento Blocco NGO 'equal-rights'
Il blocco "equal-rights" segue la convenzione di denominazione semplificata ed esclude i campi relativi ai beneficiari. Verifica che ogni componente che si basa su questi dati utilizzi le nuove chiavi.
Line range hint
213-251: Aggiornamento Blocco NGO 'united-polio'
Il blocco "united-polio" è stato aggiornato per riflettere il nuovo approccio minimalista, concentrandosi sui dati essenziali dell’organizzazione.
Line range hint
252-277: Aggiornamento Blocco NGO 'slaes'
Il blocco "slaes" adotta un identificatore conciso ed esclude i dati relativi ai beneficiari, in linea con gli obiettivi di semplificazione. Verifica la coerenza con le altre versioni locali.
| static async build(firestoreAdmin: FirestoreAdmin): Promise<RecipientStatsCalculator> { | ||
| const completeRecipientsData = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | ||
| const recipientStatsEntries = await Promise.all( | ||
| completeRecipientsData.docs | ||
| .filter((recipientData) => !recipientData.data().test_recipient) | ||
| .map(async (recipientData) => { | ||
| const organisationSnapshot = await recipientData.data().organisation?.get(); | ||
| return { | ||
| progr_status: recipientData.data().progr_status, | ||
| organisation: organisationSnapshot?.id, | ||
| }; | ||
| }), | ||
| ); | ||
| return new RecipientStatsCalculator(_(recipientStatsEntries)); | ||
| } |
There was a problem hiding this comment.
Add error handling for Firestore operations.
The Firestore operations could fail and should be handled gracefully.
static async build(firestoreAdmin: FirestoreAdmin): Promise<RecipientStatsCalculator> {
+ try {
const completeRecipientsData = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get();
const recipientStatsEntries = await Promise.all(
completeRecipientsData.docs
.filter((recipientData) => !recipientData.data().test_recipient)
.map(async (recipientData) => {
const organisationSnapshot = await recipientData.data().organisation?.get();
return {
progr_status: recipientData.data().progr_status,
organisation: organisationSnapshot?.id,
};
}),
);
return new RecipientStatsCalculator(_(recipientStatsEntries));
+ } catch (error) {
+ console.error('Failed to build RecipientStatsCalculator:', error);
+ throw new Error('Failed to retrieve recipient statistics');
+ }
}📝 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.
| static async build(firestoreAdmin: FirestoreAdmin): Promise<RecipientStatsCalculator> { | |
| const completeRecipientsData = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | |
| const recipientStatsEntries = await Promise.all( | |
| completeRecipientsData.docs | |
| .filter((recipientData) => !recipientData.data().test_recipient) | |
| .map(async (recipientData) => { | |
| const organisationSnapshot = await recipientData.data().organisation?.get(); | |
| return { | |
| progr_status: recipientData.data().progr_status, | |
| organisation: organisationSnapshot?.id, | |
| }; | |
| }), | |
| ); | |
| return new RecipientStatsCalculator(_(recipientStatsEntries)); | |
| } | |
| static async build(firestoreAdmin: FirestoreAdmin): Promise<RecipientStatsCalculator> { | |
| try { | |
| const completeRecipientsData = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); | |
| const recipientStatsEntries = await Promise.all( | |
| completeRecipientsData.docs | |
| .filter((recipientData) => !recipientData.data().test_recipient) | |
| .map(async (recipientData) => { | |
| const organisationSnapshot = await recipientData.data().organisation?.get(); | |
| return { | |
| progr_status: recipientData.data().progr_status, | |
| organisation: organisationSnapshot?.id, | |
| }; | |
| }), | |
| ); | |
| return new RecipientStatsCalculator(_(recipientStatsEntries)); | |
| } catch (error) { | |
| console.error('Failed to build RecipientStatsCalculator:', error); | |
| throw new Error('Failed to retrieve recipient statistics'); | |
| } | |
| } |
website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
shared/src/utils/stats/RecipientStatsCalculator.ts (2)
36-50:⚠️ Potential issueAdd error handling for Firestore operations.
The Firestore operations could fail and should be handled gracefully.
static async build(firestoreAdmin: FirestoreAdmin): Promise<RecipientStatsCalculator> { + try { const completeRecipientsData = await firestoreAdmin.collection<Recipient>(RECIPIENT_FIRESTORE_PATH).get(); const recipientStatsEntries = await Promise.all( completeRecipientsData.docs .filter((recipientData) => !recipientData.data().test_recipient) .map(async (recipientData) => { const organisationSnapshot = await recipientData.data().organisation?.get(); return { progr_status: recipientData.data().progr_status, organisation: organisationSnapshot?.id, }; }), ); return new RecipientStatsCalculator(_(recipientStatsEntries)); + } catch (error) { + console.error('Failed to build RecipientStatsCalculator:', error); + throw new Error('Failed to retrieve recipient statistics'); + } }
62-75:⚠️ Potential issueReplace
anytype with proper interface.Using
anyreduces type safety. Define a proper type for the object.-const orgRecipientsObject: any = {}; +const orgRecipientsObject: OrganisationRecipientsByStatus = {};
🧹 Nitpick comments (1)
shared/src/utils/stats/RecipientStatsCalculator.ts (1)
5-27: Add comprehensive documentation for types and interfaces.Consider adding detailed JSDoc comments for
RecipientStats,TotalRecipientsByStatus, andOrganisationRecipientsByStatusto improve code maintainability.+/** + * Represents aggregated statistics for recipients, including total counts and organization-specific counts. + */ export interface RecipientStats { totalRecipients: TotalRecipientsByStatus; totalRecipientsByOrganization: OrganisationRecipientsByStatus; } +/** + * Represents recipient counts categorized by their program status. + */ export type TotalRecipientsByStatus = { total: number; active: number; former: number; suspended: number; }; +/** + * Maps organization IDs to their respective recipient statistics. + */ export type OrganisationRecipientsByStatus = { [orgId: string]: TotalRecipientsByStatus; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
shared/src/utils/stats/RecipientStatsCalculator.ts(1 hunks)website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/src/app/[lang]/[region]/(website)/partners/(sections)/ngolist.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test shared code
- GitHub Check: Test functions
- GitHub Check: Test website
- GitHub Check: Test admin
- GitHub Check: Prettify
- GitHub Check: Security checks (typescript)
🔇 Additional comments (1)
shared/src/utils/stats/RecipientStatsCalculator.ts (1)
29-31: LGTM!The constructor is well-implemented with proper immutability using
readonly.
Summary by CodeRabbit
Localization Updates
Partner Management
Data Calculation
User Interface
FundraiserBadgeto simplify user interaction by removing hover functionality.