Fix scoreboard sorting failing on custom scoreboardOrderSubmissions, add more error checking when creating scoreboard entries / displaying

app/controllers/scoreboards_controller.rb

@@ -16,11 +16,11 @@ def create
     end
     begin
       @scoreboard.save!
-      flash[:notice] = "Scoreboard Created"
+      flash[:success] = "Created scoreboard successfully."
     rescue ActiveRecord::RecordInvalid => e
       flash[:error] = "Unable to create scoreboard: #{e.message}"
     end
-    redirect_to(action: :edit) && return
+    redirect_to(edit_course_assessment_scoreboard_path(@course, @assessment))
   end
 
   action_auth_level :show, :student
@@ -81,18 +81,18 @@ def show
                         )
                       end
     rescue StandardError => e
-      # Screw 'em! usually this means the grader failed.
-      grade[:entry] = {}
-      # But, if this was an instructor, we want them to know about
-      # this.
+      # the scoreboard autoresult wasn't correctly formatted
+      # so we just return an empty array, which will be handled
+      # by the #show view code
+      grade[:entry] = []
+
       if @cud.instructor?
         # not using flash because could be too large of a message to pass
         @errorMessage = "An error occurred while calling " \
           "createScoreboardEntry(#{grade[:problems].inspect},\n"\
           "#{grade[:autoresult]})"
         @error = e
         Rails.logger.error("Scoreboard error in #{@course.name}/#{@assessment.name}: #{@error}")
-        render("scoreboards/edit") && return
       end
     end
 
@@ -105,22 +105,68 @@ def show
 
     # Catch errors along the way. An instructor will get the errors, a
     # student will simply see an unsorted scoreboard.
-    @sortedGrades = @grades.values.sort do |a, b|
-      if @assessment.overwrites_method?(:scoreboardOrderSubmissions)
-        @assessment.config_module.scoreboardOrderSubmissions(a, b)
-      else
-        scoreboardOrderSubmissions(a, b)
+    begin
+      @sortedGrades = @grades.values.sort do |a, b|
+        # we add a default sort, in the case that the scoreboard entry could
+        # not be generated correctly (which results in a empty array)
+        if a[:entry].empty? || b[:entry].empty?
+          b[:entry].length <=> a[:entry].length
+        elsif @assessment.overwrites_method?(:scoreboardOrderSubmissions)
+          begin
+            @assessment.config_module.scoreboardOrderSubmissions(a, b)
+          rescue StandardError => e
+            if @cud.instructor?
+              # not using flash because could be too large of a message to pass
+              @errorMessage = "An error occurred while calling "\
+                "custom hook scoreboardOrderSubmissions(#{a.inspect},\n"\
+                "#{b.inspect})"
+              @error = e
+              Rails.logger.error("Custom scoreboard error in " \
+                                   "#{@course.name}/#{@assessment.name}: #{@error}")
+              render("scoreboards/edit") && return
+            else
+              flash[:error] =
+                "The scoreboard cannot be viewed due to an error. Please contact your instructor."
+              redirect_to(course_assessment_path(@course, @assessment)) && return
+            end
+          end
+        else
+          begin
+            scoreboardOrderSubmissions(a, b)
+          rescue StandardError => e
+            # this is a catch all if sorting in general just doesn't work
+            if @cud.instructor?
+              # not using flash because could be too large of a message to pass
+              @errorMessage = "An error occurred while calling "\
+                "scoreboardOrderSubmissions(#{a.inspect},\n"\
+                "#{b.inspect})"
+              @error = e
+              Rails.logger.error("scoreboard order error in #{@course.name}/#{@assessment.name}: " \
+                                   "#{@error}")
+              render("scoreboards/edit") && return
+            else
+              flash[:error] =
+                "The scoreboard cannot be viewed due to an error. Please contact your instructor."
+              redirect_to(course_assessment_path(@course, @assessment)) && return
+            end
+          end
+        end
       end
-    rescue StandardError => e
+    rescue ArgumentError => e
       if @cud.instructor?
         # not using flash because could be too large of a message to pass
-        @errorMessage = "An error occurred while calling "\
-          "scoreboardOrderSubmissions(#{a.inspect},\n"\
-          "#{b.inspect})"
+        @errorMessage = "An error occurred while sorting "\
+          "submissions. Please make sure your scoreboard results are returned " \
+          "as an array of numbers. If you are using a custom scoreboard order, ensure your " \
+          "hook is functioning correctly."
         @error = e
+        Rails.logger.error("scoreboard error in #{@course.name}/#{@assessment.name}: #{@error}")
         render("scoreboards/edit") && return
+      else
+        flash[:error] =
+          "The scoreboard cannot be viewed due to an error. Please contact your instructor."
+        redirect_to(course_assessment_path(@course, @assessment)) && return
       end
-      0 # Just say they're equal!
     end
 
     @colspec = nil
@@ -139,8 +185,8 @@ def edit
   action_auth_level :update, :instructor
   def update
     if @scoreboard.update(scoreboard_params)
-      flash[:notice] =
-        "Saved!"
+      flash[:success] =
+        "Scoreboard saved."
     else
       flash[:error] =
         @scoreboard.errors.full_messages.join("")
@@ -151,21 +197,18 @@ def update
   action_auth_level :destroy, :instructor
   def destroy
     if @scoreboard.destroy
-      flash[:notice] = "Destroyed!"
+      flash[:success] = "Scoreboard successfully deleted."
     else
-      flash[:error] = "Unable to destroy scoreboard"
+      flash[:error] = "Unable to destroy scoreboard."
     end
-    redirect_to([:edit, @course, @assessment]) && return
+    redirect_to(edit_course_assessment_path(@course, @assessment))
   end
 
-  action_auth_level :help, :instructor
-  def help; end
-
 private
 
   def set_scoreboard
     @scoreboard = @assessment.scoreboard
-    redirect_to([@course, @assessment]) if @scoreboard.nil?
+    redirect_to(course_assessment_path(@course, @assessment)) if @scoreboard.nil?
   end
 
   def scoreboard_params
@@ -246,44 +289,26 @@ def createScoreboardEntry(scores, autoresult)
     # At this point we have an autograded assessment with a
     # customized scoreboard. Extract the scoreboard entry
     # from the scoreboard array object in the JSON autoresult.
-    begin
-      parsed = ActiveSupport::JSON.decode(autoresult)
-      if !parsed["scoreboard"].is_a?(Array) && @cud.instructor?
-        flash[:error] = "Error parsing scoreboard for autograded assessment: scoreboard result is"\
-          " not an array. Please ensure that the autograder returns scoreboard results as an array."
-      end
-      Rails.logger.error("Scoreboard error in #{@course.name}/#{@assessment.name}: " \
-                           "Scoreboard result is not an array")
-      raise if !parsed || !parsed["scoreboard"] || !parsed["scoreboard"].is_a?(Array)
-    rescue StandardError
+
+    parsed = ActiveSupport::JSON.decode(autoresult)
+
+    # ensure that the parsed result is a hash with scoreboard field, where scoreboard is an array
+    if !parsed || !parsed.is_a?(Hash) || !parsed["scoreboard"] ||
+       !parsed["scoreboard"].is_a?(Array)
       # If there is no autoresult for this student (typically
       # because their code did not compile or it segfaulted and
       # the instructor's autograder did not catch it) then
-      # return a nicely formatted nil result.
-      begin
-        parsed = ActiveSupport::JSON.decode(@scoreboard.colspec)
-        raise if !parsed || !parsed["scoreboard"]
-
-        entry = []
-        parsed["scoreboard"].each do |_item|
-          entry << "-"
-        end
-        return entry
-      rescue StandardError
-        if @cud.instructor?
-          flash[:error] = "Error parsing scoreboard for autograded assessment: Please ensure the"\
-          " scoreboard results from the autograder are formatted to be an array, and the colspec"\
-          " matches the expected format."
-          Rails.logger.error("Scoreboard error in #{@course.name}/#{@assessment.name}: " \
-                               "Scoreboard could not be parsed")
-        end
-        # Give up and bail
-        return ["-"]
+      # raise an error, will be handled by caller
+      if @cud.instructor?
+        (flash.now[:error] = "Error parsing scoreboard for autograded assessment: " \
+          "scoreboard result is not an array. Please ensure that the autograder returns " \
+          "scoreboard results as an array.")
       end
+      Rails.logger.error("Scoreboard error in #{@course.name}/#{@assessment.name}: " \
+                           "Scoreboard result is not an array")
+      raise StandardError
     end
-    # Found a valid scoreboard array, so simply return it. If we
-    # wanted to be really careful, we would verify that the size
-    # was the same size as the column specification.
+
     parsed["scoreboard"]
   end
 

app/views/scoreboards/_form.html.erb

@@ -1,12 +1,12 @@
-<%= form_for [@course, @assessment, @scoreboard], builder: FormBuilderWithDateTimeInput do |f| %>
+<%= form_for @scoreboard, url: course_assessment_scoreboard_path(@course, @assessment),
+                          builder: FormBuilderWithDateTimeInput do |f| %>
   <% unless @errorMessage.nil? %>
     <h4 class="error-header">Error Rendering Scoreboard:</h4>
     <div>
-      <pre style="text-wrap: wrap;"><%= @errorMessage %></pre>
+      <pre style="white-space: pre-wrap"><%= @errorMessage %></pre>
       <pre><%= @error %></pre>
     </div>
   <% end %>
-  <%= f.error_messages %>
   <h3>Scoreboard Settings</h3>
   <p>
     Learn more about how to configure <a href="https://docs.autolabproject.com/features/scoreboards/">custom scoreboards here.</a>
@@ -33,7 +33,7 @@
   <p>
 
   <%= f.submit "Save", { class: "btn primary" } %>
-  <%= link_to "Delete", [@course, @assessment, :scoreboard], method: :delete, class: "btn danger",
-                                                             data: { confirm: "Are you sure you want to delete the Scoreboard for this assessment?" } %>
+  <%= link_to "Delete", course_assessment_scoreboard_path(@course, @assessment), method: :delete, class: "btn danger",
+                                                                                 data: { confirm: "Are you sure you want to delete the Scoreboard for this assessment?" } %>
 
 <% end %>

app/views/scoreboards/show.html.erb

@@ -5,6 +5,13 @@
   <%= javascript_include_tag "sorttable" %>
 <% end %>
 
+<% unless @errorMessage.nil? %>
+  <h4 class="error-header">Error Rendering Scoreboard:</h4>
+  <div>
+    <pre style="white-space: pre-wrap"><%= @errorMessage %></pre>
+  </div>
+<% end %>
+
 <h4>Scoreboard</h4>
 
 <% if @grades.values.empty? %>
@@ -61,7 +68,7 @@
               <% if c["img"] %>
                 <td><img src="data:image/png;base64,<%= grade[:entry][i] %>"></td>
               <% else %>
-                <% if grade[:entry].is_a?(Array) && grade[:entry][i] != "-" %>
+                <% if grade[:entry].is_a?(Array) && grade[:entry].length > i %>
                   <td><%= grade[:entry][i] %></td>
                 <% else %>
                   <%= render 'error_icon' %>
@@ -70,10 +77,10 @@
             <% end %>
           <% else %>
             <%# this should be guaranteed to be an array, but for redundancy, check that entry is array %>
-            <% if grade[:entry].is_a?(Array) && grade[:entry][i] != "-" %>
+            <% if grade[:entry].is_a?(Array) && grade[:entry].length > i %>
               <% grade[:entry].each do |column| %>
                 <td><%= column %></td>
-              <% end %>q
+              <% end %>
             <% else %>
               <%= render 'error_icon' %>
             <% end %>

config/routes.rb

@@ -135,9 +135,7 @@
         post "import", on: :collection
       end
       resources :problems, except: [:index, :show]
-      resource :scoreboard, except: [:new] do
-        get "help", on: :member
-      end
+      resource :scoreboard, except: [:new]
       resources :submissions do
         resources :annotations, only: [:create, :update, :destroy] do
           collection do