fix(loyalty): resolve critical production readiness issues

- Add pessimistic locking (SELECT FOR UPDATE) on card write operations to prevent race conditions in stamp_service and points_service - Replace 16 console.log/error/warn calls with LogConfig.createLogger() in 3 storefront JS files (dashboard, history, enroll) - Delete all stale lu.json locale files across 8 modules (lb is the correct ISO 639-1 code for Luxembourgish) - Update architecture rules and docs to reference lb.json not lu.json - Add production-readiness.md report for loyalty module Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-03-13 23:18:18 +01:00
parent 5dd5e01dc6
commit 4a1f71a312
18 changed files with 194 additions and 118 deletions
--- 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/<code>/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"
--- a/app/modules/analytics/locales/lu.json
+++ b/app/modules/analytics/locales/lu.json
@@ -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"
-  }
-}
--- a/app/modules/customers/locales/lu.json
+++ b/app/modules/customers/locales/lu.json
@@ -1 +0,0 @@
-{}
--- a/app/modules/dev_tools/locales/lu.json
+++ b/app/modules/dev_tools/locales/lu.json
@@ -1 +0,0 @@
-{}
--- a/app/modules/inventory/locales/lu.json
+++ b/app/modules/inventory/locales/lu.json
@@ -1 +0,0 @@
-{}
--- a/app/modules/loyalty/docs/production-readiness.md
+++ 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
--- a/app/modules/loyalty/locales/lu.json
+++ b/app/modules/loyalty/locales/lu.json
@@ -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"
-    }
-  }
-}
--- 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 (
--- 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
--- 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)
--- 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);
            }
        },

--- 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 {
--- 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);
            }
        },

--- a/app/modules/messaging/locales/lu.json
+++ b/app/modules/messaging/locales/lu.json
@@ -1 +0,0 @@
-{}
--- a/app/modules/monitoring/locales/lu.json
+++ b/app/modules/monitoring/locales/lu.json
@@ -1 +0,0 @@
-{}
--- a/app/modules/orders/locales/lu.json
+++ b/app/modules/orders/locales/lu.json
@@ -1 +0,0 @@
-{}
--- 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
--- 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))