Refactor Teacher#edit and #update

app/controllers/teachers_controller.rb

@@ -46,18 +46,10 @@ def new
     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])
-    if @teacher && defined?(current_user.id) && (current_user.id == @teacher.id)
-      params[:id] = current_user.id
-      update
-      return
-    elsif @teacher
+    if Teacher.find_by(email: params[:email])
       redirect_to login_path,
-                  notice: "You already have signed up with '#{@teacher.email}'. Please log in."
+                  notice: "You already have signed up with '#{params[:email]}'. Please log in."
       return
     end
 
@@ -80,7 +72,7 @@ def create
       TeacherMailer.form_submission(@teacher).deliver_now
       redirect_to root_path
     else
-      redirect_to new_teacher_path, alert: "An error occurred while trying to save. #{@teacher.errors.full_messages}"
+      render :new, alert: "An error occurred while trying to save. #{@teacher.errors.full_messages}"
     end
   end
 
@@ -92,6 +84,15 @@ def edit
   end
 
   def update
+    if @teacher.denied? && !is_admin?
+      redirect_to root_path, alert: "Failed to update your information. You have already been denied. If you have questions, please email [email protected]."
+      return
+    elsif @teacher.id != current_user.id && !is_admin?
+      Sentry.capture_message("BAD UPDATE: #{current_user.id} attempted to edit #{@teacher.id}")
+      redirect_to root_path, alert: "You are attempting to update another user's record."
+      return
+    end
+
     load_school
     ordered_schools
     @teacher.assign_attributes(teacher_params)
@@ -102,11 +103,7 @@ def update
       @school.save!
       @teacher.school = @school
     end
-    if @teacher.denied? && !is_admin?
-      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?
+    if (@teacher.admin_attributes_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]."
       return
     end
@@ -115,16 +112,17 @@ def update
                 alert: "Failed to update data. #{@teacher.errors.full_messages.to_sentence}"
       return
     end
+
     if [email protected]? && !current_user.admin?
+      @teacher.not_reviewed!
       TeacherMailer.form_submission(@teacher).deliver_now
     end
+
     if is_admin?
-      redirect_to teachers_path, notice: "Saved #{@teacher.full_name}"
-      return
+      redirect_to teacher_path(@teacher), notice: "Saved #{@teacher.full_name}"
     else
-      @teacher.try_append_ip(request.remote_ip)
+      redirect_to root_path, notice: "Saved successfully. Thanks!"
     end
-    redirect_to edit_teacher_path(current_user.id), notice: "Successfully updated your information"
   end
 
   def request_info
@@ -141,7 +139,6 @@ def validate
     redirect_to root_path
   end
 
-  # TODO: Handle the more info / intermediate status route.
   def deny
     @teacher.denied!
     if !params[:skip_email].present?

app/models/teacher.rb

@@ -93,14 +93,10 @@ class Teacher < ApplicationRecord
     "I am teaching Middle School BJC.",
   ].freeze
 
-  before_update :reset_validation_status
+  delegate :name, :location, :grade_level, :website, to: :school, prefix: true
+  delegate :school_type, to: :school # don't add a redundant prefix.
 
-  def reset_validation_status
-    return if application_status_changed? || school_id_changed?
-    if info_needed?
-      not_reviewed!
-    end
-  end
+  before_update :reset_validation_status
 
   def full_name
     "#{first_name} #{last_name}"
@@ -116,6 +112,10 @@ def snap_username
     self.snap
   end
 
+  def admin_attributes_changed?
+    self.email_changed? || self.personal_email_changed? || self.snap_changed?
+  end
+
   def status=(value)
     value = value.to_i if value.is_a?(String)
     super(value)
@@ -216,8 +216,7 @@ def email_attributes
     }
   end
 
-  delegate :name, :location, :grade_level, :website, to: :school, prefix: true
-  delegate :school_type, to: :school # don't add a redundant prefix.
+  # TODO: Move this to a TeacherCSVExports lib file
   # TODO: The school data needs to be cleaned up.
   def self.csv_export
     attributes = %w|