Implement PR feedback

https://trello.com/c/ryoxvses

app/controllers/placements/schools/placements/build_controller.rb

@@ -37,15 +37,10 @@ def update
       @placement.subject_ids = subject_ids
       @placement.build_subjects(subject_ids)
       @placement.build_mentors(mentor_ids)
+      @placement.save!
 
-      if @placement.all_valid?
-        @placement.save!
-
-        session.delete(:add_a_placement)
-        redirect_to placements_school_placements_path(school), flash: { success: t(".success") } and return
-      else
-        render :check_your_answers and return
-      end
+      session.delete(:add_a_placement)
+      redirect_to placements_school_placements_path(school), flash: { success: t(".success") } and return
     when :add_phase
       @placement = build_placement
       @placement.phase = phase_params
@@ -77,8 +72,6 @@ def update
       else
         render :add_mentors and return
       end
-    else
-      raise ActionController::RoutingError, "Not Found"
     end
 
     redirect_to public_send(next_step(params[:id]))
@@ -117,14 +110,19 @@ def build_or_retrieve_placement
   end
 
   def assign_subjects_based_on_phase
-    @phase = session.dig(:add_a_placement, "phase").presence || (school.primary_or_secondary_only? ? school.phase : "Primary")
+    phase = session.dig(:add_a_placement, "phase")
+    @phase = @placement.build_phase(phase)
     @subjects = @phase == "Primary" ? Subject.primary : Subject.secondary
     @placement.build_subjects(subject_ids)
     @selected_subjects = @placement.subjects
   end
 
   def find_mentors
-    session.dig(:add_a_placement, "mentor_ids").present? ? school.mentors.find(session.dig(:add_a_placement, "mentor_ids").compact_blank) : []
+    if session.dig(:add_a_placement, "mentor_ids").present?
+      school.mentors.find(session.dig(:add_a_placement, "mentor_ids").compact_blank)
+    else
+      []
+    end
   end
 
   def initialize_placement
@@ -144,7 +142,8 @@ def school
 
   def build_placement
     if session.dig(:add_a_placement, "phase").present?
-      Placements::Schools::Placements::Build::Placement.new(school:, phase: session.dig(:add_a_placement, "phase"))
+      Placements::Schools::Placements::Build::Placement.new(school:,
+                                                            phase: session.dig(:add_a_placement, "phase"))
     else
       Placements::Schools::Placements::Build::Placement.new(school:)
     end

app/models/placements/mentor.rb

@@ -21,8 +21,4 @@ class Placements::Mentor < Mentor
   has_many :placements, through: :placement_mentor_joins
 
   default_scope { joins(:mentor_memberships).distinct }
-
-  def full_name
-    "#{first_name} #{last_name}"
-  end
 end

app/models/placements/schools/placements/build/placement.rb

@@ -14,7 +14,7 @@ class Placements::Schools::Placements::Build::Placement < Placement
   def valid_phase?
     return true if phase.present? && [Placements::School::PRIMARY_PHASE, Placements::School::SECONDARY_PHASE].include?(phase)
 
-    errors.add(:phase, "Select a phase")
+    errors.add(:phase, I18n.t("activerecord.errors.models.placements/schools/placements/build/placement.attributes.phase.invalid"))
     false
   end
 
@@ -24,7 +24,7 @@ def valid_mentor_ids?
     if mentor_ids.present? && mentor_ids.all? { |id| Placements::Mentor.exists?(id:) && school.mentors.exists?(id:) }
       true
     else
-      errors.add(:mentor_ids, "Select a mentor or not known")
+      errors.add(:mentor_ids, I18n.t("activerecord.errors.models.placements/schools/placements/build/placement.attributes.mentor_ids.invalid"))
       false
     end
   end
@@ -34,7 +34,7 @@ def valid_subjects?
     if subject_ids.present? && converted_subject_ids.all? { |id| Subject.exists?(id:) && Subject.find(id).subject_area.downcase == phase.downcase }
       true
     else
-      errors.add(:subject_ids, "Select a subject")
+      errors.add(:subject_ids, I18n.t("activerecord.errors.models.placements/schools/placements/build/placement.attributes.subject_ids.invalid"))
       false
     end
   end
@@ -44,22 +44,23 @@ def all_valid?
   end
 
   def build_subjects(subject_ids = nil)
-    if subject_ids.instance_of?(String)
-      subjects << Subject.find(subject_ids)
-    elsif subject_ids.present?
-      subject_ids.each { |subject_id| subjects << Subject.find(subject_id) }
+    if subject_ids.present?
+      subjects << Subject.where(id: subject_ids)
     else
       subjects.build
     end
   end
 
   def build_mentors(mentor_ids = nil)
     if mentor_ids.present?
-      mentor_ids.compact_blank.each do |mentor_id|
-        mentors << Placements::Mentor.find(mentor_id)
-      end
+      mentors << Placements::Mentor.where(id: mentor_ids.compact_blank)
     else
       mentors.build
     end
   end
+
+  def build_phase(phase)
+    phase.presence ||
+      (school.primary_or_secondary_only? ? school.phase : "Primary")
+  end
 end

config/locales/en/placements/schools/placements.yml

@@ -1,5 +1,16 @@
 
 en:
+  activerecord:
+    errors:
+      models:
+        placements/schools/placements/build/placement:
+          attributes:
+            phase:
+              invalid: Select a phase
+            mentor_ids:
+              invalid: Select a mentor or not known
+            subject_ids:
+              invalid: Select a subject
   placements:
     schools:
       placements:

spec/models/placements/mentor_spec.rb

@@ -44,11 +44,4 @@
       expect(described_class.all).not_to include(claims_mentor)
     end
   end
-
-  describe "#full_name" do
-    it "returns the mentor's full name" do
-      mentor = build(:placements_mentor, first_name: "Anne", last_name: "Smith")
-      expect(mentor.full_name).to eq("Anne Smith")
-    end
-  end
 end

spec/models/placements/schools/placements/build/placement_spec.rb

@@ -102,17 +102,6 @@
   end
 
   describe "#build_subjects" do
-    context "when passes a string" do
-      it "builds subjects" do
-        placement = described_class.new(school:)
-        subject = create(:subject)
-
-        placement.build_subjects(subject.id)
-
-        expect(placement.subjects).to eq([subject])
-      end
-    end
-
     context "when passed an array" do
       it "builds subjects" do
         placement = described_class.new(school:)
@@ -121,7 +110,10 @@
 
         placement.build_subjects([subject_1.id, subject_2.id])
 
-        expect(placement.subjects).to eq([subject_1, subject_2])
+        expect(placement.subjects).to match_array([
+          have_attributes(subject_1.attributes),
+          have_attributes(subject_2.attributes),
+        ])
       end
     end
 
@@ -131,7 +123,9 @@
 
         placement.build_subjects
 
-        expect(placement.subjects.first).to have_attributes(Subject.new.attributes)
+        expect(placement.subjects).to match_array([
+          have_attributes(Subject.new.attributes),
+        ])
       end
     end
   end
@@ -145,7 +139,10 @@
 
         placement.build_mentors([mentor_1.id, mentor_2.id])
 
-        expect(placement.mentors).to eq([mentor_1, mentor_2])
+        expect(placement.mentors).to match_array([
+          have_attributes(mentor_1.attributes),
+          have_attributes(mentor_2.attributes),
+        ])
       end
     end
 
@@ -155,7 +152,37 @@
 
         placement.build_mentors
 
-        expect(placement.mentors.first).to have_attributes(Placements::Mentor.new.attributes)
+        expect(placement.mentors).to match_array([
+          have_attributes(Placements::Mentor.new.attributes),
+        ])
+      end
+    end
+  end
+
+  describe "#build_phase" do
+    context "when passed a phase" do
+      it "returns the phase" do
+        placement = described_class.new
+
+        expect(placement.build_phase("Primary")).to eq("Primary")
+      end
+    end
+
+    context "when phase is blank and the school phase is set" do
+      it "returns the school's phase" do
+        allow(school).to receive(:phase).and_return("Primary")
+        placement = described_class.new(school:)
+
+        expect(placement.build_phase(nil)).to eq(school.phase)
+      end
+    end
+
+    context "when phase is blank and the school phase is not set" do
+      it "returns 'Primary'" do
+        allow(school).to receive(:phase).and_return(nil)
+        placement = described_class.new(school:)
+
+        expect(placement.build_phase(nil)).to eq("Primary")
       end
     end
   end

spec/system/placements/schools/placements/add_a_placement_spec.rb

@@ -3,18 +3,13 @@
 RSpec.describe "Placements / Schools / Placements / Add a placement",
                type: :system, service: :placements do
   let(:school) { build(:placements_school, name: "School 1", phase: "Primary") }
-  let(:subject_1) { create(:subject, name: "Primary subject", subject_area: :primary) }
-  let(:subject_2) { create(:subject, name: "Secondary subject", subject_area: :secondary) }
-  let(:subject_3) { create(:subject, name: "Secondary subject 2", subject_area: :secondary) }
-  let(:mentor_1) { create(:placements_mentor_membership, school:).mentor }
-  let(:mentor_2) { create(:placements_mentor_membership, school:).mentor }
+  let!(:subject_1) { create(:subject, name: "Primary subject", subject_area: :primary) }
+  let!(:subject_2) { create(:subject, name: "Secondary subject", subject_area: :secondary) }
+  let!(:subject_3) { create(:subject, name: "Secondary subject 2", subject_area: :secondary) }
+  let!(:mentor_1) { create(:placements_mentor_membership, school:).mentor }
+  let!(:mentor_2) { create(:placements_mentor_membership, school:).mentor }
 
   before do
-    mentor_1
-    mentor_2
-    subject_1
-    subject_2
-    subject_3
     given_i_sign_in_as_anne
   end
 
@@ -23,12 +18,26 @@
       when_i_visit_the_placements_page
       and_i_click_on("Add placement")
       then_i_see_the_add_a_placement_subject_page(school.phase)
-      when_i_choose_a_subject("Primary subject")
+      when_i_choose_a_subject(subject_1.name)
       and_i_click_on("Continue")
       then_i_see_the_add_a_placement_mentor_page
       when_i_check_a_mentor(mentor_1.full_name)
       and_i_click_on("Continue")
-      then_i_see_the_check_your_answers_page(school.phase)
+      then_i_see_the_check_your_answers_page(school.phase, mentor_1)
+      and_i_click_on("Publish placement")
+      then_i_see_the_placements_page
+      and_i_see_my_placement(school.phase)
+    end
+
+    scenario "when I select not known for the mentor" do
+      when_i_visit_the_placements_page
+      and_i_click_on("Add placement")
+      when_i_choose_a_subject(subject_1.name)
+      and_i_click_on("Continue")
+      then_i_see_the_add_a_placement_mentor_page
+      when_i_check_a_mentor("Not known")
+      and_i_click_on("Continue")
+      then_i_see_the_check_your_answers_page(school.phase, nil)
       and_i_click_on("Publish placement")
       then_i_see_the_placements_page
       and_i_see_my_placement(school.phase)
@@ -74,11 +83,10 @@
       scenario "my selected options are rendered when navigating using the back button" do
         when_i_visit_the_placements_page
         and_i_click_on("Add placement")
-        when_i_choose_a_subject("Primary subject")
+        when_i_choose_a_subject(subject_1.name)
         and_i_click_on("Continue")
         when_i_click_on("Back")
-        then_my_chosen_subject_is_selected("Primary subject")
-
+        then_my_chosen_subject_is_selected(subject_1.name)
         when_i_visit_the_add_mentor_page
         when_i_check_a_mentor(mentor_1.full_name)
         and_i_click_on("Continue")
@@ -93,7 +101,7 @@
         then_i_see_the_add_a_placement_subject_page(school.phase)
         and_i_see_the_error_message("Select a subject")
 
-        when_i_choose_a_subject("Primary subject")
+        when_i_choose_a_subject(subject_1.name)
         and_i_click_on("Continue")
         and_i_click_on("Continue")
         then_i_see_the_add_a_placement_mentor_page
@@ -108,12 +116,12 @@
       when_i_visit_the_placements_page
       and_i_click_on("Add placement")
       then_i_see_the_add_a_placement_subject_page(school.phase)
-      when_i_check_the_subject("Secondary subject")
+      when_i_check_the_subject(subject_2.name)
       and_i_click_on("Continue")
       then_i_see_the_add_a_placement_mentor_page
       when_i_check_a_mentor(mentor_1.full_name)
       and_i_click_on("Continue")
-      then_i_see_the_check_your_answers_page(school.phase)
+      then_i_see_the_check_your_answers_page(school.phase, mentor_1)
       and_i_click_on("Publish placement")
       then_i_see_the_placements_page
       and_i_see_my_placement(school.phase)
@@ -130,14 +138,14 @@
         when_i_choose_a_phase("Primary")
         and_i_click_on("Continue")
         then_i_see_the_add_a_placement_subject_page("Primary")
-        when_i_choose_a_subject("Primary")
+        when_i_choose_a_subject(subject_1.name)
         then_i_see_the_add_a_placement_subject_page("Primary")
-        when_i_choose_a_subject("Primary subject")
+        when_i_choose_a_subject(subject_1.name)
         and_i_click_on("Continue")
         then_i_see_the_add_a_placement_mentor_page
         when_i_check_a_mentor(mentor_1.full_name)
         and_i_click_on("Continue")
-        then_i_see_the_check_your_answers_page("Primary")
+        then_i_see_the_check_your_answers_page("Primary", mentor_1)
         and_i_click_on("Publish placement")
         then_i_see_the_placements_page
         and_i_see_my_placement("Primary")
@@ -153,13 +161,13 @@
         when_i_choose_a_phase("Secondary")
         and_i_click_on("Continue")
         then_i_see_the_add_a_placement_subject_page("Secondary")
-        when_i_check_the_subject("Secondary subject")
-        and_i_check_the_subject("Secondary subject 2")
+        when_i_check_the_subject(subject_2.name)
+        and_i_check_the_subject(subject_3.name)
         and_i_click_on("Continue")
         then_i_see_the_add_a_placement_mentor_page
         when_i_check_a_mentor(mentor_1.full_name)
         and_i_click_on("Continue")
-        then_i_see_the_check_your_answers_page("Secondary")
+        then_i_see_the_check_your_answers_page("Secondary", mentor_1)
         and_i_click_on("Publish placement")
         then_i_see_the_placements_page
         and_i_see_my_placement("Secondary")
@@ -272,11 +280,11 @@ def when_i_check_a_mentor(mentor_name)
     check mentor_name
   end
 
-  def then_i_see_the_check_your_answers_page(phase)
+  def then_i_see_the_check_your_answers_page(phase, mentor)
     expect(page).to have_content("Check your answers")
     expect(page).to have_content(phase)
     expect(page).to have_content("#{phase} subject")
-    expect(page).to have_content(mentor_1.full_name)
+    expect(page).to have_content(mentor.full_name) if mentor.present?
   end
 
   def and_my_chosen_mentor_is_checked(mentor_name)