-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fixed timestamp in action log for bulk accessory check in #16489
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
Conversation
PR Summary
|
|
Ignore the failing test - this seems like a test race condition and is unrelated. |
|
Ah yeah...that test is super flaky. I plan on working on making it more stable at some point. |
snipe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should pull the consumable checkin? We don't allow checkin anywhere else. Should be a delete if anything.
| $logAction->target_id = $consumableUserRow->assigned_to; | ||
| $logAction->target_type = User::class; | ||
| $logAction->created_at = auth()->id(); | ||
| $logAction->created_by = auth()->id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't check in consumables though? Should we pull this method altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this was carried over in #15221 from previous behavior but yeah...we don't actually check in consumables.
Do we want to purge the action log of existing consumable checkins? If so I think we should do another PR that includes removing this and the migration to do the purge...
I'm ok if you'd rather me just pull it from here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll merge this one as-is, to fix the current issue, but I think another PR + migration should come soon after. To be clear though, deleting a consumable (FCO) is a recordable (loggable) event. Deleting the consumable join to a person shouldn’t be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
10-4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference: #16494
This PR fixes an issue where an incorrect timestamp would be written in the action log for accessory check-in via bulk check-in on users.