Save synced Live Results in LiveResults model#13261
Save synced Live Results in LiveResults model#13261FinnIckler wants to merge 13 commits intothewca:mainfrom
Conversation
|
Fixing the second test threw some questions at me. What it does is create "wrong" live results, meaning the rankings are just not correct. That's why I had to Really not sure what the best way to handle that is |
|
I do have a solution but it won't be super pretty |
FinnIckler
force-pushed
the
sync-to-live-results
branch
from
2d993cb to
4b5def2
Compare
|
This approach also now handles cases with empty attempts |
|
I wonder, should we just create these results as locked and then we don't have to bother with this complicated insertion |
| { event_id: event_id, round_number: round_number } | ||
| end | ||
|
|
||
| def load_live_results(results_wcif) |
There was a problem hiding this comment.
Per the fact that this uses insert_all! internally, you should also call this method load_live_results!
There was a problem hiding this comment.
As far as I can tell, the ! has not made it into the method name yet
There was a problem hiding this comment.
the error is eaten by the transaction block though?
There was a problem hiding this comment.
If it is, then that's probably not good... At least not for this specific scenario.
| "advancementCondition" => advancement_condition&.to_wcif, | ||
| "scrambleSetCount" => self.scramble_set_count, | ||
| "results" => round_results.map(&:to_wcif), | ||
| "results" => live_results.map(&:to_wcif), |
There was a problem hiding this comment.
Since you're now accessing an actual table relation (instead of just a field that was arbitrarily serialized into one column) you should probably look out for includes calls around the code, at least in the API controller that directly serves WCIF, and add live_results at the appropriate spot in there
| { event_id: event_id, round_number: round_number } | ||
| end | ||
|
|
||
| def load_live_results(results_wcif) |
There was a problem hiding this comment.
As far as I can tell, the ! has not made it into the method name yet
| def load_live_results(results_wcif) | ||
| self.transaction do | ||
| live_results.destroy_all | ||
| registrations_by_wcif_id = competition.registrations.index_by(&:registrant_id) |
There was a problem hiding this comment.
Are the registrations prt of the includes in the controller method of the PATCH endpoint?
| LiveAttempt.build_with_history_entry(attempt["result"], attempt_number, 1) | ||
| .attributes |
There was a problem hiding this comment.
I am 99% certain that calling attributes here entirely discards the _with_history_entry part. You're building a proper AR model class with a history entry correctly attached as a child, but calling attributes turns the parent LiveAttempt record into a pure hash which (obviously) does not understand anything about the concept of "associations".
app/models/round.rb
Outdated
| created_at: now, | ||
| updated_at: now, |
There was a problem hiding this comment.
See my other comment about time not needing to be passed in
| } | ||
| end | ||
|
|
||
| LiveResult.insert_all!(results_to_load) |
There was a problem hiding this comment.
Should this really be an insert? Feels more like an upsert...
There was a problem hiding this comment.
we just destroyed all live results, so this should be an insert
There was a problem hiding this comment.
Okay then (a) Is there a reason why we destroy + insert instead of upserting? (b) If the answer to A is Yes, then move the destroy_all directly towards the insert_all along with a comment explaining your thinking around question A
2 participants
opening in draft so I can check it out from my laptop