Fix: Persist Elo rating change in debate history #266
Conversation
📝 WalkthroughWalkthroughThis PR implements Elo rating change tracking in debate history by adding an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/models/transcript.go (1)
41-55: MarshalJSON produces duplicate JSON keys foridanduserId.The embedded
Aliasstruct still contains theIDandUserIDfields (set toNilObjectID), which will serialize alongside the explicitIDandUserIDstring fields. This results in duplicate keys in the JSON output, causing undefined behavior in JSON parsers.Suggested fix using struct tags to omit the alias fields
func (s SavedDebateTranscript) MarshalJSON() ([]byte, error) { type Alias SavedDebateTranscript a := Alias(s) - a.ID = primitive.NilObjectID - a.UserID = primitive.NilObjectID return json.Marshal(&struct { - ID string `json:"id"` - UserID string `json:"userId"` + ID string `json:"id"` + UserID string `json:"userId"` + Alias + }{ + ID: s.ID.Hex(), + UserID: s.UserID.Hex(), Alias }{ ID: s.ID.Hex(), UserID: s.UserID.Hex(), Alias: a, }) }A cleaner approach is to use a nested type that shadows the problematic fields:
func (s SavedDebateTranscript) MarshalJSON() ([]byte, error) { type Alias SavedDebateTranscript return json.Marshal(&struct { ID string `json:"id"` UserID string `json:"userId"` *Alias }{ ID: s.ID.Hex(), UserID: s.UserID.Hex(), Alias: (*Alias)(&s), }) }Note: Using a pointer to
Aliasand placing explicit fields first allows them to shadow the embedded struct's fields during serialization.backend/services/transcriptservice.go (2)
180-206: Critical:forRecordis undefined andeloChangeargument is missing.Two compilation errors exist here:
Line 189:
forRecord.RatingChangereferences an undefined variable.UpdateRatings(which returnsdebateRecord) is called on line 217, after thisSaveDebateTranscriptcall.Lines 195-204: The second
SaveDebateTranscriptcall for the "against" user is missing the requiredeloChangeargument (function signature expects 9 arguments, only 8 provided).The PR description states the workflow was refactored to "calculate ratings before saving transcripts," but the current code still saves transcripts before calling
UpdateRatings.Suggested fix: Reorder to calculate ratings first, then save transcripts
- // Save transcript for "for" user - err = SaveDebateTranscript( - forUser.ID, - forUser.Email, - "user_vs_user", - topic, - againstUser.Email, - resultFor, - []models.Message{}, // You might want to reconstruct messages from transcripts - forSubmission.Transcripts, - forRecord.RatingChange - ) - if err != nil { - } - - // Save transcript for "against" user - err = SaveDebateTranscript( - againstUser.ID, - againstUser.Email, - "user_vs_user", - topic, - forUser.Email, - resultAgainst, - []models.Message{}, // You might want to reconstruct messages from transcripts - againstSubmission.Transcripts, - ) - if err != nil { - } - - // Update ratings based on the result + // Update ratings based on the result (must happen BEFORE saving transcripts) outcomeFor := 0.5 switch strings.ToLower(resultFor) { case "win": @@ -215,6 +195,30 @@ opponentRecord.Topic = topic opponentRecord.Result = resultAgainst + // Save transcript for "for" user with actual rating change + err = SaveDebateTranscript( + forUser.ID, + forUser.Email, + "user_vs_user", + topic, + againstUser.Email, + resultFor, + []models.Message{}, + forSubmission.Transcripts, + debateRecord.RatingChange, + ) + if err != nil { + } + + // Save transcript for "against" user with actual rating change + err = SaveDebateTranscript( + againstUser.ID, + againstUser.Email, + "user_vs_user", + topic, + forUser.Email, + resultAgainst, + []models.Message{}, + againstSubmission.Transcripts, + opponentRecord.RatingChange, + ) + if err != nil { + } + records := []interface{}{debateRecord, opponentRecord}
975-976: Stale TODO:GetDebateStatsstill uses hardcodedeloChange: 0.This function was not updated to use
transcript.EloChange, inconsistent with the changes inGetProfile. The TODO comment is now addressable.Suggested fix
- "eloChange": 0, // TODO: Add actual Elo change tracking + "eloChange": transcript.EloChange,
🧹 Nitpick comments (1)
backend/controllers/profile_controller.go (1)
257-258: Remove stale TODO comment.The TODO comment is now obsolete since this PR implements EloChange tracking. The comment should be removed to avoid confusion.
Suggested fix
- "eloChange": transcript.EloChange,// TODO: Add actual Elo change tracking + "eloChange": transcript.EloChange,
1 participant
Problem
Debate history always displayed an Elo change of 0 because the rating delta
was never persisted. The transcript was saved before rating updates were
calculated, causing the value to be lost.
Solution
This PR implements full Elo change tracking for debate transcripts.
Changes
EloChangefield toSavedDebateTranscriptResult
Closes #265
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.