# Codebase Concerns

**Analysis Date:** 2026-02-13

## Tech Debt

### FinanceDocument Model Complexity
- **Issue:** `app/Models/FinanceDocument.php` is 882 lines with 27+ status constants and multiple workflows (approval, disbursement, recording, payment). Contains both old workflow (cashier→accountant) and new workflow (secretary→chair→board) fields and methods coexisting.
- **Files:** `app/Models/FinanceDocument.php` (lines 1-882)
- **Impact:** Hard to maintain, difficult to extend, high risk of introducing bugs in approval logic. Multiple overlapping status fields make state transitions error-prone.
- **Fix approach:**
  - Extract new workflow logic into separate `FinanceDocumentApprovalService` class (already partially exists in codebase)
  - Remove deprecated cashier/accountant approval workflow entirely once old code migrated
  - Consider using state machine package (e.g., `thiagoprz/eloquent-state-machine`) to manage state transitions

### Large Model Files
- **Issue:** Multiple models exceed 400+ lines: `Income` (467), `Document` (466), `Article` (460), `Member` (437), `Announcement` (427), `Issue` (363)
- **Files:** `app/Models/{Income,Document,Article,Member,Announcement,Issue}.php`
- **Impact:** Models are bloated with business logic; difficult to test and maintain
- **Fix approach:** Extract domain logic to service classes, keep models for data relationships only

### Backward Compatibility Overhead
- **Issue:** FinanceDocument maintains both old (cashier/accountant) and new (secretary/chair/board) approval workflows with deprecated methods marked `@deprecated`
- **Files:** `app/Models/FinanceDocument.php` (lines 551-568, 757+)
- **Impact:** Code duplication, confusing for developers, increases cognitive load
- **Fix approach:** Set migration deadline (e.g., March 2026), remove all deprecated methods and legacy status constants

## Known Bugs

### Site Revalidation Webhook Silent Failures
- **Symptoms:** Next.js static site may not update when articles/pages/documents are modified in Laravel admin
- **Files:** `app/Services/SiteRevalidationService.php` (lines 34-59)
- **Trigger:** Log warnings only at WARNING level when webhook fails; developers may not notice missed revalidations
- **Current state:** Catches exceptions but logs only warnings; webhook timeout is 5 seconds
- **Workaround:** Check Laravel logs manually for `"Site revalidation failed"` warnings
- **Fix approach:**
  - Queue webhook calls with retry logic (e.g., 3 retries with exponential backoff)
  - Log errors at ERROR level with alert notification
  - Add monitoring for failed revalidations

### Email Notifications May Fail Silently
- **Symptoms:** Users don't receive approval/rejection notifications (e.g., `FinanceDocumentFullyApproved`, `PaymentSubmittedMail`)
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 105-108, 174, 184, etc.)
- **Trigger:** Uses `Mail::queue()` without error handling or confirmation
- **Current state:** Emails queued but no verification of delivery
- **Fix approach:**
  - Add event listeners to track mail failures
  - Implement fallback notification channel (SMS via existing SMS service or in-app notifications)
  - Log all mail queue operations with request ID for audit trail

## Security Considerations

### National ID Decryption Exception Not Fully Handled
- **Risk:** Corrupted or mismatched encryption keys could cause decryption to fail; exception logged but member object may be in inconsistent state
- **Files:** `app/Models/Member.php` (lines 177-190)
- **Current mitigation:** Try-catch block logs error to Laravel log
- **Recommendations:**
  - Rotate encryption keys with migration strategy for existing encrypted data
  - Add health check command to verify all encrypted data can be decrypted
  - Implement decryption retry with fallback (query original source if available)
  - Document key rotation procedure in SECURITY.md

### Password Generation from Phone Number
- **Risk:** Password derived from last 4 digits of phone (`substr($phone, -4)`) is not cryptographically secure for initial user setup
- **Files:** `app/Console/Commands/ImportMembersCommand.php` (lines 135-146)
- **Current mitigation:** Used only during bulk import; users should change password after first login
- **Recommendations:**
  - Force password reset on first login (add `force_password_reset` flag to User model)
  - Use random password generation instead of phone-derived passwords
  - Send temporary password via email (encrypted) or secure channel, never via phone number

### Mail Headers Not Validated
- **Risk:** Site revalidation webhook uses unvalidated `x-revalidate-token` header; no CSRF protection
- **Files:** `app/Services/SiteRevalidationService.php` (line 54)
- **Current mitigation:** Token stored in `.env` file
- **Recommendations:**
  - Document token rotation process
  - Add rate limiting to revalidation endpoint
  - Consider using HMAC-SHA256 signed requests instead of bearer tokens

### Public Document UUID Leakage
- **Risk:** Documents have `public_uuid` exposed in API; uuid may be guessable if predictable
- **Files:** `app/Models/Document.php` (revalidation calls public_uuid)
- **Current mitigation:** Appears to use Laravel UUIDs (non-sequential)
- **Recommendations:**
  - Verify UUID generation uses `Illuminate\Support\Str::uuid()` (v4, cryptographically random)
  - Audit Document access control in `app/Http/Controllers/PublicDocumentController.php`

## Performance Bottlenecks

### Large Load in FinanceDocument Show Page
- **Problem:** `FinanceDocumentController::show()` eager loads 17 relationships, many of which may be unused in view
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 116-141)
- **Cause:** Over-eager loading; should load relationships based on view requirements
- **Improvement path:**
  - Split load into view-specific queries
  - Use `with()` conditionally based on user role
  - Measure N+1 queries with Laravel Debugbar

### N+1 Queries on Finance Index Listing
- **Problem:** FinanceDocument index loads 15 results with full relationships but filters happen in PHP, not database
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 20-56)
- **Cause:** Workflow stage filtering happens after query, not in WHERE clause
- **Improvement path:**
  - Move workflow_stage logic into Eloquent scopes
  - Use `whereRaw()` for date-based stage detection if necessary
  - Index database columns: `payment_order_created_at`, `payment_executed_at`, `cashier_ledger_entry_id`, `accounting_transaction_id`

### Import Commands Process Large Files In Memory
- **Problem:** `ImportMembersCommand`, `ImportAccountingData`, `ImportHugoContent` load entire Excel files into memory with PhpSpreadsheet
- **Files:** `app/Console/Commands/{ImportMembersCommand,ImportAccountingData,ImportHugoContent}.php`
- **Cause:** Using `IOFactory::load()` without streaming
- **Improvement path:**
  - Use chunked reading for large files (PhpSpreadsheet supports `ChunkReadFilter`)
  - Batch insert rows in groups of 100-500
  - Add progress bar using Artisan command

### Mail Queue Without Batch Processing
- **Problem:** Finance document approval sends individual emails in loop (e.g., line 105-108) without batching
- **Files:** `app/Http/Controllers/FinanceDocumentController.php` (lines 105-108, 182-185, 215-218)
- **Cause:** Loop creates separate Mail::queue() call for each cashier/chair/board member
- **Improvement path:**
  - Collect all recipients and send single batch notification
  - Use Mailable::withSymfonyMessage() to set BCC instead of TO:recipient loop
  - Consider notification channels for role-based sending

## Fragile Areas

### Multi-Tier Approval Workflow State Machine
- **Files:** `app/Models/FinanceDocument.php`, `app/Http/Controllers/FinanceDocumentController.php`
- **Why fragile:**
  - Complex conditional logic determining next approver (amount tier + status = next tier)
  - No atomic state transitions; multiple `update()` calls without transactions
  - Amount tier can change after approval (potential issue if amount modified)
  - Self-approval prevention logic repeated in multiple methods
- **Safe modification:**
  - Add database constraint: `UNIQUE(finance_documents, status, approved_by_secretary_id)` to prevent duplicate approvals
  - Wrap approval logic in explicit DB::transaction() in controller
  - Add test for each tier (small, medium, large) with all roles
- **Test coverage:** Good coverage exists (`tests/Unit/FinanceDocumentTest.php`) but missing edge cases

### Member Encryption/Decryption
- **Files:** `app/Models/Member.php` (lines 175-207)
- **Why fragile:**
  - Decryption failure returns null without distinction from empty field
  - No validation that decrypted data is valid national ID format
  - Hash function (SHA256) hardcoded; no version indicator if hashing algorithm changes
- **Safe modification:**
  - Create `MemberEncryption` helper class with version support
  - Add `$member->national_id_decryption_failed` flag to track failures
  - Validate decrypted ID format matches Taiwan ID requirements
- **Test coverage:** Missing - no tests for encryption/decryption failure scenarios

### Article Slug Generation with Chinese Characters
- **Files:** `app/Models/Article.php` (lines 84-109)
- **Why fragile:**
  - Slug fallback uses `Str::ascii()` which may produce empty string for pure Chinese titles
  - Final fallback `'article-'.time()` creates non-semantic slugs
  - No uniqueness guarantee if two articles created in same second
- **Safe modification:**
  - Use pinyin conversion library for Chinese titles
  - Always append counter to ensure uniqueness (current code does this, but starting from base slug not timestamp)
- **Test coverage:** Missing - no tests for Chinese title slug generation

### Document Access Control
- **Files:** `app/Http/Controllers/PublicDocumentController.php`, `app/Http/Controllers/Api/PublicDocumentController.php`
- **Why fragile:**
  - Document access depends on category and user role
  - Multiple access control points (controller + model) make audit trail difficult
  - No consistent logging of document access
- **Safe modification:**
  - Centralize access checks in `Document::canBeViewedBy()` method
  - Add audit logging to `DocumentAccessLog` table on every access
  - Use policy classes for clearer authorization
- **Test coverage:** Missing - no tests for document access control by category

## Scaling Limits

### SQLite in Development, MySQL in Production
- **Current capacity:** SQLite limited by single process/file locks
- **Limit:** Development environment may not catch MySQL-specific issues (JSON functions, full text search, etc.)
- **Scaling path:**
  - Add CI step running tests against MySQL
  - Use Docker Compose with MySQL for local development
  - Document differences in CLAUDE.md

### Audit Logging Unbounded Growth
- **Current capacity:** `AuditLog` table grows with every action (`AuditLogger::log()` called throughout codebase)
- **Limit:** No retention policy; table will grow indefinitely
- **Scaling path:**
  - Add scheduled cleanup command (`php artisan audit-logs:prune --days=90`)
  - Archive old logs to separate table or external storage
  - Add indexes on `auditable_id`, `auditable_type`, `created_at` for queries

### File Upload Storage
- **Current capacity:** Attachments stored in `storage/app/local` and `storage/app/public` without quota
- **Limit:** Disk space will fill with finance documents, articles, disability certificates
- **Scaling path:**
  - Migrate uploads to S3 or cloud storage
  - Add cleanup for soft-deleted documents
  - Implement virus scanning for uploaded files

## Dependencies at Risk

### barryvdh/laravel-dompdf (PDF Generation)
- **Risk:** Package maintained but based on Dompdf which has history of security issues in HTML/CSS parsing
- **Impact:** User-submitted content (article attachments, issue descriptions) rendered to PDF could be exploited
- **Migration plan:**
  - Sanitize HTML before PDF generation
  - Consider mPDF or TCPDF as alternatives
  - Test with OWASP XSS payloads in PDF generation

### maatwebsite/excel (Excel Import/Export)
- **Risk:** PhpSpreadsheet (underlying library) may have formula injection vulnerabilities
- **Impact:** Imported Excel files with formulas could execute on user machines
- **Migration plan:**
  - Strip formulas during import (`setReadDataOnly(true)`)
  - Validate cell content is not formula before importing
  - Document in user guide: "Do not import untrusted Excel files"

### simplesoftwareio/simple-qrcode
- **Risk:** Package has been archived; no active maintenance
- **Impact:** Potential security vulnerabilities won't be patched
- **Migration plan:**
  - Consider switch to `chillerlan/php-qrcode` (actively maintained)
  - Audit current QR generation code for security issues
  - Test QR code generation with large payloads

### spatie/laravel-permission (Role/Permission)
- **Risk:** Complex permission system; incorrect configuration could expose restricted data
- **Impact:** Member lifecycle, finance approvals depend entirely on permission configuration
- **Migration plan:**
  - Document all role/permission requirements in RBAC_SPECIFICATION.md
  - Add smoke test verifying each role can/cannot access correct routes
  - Audit permission checks with debugbar on each protected route

## Missing Critical Features

### No Rate Limiting on Finance Approval
- **Problem:** No rate limit prevents rapid approval/rejection spam
- **Blocks:** Audit trail integrity; potential for accidental approvals
- **Recommendation:** Add throttle middleware on `FinanceDocumentController::approve()` (1 approval per 2 seconds per user)

### No Email Delivery Confirmation
- **Problem:** System queues emails but provides no confirmation users received them
- **Blocks:** Important notifications (payment rejection, expense approval) may go unseen
- **Recommendation:** Implement email open tracking or delivery webhook with SendGrid/Mailgun

### No Database Transaction on Multi-Step Operations
- **Problem:** Finance document approval updates multiple fields in separate queries (lines 160-164)
- **Blocks:** Partial failures could leave document in inconsistent state
- **Recommendation:** Wrap all multi-step operations in explicit `DB::transaction()`

### No Async Job Retry Logic
- **Problem:** Site revalidation webhook has only catch block, no retry queue
- **Blocks:** Temporary network issues cause permanent cache inconsistency
- **Recommendation:** Use queued jobs with `ShouldBeEncrypted` and exponential backoff

## Test Coverage Gaps

### Finance Approval Multi-Tier Logic
- **What's not tested:** All tier combinations with role changes (e.g., chair approves small amount, medium amount requires board)
- **Files:** `app/Models/FinanceDocument.php` (determineAmountTier, canBeApprovedByChair, etc.)
- **Risk:** Tier logic changes could silently break approval chain
- **Priority:** High - this is financial workflow core

### Member Encryption Edge Cases
- **What's not tested:** Decryption failures, key rotation, migration from plaintext
- **Files:** `app/Models/Member.php` (getNationalIdAttribute, setNationalIdAttribute)
- **Risk:** Corrupted encryption keys could cause data loss
- **Priority:** High - financial impact

### Article Slug Generation for Chinese Titles
- **What's not tested:** Pure Chinese titles, mixed Chinese/English, special characters
- **Files:** `app/Models/Article.php` (generateUniqueSlug)
- **Risk:** Frontend routing breaks if slug generation fails
- **Priority:** Medium - affects CMS functionality

### Document Access Control by Category and Role
- **What's not tested:** Board members accessing member-only documents, public documents accessible without auth
- **Files:** `app/Http/Controllers/PublicDocumentController.php`, `app/Models/Document.php`
- **Risk:** Private documents exposed to wrong users
- **Priority:** High - security critical

### Email Notification Delivery
- **What's not tested:** Actual email sends, template rendering, missing email addresses
- **Files:** All Mail classes in `app/Mail/`
- **Risk:** Users don't know about approvals/rejections
- **Priority:** Medium - business critical but hard to test

---

*Concerns audit: 2026-02-13*