diff --git a/app/controllers/merge_controller.rb b/app/controllers/merge_controller.rb index d860662d..0c6e9676 100644 --- a/app/controllers/merge_controller.rb +++ b/app/controllers/merge_controller.rb @@ -15,12 +15,16 @@ def execute @merged_teacher = merge_teachers(@from_teacher, @into_teacher) merged_attributes = @merged_teacher.attributes.except("id") - @into_teacher.update!(merged_attributes) - @from_teacher.destroy + Teacher.transaction do + merge_email_addresses(@from_teacher, @into_teacher) + @from_teacher.destroy + @into_teacher.update!(merged_attributes) + end redirect_to teachers_path, notice: "Teachers merged successfully." end private + # TODO: Migrate to a library/service method # Returns a merged teacher without saving it to the database. # Rendered in the preview, and only saved to the DB in a call to merge. def merge_teachers(from_teacher, into_teacher) @@ -50,6 +54,26 @@ def merge_teachers(from_teacher, into_teacher) end 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.email_addresses.update_all(primary: false) + end + + from_teacher.email_addresses.each do |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) + end + end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index ef4a6440..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 @@ -52,6 +52,14 @@ def omniauth_failure Sentry.add_breadcrumb(crumb) Sentry.capture_message("Omniauth Failure") redirect_to root_url, - alert: "Login failed unexpectedly. Please reach out to contact@bjc.berkeley.edu (#{params[:message]})" + alert: "Login failed unexpectedly. Please reach out to contact@bjc.berkeley.edu #{nyc_message} (#{params[:message]})" + end + + private + # Special warning for emails that end with @schools.nyc.gov + def nyc_message + return "" unless omniauth_info.email.downcase.ends_with?("@schools.nyc.gov") + + "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/models/teacher.rb b/app/models/teacher.rb index 7bc64e0a..0c9f9d70 100644 --- a/app/models/teacher.rb +++ b/app/models/teacher.rb @@ -228,7 +228,14 @@ def short_application_status end def self.user_from_omniauth(omniauth) - EmailAddress.find_by(email: omniauth.email.downcase)&.teacher + email = EmailAddress.find_by(email: omniauth.email.downcase) + + # We should handle this separately. + # Trim emails that end with email+snap-id-XXX@domain which come from the snap forum + if email.blank? && omniauth.email.match?(/\+snap-id-\d+/) + email = EmailAddress.find_by(email: omniauth.email.downcase.gsub(/\+snap-id-\d+@/, "@")) + end + email&.teacher end def try_append_ip(ip) 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 %> +