From 25236222de6d4dfbd3e81eaf945cfc428fdce17f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Gierwia=C5=82o?= Date: Sun, 30 Nov 2025 15:53:00 +0100 Subject: [PATCH] feat(matching): prevent auto suggestions when manual match exists + comprehensive test scenarios Matching Service: - Fetch manual matches at start of runMatching() (suggestionId: null) - Build manualBlockedPairs set with both directions (A:B and B:A) - Skip recorder candidates if manual match exists between dancer and recorder - Ensures no duplicate matches on /matches page - Manual match = user-controlled, algorithm respects user decisions Documentation (docs/TODO.md): - Add comprehensive matching system test scenarios (S1-S16) - Document 4 critical gaps (P0): ratings/stats, admin middleware, participant validation - Document 3 high priority items (P1): cleanup conflicts, rate limiting, notifications - Document 6 medium priority items (P2): audit endpoints, zombie cleanup, reminders - List 11 edge cases for team discussion (E1-E11) - Clear priority ranking and questions for team Critical Findings: - recordingsDone/recordingsReceived fields exist but NEVER updated (fairness broken!) - Admin endpoints lack security middleware - Inconsistent event participant validation across endpoints Test Coverage: - S1-S7: Implemented (basic flow, collisions, limits, manual vs auto) - S10: NOT IMPLEMENTED - ratings/stats system (CRITICAL!) - S11: Partially implemented - audit trail exists, API endpoints missing - S14: Partially implemented - recorder-only auth works, admin middleware missing - S15-S16: NOT IMPLEMENTED - security, notifications --- backend/src/services/matching.js | 20 ++ docs/TODO.md | 449 +++++++++++++++++++++++++++++++ 2 files changed, 469 insertions(+) diff --git a/backend/src/services/matching.js b/backend/src/services/matching.js index c6082c4..1f30bc9 100644 --- a/backend/src/services/matching.js +++ b/backend/src/services/matching.js @@ -273,6 +273,23 @@ async function runMatching(eventId) { // Build division-to-slot map from schedule config const divisionSlotMap = buildDivisionSlotMap(event?.scheduleConfig); + // 0a. Get manual matches - we won't create auto suggestions for these pairs + const manualMatches = await prisma.match.findMany({ + where: { + eventId, + suggestionId: null, // Manual matches don't have a suggestionId + status: { in: ['pending', 'accepted', 'completed'] }, + }, + select: { user1Id: true, user2Id: true }, + }); + + // Build set of blocked pairs (both directions) + const manualBlockedPairs = new Set(); + for (const m of manualMatches) { + manualBlockedPairs.add(`${m.user1Id}:${m.user2Id}`); + manualBlockedPairs.add(`${m.user2Id}:${m.user1Id}`); // both directions + } + // 1. Get all participants with their heats and user info const participants = await prisma.eventParticipant.findMany({ where: { eventId }, @@ -416,6 +433,9 @@ async function runMatching(eventId) { // Skip self if (recorder.userId === dancer.userId) continue; + // Skip if manual match exists between dancer and recorder + if (manualBlockedPairs.has(`${dancer.userId}:${recorder.userId}`)) continue; + // Check assignment limit const currentCount = recorderAssignmentCount.get(recorder.userId) || 0; if (currentCount >= MAX_RECORDINGS_PER_PERSON) continue; diff --git a/docs/TODO.md b/docs/TODO.md index 1029673..220d6ac 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -34,6 +34,7 @@ 5. **Phase 5:** Edge Cases (TC17-18) - Sanity checks - **Impact:** Ensures matching algorithm works correctly end-to-end, verifies critical bug fixes - **Status:** Test plan documented, implementation pending +- **Extended Scenarios:** See comprehensive test scenarios below ### Recently Completed (2025-11-29) - 3-Tier Account System (BASIC/SUPPORTER/COMFORT) with fairness algorithm @@ -48,6 +49,454 @@ --- +## Matching System - Comprehensive Test Scenarios + +**Last Updated:** 2025-11-30 +**Status:** Documented for team discussion + +### Implementation Status + +#### βœ… Implemented Scenarios +- **S1-S3:** Basic flow, collision detection, limits (covered by existing tests) +- **S7.1-7.2:** Manual match blocks auto suggestions (implemented 2025-11-30) +- **S12:** Multi-heat collision detection (existing logic) +- **S14.1:** Only recorder can accept/reject (implemented in MVP) + +#### πŸ”΄ Critical Gaps (P0 - Before Production) + +1. **S10: Ratings & Stats System** - **CRITICAL** + - Fields `recordingsDone`/`recordingsReceived` exist but NEVER updated + - Fairness algorithm depends on these stats - currently broken! + - Need: `statsApplied` flag on Match model + - Need: Auto-increment stats after both users rate (only for auto matches) + +2. **S14.2: Admin Middleware** - **SECURITY** + - Admin endpoints not protected: `/admin/events/:slug/run-now`, `/admin/matching-runs` + - Need: `requireAdmin` middleware + +3. **S14.3: Event Participant Validation** - **SECURITY** + - Inconsistent checks across endpoints + - Need: Audit all suggestion/match endpoints for participant validation + +#### ⚠️ High Priority (P1 - First Month) + +4. **E9/S13.2: Manual match created AFTER auto suggestion** + - Current: Manual blocks only NEW auto suggestions, old pending remain + - Need: Cleanup conflicting pending auto suggestions when manual match created + +5. **S15.1-15.2: Rate Limiting & Spam Protection** + - Max pending outgoing requests (20) + - Rate limit manual match requests (10/minute) + +6. **S16.1: Socket Notifications** + - Real-time notification when new suggestion created + +#### πŸ“‹ Medium Priority (P2 - Q1 2025) + +7. **S11.3-11.4: Matching Run Details API** + - Endpoint: `GET /matching-runs/:id/suggestions` + - Filters: `onlyAssigned`, `includeNotFound` + +8. **S15.3: Zombie Matches Cleanup** + - Auto-cancel pending matches older than 30 days + +9. **S16.3: Email Reminders** + - Reminder before event for accepted recording assignments + +### Test Scenarios by Category + +
+S1: BASIC FLOW βœ… Implemented + +#### S1.1: Happy path - basic assignment +- **Given:** Event with deadline, user A has heat H1, user B no conflict +- **When:** Matching runs after deadline +- **Then:** Creates suggestion B records A in H1 (pending) + +#### S1.2: Recorder accepts suggestion +- **Given:** Pending suggestion: B records A +- **When:** B clicks Accept +- **Then:** Statusβ†’accepted, creates Match (type: auto), both see on /matches + +#### S1.3: Recorder rejects suggestion +- **Given:** Pending suggestion: B records A +- **When:** B clicks Reject +- **Then:** Statusβ†’rejected, A sees read-only, next run may assign different recorder + +
+ +
+S2: TIME COLLISION DETECTION βœ… Implemented + +#### S2.1: Can't record while dancing +- **Given:** A has heat H1, B has heat H1 (same slot) +- **Then:** B NOT candidate to record A in H1 + +#### S2.2: Buffer before - preparation time +- **Given:** A has heat H5, B has heat H6 (next) +- **When:** HEAT_BUFFER_BEFORE = 1 +- **Then:** B NOT candidate (needs H5 for prep) + +#### S2.3: Buffer after - rest time +- **Given:** A has heat H5, B has heat H4 (previous) +- **When:** HEAT_BUFFER_AFTER = 1 +- **Then:** B NOT candidate (needs H5 for rest) + +#### S2.4: Schedule config - divisions in same slot +- **Given:** Schedule config: slot 1 = [Open, Newcomer]; A has Open H3, B has Newcomer H3 +- **Then:** B can't record A (same slot despite different divisions) + +
+ +
+S3: LIMITS & CONSTRAINTS βœ… Implemented + +#### S3.1: Max 3 recordings per person +- **Given:** B already assigned to record 3 others +- **Then:** B NOT candidate for dancer A + +#### S3.2: Opt-out from recording +- **Given:** B checked "I don't want to record others" +- **Then:** B completely excluded as potential recorder + +#### S3.3: No self-recording +- **Given:** A has heat H1 +- **Then:** A can't be recorder for themselves + +
+ +
+S4: FAIRNESS & TIER SYSTEM βœ… Implemented (but stats not updating!) + +#### S4.1: BASIC user - normal frequency +- **Given:** C (BASIC): done=5, received=0; A,B (BASIC): done=0, received=0 +- **Then:** C has priority (fairness debt = -5) + +#### S4.2: SUPPORTER user - reduced frequency (-10) +- **Given:** A (SUPPORTER) vs B (BASIC), both 0/0 +- **Then:** B has priority (A has -10 penalty) + +#### S4.3: COMFORT user - significantly reduced (-50) +- **Given:** A (COMFORT), B (SUPPORTER), C (BASIC), all 0/0 +- **Then:** Priority: C > B > A + +#### S4.4: Location preference +- **Given:** Dancer A in LA,US; B (LA,US), C (NYC,US), D (London,UK) +- **Then:** Priority: B > C > D + +
+ +
+S5: INCREMENTAL MATCHING βœ… Implemented + +#### S5.1: Accepted suggestions preserved +- **Given:** Run 1 created suggestion Bβ†’A (pending), B accepted +- **When:** Run 2 +- **Then:** A-B suggestion NOT deleted or overwritten + +#### S5.2: Pending suggestions regenerated +- **Given:** Run 1 created Bβ†’A (pending), B didn't accept +- **When:** Run 2 (new users joined) +- **Then:** Pending A-B deleted, new suggestion may assign Cβ†’A + +#### S5.3: Accepted suggestions block slots +- **Given:** B accepted recording A in heat H5 +- **When:** Run 2 for dancer D (also has H5) +- **Then:** B NOT candidate (busy in H5) + +
+ +
+S6: SCHEDULER βœ… Implemented + +#### S6.1: Waits until deadline +- **Given:** Event has registrationDeadline in 2 hours +- **Then:** Scheduler does NOT run matching (waits) + +#### S6.2: Runs after deadline +- **Given:** registrationDeadline passed 1 minute ago +- **Then:** Runs matching (run 1/5) + +#### S6.3: 5 runs at 5-minute intervals +- **Then:** Run 1 (immediately), Run 2 (+5min), Run 3 (+10min), Run 4 (+15min), Run 5 (+20min), STOP + +#### S6.4: Audit trail +- **Given:** Matching ran +- **Then:** Admin sees table of runs with timestamps and stats (total/pending/accepted/rejected/not_found) + +
+ +
+S7: MANUAL vs AUTO MATCHES βœ… S7.1-7.4 Implemented + +#### S7.1: Manual match blocks auto suggestion βœ… +- **Given:** A sent manual match request to B (pending) +- **When:** Matching algorithm runs +- **Then:** Does NOT create auto suggestion A↔B or B↔A + +#### S7.2: All manual statuses block βœ… +- **Given:** Manual match A↔B in status: pending/accepted/completed +- **Then:** All cases block auto suggestion + +#### S7.3: /matches shows both types with badges βœ… +- **Given:** A has manual match with B (pending), auto suggestion with C (pending - C records A) +- **Then:** A sees 2 items: B (badge "Manual"), C (badge "Auto") + +#### S7.4: Auto pending - redirect to Records βœ… +- **Given:** A has auto suggestion with C (pending, C records A) +- **Then:** Button "Go to Records" β†’ /events/X/chat?tab=records (no Accept/Reject for dancer) + +
+ +
+S10: RATINGS & STATS πŸ”΄ NOT IMPLEMENTED - CRITICAL! + +#### S10.1: Auto match completed β†’ stats updated exactly once +- **Given:** Auto Match A↔B (suggestionId != null, statsApplied=false), both rated +- **When:** Ratings endpoint called +- **Then:** + - `recordingsDone++` for recorder + - `recordingsReceived++` for dancer + - `match.status = 'completed'` + - `match.statsApplied = true` + +#### S10.2: Only one rated β†’ no stats +- **Given:** Auto Match A↔B, only A rated +- **Then:** `statsApplied` stays false, stats don't change + +#### S10.3: Manual match completion β†’ no stats update +- **Given:** Match A↔B (suggestionId=null), both rated +- **Then:** Stats don't change (manual matches don't affect fairness) + +#### S10.4: Rating edit β†’ no double counting +- **Given:** Auto Match A↔B has `statsApplied=true`, user edits rating +- **Then:** Stats don't change (already applied) + +**Implementation needed:** +```javascript +// Match model +statsApplied: Boolean @default(false) + +// After both ratings (pseudocode): +if (bothRated && !match.statsApplied && match.suggestionId) { + // Increment stats + // Set statsApplied = true +} +``` + +
+ +
+S11: AUDIT & origin_run_id βœ… Partially Implemented + +#### S11.1: New suggestions get correct origin_run_id βœ… +- **Given:** Run #5 runs matching +- **Then:** Each new recording_suggestion has origin_run_id = 5 + +#### S11.2: Existing origin_run_id preserved βœ… +- **Given:** Suggestion S1 from run #3 (accepted/completed) +- **When:** Run #4 +- **Then:** S1 keeps origin_run_id=3 (not deleted) + +#### S11.3: Get suggestions by run πŸ”΄ NOT IMPLEMENTED +- **Need:** `GET /matching-runs/:id/suggestions` +- **Then:** Returns only suggestions with origin_run_id=:id + +#### S11.4: Filters work correctly πŸ”΄ NOT IMPLEMENTED +- **Need:** Query params `onlyAssigned`, `includeNotFound` +- **Then:** Filters recorder presence and NOT_FOUND status + +
+ +
+S12: MULTIPLE HEATS & COMPLEX COLLISIONS βœ… Implemented + +#### S12.1: Same dancer in multiple heats +- **Given:** Dancer A in H10 and H12 (Novice J&J), B no conflict +- **Then:** B can record A in both heats (if under limit 3) + +#### S12.2: Recorder records multiple dancers in different slots +- **Given:** Recorder B has zero own heats, A has H5 (slot 2), C has H8 (slot 3) +- **Then:** B can be assigned to both A and C (different slots) + +#### S12.3: Same recorder can't record two people in same slot +- **Given:** B accepted recording A in Novice H10 (slot 2), D has Intermediate H10 (also slot 2) +- **When:** Next matching run for D +- **Then:** B excluded (busySlot recording A) + +
+ +
+S13: MANUAL vs AUTO - FURTHER INTERACTIONS ⚠️ Edge Case + +#### S13.1: Manual before matching βœ… DONE +- See S7.1 + +#### S13.2: Manual created AFTER auto suggestion ⚠️ EDGE CASE +- **Given:** Run #1 created auto suggestion A↔B (pending), then user A creates manual match with B +- **When:** Run #2 +- **Then:** + - **Current behavior:** Manual blocks NEW auto suggestions, old pending remains + - **Recommended:** Cleanup conflicting pending auto suggestions when manual created + +**Recommended implementation:** +```javascript +// routes/matches.js - POST / +// After creating manual match: +await prisma.recordingSuggestion.deleteMany({ + where: { + eventId, + OR: [ + { heatId: { in: user1HeatIds }, recorderId: user2Id }, + { heatId: { in: user2HeatIds }, recorderId: user1Id }, + ], + status: 'pending', // only pending, don't touch accepted + } +}); +``` + +
+ +
+S14: PERMISSIONS & VISIBILITY ⚠️ Partially Implemented + +#### S14.1: Only recorder can update status βœ… +- **Given:** Suggestion B records A (pending), A tries PUT /suggestions/:id/status +- **Then:** 403 Forbidden + +#### S14.2: Admin-only endpoints πŸ”΄ NOT SECURE +- **Given:** Regular user tries POST /admin/events/:slug/run-now or GET /admin/matching-runs +- **Then:** Should get 403, but **middleware missing!** + +**Need to implement:** +```javascript +// middleware/auth.js +const requireAdmin = (req, res, next) => { + if (req.user.role !== 'admin') { + return res.status(403).json({ error: 'Admin access required' }); + } + next(); +}; +``` + +#### S14.3: Event participant visibility ⚠️ INCONSISTENT +- **Need to audit:** Do all suggestion/match endpoints verify user is event participant? +- **Locations to check:** + - GET /api/matches?eventSlug=X + - GET /api/events/:slug/matching/suggestions + - Frontend: /events/:slug/chat?tab=records + +
+ +
+S15: SECURITY & ABUSE PREVENTION πŸ”΄ NOT IMPLEMENTED + +#### S15.1: Rate limiting on manual match requests +- **Given:** User A sends 100 manual requests in 1 minute +- **Then:** 429 Too Many Requests after 10 requests/minute + +#### S15.2: Max pending requests limit +- **Given:** User A has 20 pending outgoing requests +- **Then:** Can't send more (limit 20) + +#### S15.3: Zombie matches cleanup +- **Given:** Pending match older than 30 days +- **Then:** Auto-cancelled by cron job + +
+ +
+S16: NOTIFICATIONS & REAL-TIME πŸ”΄ NOT IMPLEMENTED + +#### S16.1: Socket event for new suggestion +- **Given:** Matching created suggestion B records A, B is online +- **Then:** B receives socket event β†’ toast notification + +#### S16.2: Push notification for offline users +- **Given:** B offline when suggestion created +- **Then:** Email/push: "You have a new recording assignment" + +#### S16.3: Reminder before event +- **Given:** B accepted recording A, event in 2 days +- **Then:** Email: "Don't forget to record @userA on Saturday" + +
+ +### Edge Cases for Discussion + +#### E1: User has 10 heats, no one can record +- **Question:** Show all as NOT_FOUND? Prioritize which heats are most important? + +#### E2: Recorder wants to un-accept +- **Current:** No "un-accept" option +- **Question:** Add cancel feature for accepted suggestions? + +#### E3: Manual match rejected, then matching runs +- **Current:** Rejected manual still blocks auto suggestions +- **Question:** Should rejected manual stop blocking? + +#### E4: User joins AFTER all 5 scheduler runs +- **Current:** Won't get auto suggestions (scheduler stopped) +- **Question:** Allow admin to manually trigger additional run? + +#### E5: Timezone for deadline +- **Current:** Server timezone +- **Question:** Use event/organizer timezone? + +#### E6: Load balancing - one recorder has best scores +- **Current:** Can get max 3 assignments +- **Question:** Distribute more evenly even if scores worse? + +#### E7: Dancer rejects auto suggestion +- **Current:** Dancer CAN'T reject (only recorder in MVP) +- **Question:** Allow dancers to reject in future? + +#### E8: Event without schedule config +- **Current:** Fallback to division-based slots +- **Question:** Show admin warning that matching may be less efficient? + +#### E9: Manual match created AFTER auto suggestion exists +- See S13.2 above + +#### E10: User joins BETWEEN scheduler runs +- **Current:** βœ… Works - each run regenerates pending suggestions +- **Status:** No issue + +#### E11: Recorder accepted but didn't actually record +- **Question:** Timeout (7 days after event β†’ expired)? "Report issue" button? Admin override? + +### Priority Ranking + +#### P0 - CRITICAL (Before Production) +1. βœ… Manual blocks auto suggestions (DONE 2025-11-30) +2. πŸ”΄ **Implement ratings and stats system** (S10 - fairness broken without this!) +3. πŸ”΄ **Admin middleware** (S14.2 - security) +4. πŸ”΄ **Event participant validation** (S14.3 - security) + +#### P1 - HIGH (First Month of Production) +5. ⚠️ Manual match cleanup on conflict (S13.2 / E9) +6. ⚠️ Rate limiting and spam protection (S15.1, S15.2) +7. ⚠️ Socket notifications for new suggestions (S16.1) + +#### P2 - MEDIUM (Q1 2025) +8. πŸ“‹ Endpoint /matching-runs/:id/suggestions with filters (S11.3, S11.4) +9. πŸ“‹ Zombie matches cleanup (S15.3) +10. πŸ“‹ Email reminders (S16.3) + +#### P3 - LOW (Nice to Have) +11. πŸ“‹ Report issue for dancers (E11) +12. πŸ“‹ Admin manual override for suggestions +13. πŸ“‹ Analytics dashboard (success rate, acceptance rate) + +### Questions for Team + +1. **Is ratings system in roadmap?** If yes - THIS IS THE MOST IMPORTANT (fairness is broken without it) +2. **Do we have existing admin middleware?** If not - must add ASAP for security +3. **Which edge case (E1-E11) is most likely in production?** +4. **Should we implement S13.2 cleanup before launch?** (manual match after auto suggestion) + +--- + ## Security Audit Findings ### Critical Issues (Must Fix Before Production)