From 55b687231218f146ed63b64ebf702d3e9f857b09 Mon Sep 17 00:00:00 2001 From: Michael Ball Date: Fri, 28 Jun 2024 17:56:34 -0700 Subject: [PATCH] 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 --- app/controllers/merge_controller.rb | 12 ++-- app/controllers/sessions_controller.rb | 4 +- app/controllers/teachers_controller.rb | 6 +- app/helpers/teacher_helper.rb | 5 ++ app/views/teachers/_form.html.erb | 9 ++- app/views/teachers/_teacher.erb | 8 +-- app/views/teachers/_teacher_info.html.erb | 83 ++++++++++------------- app/views/teachers/show.html.erb | 15 ++-- features/step_definitions/form_steps.rb | 4 +- 9 files changed, 67 insertions(+), 79 deletions(-) diff --git a/app/controllers/merge_controller.rb b/app/controllers/merge_controller.rb index 1c0baff5..0c6e9676 100644 --- 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 index bb02effa..607f1402 100644 --- 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 index aa83352d..ec672eef 100644 --- 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 index 617520c5..122b159b 100644 --- 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? + '  primary'.html_safe + end end diff --git a/app/views/teachers/_form.html.erb b/app/views/teachers/_form.html.erb index 683e9629..014a7d7b 100644 --- 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. %> <% else %> +