Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ol-org/viz into WEB-4330-missing-settings
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ol-org/viz into WEB-4330-missing-settings
[WEB-4330] - Missing Pump Settings in PDF
Release 1.54.0 to develop
[WEB-4384] - Date Range Header lines up with Copy-as-Text
There was a problem hiding this comment.
Pull request overview
Updates text export/reporting period date formatting and improves pump settings association robustness by tolerating limited device/server clock drift, with corresponding test updates.
Changes:
- Added chart/reporting date bound format utilities and updated TextUtil to use them (including partial-date/time display and inclusive end dates where appropriate).
- Updated Basics/Trends text exports to opt into partial-date/time reporting period display.
- Expanded pump settings selection logic to allow up to 15 minutes of clock drift, with new/updated tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/datetime.js |
Adds shared chart date bound format constants/helper. |
src/utils/text/TextUtil.js |
Reworks reporting period formatting using moment-timezone + new helper. |
src/utils/basics/data.js |
Enables partial-date/time reporting period in Basics text export. |
src/utils/trends/data.js |
Enables partial-date/time reporting period in Trends text export. |
src/utils/DataUtil.js |
Adds 15-minute drift tolerance when selecting pumpSettings for latest upload. |
test/utils/text/TextUtil.test.js |
Updates/extends coverage for new reporting period formatting behavior. |
test/utils/DataUtil.test.js |
Adds coverage for pumpSettings selection within/outside drift window. |
src/index.js |
Exposes new datetime utilities via public utils export. |
package.json |
Bumps package version for the release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Clock drift on user's device may cause pumpSettings datum to have LATER timestamp than upload | ||
| // datum. The maximum time deviation that the Uploader allows between the user's device and Tidepool | ||
| // server time is 15 minutes, so we search up to 15 minutes into the future for the pumpSettings datum | ||
| const UPLOADER_TIME_DEVIATION_TOLERANCE = 15 * MS_IN_MIN; | ||
|
|
||
| pumpSettingsForUpload = _.filter( | ||
| pumpSettingsForUpload, | ||
| ps => ps.time <= latestPumpUpload.time | ||
| ps => ps.time <= (latestPumpUpload.time + UPLOADER_TIME_DEVIATION_TOLERANCE) | ||
| ); |
There was a problem hiding this comment.
Clock-drift tolerance is only applied when filtering pumpSettingsForUpload from pumpSettingsDatumsByIdMap, but the later fallback to this.latestDatumByType.pumpSettings still requires candidate.time <= latestPumpUpload.time for non-continuous uploads. This can still exclude a valid pumpSettings datum that’s within the 15-minute tolerance window when the by-id map doesn’t yield a match. Consider reusing the same tolerance check for the fallback path as well (and ideally hoist the tolerance constant so it’s not duplicated).
| export const CHART_DATE_BOUND_FORMAT = { | ||
| DATE_AND_TIME: 'MMM D, YYYY (h:mm A)', | ||
| DATE_ONLY: 'MMM D, YYYY', | ||
| }; | ||
|
|
||
| /** | ||
| * getChartDateBoundFormat | ||
| * @param {Object} startDate - a moment time object | ||
| * @param {Object} endDate - a moment time object | ||
| * | ||
| * @return {String} a moment time format (e.g 'MMM D, YYYY') | ||
| */ | ||
| export function getChartDateBoundFormat(startDate, endDate) { | ||
| if (!endDate) return CHART_DATE_BOUND_FORMAT.DATE_ONLY; | ||
|
|
||
| const isStartDateMidnight = (startDate?.hours() === 0 && startDate?.minutes() === 0) || | ||
| (startDate?.hours() === 23 && startDate?.minutes() >= 59); | ||
|
|
||
| const isEndDateMidnight = (endDate?.hours() === 0 && endDate?.minutes() === 0) || | ||
| (endDate?.hours() === 23 && endDate?.minutes() >= 59); | ||
|
|
||
| const isMatchingDateBounds = isStartDateMidnight && isEndDateMidnight; | ||
|
|
||
| if (!isMatchingDateBounds) { | ||
| return CHART_DATE_BOUND_FORMAT.DATE_AND_TIME; | ||
| } | ||
|
|
||
| return CHART_DATE_BOUND_FORMAT.DATE_ONLY; | ||
| } |
There was a problem hiding this comment.
src/utils/datetime.js has an in-file guideline to keep utilities organized alphabetically, but the newly added CHART_DATE_BOUND_FORMAT/getChartDateBoundFormat appear inserted here after formatDateRange, which breaks that convention. Please relocate these exports to match the file’s stated ordering so future diffs remain easy to scan.
along side tidepool-org/blip#1873
This pull request introduces improvements to date formatting and reporting period display, particularly for text exports, and enhances robustness in handling device clock drift for pump settings data. It also adds new utilities for chart date formatting and updates related tests to ensure correct behavior.
Date formatting and reporting period improvements:
getChartDateBoundFormatandCHART_DATE_BOUND_FORMATutilities to standardize chart/reporting date and time formatting insrc/utils/datetime.js, and integrated them intoTextUtilfor more accurate reporting period display, including support for partial dates and inclusive end dates. [1] [2] [3] [4] [5] [6] [7]buildDocumentDatesmethod inTextUtilto use the new chart date formatting logic, ensuring correct handling of midnight bounds and time zones.Pump settings clock drift handling:
DataUtilto tolerate up to 15 minutes of device/server clock drift when associating pump settings data with uploads, preventing exclusion of valid settings due to minor time discrepancies.Testing improvements:
test/utils/DataUtil.test.jsto verify correct selection of pump settings data within the 15-minute drift window, and to ensure exclusion beyond that threshold. [1] [2] [3]test/utils/text/TextUtil.test.jsto cover new date formatting scenarios, including partial dates and inclusive end dates.