Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip school selection in CourseSelection based on Provider#selectable_school #9758

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ def multiple_study_modes?
course.currently_has_both_study_modes_available?
end

delegate :multiple_sites?, to: :course
def multiple_sites?
course.multiple_sites? && provider.selectable_school?
end

def provider_exists?
Provider.exists?(provider_id)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,11 @@ def next_step
end
end

# When editing an application choice, we want to load the show path for
# some steps that don't have an edit action / template
def next_edit_step_path(next_step_klass)
return next_step_path(next_step_klass) unless next_step_klass.new.next_step
classes_without_edit = [DuplicateCourseSelectionStep, FullCourseSelectionStep, ClosedCourseSelectionStep]
return next_step_path(next_step_klass) if classes_without_edit.include?(next_step_klass)
Comment on lines +59 to +60
Copy link
Collaborator Author

@inulty-dfe inulty-dfe Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug fix.

We get silent exception.

When we try to instantiate the next step and call next_step on that (to know if there is a next step) we get errors about the params required to create the next - next step.

This was originally designed to avoid having to list all the steps we know do not have a next step.

We now revert to doing that out of necessity. Description added to the commit message also.

Copy link
Collaborator Author

@inulty-dfe inulty-dfe Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally "terminal" or "editable" steps should have a property so we can identify them.

class ThingStep < DfE::Wizard::Step

  def self.terminal
    true
  end

...

return next_step_path(next_step_klass) unless next_step_klass.terminal?

@tomas-stefano

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to read and read and I don't understand to be fair. 🤔

Let's huddle in the afternoon?


super
end
Expand Down
6 changes: 6 additions & 0 deletions app/models/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ def onboarded?
provider_agreements.any?
end

def selectable_school?
return true unless CycleTimetable.current_year >= 2025

super
end

def lacks_admin_users?
courses.any? &&
!(provider_permissions.exists?(manage_users: true) &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
end

describe '#next_step' do
let(:provider) { create(:provider) }
let(:provider) { create(:provider, selectable_school: true) }
let(:course) do
create(
:course,
Expand All @@ -42,6 +42,19 @@
end
end

context 'when course has multiple sites and provider school is not selectable', time: CycleTimetableHelper.mid_cycle(2025) do
let(:provider) { create(:provider, selectable_school: false) }

before do
create(:course_option, site: create(:site, provider:), course:)
create(:course_option, site: create(:site, provider:), course:)
end

it 'returns :course_review' do
expect(course_study_mode_step.next_step).to be(:course_review)
end
end

context 'when course has single site' do
let(:site) { create(:site, provider:) }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
subject(:which_course_are_you_applying_to_step) { described_class.new(provider_id:, course_id:, wizard:) }

let(:candidate) { create(:candidate) }
let(:provider) { create(:provider) }
let(:provider) { create(:provider, selectable_school: true) }
let(:course) do
create(
:course,
Expand Down Expand Up @@ -114,7 +114,7 @@
end

describe '#next_step' do
let(:provider) { create(:provider) }
let(:provider) { create(:provider, selectable_school: true) }
let(:course) do
create(
:course,
Expand Down Expand Up @@ -163,6 +163,19 @@
end
end

context 'when course has multiple sites and provider school is not selectable', time: CycleTimetableHelper.mid_cycle(2025) do
let(:provider) { create(:provider, selectable_school: false) }

before do
create(:course_option, site: create(:site, provider:), course:)
create(:course_option, site: create(:site, provider:), course:)
end

it 'returns :course_review' do
expect(which_course_are_you_applying_to_step.next_step).to be(:course_review)
end
end

context 'when course has no course availability' do
it 'returns :full_course_selection' do
expect(which_course_are_you_applying_to_step.next_step).to be(:full_course_selection)
Expand Down
34 changes: 34 additions & 0 deletions spec/models/provider_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,38 @@
expect(described_class.with_courses).to eq([provider])
end
end

describe 'selectable_school?' do
context 'provider is selectable_school' do
let(:provider) { create(:provider, selectable_school: true) }

context 'when Current Cycle is 2024', time: mid_cycle(2024) do
it 'is not selectable school' do
expect(provider).to be_selectable_school
end
end

context 'when Current Cycle is greater than 2024', time: mid_cycle(2025) do
it 'is selectable school' do
expect(provider).to be_selectable_school
end
end
end

context 'provider is not selectable_school' do
let(:provider) { create(:provider, selectable_school: false) }

context 'when Current Cycle is 2024', time: mid_cycle(2024) do
it 'is not selectable school' do
expect(provider).to be_selectable_school
end
end

context 'when Current Cycle is greater than 2024', time: mid_cycle(2025) do
it 'is not selectable school' do
expect(provider).not_to be_selectable_school
end
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def given_i_am_signed_out
end

def and_the_course_i_selected_only_has_one_site
@course = create(:course, :open, name: 'Potions')
@course = create(:course, :open, name: 'Potions', provider: create(:provider, selectable_school: true))
@site = create(:site, provider: @course.provider)
create(:course_option, site: @site, course: @course)
end
Expand Down Expand Up @@ -151,6 +151,7 @@ def given_the_course_i_selected_has_multiple_sites
:course,
:open,
:with_both_study_modes,
provider: create(:provider, selectable_school: true),
name: 'Herbology',
)
@site1 = create(:site, provider: @course_with_multiple_sites.provider)
Expand Down Expand Up @@ -198,7 +199,7 @@ def and_i_see_a_link_to_the_course_on_find
private

def application_choice_for_candidate(candidate:, application_choice_count:)
provider = create(:provider)
provider = create(:provider, selectable_school: true)
application_form = create(:application_form, candidate:)
application_choice_count.times { course_option_for_provider(provider:) }
provider.courses.each do |course|
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
end

def given_there_is_a_provider_with_a_course_that_is_only_accepting_applications_on_apply
@provider = create(:provider, code: '8N5', name: 'Snape University')
@provider = create(:provider, code: '8N5', name: 'Snape University', selectable_school: true)
@course = create(:course, :open, name: 'Potions', provider: @provider)
first_site = create(
:site,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
end

def given_there_is_a_provider_with_a_course_that_is_only_accepting_applications_on_apply
@provider = create(:provider, name: 'Vim masters')
@provider = create(:provider, name: 'Vim masters', selectable_school: true)

@first_site = create(:site, provider: @provider, name: 'Site 1')
@second_site = create(:site, provider: @provider, name: 'Site 2')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def given_i_am_signed_in
end

def and_there_is_a_course_with_one_course_option
@provider = create(:provider)
@provider = create(:provider, selectable_school: true)
create(:course, :open, name: 'English', provider: @provider, study_mode: :full_time)

course_option_for_provider(provider: @provider, course: @provider.courses.first)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ def given_i_am_signed_in
end

def and_there_are_course_options
@provider = create(:provider, name: 'University of Alien Dance')
@provider = create(:provider, name: 'University of Alien Dance', selectable_school: true)

@first_site = create(:site, provider: @provider, name: 'Site 1')
@second_site = create(:site, provider: @provider, name: 'Site 2')
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
require 'rails_helper'

RSpec.describe 'Selecting a course with multiple sites when the provider is not selectable_school', time: CycleTimetableHelper.mid_cycle(2025) do
include CandidateHelper

it 'Candidate skips the school selection' do
given_i_am_signed_in
and_there_are_course_options

when_i_visit_the_site
and_i_click_on_course_choices

and_i_choose_that_i_know_where_i_want_to_apply
and_i_click_continue
and_i_choose_a_provider
and_i_choose_a_course

then_i_am_on_the_application_choice_review_page
end

def given_i_am_signed_in
create_and_sign_in_candidate
end

def when_i_visit_the_site
visit candidate_interface_continuous_applications_details_path
end

def and_i_click_on_course_choices
click_link_or_button 'Your application'
click_link_or_button 'Add application'
end

def and_i_choose_that_i_know_where_i_want_to_apply
choose 'Yes, I know where I want to apply'
and_i_click_continue
end

def when_i_choose_that_i_know_where_i_want_to_apply
and_i_choose_that_i_know_where_i_want_to_apply
end

def and_i_choose_a_provider
select 'Gorse SCITT (1N1)'
click_link_or_button t('continue')
end

def and_i_choose_a_course
choose 'Primary (2XT2)'
click_link_or_button t('continue')
end

def when_i_click_continue
click_link_or_button t('continue')
end

def and_i_click_continue
when_i_click_continue
end

def application_choice
current_candidate.current_application.application_choices.last
end

def and_there_are_course_options
@provider = create(:provider, name: 'Gorse SCITT', code: '1N1', selectable_school: false)
first_site = create(
:site,
name: 'Main site',
code: '-',
provider: @provider,
address_line1: 'Gorse SCITT',
address_line2: 'C/O The Bruntcliffe Academy',
address_line3: 'Bruntcliffe Lane',
address_line4: 'MORLEY, lEEDS',
postcode: 'LS27 0LZ',
)
second_site = create(
:site,
name: 'Harehills Primary School',
code: '1',
provider: @provider,
address_line1: 'Darfield Road',
address_line2: '',
address_line3: 'Leeds',
address_line4: 'West Yorkshire',
postcode: 'LS8 5DQ',
)
@multi_site_course = create(:course, :open, :with_both_study_modes, name: 'Primary', code: '2XT2', provider: @provider)
create(:course_option, site: first_site, course: @multi_site_course)
create(:course_option, site: second_site, course: @multi_site_course)
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def application_choice
end

def and_there_are_course_options
@provider = create(:provider, name: 'Gorse SCITT', code: '1N1')
@provider = create(:provider, name: 'Gorse SCITT', code: '1N1', selectable_school: true)
first_site = create(
:site,
name: 'Main site',
Expand Down
Loading
Loading