CAPT 1995/remove ecp #3415
Merged
CAPT 1995/remove ecp #3415
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
6 times, most recently
from
ca6b041 to
47236ce
Compare
asmega
reviewed
Nov 26, 2024
| @@ -63,6 +63,14 @@ def policies_eligible_now_and_sorted | |||
| policies_eligible_now_with_award_amount_and_sorted.map { |policy_with_award_amount| policy_with_award_amount.policy } | |||
| end | |||
|
|
|||
| def potentially_still_eligible | |||
There was a problem hiding this comment.
maybe rename to potentially_still_eligible_policies?
asmega
approved these changes
Nov 26, 2024
Thanks, there's still one big piece I want to remove |
|
I'll have a look today! |
kenfodder
approved these changes
Nov 28, 2024
|
All LGTM so far 👍 |
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
316c2f0 to
682d4e3
Compare
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
b13247f to
0f5456e
Compare
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
0f5456e to
e7c8c7f
Compare
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
e7c8c7f to
90a9bcf
Compare
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
90a9bcf to
661dc49
Compare
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
661dc49 to
6a5e368
Compare
|
@kenfodder thanks for the review. When you have a minute, would you mind taking a second look as I've pushed some commits since you last review (rebased it on master and added this new functionality. @slorek would be good to get your eyes on this too before merging! |
slorek
reviewed
Dec 10, 2024
| eligible_itt_years.include?(itt_academic_year) | ||
| end | ||
|
|
||
| def requires_student_loan_details? | ||
| true | ||
| end | ||
|
|
||
| def selectable_subject_symbols(journey_session) |
There was a problem hiding this comment.
Suggestion: this could probably live in the EligibleIttSubjectForm class.
There was a problem hiding this comment.
👍 that's nicer, will do this as a separate pr though
Part of refactoring to remove ECP. These concerns are only used in these classes, inlining them to make tracking the difference between ECP and LUP easier.
This was the only method EligibilityCheckable was being pulled in for. While we're duplicating the definition of a trainee teacher, inlining the method here will make splitting up the EligibilityCheckable policy easier.
FINAL_LUP_POLICY_YEAR is a dupe of Lup::POLICY_END_YEAR. We should have updated Lup::POLICY_END_YEAR to 2025 as part of capt-1832
This method is only used in the specs. Inline the method to where it's used and remove the method and it's tests. One less thing to keep track off!
Splits this method by policy so it's easier to break out LUP into it's own journey.
There was a lot of duplicated business rules in this method. We were explicity checking for `nqt_in_academic_year_after_itt`, and if it was absent we were falling back to only the lup subjects. The ECP policy is ineligible for non `nqt_in_academic_year_after_itt` claims so it will be filtered out when we check for `potentially_still_eligible_policies`. We returned an empty array if the `current_academic_year` was outside the lup policy dates, however the LUP#subject_symbols method already returns an empty array of available subjects for years outside the policy. There's some weirdness in the `AdditionalPaymentsForTeaching.selectable_subject_symbols` method. If we're dealing with a trainee teacher, we return a hard coded list of subjects, however trainee teachers won't be eligible as the `EligibilityCheckable#common_ineligible_attributes?` checks if the claimant is a trainee teacher, and is thus ineligible. It seems weird to return a list of subjects if the claimant is ineligible. This was the existing behaviour so I've kept it in place but this likely needs revisiting.
As part of the refactor to remove ECP we want to move the policy specific functionality into the respective polices. This commit moves the `selectable_itt_years_for_claim_year` out of the lib class and into each policy, with the journey delegating this method to any potentially_still_eligible policies.
Move fixed_lup_subject_symbols of off JourneySubjectEligibilityChecker and onto LevellingUpPremiumPayments policy as this method is policy specific.
This class is no longer needed
Just making the code a little easier to follow
This branch can't get hit as this method is only called when the claimant is a trainee teacher, ie `nqt_in_academic_year_after_itt` is `false`.
ECP `specific_ineligible_attributes?` already checks if `trainee_teacher?` is set. For LUP trainee teachers can still be eligible later, so we want to make sure we don't return ineligible in that case. This previously hasn't surfaced as an issue due to how the code was called (a short circut specific to trainee teachers in the page sequence) but we want to move some of the code around which exposes this bug.
Removes the ineligiblity branches from the special case trainee teacher journey. We want all eligibility checking to happen in one place (the policy eligibility checker). This commit updates the mini trainee journey to not check eligibility. This also surfaced a bug in the policy eligiblity checker, a trainee teacher with an ineligible itt subject is not yet ineligible, they may still be eligible later, we don't know until they've entered their degree subject (at least according to the logic in `spec/features/trainee_teacher_subjourney_for_lup_schools_spec.rb:59`)
Previously the page sequence contained specific logic for handling an additional payments sub journey (traniee teacher), and rather than modifying the slug sequence just overwrote what is returned. Additionally there was a condition in the slug sequence that caused the whole thing to be swapped out if we were dealing with a trainee teacher. This commit removes the additional payments specific handling from the page sequence, leaving it journey agnostic, and updates the additional payments slug sequence to move the trainee teacher sub journey branching to the top level.
LUP will extend past ECP, so we can't use the combined end date when deciding what subjects to show for nqts who haven't entered an itt, otherwise we render an empty list on the itt subject screen.
Previously the hint text was showing all subjects to claimants, even if they were not eligible for those subjects, instead only show subjects the claimant is eligible for. If the claimant is not eligible for any subjects they will have been kicked out of the journey prior to this screen.
This is additional functionality, so may want splitting out into a separate PR, but it was requested as part of this ticket. If the current acadmic year is after the ECP policy close date, then selecting an ECP only school should show the claimant the ineligible screen. There's a slight bit of trickyness, the ECP::Eligibility model has an enum for itt academic year, that only allows values for dates within the ECP policy, or nil, adding a date outside this range throws an error. The page sequences calls the claim submission form which constructs an eligibility to check if the journey is completed, if the user chooses an itt date that's outside the ECP policy range (possible for future academic years) constructing the ecp eligibility throws an error. To avoid this we're using the new `validate` option on enum, so rather than throw an error when adding a date out side the range it now generates a validation error. Some of the tests needed updating to add a valid itt academic year as now ecp eligibility isn't valid if it's missing the itt year.
rjlynch
force-pushed
the
CAPT-1995/remove-ecp
branch
from
6a5e368 to
5650863
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces some refactors to separate the LUP and ECP logic as part of the move to splitting LUP into it's own journey and deprecating ECP.
This will be a lot easier to review looking at the individual commits!
At a high level this PR
lib/journey_subject_eligibility_checker.rbWe'll introduce the LUP only journey in a separate PR.