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-1077: sit participants in the right cohort on school transfers #2602

Merged
merged 4 commits into from
Oct 28, 2022
Merged

CST-1077: sit participants in the right cohort on school transfers #2602

merged 4 commits into from
Oct 28, 2022

Conversation

ltello
Copy link
Contributor

@ltello ltello commented Oct 10, 2022 •
edited
Loading

Context

Changes proposed in this pull request

  • "Calculate" the right cohort to transfer a participant (based on the induction taken or not by the participant in previous schools) ASAP in the add participant and transfer journeys
  • Redirect the user to the "school setup cohort" journey if the cohort to add/transfer a participant is not been set up yet.
  • Redirect the user back to their initial journey after setting up the missing cohort for the school.
  • Finally, sit the participant in the right cohort.

Guidance to review

@ltello ltello force-pushed the CST-1077-school-transfers-must-sit-in-the-right-cohort branch 7 times, most recently from a122ac5 to 09ddfc4 Compare October 12, 2022 13:43
@github-actions
Copy link

Created review app at https://ecf-review-pr-2602.london.cloudapps.digital

@github-actions
Copy link

Smoke tests passed against the review app.

@ltello ltello force-pushed the CST-1077-school-transfers-must-sit-in-the-right-cohort branch 12 times, most recently from 0e42369 to 67e60d0 Compare October 15, 2022 06:23
@ltello ltello requested review from tonyheadford and a team and removed request for tonyheadford October 17, 2022 08:11
peteryates
peteryates reviewed Oct 17, 2022
View reviewed changes
app/views/schools/choose_programme/show.html.erb Outdated
"https://www.gov.uk/government/publications/early-career-framework-reforms-overview/early-career-framework-reforms-overview",
target: :_blank,
rel: "noopener noreferrer",
class: "govuk-link govuk-link--no-visited-state" %>
Copy link
Member

@peteryates peteryates Oct 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not really important here but you can pass no_visited_state: true to govuk_link_to for the same effect.

https://govuk-components.netlify.app/helpers/link/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great thanks. Much less verbose!.

Copy link
Member

Choose a reason for hiding this comment

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

After Christmas it will support new_tab: true which means you'd be able to drop target: :_blank, rel: "noopener noreferrer" too, but I need to wait for Ruby 2.7 support to drop before merging.

peteryates
peteryates reviewed Oct 17, 2022
View reviewed changes
app/views/schools/add_participants/target_school_cohort_not_set.html.erb Outdated
</h1>

<p class="govuk-body">
Before you can register <%= add_participant_form.full_name.titleize %> for ECF-based training at your school,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we shold be titleizing peoples names here. It might work for some but will break others (McDonald vs Mcdonald)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I copied it from a similar view. There must be loads across the code.
I will change at least this one. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'll have a look for others. Thanks for the heads up.

peteryates
peteryates reviewed Oct 17, 2022
View reviewed changes
Copy link
Member

@peteryates peteryates left a comment

Choose a reason for hiding this comment

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

Logic all seems sensible to me. Left a couple of minor comments.

It feels like there's a lot going on in AddParticipantsController and TransferringParticipantsController. Perhaps this could be reworked slightly into the service layer as a follow up?

@ltello ltello force-pushed the CST-1077-school-transfers-must-sit-in-the-right-cohort branch 2 times, most recently from f2d2066 to 3907701 Compare October 17, 2022 13:03
@ltello ltello requested a review from peteryates October 17, 2022 13:04
@ltello ltello force-pushed the CST-1077-school-transfers-must-sit-in-the-right-cohort branch from 3907701 to 330db4a Compare October 19, 2022 15:04
@ltello ltello force-pushed the CST-1077-school-transfers-must-sit-in-the-right-cohort branch from 330db4a to b1d5251 Compare October 27, 2022 21:19
@ltello ltello merged commit 22c71f3 into develop Oct 28, 2022
@ltello ltello deleted the CST-1077-school-transfers-must-sit-in-the-right-cohort branch October 28, 2022 07:56
@ltello ltello restored the CST-1077-school-transfers-must-sit-in-the-right-cohort branch December 1, 2022 21:47
@ltello ltello deleted the CST-1077-school-transfers-must-sit-in-the-right-cohort branch July 2, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lazydevorg lazydevorg lazydevorg approved these changes

@peteryates peteryates Awaiting requested review from peteryates

@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.
3 participants
@ltello @peteryates @lazydevorg