Skip to content

Conversation

@fabricem90
Copy link
Contributor

@fabricem90 fabricem90 commented Oct 31, 2025

  • make phone in paymentInformation mandatory
  • add firebase auth service
  • create auth user by phone when recipient is being created
  • update auth user by phone when recipient is being updated
  • adapt paymentInformation phone validation to E.164 format to match firebase auth requirements

Summary by CodeRabbit

  • New Features

    • Backend now creates/updates Firebase Auth users by recipient phone number.
    • Phone numbers are normalized to a consistent E.164 format across flows.
  • Bug Fixes

    • Payment information phone is mandatory; DB migration enforces NOT NULL.
    • Recipient create/update always includes a phone payload for consistency.
    • Recipient form enforces strict E.164 validation with clearer error messaging.
    • Recipient-related error log label updated.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 31, 2025

📝 Walkthrough

Walkthrough

Make PaymentInformation.phone mandatory in Prisma and DB; add FirebaseService to manage Auth users by phone and call it from recipient create/update flows; always emit phone.create in transformers/helpers and enforce strict E.164 validation on the frontend; add SQL migration to set payment_information.phone_id NOT NULL.

Changes

Cohort / File(s) Change Summary
Prisma Schema & SQL Migration
shared/src/database/schema.prisma, shared/src/database/migrations/20251031122446_make_payment_information_phone_mandatory/migration.sql
Make PaymentInformation.phoneId and phone relation required in Prisma; add migration to set payment_information.phone_id NOT NULL and recreate FK (ON DELETE RESTRICT, ON UPDATE CASCADE).
Firebase Auth Service
shared/src/firebase/services/auth.service.ts
Add FirebaseService that initializes Firebase Admin from env credentials and provides createByPhoneNumber, updateByPhoneNumber, and getByPhoneNumber with logging/error wrapping.
Recipient Database Service
shared/src/database/services/recipient/recipient.service.ts
Instantiate and use firebaseAuthService: call create/update Firebase user by phone in recipient create/update flows; expand selects to include nested paymentInformation.phone.number.
Firestore → Postgres Transformer
firestore_to_postgres/src/recipient/recipient.transformer.ts
Add mapPhoneNumber helper and use it to produce E.164 or null; ensure paymentPhone always emits a create payload (number: '', hasWhatsApp: false) when absent.
Frontend Form Helpers & Validation
website/src/app/portal/components/forms/recipient/recipient-form-helpers.ts, website/src/app/portal/components/forms/recipient/recipient-form.tsx
Always emit paymentInformation.phone.create in create/update inputs; switch client-side validation to strict E.164 format and add explicit invalid-format error message.
Frontend Client Logging
website/src/app/portal/components/data-table/clients/recipients-table-client.tsx
Update error log label from "Local Partner Form Error" to "Recipient Form Error" (logging text change only).
Payout Transformer
firestore_to_postgres/src/payout/payout.transformer.ts
Change phoneNumber mapping to conditionally prefix with '+' when present (payout.phone_number ? \+${payout.phone_number}` : null`).

Sequence Diagram(s)

sequenceDiagram
    participant FE as Frontend
    participant RS as RecipientService
    participant FS as FirebaseService
    participant FA as Firebase Auth
    participant DB as Database

    rect rgb(240,248,255)
    Note over FE,DB: Create Recipient (payment phone always emitted)
    FE->>RS: createRecipient(payload with paymentInformation.phone.create.number)
    RS->>FS: createByPhoneNumber(phoneNumber)
    FS->>FA: getUserByPhone(phoneNumber)
    alt user exists
        FA-->>FS: user
    else not found
        FS->>FA: createUser(phoneNumber)
        FA-->>FS: created user
    end
    FS-->>RS: user
    RS->>DB: create recipient + paymentInformation + phone
    DB-->>RS: created
    RS-->>FE: created recipient
    end

    rect rgb(240,248,255)
    Note over FE,DB: Update Recipient (phone change path)
    FE->>RS: updateRecipient(payload with phone)
    RS->>DB: fetch existing (incl. paymentInformation.phone.number)
    DB-->>RS: existing record
    RS->>FS: updateByPhoneNumber(oldPhone, newPhone)
    FS->>FA: getUserByPhone(oldPhone)
    FA-->>FS: user
    FS->>FA: updateUserPhone(userId, newPhone)
    FA-->>FS: updated user
    FS-->>RS: user
    RS->>DB: upsert paymentInformation + phone, update recipient
    DB-->>RS: updated
    RS-->>FE: updated recipient
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Review Firebase admin initialization and env credential parsing in shared/src/firebase/services/auth.service.ts.
  • Check ordering/transactional implications of calling Firebase before DB writes in shared/src/database/services/recipient/recipient.service.ts.
  • Verify migration safety for existing NULL payment_information.phone_id values and FK naming.
  • Confirm transformer and frontend helper changes produce payload shapes matching the updated Prisma schema.

Possibly related PRs

Suggested reviewers

  • mkue
  • ssandino
  • triggertoo
  • socialincome-dev

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat: create and update recipients in firebase auth store' directly aligns with the main objectives: creating and updating recipients in Firebase Auth. It accurately summarizes the primary purpose of the changeset across multiple files including the new FirebaseService, RecipientService modifications, and phone formatting updates. The title is clear, specific, and reflects the core functionality being added.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/recipients-auth-sync

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
firestore_to_postgres/src/recipient/recipient.transformer.ts (1)

31-46: Empty-string phone create will break import

When raw.mobile_money_phone?.phone is missing you now create a Phone with number: ''. The phone.number column is unique and downstream code (Firebase Auth + E.164 validation) requires a real number, so repeated legacy rows without a phone will crash the import or violate constraints. Instead, skip these recipients (or surface an error) and keep the relation optional at load time. Example fix:

-			const paymentPhone = raw.mobile_money_phone?.phone
-				? { /* existing connectOrCreate */ }
-				: {
-						create: {
-							number: '',
-							hasWhatsApp: false,
-						},
-					};
+			if (!raw.mobile_money_phone?.phone) {
+				skipped++;
+				console.warn(`⚠️ Skipping recipient ${raw.id} because payment phone is missing`);
+				continue;
+			}
+
+			const paymentPhone = {
+				connectOrCreate: {
+					where: { number: raw.mobile_money_phone.phone.toString() },
+					create: {
+						number: raw.mobile_money_phone.phone.toString(),
+						hasWhatsApp: raw.mobile_money_phone.has_whatsapp,
+					},
+				},
+			};
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0749d9 and 50f8694.

📒 Files selected for processing (7)
  • firestore_to_postgres/src/recipient/recipient.transformer.ts (1 hunks)
  • shared/src/database/migrations/20251031122446_make_payment_information_phone_mandatory/migration.sql (1 hunks)
  • shared/src/database/schema.prisma (1 hunks)
  • shared/src/database/services/recipient/recipient.service.ts (5 hunks)
  • shared/src/firebase/services/auth.service.ts (1 hunks)
  • website/src/app/portal/components/data-table/clients/recipients-table-client.tsx (1 hunks)
  • website/src/app/portal/components/forms/recipient-form.tsx (3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: MDikkii
Repo: socialincome-san/public PR: 960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.
📚 Learning: 2024-11-29T06:09:31.569Z
Learnt from: activus-d
Repo: socialincome-san/public PR: 926
File: website/src/app/[lang]/[region]/(website)/open-source/(sections)/contributors.tsx:10-25
Timestamp: 2024-11-29T06:09:31.569Z
Learning: In `contributors.tsx`, additional error handling and loading states in the `OpenSourceContributors` component are not required.

Applied to files:

  • website/src/app/portal/components/data-table/clients/recipients-table-client.tsx
📚 Learning: 2024-12-16T04:00:28.526Z
Learnt from: activus-d
Repo: socialincome-san/public PR: 985
File: website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts:14-26
Timestamp: 2024-12-16T04:00:28.526Z
Learning: In 'website/src/app/[lang]/[region]/(website)/open-source/(components)/fetch-data.ts', avoid suggesting inclusion of rate limit information in the error messages within the 'fetchData' function.

Applied to files:

  • website/src/app/portal/components/data-table/clients/recipients-table-client.tsx
📚 Learning: 2025-01-06T18:59:26.103Z
Learnt from: MDikkii
Repo: socialincome-san/public PR: 960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.

Applied to files:

  • shared/src/database/services/recipient/recipient.service.ts
🧬 Code graph analysis (2)
shared/src/database/services/recipient/recipient.service.ts (1)
shared/src/firebase/services/auth.service.ts (1)
  • AuthService (16-56)
shared/src/firebase/services/auth.service.ts (2)
shared/src/firebase/admin/AuthAdmin.ts (1)
  • AuthAdmin (5-15)
shared/src/firebase/admin/app.ts (1)
  • getOrInitializeFirebaseAdmin (7-10)

Comment on lines +11 to +14
ALTER TABLE "payment_information" ALTER COLUMN "phone_id" SET NOT NULL;

-- AddForeignKey
ALTER TABLE "payment_information" ADD CONSTRAINT "payment_information_phone_id_fkey" FOREIGN KEY ("phone_id") REFERENCES "phone"("id") ON DELETE RESTRICT ON UPDATE CASCADE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Backfill payment_information.phone_id before enforcing NOT NULL

If any existing payment_information row still has phone_id = NULL, this ALTER COLUMN ... SET NOT NULL will abort the migration. Please add a data backfill (or a guard that blocks the deployment earlier) so the constraint can be applied safely.

🤖 Prompt for AI Agents
In
shared/src/database/migrations/20251031122446_make_payment_information_phone_mandatory/migration.sql
around lines 11 to 14, the migration sets payment_information.phone_id NOT NULL
but doesn't backfill existing NULL values; add a safe data backfill before the
ALTER COLUMN so the migration won't fail: within the migration create or select
an appropriate fallback phone (or require a pre-check) and run an UPDATE
payment_information SET phone_id = <fallback_phone_id> WHERE phone_id IS NULL
(or insert phones and use their ids), then apply ALTER COLUMN ... SET NOT NULL
and add the foreign key; also include a reversible down-step (or guard) so
rollback restores the previous state (e.g., NULLs or drops the constraint).

@github-actions
Copy link

github-actions bot commented Oct 31, 2025

Visit the preview URL for this PR (updated for commit 145273a):

https://si-admin-staging--pr1321-feat-recipients-auth-rprpadja.web.app

(expires Mon, 10 Nov 2025 13:24:03 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50f8694 and 527073a.

📒 Files selected for processing (4)
  • shared/src/database/services/recipient/recipient.service.ts (5 hunks)
  • website/src/app/portal/components/data-table/clients/recipients-table-client.tsx (1 hunks)
  • website/src/app/portal/components/forms/recipient/recipient-form-helpers.ts (2 hunks)
  • website/src/app/portal/components/forms/recipient/recipient-form.tsx (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-01-06T18:59:26.103Z
Learnt from: MDikkii
Repo: socialincome-san/public PR: 960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.

Applied to files:

  • shared/src/database/services/recipient/recipient.service.ts
📚 Learning: 2024-11-29T06:09:31.569Z
Learnt from: activus-d
Repo: socialincome-san/public PR: 926
File: website/src/app/[lang]/[region]/(website)/open-source/(sections)/contributors.tsx:10-25
Timestamp: 2024-11-29T06:09:31.569Z
Learning: In `contributors.tsx`, additional error handling and loading states in the `OpenSourceContributors` component are not required.

Applied to files:

  • website/src/app/portal/components/data-table/clients/recipients-table-client.tsx
🧬 Code graph analysis (1)
shared/src/database/services/recipient/recipient.service.ts (1)
shared/src/firebase/services/auth.service.ts (1)
  • AuthService (16-56)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Deploy admin on Firebase (social-income-staging) / Deploy admin on Firebase
  • GitHub Check: Test website
🔇 Additional comments (2)
website/src/app/portal/components/data-table/clients/recipients-table-client.tsx (1)

41-44: Log label alignment looks good.

Renaming the console message keeps diagnostics consistent with the recipient form flow.

website/src/app/portal/components/forms/recipient/recipient-form.tsx (1)

101-104: E.164 validation update is on point.

The stricter regex ensures we only pass Firebase-compatible numbers while giving users a precise hint.

Copy link
Contributor

@rawi96 rawi96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, 1 little nit, rest looks awesome. 🚀

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
website/src/app/portal/components/forms/recipient/recipient-form-helpers.ts (1)

94-94: Same issue: unconditional create with potentially undefined value.

Identical to Line 53—if paymentInfoFields.phone.value is undefined, the backend will reject the request. Apply the same conditional check here.

-				phone: { create: { number: paymentInfoFields.phone.value } },
+				phone: paymentInfoFields.phone.value
+					? { create: { number: paymentInfoFields.phone.value } }
+					: undefined,
🧹 Nitpick comments (2)
shared/src/database/services/recipient/recipient.service.ts (2)

32-37: Firebase user creation return value discarded.

createByPhoneNumber returns a UserRecord containing the Firebase UID. If you need to store or reference the Firebase UID in your database later, capture the return value now. Otherwise, you'll have to query Firebase again.


73-77: Consider capturing Firebase UID and optimizing no-change case.

Similar to the create flow, the updateByPhoneNumber return value (containing the Firebase UID) is discarded. Additionally, if the phone number hasn't changed (oldPhoneNumber === phoneNumber), you could skip the Firebase call entirely.

+			if (existing.paymentInformation.phone.number === phoneNumber) {
+				// Phone unchanged, skip Firebase update
+			} else {
 				await this.firebaseAuthService.updateByPhoneNumber(existing.paymentInformation.phone.number, phoneNumber);
+			}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 527073a and b300f10.

📒 Files selected for processing (3)
  • shared/src/database/services/recipient/recipient.service.ts (5 hunks)
  • shared/src/firebase/services/auth.service.ts (1 hunks)
  • website/src/app/portal/components/forms/recipient/recipient-form-helpers.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • shared/src/firebase/services/auth.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-06T18:59:26.103Z
Learnt from: MDikkii
Repo: socialincome-san/public PR: 960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.

Applied to files:

  • shared/src/database/services/recipient/recipient.service.ts
🧬 Code graph analysis (1)
shared/src/database/services/recipient/recipient.service.ts (1)
shared/src/firebase/services/auth.service.ts (1)
  • FirebaseService (16-59)
🔇 Additional comments (1)
shared/src/database/services/recipient/recipient.service.ts (1)

55-55: LGTM: Select properly includes existing phone number.

This change correctly fetches the existing phone number needed for the Firebase update call on Line 77.

Comment on lines +69 to +71
const phoneNumber =
recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number?.toString() ||
recipient.paymentInformation?.upsert?.create?.phone?.create?.number?.toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove unnecessary .toString() calls.

Phone numbers are already strings. Calling .toString() on null or undefined produces the literal strings "null" or "undefined", which could bypass validation and cause subtle bugs downstream.

 const phoneNumber =
-    recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number?.toString() ||
-    recipient.paymentInformation?.upsert?.create?.phone?.create?.number?.toString();
+    recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number ||
+    recipient.paymentInformation?.upsert?.create?.phone?.create?.number;
📝 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.

Suggested change
const phoneNumber =
recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number?.toString() ||
recipient.paymentInformation?.upsert?.create?.phone?.create?.number?.toString();
const phoneNumber =
recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number ||
recipient.paymentInformation?.upsert?.create?.phone?.create?.number;
🤖 Prompt for AI Agents
In shared/src/database/services/recipient/recipient.service.ts around lines 69
to 71, remove the unnecessary .toString() calls on the phone number accessors;
instead read the number directly via the optional-chained properties so you get
either the original string or undefined (avoiding literal "null"/"undefined").
Update the expression to return the direct property (no .toString()), and if
downstream code requires a guaranteed string, perform an explicit, safe
validation or normalization step (e.g., check for a non-empty string) rather
than calling .toString().

create: {
...basePaymentInfo,
phone: paymentInfoFields.phone.value ? { create: { number: paymentInfoFields.phone.value } } : undefined,
phone: { create: { number: paymentInfoFields.phone.value } },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Unconditional create may pass undefined phone number.

If paymentInfoFields.phone.value is undefined, Prisma receives { create: { number: undefined } }. Since phone is now mandatory, this will fail backend validation. Consider adding a conditional check or ensuring frontend validation prevents submission without a phone number.

-					phone: { create: { number: paymentInfoFields.phone.value } },
+					phone: paymentInfoFields.phone.value 
+						? { create: { number: paymentInfoFields.phone.value } }
+						: undefined,
📝 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.

Suggested change
phone: { create: { number: paymentInfoFields.phone.value } },
phone: paymentInfoFields.phone.value
? { create: { number: paymentInfoFields.phone.value } }
: undefined,
🤖 Prompt for AI Agents
In website/src/app/portal/components/forms/recipient/recipient-form-helpers.ts
around line 53, the code unconditionally sets phone: { create: { number:
paymentInfoFields.phone.value } } which can pass number: undefined to Prisma and
break backend validation; change this to only include the phone create object
when paymentInfoFields.phone.value is non-empty (e.g., conditionally add the
phone property or use a conditional spread so phone is omitted when undefined),
or alternatively enforce frontend validation to prevent submission without a
phone before building this object.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b300f10 and 145273a.

📒 Files selected for processing (1)
  • shared/src/database/services/recipient/recipient.service.ts (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-06T18:59:26.103Z
Learnt from: MDikkii
Repo: socialincome-san/public PR: 960
File: recipients_app/lib/data/datasource/remote/user_remote_data_source.dart:87-91
Timestamp: 2025-01-06T18:59:26.103Z
Learning: The `Recipient` model enforces a non-nullable `userId` property, so no null checks are necessary there.

Applied to files:

  • shared/src/database/services/recipient/recipient.service.ts
🧬 Code graph analysis (1)
shared/src/database/services/recipient/recipient.service.ts (1)
shared/src/firebase/services/auth.service.ts (1)
  • FirebaseService (16-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Test website
  • GitHub Check: Test functions
  • GitHub Check: Test admin
  • GitHub Check: Test shared code
  • GitHub Check: Prettify

Comment on lines +70 to +78
const phoneNumber =
recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number?.toString() ||
recipient.paymentInformation?.upsert?.create?.phone?.create?.number?.toString();

if (!phoneNumber || !existing.paymentInformation?.phone?.number) {
return this.resultFail('No phone number available for recipient update');
}

await this.firebaseAuthService.updateByPhoneNumber(existing.paymentInformation?.phone.number, phoneNumber);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Blocker: allow recipient updates when phone stays untouched.

phoneNumber is undefined whenever the update payload doesn’t modify paymentInformation.phone, so the new guard immediately returns resultFail(...). That breaks every update (status changes, notes, etc.) that leaves the phone alone. Please only hit Firebase when a new number is supplied, and otherwise continue with the DB update.

-			const phoneNumber =
-				recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number?.toString() ||
-				recipient.paymentInformation?.upsert?.create?.phone?.create?.number?.toString();
-
-			if (!phoneNumber || !existing.paymentInformation?.phone?.number) {
-				return this.resultFail('No phone number available for recipient update');
-			}
-
-			await this.firebaseAuthService.updateByPhoneNumber(existing.paymentInformation?.phone.number, phoneNumber);
+			const newPhoneNumber =
+				recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number ||
+				recipient.paymentInformation?.upsert?.create?.phone?.create?.number;
+			const existingPhoneNumber = existing.paymentInformation?.phone?.number;
+
+			if (newPhoneNumber) {
+				if (!existingPhoneNumber) {
+					return this.resultFail('No phone number available for recipient update');
+				}
+				await this.firebaseAuthService.updateByPhoneNumber(existingPhoneNumber, newPhoneNumber);
+			}
📝 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.

Suggested change
const phoneNumber =
recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number?.toString() ||
recipient.paymentInformation?.upsert?.create?.phone?.create?.number?.toString();
if (!phoneNumber || !existing.paymentInformation?.phone?.number) {
return this.resultFail('No phone number available for recipient update');
}
await this.firebaseAuthService.updateByPhoneNumber(existing.paymentInformation?.phone.number, phoneNumber);
const newPhoneNumber =
recipient.paymentInformation?.upsert?.update?.phone?.upsert?.update.number ||
recipient.paymentInformation?.upsert?.create?.phone?.create?.number;
const existingPhoneNumber = existing.paymentInformation?.phone?.number;
if (newPhoneNumber) {
if (!existingPhoneNumber) {
return this.resultFail('No phone number available for recipient update');
}
await this.firebaseAuthService.updateByPhoneNumber(existingPhoneNumber, newPhoneNumber);
}
🤖 Prompt for AI Agents
In shared/src/database/services/recipient/recipient.service.ts around lines
70-78, the current guard returns resultFail when phoneNumber is undefined which
blocks updates that don't change the phone; change the logic to NOT return when
phoneNumber is undefined and only call firebaseAuthService.updateByPhoneNumber
if a new phoneNumber is supplied and it is different from
existing.paymentInformation?.phone?.number (i.e., if phoneNumber && phoneNumber
!== existing.paymentInformation?.phone?.number). If phoneNumber is undefined,
proceed with the database update path; if phoneNumber is provided and existing
number is missing or different, call firebaseAuthService.updateByPhoneNumber
with the existing number (if any) and the new phoneNumber, otherwise skip the
Firebase call.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
firestore_to_postgres/src/recipient/recipient.transformer.ts (1)

127-130: Helper is correct but duplicates logic in payout transformer.

The E.164 formatting logic is sound. However, this duplicates the same formatting in payout.transformer.ts line 24. Consider extracting to a shared utility as noted in the payout transformer review.

firestore_to_postgres/src/payout/payout.transformer.ts (1)

24-24: Extract duplicate E.164 phone formatting to a shared utility function.

The phone formatting logic (+${phoneNumber}) is duplicated: payout.transformer.ts applies it inline (line 24), while recipient.transformer.ts encapsulates it in a mapPhoneNumber method (lines 127–130). The template literal safely coerces both string and number types, so removing .toString() poses no type issues. However, consolidating this logic into a shared utility would eliminate duplication and improve maintainability.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 145273a and ff962a1.

📒 Files selected for processing (2)
  • firestore_to_postgres/src/payout/payout.transformer.ts (1 hunks)
  • firestore_to_postgres/src/recipient/recipient.transformer.ts (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Security checks (typescript)
  • GitHub Check: Prettify
🔇 Additional comments (1)
firestore_to_postgres/src/recipient/recipient.transformer.ts (1)

19-29: LGTM - Proper E.164 formatting for optional communication phone.

The helper correctly formats phone numbers to E.164 and appropriately returns undefined when no phone exists.

Comment on lines 31 to +46
const paymentPhone = raw.mobile_money_phone?.phone
? {
connectOrCreate: {
where: { number: raw.mobile_money_phone.phone.toString() },
where: { number: this.mapPhoneNumber(raw.mobile_money_phone.phone) },
create: {
number: raw.mobile_money_phone.phone.toString(),
number: this.mapPhoneNumber(raw.mobile_money_phone.phone),
hasWhatsApp: raw.mobile_money_phone.has_whatsapp,
},
},
}
: undefined;
: {
create: {
number: '',
hasWhatsApp: false,
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Empty string violates E.164 format and Firebase Auth requirements.

Creating a phone record with number: '' when no phone exists violates the E.164 format requirement stated in the PR objectives. Firebase Auth will reject empty phone numbers, causing the auth sync to fail.

If phone is now mandatory for PaymentInformation, the transformer should either:

  1. Skip recipients without valid payment phone numbers
  2. Fail with a clear error message

Apply this diff to skip recipients without payment phones:

  const paymentPhone = raw.mobile_money_phone?.phone
    ? {
        connectOrCreate: {
          where: { number: this.mapPhoneNumber(raw.mobile_money_phone.phone) },
          create: {
            number: this.mapPhoneNumber(raw.mobile_money_phone.phone),
            hasWhatsApp: raw.mobile_money_phone.has_whatsapp,
          },
        },
      }
-   : {
-       create: {
-         number: '',
-         hasWhatsApp: false,
-       },
-     };
+   : null;
+
+ if (!paymentPhone) {
+   console.log(`⚠️ Skipping recipient ${raw.id} - missing payment phone`);
+   continue;
+ }

Committable suggestion skipped: line range outside the PR's diff.

@rawi96 rawi96 merged commit 4ba225e into main Nov 3, 2025
14 checks passed
@rawi96 rawi96 deleted the feat/recipients-auth-sync branch November 3, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants