Skip to content
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

Pull from master to get the new onboarding android flow #1080

Closed
wants to merge 1,069 commits into from

Conversation

shankari
Copy link
Contributor

No description provided.

JGreenlee and others added 30 commits January 25, 2024 01:47
dt might be undefined here (like in the case of the most recent / current place in the timeline)
In that case we should return nothing instead of letting Luxon create a new date (it would fallback to the current date&time which we don't want)
this is used in a few places - it's tidier to have a common type definition
i) instead of providing timelineLabelMap and timelineNotesMap to the entire component tree under LabelTab, we can pass accessor functions (userInputFor, labelFor, notesFor) to get user inputs in a cleaner way

ii) Unify the structure of how multilabel vs enketo user inputs are kept on the phone. Now both will be the 'raw' format (an object with 'data', 'metadata' etc).
Note that there is still a discrepancy on the server - for multilabel inputs we get a plain string and for enketo inputs we get the whole userinput entry.
The plan is to unify this on the server at some point, so I am doing it here on the phone in advance.

iii) instead of using a 'justRepopulated' flag to signify that an entry should not be filtered within 30 seconds of having a user input recorded on it, we can just look at the write_ts in the metadata of that entry's inputs and see if it was within 30 seconds ago. If so, it's immune to filtering.
After a label is recorded and added to the timelineLabelMap, we'll trigger a refilter after 30 seconds by calling setLastFilteredTs.
This approach negates the need for an extra property on the timelineEntry object - which is good because we want it to match the server type.
This useEffect was watching timelineEntry for updates, but we changed the mechanism so timelineMap is no longer updated -- only the timelineLabelMap gets updated when a new response is recorded. By watching the result of userInputFor(...), this component will recieve the update.
This has always been a property on the server for trips as well as places. For some reason I must have missed it on the first pass of making the ConfirmedPlace type definition.
It keeps the codebase more logical (and more modular) to keep Enketo survey-related type definitions encapsulated away in enketoHelper.
When we have Typescript compiler in 'watch' mode (useful for linting/ finding type errors in VSCode), it outputs to the root level directory 'dist'. Adding this directory to gitignore so we don't end up needlessly committing these output files.
Each tab gets wrapped by an error boundary. We were doing this in the body of the App component which means every time the App component was updated/rerendered, the tabs were recomputed too.
Doing the error boundary wrapping outside of the App component ensures that it only happens once and we use the same tab components each time
-Fix size of Leaflet markers
-- this changed when we removed all the extra CSS bloat and easy one line restores the previous size

-Unify app bar styles
-- removed elevation: 3 to soften the shadow (default is 1 and matches the rest of the app better - we were only using elevation 3 to match the style of the Ionic header)
-- use 'colors.surface' instead of hardcoded 'white'

- unify 'notesButton' style of diary cards
- add paddingHorizontal to ModesIndicator
-- this way, if there are a lot of modes, they have room to breathe
As in fa1dfbf, TypeScript puts output files here. We don't need git NOR Jest to pick up on these 'dist' files.
With invalid inputs, this function should return early rather than continuing and resulting in a zero-length duration.
Fixes the diaryHelper test that was failing.
We need a safe way to mark that validation fails in this function, without actually throwing an error. Let's use a third parameter for this purpose which is a callback function 'onFail' to handle the invalid case.
- Updated tests accordingly - now passing
Typescript wants us to be more explicit about what could be null/undefined and what couldn't be.

transitionTrip2TripObj should return a Promise that could resolve as an UnprocessedTrip or undefined.
(If undefined, it gets filtered later in readUnprocessedTrips)

We also need to address that Luxon's DateTime.toISO has string or null as the return type – null is for if the input was invalid. It never should be, but if it was for some reason we can have an error dialog and assert type with an empty string.
mockUnprocessedTrip was unnecessary and not used anywhere
mockFilterLocation didn't need all these fake properties and instead could just include the few that are needed and then declare it 'as FilteredLocation'
enketoHelper.test.ts had a lot of the wrong types in the fake responses. They were all strings but I've since updated them so there some strings, some numbers, objects.
EnketoResponseData in enketoHelper is correct.

- some other type casting (with 'any') in both of these tests to ensure fake inputs satisfy parameters types
this test still had LabelOptions type for the userInput values instead of the UserInputEntry type.
It didn't cause the tests to fail because inferFinalLabels and verifiabilityForTrip only check that the values exist - they don't care what properties they have.
Anyhow, the types line up now.
In 4f173ba I bumped the prettier version to 3.1. Whatever version we are using for the project should be the same as the workflow runs with (as we established in #1117)
-- Part of the fix for e-mission/e-mission-docs#1034

When a user input is recorded, we put it in storage. For snappy performance we also cache it in the timelineLabelMap / timelineNotesMap state.
However, if we switch filters or load more trips, this state gets overridden. So, we need to call updateLocalUnprocessedInputs at some point.
It is an async function but we can call it without 'await' and just allow it to execute in the background.
-- Part of the fix for e-mission/e-mission-docs#1034

When the yellow checkmark is used to verify a pair of inferred mode+purpose labels, both labels should be verified by a single click.
The labels get stored fine; however, the way we update state after doing so prevents both labels from being updated simultaneously and only one of them gets filled in. The resulting UX is that it takes 2 clicks and this is not desired.
We could: update state in succession, one after the other (await label A, then proceed label B),
OR we could rework these functions to support adding labels for a particular trip as a batch (so the same function call could store mode+purpose at the same time). This was decided as a better and more performant option.

It uses Promise.all to store all labels from the batch, then it updates the state with all changes from that batch.
When a single label is recorded, it is a batch of 1 - when verifying inferences, it may be a batch of multiple labels.
When selecting date, the following behavior occurs:
- If 2+ days, behave as normal (Range of Day1 -> Day2)
- If  "open Ended" (i.e., only start is picked), fetch
  a range of days between that day and present (Day1 -> Today)
- If 1 day, fetch range for 48 hours from that day

TODO: The 1 day range going to 48 hours is meant as a patch, so that
this WSOD fix can go to staging.  To make a single day only fetch
24 hrs, some changes to the server may be needed.
Instead of mutating an extra property 'displayDt' onto the entries, let's use a get function that returns the displayDt. This should fix issue (4) in e-mission/e-mission-docs#1034.
And let's also memoize that function so performance is not impacted
If something was thrown, it could display 'undefined' if 'err' was an error message and not the error object.
Using the displayError function is a more proper way to guarantee we see any error messages
- 'key' could be kept in 'data' or 'metadata' depending on if it is a processed or unprocessed entry (this is an inconsistency we may want to revisit later)
- handle possible null/ undefined parameters in functions: check for truthy inputs before executing
JGreenlee and others added 28 commits April 17, 2024 01:21
We recently added the ability to refresh the config from inside the app.
But whenever the config is downloaded for the first time, we cache the resources referenced in it by URL. Subsequent config downloads would still use the resources from the first time.

fetchUrlCached now accepts options to pass through to the fetch API; if cache is 'reload', we will skip checking our localStorage cache for a previously stored value. This option will also cause the fetch API will also skip its own internal cache

The result of this is that when we refresh the config, URL-referenced resources inside it will also be refreshed.
Add 'collectCoverageForm' configuration to track all files for Jest coverage
e-mission/e-mission-data-collection#226
is now merged and a new release
https://github.com/e-mission/e-mission-data-collection/releases/tag/v1.8.5
has been created

So we can now use the version number instead of a branch while adding the
plugin

With this commit, the initial implementation in
#1144
is done, and we can merge it
* lock React version to ~18.2.0

React 19 and React 18.3 are out (https://react.dev/blog/2024/04/25/react-19-upgrade-guide#react-18-3), but the React Native ecosystem is still on React 18.2.
We need to lock the versions down until the React Native packages update.

* 💩 Temporarily pin the version to 14.1 so that the CI is successful

Consisent with
#1148 (comment)

* 💩 Revert back even further to xcode 14

Consistent with
#1148 (comment)

* 📌 Ensure that we are using the correct version of java

Before this, we set only the `JAVA_HOME` to pin the java version
This was working until a couple of days ago, when, even after setting the
version, the java version was not changed.

This may be due to the location for the java executable changing, so let's try
to set the PATH as well

Also print out several environment variables to help debug further

Related info:
#1148 (comment)
#1148 (comment)

* 🔧 Change the environment variable to the one used in the new github actions runner

Related context:
#1148 (comment)

* 📌 Pin the mac version to 13

To be consistent with
#1148 (comment)

Before we fix
e-mission/e-mission-docs#1060

* 📌 Pin the mac version to 13

Consistent with
#1148 (comment)
and
b5072f3 (for iOS)

* ⏪️ Revert previous attempts to fix the issue

since pinning to OSX-13 fixed it
#1148 (comment)

* 📌 Pin the xcode version again

Because apparently the default version, even on the OSX 13 runner, is now xcode 15
#1148 (comment)

* Ensure that the switch command is run as root

---------

Co-authored-by: K. Shankari <shankari@eecs.berkeley.edu>
So we don't get false negatives based on currently active pull requests
Expand the profile screen to simulate bluetooth actions separately and handle them properly in the native code
…est` matches

At some point, we changed the mode and purpose user inputs as objects that were
stored to be full objects, with start and end timestamps, instead of just the
labels. We changed all uses of the MODE and PURPOSE to match it, but apparently
forgot to change this location, where the replaced mode button is conditionally
displayed.

This is a quick change to make the usage here consistent with the rest of the
code so that we can push this out ASAP.

@JGreenlee, please let me know if there is a more principled fix, it is late
and I don't want to experiment any further.

Testing done:
Please see PR
Since the fleet deployments are installed on work phones, and the background restriction is apparently grayed out on android work phones.
e-mission/e-mission-docs#1070

This is a hack (hence the 💩 emoji) but it will allow us to make progress
through the onboarding, get the logs and then resolve the issue the right way.
Or if we can't figure out how to access it the right way, this can become a
config option for deployments that plan to use work phones.

This should unblock
e-mission/e-mission-docs#1070 (comment)
The previous unit tests for the input matching assumed that the user input
would be of the form of the label options, so reused the label options as mock
options. However, they actually are of the form of user input objects with data
and metadata and the input as a label.

We create new mock objects with the correct format, and use them in the tests.

This is the reason why #1150
was not caught for so long.

And when we fixed the code, the test broke.
#1150 (comment)
#1150 (comment)

Testing done:

```
npx jest www/__tests__/confirmHelper.test.ts

Test Suites: 1 passed, 1 total
Tests:       11 passed, 11 total
```
🚑️ 🐛 Ensure that we do show the replaced mode when the `mode_of_inter…
In the previous commit, we had disabled all android background checks.
This means that the optimization check would also be disabled, and we would not
be able to start the foreground service from the background.

Let's change this to only disable to unused apps check
- We want to upgrade the pinned version of the OS to the next one
- We don't want to run on `latest` because then changes to the underlying
  runner will break all tests, including for pull requests, and block
development.

Instead, we should schedule a periodic (~ once a week) check against `latest`
so we know when the latest has changed, and can fix it before again bumping up
the pinned version @louisg1337

Also, there was a typo in `latest` :smile
…ck_for_android_fermata

💩 Turn off the background checks for fleet deployments
@shankari
Copy link
Contributor Author

shankari commented May 5, 2024

New onboarding flow has been done for a while.
React version is on production.
This is obsolete and is gunking up our CI/CD
Closing now...

@shankari shankari closed this May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants