Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WCA Live Result Admin #1: Submit and Updating Results #10776

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions app/controllers/live_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
# frozen_string_literal: true

class LiveController < ApplicationController
def admin
@competition_id = params[:competition_id]
@competition = Competition.find(@competition_id)
@round = Round.find(params[:round_id])
@event_id = @round.event.id
@competitors = @round.registrations_with_wcif_id
end

def add_result
results = params.require(:attempts)
round_id = params.require(:round_id)
registration_id = params.require(:registration_id)

if LiveResult.exists?(round_id: round_id, registration_id: registration_id)
return render json: { status: "result already exist" }, status: :unprocessable_entity
end

AddLiveResultJob.perform_now({ results: results,
round_id: round_id,
registration_id: registration_id,
entered_by: current_user })
Comment on lines +21 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding my other comment at #10685 (comment): You would also need to update the call here and get rid of the hash { braces.


render json: { status: "ok" }
end

def update_result
results = params.require(:attempts)
round = Round.find(params.require(:round_id))
registration_id = params.require(:registration_id)

result = LiveResult.includes(:live_attempts).find_by(round: round, registration_id: registration_id)

unless result.present?
return render json: { status: "result does not exist" }, status: :unprocessable_entity
end
Comment on lines +36 to +38
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be return render json: ... unless result.present?. Surprised that RuboCop isn't complaining.


previous_attempts = result.live_attempts

new_attempts = results.map.with_index(1) do |r, i|
same_result = previous_attempts.find_by(result: r, attempt_number: i)
if same_result.present?
same_result
else
different_result = previous_attempts.find_by(attempt_number: i)
new_result = LiveAttempt.build(result: r, attempt_number: i)
different_result&.update(replaced_by_id: new_result.id)
new_result
end
end
Comment on lines +42 to +52
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: I have a hunch there might be an even more efficient way to handle this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will play around later when I have more headspace.


# TODO: What is the best way to do this?
r = Result.build({ value1: results[0], value2: results[1], value3: results[2], value4: results[3] || 0, value5: results[4] || 0, event_id: round.event.id, round_type_id: round.round_type_id, format_id: round.format_id })

result.update(average: r.compute_correct_average, best: r.compute_correct_best, live_attempts: new_attempts, entered_at: Time.now.utc, entered_by: current_user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we send a timestamp from the frontend when the query was fired?
If we're running this in a queue later, the "now" here in the backend could be significantly different from the "now" happening for the people in front of their screens.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating is currently not done in a queue, maybe it should? But we aren't doing that for registrations for example (but there updating doesn't create anything new)


render json: { status: "ok" }
end

def round_results
round_id = params.require(:round_id)

# TODO: Figure out why this fires a query for every live_attempt
# LiveAttempt Load (0.6ms) SELECT `live_attempts`.* FROM `live_attempts` WHERE `live_attempts`.`live_result_id` = 39 AND `live_attempts`.`replaced_by_id` IS NULL ORDER BY `live_attempts`.`attempt_number` ASC
render json: Round.includes(live_results: [:live_attempts, :round, :event]).find(round_id).live_results
end

def open_round
round_id = params.require(:round_id)
competition_id = params.require(:competition_id)
round = Round.find(round_id)

if round.is_open?
flash[:danger] = "Round is already open"
return redirect_to live_schedule_admin_path(competition_id: competition_id)
end

unless round.round_can_be_opened?
flash[:danger] = "You can't open this round yet"
return redirect_to live_schedule_admin_path(competition_id: competition_id)
end

round.update(is_open: true)
flash[:success] = "Successfully opened round"
redirect_to live_schedule_admin_path(competition_id: competition_id)
end
Comment on lines +70 to +88
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this part of a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, that was accidental


def double_check
@round = Round.find(params.require(:round_id))
@competition = Competition.find(params.require(:competition_id))

@competitors = @round.registrations_with_wcif_id
end

def schedule_admin
@competition_id = params.require(:competition_id)
@competition = Competition.find(@competition_id)

@rounds = Round.joins(:competition_event).where(competition_event: { competition_id: @competition_id })
end
end
36 changes: 36 additions & 0 deletions app/models/round.rb
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,42 @@ def name
Round.name_from_attributes(event, round_type)
end

def registrations
if number == 1
Registration.joins(:registration_competition_events)
.where(
competition_id: competition_event.competition_id,
competing_status: 'accepted',
registration_competition_events: { competition_event_id: competition_event_id }
).includes([:user])
else
previous_round = Round.joins(:competition_event).find_by(competition_event: { competition_id: competition_event.competition_id, event_id: event.id }, number: number - 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
previous_round = Round.joins(:competition_event).find_by(competition_event: { competition_id: competition_event.competition_id, event_id: event.id }, number: number - 1)
previous_round = Round.joins(:competition_event).find_by(competition_event: { competition_id: competition_event.competition_id, event_id: competition_event.event_id }, number: number - 1)

previous_round.live_results.where(advancing: true).includes(:registration).map(&:registration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, that's a lot of database load. Can we somehow pluck registration IDs with a single SELECT and then load registrations based on that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I was thinking something like that too

end
end

def registrations_with_wcif_id
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need both registrations and also registrations_with_wcif_id?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we should probably take this horrendous query as an incentive to think about a better way for coming up with registration IDs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registrations -> loading data for front office, registrations_with_wcif_id -> loading data for back office

if number == 1
Registration.where(competition_id: competition_event.competition_id)
.includes([:user])
.wcif_ordered
.to_enum
.with_index(1)
.select { |r, registrant_id| r.competing_status == 'accepted' && r.event_ids.include?(event.id) }
.map { |r, registrant_id| r.as_json({ include: [:user => { only: [:name], methods: [], include: []}]}).merge("registration_id" => registrant_id) }
else
previous_round = Round.joins(:competition_event).find_by(competition_event: { competition_id: competition_event.competition_id, event_id: event.id }, number: number - 1)
advancing = previous_round.live_results.where(advancing: true).pluck(:registration_id)
Registration.where(competition_id: competition_event.competition_id)
.includes([:user])
.wcif_ordered
.to_enum
.with_index(1)
.select { |r, registrant_id| advancing.include?(r.id) }
.map { |r, registrant_id| r.as_json({ include: [:user => { only: [:name], methods: [], include: []}]}).merge("registration_id" => registrant_id) }
end
end

def time_limit_to_s
time_limit.to_s(self)
end
Expand Down
5 changes: 5 additions & 0 deletions app/views/live/admin.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<% provide(:title, @round.name) %>

<%= render layout: 'competitions/nav' do %>
<%= react_component('Live/Admin/Results', { competitionId: @competition_id, competitors: @competitors, roundId: @round.id, eventId: @event_id }) %>
<% end %>
5 changes: 5 additions & 0 deletions app/views/live/double_check.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<% provide(:title, "#{@round.name} Double Check") %>

<%= render layout: 'competitions/nav' do %>
<%= react_component('Live/Admin/DoubleCheck', { competitors: @competitors.as_json({ methods: [:name, :user]}), round: @round.as_json( { include: [:event], only: [:id], methods: [:name]}), competitionId: @competition.id, }) %>
<% end %>
151 changes: 151 additions & 0 deletions app/webpacker/components/Live/Admin/DoubleCheck/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
import React, {
useCallback, useEffect, useMemo, useState,
} from 'react';
import {
Button, Card, Form,
Grid, Header,
Segment,
} from 'semantic-ui-react';
import { useMutation, useQuery } from '@tanstack/react-query';
import { events } from '../../../../lib/wca-data.js.erb';
import WCAQueryClientProvider from '../../../../lib/providers/WCAQueryClientProvider';
import AttemptResultField from '../../../EditResult/WCALive/AttemptResultField/AttemptResultField';
import updateRoundResults from '../../api/updateRoundResults';
import getRoundResults from '../../api/getRoundResults';
import Loading from '../../../Requests/Loading';

export default function Wrapper({
competitionId, competitors, round,
}) {
return (
<WCAQueryClientProvider>
<Competitors
eventId={round.event.id}
competitionId={competitionId}
competitors={competitors}
round={round}
/>
</WCAQueryClientProvider>
);
}

function zeroedArrayOfSize(size) {
return Array(size).fill(0);
}

function Competitors({
competitionId, competitors, round, eventId,
}) {
const event = events.byId[eventId];
const solveCount = event.recommendedFormat().expectedSolveCount;
const [currentIndex, setCurrentIndex] = useState(0);
const [registrationId, setRegistrationId] = useState(competitors[0].id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if competitors is empty?

const [attempts, setAttempts] = useState(zeroedArrayOfSize(solveCount));

const { data: results, isLoading } = useQuery({
queryKey: [round.id, 'results'],
queryFn: () => getRoundResults(round.id, competitionId),
});

const {
mutate: mutateUpdate, isPending: isPendingUpdate,
} = useMutation({
mutationFn: updateRoundResults,
});

const handleSubmit = async () => {
mutateUpdate({
roundId: round.id, registrationId, competitionId, attempts,
});
};

const handleAttemptChange = (index, value) => {
const newAttempts = [...attempts];
newAttempts[index] = value;
setAttempts(newAttempts);
};

const handleRegistrationIdChange = useCallback((_, { value }) => {
setRegistrationId(value);
const alreadyEnteredResults = results.find((r) => r.registration_id === value);
setAttempts(alreadyEnteredResults.attempts.map((a) => a.result));
}, [results]);

const currentCompetitor = useMemo(() => {
if (results) {
return competitors.find((r) => r.id === results[currentIndex].registration_id);
}
return {};
}, [competitors, currentIndex, results]);

useEffect(() => {
if (results) {
setAttempts(results[currentIndex].attempts.map((a) => a.result));
}
}, [currentIndex, results]);
Comment on lines +81 to +85
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like you can achieve the same thing without useEffect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this essentially a useMemo depending on results?


if (isLoading) {
return <Loading />;
}

return (
<Grid>
<Grid.Column width={1} verticalAlign="middle">
{ currentIndex !== 0 && <Button onClick={() => setCurrentIndex((oldIndex) => oldIndex - 1)}>{'<'}</Button>}
</Grid.Column>
<Grid.Column width={7}>
<Segment loading={isPendingUpdate}>
<Form>
<Form.Select
label="Competitor"
placeholder="Competitor"
value={currentCompetitor.id}
search={(inputs, value) => inputs.filter((d) => d.text.toLowerCase().includes(value.toLowerCase()) || parseInt(value, 10) === d.registrationId)}

Check failure on line 103 in app/webpacker/components/Live/Admin/DoubleCheck/index.jsx

View workflow job for this annotation

GitHub Actions / test

This line has a length of 158. Maximum allowed is 100.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that SemUI doesn't handle lowercasing natively. I am 97% sure I've added myself as a dummy organizer to comps before by using gregor billing. Does the deburr option change anything?

onChange={handleRegistrationIdChange}
options={competitors.map((p) => ({
key: p.id,
value: p.id,
registrationId: p.registration_id,
text: `${p.user.name} (${p.registration_id})`,
}))}
Comment on lines +105 to +110
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should probably be memoed somewhere.

/>
{Array.from(zeroedArrayOfSize(solveCount).keys()).map((index) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're building an array based on keys of a zeroed array? Since when do arrays have keys anyways? And what are you actually trying to do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like one of those typical features that lodash has a helper fpr

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this creates an array with the values [0,1,2,3,4,5]

<AttemptResultField
eventId={eventId}
key={index}
label={`Attempt ${index + 1}`}
placeholder="Time in milliseconds or DNF"
value={attempts[index] ?? 0}
onChange={(value) => handleAttemptChange(index, value)}
/>
))}
<Form.Button primary onClick={handleSubmit}>Submit Results</Form.Button>
</Form>
</Segment>
</Grid.Column>
<Grid.Column width={1} verticalAlign="middle">
{currentIndex !== results.length - 1 && <Button onClick={() => setCurrentIndex((oldIndex) => oldIndex + 1)}>{'>'}</Button>}
</Grid.Column>
<Grid.Column textAlign="center" width={7} verticalAlign="middle">
<Card fluid raised>
<Card.Header>
{currentIndex + 1}
{' '}
of
{' '}
{results.length}
</Card.Header>
<Card.Content>
<Header>{round.name}</Header>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just completed a Card.Header above and the first thing you're doing in the body is opening a Header. Why not just move round.name into the card header?

Double-check
</Card.Content>
<Card.Description>
Here you can iterate over results ordered by entry time (newest first). When doing double-check you can place a

Check failure on line 143 in app/webpacker/components/Live/Admin/DoubleCheck/index.jsx

View workflow job for this annotation

GitHub Actions / test

This line has a length of 123. Maximum allowed is 100.
scorecard next to the form to quickly compare attempt results. For optimal experience make sure to always put

Check failure on line 144 in app/webpacker/components/Live/Admin/DoubleCheck/index.jsx

View workflow job for this annotation

GitHub Actions / test

This line has a length of 121. Maximum allowed is 100.
entered/updated scorecard at the top of the pile.
</Card.Description>
</Card>
</Grid.Column>
</Grid>
);
}
Loading
Loading