General: Fix donation certificates #1021
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 ↗︎ 1 Skipped Deployment
|
📝 WalkthroughWalkthroughThis pull request involves a comprehensive refactoring of donation certificates functionality, removing Twilio-related components, and updating storage and file handling mechanisms. The changes primarily focus on improving donation certificate generation, storage, and download processes across multiple components of the application. Changes
Assessment against linked issues
The changes directly address the issue by:
✨ Finishing Touches
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: 2
🔭 Outside diff range comments (1)
functions/src/lib/donation-certificates/DonationCertificatePDFTemplate.tsx (1)
Line range hint
48-56: Add error handling around PDF rendering.Wrap the call to
ReactPDF.renderin a try/catch to avoid uncaught errors if rendering fails.-export const renderDonationCertificatePDFTemplate = async (user: User, year: number, path: string) => { - const translator = await Translator.getInstance({ - language: user.language || 'de', - namespaces: ['donation-certificate', 'countries'], - }); - ReactPDF.render(<DonationCertificatePDFTemplate user={user} year={year} translator={translator} />, path); -}; +export const renderDonationCertificatePDFTemplate = async (user: User, year: number, path: string) => { + try { + const translator = await Translator.getInstance({ + language: user.language || 'de', + namespaces: ['donation-certificate', 'countries'], + }); + await ReactPDF.render(<DonationCertificatePDFTemplate user={user} year={year} translator={translator} />, path); + } catch (error) { + console.error('Error rendering PDF', error); + throw error; + } +};
🧹 Nitpick comments (15)
ui/src/components/typography/typography.tsx (1)
21-21: Consider reordering 'semibold' to reflect proper weight hierarchy.
Currently,'semibold'is placed after'bold', though semibold is lighter than bold. An updated order might be['normal', 'medium', 'semibold', 'bold'].-const FONT_WEIGHTS = ['normal', 'medium', 'bold', 'semibold'] as const; +const FONT_WEIGHTS = ['normal', 'medium', 'semibold', 'bold'] as const;functions/src/firebase.ts (1)
202-202: Refactor suggestion for repeated document references.
Each organization reference uses'organisations', 'aurora'inline. Centralizing this into a constant or helper can improve maintainability and reduce duplication.- organisation: firestoreAdmin.doc<PartnerOrganisation>('organisations', 'aurora'), + const auroraRef = firestoreAdmin.doc<PartnerOrganisation>('organisations', 'aurora'); + organisation: auroraRef,Also applies to: 216-216, 230-230, 244-244, 258-258
functions/src/lib/donation-certificates/DonationCertificatePDFTemplate.tsx (2)
2-2: Remove or fix the TypeScript ignore directive.This directive suppresses type checking for the entire file and can hide potential errors. Try removing
@ts-nocheckand resolving any underlying issues to maintain type safety.
28-28: Include or remove the unused "year" property.The interface defines "year," but this property is not being used or destructured in the function parameters. Either remove it from the interface or incorporate it into the component if needed.
-function DonationCertificatePDFTemplate({ user, translator }: DonationCertificatePDFTemplateProps) { +function DonationCertificatePDFTemplate({ user, translator, year }: DonationCertificatePDFTemplateProps) {admin/src/collections/Contributors.tsx (1)
13-13: Icon change from 'VolunteerActivism' to 'Person' is purely aesthetic.
The new icon suits the concept of an individual contributor. No further action needed.functions/src/lib/donation-certificates/DonationCertificateWriter.ts (4)
23-30: Add error handling for Firestore queries.
Currently, any Firestore failures inside this method won't be caught, which could mask issues with connectivity or permissions.
61-61: Use robust lookups for country.
If user data is missing or partial, ensure translator handles unexpected values gracefully.
108-110: Gracefully handle missing address fields.
If the user hasn’t provided a full address, these references might cause errors or display incomplete data.
125-137: Complete the locale TODO.
Line 132 notes using the correct locale. Finalizing that will help display currency amounts properly.functions/src/functions/cron/donation-certificates/index.ts (1)
1-10: Add robust error handling
Use a try/catch to handle possible errors fromcreateDonationCertificatesso the function fails gracefully.export default onSchedule({ schedule: '0 0 2 1 *', memory: '4GiB' }, async () => { const now = DateTime.now(); - const msg = await createDonationCertificates({ year: now.year }); - logger.info(msg); + try { + const msg = await createDonationCertificates({ year: now.year }); + logger.info(msg); + } catch (err) { + logger.error('Donation certificate creation failed', err); + throw err; + } });functions/src/functions/webhooks/admin/donation-certificates/index.ts (1)
1-10: Ensure graceful failure for unauthorized calls
WrapassertGlobalAdminin a try/catch to return a user-friendly error if unauthorized.export default onCall<CreateDonationCertificatesProps, Promise<string>>({ memory: '4GiB' }, async (request) => { const firestoreAdmin = new FirestoreAdmin(); - await firestoreAdmin.assertGlobalAdmin(request.auth?.token?.email); + try { + await firestoreAdmin.assertGlobalAdmin(request.auth?.token?.email); + } catch (error) { + throw new Error("Unauthorized. Only global admins can create donation certificates."); + } return await createDonationCertificates(request.data); });website/src/app/[lang]/[region]/(website)/me/donation-certificates/donation-certificates-table.tsx (1)
27-32: Add fallback or error handling for file fetch.Currently, the file link is hidden if
datais absent. Consider showing a message or indicator for missing files.functions/src/lib/donation-certificates/index.ts (1)
66-67: Return structured data instead of a multiline string.Returning an object or structured response can be more convenient for downstream logging or processing compared to a multiline string.
.github/workflows/shared.yml (1)
35-35: Remove or populate the empty env object.Leaving
env: {}unused can be confusing. Delete it entirely if no environment variables are required.- env: {} + # Remove this line if no env variables are needed🧰 Tools
🪛 actionlint (1.7.4)
35-35: env should not be empty. please remove this section if it's unnecessary
(syntax-check)
.github/workflows/functions.yml (1)
35-35: Remove unnecessary empty env block.The empty environment block can be safely removed as it serves no purpose.
Apply this diff:
- env: {}🧰 Tools
🪛 actionlint (1.7.4)
35-35: env should not be empty. please remove this section if it's unnecessary
(syntax-check)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (64)
.github/workflows/deployment.yml(0 hunks).github/workflows/functions.yml(1 hunks).github/workflows/shared.yml(1 hunks)admin/src/App.tsx(2 hunks)admin/src/actions/CreateDonationCertificatesAction.tsx(3 hunks)admin/src/actions/InviteWhatsappAction.tsx(0 hunks)admin/src/actions/PaymentProcessAction.tsx(1 hunks)admin/src/collections/Contributors.tsx(3 hunks)admin/src/collections/DonationCertificate.tsx(2 hunks)admin/src/collections/Messages.ts(0 hunks)admin/src/collections/recipients/Recipients.tsx(1 hunks)functions/.env.sample(0 hunks)functions/package.json(0 hunks)functions/src/config.ts(0 hunks)functions/src/firebase.ts(5 hunks)functions/src/functions/cron/donation-certificates/index.ts(1 hunks)functions/src/functions/cron/exchange-rate-import/ExchangeRateImporter.test.ts(1 hunks)functions/src/functions/cron/exchange-rate-import/ExchangeRateImporter.ts(1 hunks)functions/src/functions/cron/first-payout-email/index.ts(1 hunks)functions/src/functions/cron/postfinance-payments-files-import/index.ts(1 hunks)functions/src/functions/firestore/firestore-auditor/FirestoreAuditor.test.ts(1 hunks)functions/src/functions/firestore/firestore-auditor/FirestoreAuditor.ts(1 hunks)functions/src/functions/storage/postfinance-payments-files/index.ts(1 hunks)functions/src/functions/webhooks/admin/donation-certificates/index.ts(1 hunks)functions/src/functions/webhooks/admin/payment-forecast/index.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/index.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/tasks/PaymentCSVTask.test.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/tasks/PaymentCSVTask.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/tasks/PaymentTask.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/tasks/RegistrationCSVTask.test.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/tasks/UpdateDatabaseEntriesTask.test.ts(1 hunks)functions/src/functions/webhooks/admin/payment-process/tasks/UpdateDatabaseEntriesTask.ts(1 hunks)functions/src/functions/webhooks/admin/scripts/BatchAddCHFToPayments.test.ts(1 hunks)functions/src/functions/webhooks/admin/scripts/PaymentsManager.ts(1 hunks)functions/src/functions/webhooks/admin/scripts/SurveyManager.ts(1 hunks)functions/src/functions/webhooks/admin/scripts/index.ts(1 hunks)functions/src/functions/webhooks/stripe/index.ts(1 hunks)functions/src/functions/webhooks/website/survey-login/index.ts(1 hunks)functions/src/index.ts(1 hunks)functions/src/lib/donation-certificates/DonationCertificatePDFTemplate.tsx(2 hunks)functions/src/lib/donation-certificates/DonationCertificateWriter.ts(5 hunks)functions/src/lib/donation-certificates/index.ts(1 hunks)functions/src/webhooks/admin/donation-certificates/index.ts(0 hunks)functions/src/webhooks/admin/payment-process/tasks/SendNotificationsTask.ts(0 hunks)functions/src/webhooks/twilio/TwilioIncomingMessageHandler.ts(0 hunks)functions/src/webhooks/twilio/TwilioOutgoingMessageHandler.ts(0 hunks)seed/storage_export/buckets.json(1 hunks)shared/.env.sample(0 hunks)shared/package.json(1 hunks)shared/src/firebase/admin/StorageAdmin.ts(1 hunks)shared/src/types/donation-certificate.ts(1 hunks)shared/src/types/language.ts(1 hunks)shared/src/types/message.ts(0 hunks)shared/src/types/payment.ts(0 hunks)shared/src/types/recipient.ts(2 hunks)shared/src/utils/messaging/sms.ts(0 hunks)shared/src/utils/messaging/whatsapp.ts(0 hunks)shared/src/utils/sms.test.ts(0 hunks)storage.rules(1 hunks)ui/src/components/typography/typography.tsx(1 hunks)website/.env.development(1 hunks)website/src/app/[lang]/[region]/(website)/me/donation-certificates/donation-certificates-table.tsx(3 hunks)website/src/app/[lang]/[region]/(website)/me/hooks.ts(2 hunks)website/tailwind.config.ts(1 hunks)
💤 Files with no reviewable changes (16)
- shared/src/types/payment.ts
- shared/.env.sample
- functions/.env.sample
- shared/src/utils/sms.test.ts
- shared/src/utils/messaging/whatsapp.ts
- functions/src/config.ts
- admin/src/actions/InviteWhatsappAction.tsx
- .github/workflows/deployment.yml
- admin/src/collections/Messages.ts
- shared/src/utils/messaging/sms.ts
- functions/src/webhooks/twilio/TwilioIncomingMessageHandler.ts
- shared/src/types/message.ts
- functions/package.json
- functions/src/webhooks/admin/payment-process/tasks/SendNotificationsTask.ts
- functions/src/webhooks/twilio/TwilioOutgoingMessageHandler.ts
- functions/src/webhooks/admin/donation-certificates/index.ts
✅ Files skipped from review due to trivial changes (20)
- functions/src/functions/cron/postfinance-payments-files-import/index.ts
- functions/src/functions/cron/exchange-rate-import/ExchangeRateImporter.ts
- functions/src/functions/cron/exchange-rate-import/ExchangeRateImporter.test.ts
- functions/src/functions/webhooks/stripe/index.ts
- functions/src/functions/firestore/firestore-auditor/FirestoreAuditor.test.ts
- functions/src/functions/firestore/firestore-auditor/FirestoreAuditor.ts
- website/tailwind.config.ts
- functions/src/functions/webhooks/admin/payment-process/tasks/PaymentCSVTask.ts
- functions/src/functions/webhooks/admin/payment-forecast/index.ts
- functions/src/functions/webhooks/admin/scripts/index.ts
- functions/src/functions/webhooks/admin/payment-process/tasks/UpdateDatabaseEntriesTask.ts
- functions/src/functions/webhooks/admin/scripts/SurveyManager.ts
- functions/src/functions/cron/first-payout-email/index.ts
- functions/src/functions/webhooks/admin/scripts/PaymentsManager.ts
- functions/src/functions/webhooks/admin/scripts/BatchAddCHFToPayments.test.ts
- functions/src/functions/webhooks/admin/payment-process/tasks/PaymentTask.ts
- functions/src/functions/webhooks/admin/payment-process/tasks/PaymentCSVTask.test.ts
- functions/src/functions/webhooks/website/survey-login/index.ts
- functions/src/functions/webhooks/admin/payment-process/tasks/RegistrationCSVTask.test.ts
- functions/src/functions/webhooks/admin/payment-process/tasks/UpdateDatabaseEntriesTask.test.ts
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/shared.yml
35-35: env should not be empty. please remove this section if it's unnecessary
(syntax-check)
.github/workflows/functions.yml
35-35: env should not be empty. please remove this section if it's unnecessary
(syntax-check)
⏰ 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 (38)
ui/src/components/typography/typography.tsx (1)
27-27: Font weight map logic appears correct.
This new mapping maintains consistency with Tailwind’sfont-semibold.shared/src/types/recipient.ts (2)
3-4: Imports are consistent and clear.
ImportingDocumentReferenceandPartnerOrganisationis suitable for referencing partner organization documents in Firestore. No issues found.
39-39: Good transition from string to typed reference.
Switching theorganisationfield to aDocumentReference<PartnerOrganisation>improves type safety, but ensure the referenced document always exists to avoid runtime errors.admin/src/actions/PaymentProcessAction.tsx (1)
11-11: Switching to a type-only import is a good practice
It helps ensure that only types are imported, reducing bundling overhead. The updated path also appears valid and consistent with the project's structure.admin/src/actions/CreateDonationCertificatesAction.tsx (3)
19-19: Updated import path seems valid.
Switching from a webhook path to a library path aligns with the Twilio removal efforts. No issues here.
57-57: Handle undefined userIds carefully.
If the Cloud Function expects an array, passingundefinedmay cause unexpected results. Consider passing an empty array instead or confirm thatundefinedis supported on the server.
47-47: Confirm the updated callable function types.
Everything looks consistent with the newCreateDonationCertificatesProps. Please verify that the Cloud Function is updated accordingly.✅ Verification successful
Cloud Function implementation verified and type-safe
The Cloud Function is properly implemented using the sameCreateDonationCertificatesPropstype, with correct authentication and memory settings.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify references to CreateDonationCertificatesProps throughout the repository rg -A 5 "CreateDonationCertificatesProps"Length of output: 2932
functions/src/lib/donation-certificates/DonationCertificatePDFTemplate.tsx (1)
Line range hint
29-46: Handle null or undefined "address.country" gracefully.Although optional chaining is used, the fallback cast to
stringmight still lead to issues ifcountryis undefined. Consider providing a fallback for country or gracefully handling missing data.shared/src/types/language.ts (1)
3-3: Exporting the LANGUAGE_CODES constant is a fine enhancement.
This change promotes reusability, allowing the language codes array to be consumed across different modules. Keep the TODO in mind if you plan to expand ISO codes in the future.admin/src/collections/recipients/Recipients.tsx (1)
96-96: Confirm removal of messagesCollection references.
You removedmessagesCollectionfrom thesubcollections. Verify that no other code paths still rely on it.✅ Verification successful
Removal of messagesCollection is safe
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking for references to messagesCollection rg 'messagesCollection'Length of output: 23
Script:
#!/bin/bash # Search for various forms of messages/collection references rg -i 'messages?[^a-z]*(collection|subcollection)' -g '*.{ts,tsx,js,jsx}' # Search for any collection-related imports or definitions ast-grep --pattern 'import { $_ } from $path' --lang typescript | grep -i 'message' # Search for message-related patterns rg -i 'messages?\.' -g '*.{ts,tsx,js,jsx}' -A 2Length of output: 1355
admin/src/collections/Contributors.tsx (4)
1-2: Imports for country and language codes are correct.
Importing these constants here ensures a centralized definition for all code references.
10-10: Renaming the collection clarifies its purpose.
Switching fromusersCollectiontocontributorsCollectionbetter describes its contents, avoiding confusion with user-based authentication entities.
85-87: Dynamically populating country enum is good for maintainability.
UsingCOUNTRY_CODES.mapis concise and avoids duplicating the list. Confirm that this meets your localization needs.
110-110: Language codes enumeration is well-applied.
Leveraging the newly exportedLANGUAGE_CODESensures a single source of truth for languages across the codebase.admin/src/App.tsx (2)
52-53: Inserting contributorsCollection into the main collections array
Ensures contributor data is now fully integrated and accessible in the Admin app.
13-13: Adding the contributorsCollection import
This import aligns with the rename inContributors.tsx. Make sure references to the oldusersCollectionimport are fully removed elsewhere.✅ Verification successful
Migration from
usersCollectiontocontributorsCollectionis complete
All references tousersCollectionhave been properly removed, and the newcontributorsCollectionis correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg 'usersCollection'Length of output: 20
Script:
#!/bin/bash # Case-insensitive search for any variation of usersCollection rg -i 'userscollection' # Search specifically in TS/TSX files rg 'usersCollection' -g '*.{ts,tsx}' # Search for partial matches to catch any remnants rg 'users.*Collection' # Verify the new contributorsCollection usage rg 'contributorsCollection' # Use ast-grep to find import statements containing either collection ast-grep --pattern 'import { $_ } from $path'Length of output: 400
functions/src/lib/donation-certificates/DonationCertificateWriter.ts (7)
2-2: The import for DocumentSnapshot looks correct.
7-10: Imports from shared modules are consistent and properly referenced.
15-15: Upgraded property from user to userDoc is appropriate.
50-52: Validate user data after retrieval.
Checking for null or missing fields in the user document ensures robust handling of incomplete records.
55-55: Localizing default user language is a good fallback.
The approach of defaulting to 'de' if language is missing is reasonable.
65-67: Check presence of personal fields.
Personal data might be unavailable or incomplete; consider safe defaults or placeholders.
154-154: Spacing tweak is harmless.
This line shift is inconsequential and safe.shared/src/types/donation-certificate.ts (1)
6-6: Ensure references to removed url property are updated.
Replacing url with storage_path may break existing code if references still exist. Verify usage in the codebase.storage.rules (1)
3-6: Tightened rules limit access to user-owned paths only.
This is a security improvement, but be sure no other legitimate paths are needed. Denying all else may restrict service functionality.functions/src/index.ts (1)
6-9: Paths updated for clarity
Ensures a more organized directory structure. Make sure these deeper paths exist at build.✅ Verification successful
All directory paths verified and exist
functions/src/functions/cron/functions/src/functions/firestore/functions/src/functions/storage/functions/src/functions/webhooks/🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify existence of new paths. fd 'cron' functions/src/functions | xargs ls -d fd 'firestore' functions/src/functions | xargs ls -d fd 'storage' functions/src/functions | xargs ls -d fd 'webhooks' functions/src/functions | xargs ls -dLength of output: 537
functions/src/functions/storage/postfinance-payments-files/index.ts (1)
2-3: Paths realigned correctly
Imports now align with the updated structure, no issues found.shared/src/firebase/admin/StorageAdmin.ts (1)
25-25: Returning the upload result is helpful.This lets the caller handle post-upload operations, such as logging or error handling.
admin/src/collections/DonationCertificate.tsx (1)
32-36: New 'storage_path' property aligns well with the updated architecture.This change replaces the previous URL-based approach and should simplify retrieval of stored documents.
functions/src/functions/webhooks/admin/payment-process/index.ts (1)
4-5: Ensure no references remain to removed Twilio tasks.Changing imports and removing
SendNotificationsfrom the task enum is appropriate, but verify no leftover calls rely on that type.Also applies to: 12-15
✅ Verification successful
All Twilio task references have been properly removed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls or references to PaymentProcessTaskType.SendNotifications rg 'PaymentProcessTaskType\.SendNotifications'Length of output: 48
Script:
#!/bin/bash # Search for enum definition and any references to SendNotifications or Twilio ast-grep --pattern 'enum PaymentProcessTaskType { $$$ }' # Search for any remaining Twilio references in relevant paths rg -i "twilio|sendnotification" --type ts # Search for string literals that might reference the task rg "'SendNotifications'|\"SendNotifications\"" --type tsLength of output: 475
website/src/app/[lang]/[region]/(website)/me/donation-certificates/donation-certificates-table.tsx (3)
15-17: Appropriate usage of Firebase imports.Using
reffromfirebase/storageandreactfirehooks helps keep file storage logic self-contained.
41-41: Good check for undefined or empty collection.Ensures a descriptive message when donation certificates are unavailable.
52-61: Conditional row rendering.Skipping rows without
storage_pathis sensible here but might obscure partial data if certificates are incomplete.functions/src/lib/donation-certificates/index.ts (1)
22-31: Verify potential empty query result.If the Firestore query returns zero documents,
userIdswill be an empty array. Ensure that your logic handles this case gracefully.website/src/app/[lang]/[region]/(website)/me/hooks.ts (1)
62-66: Check for missing user context.If
useris unavailable, the call touser.ref.pathmay fail. Ensure that the user context is always defined or handle the null scenario properly.seed/storage_export/buckets.json (1)
4-4: Verify references to the updated bucket.Renaming the bucket could break any hardcoded references or environment variables referencing the old bucket name. Double-check for references that may need updating.
shared/package.json (1)
30-30: Clean removal of Twilio dependency confirmed.The removal of Twilio while maintaining Stripe is consistent with the broader changes across the codebase.
website/.env.development (1)
15-17: Firebase Storage emulator configuration looks good.The configuration uses correct default values and follows Firebase conventions.
Run this script to verify the storage emulator configuration:
✅ Verification successful
Firebase Storage emulator configuration is correctly implemented
The environment variables are properly used in the context provider for initializing the Firebase Storage emulator with appropriate host, port, and bucket settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Firebase Storage emulator configuration usage # Look for storage initialization code that uses these environment variables rg -l "NEXT_PUBLIC_FIREBASE_STORAGE" --type tsLength of output: 101
Script:
#!/bin/bash # Check how Firebase Storage environment variables are used in the context provider ast-grep --pattern 'process.env.NEXT_PUBLIC_FIREBASE_STORAGE' website/src/components/providers/context-providers.tsxLength of output: 116
Script:
#!/bin/bash # View the complete content of the context provider file cat website/src/components/providers/context-providers.tsxLength of output: 9502
| constructor(userDoc: DocumentSnapshot<User>, year: number) { | ||
| assert(userDoc.get('address.country') === 'CH', 'Donation Certificates are supported for Swiss residents'); | ||
| this.userDoc = userDoc; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling undefined address fields.
The assertion may throw an error if the Firestore document has no 'address' or 'country' field. A safer check would prevent potential runtime exceptions.
| export async function createDonationCertificates({ year, userIds }: CreateDonationCertificatesProps): Promise<string> { | ||
| const storageAdmin = new StorageAdmin(); | ||
| const firestoreAdmin = new FirestoreAdmin(); | ||
|
|
There was a problem hiding this comment.
Avoid concurrency issues with shared state.
Incrementing successCount in parallel within Promise.all can result in a race condition. Consider storing counts in local variables and aggregating after all tasks complete.
|
Visit the preview URL for this PR (updated for commit 3e6efd7): https://si-admin-staging--pr1021-michael-donation-cer-seom8c0v.web.app (expires Fri, 31 Jan 2025 12:51:40 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
shared/src/types/recipient.ts (1)
39-39: Consider data migration and query impacts.The switch to DocumentReference requires:
- Migration plan for existing recipient data
- Updates to database queries
- Frontend changes to handle document references
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shared/src/types/recipient.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Deploy Preview
- GitHub Check: Deploy admin on Firebase (social-income-staging) / Deploy admin on Firebase
- GitHub Check: Test website
🔇 Additional comments (2)
shared/src/types/recipient.ts (2)
1-1: LGTM! Import changes support the new type system.The new imports enable proper typing for Firestore document references and partner organizations.
Also applies to: 3-3
39-39: Verify all Recipient object usages are updated.The change from
stringtoDocumentReference<PartnerOrganisation>is a breaking change that requires updates wherever Recipient objects are created or read.✅ Verification successful
All Recipient object usages are properly updated
The codebase has been thoroughly checked and all instances of Recipient type usage properly handle the organization field as DocumentReference. No incompatible usages were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Recipient type usage to verify all instances are updated rg -A 5 "Recipient[^a-zA-Z]" --type typescriptLength of output: 83
Script:
#!/bin/bash # Search for Recipient type usage and related patterns echo "=== Searching for Recipient type usage ===" rg -t ts -t tsx "Recipient(\s|[:<{])" -A 5 echo -e "\n=== Searching for organisation field access ===" rg -t ts -t tsx "\.organisation\s*[:=]" -A 2 echo -e "\n=== Searching for Recipient imports ===" rg -t ts -t tsx "import.*Recipient.*from" echo -e "\n=== AST search for Recipient type annotations ===" ast-grep --pattern 'const $_ : Recipient = $_'Length of output: 684
Script:
#!/bin/bash # First, let's find all TypeScript files echo "=== TypeScript files in the project ===" fd -e ts -e tsx echo -e "\n=== Searching for Recipient usage ===" fd -e ts -e tsx --exec rg -l "Recipient" echo -e "\n=== Detailed search in found files ===" fd -e ts -e tsx --exec rg "Recipient|\.organisation"Length of output: 36701
Fixes #993
Summary by CodeRabbit
Release Notes
Removed Features
New Features
Improvements
Bug Fixes