Merge pull request #45 from cs169/cleanup-schools-bug-fix

Cleanup schools bug fix

app/controllers/email_templates_controller.rb

@@ -18,7 +18,7 @@ def update
       flash[:success] = "Updated #{@email_template.title} template successfully."
       redirect_to email_templates_path
     else
-      flash.now[:alert] = "Failed to save #{@email_template.title} template: #{@email_template.errors.full_messages.join(", ")}"
+      flash.now[:alert] = "An error occurred: #{@email_template.errors.full_messages.join(', ')}"
       render "edit"
     end
   end
@@ -36,7 +36,7 @@ def create
       flash[:success] = "Created #{@email_template.title} successfully."
       redirect_to email_templates_path
     else
-      flash.now[:alert] = "Failed to submit information: #{@email_template.errors.full_messages.join(", ")}"
+      flash[:alert] = "An error occurred: #{@email_template.errors.full_messages.join(', ')}"
       render "new"
     end
   end

app/controllers/pages_controller.rb

@@ -25,7 +25,7 @@ def create
       flash[:success] = "Created #{@page.title} page successfully."
       redirect_to action: "show", url_slug: @page.url_slug
     else
-      flash.now[:alert] = "An error occurred! #{@page.errors.full_messages}"
+      flash.now[:alert] = "An error occurred: #{@page.errors.full_messages.join(", ")}"
       render "edit"
     end
   end
@@ -58,7 +58,7 @@ def update
       flash[:success] = "Updated #{@page.title} page successfully."
       redirect_to pages_path
     else
-      flash.now[:alert] = "An error occurred! #{@page.errors.full_messages.to_sentence}"
+      flash.now[:alert] = "An error occurred: #{@page.errors.full_messages.join(', ')}"
       render "edit"
     end
   end

app/controllers/schools_controller.rb

@@ -27,7 +27,7 @@ def create
       flash[:success] = "Created #{@school.name} successfully."
       redirect_to schools_path
     else
-      flash[:alert] = "Failed to submit information :("
+      flash[:alert] = "An error occurred: #{@school.errors.full_messages.join(', ')}"
       render "new"
     end
   end
@@ -45,10 +45,11 @@ def update
     @school = School.find(params[:id])
     @school.assign_attributes(school_params)
     if @school.save
-      flash[:success] = "Update #{@school.name} successfully."
+      flash[:success] = "Updated #{@school.name} successfully."
       redirect_to school_path(@school)
     else
-      render "edit", alert: "Failed to submit information :("
+      flash[:alert] = "An error occurred: #{@school.errors.full_messages.join(', ')}"
+      render "edit"
     end
   end
 

app/controllers/teachers_controller.rb

@@ -65,7 +65,7 @@ def create
     if @school.new_record?
       @school = School.new(school_params)
       unless @school.save
-        flash[:alert] = "An error occurred! #{@school.errors.full_messages}"
+        flash[:alert] = "An error occurred! #{@school.errors.full_messages.join(', ')}"
         render "new" && return
       end
     end
@@ -81,7 +81,7 @@ def create
       TeacherMailer.teacher_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}"
+      redirect_to new_teacher_path, alert: "An error occurred: #{@teacher.errors.full_messages.join(', ')}"
     end
   end
 
@@ -100,7 +100,10 @@ def update
       @teacher.school = @school
     else
       @school.update(school_params) if school_params
-      @school.save!
+      unless @school.save
+        flash[:alert] = "An error occurred: #{@school.errors.full_messages.join(', ')}"
+        render "new" && return
+      end
       @teacher.school = @school
     end
     send_email_if_application_status_changed_and_email_resend_enabled
@@ -114,15 +117,15 @@ def update
     end
     if !@teacher.save
       redirect_to edit_teacher_path(current_user.id),
-                alert: "Failed to update data. #{@teacher.errors.full_messages.to_sentence}"
+                alert: "An error occurred: #{@teacher.errors.full_messages.join(', ')}"
       return
     end
     if !@teacher.validated? && !current_user.admin?
       TeacherMailer.form_submission(@teacher).deliver_now
       TeacherMailer.teacher_form_submission(@teacher).deliver_now
     end
     if is_admin?
-      redirect_to teachers_path, notice: "Saved #{@teacher.full_name}"
+      redirect_to edit_teacher_path(current_user.id), notice: "Saved #{@teacher.full_name}"
       return
     else
       @teacher.try_append_ip(request.remote_ip)

app/models/email_template.rb

@@ -22,8 +22,8 @@ class EmailTemplate < ApplicationRecord
   validates :title,
             inclusion: TeacherMailer.instance_methods(false).map { |method| method.to_s.titlecase },
             if: -> { self.required? }
-  validates :body, presence: { message: "cannot be blank" }
-  validates :to, presence: { message: "cannot be blank" }
+  validates :body, presence: true
+  validates :to, presence: true
 
   before_destroy :prevent_deleting_required_emails
 

app/models/school.rb

@@ -29,7 +29,7 @@ class School < ApplicationRecord
   VALID_STATES = [ "AL", "AK", "AS", "AZ", "AR", "CA", "CO", "CT", "DE", "DC", "FM", "FL", "GA", "GU", "HI", "ID", "IL", "IN", "IA", "KS", "KY", "LA", "ME", "MH", "MD", "MA", "MI", "MN", "MS", "MO", "MT", "NE", "NV", "NH", "NJ", "NM", "NY", "NC", "ND", "MP", "OH", "OK", "OR", "PW", "PA", "PR", "RI", "SC", "SD", "TN", "TX", "UT", "VT", "VI", "VA", "WA", "WV", "WI", "WY"].freeze
 
   validates :name, :city, :website, :country, presence: true
-  validates :country, inclusion: { in: ISO3166::Country.all.map(&:alpha2), message: "%{value} is not a valid country" }, presence: true
+  validates :country, inclusion: { in: ISO3166::Country.all.map(&:alpha2), message: "%{value} is not a valid country" }
   validates :state, presence: true, if: -> { country == "US" }
   validates :state, inclusion: { in: VALID_STATES }, if: -> { country == "US" }
   validates_format_of :website, with: /.+\..+/, on: :create

app/views/schools/_form.html.erb

@@ -2,32 +2,32 @@
 <div class='form-group row'>
   <div class='col'>
       <%= f.label :name, "School Name", class: "label-required" %>
-      <%= f.text_field :name, placeholder: 'UC Berkeley', class: 'form-control',
+      <%= f.text_field :name, placeholder: 'Name of School', class: 'form-control',
       required: false, id: 'school_name' %>
   </div>
 </div>
 <div class='form-group row'>
   <div class='col-4'>
     <%= f.label :city, class: "label-required", for: "school_city" %>
-    <%= f.text_field :city, placeholder: 'Berkeley', class: 'form-control',
+    <%= f.text_field :city, placeholder: 'Name of City', class: 'form-control',
     required: false, id: 'school_city' %>
   </div>
 
   <div class='col-4' id="state_select_container">
     <%= f.label :state, class: "label-required", for: "school_state" %>
-    <%= f.select :state, School::VALID_STATES, { include_blank: "State" }, { id: "state_select", class: 'form-control' } %>
+    <%= f.select :state, School::VALID_STATES, { include_blank: "Select a state" }, { id: "state_select", class: 'form-control' } %>
   </div>
 
   <div class='col-4' id="state_textfield_container">
     <%= f.label :state, for: "school_state" %>
-    <%= f.text_field :state, placeholder: "State", class: 'form-control', id: "state_textfield" %>
+    <%= f.text_field :state, placeholder: "Name of State", class: 'form-control', id: "state_textfield" %>
   </div>
 
   <div class='col-4'>
     <%= f.label :country, "Country", class: "label-required", for: "school_country" %>
     <%= f.country_select(
       :country,
-      { priority_countries: ['United States'] },
+      { priority_countries: ['United States'], include_blank: "Select a country" },
       { class: 'form-control', required: false, id: 'school_country', format: :with_full_country_name, selected: 'United States'}
     ) %>
   </div>
@@ -110,14 +110,18 @@
 
         function handleCountryChange() {
             if (countrySelected.val() === 'US') {
-                stateSelectContainer.show().attr('required', true);
+                stateSelectContainer.show();
+                stateSelect.attr('required', '')
                 stateTextfieldContainer.hide();
 
                 stateTextfield.removeAttr('name');
                 stateSelect.attr('name', 'school[state]');
+
             } else {
-                stateTextfieldContainer.show().attr('required', false);
+                stateTextfieldContainer.show();
+                stateSelect.removeAttr('required') //else make state select not required
                 stateSelectContainer.hide();
+
 
                 stateSelect.removeAttr('name');
                 stateTextfield.attr('name', 'school[state]');

features/admin.feature

@@ -198,7 +198,8 @@ Feature: basic admin functionality
     And I set my application status as "Validated"
     And I press "Update"
     Then I see a confirmation "Saved"
-    When I check "Validated"
+    When I go to the teachers page
+    And I check "Validated"
     Then I should see "Bobby John"
 
   Scenario: Deny teacher as an admin
@@ -362,6 +363,24 @@ Feature: basic admin functionality
     Then I send a request info email
 
 
+  Scenario: Admin update info without mandatory field shows error
+    Given the following schools exist:
+      | name        | country | city     | state | website                  | grade_level | school_type |
+      | UC Berkeley | US      | Berkeley | CA    | https://www.berkeley.edu | university  | public      |
+    And the following teachers exist:
+      | first_name | last_name | admin | email                    | school      |
+      | Jane       | Doe       | false | janedoe@berkeley.edu     | UC Berkeley |
+    Given I am on the BJC home page
+    And   I have an admin email
+    And   I follow "Log In"
+    Then  I can log in with Google
+    When  I go to the teachers page
+    And   I go to the edit page for Jane Doe
+    And   I fill in "teacher_email" with ""
+    And   I press "Update"
+    Then  I should be on the edit page for Jane Doe
+
+
 # Scenario: Admin can import csv file. The loader should filter invalid record and create associate school.
 #  Given the following schools exist:
 #  |       name      |     country     |     city     |  state  |            website            |

features/email_template.feature

@@ -38,7 +38,7 @@ Scenario: Creating new email template with blank fields displays flash error
     Given I am on the email templates index
     And I press "New Email Templates"
     When I press "Submit"
-    Then I should see "Failed to submit information: Body cannot be blank, To cannot be blank"
+    Then I should see "An error occurred: Body can't be blank, To can't be blank"
 
 Scenario: Creating and deleting new email template with valid fields succeeds
     Given I am on the email templates index
@@ -59,7 +59,7 @@ Scenario: Editing email template to have blank body or to field displays flash e
     And I follow "Welcome Email"
     And I fill in "email_template_to" with ""
     And I press "Submit"
-    Then I should see "Failed to save Welcome Email template: To cannot be blank"
+    Then I should see "An error occurred: To can't be blank"
     When I fill in TinyMCE email form with ""
     And I press "Submit"
-    Then I should see "Failed to save Welcome Email template: Body cannot be blank"
+    Then I should see "An error occurred: Body can't be blank"

features/school_form_submission.feature

@@ -70,7 +70,7 @@ Scenario: User can create an international school
     And   I fill in the school name selectize box with "Bucharest International School" and choose to add a new school
     And   I select "Romania" from "Country"
     And   I enter my "City" as "Bucharest"
-    And   I enter my "State" as "Bucharest, Sector 1"
+    And   I fill in state with "Bucharest, Sector 1"
     And   I enter my "School Website" as "https://chs.fuhsd.org"
     And   I select "University" from "Grade Level"
     And   I select "Public" from "School Type"
@@ -90,7 +90,7 @@ Scenario: Admin can see international schools in the submission
     And   I fill in the school name selectize box with "Bucharest International School" and choose to add a new school
     And   I select "Romania" from "Country"
     And   I enter my "City" as "Bucharest"
-    And   I enter my "State" as "Bucharest, Sector 1"
+    And   I fill in state with "Bucharest, Sector 1"
     And   I enter my "School Website" as "https://chs.fuhsd.org"
     And   I select "University" from "Grade Level"
     And   I select "Public" from "School Type"

features/step_definitions/form_steps.rb

@@ -110,6 +110,10 @@
   find("#school_selectize").click.set(value)
 end
 
+When(/^(?:|I )fill in state with "([^"]*)"$/) do |text|
+  fill_in("state_textfield", with: text)
+end
+
 Then(/^the new teacher form should not be submitted$/) do
   expect(current_path).to eq(new_teacher_path)
   expect(page).to have_content("Your Information")

spec/controllers/schools_controller_spec.rb

@@ -9,7 +9,7 @@
 
   before(:all) do
     @create_school_name = "University of California, Berkeley"
-    @fail_flash_alert = /Failed to submit information :\(/
+    @fail_flash_error_text = "An error occurred: "
     @success_flash_alert = Regexp.new("Created #{@create_school_name} successfully.")
   end
 
@@ -72,7 +72,8 @@
         }
       }
       expect(School.find_by(name: @create_school_name)).to be_nil
-      expect(flash[:alert]).to match @fail_flash_alert
+      error = @fail_flash_error_text + "City can't be blank"
+      expect(flash[:alert]).to match error
 
       post schools_path, params: {
         school: {
@@ -88,7 +89,8 @@
         }
       }
       expect(School.find_by(name: @create_school_name)).to be_nil
-      expect(flash[:alert]).to match @fail_flash_alert
+      error = @fail_flash_error_text + "Website can't be blank, Website is invalid"
+      expect(flash[:alert]).to match error
 
       post schools_path, params: {
         school: {
@@ -104,7 +106,8 @@
         }
       }
       expect(School.find_by(name: @create_school_name)).to be_nil
-      expect(@fail_flash_alert).to match flash[:alert]
+      error = @fail_flash_error_text + "Name can't be blank"
+      expect(flash[:alert]).to match error
     end
 
     it "requires proper inputs for fields" do
@@ -124,7 +127,8 @@
         }
       }
       expect(School.find_by(name: @create_school_name)).to be_nil
-      expect(@fail_flash_alert).to match flash[:alert]
+      error = @fail_flash_error_text + "State is not included in the list"
+      expect(flash[:alert]).to match error
 
       post schools_path, params: {
         school: {
@@ -140,7 +144,8 @@
         }
       }
       expect(School.find_by(name: @create_school_name)).to be_nil
-      expect(@fail_flash_alert).to match flash[:alert]
+      error = @fail_flash_error_text + "Website is invalid"
+      expect(flash[:alert]).to match error
 
       # Incorrect school type
       expect { post schools_path, params: {
@@ -179,6 +184,43 @@
       expect(School.find_by(name: @create_school_name)).to be_nil
     end
 
+    it "does not allow invalid country" do
+      post schools_path, params: {
+        school: {
+          name: @create_school_name,
+          city: "Test City",
+          country: "XX", # invalid country
+          state: "CA",
+          website: "www.berkeley.edu",
+          school_type: "public",
+          grade_level: "university",
+          tags: [],
+          nces_id: 123456789000
+        }
+      }
+      expect(School.find_by(name: @create_school_name)).to be_nil
+      error = @fail_flash_error_text + "Country XX is not a valid country"
+      expect(flash[:alert]).to match error
+    end
+
+    it "allows any state when country is not US" do
+      post schools_path, params: {
+        school: {
+          name: @create_school_name,
+          city: "Ottawa",
+          country: "CA", # Canada
+          state: "Ontario",
+          website: "www.berkeley.edu",
+          school_type: "public",
+          grade_level: "university",
+          tags: [],
+          nces_id: 123456789000
+        }
+      }
+      expect(School.find_by(name: @create_school_name)).not_to be_nil
+      expect(@success_flash_alert).to match flash[:success]
+    end
+
     it "does not create duplicate schools in the same city" do
       expect(School.where(name: @create_school_name).count).to eq 0