diff --git a/.architecture-rules/module.yaml b/.architecture-rules/module.yaml index 9114189f..22ff7548 100644 --- a/.architecture-rules/module.yaml +++ b/.architecture-rules/module.yaml @@ -141,7 +141,7 @@ module_rules: en.json de.json fr.json - lu.json + lb.json Translation keys are namespaced as {module}.key_name pattern: @@ -269,14 +269,14 @@ module_rules: Module locales/ directory should have translation files for all supported languages to ensure consistent i18n. - Supported languages: en, de, fr, lu + Supported languages: en, de, fr, lb Structure: app/modules//locales/ ├── en.json ├── de.json ├── fr.json - └── lu.json + └── lb.json Missing translations will fall back to English, but it's better to have all languages covered. @@ -286,7 +286,7 @@ module_rules: - "en.json" - "de.json" - "fr.json" - - "lu.json" + - "lb.json" - id: "MOD-007" name: "Module definition must match directory structure" diff --git a/app/modules/analytics/locales/lu.json b/app/modules/analytics/locales/lu.json deleted file mode 100644 index 0041a40b..00000000 --- a/app/modules/analytics/locales/lu.json +++ /dev/null @@ -1,17 +0,0 @@ -{ - "analytics": { - "page_title": "Analysen", - "dashboard_title": "Analyse-Dashboard", - "dashboard_subtitle": "Kuckt Är Buttek Leeschtungsmetriken an Abléck", - "period_7d": "Lescht 7 Deeg", - "period_30d": "Lescht 30 Deeg", - "period_90d": "Lescht 90 Deeg", - "period_1y": "Lescht Joer", - "imports_count": "Importer", - "products_added": "Produkter bäigesat", - "inventory_locations": "Lagerplazen", - "data_since": "Donnéeë vun", - "loading": "Analysen ginn gelueden...", - "error_loading": "Analysedonnéeën konnten net geluede ginn" - } -} diff --git a/app/modules/customers/locales/lu.json b/app/modules/customers/locales/lu.json deleted file mode 100644 index 0967ef42..00000000 --- a/app/modules/customers/locales/lu.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/app/modules/dev_tools/locales/lu.json b/app/modules/dev_tools/locales/lu.json deleted file mode 100644 index 0967ef42..00000000 --- a/app/modules/dev_tools/locales/lu.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/app/modules/inventory/locales/lu.json b/app/modules/inventory/locales/lu.json deleted file mode 100644 index 0967ef42..00000000 --- a/app/modules/inventory/locales/lu.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/app/modules/loyalty/docs/production-readiness.md b/app/modules/loyalty/docs/production-readiness.md new file mode 100644 index 00000000..f0ab4355 --- /dev/null +++ b/app/modules/loyalty/docs/production-readiness.md @@ -0,0 +1,134 @@ +# Loyalty Module — Production Readiness Report + +**Date:** 2026-03-13 +**Tests:** 213 passing | **Languages:** en, fr, de, lb (complete) + +--- + +## Overall Assessment: READY WITH RESERVATIONS + +The module has strong fundamentals — clean architecture, comprehensive tests, proper exception handling, full i18n (4 languages), wallet integration, and anti-fraud controls. All critical issues have been resolved. + +--- + +## CRITICAL — Must Fix Before Launch + +### ~~1. Race Conditions on Card Operations~~ — RESOLVED +Added `get_card_for_update()` method to `card_service.py` using `SELECT ... FOR UPDATE`. All 7 write methods in `stamp_service.py` (3) and `points_service.py` (4) now re-fetch the card with a row-level lock before modifying fields. + +### ~~2. Stale Locale File `lu.json`~~ — RESOLVED +All `lu.json` files deleted across the codebase. `lb` is the canonical Luxembourgish code (ISO 639-1). Architecture rules and docs updated. + +### ~~3. Console.log Statements in Storefront JS~~ — RESOLVED +All 16 `console.log/error/warn` calls replaced with `LogConfig.createLogger()` in 3 storefront JS files (dashboard, history, enroll). + +--- + +## HIGH — Should Fix Before Launch + +### 4. Rate Limiting on Public Endpoints +**Status:** Rate limiter middleware exists (`middleware/rate_limiter.py`) but is NOT applied to loyalty endpoints. + +**Vulnerable endpoints:** +- `POST /loyalty/enroll` — public, no auth required +- `POST /pins/{pin_id}/verify` — PIN lockout helps but doesn't prevent request flooding + +**Fix:** Apply `@rate_limit()` decorator to public loyalty routes. + +**Effort:** 1 hour + +### 5. T&C Strategy Unresolved +**Location:** `storefront/enroll.html` line 140 — TODO comment noting current approach (small text field on program model) won't scale for full legal T&C. + +**Options:** +- (A) Leverage CMS module to host T&C pages +- (B) Create dedicated T&C page within loyalty module +- (C) Accept current approach for MVP, plan post-launch + +**Effort:** Depends on option chosen + +### 6. Accessibility (ARIA Labels) +**Status:** Zero `aria-label` attributes across all loyalty templates. Icon-only buttons, modals, and form inputs lack screen reader support. + +**Priority areas:** +- PIN entry modal (terminal) — security-critical, needs aria-modal +- Icon-only action buttons (view, edit, delete in tables) +- Modal dialogs (delete confirmation, terms, barcode) + +**Effort:** 2-3 hours + +--- + +## MEDIUM — Address Post-Launch + +### 7. Point Expiration Task Performance +`point_expiration.py` processes cards one-by-one. With large card volumes, this could cause long-running DB locks. + +**Fix:** Batch processing with `LIMIT` and chunked commits. + +### 8. Wallet Sync Retry Logic +Wallet sync task doesn't retry failed individual cards. A transient API error skips the card until next hourly run. + +**Fix:** Add retry with exponential backoff per card. + +### 9. Audit Logging Enhancement +Current: LoyaltyTransaction captures operations but not all operations create transaction records (e.g., program config changes, PIN management). + +**Fix:** Add structured audit logging for admin/management operations. + +### 10. Test Coverage Metrics +Tests pass (213) but no coverage measurement. Unknown blind spots. + +**Fix:** Add `pytest-cov` and target >80% line coverage. + +--- + +## LOW / Nice-to-Have + +### 11. Field-Level Validation Errors +Current: Generic toast notifications for form errors. No field-specific error highlighting. + +### 12. Session Storage in Enroll-Success +`enroll-success.html` stores wallet URLs in sessionStorage. Fragile if user navigates away before page loads. + +### 13. Tier System (Future) +Model has `tier_config` field (Bronze/Silver/Gold) but no business logic implementation. Properly marked as future. + +--- + +## What's Already Solid + +| Area | Status | Details | +|------|--------|---------| +| Architecture | Excellent | Clean service layer, proper separation of concerns | +| Exception Handling | Excellent | 30+ custom exceptions, no bare except clauses | +| Authentication | Strong | All routes protected, role-based access | +| PIN Security | Strong | bcrypt hashing, lockout mechanism, timing-safe comparison | +| Wallet Security | Strong | JWT/PKCS#7 signing, env-based secrets, token validation | +| SQL Injection | Safe | SQLAlchemy ORM throughout, no raw SQL | +| Input Validation | Good | Pydantic schemas on all endpoints | +| Database Design | Excellent | Proper indexes, FKs, cascades, composite indexes | +| Tests | Good | 213 tests, unit + integration coverage | +| i18n | Complete | 4 languages (en/fr/de/lb), ~300 keys each | +| Dark Mode | Excellent | Consistent dark: variants on all elements | +| Responsive Design | Excellent | Mobile-first with proper breakpoints | +| Empty States | Excellent | All list views have proper empty states | +| Loading States | Excellent | Spinners and loading indicators everywhere | +| Confirmation Dialogs | Good | All destructive actions have modals | +| Pagination | Excellent | Full pagination on all list views | +| Documentation | Good | 5 docs (business logic, data model, user journeys, UI design, analysis) | + +--- + +## Pre-Launch Checklist + +- [x] ~~Fix race conditions with `with_for_update()` on card operations~~ — DONE +- [x] ~~Resolve `lu.json` vs `lb.json` locale situation~~ — DONE +- [x] ~~Replace console.log with LogConfig in 3 storefront JS files~~ — DONE +- [ ] Apply rate limiting to public enrollment endpoint +- [ ] Decide on T&C strategy (or accept current for MVP) +- [ ] Add basic ARIA labels to modals and icon-only buttons +- [ ] Manual QA: Walk through all 8 user journeys in `docs/user-journeys.md` +- [ ] Verify Google Wallet configuration in production environment +- [ ] Configure Apple Wallet (if needed for launch) or gracefully disable +- [ ] Review `LOYALTY_*` environment variables in production config diff --git a/app/modules/loyalty/locales/lu.json b/app/modules/loyalty/locales/lu.json deleted file mode 100644 index 33c81f50..00000000 --- a/app/modules/loyalty/locales/lu.json +++ /dev/null @@ -1,72 +0,0 @@ -{ - "loyalty": { - "module": { - "name": "Treieprogrammer", - "description": "Stempel- a punktebaséiert Treieprogrammer mat Wallet-Integratioun" - }, - "program": { - "title": "Treieprogramm", - "create": "Programm erstellen", - "edit": "Programm beaarbechten", - "activate": "Aktivéieren", - "deactivate": "Deaktivéieren", - "type": { - "stamps": "Stempelen", - "points": "Punkten", - "hybrid": "Hybrid" - } - }, - "card": { - "title": "Treiekaart", - "number": "Kaartennummer", - "qr_code": "QR-Code", - "enroll": "Client umellen", - "deactivate": "Kaart deaktivéieren" - }, - "stamp": { - "title": "Stempelen", - "add": "Stempel dobäisetzen", - "redeem": "Belounung aléisen", - "count": "{current} vun {target}", - "until_reward": "Nach {count} bis zur Belounung" - }, - "points": { - "title": "Punkten", - "earn": "Punkten sammelen", - "redeem": "Punkten aléisen", - "balance": "{count} Punkten", - "per_euro": "{points} Punkten pro Euro" - }, - "pin": { - "title": "Mataarbechter-PINen", - "create": "PIN erstellen", - "edit": "PIN beaarbechten", - "unlock": "PIN entspären", - "locked": "PIN gespaart bis {time}" - }, - "wallet": { - "google": "Bäi Google Wallet bäisetzen", - "apple": "Bäi Apple Wallet bäisetzen" - }, - "stats": { - "title": "Statistiken", - "total_cards": "Total Kaarten", - "active_cards": "Aktiv Kaarten", - "stamps_issued": "Ausgestallte Stempelen", - "rewards_redeemed": "Agelëst Belounungen" - }, - "errors": { - "program_not_found": "Treieprogramm net fonnt", - "program_inactive": "Treieprogramm ass net aktiv", - "card_not_found": "Treiekaart net fonnt", - "card_inactive": "Treiekaart ass net aktiv", - "cooldown": "Waart w.e.g. {minutes} Minutten virum nächste Stempel", - "daily_limit": "Dageslimit vun {limit} Stempelen erreecht", - "insufficient_stamps": "Brauch {required} Stempelen, hutt {current}", - "insufficient_points": "Brauch {required} Punkten, hutt {current}", - "pin_required": "Mataarbechter-PIN erfuerdert", - "pin_invalid": "Ongültege PIN", - "pin_locked": "PIN gespaart wéinst ze vill Feelverséich" - } - } -} diff --git a/app/modules/loyalty/services/card_service.py b/app/modules/loyalty/services/card_service.py index f0ab2b10..d3682c38 100644 --- a/app/modules/loyalty/services/card_service.py +++ b/app/modules/loyalty/services/card_service.py @@ -52,6 +52,19 @@ class CardService: .first() ) + def get_card_for_update(self, db: Session, card_id: int) -> LoyaltyCard | None: + """Get a loyalty card by ID with a row-level lock (SELECT ... FOR UPDATE). + + Note: Does not use joinedload to avoid LEFT OUTER JOIN which is + incompatible with FOR UPDATE in PostgreSQL. + """ + return ( + db.query(LoyaltyCard) + .filter(LoyaltyCard.id == card_id) + .with_for_update() + .first() + ) + def get_card_by_qr_code(self, db: Session, qr_code: str) -> LoyaltyCard | None: """Get a loyalty card by QR code data.""" return ( diff --git a/app/modules/loyalty/services/points_service.py b/app/modules/loyalty/services/points_service.py index f27e6edd..c5be01a5 100644 --- a/app/modules/loyalty/services/points_service.py +++ b/app/modules/loyalty/services/points_service.py @@ -140,6 +140,9 @@ class PointsService: "total_points_earned": card.total_points_earned, } + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Add points now = datetime.now(UTC) card.points_balance += points_earned @@ -271,6 +274,9 @@ class PointsService: raise StaffPinRequiredException() verified_pin = pin_service.verify_pin(db, program.id, staff_pin, store_id=store_id) + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Redeem points now = datetime.now(UTC) card.points_balance -= points_required @@ -419,6 +425,9 @@ class PointsService: "points_balance": card.points_balance, } + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Void the points (can reduce balance below what was earned) now = datetime.now(UTC) actual_voided = min(points_to_void, card.points_balance) @@ -503,6 +512,9 @@ class PointsService: if program.require_staff_pin and staff_pin and store_id: verified_pin = pin_service.verify_pin(db, program.id, staff_pin, store_id=store_id) + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Apply adjustment now = datetime.now(UTC) card.points_balance += points_delta diff --git a/app/modules/loyalty/services/stamp_service.py b/app/modules/loyalty/services/stamp_service.py index 3b908d5d..a484c4e1 100644 --- a/app/modules/loyalty/services/stamp_service.py +++ b/app/modules/loyalty/services/stamp_service.py @@ -125,6 +125,9 @@ class StampService: if stamps_today >= program.max_daily_stamps: raise DailyStampLimitException(program.max_daily_stamps, stamps_today) + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Add the stamp card.stamp_count += 1 card.total_stamps_earned += 1 @@ -249,6 +252,9 @@ class StampService: raise StaffPinRequiredException() verified_pin = pin_service.verify_pin(db, program.id, staff_pin, store_id=store_id) + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Redeem stamps now = datetime.now(UTC) stamps_redeemed = program.stamps_target @@ -383,6 +389,9 @@ class StampService: "stamp_count": card.stamp_count, } + # Re-fetch with row lock to prevent concurrent modification + card = card_service.get_card_for_update(db, card.id) + # Void the stamps (can reduce balance below what was earned) now = datetime.now(UTC) actual_voided = min(stamps_to_void, card.stamp_count) diff --git a/app/modules/loyalty/static/storefront/js/loyalty-dashboard.js b/app/modules/loyalty/static/storefront/js/loyalty-dashboard.js index 0cb0505e..2cbfd167 100644 --- a/app/modules/loyalty/static/storefront/js/loyalty-dashboard.js +++ b/app/modules/loyalty/static/storefront/js/loyalty-dashboard.js @@ -1,5 +1,6 @@ // app/modules/loyalty/static/storefront/js/loyalty-dashboard.js // Customer loyalty dashboard +const loyaltyDashboardLog = window.LogConfig.loggers.loyaltyDashboard || window.LogConfig.createLogger('loyaltyDashboard'); function customerLoyaltyDashboard() { return { @@ -20,7 +21,7 @@ function customerLoyaltyDashboard() { showBarcode: false, async init() { - console.log('Customer loyalty dashboard initializing...'); + loyaltyDashboardLog.info('Customer loyalty dashboard initializing...'); await this.loadData(); }, @@ -32,7 +33,7 @@ function customerLoyaltyDashboard() { this.loadTransactions() ]); } catch (error) { - console.error('Failed to load loyalty data:', error); + loyaltyDashboardLog.error('Failed to load loyalty data:', error); } finally { this.loading = false; } @@ -47,11 +48,11 @@ function customerLoyaltyDashboard() { this.rewards = response.program?.points_rewards || []; this.locations = response.locations || []; this.walletUrls = response.wallet_urls || { google_wallet_url: null, apple_wallet_url: null }; - console.log('Loyalty card loaded:', this.card?.card_number); + loyaltyDashboardLog.info('Loyalty card loaded:', this.card?.card_number); } } catch (error) { if (error.status === 404) { - console.log('No loyalty card found'); + loyaltyDashboardLog.info('No loyalty card found'); this.card = null; } else { throw error; @@ -66,7 +67,7 @@ function customerLoyaltyDashboard() { this.transactions = response.transactions; } } catch (error) { - console.warn('Failed to load transactions:', error.message); + loyaltyDashboardLog.warn('Failed to load transactions:', error.message); } }, diff --git a/app/modules/loyalty/static/storefront/js/loyalty-enroll.js b/app/modules/loyalty/static/storefront/js/loyalty-enroll.js index 9e80dff8..64d8d488 100644 --- a/app/modules/loyalty/static/storefront/js/loyalty-enroll.js +++ b/app/modules/loyalty/static/storefront/js/loyalty-enroll.js @@ -1,5 +1,6 @@ // app/modules/loyalty/static/storefront/js/loyalty-enroll.js // Self-service loyalty enrollment +const loyaltyEnrollLog = window.LogConfig.loggers.loyaltyEnroll || window.LogConfig.createLogger('loyaltyEnroll'); function customerLoyaltyEnroll() { return { @@ -28,7 +29,7 @@ function customerLoyaltyEnroll() { showTerms: false, async init() { - console.log('Customer loyalty enroll initializing...'); + loyaltyEnrollLog.info('Customer loyalty enroll initializing...'); await this.loadProgram(); }, @@ -38,14 +39,14 @@ function customerLoyaltyEnroll() { const response = await apiClient.get('/storefront/loyalty/program'); if (response) { this.program = response; - console.log('Program loaded:', this.program.display_name); + loyaltyEnrollLog.info('Program loaded:', this.program.display_name); } } catch (error) { if (error.status === 404) { - console.log('No loyalty program available'); + loyaltyEnrollLog.info('No loyalty program available'); this.program = null; } else { - console.error('Failed to load program:', error); + loyaltyEnrollLog.error('Failed to load program:', error); this.error = I18n.t('loyalty.enrollment.errors.load_failed'); } } finally { @@ -73,7 +74,7 @@ function customerLoyaltyEnroll() { if (response) { const cardNumber = response.card?.card_number || response.card_number; - console.log('Enrollment successful:', cardNumber); + loyaltyEnrollLog.info('Enrollment successful:', cardNumber); // Store wallet URLs for the success page (no auth needed) if (response.wallet_urls) { @@ -87,7 +88,7 @@ function customerLoyaltyEnroll() { window.location.href = successUrl; } } catch (error) { - console.error('Enrollment failed:', error); + loyaltyEnrollLog.error('Enrollment failed:', error); if (error.message?.includes('already')) { this.error = I18n.t('loyalty.enrollment.errors.email_exists'); } else { diff --git a/app/modules/loyalty/static/storefront/js/loyalty-history.js b/app/modules/loyalty/static/storefront/js/loyalty-history.js index 68c5ab56..8bbe440a 100644 --- a/app/modules/loyalty/static/storefront/js/loyalty-history.js +++ b/app/modules/loyalty/static/storefront/js/loyalty-history.js @@ -1,5 +1,6 @@ // app/modules/loyalty/static/storefront/js/loyalty-history.js // Customer loyalty transaction history +const loyaltyHistoryLog = window.LogConfig.loggers.loyaltyHistory || window.LogConfig.createLogger('loyaltyHistory'); function customerLoyaltyHistory() { return { @@ -21,7 +22,7 @@ function customerLoyaltyHistory() { loading: false, async init() { - console.log('Customer loyalty history initializing...'); + loyaltyHistoryLog.info('Customer loyalty history initializing...'); await this.loadData(); }, @@ -33,7 +34,7 @@ function customerLoyaltyHistory() { this.loadTransactions() ]); } catch (error) { - console.error('Failed to load history:', error); + loyaltyHistoryLog.error('Failed to load history:', error); } finally { this.loading = false; } @@ -46,7 +47,7 @@ function customerLoyaltyHistory() { this.card = response.card; } } catch (error) { - console.warn('Failed to load card:', error.message); + loyaltyHistoryLog.warn('Failed to load card:', error.message); } }, @@ -61,10 +62,10 @@ function customerLoyaltyHistory() { this.transactions = response.transactions || []; this.pagination.total = response.total || 0; this.pagination.pages = Math.ceil(this.pagination.total / this.pagination.per_page); - console.log(`Loaded ${this.transactions.length} transactions`); + loyaltyHistoryLog.info(`Loaded ${this.transactions.length} transactions`); } } catch (error) { - console.error('Failed to load transactions:', error); + loyaltyHistoryLog.error('Failed to load transactions:', error); } }, diff --git a/app/modules/messaging/locales/lu.json b/app/modules/messaging/locales/lu.json deleted file mode 100644 index 0967ef42..00000000 --- a/app/modules/messaging/locales/lu.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/app/modules/monitoring/locales/lu.json b/app/modules/monitoring/locales/lu.json deleted file mode 100644 index 0967ef42..00000000 --- a/app/modules/monitoring/locales/lu.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/app/modules/orders/locales/lu.json b/app/modules/orders/locales/lu.json deleted file mode 100644 index 0967ef42..00000000 --- a/app/modules/orders/locales/lu.json +++ /dev/null @@ -1 +0,0 @@ -{} diff --git a/docs/architecture/module-system.md b/docs/architecture/module-system.md index 1e8a59a8..dc03409c 100644 --- a/docs/architecture/module-system.md +++ b/docs/architecture/module-system.md @@ -163,7 +163,7 @@ app/modules/analytics/ │ ├── en.json │ ├── de.json │ ├── fr.json -│ └── lu.json +│ └── lb.json ├── tasks/ # Auto-discovered by Celery │ ├── __init__.py # REQUIRED for Celery discovery │ └── reports.py diff --git a/docs/development/creating-modules.md b/docs/development/creating-modules.md index f3b48541..c8b9eb25 100644 --- a/docs/development/creating-modules.md +++ b/docs/development/creating-modules.md @@ -164,7 +164,7 @@ app/modules/mymodule/ │ ├── en.json │ ├── de.json │ ├── fr.json -│ └── lu.json +│ └── lb.json │ ├── tasks/ # Celery tasks (auto-discovered) │ ├── __init__.py # REQUIRED for discovery @@ -539,7 +539,7 @@ python scripts/validate/validate_architecture.py - [ ] Create `exceptions.py` - [ ] Create routes with `router` variable - [ ] Create templates with namespace prefix -- [ ] Create locales (en, de, fr, lu) +- [ ] Create locales (en, de, fr, lb) - [ ] Create `config.py` if module needs environment settings (optional) - [ ] Create `migrations/versions/` with `__init__.py` files if module has database tables - [ ] Create `docs/index.md` with module overview (see [Module Documentation](module-documentation.md))