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

[CPDNPQ-2435] Add "in review" applications screen #2142

Merged
merged 3 commits into from
Jan 27, 2025

Conversation

rwrrll
Copy link
Contributor

@rwrrll rwrrll commented Jan 22, 2025

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDNPQ-2435

UCD prototype: https://npq-prototype.herokuapp.com/admin-v3/registrations-review

Screenshot of the new screen added by this PR:

image

Copy link
Contributor

@rwrrll rwrrll marked this pull request as ready for review January 22, 2025 14:23
@rwrrll rwrrll requested a review from a team as a code owner January 22, 2025 14:23

private

def employment_types
Copy link
Contributor

Choose a reason for hiding this comment

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

should this array be on the Application model?

Copy link
Contributor Author

@rwrrll rwrrll Jan 23, 2025

Choose a reason for hiding this comment

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

I think so, yes, but employment_type is a bit of a mess throughout the app with string checking all over the place... I feel like the correct solution is to formally define it as an enum or something once and for all and then tidy up all the corners of the app where it's used, but I'd prefer to do that as a separate ticket because it needs a bit of care and BA attention to do properly, whereas this ticket is needed for BAU in a few weeks I believe.

@jebw are you up for scheduling an employment_type clean up ticket, or would you prefer we hold up this ticket and roll it in now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to also mention here while it's in my mind, referred_by_return_to_teaching_adviser is a string that can be nil... it should probably be a NOT NULL boolean, that should also be captured for fixing at some point!

Copy link
Contributor

Choose a reason for hiding this comment

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

@rwrrll Feel free to write a ticket - maybe schedule it for a couple of sprints time and we can evaluate in sprint review whether we have adequate capacity to do with it

Put it under uptime and maintenance epic, thanks


def employment_types
%w[
hospital_school
Copy link
Contributor

Choose a reason for hiding this comment

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

do we also need lead_mentor_for_accredited_itt_provider in this array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, amazing catch, thanks! 🦅 👁️

Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@rwrrll rwrrll added this pull request to the merge queue Jan 27, 2025
Merged via the queue into main with commit 513ac26 Jan 27, 2025
18 checks passed
@rwrrll rwrrll deleted the 2435-edge-case-applications branch January 27, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants