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

app/controllers/scoreboards_controller.rb

@@ -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
@@ -246,44 +292,33 @@ 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
+
+    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 @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 if !parsed || !parsed["scoreboard"] || !parsed["scoreboard"].is_a?(Array)
-    rescue StandardError
-      # 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 ["-"]
-      end
     end
+
+    # 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
+    # raise an error, will be handled by caller
+    raise if !parsed || !parsed.is_a?(Hash) || !parsed["scoreboard"] ||
+             !parsed["scoreboard"].is_a?(Array)
+
     # 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.
+    # 2-17-2024: size of entry is checked on #show
     parsed["scoreboard"]
   end
 

app/views/scoreboards/show.html.erb

@@ -5,6 +5,14 @@
   <%= javascript_include_tag "sorttable" %>
 <% end %>
 
+<% unless @errorMessage.nil? %>
+  <h4 class="error-header">Error Rendering Scoreboard:</h4>
+  <div>
+    <pre style="text-wrap: wrap;"><%= @errorMessage %></pre>
+    <pre><%= @error %></pre>
+  </div>
+<% end %>
+
 <h4>Scoreboard</h4>
 
 <% if @grades.values.empty? %>
@@ -61,7 +69,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,7 +78,7 @@
             <% 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