Add data util method to allow adding locally stored data - #584
Add data util method to allow adding locally stored data#584clintonium-119 wants to merge 4 commits into
Conversation
Release 1.53.1 to master
There was a problem hiding this comment.
Pull request overview
This PR refactors local data loading functionality by consolidating it from the storybook preview configuration into a reusable addLocalData method within the DataUtil class. This enables local rawData.json files used for visualization exploration to be leveraged within Tidepool Web patient data views.
Changes:
- Added new
addLocalDatamethod to DataUtil class that handles loading and processing local JSON data files - Simplified storybook preview.js by removing local data loading logic and replacing it with a call to the new method
- Updated package version to reflect the changes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/utils/DataUtil.js | Adds new addLocalData method that encapsulates local data file loading, format detection, and processing logic |
| storybook/preview.js | Simplifies preview configuration by delegating local data loading to DataUtil's new method |
| package.json | Updates version number for the new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| this.log(`Loading dataset provided by ${dataSource}`); | ||
| } catch (e) { | ||
| data = { data: [] }; |
There was a problem hiding this comment.
The error handling sets data to an object { data: [] } instead of an array. This will cause issues when the data is processed later, as the code expects data to be an array. The subsequent check data?.[0].dataset on line 122 will fail (since data[0] would be undefined for this object), and the conditional on line 132 expects data to be an array for _.map. The catch block should set data = [] instead to match the expected type.
| data = { data: [] }; | |
| data = []; |
| try { | ||
| // eslint-disable-next-line global-require, import/no-unresolved | ||
| data = require('../../local/rawData.json'); | ||
| let dataSource = localDataSource === 'export' ? 'the Tidepool export service' : 'the Tidepool API'; |
There was a problem hiding this comment.
The dataSource variable initialized on line 117 is declared but then immediately overwritten in the conditional blocks on lines 121 and 124. This makes the initial assignment on line 117 unnecessary and potentially confusing. Consider removing the initial assignment and declaring dataSource with let without initialization, or restructure the logic to avoid the unnecessary assignment.
| this.endTimer('init total'); | ||
| }; | ||
|
|
||
| addLocalData = (patientId, returnData = false, localDataSource) => { |
There was a problem hiding this comment.
The localDataSource parameter name is ambiguous and doesn't clearly indicate its purpose. The parameter only accepts the value 'export' to enable special export-related processing, but the name suggests it might specify the source location of local data. Consider renaming to something more descriptive like isExportFormat or exportDataFormat to better communicate its purpose.
| @@ -109,6 +109,34 @@ export class DataUtil { | |||
| this.endTimer('init total'); | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
The new addLocalData method lacks JSDoc documentation. Given that other methods in the DataUtil class have JSDoc comments (as seen in the constructor), this method should include documentation explaining its parameters (especially the localDataSource parameter which has specific behavior), return value, and purpose. This is particularly important since this is a new public API.
| /** | |
| * Loads local raw Tidepool data from a JSON fixture and forwards it to {@link addData} | |
| * for normalization and indexing. | |
| * | |
| * The source of the local data can affect how basal durations are interpreted: | |
| * when {@code localDataSource} is set to {@code 'export'}, basal records with a | |
| * duration less than {@code 1000} are treated as minutes and converted to milliseconds. | |
| * For any other value (or when omitted), durations are assumed to already be in | |
| * milliseconds and are left unchanged. | |
| * | |
| * @param {string} patientId - Identifier of the patient the local data belongs to. | |
| * @param {boolean} [returnData=false] - When {@code true}, returns the normalized data | |
| * from {@link addData}; when {@code false}, performs the load as a side effect only. | |
| * @param {('export'|undefined)} [localDataSource] - Indicates the origin/format of the | |
| * local data. Use {@code 'export'} when loading data from a Tidepool export so basal | |
| * durations shorter than {@code 1000} are interpreted as minutes. | |
| * @returns {*} The value returned by {@link addData}, typically the normalized dataset | |
| * when {@code returnData} is {@code true}; otherwise may be {@code undefined}. | |
| */ |
| addLocalData = (patientId, returnData = false, localDataSource) => { | ||
| let data; | ||
| try { | ||
| // eslint-disable-next-line global-require, import/no-unresolved | ||
| data = require('../../local/rawData.json'); | ||
| let dataSource = localDataSource === 'export' ? 'the Tidepool export service' : 'the Tidepool API'; | ||
|
|
||
| if (data?.data?.current?.data) { | ||
| data = _.flatten(_.values(data.data.current.data)); | ||
| dataSource = 'a Tidepool Web console export'; | ||
| } else if (data?.[0].dataset) { | ||
| data = _.flatten(_.map(data, v => v.data)); | ||
| dataSource = 'a Tidepool Account Tool export'; | ||
| } | ||
|
|
||
| this.log(`Loading dataset provided by ${dataSource}`); | ||
| } catch (e) { | ||
| data = { data: [] }; | ||
| } | ||
|
|
||
| if (localDataSource === 'export') { | ||
| data = _.map(data, d => ({ ...d, duration: d.type === 'basal' && d.duration < 1000 ? d.duration * MS_IN_MIN : d.duration })); | ||
| } | ||
|
|
||
| return this.addData(data, patientId, returnData); | ||
| }; |
There was a problem hiding this comment.
The new addLocalData method lacks test coverage. The repository has comprehensive tests for DataUtil (as seen in test/utils/DataUtil.test.js with 6454 lines), and other methods like addData are thoroughly tested. This new method should have corresponding test cases covering different scenarios: successful data loading from different formats (Tidepool Web console export, Tidepool Account Tool export, direct API format), error handling when the file doesn't exist, and the export format duration conversion logic.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Supports using the local/rawData.json that we use for viz dataUtil exporation withing Tidepool Web patient data views
Related PRs:
tidepool-org/blip#1846
tidepool-org/tideline#521