fix: Fix validation modal doesn't update custom metrics #15671
Conversation
|
Your preview environment pr-15671 has been deployed. Preview environment endpoints are available at: |
|
Preview environment logs can be accessed at: https://console.cloud.google.com/logs/query;query=resource.labels.namespace_name%3D%22pr-15671%22;duration=PT30M?project=lightdash-previews |
3f027dc to
ccffb2d
Compare
|
Preview environment logs can be accessed at: https://console.cloud.google.com/logs/query;query=resource.labels.namespace_name%3D%22pr-15671%22;duration=PT30M?project=lightdash-previews |
There was a problem hiding this comment.
I've only updated docs in this file. I found the previous ones confusing, so let me know if this makes sense 🙏
There was a problem hiding this comment.
Much clearer now. thank you 🙏
| const searchTerms = Object.values({ | ||
| from: addSuffixIfPrefix(nameChanges.from, isPrefix), | ||
| fromReference: addSuffixIfPrefix(nameChanges.fromReference, isPrefix), | ||
| fromFieldName: addSuffixIfPrefix(nameChanges.fromFieldName, isPrefix), |
There was a problem hiding this comment.
I understand looking at fromReference, but why fieldName ? if we don't need it, I'd suggest to remove it to avoid false positives.
There was a problem hiding this comment.
I thought of using fieldName too as it's only available if the RenameType is Field. I think that's enough to avoid false positives when updating models?
| name: 'Customer_ID_min_of_Customer_ID_10', | ||
| label: 'Min of Customer id 10', | ||
| description: | ||
| 'Min of Min of Customer id on the table Customers with filters customers.Customer_ID', |
There was a problem hiding this comment.
that's true and intentional! This description was autogenerated, and I wanted to include it here in the tests to make sure we only update the required values and not user defined things, like titles and descriptions, does it make sense?
ccffb2d to
d50572b
Compare
| // These are for ${TABLE}.field references like in additionalMetrics | ||
| // We only update here if the str is the same as the expected fromReference | ||
| // to avoid updating other references | ||
| if (str.startsWith('${TABLE}')) { | ||
| const strWithoutTable = str.split('.')[1]; | ||
| const fromReferenceWithoutTable = fromReference.split('.')[1]; | ||
| const toReferenceWithoutTable = toReference.split('.')[1]; | ||
| return str.replace( | ||
| fromReferenceWithoutTable, | ||
| toReferenceWithoutTable, | ||
| ); | ||
| if (strWithoutTable === fromReferenceWithoutTable) { | ||
| return str.replace( | ||
| fromReferenceWithoutTable, | ||
| toReferenceWithoutTable, | ||
| ); | ||
| } | ||
| return str; | ||
| } |
There was a problem hiding this comment.
@ZeRego This now covers the case you discover while reviewing.
This function was updating the value no matter what. We are now checking that the string to update actually matches the fromReference value before updating
| // Only mark changes if there was actually an update. There are renameFunctions that won't change the values if not needed | ||
| const hasChanges = JSON.stringify(chart) !== JSON.stringify(updatedChart); |
There was a problem hiding this comment.
Another false positive here. The hasChanges flag was always set to true whenever the replace function was called, regardless of whether it actually changed anything. We only want to change this flag if there was actually a change, so we do a final string-to-string comparison to set it instead.
|
Preview environment logs can be accessed at: https://console.cloud.google.com/logs/query;query=resource.labels.namespace_name%3D%22pr-15671%22;duration=PT30M?project=lightdash-previews |
Merge activity
|
|
🎉 This PR is included in version 0.1780.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes: #15652
Description:
Added support for renaming fields in custom metrics within saved charts. Updated the
renameSavedChartfunction to properly handle field name changes in additional metrics, including their SQL expressions and filters. Added test cases with mock data to verify the functionality works correctly for charts containing custom metrics.Videos
Before
Screen.Recording.2025-07-02.at.15.30.04.mov
After
Screen.Recording.2025-07-02.at.15.31.14.mov