Relax validation on ChangeSchedule service for certain participants #4838

ethax-ross · 2024-05-23T14:02:59Z

Context

We want to allow ECTs and mentors to change schedule/cohort even if they have billable declarations under certain scenarios.

The participants eligible are identified by eligible_to_change_cohort_and_continue_training.

Participants that change cohort using this mechanism should be able to change back to their original cohort providing no billable declarations were created against the new cohort.

Changes proposed in this pull request

Relax validation on ChangeSchedule service for certain participants

By default the service will not allow eligible participants to transfer; it needs to be called explicitly with attempt_to_transfer_leaving_billable_declarations set to true. This is to prevent lead providers performing these changes for now.

Guidance to review

A participant will be able to transfer if:

attempt_to_transfer_leaving_billable_declarations is true
their participant profile is eligible_to_change_cohort_and_continue_training?
the cohort they are transferring to is the active registration cohort
their current cohort has payments frozen

On transferring, the cohort_changed_after_payments_frozen will be true.

A participant will be able to transfer back if:

attempt_to_transfer_leaving_billable_declarations is true
they have no billable declarations on the cohort they are leaving
cohort_changed_after_payments_frozen is true on the profile
they have billable declarations in the profile they are transferring back to
the cohort they are transferring back to is payments_frozen

On transferring back, the cohort_changed_after_payments_frozen will be false.

github-actions · 2024-05-23T14:11:30Z

Review app deployed to https://cpd-ecf-review-4838-web.test.teacherservices.cloud

ltello · 2024-05-25T09:25:22Z

app/models/participant_profile/ecf.rb

+  def eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort_start_year:)
+    to_cohort = Cohort.find_by_start_year(to_cohort_start_year)


Suggested change

def eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort_start_year:)

to_cohort = Cohort.find_by_start_year(to_cohort_start_year)

def eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort:)

We have already the cohort instance everytime we need to call this method.
Better we pass it and save one query to the db?

ltello · 2024-05-25T09:33:44Z

app/models/participant_profile/ecf.rb

@@ -83,7 +92,7 @@ def self.archivable(for_cohort_start_year:, restrict_to_participant_ids: [])
    query = left_joins(:participant_declarations, schedule: :cohort)
      # Exclude participants that have any induction records that are not FIP.
      .where.not(id: not_fip_induction_records.select(:participant_profile_id))


Why are we excluding participants not doing FIP even if they don't have billable declarations?
Maybe I don't know all requirements to archive participants.

This seems to be excluding participants that once (any induction record) were associated to a non-FIP programme <> participants doing non-FIP right now (latest induction record).

Is that exactly what we want?

If we don't archive 2021 participants doing non-FIP programmes and with no billable declarations, they might need to be transferred to 2024.

This means the requirements for the service Induction::AmendParticipantCohort change and we have to relax its validations to cope with these participants. Same for the ChangeSchedule service...

The requirements for the archive logic are at the bottom of Nathan's word doc, he's also included this explanation for those undeclared participants that will not be archived:

This means that there will be some undeclared participants who are not archived. A tiny number of these participants may require FIP/CIP training in the 2024 cohort (estimate 10s not 100s). It may be preferable for these participants to be transferred manually by DfE admin users if flagged by providers/helpdesk.

Ok, if he wants to transfer them manually when flagged by providers/helpdesk, I am happy to leave the code as it is now.

ltello · 2024-05-25T09:47:29Z

app/models/participant_profile/ect.rb

      .where(induction_completion_date: nil)
-      .where("induction_start_date IS NULL OR induction_start_date < ?", latest_induction_start_date)
+      .where("induction_start_date IS NULL OR induction_start_date < make_date(cohorts.start_year, 9, 1)")


Why do the induction start date of the participant matter to archive them or not?

This was a requirement from Nathan, I'm not sure of the reasoning behind it but I'll follow up with him to check:

I think if they have an induction start date of 1/9/2021 or later, we should exclude them and not archive them. We should only archive those where there is no induction start date or it is earlier then 1/9/2021

here's the reasoning:

Archiving is primarily intended for participants who were registered in error. The two typical cases of this are a) where the ECT wasn't actually an ECT but was instead starting their initial teacher training and b) the ECT had already started induction before the current funding and framework kicked in.

Where an ECT did actually start induction on or after 1 Sep 2021, the training needs are trumped by the school funding needs. The record will have been used to inform school payments, so we should not archive any such records

ltello · 2024-05-25T09:51:17Z

app/models/participant_profile/mentor.rb

-  def self.archivable(for_cohort_start_year:, restrict_to_participant_ids: [])
-    super(for_cohort_start_year:, restrict_to_participant_ids:)
+  def self.archivable(restrict_to_participant_ids: [])
+    super(restrict_to_participant_ids:)
      .where(mentor_completion_date: nil)
      .where.not(id: InductionRecord.where.not(mentor_profile_id: nil).select(:mentor_profile_id).distinct)


This seems to exclude mentors that once were assigned to an ECT no matter if they currently have any assignment or not.
Is this what we want?

I believe so, here's the full spec:

- 2021 cohort (according to their schedule -> cohort) - Profile has no declarations where declaration state is one of eligible/payable/paid/awaiting_clawback/clawed_back - Participant has only FIP induction records - Mentor funding end date is NULL - No mentees in induction records

That 5th condition (No mentees in induction records) is not in Nathan's doc. (At least I can't find it).
In any case, can it mean the mentor is not CURRENTLY assigned to any mentee?

Yeah it looks like that requirement hasn't made it into the Word doc for some reason, but this was the ask from Nathan:

Could we add a criterion that the mentor has no links to any ECT in any of their induction period records?

Ok, so in that case, it is clear that he wants no mentor-mentee at any time.

ltello · 2024-05-25T09:53:47Z

app/services/change_schedule.rb

+    return false unless attempt_to_change_cohort_leaving_billable_declarations
+    return true if participant_profile.eligible_to_change_cohort_and_continue_training?(in_cohort_start_year: cohort.start_year)
+
+    participant_profile.eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort_start_year: cohort.start_year)


Suggested change

participant_profile.eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort_start_year: cohort.start_year)

participant_profile.eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort: cohort)

...if we decide to make the change suggested above to the signature of this method

ltello · 2024-05-25T10:07:47Z

app/services/change_schedule.rb

@@ -9,6 +9,7 @@ class ChangeSchedule
  attribute :course_identifier
  attribute :schedule_identifier
  attribute :cohort, :integer
+  attribute :attempt_to_change_cohort_leaving_billable_declarations, :boolean, default: false


Do we need this security attribute?
Nothing will change until we set a cohort as payments-frozen in the database, no?

We need this because the ChangeSchedule service is exposed via the lead provider API and we don't necessarily want them to be able to use it to perform this kind of cohort change yet

That's fine but if we don't set 2021 as payments-frozen yet, even if they try that cohort change the code we have written won't allow it (even if we remove attempt_to_change_cohort_leaving_billable_declarations)
In any case, fine by me if you want to keep it for extra security reasons.

Yeah I understand that it won't let it happen until the payments frozen is switched on, but we want this as an extra safety net as it looks like we may not want to allow providers to use the service for this purpose even when the cohort is frozen (but we want the option to trigger it via other means, so that's where the flag will come in)

We want to allow ECTs and mentors to change schedule/cohort even if they have billable declarations under certain scenarios. The participants eligible are identified by `eligible_to_change_cohort_and_continue_training`. By default the service will not allow eligible participants to transfer; it needs to be called explicitly with `attempt_to_transfer_leaving_billable_declarations` set to `true`. This is to prevent lead providers performing these changes for now. Participants that change cohort using this mechanism should be able to change back to their original cohort providing no billable declarations were created against the new cohort.

Tweaks the logic to change cohort and continue training to aign with the schools team. This means instead of checking for +/- 3 years from their current cohort, we will instead let them transfer only ot the active registration cohort and only back to a cohort that is payments frozen and for which they have billable declarations.

cwrw · 2024-05-28T13:58:59Z

app/services/change_schedule.rb

+    changing_cohort = new_schedule.cohort != participant_profile.schedule.cohort
+
+    if changing_cohort && can_change_cohort_leaving_billable_declarations?
+      participant_profile.toggle!(:cohort_changed_after_payments_frozen)


toggle! skips validation checks, is this intentional?
Also was wondering if we could somehow have this logic into a different method to update_school_cohort_and_schedule! to make the intention clear?

No, good catch I'll change this 👍

cwrw

thanks @ethax-ross for this, really cleanly implemented! I left a few minor comments, and we discussed in person possibly moving shared validations to validators but after this is merged.

app/models/participant_profile/ecf.rb

app/services/change_schedule.rb

- Pass in current cohort to `eligible_to_change_cohort_back_to_their_payments_frozen_original?` - Remove unused method `cohort_start_years_delta` - Select current cohort from `relevant_induction_record`

cwrw

Thanks @ethax-ross 💯 this took a lot of 🧠 to get through but very cleanly done

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from 81ddb5a to 5c61e21 Compare May 23, 2024 14:03

ethax-ross temporarily deployed to review May 23, 2024 14:08 — with GitHub Actions Inactive

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from 5c61e21 to 6859d40 Compare May 23, 2024 14:26

ethax-ross temporarily deployed to review May 23, 2024 14:31 — with GitHub Actions Inactive

ethax-ross requested a deployment to review May 23, 2024 14:39 — with GitHub Actions Abandoned

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from 6859d40 to c17ec0d Compare May 23, 2024 15:03

ethax-ross temporarily deployed to review May 23, 2024 15:08 — with GitHub Actions Inactive

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from c17ec0d to 28db978 Compare May 24, 2024 08:54

ethax-ross temporarily deployed to review May 24, 2024 08:58 — with GitHub Actions Inactive

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from 28db978 to eb7bb41 Compare May 24, 2024 10:27

ethax-ross temporarily deployed to review May 24, 2024 10:32 — with GitHub Actions Inactive

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from eb7bb41 to 95fae5f Compare May 24, 2024 10:59

ethax-ross temporarily deployed to review May 24, 2024 11:04 — with GitHub Actions Inactive

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from 95fae5f to 7ab766a Compare May 24, 2024 12:21

ethax-ross temporarily deployed to review May 24, 2024 12:25 — with GitHub Actions Inactive

ethax-ross temporarily deployed to review May 24, 2024 13:23 — with GitHub Actions Inactive

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from 5f1a650 to f0ca8df Compare May 24, 2024 14:06

ethax-ross marked this pull request as ready for review May 24, 2024 14:06

ethax-ross requested review from ltello and a team May 24, 2024 14:07

ethax-ross temporarily deployed to review May 24, 2024 14:11 — with GitHub Actions Inactive

ltello reviewed May 25, 2024

View reviewed changes

ethax-ross temporarily deployed to review May 28, 2024 08:26 — with GitHub Actions Inactive

ltello self-requested a review May 28, 2024 10:22

ltello approved these changes May 28, 2024

View reviewed changes

ethax-ross added 3 commits May 28, 2024 14:14

Pass cohort into eligibility methors rather than start year

dbdd98d

ethax-ross force-pushed the 3062-relax-change-schedule-validation branch from ba07b97 to dbdd98d Compare May 28, 2024 13:15

ethax-ross temporarily deployed to review May 28, 2024 13:19 — with GitHub Actions Inactive

cwrw reviewed May 28, 2024

View reviewed changes

app/models/participant_profile/ecf.rb Outdated Show resolved Hide resolved

app/services/change_schedule.rb Outdated Show resolved Hide resolved

app/services/change_schedule.rb Outdated Show resolved Hide resolved

cwrw reviewed May 28, 2024

View reviewed changes

app/services/change_schedule.rb Outdated Show resolved Hide resolved

ethax-ross added 2 commits May 29, 2024 11:01

Minor updates to ChangeSchedule service

fab98ef

- Pass in current cohort to `eligible_to_change_cohort_back_to_their_payments_frozen_original?` - Remove unused method `cohort_start_years_delta` - Select current cohort from `relevant_induction_record`

Rename flag to allow schedule changes on frozen cohorts

7032003

ethax-ross temporarily deployed to review May 29, 2024 12:46 — with GitHub Actions Inactive

cwrw approved these changes May 29, 2024

View reviewed changes

ethax-ross added this pull request to the merge queue May 30, 2024

Merged via the queue into main with commit 886c54d May 30, 2024
37 checks passed

ethax-ross deleted the 3062-relax-change-schedule-validation branch May 30, 2024 08:48

ethax-ross temporarily deployed to review May 30, 2024 08:48 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relax validation on ChangeSchedule service for certain participants #4838

Relax validation on ChangeSchedule service for certain participants #4838

ethax-ross commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024

ltello May 25, 2024

ltello May 25, 2024

ltello May 25, 2024

ltello May 25, 2024

ltello May 25, 2024 •

edited

Loading

ethax-ross May 28, 2024

ltello May 28, 2024 •

edited

Loading

ltello May 25, 2024 •

edited

Loading

ethax-ross May 28, 2024

ethax-ross May 28, 2024

ltello May 25, 2024 •

edited

Loading

ethax-ross May 28, 2024

ltello May 28, 2024

ethax-ross May 28, 2024

ltello May 28, 2024

ltello May 25, 2024

ltello May 25, 2024

ltello May 25, 2024

ethax-ross May 28, 2024

ltello May 28, 2024

ethax-ross May 28, 2024 •

edited

Loading

cwrw May 28, 2024

ethax-ross May 29, 2024

cwrw left a comment

cwrw left a comment

		def eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort_start_year:)
		to_cohort = Cohort.find_by_start_year(to_cohort_start_year)

	participant_profile.eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort_start_year: cohort.start_year)
	participant_profile.eligible_to_change_cohort_back_to_their_payments_frozen_original?(to_cohort: cohort)

Relax validation on ChangeSchedule service for certain participants #4838

Relax validation on ChangeSchedule service for certain participants #4838

Conversation

ethax-ross commented May 23, 2024 • edited Loading

Context

Changes proposed in this pull request

Guidance to review

github-actions bot commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltello May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltello May 28, 2024 • edited Loading

Choose a reason for hiding this comment

ltello May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltello May 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethax-ross May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwrw left a comment

Choose a reason for hiding this comment

cwrw left a comment

Choose a reason for hiding this comment

ethax-ross commented May 23, 2024 •

edited

Loading

ltello May 25, 2024 •

edited

Loading

ltello May 28, 2024 •

edited

Loading

ltello May 25, 2024 •

edited

Loading

ltello May 25, 2024 •

edited

Loading

ethax-ross May 28, 2024 •

edited

Loading