Improve attachments interface and error handling (#1682)

* Add "Add Attachment" button for instructors when viewing a course

* Decouple create attachment logic from update attachment

* Move "add attachment" to top, wrap in <p> to be consistent

* Use modern syntax for building routes

* Make attachment list appearance consistent across course and assessment attachments

* Remove more dead code

* Make assessment list page appearance consistent

Co-authored-by: Joey Wildman <[email protected]>

Author: damianhxy

app/controllers/attachments_controller.rb

@@ -31,7 +31,28 @@ def create
                     @course.attachments.new
                   end
 
-    update
+    if @attachment.update(attachment_params)
+      flash[:success] = "Attachment created"
+      redirect_to_attachment_list
+    else
+      error_msg = "Attachment create failed:"
+      if !@attachment.valid?
+        @attachment.errors.full_messages.each do |msg|
+          error_msg += "<br>#{msg}"
+        end
+      else
+        error_msg += "<br>Unknown error"
+      end
+      flash[:error] = error_msg
+      flash[:html_safe] = true
+      COURSE_LOGGER.log("Failed to create attachment: #{error_msg}")
+
+      if @is_assessment
+        redirect_to new_course_assessment_attachment_path(@course, @assessment)
+      else
+        redirect_to new_course_attachment_path(@course)
+      end
+    end
   end
 
   action_auth_level :show, :student
@@ -61,11 +82,9 @@ def edit; end
   action_auth_level :update, :instructor
   def update
     if @attachment.update(attachment_params)
-      # is successful
       flash[:success] = "Attachment updated"
-      redirect_to_attachment_list && return
+      redirect_to_attachment_list
     else
-      # not successful, go back to edit page
       error_msg = "Attachment update failed:"
       if !@attachment.valid?
         @attachment.errors.full_messages.each do |msg|
@@ -79,18 +98,18 @@ def update
       COURSE_LOGGER.log("Failed to update attachment: #{error_msg}")
 
       if @is_assessment
-        redirect_to([:edit, @course, @assessment, @attachment]) && return
+        redirect_to edit_course_assessment_attachment_path(@course, @assessment, @attachment)
+      else
+        redirect_to edit_course_attachment_path(@course, @attachment)
       end
-
-      redirect_to([:edit, @course, @attachment]) && return
     end
   end
 
   action_auth_level :destroy, :instructor
   def destroy
     @attachment.destroy
     flash[:success] = "Attachment deleted"
-    redirect_to_attachment_list && return
+    redirect_to_attachment_list
   end
 
 private
@@ -110,15 +129,15 @@ def set_attachment
 
     COURSE_LOGGER.log("Cannot find attachment with id: #{params[:id]}")
     flash[:error] = "Could not find Attachment \# #{params[:id]}"
-    redirect_to_attachment_list && return
+    redirect_to_attachment_list
   end
 
   def redirect_to_attachment_list
     if @is_assessment
-      (redirect_to([@course, @assessment]) && return)
+      redirect_to course_assessment_path(@course, @assessment)
+    else
+      redirect_to course_path(@course)
     end
-
-    redirect_to([@course, :attachments]) && return
   end
 
   def add_attachments_breadcrumb

app/views/assessments/index.html.erb

@@ -121,10 +121,20 @@
   </div>
 </div>
 
-<% if @attachments.any? %>
+<% if @attachments.any? or @cud.instructor? %>
   <div class="section">
     <h3 class="section-title"><span class="white">Attachments</span></h3>
     <ul class="collection with-header attachments">
+      <% if @cud.instructor? %>
+        <li class="collection-item add">
+          <p>
+          <%= link_to new_course_attachment_path(@course) do %>
+            <i class="material-icons left">note_add</i>Add Attachment
+          <% end %>
+          <span class="secondary-content"></span>
+          </p>
+        </li>
+      <% end %>
       <%= render @attachments %>
     </ul>
   </div>

app/views/assessments/show.html.erb

@@ -195,8 +195,6 @@
     <div class="section">
       <h4 class="section-title"><span class="white">Attachments</span></h4>
       <ul class="collection with-header attachments">
-        <%= render @attachments %>
-
         <% # this code is temporarily "commented out" because Dan wants to preserve the logic for future changes to _attachment.html.erb %>
         <% if false then %>
           <% for a in @assessment.attachments do %>
@@ -211,14 +209,17 @@
             <% end %>
           <% end %>
         <% end %>
-      <% if @cud.instructor? then %>
-        <li class="collection-item add">
-          <%= link_to new_course_assessment_attachment_path(@course, @assessment) do %>
-            <i class="material-icons left">note_add</i>Add Attachment
-          <% end %>
-          <span class="secondary-content"></span>
-        </li>
-      <% end %>
+        <% if @cud.instructor? %>
+          <li class="collection-item add">
+            <p>
+            <%= link_to new_course_assessment_attachment_path(@course, @assessment) do %>
+              <i class="material-icons left">note_add</i>Add Attachment
+            <% end %>
+            <span class="secondary-content"></span>
+            </p>
+          </li>
+        <% end %>
+        <%= render @attachments %>
       </ul>
     </div>
     <hr>

app/views/attachments/_assessment_attachment.html.erb

@@ -1,28 +1,31 @@
 
 <li class="collection-item">
   <p>
-  <% if attachment.assessment.nil? then %>
-    <i class="material-icons left">file_download</i>
+  <% if attachment.assessment.nil? %>
     <%= link_to [@course, attachment] do %>
+      <i class="material-icons left">file_download</i>
       <%= attachment.name %>
+      <% unless attachment.released %>
+        <b>(Not Released)</b>
+      <% end %>
     <% end %>
   <% else %> 
     <%= link_to [@course, attachment.assessment, attachment] do %>
       <i class="material-icons left">file_download</i>
       <%= attachment.name %>
       <% unless attachment.released %>
           <b>(Not Released)</b>
-      <%  end %>
+      <% end %>
     <% end %>
   <% end %>
 
-  <% if !attachment.assessment.nil? && @assessment.nil? then %>
+  <% if !attachment.assessment.nil? && @assessment.nil? %>
     (<%= attachment.assessment.display_name %>)
   <% end %>
 
   <span class="secondary-content">
-  <% if @cud.instructor? then %>
-    <% if attachment.assessment.nil? then %>
+  <% if @cud.instructor? %>
+    <% if attachment.assessment.nil? %>
       <%= link_to '<i class="material-icons left">mode_edit</i>'.html_safe, [:edit, @course, attachment], {class:"small"} %>
       <%= link_to '<i class="material-icons left">delete</i>'.html_safe, [@course, attachment], {method: :delete, class:"small"} %>
     <% else %>

app/views/attachments/_attachment.html.erb

@@ -1,23 +1,23 @@
-<% if @is_assessment then %> 
+<% if @is_assessment %>
   <h2>Listing <%= link_to @assessment.display_name, [@course, @assessment] %> Attachments</h2>
 <% else %>
   <h2>Listing <%= link_to @course.full_name, @course %> Attachments</h2>
 <% end %>
 
-
-<p>
-  <% if @is_assessment then %>
-    <%= link_to 'Add '+@assessment.display_name+' Attachment', new_course_assessment_attachment_path(@course, @assessment) %>
-  <% else %>
-    <%= link_to 'Add Course Attachment', new_course_attachment_path(@course) %>
-  <% end %>
-</p>
-
-<% if @attachments.any? then  %>
-  <ul>
-    <%= render @attachments %>
-  </ul>
-<% else %>
-  There are no attachments yet.
-<% end %>
+<ul class="collection with-header attachments">
+  <li class="collection-item add">
+    <p>
+      <% if @is_assessment %>
+        <%= link_to new_course_assessment_attachment_path(@course, @assessment) do %>
+          <i class="material-icons left">note_add</i>Add <%= @assessment.display_name %> Attachment
+        <% end %>
+      <% else %>
+        <%= link_to new_course_attachment_path(@course) do %>
+          <i class="material-icons left">note_add</i>Add Course Attachment
+        <% end %>
+      <% end %>
+    </p>
+  </li>
+  <%= render @attachments %>
+</ul>