fix(asset): calibration forecast timezone issues on datalinks #111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mure
approved these changes
Nov 14, 2024
mure
pushed a commit
that referenced
this pull request
Nov 14, 2024
## [3.43.5](v3.43.4...v3.43.5) (2024-11-14) ### Bug Fixes * **asset:** calibration forecast timezone issues on datalinks ([#111](#111)) ([f5209e8](f5209e8))
|
🎉 This PR is included in version 3.43.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
saradei-ni
reviewed
Nov 15, 2024
| @@ -101,7 +101,8 @@ export class CalibrationForecastDataSource extends AssetDataSourceBase { | |||
| return null; | |||
| } | |||
|
|
|||
| processResultsGroupedByTime(result: DataFrameDTO, timeGrouping: AssetCalibrationForecastKey) { | |||
| processResultsGroupedByTime(result: DataFrameDTO, timeGrouping: AssetCalibrationForecastKey, options: DataQueryRequest) { | |||
| const originalValuesMap = this.constructDataLinkValuesMap(result, options); | |||
There was a problem hiding this comment.
As the method name is constructDataLinkValuesMap, maybe dataLinkValuesMap would be more suitable name.
saradei-ni
reviewed
Nov 15, 2024
| @@ -101,7 +101,8 @@ export class CalibrationForecastDataSource extends AssetDataSourceBase { | |||
| return null; | |||
| } | |||
|
|
|||
| processResultsGroupedByTime(result: DataFrameDTO, timeGrouping: AssetCalibrationForecastKey) { | |||
| processResultsGroupedByTime(result: DataFrameDTO, timeGrouping: AssetCalibrationForecastKey, options: DataQueryRequest) { | |||
| const originalValuesMap = this.constructDataLinkValuesMap(result, options); | |||
| result.fields.forEach(field => { | |||
| field.name = this.createColumnNameFromDescriptor(field as FieldDTOWithDescriptor); | |||
| const formatter = this.forecastDateFormatterMap.get(field.name as AssetCalibrationForecastKey); | |||
There was a problem hiding this comment.
While here, dateFormatter might be a better name for this as well.
saradei-ni
reviewed
Nov 15, 2024
| } | ||
| }); | ||
| } | ||
|
|
||
| private constructDataLinkBaseUrl(){ | ||
| private constructDataLinkValuesMap(result: DataFrameDTO, options: DataQueryRequest): Map<string, { from: number, to: number }> { | ||
| const originalValuesMap = new Map<string, { from: number, to: number }>(); |
There was a problem hiding this comment.
dataLinkValuesMap might be more suited here as well.
saradei-ni
reviewed
Nov 15, 2024
| if (formatter) { | ||
| let value = { from: 0, to: 0 } | ||
| const formattedValues: string[] = field.values!.map(formatter); | ||
| if (originalValues.length === 0) { |
There was a problem hiding this comment.
Using the available timeGrouping or the current condition along with timeGrouping might make it easier to understand. I believe length 1 is Week and the else statement has logic for both Day and Month.
saradei-ni
reviewed
Nov 15, 2024
| } | ||
|
|
||
| private createDataLinks(timeGrouping: AssetCalibrationForecastKey): DataLink[] { | ||
| private createDataLinks(timeGrouping: AssetCalibrationForecastKey, originalValuesMap: Map<string, { from: any, to: any }>): DataLink[] { |
There was a problem hiding this comment.
The type I think is { from: number, to: number }, so we can avoid any.
saradei-ni
reviewed
Nov 15, 2024
| @@ -130,36 +172,12 @@ export class CalibrationForecastDataSource extends AssetDataSourceBase { | |||
| if (!options.replaceVariables) { | |||
| return url; | |||
| } | |||
|
|
|||
| const value = options.replaceVariables(`\${__data.fields.${timeGrouping}}`); | |||
There was a problem hiding this comment.
We could have something like this if we don't have a value for the map every time.
const value = options.replaceVariables(`\${__data.fields.${timeGrouping}}`);
const valueAsDate = originalValuesMap.get(value);
if (!value && !valueAsDate) {
return url;
}
return `${url}&from=${valueAsDate.from}&to=${valueAsDate.to}`;
If we do have a value every time, what about something like this?
const value = options.replaceVariables(`\${__data.fields.${timeGrouping}}`);
if (!value) {
return url;
}
const [from, to] = originalValuesMap.get(value)!;
return `${url}&from=${from}&to=${to}`;
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
🤨 Rationale
👩💻 Implementation
🧪 Testing
✅ Checklist