Too many fixes:

* cleanup the form alignment on the homepage * fixup merging to handle merging emails ADD TESTS * cleanup the display of emails in tables and show pages * add eager loading of email_addresses + schools to table views
beautyjoy · Jun 29, 2024 · 55b6872 · 55b6872
1 parent b7485f6
commit 55b6872
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 79 deletions.
diff --git a/app/controllers/merge_controller.rb b/app/controllers/merge_controller.rb
@@ -53,24 +53,24 @@ def merge_teachers(from_teacher, into_teacher)
       end
     end
 
-    merged_teacher.email_addresses = [from_teacher.email_addresses + into_teacher.email_addresses]
-
     merged_teacher = Teacher.new(merged_attributes)
+    merged_teacher.email_addresses = from_teacher.email_addresses + into_teacher.email_addresses
     merged_teacher
   end
 
   # Handle merging EmailAddress records, so they all belong to the saved record.
+  # This method is designed to be inside a transaction for safety.
   def merge_email_addresses(from_teacher, into_teacher)
     existing_emails = into_teacher.email_addresses
 
     # Ensure there is only one primary email if both have a primary.
-    if into_teacher.primary_email.present? && from_teacher.primary_email.present?
-      from_teacher.primary_email.update(primary: false)
+    if into_teacher.primary_email.present?
+      from_teacher.email_addresses.update_all(primary: false)
     end
 
     from_teacher.email_addresses.each do |email_address|
-      if existing_emails.select(:email).include?(email_address.strip.downcase)
-        puts "[WARN]: Merge Teacher #{from_teacher.id} into #{into_teacher.id} found duplicate email: '#{email_address}'"
+      if existing_emails.select(:email).include?(email_address.email.strip.downcase)
+        puts "[WARN]: Merge Teacher #{from_teacher.id} into #{into_teacher.id} found duplicate email: '#{email_address.email}'"
         next
       end
       email_address.update!(teacher: into_teacher)

diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
@@ -28,7 +28,7 @@ def omniauth_callback
       Sentry.add_breadcrumb(crumb)
       Sentry.capture_message("Omniauth No User Found")
       session[:auth_data] = omniauth_info
-      flash[:alert] = "We couldn't find an account for #{omniauth_info.email}. Please submit a new request."
+      flash[:alert] = "We couldn't find an account for #{omniauth_info.email}. Please submit a new request. #{nyc_message}"
       redirect_to new_teacher_path
     end
   end
@@ -60,6 +60,6 @@ def omniauth_failure
   def nyc_message
     return "" unless omniauth_info.email.downcase.ends_with?("@schools.nyc.gov")
 
-    "Emails ending with @schools.nyc.gov are currently not working. Please try logging with Snap! or reach out to us to setup an alternate login method. Thanks!\n"
+    "Emails ending with @schools.nyc.gov are currently blocked by NYC DOE. Please try logging with Snap! or reach out to us to setup an alternate login method. Thanks!\n"
   end
 end
diff --git a/app/controllers/teachers_controller.rb b/app/controllers/teachers_controller.rb
@@ -24,15 +24,15 @@ class TeachersController < ApplicationController
   def index
     respond_to do |format|
       format.html {
-        @all_teachers = Teacher.where(admin: false)
-        @admins = Teacher.where(admin: true)
+        @all_teachers = Teacher.eager_load(:email_addresses, :school).where(admin: false)
+        @admins = Teacher.eager_load(:email_addresses, :school).where(admin: true)
       }
       format.csv { send_data Teacher.csv_export, filename: "teachers-#{Date.today}.csv" }
     end
   end
 
   def show
-    @all_teachers_except_current = Teacher.where.not(id: @teacher.id)
+    @all_teachers_except_current = Teacher.preload(:email_addresses, :school).where.not(id: @teacher.id)
     @school = @teacher.school
     @status = is_admin? ? "Admin" : "Teacher"
   end

diff --git a/app/helpers/teacher_helper.rb b/app/helpers/teacher_helper.rb
@@ -11,4 +11,9 @@ def ip_history_display(teacher)
     return "-" if teacher.ip_history.nil? || teacher.ip_history.empty?
     teacher.ip_history.to_sentence
   end
+
+  def email_address_label(email)
+    return unless email.primary?
+    '&nbsp; <span class="badge badge-pill badge-primary h6">primary</span>'.html_safe
+  end
 end
diff --git a/app/views/teachers/_form.html.erb b/app/views/teachers/_form.html.erb
@@ -102,6 +102,7 @@ status ONLY IF the person viewing this page is an admin. %>
     </div>
   </div>
 <% else %>
+
   <!-- Note that edit teacher page CANNOT allow direct adding/removing
   of files because the nested forms result in undefined
   behavior in Rails, including random controller
@@ -118,19 +119,17 @@ status ONLY IF the person viewing this page is an admin. %>
   </div>
 <% end %>
   <div class='form-group row'>
-    <div class='col-12'>
-      <%= f.label :education_level, "What's your education level target?", class: "label-required" %>
+    <div class='col-6'>
+      <%= f.label :education_level, "What grades to you teach?", class: "label-required" %>
       <%= f.select(
         :education_level,
         options_for_select(Teacher.education_level_options, teacher.education_level_before_type_cast),
         { include_blank: "Select an option" },
         { class: 'form-control', required: false }
       ) %>
     </div>
-  </div>
 
-  <div class='form-group row'>
-    <div class='col-12'>
+    <div class='col-6'>
       <%= f.label :languages, "What language(s) are spoken in the classroom?",
       class: "label-required" %>
       <%= f.select(

diff --git a/app/views/teachers/_teacher.erb b/app/views/teachers/_teacher.erb
@@ -10,10 +10,10 @@
   <%= link_to('(Web)', teacher.personal_website, target: '_blank') if teacher.personal_website.present? %>
 </td>
 <td>
-  <%= teacher.primary_email %>
-  <% if teacher.personal_emails.present? %>
-    <% teacher.personal_emails.each do |email| %>
-      <br><%= email %>
+  <% teacher.email_addresses.each do |email| %>
+    <%= email.email %><%= email_address_label(email) %>
+    <% unless email == teacher.email_addresses.last %>
+      <br>
     <% end %>
   <% end %>
 </td>

diff --git a/app/views/teachers/_teacher_info.html.erb b/app/views/teachers/_teacher_info.html.erb
@@ -24,34 +24,25 @@
   <div class="card-body">
     <% if @is_admin %>
       <div class="row mb-4">
-        <div class="col-sm-4 font-weight-bold">Teacher ID:</div>
-        <div class="col-sm-8">
+        <div class="col-sm-3 font-weight-bold">Teacher ID:</div>
+        <div class="col-sm-9">
           <%= teacher.id %>
         </div>
       </div>
     <% end %>
-    <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Email:</div>
-      <div class="col-sm-6">
-        <a href=<%= "mailto:#{teacher.primary_email}" %>> <%= teacher.primary_email %> </a>
-      </div>
-      <div class="col-sm-2">
-        <button class="btn btn-sm" onclick="copyToClipboard('<%= teacher.primary_email %>')"><i class="fa fa-copy"></i></button>
-      </div>
+    <div class="row mb-3">
+      <div class="col-sm-3 font-weight-bold">Emails:</div>
+        <ul class="list-unstyled col-sm-9">
+          <% teacher.email_addresses.each do |email| %>
+            <li class="">
+              <a href="mailto:<%= email.email %>" class="mr-2"><%= email.email %></a>
+              <button class="btn btn-sm" onclick="copyToClipboard('<%= email.email %>')"><i class="fa fa-copy"></i></button>
+              <%= email_address_label(email) %>
+            </li>
+        <% end %></ul>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Personal Emails:</div>
-      <div class="col-sm-6">
-        <% teacher.personal_emails.each do |email| %>
-          <div class="d-flex align-items-center mb-2">
-            <a href="mailto:<%= email %>" class="mr-2"><%= email %></a>
-            <button class="btn btn-sm" onclick="copyToClipboard('<%= email %>')"><i class="fa fa-copy"></i></button>
-          </div>
-        <% end %>
-      </div>
-    </div>
-    <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Snap<i>!</i> Username:</div>
+      <div class="col-sm-3 font-weight-bold">Snap<i>!</i> Username:</div>
       <div class="col-sm-6">
         <%= snap_link(teacher) %>
       </div>
@@ -60,20 +51,20 @@
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Application Status:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Application Status:</div>
+      <div class="col-sm-9">
         <%= teacher.display_application_status %>
       </div>
     </div>
     <% if teacher.status_before_type_cast == 9 %>
       <div class="row mb-4">
-        <div class="col-sm-4 font-weight-bold">Supporting Files:</div>
+        <div class="col-sm-3 font-weight-bold">Supporting Files:</div>
           <%= render 'files_display', teacher: @teacher, show_add_file: true, show_delete_file: true %>
-      </div>  
+      </div>
     <% end %>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Personal or Course Website:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Personal or Course Website:</div>
+      <div class="col-sm-9">
         <% if teacher.personal_website.blank? %>
           None provided
         <% else %>
@@ -82,55 +73,55 @@
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Education Level Target:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Grade Level:</div>
+      <div class="col-sm-9">
         <%= teacher.display_education_level %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Classroom Language(s):</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Classroom Language(s):</div>
+      <div class="col-sm-9">
         <%= teacher.display_languages %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Current Status:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Current Status:</div>
+      <div class="col-sm-9">
         <%= teacher.display_status %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">School:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">School:</div>
+      <div class="col-sm-9">
         <%= link_to(teacher.school.name, school_path(teacher.school)) %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">School Location:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">School Location:</div>
+      <div class="col-sm-9">
       </div>
     </div>
     <div class="row">
-      <div class="col-sm-4 font-weight-bold">Sign Up Date:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Sign Up Date:</div>
+      <div class="col-sm-9">
         <%= teacher.created_at %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">Session Count:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Session Count:</div>
+      <div class="col-sm-9">
         <%= teacher.session_count %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-4 font-weight-bold">IP History:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">IP History:</div>
+      <div class="col-sm-9">
         <%= ip_history_display(teacher) %>
       </div>
     </div>
     <div class="row">
-      <div class="col-sm-4 font-weight-bold">Last Session:</div>
-      <div class="col-sm-8">
+      <div class="col-sm-3 font-weight-bold">Last Session:</div>
+      <div class="col-sm-9">
         <%= teacher.last_session_at || "-" %>
       </div>
     </div>

diff --git a/app/views/teachers/show.html.erb b/app/views/teachers/show.html.erb
@@ -38,16 +38,9 @@
 
 <script>
   $(document).ready(function() {
-      var modal = $('#merge-modal');
-      var button = $('#merge-button');
+      let modal = $('#merge-modal');
 
-      button.click(function() {
-        modal.show();
-      });
-
-      // Close the modal when the close button is clicked
-      $('.close').click(function() {
-        modal.hide();
-      });
+      $('#merge-button').click(() => modal.show());
+      $('.close').click(() => modal.hide());
   });
-</script>
+</script>
diff --git a/features/step_definitions/form_steps.rb b/features/step_definitions/form_steps.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 STATUS_FIELD = "What's your current status?"
-EDUCATION_FIELD = "What's your education level target?"
+EDUCATION_FIELD = "What grades to you teach?"
 
 Given(/a valid teacher exists/) do
   school = create(:school)
@@ -26,7 +26,7 @@
 
 # assumes that languages dropdown is the FIRST selectize menu to appear on the page
 When(/^I select "(.*?)" from the languages dropdown$/) do |option|
-  first(".selectize-input").click  # Click on the dropdown to open it
+  first(".selectize-input").click # Click on the dropdown to open it
   find(".selectize-dropdown-content .option", text: option).click  # Click on the desired option
 end