-
Notifications
You must be signed in to change notification settings - Fork 438
3D: Auto-adjust date ranges and remove 1-year limit; add data range API #1550
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
base: development
Are you sure you want to change the base?
3D: Auto-adjust date ranges and remove 1-year limit; add data range API #1550
Conversation
- Add numDays to ThreeDState for user-selectable date range span - Add threeDInterval to GraphState for calculated 3D date range - Add reducers and selectors for both fields - Support numDays in chart link parsing
- Add MAX_3D_DAYS (1095) and DEFAULT_3D_DAYS (365) constants - Implement calculateThreeDDateRange() with 4-case logic: * Bounded/Bounded: warn if > maxDays, else use picker values * Unbounded/Bounded: use [DRE - numDays, DRE] * Bounded/Unbounded: use [max(DRS, end-numDays), latest day] * Unbounded/Unbounded: use [end - numDays, latest day] - Update isValidThreeDInterval() to accept maxDays parameter
- Dropdown with options: 1 month, 6 months, 1 year, 2 years, max, custom - Custom input with validation (1-1095 days) - Updates Redux state on selection - Similar pattern to IntervalControlsComponent
- Calculate 3D date range using 4-case logic in useEffect - Use dataRange API to get available data dates - Store calculated interval in Redux (threeDInterval) - Show warning when date range exceeds maxDays - Use threeDInterval for 3D queries instead of queryTimeInterval - Add ThreeDIntervalControlsComponent to UI options - Fix infinite loop by only dispatching on interval change
- Include numDays in 3D chart link generation - Add translation keys: threeD.numDays, threeD.date.range.exceeds.max - Add translations for 6.months and 2.years - Support English, French, and Spanish
|
|
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.
Thanks to @hmhngx for another contribution. I have made some comments to consider. I also have some thoughts that were not easy to put in a specific place so I'm putting them here.
- The custom range (and maybe the non-custom ones but not checked) seems to include one too many days. For example, choosing 3 days seems to show 4 on the graphic.
- Update: I'm having problems testing this as the date range is often not honored. This applies to several other ones in the comment.
- Setting the date range outside the range of readings caused a server error and it never graphed (
ERROR: range lower bound must be less than or equal to range upper bound) and it then went to the client.- Update: This now says rendering and also pops up a warning about the range.
- Some actions are causing a client message of exceeding the max days when it seems valid but this might naturally go away with other items.
- If set the upper bound on the date range that is above the actual readings available then get client message about exceeding days but still graphs the result.
Maybe change"threeD.y.axis.label": "Days of Calendar Year",on the 3D graphic label for the axis to "Date" since no longer just one year.- I'm thinking about whether the behavior should be changed from the issue if have only DRS is set. In this case, it would then do DRS to DRS + numDays. If there is not enough data in readings then the graphic would end early. This allows a user to easily get data from a given date forward. This also seems parallel to only have the DRE. What do you think?
As always, I'm happy to clarify and discuss as desired.
| meterOrGroupID: MeterOrGroupID | undefined; | ||
| meterOrGroup: MeterOrGroup | undefined; | ||
| readingInterval: ReadingInterval; | ||
| numDays: number | undefined; // Number of days for 3D graphic span (undefined means use default: 1 year) |
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'm curious about using undefined vs setting the default value to 1 year in src/client/app/redux/slices/graphSlice.ts.
- Add Reading.getMeterDataRange() and Reading.getGroupDataRange() methods
- Add /dataRange/meters/:meter_id and /dataRange/groups/:group_id routes
- Fix validation functions to accept single meter/group IDs
- Fix day calculation mismatch: use diff('days') + 1 to match client calculation
- Fix validateGroupThreeDReadingsParams to check group_id instead of meter_ids
- Add proper error logging for data range requests
- Add dataRange query endpoint to readingsApi - Fix meterOrGroup type to use MeterOrGroup enum instead of string literal - Endpoint fetches min/max timestamps for meter or group data
- Fix 'one too many days' bug: subtract (numDays - 1) for correct calendar days - Add validation to prevent server errors when range exceeds available data - Ensure threeDStartDate >= minDataDate and threeDEndDate <= maxDataDate - Fix calculateThreeDDateRange to handle edge cases with data boundaries - Improve autoAdjustThreeDInterval validation for maxDataDate
- Fix undefined args error in selectThreeDQueryArgs selector - Return undefined args when shouldSkipQuery is true - Add non-null assertion for args in useQuery (safe when skip=true) - Remove unused updateTimeInterval import - Fix comment placement for numDays in graphSlice (OED style preference)
- Provide default 30-day range when queryTimeInterval is unbounded - Prevents DateRangePicker from showing error state (red highlighting) - Fix TimeInterval import path to use correct relative path - Add non-null assertion for args in ReadingsPerDaySelectComponent
- Change 'max' to '3 years' in 3D date range span dropdown - Add '3.years' translation key in English, French, and Spanish - Change y-axis label from 'Days of Calendar Year' to 'Date' - Fix alphabetical order of translation keys (2.years, 3.years, 6.months) - Remove unused getLabelForValue function
huss
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.
Thanks to @hmhngx for the updated code. There are comments from the past that I think are unresolved & I made some new comments based on the changes. As indicated, it might be good to discuss the big picture ones to decide how they will be resolved. Also, there are three failing tests due to changed in 3D that need to be fixed so they pass.
| const displayInterval = queryTimeInterval.getIsBounded() | ||
| ? queryTimeInterval | ||
| : new TimeInterval( | ||
| moment().subtract(30, 'days').startOf('day'), |
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.
This seems to be causing several undesirable behaviors.
- The date range is shown and set to the thirty days back from the current day. However, the actual 3D data goes back 1 year from the last day with data. Thus, they do not correspond to each other.
- In src/client/app/components/ThreeDIntervalControlsComponent.tsx there is DEFAULT_3D_DAYS. How does this relate to the 30 days here? This relates to item 1.
- It is directly setting the data range shown and not setting queryTimeInterval. As a result, the date range is set but not shown on several other graphic pages. It also is not honored.
- I'm seeing warning, it not honoring the time interval & resetting the time interval in 3D.
My current thinking is the date range should not be modified and the 3D data is gotten from the server by going back by DEFAULT_3D_DAYS (currently 1 year). Thus, date range remains unbounded but 3D gets the correct data. I think it may be possible that this was causing an error given your comment. If so, maybe we should discuss options.
| selectSelectedUnit, | ||
| selectThreeDState, | ||
| (queryTimeInterval, selectedUnit, threeD) => { | ||
| (queryTimeInterval, threeDInterval, selectedUnit, threeD) => { |
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.
queryTimeInterval seems unused.
| "1.week": "1 week", | ||
| "1.year": "1 year", | ||
| "2.months": "2 months", | ||
| "2.years": "2 years", |
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.
Moving the items does not appear in other languages.
| "threeD.time": "Time", | ||
| "threeD.x.axis.label": "Hours of Day", | ||
| "threeD.y.axis.label": "Days of Calendar Year", | ||
| "threeD.y.axis.label": "Date", |
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.
The text change does not appear in other languages.
| return TimeInterval.unbounded(); | ||
| } | ||
|
|
||
| /** |
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'm still unsure that the calculation should happen on the client. I think we should discuss to resolve. Then I can carefully review the code.
src/server/models/Reading.js
Outdated
| * @returns {Promise<{minDate: Moment | null, maxDate: Moment | null}>} | ||
| */ | ||
| static async getMeterDataRange(meterID, conn) { | ||
| const result = await conn.oneOrNone(` |
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.
OED normally prefers to have an SQL file for queries. Unless there is a reason not to do this then it should be changed. Also, I think using $1 is less desirable and not done in our SQL files.
This also applies to group below.
src/server/models/Reading.js
Outdated
| FROM readings | ||
| WHERE meter_id = $1 | ||
| `, [meterID]); | ||
| if (result && result.min_date && result.max_date) { |
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.
My testing found (as I would expect) that if there are not readings so no min/max then you get:
result: { min_date: null, max_date: null }
Thus, I don't think you need the extra logic for this.
src/server/routes/unitReadings.js
Outdated
| const durationInDays = timeInterval.endTimestamp.diff(timeInterval.startTimestamp, 'days') + 1; | ||
| // Security: Limit 3D to MAX_3D_DAYS (1095 days = 3 years) to prevent abuse from unauthenticated users. | ||
| // Frontend auto-adjustment provides UX, but backend must enforce security limits. | ||
| if (durationInDays > 1095) { |
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.
As was done on the client, it is nice to make limits a const though it will be a little different due to JS not TS. I think that should be done here, esp. since the value is used more than once.
| <div style={divTopBottomPadding}> | ||
| <p style={labelStyle}> | ||
| {translate('threeD.numDays')}: | ||
| <TooltipMarkerComponent page='home' helpTextId='help.home.threeD.numDays.tip' /> |
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.
There should be a placeholder in src/client/app/components/TooltipHelpComponent.tsx for this popup help.
- Create get_meter_data_range.sql and get_group_data_range.sql
- Use named parameters (${meterID}, ${groupID}) instead of $1
- Simplify null check logic using optional chaining
- Define MAX_3D_DAYS = 1095 at module level - Replace hardcoded 1095 values with constant - Improves maintainability and consistency
- Allow date range to remain unbounded when queryTimeInterval is unbounded - 3D component handles its own date range calculation using DEFAULT_3D_DAYS - Fixes mismatch between displayed date range and actual 3D data range
- queryTimeInterval was in dependencies but not used in selector - Selector uses threeDInterval instead for 3D date range
- Fix alphabetical order of 2.years, 3.years, 6.months in French and Spanish - Update y-axis label from 'Days of Calendar Year' to 'Date' in French and Spanish - Add help text placeholder for help.home.threeD.numDays.tip
- Compare useMeterZone and warnOnCumulativeReset against defaults - Previously compared values against themselves, always returning false - Now correctly detects when these fields differ from defaults
- Change 'reject_disable' to 'reject_disabled' in migration files - Matches TypeScript/JavaScript code and main enum definition - Prevents constraint violations when migrating existing data
Description
(Please include a summary of the change and which issue is touched on. Please also include relevant motivation and context.)
This PR enhances the 3D visualization experience and supporting APIs:
Fixes #1185
(In general, OED likes to have at least one issue associated with each pull request. Replace [issue] with the OED GitHub issue number. In the preview you will see an issue description if you hover over that number. You can create one yourself before doing this pull request. This is where details are normally given on what is being addressed. Note you should not use the word "Fixes" if it does not completely address the issue since the issue would automatically be closed on merging the pull request. In that case use "Partly Addresses #[issue].)
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
(Describe any issues that remain or work that should still be done.)