-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Added better handling of information of asset history #13485
Conversation
PR Summary
|
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 was trying to come up with a way to check for camel case or snake case and just transform into title case with spaces, but I can't see how that'd work with the translation strings, so looks good to me!
added |
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 good!
One thing to consider is that we're not eager loading the related data (location, asset model, company, or supplier) so we're running more queries than necessary but I don't think we can easily eager load them in the transformer (that should happen before it hits the transformer).
I'm just pointing that out. I don't think it holds back this PR but is something we'll probably want to address in the future.
This looks great - thank you, @Godmartinz! |
@Godmartinz When testing this locally, I'm getting a 500 on history:
(I just changed the serial number, nothing else) Please make this your top priority today. I can't merge to master until we've got this handled, or I'll have to cherry pick this merge out. |
(Also, whatever the issue is here, it would probably trigger elsewhere in the new code, so please check) TY! |
I also wonder what the query impact here might be - none of those model/company/etc queries are being eager/lazy loaded, so in the Activity Report, with many records displayed, that could be pretty hairy. Right now, I'd just like the 500 to be mitigated, but we should probably revisit this soon and make sure we're checking query counts, etc on the Activity Report page |
Description
This adds
changedInfo()
to the action log transformer, which will interpret changes in a more readable way.array keys have been renamed to be more readable (e.g
rtd_location_id
is nowDefault Location
) in the History tab of an asset. This information will be displayed if you edit/update a single asset or multiple assets at once.If an asset never had a supplier, default location, company, or location already assigned to it, it will be noted as 'unassigned' (example below). If supplier, default location, company, or location are removed it will go back to unassigned as well.
Before:
After:
Removed the Supplier:
Fixes #sc-14441
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: