WCA Live Integration Pre Work #2: Add liveResult and liveAttempt models #10685
Conversation
FinnIckler
force-pushed
the
wca-live-prep-2
branch
from
8123f67 to
8de2648
Compare
|
I've added the factories and some basic tests. There will be a ton more when validating cutoffs and time limits will be introduced in a future pr |
| # params: { results, round_id, user_id, entered_by } | ||
| def perform(params) |
There was a problem hiding this comment.
I know that I neglected to mention this in the other PR, but you can actually do def perform(results, round_id, user_id, entered_by) and Rails will de-serialize the parameters for you
There was a problem hiding this comment.
(that's a feature of ActiveJob specifically, not a general Ruby thing)
| event = round.event | ||
| format = round.format | ||
|
|
||
| r = Result.build({ value1: results[0], value2: results[1] || 0, value3: results[2] || 0, value4: results[3] || 0, value5: results[4] || 0, event_id: event.id, round_type_id: round.round_type_id, format_id: format.id }) |
There was a problem hiding this comment.
Why are you using build instead of new here?
| def result_id | ||
| id | ||
| end |
|
|
||
| belongs_to :registration | ||
|
|
||
| belongs_to :entered_by, class_name: 'User', foreign_key: 'entered_by_id' |
There was a problem hiding this comment.
I think Rails is smart enough to infer the foreign_key, as long as it's "name of relation + _id"
| DEFAULT_SERIALIZE_OPTIONS = { | ||
| only: %w[ranking registration_id round live_attempts round best average single_record_tag average_record_tag advancing advancing_questionable entered_at entered_by_id], | ||
| methods: %w[event_id attempts result_id], | ||
| include: %w[event live_attempts round], |
There was a problem hiding this comment.
This is a heck of a lot of data you're serializing for each and every single scorecard. I won't stand in your way for your PoC experiments, but please think about how to slim this down. Especially for event, just sending along event_id and accessing the static list of WCA events in JS should be enough.
For the round, I'd also argue that a round ID should do, and you keep a lookup somewhere else in your React component.
There was a problem hiding this comment.
yeah I only included event here because I thought that would pre-load it from the database (we talked about include vs includes yesterday). event_id is definitely enough
There was a problem hiding this comment.
Need to figure out why I included round
| t.references :entered_by, type: :integer, null: false, foreign_key: { to_table: :users } | ||
| t.datetime :entered_at, null: false |
There was a problem hiding this comment.
Is it sufficient to keep this information per scorecard?
I'm thinking about FMC and MBLD where you enter results with long gaps in between, and these gaps also make it plausible that individual scores are entered by different people.
There was a problem hiding this comment.
Also related about the timestamp: I assume this is the time when the scorecard was first inserted, but what if we just change one score later? Where is that being recorded?
There was a problem hiding this comment.
I was thinking I would just overwrite entered_by and entered_at with the latest person. I think that's how jonatan does it
| t.timestamps | ||
| end | ||
|
|
||
| add_index :live_results, [:registration_id, :round_id], unique: true |
There was a problem hiding this comment.
I think this can just be t.index inside of create_table, but feel free to prove me wrong
| @@ -1084,6 +1115,7 @@ | |||
| t.text "round_results", size: :medium | |||
| t.integer "total_number_of_rounds", null: false | |||
| t.string "old_type", limit: 1 | |||
| t.boolean "is_open", default: false, null: false | |||
There was a problem hiding this comment.
I don't see a corresponding migration for this. Potential whoopsie from your other PR?
There was a problem hiding this comment.
yeah, that should probably go into the open round pr instead
Also confirmed working using my test page.
wca live's result table additionally has
But I am not sure which one of these we should also use and which one we should just compute on the fly.