Meal Editor Logging - #645
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
greinard
left a comment
greinard
left a comment
There was a problem hiding this comment.
It may be worth instrumenting a few of the other actions as well, such as on delete, on cancel, on remove image, etc.
You also probably want to target a branch off the v3.21.2 tag so this can be deployed to the Prod ViewBuilder more quickly. The v4.0.0 stuff is going to take a little longer to test/validate for release to VB, so if this is semi-urgent, having it be part of a minor bump will help expedite its rollout.
greinard
left a comment
greinard
left a comment
There was a problem hiding this comment.
Looking pretty good. Just a few more suggestions. Thanks for addressing the prior feedback.
| setImageLoading(!!imageUrl); | ||
| } else { | ||
| props.onError(); | ||
| onError(`Can't find meal reference. id=${mealReference.id}`); |
There was a problem hiding this comment.
Nitpick, but the thing we cant find here is the meal itself, not the reference. So I'd use just "Can't find meal." or "Can't find referenced meal."
| } | ||
| }); | ||
|
|
||
| MyDataHelps.getDeviceInfo().then(info => { |
There was a problem hiding this comment.
Since we would prefer to have this device info available for any of the logging, including when it loads the meal to edit, we can probably move the whole getMealToEdit().then(...) piece inside here after calling setDeviceInfo(info);.
The getDeviceInfo() call should be really fast as I believe it just runs in memory on the device or in the browser (no remote requests).
| try { | ||
| await uploadMealImageFile(mealToEdit, newImageFile); | ||
| } catch { | ||
| logMealEvent("image-uploaded", mealToEdit, deviceInfo); |
There was a problem hiding this comment.
Maybe include the { name: newImageFile.name, size: newImageFile.size } here as well.
|
|
||
| saveMeals(startOfDay(mealToEdit.timestamp), updatedMeals).then(props.onDelete); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Probably worth adding an onCancel function in here that logs an event and calls props.onCancel(), and then update the cancel button at the bottom to call this onCancel instead of the prop function directly.
| return (item2.confidenceScore ?? 0) - (item1.confidenceScore ?? 0); | ||
| } | ||
|
|
||
| export function logMealEvent(type: string, meal: Meal | undefined, deviceInfo: DeviceInfo | undefined, data: any = {}) { |
There was a problem hiding this comment.
Maybe use unknown for the type on data instead of any.
| eventType: "meal-editor", | ||
| properties: { | ||
| type: type, | ||
| body: data, |
There was a problem hiding this comment.
I like this better with the individual fields we are interested in. Ties it back to the meal and the device info nicely without logging TMI.
Was thinking maybe this field could be named just data or additionalData rather than body, but I don't feel strongly about it if you prefer body.
|
|
||
| export function logMealEvent(type: string, meal: Meal | undefined, deviceInfo: DeviceInfo | undefined, data: any = {}) { | ||
| MyDataHelps.trackCustomEvent({ | ||
| eventType: "meal-editor", |
There was a problem hiding this comment.
Oh, one more thing. I suppose at some point we might be able to reuse this function across the other meal related components, so maybe we just set the event type to meal-event here (kind of fits the function name better, too).
|
Replaced by #648 due to rebase having a fit. |
2 participants
Overview
Add custom event logging to the meal editor for troubleshooting image uploads.
Security
Testing
Test meal editor in web/mobile and ensure custom events are logged.
Documentation
@CareEvolution/api-docs.Reviewers
Assign to the appropriate reviewer(s). Minimally, a second set of eyes is needed ensure no non-public information is published. Consider also including:
Consider "Squash and merge" as needed to keep the commit history reasonable on
main.