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

Enforce checking results before submission #3127

Merged
merged 36 commits into from
Jan 26, 2019

Conversation

viroulep
Copy link
Member

@viroulep viroulep commented Jul 25, 2018

Fixes #2984, fixes #977, fixes #3222, fixes #69, fixes #80, fixes #3822.

TODOs:

This PR enables Delegates/WRT to upload a valid results JSON directly to InboxResult, InboxPerson, and Scramble.
When uploaded, the CompetitionResultsValidator inspect the data present in these tables, and check them according to the declared events, rounds, and formats for the given competition.
It reports warnings and errors found to the user, and enables the Delegate to submit the results to the WRT if no error is found.
It also gives the WRT a preview of the results when the rounds match the expected rounds for the competition.

This is actually a first step towards #297, as I introduced a per-competition navigation entry listing the various actions possible (right now only "upload and check results").

It is currently live on staging, so definitely feel free to play around it by deleting some existing competition results (and clearing both results_posted_at and results_submitted_at for the competition!), then uploading the corresponding results JSON (remember to set the DOB in the JSON to 1954-12-4, as all the data there are public!).

I've already done this for French nats this year, it's error-free but has some warnings you can check there.

Playing around in the manage events page should make it easy to raise error on this page, for example by changing cutoff and time limits.

See below for a detailed list of the checks done on data present in inboxes.

What is checked and where?

Unless stated otherwise, any failed check results in an error.
I think all these checks are more than what existed in the "check results" script, and it's kind of a "all-in-one" script for the competition.

Quite a number of checks are done before the results even land in the inboxes:

JSON format

This is done before anything lands in the inboxes, through a JSON schema here.

Importing the data to inboxes is wrapped in a transaction where:

  • Existing data for the competition are deleted
  • New data are imported

If any validation fails during this, nothing is imported and existing data are kept.
This allows the Delegates/WRT to upload multiple versions of the results (eg: to fix them) without having duplicate data for the competition.

Individual InboxPerson checks

Checks:

  • presence of name, dob, countryId
  • dob in the past
  • uniqueness of (personId, competitionId), through a mysql unique index

Individual InboxResult checks

Result and InboxResult share the same checks through the Resultable concern:

  • presence of competition, round_type, event, format
  • the number of solves is correct given the format
  • the average and best are correct
  • for each solve:
    • wca value is valid
    • multiblind time limit is respected
    • times over 10 minutes are rounded

Individual Scramble checks

Checks:

  • presence of event, round_type, scramble
  • groupId has a correct format
  • scrambleNum is a positive integer
  • isExtra is either true or false

After all these checks, we're guaranteed that the data present in the inboxes make sense, and we can start checking them according to what has been declared for the competition:

InboxPersons

Checks:

  • every person has results
  • no result miss its person
  • for persons without WCA ID
    • check if a name matches in the DB (warning)
    • check if DOB is january 1st (warning)
    • check if DOB is 3 years old or less (warning)
    • check for leading/trailing whitespaces or double whitespaces in name
  • for persons with WCA ID
    • check WCA ID exists in DB
    • check birthdate matches
    • check country matches (not activated currently, should it be?)
    • check gender matches (not activated currently, should it be?)

InboxResults

  • check events match the ones declared for the competition
  • check rounds match the ones declared for the competition
  • for each result:
    • check position matches the expected one
    • if cutoff
      • if no value meets the cutoff, check that it doesn't have more value
      • if a value meets the cutoff, check that it have all the values
    • for all events but multi and FM
      • check that all values are below the (cumulative) time limit
      • if it has a cumulative time limit, check for complete solve after DNF (warning)
      • check for complete solve after DNS (warning)
    • check for possible similar results

Scrambles

Checks:

  • all rounds have scrambles
  • no scrambles are for unexpected rounds
  • number of scrambles for the round match the expected number of scrambles according to the format

Rounds

  • check advancement conditions

viroulep added a commit to viroulep/worldcubeassociation.org that referenced this pull request Jul 26, 2018
It turns out I need it for thewca#3127
@viroulep viroulep force-pushed the check-uploaded-results branch 3 times, most recently from a5ea8e1 to 288436b Compare September 23, 2018 15:07
@viroulep
Copy link
Member Author

viroulep commented Oct 1, 2018

I believe I've taken care of all the WRT feedback.
I've deployed this to staging and will ping again the WRT for a last round of feedback.
I don't plan to add any more feature to this, as the PR is already quite big.

@viroulep viroulep force-pushed the check-uploaded-results branch 2 times, most recently from 5d58400 to c4ad699 Compare October 13, 2018 14:50
@viroulep viroulep force-pushed the check-uploaded-results branch 2 times, most recently from 4f59bfc to 2928615 Compare October 14, 2018 18:02
@viroulep
Copy link
Member Author

Ping @thewca/software-team, it would be awesome if I could get (part of) a review on this!
(I know the PR is quite big, sorry :()

@viroulep viroulep force-pushed the check-uploaded-results branch 4 times, most recently from ad7e069 to 43a764f Compare December 16, 2018 15:18
@viroulep
Copy link
Member Author

I have just updated the PR with more WRT feedback, and rebased it against the current master.
Most of them are just small fixes (see the last two commits), the only feature added was to include the results warnings in the mail to the WRT.
See this screenshot for how it looks like:

preview_mail_wrt

@viroulep
Copy link
Member Author

Hey @thewca/software-team, anyone willing to review this?
I have yet again to rebase this, I'll only do it when someone commits to take a look at it, as I've already rebased it multiple times :p

Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

First part of the review, gonna continue later.

WcaOnRails/app/models/concerns/resultable.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/concerns/resultable.rb Show resolved Hide resolved
WcaOnRails/app/models/concerns/resultable.rb Show resolved Hide resolved
WcaOnRails/app/models/results_submission.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/upload_json.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/upload_json.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/upload_json.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/upload_json.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/upload_json.rb Outdated Show resolved Hide resolved
@viroulep viroulep force-pushed the check-uploaded-results branch 3 times, most recently from 911d09c to 9085a5c Compare January 18, 2019 11:11
Copy link
Contributor

@jfly jfly left a comment

Choose a reason for hiding this comment

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

This is really awesome work, @viroulep!

Sorry for the massively delayed (and superficial) review, but I definitely think this is ready to go live. Super excited to hear how it goes.

WcaOnRails/app/controllers/admin_controller.rb Outdated Show resolved Hide resolved
WcaOnRails/app/controllers/admin_controller.rb Outdated Show resolved Hide resolved
WcaOnRails/app/mailers/competitions_mailer.rb Outdated Show resolved Hide resolved
WcaOnRails/app/mailers/competitions_mailer.rb Show resolved Hide resolved
WcaOnRails/app/models/round_type.rb Outdated Show resolved Hide resolved
WcaOnRails/app/models/upload_json.rb Outdated Show resolved Hide resolved
WcaOnRails/spec/factories/inbox_persons.rb Show resolved Hide resolved
@viroulep viroulep force-pushed the check-uploaded-results branch 2 times, most recently from b749ff4 to 6d149eb Compare January 25, 2019 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants