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

Conversation

inulty-dfe
Copy link
Collaborator

@inulty-dfe inulty-dfe commented Aug 28, 2024

Context

Starting in the 2025 Recruitment Cycle we will be hiding the CourseSiteStep (picking a placement school) in the CourseSelectionWizard for certain providers.
For these providers' courses, we will automatically choose a placement school. This happens already if the provider has only one placement school.
We will also hide the Location row of the Application Review component if we have automatically selected a School.

Changes proposed in this pull request

  1. Automatically choose a placement school unless the provider is selectable_school.
  2. Enable the feature to be enabled only after the 2024 recruitment cycle
  3. Set ApplicationChoice#school_placement_auto_selected for applications to providers that have selectable_school set to false.
  4. Hide the Location row in the Course Review if the School was automatically selected

Guidance to review

  1. Selectable school factory defaults
    Q: Should factories reflect the most common conditions or the most complex conditions by default?
    For example, adding selectable_school to Provider.
    In production the default will be false.
    99%+ providers will have this as false.
    This makes the Course selection journey simpler. But it also means we have to set selectable_school to true if we want to test existing examples.
    Would you set the factory by default to true or false

  2. Whats the best way to clean up the recruitment cycle check for enabling the feature?

Testing

The environment is already set up to after apply opens in 2025.

Provider id course selectable_school sites
NIoT@Harris Initial Teacher Education 16 any true many
Keele and North Staffordshire Teacher Education (24J) 3 Primary (3-7) (2Q5R) false 3

Video

As described above, Harris is selectable_school true and Keele is selectable_school false

selectable_school-2025.webm

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Required environment variables have been updated added to the Azure KeyVault
  • Inform data insights team due to database changes
  • Make sure all information from the Trello card is in here
  • Rebased main
  • Cleaned commit history
  • Tested by running locally
  • Add PR link to Trello card

@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from def6227 to 03a17f9 Compare August 28, 2024 10:39
Comment on lines +59 to +60
classes_without_edit = [DuplicateCourseSelectionStep, FullCourseSelectionStep, ClosedCourseSelectionStep]
return next_step_path(next_step_klass) if classes_without_edit.include?(next_step_klass)
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?

@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from 03a17f9 to fefade0 Compare August 28, 2024 11:50
Copy link
Contributor

You have one or more flakey tests on this branch! ❄️ ❄️ ❄️

Failed 1 out of 3 times at ./spec/system/provider_interface/change_existing_offer_ske_standard_flow_spec.rb:28: ⚠️ expected to find visible field "Condition 2" that is not disabled with value "A* on Maths A Level" but there were no matches. Also found "Be cool", which matched the selector but not all filters. Expected value to be "A* on Maths A Level" but was "Be cool"

@inulty-dfe
Copy link
Collaborator Author

@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from fefade0 to 09d9273 Compare August 29, 2024 09:35
@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch 2 times, most recently from 2e6cbbb to 9416059 Compare August 29, 2024 09:48
@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from 9416059 to 63eb509 Compare August 29, 2024 10:11
@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from a52d2f1 to 3fab9c5 Compare August 29, 2024 13:59
@inulty-dfe inulty-dfe added the deploy_v2 Deploy the review app to AKS label Aug 29, 2024
  The next_edit_step_path does not exist for steps that are not
  editable.

  When we need to generate a path for a non-editable step when we are in
  the process of editing a course, we need to return the show action
  rather than the edit action.

  The previous implementation of this tried to detect if the next step
  had an edit page by instantiating the next step and calling
  `next_step` on that. This introduced a bug, because calling
  `next_step` required all the steps parameters to be present and we
  cannot reliably provide those in the absence of candidate input.

  This appears to be the best way to check the conditions above for now.
  Providers will have the attribute `selectable_school`. If this is
  true, we will show the Candidate the School Placement
  (course_site_step) in the Course Selection wizard.

  If it's false, we will select a site for the application
  automatically.
  We test the case where the course has multiple sites but the provider
  is selectable school false.

  Test for courses with and without multiple study modes.
@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from 3fab9c5 to 03e61b6 Compare August 29, 2024 14:02
@github-actions github-actions bot temporarily deployed to review_aks-9758 August 29, 2024 14:23 Destroyed
@github-actions github-actions bot temporarily deployed to review_aks-9758 August 29, 2024 14:50 Destroyed
@inulty-dfe inulty-dfe requested a review from a team August 29, 2024 16:00
@inulty-dfe inulty-dfe marked this pull request as ready for review August 29, 2024 16:00
Copy link
Contributor

@CatalinVoineag CatalinVoineag left a comment

Choose a reason for hiding this comment

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

Tested this on review, works as expected. Thanks for the video, that helped 👍🏻

@github-actions github-actions bot temporarily deployed to review_aks-9758 August 30, 2024 12:58 Destroyed
Copy link
Contributor

You have one or more flakey tests on this branch! ❄️ ❄️ ❄️

Failed 1 out of 3 times at ./spec/system/candidate_interface/carry_over/candidate_carries_over_after_rejecting_offer_between_cycles_spec.rb:6: ⚠️ Ambiguous match, found 2 elements matching visible link or button "West Gutkowski College"

@github-actions github-actions bot temporarily deployed to review_aks-9758 August 30, 2024 14:06 Destroyed
@github-actions github-actions bot temporarily deployed to review_aks-9758 August 30, 2024 16:11 Destroyed
@inulty-dfe inulty-dfe force-pushed the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch from 2b54ab0 to dd18845 Compare September 2, 2024 07:32
@github-actions github-actions bot temporarily deployed to review_aks-9758 September 2, 2024 07:37 Destroyed
Copy link
Contributor

github-actions bot commented Sep 2, 2024

You have one or more flakey tests on this branch! ❄️ ❄️ ❄️

Failed 1 out of 3 times at ./spec/system/candidate_interface/course_selection/candidate_deletes_application_spec.rb:6: ⚠️ Ambiguous match, found 2 elements matching visible link or button "Boyle University"

@inulty-dfe inulty-dfe merged commit f5a125b into main Sep 2, 2024
58 of 60 checks passed
@inulty-dfe inulty-dfe deleted the 2217-skip-school-selection-in-courseselection-based-on-providerselectable-school branch September 2, 2024 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy_v2 Deploy the review app to AKS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants