-
-
Notifications
You must be signed in to change notification settings - Fork 8
[WEB-3841] PDF bolus limit #529
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
Open
krystophv
wants to merge
4
commits into
develop
Choose a base branch
from
WEB-3841-pdf-bolus-limit
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| "node": "20.8.0" | ||
| }, | ||
| "packageManager": "[email protected]", | ||
| "version": "1.51.0-web-3830-pregnancy-ranges.3", | ||
| "version": "1.50.0-WEB-3841-pdf-bolus-limit.1", | ||
| "description": "Tidepool data visualization for diabetes device data.", | ||
| "keywords": [ | ||
| "data visualization" | ||
|
|
@@ -65,7 +65,7 @@ | |
| "moment": "2.29.4", | ||
| "moment-timezone": "0.5.43", | ||
| "parse-svg-path": "0.1.2", | ||
| "pdfkit": "0.13.0", | ||
| "pdfkit": "0.15.0", | ||
| "process": "0.11.10", | ||
| "prop-types": "15.8.1", | ||
| "react": "16.14.0", | ||
|
|
||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -241,6 +241,35 @@ class DailyPrintView extends PrintView { | |
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Counts the number of lines needed to render bolus entries, respecting a maximum line limit. | ||
| * Determines which boluses can be rendered within the limit and whether an ellipsis is needed. | ||
| * | ||
| * @param {Array<Object>} boluses - Array of bolus objects to be rendered. | ||
| * @param {number} [maxLines=50] - Maximum number of lines allowed for rendering. | ||
| * @returns {{ count: number, bolusesToRender: Array<Object>, needsEllipsis: boolean }} | ||
| * An object containing: | ||
| * - count: Total number of lines used. | ||
| * - bolusesToRender: Array of bolus objects that fit within the line limit. | ||
| * - needsEllipsis: Whether additional boluses exist beyond the limit. | ||
| */ | ||
| countBolusLinesWithLimit(boluses, maxLines = 50) { | ||
| let lineCount = 0; | ||
| let needsEllipsis = false; | ||
| const bolusesToRender = []; | ||
| for (let i = 0; i < boluses.length; ++i) { | ||
| const bolus = boluses[i]; | ||
| const linesForBolus = (bolus.extended != null || bolus.expectedExtended != null) ? 2 : 1; | ||
| if (lineCount + linesForBolus > maxLines) { | ||
| needsEllipsis = true; | ||
| break; | ||
| } | ||
| bolusesToRender.push(bolus); | ||
| lineCount += linesForBolus; | ||
| } | ||
| return { count: lineCount, bolusesToRender, needsEllipsis }; | ||
| } | ||
|
|
||
| calculateDateChartHeight({ data, date }) { | ||
| this.doc.fontSize(this.smallFontSize); | ||
| const lineHeight = this.doc.currentLineHeight() * 1.25; | ||
|
|
@@ -252,14 +281,8 @@ class DailyPrintView extends PrintView { | |
| const maxBolusStack = _.max(_.map( | ||
| _.keys(threeHrBinnedBoluses), | ||
| (key) => { | ||
| const totalLines = _.reduce(threeHrBinnedBoluses[key], (lines, insulinEvent) => { | ||
| const bolus = getBolusFromInsulinEvent(insulinEvent); | ||
| if (bolus.extended || bolus.expectedExtended) { | ||
| return lines + 2; | ||
| } | ||
| return lines + 1; | ||
| }, 0); | ||
| return totalLines; | ||
| const boluses = _.sortBy(threeHrBinnedBoluses[key].map(getBolusFromInsulinEvent), 'normalTime'); | ||
| return this.countBolusLinesWithLimit(boluses, 50).count; | ||
| } | ||
| )); | ||
|
|
||
|
|
@@ -898,7 +921,9 @@ class DailyPrintView extends PrintView { | |
| }, | ||
| }; | ||
| }(this.doc)); | ||
| _.each(_.sortBy(binOfBoluses, 'normalTime'), (bolus) => { | ||
| const sortedBoluses = _.sortBy(binOfBoluses, 'normalTime'); | ||
| const { bolusesToRender, needsEllipsis } = this.countBolusLinesWithLimit(sortedBoluses, 50); | ||
| _.each(bolusesToRender, (bolus) => { | ||
| const displayTime = formatLocalizedFromUTC(bolus.normalTime, this.timePrefs, 'h:mma') | ||
| .slice(0, -1); | ||
| this.doc.text( | ||
|
|
@@ -911,7 +936,7 @@ class DailyPrintView extends PrintView { | |
| { align: 'right' } | ||
| ); | ||
|
|
||
| if (bolus.extended != null) { | ||
| if (bolus.extended != null || bolus.expectedExtended != null) { | ||
|
||
| const normalPercentage = getNormalPercentage(bolus); | ||
| const extendedPercentage = getExtendedPercentage(bolus); | ||
| const durationOpts = { ascii: true }; | ||
|
|
@@ -927,6 +952,15 @@ class DailyPrintView extends PrintView { | |
| } | ||
| yPos.update(); | ||
| }); | ||
| if (needsEllipsis) { | ||
| this.doc.text( | ||
| '…', | ||
| groupXPos, | ||
| yPos.current(), | ||
| { indent: 2, width: groupWidth } | ||
| ); | ||
| yPos.update(); | ||
| } | ||
| }); | ||
|
|
||
| return this; | ||
|
|
||
This file contains hidden or 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
This file contains hidden or 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
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.
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 see that you're sorting the boluses here prior to passing to
countBolusLinesWithLimit, which makes sense.However, you haven't done the same in when passing the 3h binned boluses in
calculateDateChartHeightearlier.I'm thinking it's possible that the earlier height is then calculated based on a different set of boluses that get rendered here.
i.e. perhaps the first 50 (of 60) when called within
calculateDateChartHeightare normal boluses, but when you pass the sorted boluses in here, there are a few extended boluses that needed more lines than what was allotted for height.Should we also pass sorted boluses in the earlier calls to
countBolusLinesWithLimit?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.
We can add the sort, for sure, but I can't think of a pathological case for a mismatch causing serious issues as is. If the first unsorted 50 of 60 happen to be 1 line boluses, we'd still end up counting at least up to the 50 line limit and returning the "count" there as 50. When it comes to this sorted set, we actually get the list of boluses to render also taking into account the 50 line limit. So the height count will always take into account the whole set up to the 50 line limit and the rendering will only render the first
Xnumber of boluses that will fit into a 50 line limit.The only mismatch I can come up with would be potentially landing on a "count" returned for the height calculation of 49 instead of 50 by hitting an extended bolus and returning early on that count, but given that calculateDateChartHeight is just used for charts-per-page layout, I don't think that the 1 line difference could ever result in the wrong number of charts on a page (given proximity to the maximum).
For the sake of consistency, adding the sort is a good call though.
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.
Yeah, I get it. This whole PR is based on an unrealistic edge case, and the sort issue has a basically zero chance of happening.
You can put it for re-review and skip the sorting if you'd prefer.
Uh oh!
There was an error while loading. Please reload this page.
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 added the sort (ecce2ff) 😄 , will put it up for re-review. And my reply was at least as much just me re-stating the logic to convince myself that I hadn't introduced a new edge case as it was trying to convince anyone else.