payment gateways and plans billing and signup pages refactored
This commit is contained in:
IGNY8 VPS (Salman)
1120
docs/90-REFERENCE/PAYMENT-SYSTEM-ARCHITECTURE.md
Normal file
1120
docs/90-REFERENCE/PAYMENT-SYSTEM-ARCHITECTURE.md
Normal file
File diff suppressed because it is too large
Load Diff
959
docs/90-REFERENCE/PAYMENT-SYSTEM-AUDIT-REPORT.md
Normal file
959
docs/90-REFERENCE/PAYMENT-SYSTEM-AUDIT-REPORT.md
Normal file
@@ -0,0 +1,959 @@
|
||||
# Payment System Audit Report
|
||||
|
||||
> **Audit Date:** January 7, 2026
|
||||
> **Status:** Complete (Updated with Frontend UX Audit)
|
||||
> **Severity Levels:** CRITICAL | HIGH | MEDIUM | LOW
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The IGNY8 payment system documentation is **accurate and matches the implementation**. However, the deep audit revealed **45+ issues** across security, reliability, UX, and functionality areas. The most critical concerns involve:
|
||||
|
||||
1. **PayPal webhook signature verification disabled** (security risk)
|
||||
2. **Missing idempotency in payment processing** (double-charge risk)
|
||||
3. **No admin dashboard for manual payment approval** (operational gap)
|
||||
4. **Plan shows "Active" even with unpaid invoice** (misleading UX)
|
||||
5. **Payment options not properly restricted by state** (UX confusion)
|
||||
6. **Hardcoded currency exchange rates** (financial accuracy)
|
||||
7. **Refund functions reference non-existent modules** (broken feature)
|
||||
|
||||
---
|
||||
|
||||
## Documentation Verification
|
||||
|
||||
### All Documented Files: VERIFIED
|
||||
|
||||
| Category | File | Status |
|
||||
|----------|------|--------|
|
||||
| Frontend Entry Points | SignUpFormUnified.tsx | EXISTS |
|
||||
| | PlansAndBillingPage.tsx | EXISTS |
|
||||
| | PayInvoiceModal.tsx | EXISTS |
|
||||
| | PendingPaymentBanner.tsx | EXISTS |
|
||||
| Frontend Services | billing.api.ts | EXISTS |
|
||||
| | authStore.ts | EXISTS |
|
||||
| Backend Views | stripe_views.py | EXISTS |
|
||||
| | paypal_views.py | EXISTS |
|
||||
| Backend Services | stripe_service.py | EXISTS |
|
||||
| | paypal_service.py | EXISTS |
|
||||
| | payment_service.py | EXISTS |
|
||||
| | invoice_service.py | EXISTS |
|
||||
| Models | billing/models.py | EXISTS |
|
||||
| | auth/models.py | EXISTS |
|
||||
|
||||
### Country-Based Payment Logic: CORRECT
|
||||
|
||||
- **Pakistan (PK):** Stripe + Bank Transfer (NO PayPal)
|
||||
- **Global:** Stripe + PayPal
|
||||
|
||||
Logic correctly implemented in:
|
||||
- `SignUpFormUnified.tsx:160-186`
|
||||
- `PayInvoiceModal.tsx:69-97`
|
||||
- `payment_service.py:260-263`
|
||||
|
||||
---
|
||||
|
||||
## PlansAndBillingPage UX Audit (NEW)
|
||||
|
||||
### Overview
|
||||
|
||||
The `/account/plans` page (`PlansAndBillingPage.tsx`) is the central hub for subscription management. This audit identifies **critical UX issues** related to state handling, invoice lifecycle, and payment option restrictions.
|
||||
|
||||
---
|
||||
|
||||
### CRITICAL UX Issue #1: Plan Shows "Active" Even With Unpaid Invoice
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:459-461`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
<Badge variant="solid" tone={hasActivePlan ? 'success' : 'warning'}>
|
||||
{hasActivePlan ? 'Active' : 'Inactive'}
|
||||
</Badge>
|
||||
```
|
||||
|
||||
**Problem:**
|
||||
- `hasActivePlan` is `true` if user has ANY plan assigned (Line 384)
|
||||
- User who signed up for paid plan but never completed payment sees **"Active"**
|
||||
- This is misleading - their account is actually `pending_payment`
|
||||
|
||||
**Correct Logic Should Check:**
|
||||
1. `account.status === 'active'` (not just plan existence)
|
||||
2. No pending invoices
|
||||
3. Subscription status is `active` (not `pending_payment`)
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
const accountStatus = user?.account?.status;
|
||||
const subscriptionStatus = currentSubscription?.status;
|
||||
const isFullyActive = accountStatus === 'active' &&
|
||||
subscriptionStatus === 'active' &&
|
||||
!hasPendingInvoice;
|
||||
|
||||
<Badge variant="solid" tone={isFullyActive ? 'success' : accountStatus === 'pending_payment' ? 'warning' : 'error'}>
|
||||
{isFullyActive ? 'Active' : accountStatus === 'pending_payment' ? 'Pending Payment' : 'Inactive'}
|
||||
</Badge>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### CRITICAL UX Issue #2: Subscription States Not Properly Reflected
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:379-386`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
const currentSubscription = subscriptions.find((sub) => sub.status === 'active') || subscriptions[0];
|
||||
// ...
|
||||
const hasActivePlan = Boolean(effectivePlanId);
|
||||
```
|
||||
|
||||
**Problem:**
|
||||
The page doesn't distinguish between subscription statuses:
|
||||
- `active` - Paid and working
|
||||
- `pending_payment` - Waiting for first payment
|
||||
- `past_due` - Renewal payment failed
|
||||
- `canceled` - User cancelled
|
||||
|
||||
**Missing Status Handling:**
|
||||
|
||||
| Subscription Status | What User Sees | What They SHOULD See |
|
||||
|---------------------|----------------|----------------------|
|
||||
| `pending_payment` | "Active" badge | "Payment Required" with prominent CTA |
|
||||
| `past_due` | No indication | "Payment Overdue" warning |
|
||||
| `canceled` | May still show "Active" | "Cancels on [date]" |
|
||||
| `trialing` | "Active" | "Trial (X days left)" |
|
||||
|
||||
**Fix Required:** Add comprehensive status display:
|
||||
```tsx
|
||||
const getSubscriptionDisplay = () => {
|
||||
if (!currentSubscription) return { label: 'No Plan', tone: 'error' };
|
||||
|
||||
switch (currentSubscription.status) {
|
||||
case 'active':
|
||||
return hasPendingInvoice
|
||||
? { label: 'Payment Due', tone: 'warning' }
|
||||
: { label: 'Active', tone: 'success' };
|
||||
case 'pending_payment':
|
||||
return { label: 'Awaiting Payment', tone: 'warning' };
|
||||
case 'past_due':
|
||||
return { label: 'Payment Overdue', tone: 'error' };
|
||||
case 'canceled':
|
||||
return { label: `Cancels ${formatDate(currentSubscription.cancel_at)}`, tone: 'warning' };
|
||||
case 'trialing':
|
||||
return { label: `Trial`, tone: 'info' };
|
||||
default:
|
||||
return { label: currentSubscription.status, tone: 'neutral' };
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### HIGH UX Issue #3: Upgrade Button Available When Payment Pending
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:478-486`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
<Button
|
||||
variant="primary"
|
||||
tone="brand"
|
||||
onClick={() => setShowUpgradeModal(true)}
|
||||
startIcon={<ArrowUpIcon className="w-4 h-4" />}
|
||||
>
|
||||
Upgrade
|
||||
</Button>
|
||||
```
|
||||
|
||||
**Problem:** User with pending invoice can click "Upgrade" and attempt to subscribe to another plan, creating confusion and potentially duplicate subscriptions.
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
<Button
|
||||
variant="primary"
|
||||
tone="brand"
|
||||
onClick={() => setShowUpgradeModal(true)}
|
||||
disabled={hasPendingInvoice || accountStatus === 'pending_payment'}
|
||||
startIcon={<ArrowUpIcon className="w-4 h-4" />}
|
||||
>
|
||||
{hasPendingInvoice ? 'Pay Invoice First' : 'Upgrade'}
|
||||
</Button>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### HIGH UX Issue #4: Cancel Subscription Available When Account Already Pending
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:609-616`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
{hasActivePlan && (
|
||||
<button onClick={() => setShowCancelConfirm(true)} ...>
|
||||
Cancel Subscription
|
||||
</button>
|
||||
)}
|
||||
```
|
||||
|
||||
**Problem:** User with `pending_payment` status can "cancel" a subscription they never paid for. This is confusing.
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
{hasActivePlan && accountStatus === 'active' && !hasPendingInvoice && (
|
||||
<button onClick={() => setShowCancelConfirm(true)} ...>
|
||||
Cancel Subscription
|
||||
</button>
|
||||
)}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### HIGH UX Issue #5: "Manage Billing" Button Shown to Non-Stripe Users
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:468-477`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
{availableGateways.stripe && hasActivePlan && (
|
||||
<Button ... onClick={handleManageSubscription}>
|
||||
Manage Billing
|
||||
</Button>
|
||||
)}
|
||||
```
|
||||
|
||||
**Problem:**
|
||||
- Shows "Manage Billing" if Stripe is available, even if user pays via Bank Transfer
|
||||
- Bank Transfer users clicking this get error "No Stripe customer ID"
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
{availableGateways.stripe &&
|
||||
hasActivePlan &&
|
||||
user?.account?.stripe_customer_id &&
|
||||
selectedPaymentMethod === 'stripe' && (
|
||||
<Button ... onClick={handleManageSubscription}>
|
||||
Manage Billing
|
||||
</Button>
|
||||
)}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### HIGH UX Issue #6: Credits Section Doesn't Show Pending Credit Purchases
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:689-713`
|
||||
|
||||
**Problem:** If user purchased credits via bank transfer and it's `pending_approval`, they don't see this anywhere clearly. They might try to purchase again.
|
||||
|
||||
**Fix Required:** Add pending credits indicator:
|
||||
```tsx
|
||||
{pendingCreditPayments.length > 0 && (
|
||||
<div className="mb-4 p-3 bg-info-50 border border-info-200 rounded-lg">
|
||||
<p className="text-sm text-info-700">
|
||||
You have {pendingCreditPayments.length} credit purchase(s) pending approval
|
||||
</p>
|
||||
</div>
|
||||
)}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM UX Issue #7: Invoice Status Badge Colors Inconsistent
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:817-819`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
<Badge variant="soft" tone={invoice.status === 'paid' ? 'success' : 'warning'}>
|
||||
{invoice.status}
|
||||
</Badge>
|
||||
```
|
||||
|
||||
**Problem:** Only handles `paid` and everything else is `warning`. Missing:
|
||||
- `draft` - Should be gray/neutral
|
||||
- `void` - Should be gray
|
||||
- `uncollectible` - Should be error/red
|
||||
- `pending` - Warning (correct)
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
const getInvoiceStatusTone = (status: string) => {
|
||||
switch (status) {
|
||||
case 'paid': return 'success';
|
||||
case 'pending': return 'warning';
|
||||
case 'void':
|
||||
case 'draft': return 'neutral';
|
||||
case 'uncollectible': return 'error';
|
||||
default: return 'neutral';
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM UX Issue #8: No Clear Indication of Payment Method per Invoice
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:809-849` (Invoice table)
|
||||
|
||||
**Problem:** Invoice table doesn't show which payment method was used/expected. User can't tell if they need to do bank transfer or card payment.
|
||||
|
||||
**Fix Required:** Add payment method column:
|
||||
```tsx
|
||||
<td className="px-6 py-3 text-center">
|
||||
{invoice.payment_method === 'bank_transfer' ? (
|
||||
<span className="flex items-center gap-1"><Building2Icon className="w-4 h-4" /> Bank</span>
|
||||
) : invoice.payment_method === 'paypal' ? (
|
||||
<span className="flex items-center gap-1"><WalletIcon className="w-4 h-4" /> PayPal</span>
|
||||
) : (
|
||||
<span className="flex items-center gap-1"><CreditCardIcon className="w-4 h-4" /> Card</span>
|
||||
)}
|
||||
</td>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM UX Issue #9: Renewal Date Shows Even When No Active Subscription
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:514-522`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
<div className="text-2xl font-bold text-gray-900 dark:text-white">
|
||||
{currentSubscription?.current_period_end
|
||||
? new Date(currentSubscription.current_period_end).toLocaleDateString(...)
|
||||
: '—'}
|
||||
</div>
|
||||
<div className="text-xs text-gray-500 dark:text-gray-400 mt-1">Next billing</div>
|
||||
```
|
||||
|
||||
**Problem:** Shows "—" and "Next billing" even when:
|
||||
- Account is `pending_payment` (never billed yet)
|
||||
- Subscription is `canceled` (won't renew)
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
<div className="text-xs text-gray-500 dark:text-gray-400 mt-1">
|
||||
{currentSubscription?.status === 'canceled' ? 'Ends on' :
|
||||
currentSubscription?.status === 'pending_payment' ? 'Starts after payment' :
|
||||
'Next billing'}
|
||||
</div>
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### MEDIUM UX Issue #10: Payment Gateway Selection Not Synced With Account
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:655-686`
|
||||
|
||||
**Problem:** Payment method selector in "Buy Credits" section can show options the user hasn't verified. If user signed up with bank transfer, they shouldn't see PayPal as an option until they've added it.
|
||||
|
||||
**Current Logic:** Shows all `availableGateways` regardless of user's `AccountPaymentMethod` records.
|
||||
|
||||
**Fix Required:**
|
||||
```tsx
|
||||
// Only show gateways user has verified OR is willing to add
|
||||
const userCanUseGateway = (gateway: PaymentGateway) => {
|
||||
const userHasMethod = userPaymentMethods.some(m =>
|
||||
(gateway === 'stripe' && m.type === 'stripe') ||
|
||||
(gateway === 'paypal' && m.type === 'paypal') ||
|
||||
(gateway === 'manual' && ['bank_transfer', 'local_wallet'].includes(m.type))
|
||||
);
|
||||
return availableGateways[gateway] && (userHasMethod || gateway === selectedGateway);
|
||||
};
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### LOW UX Issue #11: Annual Billing Toggle Does Nothing
|
||||
|
||||
**Location:** `PlansAndBillingPage.tsx:959-983`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
const displayPrice = selectedBillingCycle === 'annual' ? (annualPrice / 12).toFixed(0) : planPrice;
|
||||
```
|
||||
|
||||
**Problem:**
|
||||
- Shows "Save 20%" badge for annual
|
||||
- Calculates display price
|
||||
- But **no annual plans exist** in database
|
||||
- Clicking "Choose Plan" subscribes to monthly regardless
|
||||
|
||||
**Fix Required:** Either:
|
||||
1. Remove annual toggle until annual plans implemented
|
||||
2. Implement annual plan variants in backend
|
||||
3. Pass `billing_cycle` to `subscribeToPlan()` and handle in backend
|
||||
|
||||
---
|
||||
|
||||
### LOW UX Issue #12: PayInvoiceModal Hardcodes Bank Details
|
||||
|
||||
**Location:** `PayInvoiceModal.tsx:437-443`
|
||||
|
||||
**Current Code:**
|
||||
```tsx
|
||||
<p><span className="font-medium">Bank:</span> Standard Chartered Bank Pakistan</p>
|
||||
<p><span className="font-medium">Account Title:</span> IGNY8 Technologies</p>
|
||||
<p><span className="font-medium">Account #:</span> 01-2345678-01</p>
|
||||
```
|
||||
|
||||
**Problem:** Bank details hardcoded in frontend. Should come from `PaymentMethodConfig` in backend.
|
||||
|
||||
**Fix Required:** Fetch bank details from `/v1/billing/payment-configs/payment-methods/?country_code=PK&payment_method=bank_transfer` and display dynamically.
|
||||
|
||||
---
|
||||
|
||||
### Account Lifecycle State Machine (Missing)
|
||||
|
||||
The page doesn't follow a clear state machine. Here's what it SHOULD be:
|
||||
|
||||
```
|
||||
┌─────────────────────────────────────────────────────────────────────┐
|
||||
│ ACCOUNT STATES │
|
||||
├─────────────────────────────────────────────────────────────────────┤
|
||||
│ │
|
||||
│ ┌──────────┐ Payment ┌──────────┐ Payment ┌──────────┐ │
|
||||
│ │ trial │ ────────────▶ │ pending_ │ ────────────▶ │ active │ │
|
||||
│ │ │ Required │ payment │ Success │ │ │
|
||||
│ └──────────┘ └──────────┘ └──────────┘ │
|
||||
│ │ │ │ │
|
||||
│ │ │ Payment │ │
|
||||
│ │ │ Failed │ │
|
||||
│ ▼ ▼ ▼ │
|
||||
│ ┌──────────┐ ┌──────────┐ ┌──────────┐ │
|
||||
│ │ expired │ │suspended │ ◀─────────── │ past_due │ │
|
||||
│ │ │ │ │ Auto │ │ │
|
||||
│ └──────────┘ └──────────┘ Suspend └──────────┘ │
|
||||
│ │ │ │
|
||||
│ │ Admin │ │
|
||||
│ │ Action │ │
|
||||
│ ▼ │ │
|
||||
│ ┌──────────┐ │ │
|
||||
│ │cancelled │ ◀──────────────────┘ │
|
||||
│ │ │ User Cancel │
|
||||
│ └──────────┘ │
|
||||
│ │
|
||||
└─────────────────────────────────────────────────────────────────────┘
|
||||
```
|
||||
|
||||
**Each State Should Show:**
|
||||
|
||||
| State | Plan Badge | Actions Available | Warnings |
|
||||
|-------|------------|-------------------|----------|
|
||||
| `trial` | "Trial (X days)" | Upgrade, Buy Credits | "Trial ends [date]" |
|
||||
| `pending_payment` | "Awaiting Payment" | Pay Invoice, Change Method | "Complete payment to activate" |
|
||||
| `active` | "Active" | Upgrade, Buy Credits, Cancel, Manage | None |
|
||||
| `past_due` | "Payment Overdue" | Update Payment, Pay Now | "Update payment to avoid suspension" |
|
||||
| `suspended` | "Suspended" | Pay to Reactivate | "Account suspended - pay to restore" |
|
||||
| `cancelled` | "Cancels [date]" | Resubscribe | "Access ends [date]" |
|
||||
|
||||
---
|
||||
|
||||
### Payment Options By State (Missing Restrictions)
|
||||
|
||||
**Current:** All payment options shown regardless of account state.
|
||||
|
||||
**Required Restrictions:**
|
||||
|
||||
| Account State | Can Upgrade? | Can Buy Credits? | Can Change Plan? | Can Cancel? |
|
||||
|---------------|--------------|------------------|------------------|-------------|
|
||||
| `trial` | Yes | Yes | N/A | No |
|
||||
| `pending_payment` | No (Pay first) | No (Pay first) | No | No |
|
||||
| `active` | Yes | Yes | Yes | Yes |
|
||||
| `past_due` | No (Pay first) | No | No | Yes |
|
||||
| `suspended` | No | No | No | No |
|
||||
| `cancelled` | Yes (Resubscribe) | No | No | No |
|
||||
|
||||
---
|
||||
|
||||
### Summary of PlansAndBillingPage Fixes Needed
|
||||
|
||||
| # | Issue | Severity | Effort |
|
||||
|---|-------|----------|--------|
|
||||
| 1 | Plan shows "Active" with unpaid invoice | CRITICAL | 30 min |
|
||||
| 2 | Subscription states not reflected | CRITICAL | 1 hr |
|
||||
| 3 | Upgrade available when payment pending | HIGH | 15 min |
|
||||
| 4 | Cancel available for unpaid subscriptions | HIGH | 15 min |
|
||||
| 5 | Manage Billing shown to non-Stripe users | HIGH | 15 min |
|
||||
| 6 | No pending credit purchase indicator | HIGH | 30 min |
|
||||
| 7 | Invoice status colors inconsistent | MEDIUM | 15 min |
|
||||
| 8 | No payment method shown per invoice | MEDIUM | 30 min |
|
||||
| 9 | Renewal date context wrong | MEDIUM | 15 min |
|
||||
| 10 | Gateway selection not synced | MEDIUM | 30 min |
|
||||
| 11 | Annual billing does nothing | LOW | 2 hrs |
|
||||
| 12 | Bank details hardcoded | LOW | 1 hr |
|
||||
|
||||
---
|
||||
|
||||
## Critical Issues (Immediate Action Required)
|
||||
|
||||
### 1. PayPal Webhook Signature Not Enforced
|
||||
|
||||
**Location:** `paypal_views.py:498-511`
|
||||
|
||||
**Problem:**
|
||||
```python
|
||||
if not is_valid:
|
||||
logger.warning("PayPal webhook signature verification failed")
|
||||
# Optionally reject invalid signatures
|
||||
# return Response({'error': 'Invalid signature'}, status=400) # COMMENTED OUT!
|
||||
```
|
||||
|
||||
**Risk:** Malicious actors can craft fake webhook events to:
|
||||
- Approve payments that never happened
|
||||
- Cancel legitimate subscriptions
|
||||
- Add credits without payment
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
if not is_valid:
|
||||
logger.error("PayPal webhook signature verification failed")
|
||||
return Response({'error': 'Invalid signature'}, status=400) # UNCOMMENT
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 2. Stripe Webhook Not Idempotent (Double-Charge Risk)
|
||||
|
||||
**Location:** `stripe_views.py:380-391` (_handle_checkout_completed)
|
||||
|
||||
**Problem:** Webhook can be called multiple times for same event. No check prevents duplicate invoice/payment creation.
|
||||
|
||||
**Scenario:**
|
||||
1. Stripe sends webhook
|
||||
2. Invoice and payment created
|
||||
3. Stripe retries webhook (network timeout)
|
||||
4. **Duplicate invoice and payment created**
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
# At start of _handle_checkout_completed:
|
||||
session_id = session.get('id')
|
||||
if Payment.objects.filter(stripe_checkout_session_id=session_id).exists():
|
||||
logger.info(f"Webhook already processed for session {session_id}")
|
||||
return # Already processed
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 3. PayPal Capture Order Not Idempotent
|
||||
|
||||
**Location:** `paypal_views.py:261-365` (PayPalCaptureOrderView)
|
||||
|
||||
**Problem:** If frontend calls `/capture-order/` twice (network timeout), payment captured twice.
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
# Check if order already captured
|
||||
existing = Payment.objects.filter(paypal_order_id=order_id, status='succeeded').first()
|
||||
if existing:
|
||||
return Response({'status': 'already_captured', 'payment_id': existing.id})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 4. Refund Functions Call Non-Existent Modules
|
||||
|
||||
**Location:** `refund_views.py:160-208`
|
||||
|
||||
**Problem:**
|
||||
```python
|
||||
from igny8_core.business.billing.utils.payment_gateways import get_stripe_client # DOESN'T EXIST
|
||||
from igny8_core.business.billing.utils.payment_gateways import get_paypal_client # DOESN'T EXIST
|
||||
```
|
||||
|
||||
**Risk:** Refunds marked as processed but **never actually charged back** to customer.
|
||||
|
||||
**Fix Required:** Create the missing modules or use existing service classes:
|
||||
```python
|
||||
from igny8_core.business.billing.services.stripe_service import StripeService
|
||||
from igny8_core.business.billing.services.paypal_service import PayPalService
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 5. Amount Validation Missing for PayPal
|
||||
|
||||
**Location:** `paypal_views.py:569`
|
||||
|
||||
**Problem:**
|
||||
```python
|
||||
amount = float(capture_result.get('amount', package.price)) # Trusts PayPal amount
|
||||
```
|
||||
|
||||
**Risk:** If PayPal returns wrong amount, system processes it as correct.
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
captured_amount = float(capture_result.get('amount', 0))
|
||||
expected_amount = float(package.price)
|
||||
if abs(captured_amount - expected_amount) > 0.01: # Allow 1 cent tolerance
|
||||
logger.error(f"Amount mismatch: captured={captured_amount}, expected={expected_amount}")
|
||||
return Response({'error': 'Amount mismatch'}, status=400)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## High Priority Issues
|
||||
|
||||
### 6. No Admin Dashboard for Pending Payments
|
||||
|
||||
**Problem:** Admins must use Django admin to approve manual payments.
|
||||
|
||||
**Missing Endpoint:**
|
||||
```
|
||||
GET /v1/admin/billing/pending-payments/ - List pending approvals
|
||||
POST /v1/admin/billing/payments/{id}/approve/ - Approve payment
|
||||
POST /v1/admin/billing/payments/{id}/reject/ - Reject payment
|
||||
```
|
||||
|
||||
**Required Implementation:**
|
||||
```python
|
||||
class AdminPaymentViewSet(viewsets.ModelViewSet):
|
||||
permission_classes = [IsAdminUser]
|
||||
|
||||
@action(detail=False, methods=['get'])
|
||||
def pending(self, request):
|
||||
payments = Payment.objects.filter(status='pending_approval')
|
||||
return Response(PaymentSerializer(payments, many=True).data)
|
||||
|
||||
@action(detail=True, methods=['post'])
|
||||
def approve(self, request, pk=None):
|
||||
payment = self.get_object()
|
||||
PaymentService.approve_manual_payment(payment, request.user.id, request.data.get('notes'))
|
||||
return Response({'status': 'approved'})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 7. Invoice Number Race Condition
|
||||
|
||||
**Location:** `invoice_service.py:52-78`
|
||||
|
||||
**Problem:**
|
||||
```python
|
||||
count = Invoice.objects.select_for_update().filter(...).count()
|
||||
invoice_number = f"{prefix}-{count + 1:04d}"
|
||||
while Invoice.objects.filter(invoice_number=invoice_number).exists(): # NOT LOCKED!
|
||||
count += 1
|
||||
```
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
# Use database unique constraint + retry logic
|
||||
@transaction.atomic
|
||||
def generate_invoice_number(account_id, invoice_type='SUB'):
|
||||
prefix = f"INV-{account_id}-{invoice_type}-{timezone.now().strftime('%Y%m')}"
|
||||
for attempt in range(5):
|
||||
count = Invoice.objects.filter(invoice_number__startswith=prefix).count()
|
||||
invoice_number = f"{prefix}-{count + 1:04d}"
|
||||
try:
|
||||
# Use get_or_create with unique constraint
|
||||
return invoice_number
|
||||
except IntegrityError:
|
||||
continue
|
||||
raise ValueError("Unable to generate unique invoice number")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 8. Browser Redirect Lost After Payment
|
||||
|
||||
**Problem:** If user closes browser after Stripe payment but before redirect:
|
||||
- Payment succeeds (webhook processes it)
|
||||
- User doesn't know payment succeeded
|
||||
- May attempt to pay again
|
||||
|
||||
**Fix Required:** Add payment status check endpoint:
|
||||
```python
|
||||
# New endpoint
|
||||
GET /v1/billing/payment-status/{session_id}/
|
||||
|
||||
# Frontend should check this on /account/plans load
|
||||
const checkPaymentStatus = async (sessionId) => {
|
||||
const response = await fetch(`/v1/billing/payment-status/${sessionId}/`);
|
||||
if (response.data.status === 'completed') {
|
||||
toast.success('Your previous payment was successful!');
|
||||
refreshUser();
|
||||
}
|
||||
};
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 9. Subscription Renewal Gets Stuck
|
||||
|
||||
**Location:** `subscription_renewal.py:78-131`
|
||||
|
||||
**Problem:** Status set to `pending_renewal` with no expiry or retry mechanism.
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
# Add Celery task
|
||||
@app.task
|
||||
def check_stuck_renewals():
|
||||
"""Run daily to check for stuck renewals"""
|
||||
stuck = Subscription.objects.filter(
|
||||
status='pending_renewal',
|
||||
metadata__renewal_required_at__lt=timezone.now() - timedelta(days=7)
|
||||
)
|
||||
for sub in stuck:
|
||||
# Send reminder email
|
||||
send_renewal_reminder(sub)
|
||||
# After 14 days, suspend
|
||||
if sub.metadata.get('renewal_required_at') < timezone.now() - timedelta(days=14):
|
||||
sub.status = 'past_due'
|
||||
sub.account.status = 'suspended'
|
||||
sub.save()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 10. Currency Exchange Rates Hardcoded
|
||||
|
||||
**Location:** `currency.py:137-157`
|
||||
|
||||
**Problem:**
|
||||
```python
|
||||
CURRENCY_MULTIPLIERS = {
|
||||
'PKR': 278.0, # STATIC - real rate changes daily!
|
||||
'INR': 83.0,
|
||||
# ...
|
||||
}
|
||||
```
|
||||
|
||||
**Risk:** Users charged incorrect amounts over time.
|
||||
|
||||
**Fix Required:**
|
||||
```python
|
||||
class ExchangeRateService:
|
||||
CACHE_KEY = 'exchange_rates'
|
||||
CACHE_TTL = 86400 # 24 hours
|
||||
|
||||
@classmethod
|
||||
def get_rate(cls, currency):
|
||||
rates = cache.get(cls.CACHE_KEY)
|
||||
if not rates:
|
||||
rates = cls._fetch_from_api()
|
||||
cache.set(cls.CACHE_KEY, rates, cls.CACHE_TTL)
|
||||
return rates.get(currency, 1.0)
|
||||
|
||||
@classmethod
|
||||
def _fetch_from_api(cls):
|
||||
# Use OpenExchangeRates, Fixer.io, or similar
|
||||
response = requests.get('https://api.exchangerate-api.com/v4/latest/USD')
|
||||
return response.json()['rates']
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Medium Priority Issues
|
||||
|
||||
### 11. No Promo Code/Discount Support
|
||||
|
||||
**Missing Models:**
|
||||
```python
|
||||
class PromoCode(models.Model):
|
||||
code = models.CharField(max_length=50, unique=True)
|
||||
discount_type = models.CharField(choices=[('percent', '%'), ('fixed', '$')])
|
||||
discount_value = models.DecimalField(max_digits=10, decimal_places=2)
|
||||
valid_from = models.DateTimeField()
|
||||
valid_until = models.DateTimeField(null=True)
|
||||
max_uses = models.IntegerField(null=True)
|
||||
current_uses = models.IntegerField(default=0)
|
||||
applicable_plans = models.ManyToManyField('Plan', blank=True)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 12. No Partial Payment Support
|
||||
|
||||
**Current:** User must pay full invoice amount.
|
||||
|
||||
**Needed:**
|
||||
- Split invoice into multiple payments
|
||||
- Track partial payment progress
|
||||
- Handle remaining balance
|
||||
|
||||
---
|
||||
|
||||
### 13. No Dunning Management
|
||||
|
||||
**Missing:** When payment fails:
|
||||
- Day 1: Payment failed notification
|
||||
- Day 3: Retry attempt + reminder
|
||||
- Day 7: Second retry + warning
|
||||
- Day 14: Account suspension warning
|
||||
- Day 21: Account suspended
|
||||
|
||||
---
|
||||
|
||||
### 14. No Manual Payment Reference Uniqueness
|
||||
|
||||
**Location:** `models.py:487-490`
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
manual_reference = models.CharField(
|
||||
max_length=255,
|
||||
blank=True,
|
||||
unique=True, # ADD THIS
|
||||
null=True # Allow null for non-manual payments
|
||||
)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 15. Refund Credit Deduction Race Condition
|
||||
|
||||
**Location:** `refund_views.py:108-129`
|
||||
|
||||
**Fix:** Use `select_for_update()`:
|
||||
```python
|
||||
with transaction.atomic():
|
||||
account = Account.objects.select_for_update().get(id=payment.account_id)
|
||||
if account.credit_balance >= credits_to_deduct:
|
||||
account.credit_balance -= credits_to_deduct
|
||||
account.save()
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 16. Invoice Total Calculation Silent Failure
|
||||
|
||||
**Location:** `models.py:439-448`
|
||||
|
||||
**Problem:**
|
||||
```python
|
||||
except Exception:
|
||||
pass # SILENT FAILURE!
|
||||
```
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
except Exception as e:
|
||||
logger.error(f"Invalid line item in invoice {self.id}: {item}, error: {e}")
|
||||
raise ValueError(f"Invalid invoice line item: {item}")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 17. No Webhook Event Storage
|
||||
|
||||
**Missing:** All incoming webhooks should be stored for:
|
||||
- Audit trail
|
||||
- Replay on failure
|
||||
- Debugging
|
||||
|
||||
**Add Model:**
|
||||
```python
|
||||
class WebhookEvent(models.Model):
|
||||
event_id = models.CharField(max_length=255, unique=True)
|
||||
provider = models.CharField(max_length=20) # stripe, paypal
|
||||
event_type = models.CharField(max_length=100)
|
||||
payload = models.JSONField()
|
||||
processed = models.BooleanField(default=False)
|
||||
processed_at = models.DateTimeField(null=True)
|
||||
error_message = models.TextField(blank=True)
|
||||
created_at = models.DateTimeField(auto_now_add=True)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 18. No Payment Audit Trail for Rejections
|
||||
|
||||
**Missing Fields in Payment model:**
|
||||
```python
|
||||
rejected_by = models.ForeignKey('User', null=True, related_name='rejected_payments')
|
||||
rejected_at = models.DateTimeField(null=True)
|
||||
rejection_reason = models.TextField(blank=True)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Recommendations Summary
|
||||
|
||||
### Immediate (This Week)
|
||||
|
||||
| # | Issue | Effort | Impact |
|
||||
|---|-------|--------|--------|
|
||||
| 1 | Enable PayPal webhook signature verification | 5 min | CRITICAL |
|
||||
| 2 | Add Stripe webhook idempotency check | 30 min | CRITICAL |
|
||||
| 3 | Add PayPal capture idempotency check | 30 min | CRITICAL |
|
||||
| 4 | Fix refund module imports | 1 hr | CRITICAL |
|
||||
| 5 | Add PayPal amount validation | 30 min | CRITICAL |
|
||||
|
||||
### Short-Term (This Month)
|
||||
|
||||
| # | Issue | Effort | Impact |
|
||||
|---|-------|--------|--------|
|
||||
| 6 | Build admin pending payments dashboard | 1 day | HIGH |
|
||||
| 7 | Fix invoice number race condition | 2 hrs | HIGH |
|
||||
| 8 | Add payment status check endpoint | 2 hrs | HIGH |
|
||||
| 9 | Fix stuck renewal subscriptions | 1 day | HIGH |
|
||||
| 10 | Implement dynamic currency rates | 1 day | HIGH |
|
||||
|
||||
### Medium-Term (This Quarter)
|
||||
|
||||
| # | Issue | Effort | Impact |
|
||||
|---|-------|--------|--------|
|
||||
| 11 | Promo code system | 3 days | MEDIUM |
|
||||
| 12 | Partial payment support | 2 days | MEDIUM |
|
||||
| 13 | Dunning management | 2 days | MEDIUM |
|
||||
| 14 | Webhook event storage | 1 day | MEDIUM |
|
||||
| 15 | Payment audit trail | 1 day | MEDIUM |
|
||||
|
||||
---
|
||||
|
||||
## Architecture Assessment
|
||||
|
||||
### What's Good
|
||||
|
||||
1. **Clean separation** between frontend entry points, services, and backend
|
||||
2. **Country-based logic** correctly isolates Pakistan users from PayPal
|
||||
3. **Multi-gateway support** (Stripe, PayPal, Manual) well architected
|
||||
4. **Service layer abstraction** (`StripeService`, `PayPalService`, `PaymentService`)
|
||||
5. **Invoice and payment tracking** comprehensive
|
||||
6. **Webhook handlers** exist for both gateways
|
||||
|
||||
### What Needs Improvement
|
||||
|
||||
1. **Idempotency** - Critical for payment processing
|
||||
2. **Transaction safety** - Need more `@transaction.atomic()` and `select_for_update()`
|
||||
3. **Observability** - No webhook event storage, limited metrics
|
||||
4. **Admin tooling** - Manual payments need proper dashboard
|
||||
5. **Error handling** - Too many silent failures
|
||||
6. **Feature gaps** - No promo codes, partial payments, dunning
|
||||
|
||||
---
|
||||
|
||||
## Final Assessment
|
||||
|
||||
| Area | Rating | Notes |
|
||||
|------|--------|-------|
|
||||
| Documentation Accuracy | A | Matches codebase |
|
||||
| Security | C | Webhook verification gaps |
|
||||
| Reliability | C | Idempotency issues |
|
||||
| Completeness | B | Core features present |
|
||||
| Admin Experience | D | No proper dashboard |
|
||||
| User Experience | B | Good flows, missing status checks |
|
||||
| Code Quality | B | Good structure, some silent failures |
|
||||
|
||||
**Overall Grade: C+**
|
||||
|
||||
The payment system is functional but has critical security and reliability gaps that must be addressed before scaling. The architecture is sound, but implementation details need hardening.
|
||||
|
||||
---
|
||||
|
||||
## Quick Wins (Can Do Today)
|
||||
|
||||
1. Uncomment PayPal webhook signature rejection (5 min)
|
||||
2. Add `@transaction.atomic()` to all payment handlers (30 min)
|
||||
3. Add duplicate check before creating payments (30 min)
|
||||
4. Add unique constraint to `manual_reference` (migration)
|
||||
5. Remove silent `except: pass` blocks (30 min)
|
||||
|
||||
---
|
||||
|
||||
*Report generated by deep audit of IGNY8 payment system codebase.*
|
||||
1350
docs/90-REFERENCE/PAYMENT-SYSTEM-REFACTOR-PLAN.md
Normal file
1350
docs/90-REFERENCE/PAYMENT-SYSTEM-REFACTOR-PLAN.md
Normal file
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user