Revert "Improve Teaching Merging + Email Management "

beautyjoy · Jun 30, 2024 · 23aa384 · 23aa384
1 parent 49575e3
commit 23aa384
Show file tree

Hide file tree

Showing 11 changed files with 86 additions and 103 deletions.
diff --git a/app/controllers/merge_controller.rb b/app/controllers/merge_controller.rb
@@ -15,16 +15,12 @@ def execute
     @merged_teacher = merge_teachers(@from_teacher, @into_teacher)
 
     merged_attributes = @merged_teacher.attributes.except("id")
-    Teacher.transaction do
-      merge_email_addresses(@from_teacher, @into_teacher)
-      @from_teacher.destroy
-      @into_teacher.update!(merged_attributes)
-    end
+    @into_teacher.update!(merged_attributes)
+    @from_teacher.destroy
     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)
@@ -54,26 +50,6 @@ 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
@@ -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. #{nyc_message}"
+      flash[:alert] = "We couldn't find an account for #{omniauth_info.email}. Please submit a new request."
       redirect_to new_teacher_path
     end
   end
@@ -52,14 +52,6 @@ def omniauth_failure
     Sentry.add_breadcrumb(crumb)
     Sentry.capture_message("Omniauth Failure")
     redirect_to root_url,
-                alert: "Login failed unexpectedly. Please reach out to [email protected] #{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"
+                alert: "Login failed unexpectedly. Please reach out to [email protected] (#{params[:message]})"
   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.eager_load(:email_addresses, :school).where(admin: false)
-        @admins = Teacher.eager_load(:email_addresses, :school).where(admin: true)
+        @all_teachers = Teacher.where(admin: false)
+        @admins = Teacher.where(admin: true)
       }
       format.csv { send_data Teacher.csv_export, filename: "teachers-#{Date.today}.csv" }
     end
   end
 
   def show
-    @all_teachers_except_current = Teacher.preload(:email_addresses, :school).where.not(id: @teacher.id)
+    @all_teachers_except_current = Teacher.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,9 +11,4 @@ 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/models/teacher.rb b/app/models/teacher.rb
@@ -228,14 +228,7 @@ def short_application_status
   end
 
   def self.user_from_omniauth(omniauth)
-    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
+    EmailAddress.find_by(email: omniauth.email.downcase)&.teacher
   end
 
   def try_append_ip(ip)

diff --git a/app/views/teachers/_form.html.erb b/app/views/teachers/_form.html.erb
@@ -102,7 +102,6 @@ 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
@@ -119,17 +118,19 @@ status ONLY IF the person viewing this page is an admin. %>
   </div>
 <% end %>
   <div class='form-group row'>
-    <div class='col-6'>
-      <%= f.label :education_level, "What grades to you teach?", class: "label-required" %>
+    <div class='col-12'>
+      <%= f.label :education_level, "What's your education level target?", 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='col-6'>
+  <div class='form-group row'>
+    <div class='col-12'>
       <%= 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.email_addresses.each do |email| %>
-    <%= email.email %><%= email_address_label(email) %>
-    <% unless email == teacher.email_addresses.last %>
-      <br>
+  <%= teacher.primary_email %>
+  <% if teacher.personal_emails.present? %>
+    <% teacher.personal_emails.each do |email| %>
+      <br><%= email %>
     <% end %>
   <% end %>
 </td>

diff --git a/app/views/teachers/_teacher_info.html.erb b/app/views/teachers/_teacher_info.html.erb
@@ -24,25 +24,34 @@
   <div class="card-body">
     <% if @is_admin %>
       <div class="row mb-4">
-        <div class="col-sm-3 font-weight-bold">Teacher ID:</div>
-        <div class="col-sm-9">
+        <div class="col-sm-4 font-weight-bold">Teacher ID:</div>
+        <div class="col-sm-8">
           <%= teacher.id %>
         </div>
       </div>
     <% end %>
-    <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 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>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">Snap<i>!</i> Username:</div>
+      <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-6">
         <%= snap_link(teacher) %>
       </div>
@@ -51,20 +60,20 @@
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">Application Status:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Application Status:</div>
+      <div class="col-sm-8">
         <%= teacher.display_application_status %>
       </div>
     </div>
     <% if teacher.status_before_type_cast == 9 %>
       <div class="row mb-4">
-        <div class="col-sm-3 font-weight-bold">Supporting Files:</div>
+        <div class="col-sm-4 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-3 font-weight-bold">Personal or Course Website:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Personal or Course Website:</div>
+      <div class="col-sm-8">
         <% if teacher.personal_website.blank? %>
           None provided
         <% else %>
@@ -73,55 +82,55 @@
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">Grade Level:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Education Level Target:</div>
+      <div class="col-sm-8">
         <%= teacher.display_education_level %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">Classroom Language(s):</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Classroom Language(s):</div>
+      <div class="col-sm-8">
         <%= teacher.display_languages %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">Current Status:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Current Status:</div>
+      <div class="col-sm-8">
         <%= teacher.display_status %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">School:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">School:</div>
+      <div class="col-sm-8">
         <%= link_to(teacher.school.name, school_path(teacher.school)) %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">School Location:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">School Location:</div>
+      <div class="col-sm-8">
       </div>
     </div>
     <div class="row">
-      <div class="col-sm-3 font-weight-bold">Sign Up Date:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Sign Up Date:</div>
+      <div class="col-sm-8">
         <%= teacher.created_at %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">Session Count:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Session Count:</div>
+      <div class="col-sm-8">
         <%= teacher.session_count %>
       </div>
     </div>
     <div class="row mb-4">
-      <div class="col-sm-3 font-weight-bold">IP History:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">IP History:</div>
+      <div class="col-sm-8">
         <%= ip_history_display(teacher) %>
       </div>
     </div>
     <div class="row">
-      <div class="col-sm-3 font-weight-bold">Last Session:</div>
-      <div class="col-sm-9">
+      <div class="col-sm-4 font-weight-bold">Last Session:</div>
+      <div class="col-sm-8">
         <%= teacher.last_session_at || "-" %>
       </div>
     </div>

diff --git a/app/views/teachers/show.html.erb b/app/views/teachers/show.html.erb
@@ -38,9 +38,16 @@
 
 <script>
   $(document).ready(function() {
-      let modal = $('#merge-modal');
+      var modal = $('#merge-modal');
+      var button = $('#merge-button');
 
-      $('#merge-button').click(() => modal.show());
-      $('.close').click(() => modal.hide());
+      button.click(function() {
+        modal.show();
+      });
+
+      // Close the modal when the close button is clicked
+      $('.close').click(function() {
+        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 grades to you teach?"
+EDUCATION_FIELD = "What's your education level target?"
 
 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
 

diff --git a/spec/controllers/teachers_controller_spec.rb b/spec/controllers/teachers_controller_spec.rb
@@ -237,11 +237,21 @@
       allow(teacher).to receive(:school).and_return(school)
     end
 
+    it "assigns school and teacher status" do
+      allow(controller).to receive(:is_admin?).and_return(false)
+      get :show, params: { id: 1 }
+      expect(assigns(:school)).to eq(school)
+      expect(assigns(:status)).to eq("Teacher")
+      expect(assigns(:all_teachers_except_current)).to eq(other_teacher)
+      expect(response).to render_template("show")
+    end
+
     it "assigns admin if teacher is admin" do
       allow(controller).to receive(:is_admin?).and_return(true)
       get :show, params: { id: 1 }
       expect(assigns(:school)).to eq(school)
       expect(assigns(:status)).to eq("Admin")
+      expect(assigns(:all_teachers_except_current)).to eq(other_teacher)
       expect(response).to render_template("show")
     end
   end