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.

Sign up for GitHub

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

Jump to bottom

CST-2575: closing 2021 allow cohort changes for participants from payments-frozen cohorts #4831

Merged
merged 11 commits into from
May 30, 2024
Merged

CST-2575: closing 2021 allow cohort changes for participants from payments-frozen cohorts #4831

merged 11 commits into from
May 30, 2024

Conversation

ltello
Copy link
Contributor

@ltello ltello commented May 22, 2024 •
edited
Loading

Context

Accomodate Induction::AmendParticipantCohort service object to accept participants coming from payments_frozen cohorts.

This piece of code will eventually be called by all the features/journeys/events we are updating as part of the Closing 21 cohort implementation.

Ticket

Changes proposed in this pull request

Guidance to review

@github-actions GitHub Actions
Copy link

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

@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from 0dfae92 to 4f95dea Compare May 22, 2024 18:31
@ltello ltello marked this pull request as ready for review May 22, 2024 18:31
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from 73dfb43 to b48d134 Compare May 23, 2024 07:25
steventux
steventux approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@steventux steventux left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple of small suggestions, nothing blocking though.

config/locales/en.yml Outdated Show resolved Hide resolved
app/models/participant_profile/ect.rb Outdated Show resolved Hide resolved
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from b71f26a to 37c9d4d Compare May 24, 2024 12:02
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch 2 times, most recently from 4d687dd to e7f3f15 Compare May 25, 2024 10:16
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from e7f3f15 to ba983cb Compare May 25, 2024 10:29
@ltello ltello changed the title Cst 2575 closing 2021 allow cohort changes for participants from payments closed cohorts CST-2575: closing 2021 allow cohort changes for participants from payments closed cohorts May 28, 2024
@ltello ltello changed the title CST-2575: closing 2021 allow cohort changes for participants from payments closed cohorts CST-2575: closing 2021 allow cohort changes for participants from payments-frozen cohorts May 28, 2024
cwrw
cwrw reviewed May 28, 2024
View reviewed changes
app/validators/participant_profile_active_validator.rb
end

def active?(participant_profile)
participant_profile&.active_record? && participant_profile&.training_status_active?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Im curious as to why this changed, and we no longer want training status active? I've checked our service and we prevent changing things if a participant is withdrawn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the question we made to Nathan about setting training_status: :active when executing the cohort change via this tool we finally concluded to leave training_status as it was originally.
So why required the participants to be training_status: :active?

Also it feels to me that the original training status refers to that training period. Why would that block starting a new training period somewhere else?

In any case, happy to reinstate it if needed

Copy link
Collaborator

Choose a reason for hiding this comment

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

having a think maybe we can keep it as it was currently? as I think resetting the training status is quite different than the validation itself. Just erroring on the side of caution as it's already there, if we later find it an issue maybe we can remove it?

cwrw
cwrw reviewed May 28, 2024
View reviewed changes
Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

thanks @ltello very minor comments/questions but generally looks great , and after @ethax-ross pr is merged we can align the methods :)

@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from ba983cb to bdc3c58 Compare May 28, 2024 22:31
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch 2 times, most recently from f6f333b to a6747a6 Compare May 29, 2024 14:07
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from a6747a6 to d7fa262 Compare May 29, 2024 14:26
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from d7fa262 to d6fdb6c Compare May 30, 2024 11:14
cwrw
cwrw approved these changes May 30, 2024
View reviewed changes
Copy link
Collaborator

@cwrw cwrw left a comment

Choose a reason for hiding this comment

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

thanks @ltello 🙌

config/locales/en.yml Outdated Show resolved Hide resolved
config/locales/en.yml Outdated Show resolved Hide resolved
ltello and others added 11 commits May 30, 2024 14:45
@ltello
CST-2575: update amend cohort tool to cope with participants moved af…
2e33f95 
…ter their cohort is frozen for payments
@ltello
CST-2575: refactor the field to flag cohort changes
f976191
@ltello
CST-2575: simpler approach to flag participants move due to frozen co…
ca11226 
…horts
@ltello
CST-2575: improved doc and error messages
a109aa1
@ltello @steventux
Update app/models/participant_profile/ect.rb
cf42706 
Co-authored-by: Steve Laing <[email protected]>
@ltello @steventux
Update config/locales/en.yml
f3a87b3 
Co-authored-by: Steve Laing <[email protected]>
@ltello
CST-2575: fix bug after refactor
6481a6b
@ltello
CST-2575: reuse methods to detect eligible participants moved out of …
a2ca612 
…payments-frozen cohorts
@ltello
CST-2575: allow payment-frozen participants to be transferred back to…
c443635 
… their original cohort
@ltello
CST-2575: allow participants to be sit in payments-frozen cohort
748e08a
@ltello
CST-2575: remove unused locales
00efb07
@ltello ltello force-pushed the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch from 7be3153 to 00efb07 Compare May 30, 2024 13:45
@ltello ltello added this pull request to the merge queue May 30, 2024
Merged via the queue into main with commit 9ef4faa May 30, 2024
31 checks passed
@ltello ltello deleted the CST-2575-closing-2021-allow-cohort-changes-for-participants-from-payments-closed-cohorts branch May 30, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethax-ross ethax-ross ethax-ross left review comments

@cwrw cwrw cwrw approved these changes

@steventux steventux steventux approved these changes

@tonyheadford tonyheadford Awaiting requested review from tonyheadford

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ltello @steventux @cwrw @ethax-ross