[improvement] Separate info from error flash notices (#149)

app/assets/stylesheets/general/_flashes.sass

@@ -9,6 +9,10 @@
   animation: flashesFadeIn 400ms 100ms ease-out forwards
   animation-delay: 0.4s
 
+.flashes--error
+  background: $flashes--error--background
+  color: $flashes--error--text
+
 .flashes__icon
   margin-right: 0.4em
 

app/assets/stylesheets/general/_variables.sass

@@ -121,6 +121,9 @@ $alert--text--error: $white_pure !default
 $flashes--background: hsl(51, 87%, 94%) !default
 $flashes--text: hsl(40, 68%, 20%) !default
 
+$flashes--error--background: hsl(11, 87%, 94%) !default
+$flashes--error--text: hsl(0, 68%, 20%) !default
+
 /*
  * Account header
  */

app/controllers/manage/messages_controller.rb

@@ -44,11 +44,11 @@ def destroy
 
   def deliver
     if @message.automated?
-      flash[:notice] = "Automated messages cannot be manually delivered. Only bulk messages can."
+      flash[:error] = "Automated messages cannot be manually delivered. Only bulk messages can."
       return redirect_to manage_message_path(@message)
     end
     if @message.status != "drafted"
-      flash[:notice] = "Message cannot be re-delivered"
+      flash[:error] = "Message cannot be re-delivered"
       return redirect_to manage_messages_path
     end
     @message.update_attribute(:queued_at, Time.now)
@@ -96,7 +96,7 @@ def set_message
   def check_message_access
     return if @message.can_edit?
 
-    flash[:notice] = "Message can no longer be modified"
+    flash[:error] = "Message can no longer be modified"
     redirect_to manage_message_path(@message)
   end
 end

app/controllers/manage/questionnaires_controller.rb

@@ -38,7 +38,7 @@ def create
       if user.save
         @questionnaire.save
       else
-        flash[:notice] = user.errors.full_messages.join(", ")
+        flash[:error] = user.errors.full_messages.join(", ")
         if user.errors.include?(:email)
           @questionnaire.errors.add(:email, user.errors[:email].join(", "))
         end
@@ -70,7 +70,7 @@ def check_in
         @questionnaire.user.update_attributes(email: email)
       end
       unless @questionnaire.valid?
-        flash[:notice] = @questionnaire.errors.full_messages.join(", ")
+        flash[:error] = @questionnaire.errors.full_messages.join(", ")
         redirect_to show_redirect_path
         return
       end
@@ -82,7 +82,7 @@ def check_in
       @questionnaire.update_attribute(:checked_in_by_id, current_user.id)
       flash[:notice] = "#{@questionnaire.full_name} no longer checked in."
     else
-      flash[:notice] = "No check-in action provided!"
+      flash[:error] = "No check-in action provided!"
       redirect_to show_redirect_path
       return
     end
@@ -106,7 +106,7 @@ def destroy
   def update_acc_status
     new_status = params[:questionnaire][:acc_status]
     if new_status.blank?
-      flash[:notice] = "No status provided"
+      flash[:error] = "No status provided"
       redirect_to(manage_questionnaire_path(@questionnaire))
       return
     end
@@ -118,7 +118,7 @@ def update_acc_status
     if @questionnaire.save(validate: false)
       flash[:notice] = "Updated acceptance status to \"#{Questionnaire::POSSIBLE_ACC_STATUS[new_status]}\""
     else
-      flash[:notice] = "Failed to update acceptance status"
+      flash[:error] = "Failed to update acceptance status"
     end
 
     redirect_to manage_questionnaire_path(@questionnaire)

app/controllers/manage/schools_controller.rb

@@ -45,14 +45,14 @@ def merge
   def perform_merge
     new_school_name = params[:school][:id]
     if new_school_name.blank?
-      flash[:notice] = "Error: Must include the new school to merge into!"
+      flash[:error] = "Error: Must include the new school to merge into!"
       render :merge
       return
     end
 
     new_school = School.where(name: new_school_name).first
     if new_school.blank?
-      flash[:notice] = "Error: School doesn't exist: #{new_school_name}"
+      flash[:error] = "Error: School doesn't exist: #{new_school_name}"
       render :merge
       return
     end
@@ -68,7 +68,7 @@ def perform_merge
     if @school.questionnaire_count < 1
       @school.destroy
     else
-      flash[:notice] = "*** Old school NOT deleted: #{@school.questionnaire_count} questionnaires still associated!\n"
+      flash[:error] = "*** Old school NOT deleted: #{@school.questionnaire_count} questionnaires still associated!\n"
     end
 
     redirect_to manage_school_path(new_school)

app/controllers/rsvps_controller.rb

@@ -21,7 +21,7 @@ def accept
       flash[:notice] = "Thank you for confirming your attendance! You're all set to attend."
       flash[:notice] += " See below for additional bus information." if BusList.any?
     else
-      flash[:notice] = rsvp_error_notice
+      flash[:error] = rsvp_error_notice
     end
     redirect_to rsvp_path
   end
@@ -31,10 +31,11 @@ def deny
     @questionnaire.acc_status = "rsvp_denied"
     @questionnaire.acc_status_author_id = current_user.id
     @questionnaire.acc_status_date = Time.now
-    unless @questionnaire.save
-      flash[:notice] = rsvp_error_notice
+    if @questionnaire.save
+      flash[:notice] = "Your RSVP has been updated."
+    else
+      flash[:error] = rsvp_error_notice
     end
-    flash[:notice] = "Your RSVP has been updated." if flash[:notice].blank?
     redirect_to rsvp_path
   end
 
@@ -43,13 +44,13 @@ def deny
   # rubocop:disable PerceivedComplexity
   def update
     unless @questionnaire.update_attributes(params.require(:questionnaire).permit(:agreement_accepted, :phone))
-      flash[:notice] = @questionnaire.errors.full_messages.join(", ")
+      flash[:error] = @questionnaire.errors.full_messages.join(", ")
       redirect_to rsvp_path
       return
     end
 
     unless ["rsvp_confirmed", "rsvp_denied"].include? params[:questionnaire][:acc_status]
-      flash[:notice] = "Please select a RSVP status."
+      flash[:error] = "Please select a RSVP status."
       redirect_to rsvp_path
       return
     end
@@ -63,23 +64,25 @@ def update
     is_joining_bus = new_bus_list.present? && @questionnaire.bus_list != new_bus_list
     if is_joining_bus && new_bus_list.full?
       if @questionnaire.bus_list_id?
-        flash[:notice] = "Sorry, that bus is full. You are still signed up for the '#{@questionnaire.bus_list.name}' bus."
+        flash[:error] = "Sorry, that bus is full. You are still signed up for the '#{@questionnaire.bus_list.name}' bus."
       else
-        flash[:notice] = "Sorry, that bus is full. You may need to arrange other plans for transportation."
+        flash[:error] = "Sorry, that bus is full. You may need to arrange other plans for transportation."
       end
     else
       @questionnaire.bus_list = new_bus_list
       @questionnaire.bus_captain_interest = params[:questionnaire][:bus_captain_interest]
     end
 
     unless @questionnaire.save
-      flash[:notice] = @questionnaire.errors.full_message.join(", ")
+      flash[:error] = @questionnaire.errors.full_message.join(", ")
       redirect_to rsvp_path
       return
     end
 
-    flash[:notice] = "Your RSVP has been updated." if flash[:notice].blank?
-    flash[:notice] += " See below for additional bus information!" if @questionnaire.bus_list_id?
+    if flash[:notice].blank? && flash[:error].blank?
+      flash[:notice] = "Your RSVP has been updated."
+      flash[:notice] += " See below for additional bus information!" if @questionnaire.bus_list_id?
+    end
 
     redirect_to rsvp_path
   end
@@ -103,6 +106,7 @@ def check_user_has_questionnaire
 
   def require_accepted_questionnaire
     return if @questionnaire.can_rsvp? || @questionnaire.checked_in?
+
     redirect_to new_questionnaires_path
   end
 end

app/controllers/users/omniauth_callbacks_controller.rb

@@ -11,7 +11,7 @@ def mlh
   end
 
   def failure
-    flash[:notice] = "External authentication failed - try again?"
+    flash[:error] = "External authentication failed - try again?"
     redirect_to new_user_session_url
   end
 end

app/views/layouts/_flashes.html.haml

@@ -1,3 +1,10 @@
+- if flash[:error].present?
+  .flashes.flashes--error
+    .container
+      .form-container
+        %span.fa.fa-info-circle.flashes__icon
+        = flash[:error].html_safe
+
 - if flash[:notice].present?
   .flashes
     .container

app/views/layouts/manage/_flashes.html.haml

@@ -1,3 +1,11 @@
+- flash[:error] = 'This is a special error flash'
+- flash[:notice] = 'This is a regular notice flash'
+
+- if flash[:error].present?
+  .alert.alert-danger.mt-3
+    %span.fa.fa-exclamation-triangle.icon-space-r-half
+    = flash[:error].html_safe
+
 - if flash[:notice].present?
   .alert.alert-info.mt-3
     %span.fa.fa-info-circle.icon-space-r-half

test/controllers/manage/messages_controller_test.rb

@@ -303,22 +303,22 @@ class Manage::MessagesControllerTest < ActionController::TestCase
       assert_difference('BulkMessageWorker.jobs.size', 0) do
         patch :deliver, params: { id: @message }
       end
-      assert_match /Automated/, flash[:notice]
+      assert_match /cannot be manually delivered/, flash[:error]
       assert_redirected_to manage_message_path(assigns(:message))
     end
 
     should "not allow multiple deliveries" do
       patch :deliver, params: { id: @message }
       assert_match /queued/, flash[:notice]
       patch :deliver, params: { id: @message }
-      assert_match /cannot/, flash[:notice]
+      assert_match /cannot/, flash[:error]
     end
 
     should "not be able to modify message after delivery" do
       @message.update_attribute(:delivered_at, 1.minute.ago)
       old_message_name = @message.name
       patch :update, params: { id: @message, message: { name: "New Message Name" } }
-      assert_match /can no longer/, flash[:notice]
+      assert_match /can no longer/, flash[:error]
       assert_equal old_message_name, @message.reload.name
     end
 

test/controllers/manage/questionnaires_controller_test.rb

@@ -375,7 +375,7 @@ class Manage::QuestionnairesControllerTest < ActionController::TestCase
       assert_equal false, @questionnaire.can_share_info
       assert_equal "", @questionnaire.phone
       assert_equal "old_email@example.com", @questionnaire.email
-      assert_match /No check-in action provided/, flash[:notice]
+      assert_match /No check-in action provided/, flash[:error]
       assert_response :redirect
       assert_redirected_to manage_questionnaire_path(@questionnaire)
     end

test/controllers/rsvps_controller_test.rb

@@ -109,7 +109,7 @@ class RsvpsControllerTest < ActionController::TestCase
         context "attempting #{status}" do
           should "include error message" do
             get status
-            assert_match /was an error/, flash[:notice]
+            assert_match /was an error/, flash[:error]
           end
 
           should "not change acceptance status" do
@@ -120,7 +120,7 @@ class RsvpsControllerTest < ActionController::TestCase
           should "include hackathon name in notice" do
             HackathonConfig['name'] = 'Foo Bar'
             get status
-            assert_match /Foo Bar Agreement/, flash[:notice]
+            assert_match /Foo Bar Agreement/, flash[:error]
           end
         end
       end
@@ -153,8 +153,8 @@ class RsvpsControllerTest < ActionController::TestCase
       assert_equal "rsvp_confirmed", @questionnaire.reload.acc_status
       assert_equal false, @questionnaire.reload.bus_list_id?
       assert_equal 0, bus_list.passengers.count
-      assert_match /full/, flash[:notice]
-      assert_no_match /still signed up/, flash[:notice]
+      assert_match /full/, flash[:error]
+      assert_no_match /still signed up/, flash[:error]
       assert_redirected_to rsvp_path
     end
 
@@ -180,19 +180,19 @@ class RsvpsControllerTest < ActionController::TestCase
       assert_equal "rsvp_confirmed", @questionnaire.reload.acc_status
       assert_equal 0, bus_list2.passengers.count, 'passenger should not be assigned to bus that is full'
       assert_equal 1, bus_list1.passengers.count, 'passenger should stay on original bus'
-      assert_match /full/, flash[:notice]
-      assert_match /still signed up/, flash[:notice]
+      assert_match /full/, flash[:error]
+      assert_match /still signed up/, flash[:error]
       assert_redirected_to rsvp_path
     end
 
     should "not error if submitting while already on a full bus" do
       bus_list = create(:bus_list, capacity: 1)
       # Initial sign up
       patch :update, params: { questionnaire: { acc_status: "rsvp_confirmed", bus_list_id: bus_list.id } }
-      assert_no_match /full/, flash[:notice], 'should not complain about bus being full'
+      assert_no_match /full/, flash[:error], 'should not complain about bus being full'
       # Submit again
       patch :update, params: { questionnaire: { acc_status: "rsvp_confirmed", bus_list_id: bus_list.id } }
-      assert_no_match /full/, flash[:notice], 'should not complain about bus being full'
+      assert_no_match /full/, flash[:error], 'should not complain about bus being full'
     end
 
     should "not send email if updating info after confirming" do
@@ -222,7 +222,7 @@ class RsvpsControllerTest < ActionController::TestCase
       @questionnaire.update_attribute(:phone, "1111111111")
       @questionnaire.update_attribute(:agreement_accepted, false)
       patch :update, params: { questionnaire: { phone: "1234567890" } }
-      assert_not_nil flash[:notice]
+      assert_not_nil flash[:error]
       assert_equal "1111111111", @questionnaire.reload.phone
       assert_redirected_to rsvp_path
     end
@@ -231,15 +231,15 @@ class RsvpsControllerTest < ActionController::TestCase
       @questionnaire.update_attribute(:phone, "1111111111")
       @questionnaire.update_attribute(:first_name, "")
       patch :update, params: { questionnaire: { phone: "1234567890" } }
-      assert_not_nil flash[:notice]
+      assert_not_nil flash[:error]
       assert_equal "1111111111", @questionnaire.reload.phone
       assert_redirected_to rsvp_path
     end
 
     should "not allow forbidden status update to questionnaire" do
       patch :update, params: { questionnaire: { acc_status: "pending" } }
       assert_equal "accepted", @questionnaire.reload.acc_status
-      assert_match /select a RSVP status/, flash[:notice]
+      assert_match /select a RSVP status/, flash[:error]
       assert_redirected_to rsvp_path
     end
   end