-
Notifications
You must be signed in to change notification settings - Fork 114
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
⏱️ Implement TimelineContext (refactoring of LabelTabContext) #1138
⏱️ Implement TimelineContext (refactoring of LabelTabContext) #1138
Conversation
…-mission-phone into conditional-surveys-gpg
<Main> will sit within <App> and contain BottomNavigation, which has the 3 tabs LabelTab, MetricsTab, and ProfileSettings. This will allow the tabs to share some information, but be categorically separate from OnboardingStack.
Up to this point, the Label tab and Dashboard tab have been completely separate in terms of the data they use. The label tab read composite trips and the dashboard tab read metrics from the server. We plan to unify them to both use composite trips. This way, one query provides all the needed information to both tabs and there will no longer be a need to re-fetch when switching between these tabs. To do this, the tabs need to share common data in a context that is higher up in the component tree. Thus, much of the state that was previously kept at the level of <LabelTab>, (in LabelTabContext) is being hoisted up to the level of <Main>, and will be kept in a context called TimelineContext, which can be shared between the tabs. The contents of TimelineContext are essentially just copied over from LabelTabContext. Two values are renamed: "isLoading" -> "timelineIsLoading" and "refresh" -> "refreshTimeline". The things that are still specific to the Label tab (i.e. filtering dropdown "To Label" vs "All trips") are kept in LabelTab. Since LabelTabContext now has only 3 state values, and LabelTab.tsx is not that complex anymore, I included LabelTabContext directly in LabelTab.tsx rather than its own file. At the end of this refactoring, the majority of the complexity is now in TimelineContext. It is instantiated in <Main> and provided to the children of <Main> (the tabs).
The way this has been working up to now is that there are specific functions to load one week of data - either a specific week or the previous/next week. These functions load new trips and then update the queriedRange after the fact. But we need this to now work on the Metrics tab as well (where you can select *any* date range) so we are going to need to make some changes. We need to keep the chosen dates (from datepicker) centrally as state. Then we can watch it for changes; when it changes, we can compare the old and new ranges to see how they intersect: it will be either i) we are loading more data into the past ii) we are loading more data into the future or iii) this is an entirely new range Once we have determined that, we can load in the new part(s) of the range and handle the new data in the same way as before. I chose to store the date range as yyyy-mm-dd strings. The queried date range will also be stored as yyyy-mm-dd strings The initial date range is now set as the default value of `dateRange` in TimelineContext. So the initialy query for the past week can be removed from LabelTab.
The previous commit implemented a central state for the selected timeline range. The DateSelect component needs to control this, and both the label tab and dashboard tab needs to use it. DateSelect can be made generic such that it supports either a 'single' selection (which selects a whole week on the Label tab), or it supports a 'range' selection (as on the Dashboard tab). The DateSelect will display the queried date range, and it will accept an onChoose function that controls what the selection of date or date range will do. On LabelListScreen and MetricsTab are where DateSelect will be used. On LabelListScreen, the onChoose function calls loadSpecificWeek from TimelineContext (which sets the range to the week of [3 days before X, 3 days after X]. On MetricsTab, onChoose just sets the TimelineContext date range to the same date range from the datepicker. MetricsDateSelect is no longer needed. TimelineScrollList updated to use queriedDateRange instead of queriedRange; also access timeline variables from TimelineContext instead of passing a bunch of props.
Normally with loadSpecifcWeek, we use start date of 3 days before and end date of 3 days after. But we must handle edge cases wehre the user selects a date within 3 days of either the pipeline start or today
Because of unprocessed trips / user not being online for a while, they could have unprocessed trips on days up to and/or including today. So the datepicker should be able to go past pipelineEnd
If mode is 'single', we don't need to wait for user to confirm / press "Save". From onChange, we can call onChoose and dismiss immediately. If mode is 'range' we will do nothing onChange, but onConfirm we will call onChoose and then dismiss.
Apparently, JS Date's toISOString() does not include the local timezone offset; it is UTC. We want the local date here. So we need to use Luxon.
In MetricsTab, userMetrics will be populated not by a call to 'fetchMetricsFromServer', but by MetricsSummaries.generate_summaries from e-mission-common, which will compute user metrics from timeline data (ie timelineMap and timelineLabelMap) aggMetrics will still be the same as before, fetched from the server. There are a couple differences between metrics computed on e-misison-server and metrics computed on e-mission-common. (see metricsTypes.ts where DayOfMetricData can now be either DayOfClientMetricData or DayOfServerMetricData) To reconcile these differences, added helper functions `dateForDayOfMetricData`, `tsForDayOfMetricData`, and `valueForModeOnDay`
Refactored to a more generic function that accepts a second parameter for to load any number of days and not always 1 week (7 days) Replaced uses of loadAnotherWeek with loadMoreDays. (Except in LabelTab.tsx, where loadAnotherWeek was unused so it was just removed rather than replaced by the new function)
With the server calls, the metrics data always had 1 entry for every date in the query. Even if there was no data for that date, it would give a blank entry. e-mission-common isn't set up to work like this; it only gives entries on days that had trips. This change makes segmentDaysByWeeks work if there are missing dates. it requires an extra parameter for 'end date' I also noticed there is a lot of repeated code between these "Card" components; this should be refactored later
The MetricsTab compares past week to previous week, which means it makes the most sense with 2 weeks of data loaded. The common scenario is switching from the label tab, where the last 7 days are shown by default, to the metrics tab. We would need to load the previous week.
For debugging performance
getLastTwoWeeksDtRange is not used, debug statements made into logDebugs if useful and removed if unnecessary
It should return to the initial state which has queriedDateRange as null and dateRange as the initial range.
for..in loop can pick up on extra properties in the object, while forEach will only pick up on the numbered index values (like array values) Python lists that come from e-mission-common will have extra properties, (e.g. __class__) So we should make sure to use forEach instead of for..in.
I was using a local filesystem import for the purpose of dev testing. Changing to a normal package import now that this is done and working.
I was using a local filesystem import for the purpose of dev testing. Changing to a normal package import now that this is done and working.
…econtext' of https://github.com/JGreenlee/e-mission-phone into refactoring_timelinecontext
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1138 +/- ##
==========================================
+ Coverage 26.57% 28.95% +2.37%
==========================================
Files 114 115 +1
Lines 4993 5060 +67
Branches 1033 1088 +55
==========================================
+ Hits 1327 1465 +138
+ Misses 3666 3593 -73
- Partials 0 2 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
WeeklyActiveMinutesCard.tsx: Add records even if the total is 0 so that both "Walk" and "Regular bike" will show up in the legend. DailyActiveMinutesCard.tsx: The line chart was not showing any points at all. Turns out it's because the timestamp values were strings and needed to be numbers. We will now add records with 'null' values even if there is no value for a mode on a day. Both "Walk" and "Regular bike" now show in the legend. Adding 'spanGaps' to the chart options (in Chart.tsx), causes a line on the chart to connect only values that were on consecutive days; otherwise there will be a gap between entries on non-consecutive days. This gives users a nice way to visualize having a "streak" of days they used a particular active mode.
Basic test for loading composite trips into the timeline. Mocks a unified query response with 3 composite trips. Then checks to make sure those 3 trips can be rendered in a dummy component by reading 'timelineMap'. in timelineHelper.ts, do not attempt to unpack server data that doesn't exist (added because start_confirmed_place and end_confirmed_place don't exist on the dummy trips used for testing) In cordovaMocks.ts, resolve with [] instead of returning undefined
Codecov is reporting a big drop in coverage % from this PR, despite the fact that I added a lot of tests and didn't add that much new code. I don't fully know why, but I wonder if it's because I moved a bunch of stuff from Is Codecov actually considering our .tsx files, or only our .ts files? If it is only looking at our .ts files, then our coverage % is probably not nearly as good as it would appear. |
I added tests for about 1/2 of Both of these test suites can be made more robust later, but this is all I have time for at the moment. |
fixed bug where the returned array had 1 element but mismatched length, causing a second undefined element. This was caused by assigning elements to the array by arr[index] rather than using arr.push(). New implementation uses .push() and also removes the unneeded weekIndex variable
Up to this point, we have been querying all unprocessed/draft trips from pipelineRange start up to the endTs. This unifies the start ts for both queries. (the end ts still differs because unprocessed trips could exist after the pipeline end, while processed trips will always be before pipeline end). If for some reason there's an unprocessed trip older than the daterange start, it won't be included in the timeline anymore.
As it turns out, it's not just It seems counter-intuitive to me, but I guess this is the default behavior for Jest coverage. Any files that the tests don't reach are completely ignored. I'm going to start an issue for this. In the meantime, I think we shouldn't put too much stock in the coverage %. |
as described in patch notes https://github.com/JGreenlee/e-mission-common/releases/tag/0.1.3, the `@memoize` decorator caused slowness in the metrics computations, so I removed it and the sumaries are generated much faster now (difference of ~5000ms to ~10ms)
This was an obvious mistake in a recent commit – we should never query for unprocessed trips before the pipeline end. Any trip before the pipeline end is, by definition, not unprocessed.
With the recent dashboard tab changes, we can show some preliminary metrics while lazy-loading additional trips. We need some way to communicate to the user that work is being done. This will add a pulsing line animation (aka indeterminate progress bar) at the bottom of the NavBar whenever timelineIsLoading.
the early return at `if (!change) return;` would cause the ChangeIndicator to still be rendered with the text "undefined". This is because it was rendered if `changeText != ''`. A cleaner way is just checking for truthiness. Also generally refactored to simplify and add some comments inside the changeText useMemo()
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 found a bug on map list but I will take care of this in my next PR!
https://github.com/e-mission/e-mission-phone/assets/47590587/0cbf1bf5-0cf2-446e-b188-9e138f8ba065
tsRange={{ oldestTs: queriedRange?.start_ts, latestTs: queriedRange?.end_ts }} | ||
loadSpecificWeekFn={loadSpecificWeek} | ||
mode="single" | ||
onChoose={({ date }) => { | ||
const d = DateTime.fromJSDate(date).toISODate(); | ||
if (!d) return displayErrorMsg('Invalid date'); | ||
loadSpecificWeek(d); | ||
}} |
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.
Is there a specific reason we use "single" mode for the Label section and "range" mode for the Dashboard section? I would expect these two calendar behaviors to be consistent.
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.
Yes, that's a good observation. I was wondering that too because the datepickers on both tabs are now controlling the same thing. It would certainly be simpler and more consistent to make both a "range" selection.
My main focus for this PR was refactoring and I didn't want to deviate too far from the current behavior, but we should definitely consider for the future.
My biggest reservation is that a "range" selection on the label tab would add a couple more clicks to the flow.
"Single" date selection takes 1 click: user selects one date and we immediately load the week surrounding that date. This is handy for the label screen to quickly look at some week in the past.
"Range" date selection takes 3 clicks: i) select start date, ii) select end date, iii) click "Save". This gives the user more control over which specific days are going to be shown, which seems valuable for exploring historical metrics data.
Because of this, I think some users might prefer to keep the one-click datepicker on the Label tab.
Although, we might be able to get the "range" selection down to 2 clicks, if we are ok with the datepicker "auto-confirming" when a start and end date have been selected (eliminating the need to click "Save")
@shankari What do you think?
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.
@JGreenlee and @jiji14 I think that consistency is better than fewer clicks. Although the 1-click option has fewer clicks, it is not clear to the user what that click does (retrieve data for the week surrounding that date). The range makes it more clear and more flexible - what if I took a 2-week vacation and want to load all the data for that week.
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.
Great points. Next time we work on the Label screen we will unify the datepickers TODO
www/js/metrics/MetricsTab.tsx
Outdated
if (dateRangeDays < 14) { | ||
if (timelineIsLoading) { | ||
logDebug('MetricsTab: timeline is still loading, not loading more days yet'); | ||
} else { | ||
logDebug('MetricsTab: loading more days'); | ||
loadMoreDays('past', 14 - dateRangeDays); | ||
} | ||
} else { | ||
logDebug('MetricsTab: date range >= 14 days, not loading more days'); | ||
} | ||
}, [dateRange, timelineIsLoading, appConfig?.server]); | ||
|
||
// aggregate metrics fetched from the server whenever the date range is set | ||
useEffect(() => { | ||
logDebug('MetricsTab: dateRange updated to ' + JSON.stringify(dateRange)); | ||
const dateRangeDays = isoDatesDifference(...dateRange); | ||
if (dateRangeDays < 14) { | ||
logDebug('MetricsTab: date range < 14 days, not loading aggregate metrics yet'); | ||
} else { | ||
loadMetricsForPopulation('aggregate', dateRange); | ||
} | ||
}, [dateRange]); |
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.
How about defining a constant variable for the number 14? If we later decide to display only 7 days or extend it to 21 days, we can easily modify just one variable
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.
Good idea, I will make this change
www/js/metrics/metricsHelper.ts
Outdated
[...days].reverse().forEach((d) => { | ||
const date = dateForDayOfMetricData(d); | ||
// if date is older than cutoff, start a new week | ||
if (isoDatesDifference(date, cutoff) > 0) { | ||
weeks.push([]); | ||
cutoff = isoDateWithOffset(lastDate, -7 * weeks.length); | ||
} | ||
weeks[weeks.length - 1].push(d); | ||
}); |
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.
Could we use a regular for
loop instead of forEach
? This way, we wouldn't need to reverse the array. Please let me know if this change would have any other effects!
for(let i=days.length-1; i>=0; i--) {
const date = dateForDayOfMetricData(days[i]);
// if date is older than cutoff, start a new week
if (isoDatesDifference(date, cutoff) > 0) {
weeks.push([]);
cutoff = isoDateWithOffset(lastDate, -7 * weeks.length);
}
weeks[weeks.length - 1].push(days[i]);
}
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.
Great suggestion! I think this will have the same effect and be a bit more performant
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 has not yet been implemented; I assume you will handle it in the next PR/.
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.
TODO
From suggestion e-mission#1138 (comment)
…into refactoring_timelinecontext
Resolved merge conflicts from the bluetooth changes that were merged into |
ProgressBar wants this to be explicitly 'false' or it will still show up. The profile tab does not define 'isLoading' because it does not use timeline data and should not have the progress indicator visible.
…into refactoring_timelinecontext
resolved merge conflicts again |
FYI, I am done reviewing 32 out of the 40 files here and don't see any showstoppers. On the other hand, the remaining files are the more complex ones. Will finish review tomorrow. |
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 don't see any showstoppers, so I am going to merge this and get it out onto staging. As always, I have review comments that can be addressed in the next iteration.
const { notesFor, addUserInputToEntry } = useContext(LabelTabContext); | ||
const { notesFor, addUserInputToEntry } = useContext(TimelineContext); |
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.
These useContext
calls all have a warning that they are not covered by tests. But how would you even unit test this line? Does the react community have a suggestion?
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 don't know
TODO investiagate
date: string; // yyyy-mm-dd | ||
}; | ||
|
||
export type DayOfMetricData = DayOfClientMetricData | DayOfServerMetricData; |
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.
After we change the server side code to use mode_XXX
as well (as part of the survey unification in e-mission/e-mission-server#966 (comment)), these would be unified, right?
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.
Yes. Although I changed things a little so instead of mode_XXX
it is mode_confirm_XXX
(or purpose_confirm_XXX
, primary_ble_sensed_mode_XXX
, etc)
<LoadMoreButton onPressFn={() => loadMoreFn('past')} disabled={reachedPipelineStart}> | ||
<LoadMoreButton onPressFn={() => loadMoreDays('past', 7)} disabled={reachedPipelineStart}> |
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.
Is passing in the number of days preparing for specifying a range in the label screen as well?
Why not keep it hardcoded?
I see that it was added in JGreenlee@751c4d0 but I don't see why
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 wasn't specifically preparing for date range; I just thought this would be more flexible – and in fact, I think it is more comprehensible too
useEffect(() => { | ||
if (!pipelineRange || !tsRange.oldestTs) return; | ||
const displayStartTs = Math.max(tsRange.oldestTs, pipelineRange.start_ts); | ||
const queriedRangeAsJsDates = useMemo( |
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.
for my own understanding of React; aren't both useEffect
and useMemo
recomputed when the dependencies change? What is the benefit of switching this from useEffect
to useMemo
?
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.
Yes, both are recomputed when dependencies change. useMemo
is simpler; useful for declaring a value that is derived from other values.
useEffect
is for other "side effects" of a dependency, which could be an external function call, API call, etc. It's basically a function that runs when certain values change.
I try to use useMemo
when possible
www/js/metrics/metricsHelper.ts
Outdated
[...days].reverse().forEach((d) => { | ||
const date = dateForDayOfMetricData(d); | ||
// if date is older than cutoff, start a new week | ||
if (isoDatesDifference(date, cutoff) > 0) { | ||
weeks.push([]); | ||
cutoff = isoDateWithOffset(lastDate, -7 * weeks.length); | ||
} | ||
weeks[weeks.length - 1].push(d); | ||
}); |
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 has not yet been implemented; I assume you will handle it in the next PR/.
const rawVal = day[`label_${label}`]; | ||
const rawVal = valueForModeOnDay(day, label); | ||
if (rawVal) { | ||
records.push({ | ||
label: labelKeyToRichMode(label), | ||
x: unitFormatFn ? unitFormatFn(rawVal) : rawVal, | ||
y: day.ts * 1000, // time (as milliseconds) will go on Y axis because it will be a horizontal chart | ||
y: tsForDayOfMetricData(day) * 1000, // time (as milliseconds) will go on Y axis because it will be a horizontal chart |
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 as to why you created new functions for value
and ts
instead of returning a single data structure that had value
, ts
, and date
, similar to the current server structure. They are not even memoized, so we are likely recomputing the values over and over, which seems wasteful and will lower performance.
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 reason for valueForModeOnDay
is to support both old and new formats simultaneously. I am not as worried about performance for this one because it is just an object key lookup
The reason for tsForDayOfMetricData
instead of just including ts
directly in the metrics data is that the charts expect timestamps in the user's local timezone, and the server won't know what timezone the end user will be in.
I will look into performance optimization for this (maybe fill in each ts
upon receiving the metrics data)
TODO
setTimelineLabelMap(newTimelineLabelMap); | ||
setTimeout( | ||
() => | ||
publish('applyLabelTabFilters', { |
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.
what is the publish
function? I poked around pub/sub in React, but only see emit
.
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.
Around summer/fall, Abby wrote a lightweight pub/sub event handler because React didn't have a pub/sub pattern natively and we wanted to use it for onboarding
www/js/customEventHandler
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.
Ended up taking out this usage of the pub/sub anyway in #1159
const wentBeforePipeline = newStartDate.replace(/-/g, '') < pipelineStart.replace(/-/g, ''); | ||
const wentAfterToday = newEndDate.replace(/-/g, '') > todayDate.replace(/-/g, ''); |
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.
Why are you using a string-based operation here instead of date or timestamp? Feels like it would be more fragile and depend on the date format, which may be different in different localizations
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.
These strings are YYYY-MM-DD which are the ISO standard regardless of localization. So I don't think it matters in that regard
But for readability and conciseness, I suppose new Date(newStartDate) < new Date(pipelineStart)
may be better. TODO
} from '../js/metrics/metricsTypes'; | ||
|
||
describe('metricsHelper', () => { | ||
describe('getUniqueLabelsForDays', () => { |
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 is not really testing the uniqueness of the labels for the day. Since the labels are all unique, this seems identical to getLabelsForDay
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.
You're right; this should test a case where one label is present across multiple days
TODO
}); | ||
}); | ||
|
||
describe('formatDateRangeOfDays', () => { |
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 think you should also add tests for the part before or after the -
being blank.
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.
what do you mean by this?
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 mean that you should have tests that result in strings of the form 1/1 -
. Isn't that how we represent a range that is open at the end? Although maybe after the refactoring to always use ranges, we always have a start and an end?
@JGreenlee After pulling this and building, things seem to be working OK except that the addresses are not getting populated.
I don't appear to be able to connect to it using Safari, but from the xcode logs, the address population is not even getting invoked. I suspect a bad merge conflict resolution. |
@JGreenlee Note also that labeled trips are showing up in "To Label" |
I am able to connect via chrome on android. Only match for "geofabrik" is in failed attempts to load config from the local webserver. Copy-pasted logs from chrome are attached. Again, note no calls to the geofabrik API |
I'll look tomorrow. There's also Jiji's bugfix for the Leaflet maps which I don't think was present in this PR; it's in the next PR Note to self, maybe possible to |
As described in e-mission/e-mission-docs#1055 (comment)
Notes from commits
extract main content of into new component,
<Main>
refactor LabelTabContext into TimelineContext