187384622/new email model - PR1 - Change Email and Make Sure original tests didn't break

app/controllers/application_controller.rb

@@ -51,6 +51,6 @@ def check_teacher_admin
   end
 
   def set_sentry_user
-    Sentry.set_user(id: session[:user_id], email: current_user&.email)
+    Sentry.set_user(id: session[:user_id], email: current_user&.primary_email)
   end
 end

app/controllers/teachers_controller.rb

@@ -42,22 +42,23 @@ def new
     @school = @teacher.school # maybe delegate this
     @readonly = false
     if omniauth_data.present?
+      email_data = { email: omniauth_data.delete("email"), primary: true }
       @teacher.assign_attributes(omniauth_data)
+      @teacher.email_addresses.build(email_data)
     end
   end
 
   # TODO: This needs to be re-written.
   # If you are logged in and not an admin, this should fail.
   def create
-    # Find by email, but allow updating other info.
-    @teacher = Teacher.find_by(email: teacher_params[:email])
+    @teacher = EmailAddress.find_by(email: params.dig(:email, :primary))&.teacher
     if @teacher && defined?(current_user.id) && (current_user.id == @teacher.id)
       params[:id] = current_user.id
       update
       return
     elsif @teacher
       redirect_to login_path,
-                  notice: "You already have signed up with '#{@teacher.email}'. Please log in."
+                  notice: "You already have signed up with '#{@teacher.primary_email}'. Please log in."
       return
     end
 
@@ -71,12 +72,14 @@ def create
     end
 
     @teacher = Teacher.new(teacher_params)
+    @teacher.email_addresses.build(email: params[:email][:primary], primary: true)
+
     @teacher.try_append_ip(request.remote_ip)
     @teacher.session_count += 1
     @teacher.school = @school
     if @teacher.save
       @teacher.not_reviewed!
-      flash[:success] = "Thanks for signing up for BJC, #{@teacher.first_name}! You'll hear from us shortly. Your email address is: #{@teacher.email}."
+      flash[:success] = "Thanks for signing up for BJC, #{@teacher.first_name}! You'll hear from us shortly. Your email address is: #{@teacher.primary_email}."
       TeacherMailer.form_submission(@teacher).deliver_now
       TeacherMailer.teacher_form_submission(@teacher).deliver_now
       redirect_to root_path
@@ -95,7 +98,15 @@ def edit
   def update
     load_school
     ordered_schools
+
+    primary_email = params.dig(:email, :primary)
+    personal_emails = params[:email]&.select { |key, value| key.start_with?("personal_") }&.values
+
+    # Now, `params[:teacher]` does not contain primary_email or any personal_emailX fields
     @teacher.assign_attributes(teacher_params)
+
+    update_primary_email(primary_email)
+    update_personal_emails(personal_emails)
     if teacher_params[:school_id].present?
       @teacher.school = @school
     else
@@ -111,26 +122,27 @@ def update
       redirect_to root_path, alert: "Failed to update your information. You have already been denied. If you have questions, please email [email protected]."
       return
     end
-    if (@teacher.email_changed? || @teacher.snap_changed?) && !is_admin?
-      redirect_to edit_teacher_path(current_user.id), alert: "Failed to update your information. If you want to change your email or Snap! username, please email [email protected]."
+    if (@teacher.email_changed_flag || @teacher.snap_changed?) && !is_admin?
+      @teacher.email_changed_flag = false
+      redirect_to edit_teacher_path(params[:id]), alert: "Failed to update your information. If you want to change your email or Snap! username, please email [email protected]."
       return
     end
-    if !@teacher.save
-      redirect_to edit_teacher_path(current_user.id),
-                alert: "An error occurred: #{@teacher.errors.full_messages.join(', ')}"
+    unless @teacher.save
+      redirect_to edit_teacher_path(params[:id]),
+                  alert: "An error occurred: #{@teacher.errors.full_messages.join(', ')}"
       return
     end
     if [email protected]? && !current_user.admin?
       TeacherMailer.form_submission(@teacher).deliver_now
       TeacherMailer.teacher_form_submission(@teacher).deliver_now
     end
     if is_admin?
-      redirect_to edit_teacher_path(current_user.id), notice: "Saved #{@teacher.full_name}"
+      redirect_to edit_teacher_path(params[:id]), notice: "Saved #{@teacher.full_name}"
       return
     else
       @teacher.try_append_ip(request.remote_ip)
     end
-    redirect_to edit_teacher_path(current_user.id), notice: "Successfully updated your information"
+    redirect_to edit_teacher_path(params[:id]), notice: "Successfully updated your information"
   end
 
   def send_email_if_application_status_changed_and_email_resend_enabled
@@ -209,13 +221,12 @@ def load_school
   end
 
   def teacher_params
-    teacher_attributes = [:first_name, :last_name, :school, :email, :status, :snap,
-      :more_info, :personal_website, :education_level, :school_id]
-    if is_admin?
-      teacher_attributes << [:personal_email, :application_status,
-      :request_reason, :skip_email]
-    end
-    params.require(:teacher).permit(*teacher_attributes, languages: [])
+    teacher_attributes = [:first_name, :last_name, :school, :status, :snap,
+                          :more_info, :personal_website, :education_level, :school_id, languages: []]
+    admin_attributes = [:application_status, :request_reason, :skip_email]
+    teacher_attributes.push(*admin_attributes) if is_admin?
+
+    params.require(:teacher).permit(*teacher_attributes)
   end
 
   def school_params
@@ -230,7 +241,7 @@ def omniauth_data
   def ordered_schools
     if params[:id].present?
       load_teacher
-      @ordered_schools ||= [ @teacher.school ] +
+      @ordered_schools ||= [@teacher.school] +
         School.all.order(:name).reject { |s| s.id == @teacher.school_id }
     else
       @ordered_schools ||= School.all.order(:name)
@@ -260,4 +271,30 @@ def sanitize_params
   def load_pages
     @pages ||= Page.where(viewer_permissions: Page.viewable_pages(current_user))
   end
+
+  def update_primary_email(primary_email)
+    return unless primary_email.present?
+
+    # First, ensure the current primary email is marked as not primary if it's not the same as the new one
+    @teacher.email_addresses.find_by(primary: true)&.update(primary: false)
+
+    primary_email_record = @teacher.email_addresses.find_or_initialize_by(email: primary_email)
+    primary_email_record.primary = true
+
+    primary_email_record.save if primary_email_record.changed?
+  end
+
+  def update_personal_emails(personal_emails)
+    return unless personal_emails.present?
+    personal_emails = personal_emails.reject(&:empty?)
+    return if personal_emails.empty?
+
+    current_emails = @teacher.email_addresses.pluck(:email)
+
+    new_emails = personal_emails - current_emails
+
+    new_emails.each do |email|
+      @teacher.email_addresses.build(email:, primary: false)
+    end
+  end
 end

app/models/email_address.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+# == Schema Information
+#
+# Table name: email_addresses
+#
+#  id         :bigint           not null, primary key
+#  email      :string           not null
+#  primary    :boolean          default(FALSE), not null
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#  teacher_id :bigint           not null
+#
+# Indexes
+#
+#  index_email_addresses_on_email                   (email) UNIQUE
+#  index_email_addresses_on_teacher_id              (teacher_id)
+#  index_email_addresses_on_teacher_id_and_primary  (teacher_id,primary) UNIQUE WHERE ("primary" = true)
+#
+# Foreign Keys
+#
+#  fk_rails_...  (teacher_id => teachers.id)
+#
+class EmailAddress < ApplicationRecord
+  belongs_to :teacher
+
+  # Rail's bulit-in validation for email format regex
+  validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP }
+  validate :only_one_primary_email_per_teacher
+
+  before_save :normalize_email
+  before_save :flag_teacher_if_email_changed
+
+  private
+  def only_one_primary_email_per_teacher
+    if primary? && EmailAddress.where(teacher_id:, primary: true).where.not(id:).exists?
+      errors.add(:primary, "There can only be one primary email per teacher.")
+    end
+  end
+
+  def normalize_email
+    self.email = email.strip.downcase
+  end
+
+  def flag_teacher_if_email_changed
+    if self.email_changed? && !self.new_record?
+      teacher.email_changed_flag = true
+      teacher.handle_relevant_changes
+    end
+  end
+end

app/models/teacher.rb

@@ -40,13 +40,16 @@
 class Teacher < ApplicationRecord
   WORLD_LANGUAGES = [ "Afrikaans", "Albanian", "Arabic", "Armenian", "Basque", "Bengali", "Bulgarian", "Catalan", "Cambodian", "Chinese (Mandarin)", "Croatian", "Czech", "Danish", "Dutch", "English", "Estonian", "Fiji", "Finnish", "French", "Georgian", "German", "Greek", "Gujarati", "Hebrew", "Hindi", "Hungarian", "Icelandic", "Indonesian", "Irish", "Italian", "Japanese", "Javanese", "Korean", "Latin", "Latvian", "Lithuanian", "Macedonian", "Malay", "Malayalam", "Maltese", "Maori", "Marathi", "Mongolian", "Nepali", "Norwegian", "Persian", "Polish", "Portuguese", "Punjabi", "Quechua", "Romanian", "Russian", "Samoan", "Serbian", "Slovak", "Slovenian", "Spanish", "Swahili", "Swedish ", "Tamil", "Tatar", "Telugu", "Thai", "Tibetan", "Tonga", "Turkish", "Ukrainian", "Urdu", "Uzbek", "Vietnamese", "Welsh", "Xhosa" ].freeze
 
-  validates :first_name, :last_name, :email, :status, presence: true
-  validates :email, uniqueness: true
-  validates :personal_email, uniqueness: true, if: -> { personal_email.present? }
-  validate :ensure_unique_personal_email, if: -> { email_changed? || personal_email_changed? }
+  has_many :email_addresses, dependent: :destroy
+  accepts_nested_attributes_for :email_addresses, allow_destroy: true
+
+  validates :first_name, :last_name, :status, presence: true
   validate :valid_languages
   before_validation :sort_and_clean_languages
 
+  # Custom attribute for tracking email changes. This is not persisted in the database.
+  attribute :email_changed_flag, :boolean, default: false
+
   enum application_status: {
     validated: "Validated",
     denied: "Denied",
@@ -127,7 +130,7 @@ def full_name
 
   def email_name
     # We need to normalize names for emails.
-    "#{full_name} <#{email}>".delete(",")
+    "#{full_name} <#{primary_email}>".delete(",")
   end
 
   def snap_username
@@ -218,15 +221,8 @@ def short_application_status
   end
 
   def self.user_from_omniauth(omniauth)
-    teachers = Teacher.where("LOWER(email) = :email or LOWER(personal_email) = :email",
-      email: omniauth.email.downcase)
-    if teachers.length > 1
-      raise "Too Many Teachers Found"
-    elsif teachers.length == 1
-      teachers.first
-    else
-      nil
-    end
+    teacher = EmailAddress.find_by(email: omniauth.email.downcase)&.teacher
+    teacher
   end
 
   def try_append_ip(ip)
@@ -241,8 +237,9 @@ def email_attributes
       teacher_first_name: self.first_name,
       teacher_last_name: self.last_name,
       teacher_full_name: self.full_name,
-      teacher_email: self.email,
-      teacher_personal_email: self.personal_email,
+      teacher_email: self.primary_email,
+      # TODO: change to personal_emails
+      teacher_personal_email: self.non_primary_emails,
       teacher_more_info: self.more_info,
       teacher_snap: self.snap_username,
       teacher_snap_username: self.snap_username,
@@ -292,21 +289,25 @@ def self.csv_export
     end
   end
 
-  private
-  def ensure_unique_personal_email
-    # We want to ensure that both email and personal_email are unique across
-    # BOTH columns (i.e. the email only appears once in the combined set)
-    # However, it's perfectly valid for personal_email to be nil
-    # TODO: This really suggests email needs to be its own table...
-    teacher_with_email = Teacher.where.not(id: self.id).exists?(["email = :value OR personal_email = :value", { value: self.email }])
-    if teacher_with_email
-      errors.add(:email, "must be unique across both email and personal_email columns")
-    end
-    return unless self.personal_email.present?
+  def email
+    # Default return primary email
+    primary_email
+  end
 
-    teacher_personal_email = Teacher.where.not(id: self.id).exists?(["email = :value OR personal_email = :value", { value: self.personal_email }])
-    if teacher_personal_email
-      errors.add(:personal_email, "must be unique across both email and personal_email columns")
-    end
+  def primary_email
+    # ||:email this code is temporary for this PR: https://github.com/cs169/BJC-Teacher-Tracker-App/pull/49
+    # to make sure at least original data in db still work and passed the existing tests
+    self[:email] || email_addresses.find_by(primary: true)&.email
+  end
+
+  def personal_emails
+    non_primary_emails
+  end
+
+  private
+  def non_primary_emails
+    # email_addresses.where(primary: false)&.pluck(:email)
+    # below code is temporary for current PR, to make sure the frontend same as before (only one personal email)
+    personal_email || email_addresses.where(primary: false)&.pluck(:email)&.first
   end
 end

app/views/schools/show.html.erb

@@ -66,7 +66,7 @@
               <% @school.teachers.each do |teacher| %>
                 <tr>
                   <td><%= link_to(teacher.full_name, teacher_path(teacher)) %></td>
-                  <td><%= teacher.email %></td>
+                  <td><%= teacher.primary_email %></td>
                   <td><%= teacher.display_status %></td>
                   <td><%= teacher.display_application_status %></td>
                   <td><%= teacher.created_at.strftime("%b %d, %Y") %></td>

app/views/teachers/_form.html.erb

@@ -57,9 +57,9 @@ status ONLY IF the person viewing this page is an admin. %>
 
   <div class='form-group row'>
     <div class='col-6'>
-      <%= f.label :email, "School Email", class: "label-required" %>
-      <%= f.text_field :email, placeholder: '[email protected]', class: 'form-control',
-        required: true, type: :email, readonly: @readonly %>
+      <%= label_tag 'email[primary]', "School Email", class: "label-required" %>
+      <%= text_field_tag 'email[primary]', @teacher.primary_email, placeholder: '[email protected]',
+                         class: 'form-control', required: true, type: :email, readonly: @readonly %>
     </div>
 
     <div class='col-6'>
@@ -81,13 +81,14 @@ status ONLY IF the person viewing this page is an admin. %>
   </div>
 
   <%# For now... only admins can enter/edit personal emails. %>
-  <%- if current_user&.admin? || @teacher.personal_email.present? %>
+
+  <%- if current_user&.admin? || @teacher.personal_emails.present? %>
   <div class="form-group row">
     <div class='col-12'>
-      <%= f.label :personal_email do %>
+      <%= label_tag "email[personal_1]" do %>
         Personal Email</i>
       <% end %>
-      <%= f.text_field :personal_email, placeholder: '[email protected]', class: 'form-control',
+      <%= text_field_tag "email[personal_1]", @teacher.personal_emails, placeholder: '[email protected]', class: 'form-control',
         required: false, type: :email, readonly: @readonly %>
     </div>
   </div>

app/views/teachers/_teacher.erb

@@ -3,10 +3,11 @@
   <%= link_to('(Web)', teacher.personal_website, target: '_blank') if teacher.personal_website.present? %>
 </td>
 <td>
-  <%= teacher.email %>
-  <%- if teacher.personal_email.present? %>
-    <br><%= teacher.personal_email %>
-  <%- end %>
+  <%= teacher.primary_email %>
+<!--  Left our from current PR. Personal Emails display will be fixed at later PRs-->
+  <%#- if teacher.personal_emails.present? %>
+<!--    <br><%#= teacher.personal_emails %>-->
+  <%#- end %>
 </td>
 <td><%= teacher.display_education_level %></td>
 <td

app/views/teachers/_teacher_info.html.erb

@@ -10,20 +10,20 @@
     <div class="row mb-4">
       <div class="col-sm-4 font-weight-bold">Email:</div>
       <div class="col-sm-6">
-        <a href=<%= "mailto:#{teacher.email}" %>> <%= teacher.email %> </a>
+        <a href=<%= "mailto:#{teacher.primary_email}" %>> <%= teacher.primary_email %> </a>
       </div>
       <div class="col-sm-2">
-        <button class="btn btn-sm" onclick="copyToClipboard('<%= teacher.email %>')"><i class="fa fa-copy"></i></button>
+        <button class="btn btn-sm" onclick="copyToClipboard('<%= teacher.primary_email %>')"><i class="fa fa-copy"></i></button>
       </div>
     </div>
     <div class="row mb-4">
       <div class="col-sm-4 font-weight-bold">Personal Email:</div>
       <div class="col-sm-6">
-        <a href=<%= "mailto:#{teacher.personal_email}" %>
-          ><%= teacher.personal_email %></a>
+        <a href=<%= "mailto:#{teacher.personal_emails}" %>
+          ><%= teacher.personal_emails %></a>
       </div>
       <div class="col-sm-2">
-        <button class="btn btn-sm" onclick="copyToClipboard('<%= teacher.personal_email %>')"
+        <button class="btn btn-sm" onclick="copyToClipboard('<%= teacher.personal_emails %>')"
           ><i class="fa fa-copy"></i>
         </button>
       </div>