Open petertgiles opened 11 months ago
What splits qualified from screened out? As in the determining field?
Currently, draft deletion is a hard deletion and I think it was intentionally set as such. Is there a reason to soft delete them instead?
Would this status then be a virtual property obtained from an accessor on the pool candidate model? Computed from the various fields that returns an enum?
Where will status details or differentiator live? Like what splits placed into casual/term/indeterminate or split up the pending group?
Backing up the status could just come from renaming the current pool_candidate_status
to pool_candidate_status_legacy
instead of dropping it after copying it.
There are multiple options for timestamps updates. Like:
Submitted from the client
The injectNow directive
An setter/mutator that exists to tack on a model change.
What splits qualified from screened out? As in the determining field?
I don't think we're going to be able to use just a derived status because of that split. I think it will be actual status
and status_detail
columns in the database. I still think we should have the timestamps because they're good for audits and we sometimes use them for filtering. For example, testing that assessed_at is set will filter for DRAFT, PENDING, QUALIFIED, or SCREENED_OUT.
Currently, draft deletion is a hard deletion and I think it was intentionally set as such. Is there a reason to soft delete them instead?
Hmm, maybe we don't use soft deletes at all for this model then? Maybe deleted_at and the SoftDeletes trait should be dropped.
Would this status then be a virtual property obtained from an accessor on the pool candidate model? Computed from the various fields that returns an enum? Where will status details or differentiator live? Like what splits placed into casual/term/indeterminate or split up the pending group?
Yeah, real status and status_detail columns, probably with enums.
Backing up the status could just come from renaming the current
pool_candidate_status
topool_candidate_status_legacy
instead of dropping it after copying it.
Yep, that would be a good way to do it.
There are multiple options for timestamps updates.
Yeah, good list! Except for "Submitted from the client" - I wouldn't trust the client to tell us the right time. We should always expect malicious requests from the client and guard against them.
One other option to add to the list would be Observers, like api/app/Observers/PoolCandidateObserver.php.
If we added a field for just the assessment decision, like "SUCCESSFUL" or "UNSUCCESSFUL" we could get away without a "real" status column. We would still need a real status detail column though.
I still think we should have the timestamps because they're good for audits and we sometimes use them for filtering.
I wonder now if we can get away with relying on the activity log: https://github.com/GCTC-NTGC/gc-digital-talent/pull/8412. That, plus status
and status_detail
columns might give us everything we want, without the need for <action>_at
columns.
Maybe deleted_at and the SoftDeletes trait should be dropped.
Maybe. But just because deleting a draft doesn't use soft-deletes, doesn't mean deleting a submitted application shouldn't.
pool_candidate_status_legacy
I like it 👍
There are multiple options for timestamps updates.
I'll echo what @petertgiles already said - never let the client tell you the timestamp! Recording that is the server's job 😆 . I want to point out that in your example, @vd1992, a date is being used as a filter for a query. not saved with a mutation.
If we're using activity log (#8412) we may not even need timestamp columns any more! If we decide they're still useful, I'm in favour of letting Laravel Observers handle them.
If we added a field for just the assessment decision, like "SUCCESSFUL" or "UNSUCCESSFUL" we could get away without a "real" status column.
Good point. If we stick with timestamp columns, then the "real" status may actually be a computed status, based on SUBMITTED_AT, SUCCESSFUL_AT, UNSUCCESSFUL_AT, and PLACED_AT.
The opposite approach is to replace all the timestamp columns with an actual real status column, and check the activity log when we need dates.
Hm, I still like storing dates for important events. Occurrences like application submission seem very useful and likely to be displayed in different places.
Fetching such a date from the logging would be tricky but doable by using certain fields as proxy. Signature field would be updated once only upon submission, so combing through the updates events on an application and you can find the timestamp for when the signature was touched. But that would be messier in comparison and not always reliable since you are making assumptions
I want to point out that in your example
True, true
The opposite approach is to replace all the timestamp columns with an actual real status column
Depends a lot on how status should be controlled, a real status column enables user's to directly control and override things. Derived means users may not override with customs statuses for specific cases. I think part of the reason we have a status field to begin with is users wanted specific cases and the ability to set as desired which computed won't allow
Current accessor that had its overrides and logic commented out to just return the field
Existing fields are: submitted_at expiry_date (not a timestamp, will this pose trouble?) archived_at suspended_at deleted_at
To add timestamps: removed_at assessed_at placed_at
Strings using enums: status, 4 options assessment_reason, two options placement_type, three options removal_reason, four options
Text fields to add: removal_reason_other
Status override/derived field to be removed Migration when setting scope fields, just inject the current time I imagine
How will filtering by status now work? The scopes can be written up simply enough but how will the Client-API connection look? What will users see?
Where will status be set automatically and where will it be controlled? For instance, draft is upon creation, pending in the submit application mutation, but where will the switch to qualified/unqualified happen?
Is creating new mutations for the new fields and where they will live in scope? Like the removed field, updating it.
We've decided to hold off on this and try to launch Record of Decision with the current implementation for application statuses, to reduce the complexity of this release. We will map the current implementation of statuses to the new UI in the frontend. Later, we can come back to this and refactor the status implementation.
@tristan-orourke is this still something we intend to do?
Yeah, this is definitely something I still want to do. We should come back to look at it after we finally do #10218
This comment was automatically written by the Blocking Issues bot, and this PR will be monitored for further progress.
✨ Feature
We would like update how pool candidate statuses work to make it simpler and compatible with Record of Decision.
:older_adult: Old Issues
3659, #3947
🕵️ Details
We will switch our status column and enum to be much simpler: only four states to represent the lifecycle. It will be a "real" column, not derived.
We will have only two timestamps to record the movement through those four states.
To augment those there will be several timestamps to support scopes and business data.
And several more detail fields:
🎨 Design File
🙋♀️ Proposed Implementation
:car: Migration Plan
:card_file_box: Card Titles
Refer to https://docs.google.com/spreadsheets/d/16OjYysdlrsSP8IrQJLL7UXYzPA6iskJnlRzeMQ8gDJc/edit#gid=70566376 for the card body and link texts.
:question: Question and Answer
Why is there no placed/hired status?
In a lot of cases we treat someone as placed the same way as qualified, for example when placed casually. When placed, we will leave users in the Qualified status and automatically set them as suspended when term and indeterminate. If the user wishes to change the suspended timestamp themselves they can.
✅ Acceptance Criteria
pool_candidate_status_legacy
to make rollback possible🛑 Blockers