feat: standardize status change activity logging#741
Conversation
c361f5e to
a9d8512
Compare
a9d8512 to
0f50d5e
Compare
RussH
left a comment
There was a problem hiding this comment.
Thanks for another great PR! Definitely like separating the dedicated status-change flow from the general activity flow.
I did spot a few areas that I think should be addressed before merge:
-
Potential regression around “Placed” on filled job orders
In the previous combined flow there was a guard preventing a candidate being set to Placed when the job order was already filled. I can see that was removed from the old path, but I don’t see the equivalent protection added back into the new dedicated changeStatus flow. That looks like it could allow a filled job order to have another candidate marked as placed. -
JS escaping in the new modal
In ChangeStatusModal.tpl, several PHP values are inserted into JavaScript strings using manual str_replace() escaping for single quotes only. Lets try and get it done completely - can we handle these values with json_encode() rather than hand-escaping them?
Worth calling out - we shoud note in the PR ACL / permission migration
Splitting pipelines.addActivityChangeStatus into pipelines.addActivity and pipelines.changeStatus changes permissions Any release notes should call out a manual check for people using ACL, as they may unexpectedly lose access to one of the new actions.
If we address these, all good!
|
Non-blocking observation: this may appear as an unexpected merge artifact because |
There was a problem hiding this comment.
Thanks — understood, and I follow the point about this likely being a rename issue rather than a genuine code problem.
That said, GitHub is still showing the PR as conflicted, so please rebase the branch onto current master and once the conflicts are resolved , I’m happy to give the final state a quick check.
|
Meanwhile there is a real conflict at test/features/GET_POST_requestsSecurity.feature – will solve it! |
b0ce4f3 to
e2b659f
Compare
RussH
left a comment
There was a problem hiding this comment.
Thanks for the follow-up fixes — the two issues flagged look addressed now.
The restored placed guard is back in, and switching the modal JS values over to json_encode() is the right fix.
Only minor note from my side is that the ACL split should be called out for anyone using custom permission mappings.
Otherwise this looks good to me.
Summary
This PR standardizes pipeline status changes by separating the dedicated
changeStatusflow from the generic activity flow and by using the status change activity types introduced in #737.Instead of relying on a manually editable activity note during a status change, the resulting activity entry is now derived from the selected status change type. This makes status change logging more consistent and reduces manual variation in these entries.
To support this behavior, the previous combined
addActivityChangeStatusflow is split into dedicatedaddActivityandchangeStatusactions and templates. The related candidate and job order pipeline UI, permissions and tests are updated accordingly.Motivation
The main goal of this change is to make status changes more standardized and less dependent on manual note editing.
Previously, changing a status was tied to a flow where the activity note could be freely adjusted, which made status change logging less consistent. By moving status changes into a dedicated flow and using the status change types added in #737, the system can create more predictable and uniform activity entries for these actions.
Splitting the modal is therefore not the primary objective by itself, but a necessary step to support a more guided and standardized status change workflow.