Merge pull request #329 from beautyjoy/bug-fixes

SU24 / Bug Fixes & Refactoring

.env.example

@@ -1,5 +1,4 @@
 attr_encrypted_key=[NEW ONE]
-BJC_PASSWORD=blank
 CLEVER_CLIENT_ID=IGNORE
 CLEVER_CLIENT_SECRET=IGNORE
 email_password=[NEW ONE]
@@ -23,4 +22,4 @@ SECRET_KEY_BASE=[NEW ONE]
 SENTRY_DSN=
 SNAP_CLIENT_SECRET='ignore'
 SNAP_CLIENT_URL='https://forum.snap.berkeley.edu/session/sso_provider'
-teacher_key=
+teacher_key=

.gitignore

@@ -29,8 +29,8 @@ config/master.key
 # config/secrets.yml
 
 # dotenv
-# TODO Comment out this rule if environment variables can be committed
 .env
+!.env.example
 
 ## Environment normalization:
 /.bundle
@@ -70,10 +70,6 @@ yarn-debug.log*
 /storage/*
 !/storage/.keep
 
-# Ignore postgress-related database files
-my_database_development
-my_database_test
-
 # A copy of GitHub's template .gitignore for all JetBrains IDEs.
 # It can be found here: https://github.com/github/gitignore/blob/master/Global/JetBrains.gitignore
 

app/controllers/sessions_controller.rb

@@ -33,6 +33,11 @@ def omniauth_callback
     end
   end
 
+  # Return more data in the case of errors.
+  def omniauth_data
+    request.env["omniauth.auth"]
+  end
+
   def omniauth_info
     request.env["omniauth.auth"].info
   end
@@ -41,7 +46,7 @@ def omniauth_failure
     crumb = Sentry::Breadcrumb.new(
       category: "auth",
       data: {
-        omniauth_env: request.env["omniauth.auth"],
+        omniauth_env: omniauth_data,
         omniauth_error: request.env["omniauth.error"],
         message: params[:message],
         strategy: params[:strategy]
@@ -58,7 +63,7 @@ def omniauth_failure
   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")
+    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

app/mailers/teacher_mailer.rb

@@ -37,6 +37,7 @@ def request_info_email(teacher, request_reason)
          subject: email_template.subject
   end
 
+  # This method is used to send the /admin/ an email message when a new user signs up.
   def form_submission(teacher)
     @teacher = teacher
     set_body
@@ -57,10 +58,10 @@ def teacher_form_submission(teacher)
   private
   def liquid_assigns
     base_rules = {
-      bjc_password: Rails.application.secrets[:bjc_password],
       piazza_password: Rails.application.secrets[:piazza_password],
       denial_reason: @denial_reason,
-      request_reason: @request_reason
+      request_reason: @request_reason,
+      request_info_reason: @request_reason
     }
     base_rules.merge!(@teacher.email_attributes)
     base_rules.with_indifferent_access
@@ -70,13 +71,18 @@ def email_template
     @email_template ||= EmailTemplate.find_by(title: action_name.titlecase)
   end
 
-  # renders the email body with the {{parameter}} things
+  # renders the email body with the {{parameter}} substitutions
+  # Must be called after @teacher is set.
   def set_body
     @body = Liquid::Template.parse(email_template.body).render(liquid_assigns).html_safe
   end
 
-  # renders the list of recipients with the {{parameter}} things
+  # renders the list of recipients with the {{parameter}} substitutions
+  # Must be called after @teacher is set.
   def set_recipients
     @recipients = Liquid::Template.parse(email_template.to).render(liquid_assigns).html_safe
+    @recipients = @recipients.split(",").
+        map { |addr| addr.strip }.
+        filter { |addr| addr != "(blank)" || addr.blank? }.compact
   end
 end

app/models/email_template.rb

@@ -28,8 +28,8 @@ class EmailTemplate < ApplicationRecord
   before_destroy :prevent_deleting_required_emails
 
   def accepts_custom_reason?
-    # search for 'reason' in a liquid template
-    body.match?(/{{\s*reason/)
+    # search for 'reason' in a liquid template, e.g. denial_reason or request_reason
+    body.match?(/\{\{.*_reason.*\}\}/)
   end
 
   private

app/models/teacher.rb

@@ -38,6 +38,7 @@
 #  fk_rails_...  (school_id => schools.id)
 #
 class Teacher < ApplicationRecord
+  # TODO: Move this somewhere else...
   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
 
   has_many :email_addresses, dependent: :destroy
@@ -66,12 +67,14 @@ class Teacher < ApplicationRecord
 
   # Non-admin teachers whose application has neither been accepted nor denied
   # It might or might not have been reviewed.
+  # TODO: Ensure these scopes do not override enum names.
   scope :unvalidated, -> { where("(application_status=? OR application_status=?) AND admin=?", application_statuses[:info_needed], application_statuses[:not_reviewed], "false") }
   scope :unreviewed, -> { where("application_status=? AND admin=?", application_statuses[:not_reviewed], "false") }
   # Non-admin teachers who have been accepted/validated
   scope :validated, -> { where("application_status=? AND admin=?", application_statuses[:validated], "false") }
 
 
+  # TODO: Remove this.
   enum education_level: {
     middle_school: 0,
     high_school: 1,
@@ -94,6 +97,7 @@ class Teacher < ApplicationRecord
   }
 
   # Always add to the bottom of the list!
+  # The text here may be changed, but the index maps to the actual reason (above)
   STATUSES = [
     "I am teaching BJC as an AP CS Principles course.",
     "I am teaching BJC but not as an AP CS Principles course.",
@@ -111,6 +115,9 @@ class Teacher < ApplicationRecord
   # that would require re-reviewing their application.
   before_update :check_for_relevant_changes
 
+  delegate :name, :location, :grade_level, :website, to: :school, prefix: true
+  delegate :school_type, to: :school # don't add a redundant prefix.
+
   def check_for_relevant_changes
     ignored_fields = %w[created_at updated_at session_count last_session_at ip_history]
     # if any relevant fields have changed
@@ -139,16 +146,45 @@ def email_name
     "#{full_name} <#{primary_email}>".delete(",")
   end
 
+  # TODO: Remove this pending migration to drop email column.
+  def email
+    # Default return primary email
+    primary_email
+  end
+
+  def primary_email
+    email_addresses.find_by(primary: true)&.email
+  end
+
+  def personal_emails
+    non_primary_emails
+  end
+
+  # TODO: use this method until we rename the column.
   def snap_username
-    # TODO: use this method until we rename the column.
     self.snap
   end
 
+  def try_append_ip(ip)
+    return if ip_history.include?(ip)
+    self.ip_history << ip
+    save
+  end
+
   def status=(value)
     value = value.to_i if value.is_a?(String)
     super(value)
   end
 
+  def text_status
+    STATUSES[status_before_type_cast]
+  end
+
+  def display_status
+    formatted_status = status.to_s.titlecase
+    return "#{formatted_status} | #{more_info}" if more_info?
+    formatted_status
+  end
   # TODO: Move these to helpers.
   def self.status_options
     display_order = [
@@ -170,18 +206,6 @@ def self.status_options
     end
   end
 
-  def self.application_status_options
-    Teacher.application_statuses.map { |sym, val| [sym.to_s.titlecase, val] }
-  end
-
-  def self.education_level_options
-    Teacher.education_levels.map { |sym, val| [sym.to_s.titlecase, val] }
-  end
-
-  def self.language_options
-    WORLD_LANGUAGES
-  end
-
   def display_languages
     languages.join(", ")
   end
@@ -195,6 +219,18 @@ def sort_and_clean_languages
     # To ensure data integrity, the following code removes any occurrences of empty strings from the list.
     languages.sort!.reject!(&:blank?)
   end
+  # TODO: Move to a helper.
+  def self.application_status_options
+    Teacher.application_statuses.map { |sym, val| [sym.to_s.titlecase, val] }
+  end
+
+  def self.education_level_options
+    Teacher.education_levels.map { |sym, val| [sym.to_s.titlecase, val] }
+  end
+
+  def self.language_options
+    WORLD_LANGUAGES
+  end
 
   def display_education_level
     if education_level_before_type_cast.to_i == -1
@@ -204,16 +240,6 @@ def display_education_level
     end
   end
 
-  def text_status
-    STATUSES[status_before_type_cast]
-  end
-
-  def display_status
-    formatted_status = status.to_s.titlecase
-    return "#{formatted_status} | #{more_info}" if more_info?
-    formatted_status
-  end
-
   def display_application_status
     application_status.titlecase
   end
@@ -238,37 +264,30 @@ def self.user_from_omniauth(omniauth)
     email&.teacher
   end
 
-  def try_append_ip(ip)
-    return if ip_history.include?(ip)
-    self.ip_history << ip
-    save
-  end
-
   def email_attributes
-    # Used when passing data to liquid templates
     {
       teacher_first_name: self.first_name,
       teacher_last_name: self.last_name,
       teacher_full_name: self.full_name,
+      teacher_primary_email: self.primary_email,
       teacher_email: self.primary_email,
       # TODO: change to personal_emails
-      teacher_personal_email: self.non_primary_emails,
+      teacher_personal_email: self.non_primary_emails.join(", "),
       teacher_more_info: self.more_info,
-      teacher_snap: self.snap_username,
       teacher_snap_username: self.snap_username,
       teacher_education_level: self.education_level,
       teacher_personal_website: self.personal_website,
       teacher_teaching_status: self.text_status,
       teacher_signed_up_at: self.created_at,
+
+      # TODO: Move these to the school model.
       teacher_school_name: self.school.name,
       teacher_school_city: self.school.city,
       teacher_school_state: self.school.state,
       teacher_school_website: self.school.website,
-    }
+    }.transform_values { |value| value.blank? ? "(blank)" : value }
   end
 
-  delegate :name, :location, :grade_level, :website, to: :school, prefix: true
-  delegate :school_type, to: :school # don't add a redundant prefix.
   # TODO: The school data needs to be cleaned up.
   def self.csv_export
     attributes = %w|
@@ -302,19 +321,6 @@ def self.csv_export
     end
   end
 
-  def email
-    # Default return primary email
-    primary_email
-  end
-
-  def primary_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)

app/views/email_templates/_liquid_fields.erb

@@ -15,10 +15,6 @@
         <td><%= value %></td>
       </tr>
     <%- end %>
-    <tr>
-      <td><code>{{bjc_password}}</code></td>
-      <td>(Should no longer be relevant)</td>
-    </tr>
     <tr>
       <td><code>{{piazza_password}}</code></td>
       <td>(The current piazza sign up password)</td>

config/secrets.yml

@@ -12,20 +12,17 @@
 
 development:
   secret_key_base: 066767348481c5869625c111a6a5d489b2b1bf24f030c832cbb814faa6c0bc005653efeb79c594776703aa468884323e6e7e914e929113fb2c95b072738ed24f
-  bjc_password: development-bjc-password
   piazza_password: development-piazza-password
 
 test:
   secret_key_base: a3c66cdded70fc6fa14d8aee9446c17c740d812d0b9f0998c7f937a269cba135406087e399eb36ff230fee10befedb7b5eda0eb35dac5c8e4c427039f6c945b7
-  bjc_password: test-bjc-password
   piazza_password: test-piazza-password
 
 
 # Do not keep production secrets in the repository,
 # instead read values from the environment.
 production: &production
   secret_key_base: <%= ENV["SECRET_KEY_BASE"] %>
-  bjc_password: <%= ENV["BJC_PASSWORD"] %>
   piazza_password: <%= ENV["PIAZZA_PASSWORD"] %>
 
 staging:

db/seed_data.rb

@@ -4,7 +4,7 @@ module SeedData
   @welcome_email = <<-WELCOME_EMAIL
     <p>Hi {{teacher_first_name}},<br /><br />Thanks for teaching with BJC!</p>
 
-    <p><strong>Accessing the teacher's guide:</strong><br />To view the <a title="teacher's guide" href="https://bjc.edc.org/teacher" target="_blank" rel="noopener">teacher's guide</a>, you will need the username: <code>bjcteacher</code> and the case-sensitive password: <code>{{bjc_password}}</code><br /><br />After you enter the password once it will be stored in a cookie for that browser; if you delete your browser cookies, you will need to reenter it. If you use this password on a computer that students also use, they will have access to these pages until you clear the cookies. <br /><br />Please note that only the Teacher Guide <em>solutions</em> pages are affected by this. Students will still be able to see the Teacher Guide course overview page, the Teacher Resources page, and the Teacher Guide unit overview pages.</p>
+    <p><strong>Accessing the teacher's guide:</strong><br />To view the <a title="teacher's guide" href="https://bjc.edc.org/teacher" target="_blank" rel="noopener">teacher's guide</a>, you will login to the teacher tracker.<br /><br />After you enter the password once it will be stored in a cookie for that browser; if you delete your browser cookies, you will need to reenter it. If you use this password on a computer that students also use, they will have access to these pages until you clear the cookies. <br /><br />Please note that only the Teacher Guide <em>solutions</em> pages are affected by this. Students will still be able to see the Teacher Guide course overview page, the Teacher Resources page, and the Teacher Guide unit overview pages.</p>
 
     <p><strong>Piazza:</strong> <br />We have a teacher's forum on <a href="https://piazza.com" target="_blank" rel="noopener">Piazza</a>, where we will share updates and you can get your questions answered. If you have not been added to Piazza, you can add yourself using this email address. Go to our <a href="https://piazza.com/cs10k/other/bjcteachers" target="_blank" rel="noopener">BJC Teachers Forum</a>&nbsp;(<a href="https://piazza.com/cs10k/other/bjcteachers" target="_blank" rel="noopener">https://piazza.com/cs10k/other/bjcteachers/home</a>). Then enter the code <code>{{piazza_password}}</code> to get access. Please do not share this login information with anyone!</p>
 
@@ -18,7 +18,7 @@ module SeedData
       Here is the information that was submitted: <br>
       Name: {{teacher_first_name}} {{teacher_last_name}} <br>
       Email: {{teacher_email}} <br>
-      Snap Username: {{teacher_snap | link_to(teacher_snap, "https://snap.berkeley.edu/user?username=" + teacher_snap )}} <br>
+      Snap Username: {{teacher_snap_username | link_to(teacher_snap_username, "https://snap.berkeley.edu/user?username=" + teacher_snap_username )}} <br>
       School: {{teacher_school_name}} <br>
       Location: {{teacher_school_city}}, {{teacher_school_state}} <br>
       Website: {{ teacher_school_website | link_to(nil, teacher_school_website) }}

features/step_definitions/web_steps.rb

@@ -214,7 +214,7 @@ def with_scope(locator, &block)
   with_scope(parent) do
     field_checked = find_field(label)["checked"]
     if field_checked.respond_to? :should
-      field_checked.should be_true
+      field_checked.should be true
     else
       assert field_checked
     end