Reviews

WcaOnRails/app/controllers/admin_controller.rb

@@ -58,7 +58,7 @@ def create_results
 
     # This makes sure the json structure is valid!
     if @upload_json.import_to_inbox
-      flash[:success] = "JSON File has been imported."
+      flash[:success] = "JSON file has been imported."
       redirect_to competition_admin_upload_results_edit_path
     else
       @results_validator = CompetitionResultsValidator.new(@competition.id)
@@ -148,6 +148,6 @@ def update_statistics
   end
 
   private def competition_from_params
-    Competition.find(params[:competition_id])
+    Competition.find_by_id!(params[:competition_id])
   end
 end

WcaOnRails/app/controllers/results_submission_controller.rb

@@ -25,7 +25,7 @@ def upload_json
     # This makes sure the json structure is valid!
     if @upload_json.import_to_inbox
       flash[:success] = "JSON File has been imported."
-      @competition.uploaded_jsons = [UploadedJson.create(json_str: @upload_json.results_json_str)]
+      @competition.uploaded_jsons.create(json_str: @upload_json.results_json_str)
       redirect_to competition_submit_results_edit_path
     else
       @results_validator = CompetitionResultsValidator.new(@competition.id)

WcaOnRails/app/helpers/results_submission_helper.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+module ResultsSubmissionHelper
+  def class_for_panel(error:, warning:)
+    if error
+      "danger"
+    elsif warning
+      "warning"
+    else
+      "success"
+    end
+  end
+end

WcaOnRails/app/mailers/competitions_mailer.rb

@@ -122,7 +122,7 @@ def results_submitted(competition, results_submission, submitter_user)
     @competition = competition
     @results_submission = results_submission
     @submitter_user = submitter_user
-    last_uploaded_json = @competition.uploaded_jsons.last
+    last_uploaded_json = @competition.uploaded_jsons.order(:id).last
     if last_uploaded_json
       attachments["Results for #{@competition.id}.json"] = {
         mime_type: "application/json",

WcaOnRails/app/models/competition.rb

@@ -373,7 +373,7 @@ def build_clone
              'continent',
              'championships',
              'rounds',
-             'uploaded_jsons'
+             'uploaded_jsons',
              'wcif_extensions'
           # Do nothing as they shouldn't be cloned.
         when 'organizers'

WcaOnRails/app/models/concerns/resultable.rb

@@ -39,7 +39,7 @@ def format
     def validate_each_solve
       solve_times.each_with_index do |solve_time, i|
         unless solve_time.valid?
-          errors.add(:"value#{i + 1}", solve_time.errors.messages[:base].join(" "))
+          errors.add(:"value#{i + 1}", solve_time.errors.full_messages.join(" "))
         end
       end
     end

WcaOnRails/app/models/results_submission.rb

@@ -7,7 +7,7 @@ class ResultsSubmission
 
   validates :message, presence: true
   validates :competition_id, presence: true
-  CONFIRM_INFORMATION_ERROR = "You must confirm the information are accurate"
+  CONFIRM_INFORMATION_ERROR = "You must confirm the information is accurate"
   validates_acceptance_of :confirm_information, message: CONFIRM_INFORMATION_ERROR, allow_nil: false
   validates :schedule_url, presence: true, url: true
 
@@ -22,7 +22,8 @@ def results_validator
     @results_validator ||= CompetitionResultsValidator.new(competition_id)
   end
 
-  # FIXME: what is this used for?
+  # This is used in specs to compare two ResultsSubmission
+  # See spec/requests/results_submission_spec.rb
   def ==(other)
     self.class == other.class && self.state == other.state
   end

WcaOnRails/app/models/round_type.rb

@@ -22,22 +22,9 @@ def combined?
   end
 
   # Returns the equivalent round_type_id with cutoff (or without cutoff)
-  def self.equivalent(round_type_id)
-    case round_type_id
-    when "c"
-      "f"
-    when "f"
-      "c"
-    when "d"
-      "1"
-    when "1"
-      "d"
-    when "e"
-      "2"
-    when "2"
-      "e"
-    when "g"
-      "3"
-    end
+  def self.toggle_cutoff(round_type_id)
+    [%w(c f), %w(d 1), %w(e 2), %w(g 3)]
+      .flat_map { |pair| [pair, pair.reverse] }
+      .to_h[round_type_id]
   end
 end

WcaOnRails/app/models/upload_json.rb

@@ -13,19 +13,22 @@ class UploadJson
     else
       begin
         # Parse the json first
-        json = JSON.parse(results_json_str)
-        JSON::Validator.validate!(CompetitionResultsValidator::RESULT_JSON_SCHEMA, json)
-        if json["competitionId"] != competition_id
-          errors.add(:results_file, "this JSON file is not for this competition but for #{json["competitionId"]}!")
+        JSON::Validator.validate!(CompetitionResultsValidator::RESULT_JSON_SCHEMA, parsed_json)
+        if parsed_json["competitionId"] != competition_id
+          errors.add(:results_file, "is not for this competition but for #{parsed_json["competitionId"]}!")
         end
       rescue JSON::ParserError
         errors.add(:results_file, "must be a JSON file from the Workbook Assistant")
       rescue JSON::Schema::ValidationError => e
-        errors.add(:results_file, "The JSON file had errors: #{e.message}")
+        errors.add(:results_file, "has errors: #{e.message}")
       end
     end
   end
 
+  def parsed_json
+    @parsed_json ||= JSON.parse(results_json_str)
+  end
+
   def results_file=(results_file)
     self.results_json_str = results_file.read
     results_file.rewind
@@ -36,16 +39,14 @@ def import_to_inbox
     # This makes sure the json structure is valid!
     if valid?
       competition = Competition.find(competition_id)
-      json = JSON.parse(results_json_str)
-
       persons_to_import = []
-      json["persons"].each do |p|
+      parsed_json["persons"].each do |p|
         new_person_attributes = p.merge(competitionId: competition_id)
         persons_to_import << InboxPerson.new(new_person_attributes)
       end
       results_to_import = []
       scrambles_to_import = []
-      json["events"].each do |event|
+      parsed_json["events"].each do |event|
         event["rounds"].each do |round|
           # Import results for round
           round["results"].each do |result|
@@ -104,7 +105,7 @@ def import_to_inbox
         end
         true
       rescue ActiveRecord::RecordNotUnique
-        errors.add(:results_file, "Duplicate personId in JSON.")
+        errors.add(:results_file, "Duplicate record found while uploading results. Maybe there is a duplicate personId in the JSON?")
         false
       rescue ActiveRecord::RecordInvalid => invalid
         object = invalid.record
@@ -115,7 +116,6 @@ def import_to_inbox
         elsif object.class == InboxResult
           errors.add(:results_file, "Result for person #{object.personId} in '#{Round.name_from_attributes_id(object.eventId, object.roundTypeId)}' is invalid (#{invalid.message}), please fix it!")
         else
-          # FIXME: that's actually not supposed to happen, as the only 3 types of records we create are above
           errors.add(:results_file, "An invalid record prevented the results from being created: #{invalid.message}")
         end
         false
@@ -124,13 +124,4 @@ def import_to_inbox
       false
     end
   end
-
-  # FIXME: what is this used for?
-  def ==(other)
-    self.class == other.class && self.state == other.state
-  end
-
-  def state
-    [results_json_str]
-  end
 end

WcaOnRails/app/views/competitions_mailer/results_submitted.erb

@@ -4,15 +4,11 @@
   <%= @submitter_user.name %> has uploaded the results for <%= @competition.name %>.
   You can check and preview the uploaded results <%= link_to "here", competition_admin_upload_results_edit_url(@competition) %>, or you can go ahead with the posting process <%= link_to "here", "#{root_url}results/admin/upload_results.php?competitionId=#{@competition.id}" %>.
   <br>
-  The latest uploaded json by the Delegate is attached, if any.
+  The latest uploaded json by the Delegate is attached.
 </p>
 
 <p>
-  <% if @competition.has_schedule? %>
-    The competition schedule is available on the WCA website <%= link_to "here", link_to_competition_schedule_tab(@competition) %>.
-  <% else %>
-    The competition schedule was not entered in the WCA website but is available at <%= link_to @results_submission.schedule_url, @results_submission.schedule_url %>.
-  <% end %>
+  The competition schedule is available on the WCA website <%= link_to "here", link_to_competition_schedule_tab(@competition) %>.
   <br>
   The Delegate has confirmed the competition's events page and the competition's schedule page reflect what happened during the competition.
 </p>

WcaOnRails/app/views/results_submission/_check_results_panel.html.erb

@@ -1,13 +1,8 @@
 <%
   force_collapse ||= false
   # NOTE: expect valid 'results_validator' variable
-  panel_style = if results_validator.total_errors > 0
-                  "danger"
-                elsif results_validator.total_warnings > 0
-                  "warning"
-                else
-                  "success"
-                end
+  panel_style = class_for_panel(error: results_validator.total_errors > 0,
+                                warning: results_validator.total_warnings > 0)
   collapse_panel = force_collapse || !results_validator.has_results
 %>
 <div class="panel panel-<%= panel_style %>">

WcaOnRails/app/views/results_submission/_results_submission_panel.html.erb

@@ -1,13 +1,12 @@
 <%
   has_errors ||= false
   results_submission ||= ResultsSubmission.new
-  schedule_url_kwargs = {}
-  if competition.has_schedule?
-    schedule_url_kwargs[:readonly] = true
-    schedule_url_kwargs[:input_html] = {
+  schedule_url_kwargs = {
+    readonly: true,
+    input_html: {
       value: link_to_competition_schedule_tab(competition),
-    }
-  end
+    },
+  }
 %>
 <div class="panel panel-default">
   <div class="panel-heading heading-as-link <%= "collapsed" if has_errors %>" data-toggle="collapse" data-target="#collapse-submit-results">
@@ -19,15 +18,7 @@
   <div id="collapse-submit-results" class="panel-body collapse <%= "in" unless has_errors %>">
     <% unless has_errors %>
       <p>Please enter the body of your email to the Results Team.</p>
-      <p>
-        <% if competition.has_schedule? %>
-          It looks like you have entered your competition's schedule into the WCA website, so we automatically filled the link to the schedule below.
-          Make sure the schedule on the WCA website actually reflects what happened during the competition.
-        <% else %>
-          Make sure to include a link to the competition schedule (and all time
-          limits and cutoffs) and provide any changes to the provided schedule.
-        <% end %>
-      </p>
+      <p>Make sure the schedule on the WCA website actually reflects what happened during the competition.</p>
       <p>
         Please also make sure to include any other additional details required by the
         <%= link_to "'Submitting competition results' section of the delegate crash course",
@@ -41,7 +32,7 @@
         <%= f.submit "Submit", class: "btn btn-primary" %>
       <% end %>
     <% else %>
-      <p>Please upload a JSON file and make sure the results don't contain any error.</p>
+      <p>Please upload a JSON file and make sure the results don't contain any errors.</p>
     <% end %>
   </div>
 </div>

WcaOnRails/app/views/results_submission/_submit_json_panel.html.erb

@@ -1,15 +1,9 @@
 <% already_has_submitted_result ||= false %>
 <% action ||= competition_upload_results_json_path %>
 <% upload_json ||= UploadJson.new %>
-<%
-  panel_style = if upload_json.errors.any?
-                  "danger"
-                elsif already_has_submitted_result
-                  "success"
-                else
-                  "warning"
-                end
-%>
+<% panel_style = class_for_panel(error: upload_json.errors.any?,
+                                 warning: !already_has_submitted_result) %>
+
 <div class="panel panel-<%= panel_style %>">
   <div class="panel-heading heading-as-link <%= "collapsed" if upload_json.errors.empty? && already_has_submitted_result %>" data-toggle="collapse" data-target="#collapse-submit-json">
     <h3 class="panel-title">

WcaOnRails/app/views/results_submission/_table_for_round.html.erb

@@ -2,36 +2,29 @@
 <% persons_by_id = results_validator.persons_by_id %>
 <% expected_rounds_by_ids = results_validator.expected_rounds_by_ids %>
 <% round_name = expected_rounds_by_ids[round_id]&.name || Round.name_from_attributes_id(*round_id.split("-")) %>
-<%# FIXME: replace this by round.name when https://github.com/thewca/worldcubeassociation.org/pull/3018 is merged! %>
 <h3><%= link_to round_name, "##{round_id}", "data-toggle": "collapse" %></h3>
 <%= wca_table table_class: "wca-results collapse", floatThead: false, table_id: round_id do %>
   <thead>
     <tr>
       <th class="pos">#</th>
-
       <th class="name"><%= t 'competitions.results_table.name' %></th>
-
       <th class="single"><%= t 'common.best' %></th>
-
       <th class="average"><%= t 'common.average' %></th>
-
       <th class="country"><%= t 'common.user.citizen_of' %> </th>
-
       <th class="solves" colspan="5"><%= t 'common.solves' %></th>
-
       <% # Extra column for .table-greedy-last-column %>
       <th></th>
     </tr>
   </thead>
 
   <tbody>
     <% results.each do |result| %>
+      <% person = persons_by_id[result.personId] %>
       <tr>
         <td class="pos">
           <%= result.pos %>
         </td>
         <td class="name">
-          <% person = persons_by_id[result.personId] %>
           <% unless person.wca_id.blank? %>
             <%= link_to person.name, person_path(person.wca_id) %>
           <% else %>
@@ -42,7 +35,6 @@
         <td class="average"><%= result.average_solve.clock_format %></td>
         <td class="country"><%= person.country.name %></td>
         <%= solve_tds_for_result(result) %>
-
         <% # Extra column for .table-greedy-last-column %>
         <td></td>
       </tr>