Sort scoreboard beyond third column, improve CUD validations (#1925)

* Update styling * Allow columns beyond col 3 to be sorted * Update documentation * Remove unnecessary checks * Update colspec validations * Remove unnecessary error display * Add maxlength 32 to nickname field * Fix syntax of valid_nickname? validation * Display errors for edit page directly in flash * Skip validation if nickname is nil * Standardize new page with edit page * Update validation js * Add space in maxlength * Make error message consistent
autolab · Jun 21, 2023 · e62ca22 · e62ca22
1 parent 0011fab
commit e62ca22
Show file tree

Hide file tree

Showing 10 changed files with 57 additions and 125 deletions.
diff --git a/app/assets/javascripts/course_user_data_edit.js b/app/assets/javascripts/course_user_data_edit.js
@@ -1,28 +1,18 @@
 function formvalidation(form){
     function DoubleByte(str) {
       for (var i = 0, n = str.length; i < n; i++) {
-        if (str[i].charCodeAt() > 127) { return true; }
+        if (str.charCodeAt(i) > 127) { return true; }
       }
       return false;
     }
-    var formlog = 'Your nickname ';
-    function flag(msg, nickname){
-      nickname.setAttribute('style','background-color:#FFF352');
-      if (formlog!= 'Your nickname '){formlog+=' and ';}
-      formlog +=msg;
+    var nickname = document.getElementById('course_user_datum_nickname');
 
+    if (DoubleByte(nickname.value)){
+      nickname.setAttribute('style','background-color:#FFF352');
       nickname.focus();
-    }
-    var nickname = document.getElementById('user_nickname');
-    var approve = true;
-
-    if (nickname.value.length>20){approve = false; flag('is too long',nickname);}
-    if (DoubleByte(nickname.value)===true){approve = false; flag('has non-ASCII characters',nickname);}
-
-    if (approve){
-      form.submit();
+      alert("Your nickname has non-ASCII characters");
     } else {
-      alert(formlog);
+      form.submit();
     }
 }
 

diff --git a/app/controllers/course_user_data_controller.rb b/app/controllers/course_user_data_controller.rb
@@ -72,7 +72,7 @@ def create
         redirect_to([:users, @course]) && return
       end
     else
-      error_msg = "Adding user failed:"
+      error_msg = "Creation failed."
       if !@newCUD.valid?
         @newCUD.errors.full_messages.each do |msg|
           error_msg += "<br>#{msg}"
@@ -160,7 +160,9 @@ def update
     else
       COURSE_LOGGER.log(@editCUD.errors.full_messages.join(", "))
       # error details are shown separately in the view
-      flash[:error] = "Update failed. Check all fields"
+      flash[:error] = "Update failed.<br>"
+      flash[:error] += @editCUD.errors.full_messages.join("<br>")
+      flash[:html_safe] = true
       redirect_to(action: :edit) && return
     end
   end

diff --git a/app/controllers/scoreboards_controller.rb b/app/controllers/scoreboards_controller.rb
@@ -208,7 +208,7 @@ def emitColSpec(colspec)
         str += " | "
       end
       str += hash["hdr"].to_s.upcase
-      str += hash["asc"] ? " [asc]" : " [desc]" if i < 3
+      str += hash["asc"] ? " [asc]" : " [desc]"
       i += 1
     end
     str
@@ -318,46 +318,22 @@ def scoreboardOrderSubmissions(a, b)
       # in descending order. Lab authors can modify the default
       # direction with the "asc" key in the column spec.
     else
-      a0 = a[:entry][0].to_f
-      a1 = a[:entry][1].to_f
-      a2 = a[:entry][2].to_f
-      b0 = b[:entry][0].to_f
-      b1 = b[:entry][1].to_f
-      b2 = b[:entry][2].to_f
-
       begin
         parsed = ActiveSupport::JSON.decode(@scoreboard.colspec)
       rescue StandardError => e
         Rails.logger.error("Error in scoreboards controller updater: #{e.message}")
       end
 
-      if a0 != b0
-        if parsed && parsed["scoreboard"] &&
-           !parsed["scoreboard"].empty? &&
-           parsed["scoreboard"][0]["asc"]
-          a0 <=> b0 # ascending order
-        else
-          b0 <=> a0 # descending order
-        end
-      elsif a1 != b1
-        if parsed && parsed["scoreboard"] &&
-           parsed["scoreboard"].size > 1 &&
-           parsed["scoreboard"][1]["asc"]
-          a1 <=> b1 # ascending order
-        else
-          b1 <=> a1 # descending order
-        end
-      elsif a2 != b2
-        if parsed && parsed["scoreboard"] &&
-           parsed["scoreboard"].size > 2 &&
-           parsed["scoreboard"][2]["asc"]
-          a2 <=> b2 # ascending order
-        else
-          b2 <=> a2 # descending order
-        end
-      else
-        a[:time] <=> b[:time] # ascending by submission time
+      # Validations ensure that colspec is of the correct format
+      parsed["scoreboard"].each_with_index do |v, i|
+        ai = a[:entry][i].to_f
+        bi = b[:entry][i].to_f
+        next unless ai != bi
+        return ai <=> bi if v["asc"] # ascending
+
+        return bi <=> ai # descending otherwise
       end
+      a[:time] <=> b[:time] # ascending by submission time to tiebreak
     end
   end
 end
diff --git a/app/models/course_user_datum.rb b/app/models/course_user_datum.rb
@@ -63,17 +63,15 @@ def after_update
   end
 
   def valid_nickname?
-    if !nickname
-      true
-    elsif nickname.length > 32
+    return if nickname.nil?
+
+    if nickname.length > 32
       errors.add("nickname", "is too long (maximum is 32 characters)")
-      false
-    elsif !nickname.ascii_only?
-      errors.add("nickname", "can only contain ASCII characters")
-      false
-    else
-      true
     end
+
+    return if nickname.ascii_only?
+
+    errors.add("nickname", "can only contain ASCII characters")
   end
 
   ##

diff --git a/app/models/scoreboard.rb b/app/models/scoreboard.rb
@@ -24,14 +24,17 @@ def colspec_is_well_formed
 
     # The parse will throw an exception if the string has a JSON syntax error
     begin
-      # Quote JSON keys and values if they are not already quoted
-      quoted = colspec.gsub(/([a-zA-Z0-9]+):/, '"\1":').gsub(/:([a-zA-Z0-9]+)/, ':"\1"')
-      parsed = ActiveSupport::JSON.decode(quoted)
+      parsed = ActiveSupport::JSON.decode(colspec)
     rescue StandardError => e
       errors.add "colspec", e.to_s
       return
     end
 
+    unless parsed.is_a? Hash
+      errors.add "colspec", "must be a hash object"
+      return
+    end
+
     # Colspecs must include a scoreboard array object
     unless parsed["scoreboard"]
       errors.add "colspec", "missing 'scoreboard' array object"
@@ -60,12 +63,6 @@ def colspec_is_well_formed
           errors.add "colspec", "unknown key('#{k}') in scoreboard[#{i}]"
           return
         end
-
-        next unless k == "asc" && i > 2
-
-        errors.add "colspec",
-                   "'asc' key in col #{i} ignored because only the first",
-                   "three columns are sorted."
       end
     end
   end

diff --git a/app/views/course_user_data/_fields.html.erb b/app/views/course_user_data/_fields.html.erb
@@ -7,7 +7,7 @@
 
   <% end %>
 
-  <%= f.text_field :nickname, help_text: "Anonymous nickname to display on the public scoreboards", placeholder: "droh" %>
+  <%= f.text_field :nickname, help_text: "Anonymous nickname to display on the public scoreboards (max length: 32)", placeholder: "droh", maxlength: 32 %>
 
   <%= f.text_field :course_number, help_text: "The course number", placeholder: "15213", disabled: [email protected]? %>
 

diff --git a/app/views/course_user_data/edit.html.erb b/app/views/course_user_data/edit.html.erb
@@ -4,29 +4,13 @@
   <%= javascript_include_tag "course_user_data_edit" %>
 <% end %>
 
-<br>
 <h4>Editing <%= @editCUD.user.full_name %>'s Enrollment in <%= @course.display_name %></h4>
 
 <%= form_for @editCUD, url: course_course_user_datum_path(@course, @editCUD), builder: FormBuilderWithDateTimeInput do |f| %>
-<% if @editCUD.errors.any? %>
-	<div id="error_explanation">
-		<h2><%= pluralize(@editCUD.errors.count, "error") %>
-      prohibited the data from being saved:</h2>
-
-		<ul>
-			<% @editCUD.errors.full_messages.each do |msg| %>
-			<li><%= msg %></li>
-			<% end %>
-		</ul>
-	</div>
-<% end %>
-
-  <br><br>
+  <br>
   <%= render partial: "fields", locals: {f: f, cud: @editCUD, edit: true} %>
 
   <br>
-  <br>
-  <input id="user_submit" name="commit" type="submit" class="btn primary" value="Save Changes" onclick="formvalidation(this.parentNode); return false;">
-  <!--<%= f.submit 'Save Changes' , {:class=>""} %>-->
+  <input id="user_submit" name="commit" type="submit" class="btn primary" value="Save Changes" onclick="formvalidation(this.closest('form')); return false;">
 
-<%end %>
+<% end %>
diff --git a/app/views/course_user_data/new.html.erb b/app/views/course_user_data/new.html.erb
@@ -1,4 +1,5 @@
 <% content_for :javascripts do %>
+  <%= javascript_include_tag "course_user_data_edit" %>
   <script type="application/javascript">
     $("#course_user_datum_user_attributes_email").on('change', user_lookup);
     $("#course_user_datum_user_attributes_email").on('input', user_lookup);
@@ -31,28 +32,14 @@
 
 <% @title="Enroll User" %>
 
-<h2>Enroll User in <%= @course.display_name %></h2>
-
-<br>
+<h4>Enroll User in <%= @course.display_name %></h4>
 
 <%= form_for @newCUD, url: course_course_user_data_path, builder: FormBuilderWithDateTimeInput do |f| %>
 
-<% if @newCUD.errors.any? %>
-	<div id="error_explanation">
-		<h2><%= pluralize(@newCUD.errors.count, "error") %>
-      prohibited the data from being saved:</h2>
-
-		<ul>
-			<% @newCUD.errors.full_messages.each do |msg| %>
-			<li><%= msg %></li>
-			<% end %>
-		</ul>
-	</div>
-<% end %>
-
+  <br>
   <%= render partial: "fields", locals: {f: f, cud: @newCUD, edit: false} %>
 
   <br>
-  <input id="user_submit" class="btn primary"name="commit" type="submit" value="Save Changes" onclick="formvalidation(this.parentNode); return false;">
+  <input id="user_submit" class="btn primary" name="commit" type="submit" value="Save Changes" onclick="formvalidation(this.closest('form')); return false;">
 
 <% end %>
diff --git a/app/views/scoreboards/show.html.erb b/app/views/scoreboards/show.html.erb
@@ -1,19 +1,22 @@
 <% @title = "Scoreboard" %>
 
 <% content_for :javascripts do %>
+  <%= javascript_include_tag "course_user_data_edit" %>
   <%= javascript_include_tag "sorttable" %>
 <% end %>
 
+<h4>Scoreboard</h4>
+
 <% if @grades.values.empty? %>
-  <h3>No submissions yet.</h3>
+  <strong>There are currently no submissions.</strong>
 <% else %>
-  <h3>
+  <strong>
     <% if @scoreboard.banner.blank? %>
       Here are the most recent scores for the class.
     <% else %>
       <%= sanitize @scoreboard.banner %>
     <% end %>
-  </h3>
+  </strong>
 
   <table class="sortable prettyBorder">
     <thead>
@@ -73,19 +76,14 @@
 <% end %>
 
 
-<%= form_for @cud, url: course_course_user_datum_path(@course, @cud) do |f| %>
-<% if @cud.errors.any? %>
-  <div id="error_explanation">
-    <h2><%= pluralize(@cud.errors.count, "error") %> prohibited the data from being saved:</h2>
-    <ul>
-      <% @cud.errors.full_messages.each do |msg| %>
-        <li><%= msg %></li>
-      <% end %>
-    </ul>
+<%= form_for @cud, url: course_course_user_datum_path(@course, @cud), builder: FormBuilderWithDateTimeInput do |f| %>
+  <hr>
+  <div class="row valign-wrapper">
+    <div class="col s6">
+      <%= f.text_field :nickname, help_text: "Anonymous nickname to display on the public scoreboards (max length: 32)", maxlength: 32 %>
+    </div>
+    <div class="col s6">
+      <input id="user_submit" name="commit" type="submit" class="btn" value="Update" onclick="formvalidation(this.closest('form')); return false;">
+    </div>
   </div>
 <% end %>
-<hr>
-  <table width=50% class="verticalTable" >
-  <tr><th><h3>Nickname:</h3> </th><td><%= f.text_field :nickname %>  <input id="user_submit" name="commit" type="submit" class="btn" value="Update Nickname" onclick="formvalidation(this.parentNode); return false;"></td></tr>
-  </table>
-<% end %>
diff --git a/docs/features/scoreboards.md b/docs/features/scoreboards.md
@@ -24,7 +24,7 @@ The column spec consists of a "scoreboard" object, which is an array of JSON obj
 }
 ```
 
-A custom scoreboard sorts the first three columns, from left to right, in descending order. You can change the default sort order for a particular column by adding an optional "asc:1" element to its hash.
+A custom scoreboard sorts the columns from left to right in descending order, and tiebreaks by submission time. You can change the default sort order for a particular column by adding an optional "asc:1" element to its hash.
 
 **Example:** Scoreboard with two columns, "Score" and "Ops", with "Score" sorted descending, and then "Ops" ascending: