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

Code error while retrieving trips from the server #713

Closed
shankari opened this issue Mar 28, 2022 · 12 comments · Fixed by e-mission/e-mission-phone#815
Closed

Code error while retrieving trips from the server #713

shankari opened this issue Mar 28, 2022 · 12 comments · Fixed by e-mission/e-mission-phone#815

Comments

@shankari
Copy link
Contributor

@shankari
Copy link
Contributor Author

The related code is here:

 441             if (startChecks && !endChecks) {
 442                 if (angular.isDefined(nextTripgj)) {
 443                     endChecks = userInput.data.end_ts <= nextTripgj.data.properties.start_ts;
 444                     Logger.log("Second level of end checks when the next trip is defined("+userInput.data     .end_ts+" <= "+ nextTripgj.data.properties.start_ts+") = "+endChecks);
 445                 } else {
 446                     // next trip is not defined, last trip
 447                     endChecks = (userInput.data.end_local_dt.day == userInput.data.start_local_dt.day)
 448                     Logger.log("Second level of end checks for the last trip of the day");
 449                     Logger.log("compare "+userInput.data.end_local_dt.day + " with " + userInput.data.sta     rt_local_dt.day + " = " + endChecks);
 450                 }

So line 442 is essentially a check to see if the variable is defined. So why are we getting the error?

@shankari
Copy link
Contributor Author

Logged in as this user using the emulator; no error.

@shankari
Copy link
Contributor Author

Tracing back the nextTripgj..., we find a potential mismatch - nextTrip vs. nextTripgj

  dh.getUserInputForTrip = function(trip, nextTrip, userInputList) {
...
                if (angular.isDefined(nextTripgj)) {

But again:

  1. we are literally checking whether the nextTripgj is defined, and
  2. we pushed out the change to staging months ago, so why is it only happening now, and
  3. why is it not happening in the emulator?

@shankari
Copy link
Contributor Author

Ok, so the answer for (2) is that the version on the staging channel (which was a backport) has matching variables.
https://github.com/e-mission/e-mission-phone/blob/ceo_ebike_project_stage/www/js/diary/services.js#L411

@shankari
Copy link
Contributor Author

The answer for (3) is that userInputList.length is consistently 0, so the filter code is never invoked.
Let's try adding some inputs (remembering not to sync) and see if we can reproduce. I assume not, since I have used CanBikeCO on my own phone with unpushed labels and not got any errors.

@shankari
Copy link
Contributor Author

Labeling the trip from Sun 27th at around 8am with fake labels (Pilot e-bike, Transit transfer and Walk).
Found the trip. In this case:

Cleaned trip: comparing user = ... trip = ... start checks are true && true end checks are true || true):undefined

So we don't have to resort to the fallback.

@shankari
Copy link
Contributor Author

shankari commented Mar 28, 2022

Ok, so we only call the fallback when the labels were generated on a draft trip and the end of the draft trip was greater than 15 mins after the end of the real trip. I have only been labeling cleaned trips which may be why I have not encountered this before.

Note also that this only happens when we try to match up the user input after the trip has processed.

So the sequence needs to be:

  • trip is draft
  • user labels trips
  • trip is processed and cleaned end is 15 mins before draft end
  • user input is NOT YET pushed to the server/has not been incorporated into the confirmed trip
  • user loads trips

@shankari
Copy link
Contributor Author

If that is the case, we should be able to test this by adding a new user input with those characteristics for an existing cleaned trip. Let's do this on one of the test accounts to avoid messing up the real staging server.

@shankari
Copy link
Contributor Author

Created fake new labels that have the same start but a vastly truncated end compared to the real trip.
Made sure that the write timestamps were right now to ensure that the values are seen.

Loaded them using

$ ./e-mission-py.bash bin/debug/load_multi_timeline_for_range.py -p shankari /tmp/bad_manual_input

Now at

Cleaned trip: comparing user = 2022-03-27T15:59:14-07:00 -> 2022-03-27T16:34:55-07:00 trip = 2022-03-27T15:59:14-07:00 -> 2022-03-27T16:04:55-07:00 start checks are true && true end checks are false || false):undefined
startChecks: true
endChecks: false
startChecks && !endChecks: true

Aha! Reproduced!

@shankari
Copy link
Contributor Author

shankari commented Mar 28, 2022

Regression caused by e-mission/e-mission-phone@3ec5734

Double-checked the commit to see if there were actually any other misses; there were not.

@shankari
Copy link
Contributor Author

shankari commented Mar 28, 2022

Checked all instances of tripgj in this file and ensured that we hadn't broken anything else

kshankar-35069s:native_code_upgrade kshankar$ grep -ri tripgj www/js/diary/services.js
www/js/diary/services.js:  dh.isDraft = function(tripgj) {
www/js/diary/services.js:    if (// tripgj.data.features.length == 3 && // reinstate after the local and remote paths are unified
www/js/diary/services.js:      angular.isDefined(tripgj.data.features[2].features) &&
www/js/diary/services.js:      tripgj.data.features[2].features[0].properties.feature_type == "section" &&
www/js/diary/services.js:      tripgj.data.features[2].features[0].properties.sensed_mode == "MotionTypes.UNPROCESSED") {
www/js/diary/services.js:  dh.getTripBackground = function(tripgj) {
www/js/diary/services.js:      if (dh.isDraft(tripgj)) {
www/js/diary/services.js:                if (angular.isDefined(nextTripgj)) {
www/js/diary/services.js:                    endChecks = userInput.data.end_ts <= nextTripgj.data.properties.start_ts;
www/js/diary/services.js:                    Logger.log("Second level of end checks when the next trip is defined("+userInput.data.end_ts+" <= "+ nextTripgj.data.properties.start_ts+") = "+endChecks);

@shankari
Copy link
Contributor Author

obvious fix (replace nextTripgj with nextTrip) failed because the structure of the object changed.
Also changing nextTrip.data.properties.start_ts -> nextTrip.start_ts fixed the problem.

First try actual fix
Screenshot_1648494714 Screenshot_1648495973

shankari added a commit to shankari/e-mission-phone that referenced this issue Mar 28, 2022
As part of e-mission@3ec5734
we changed the input from a trip geojson to a trip object. But we forgot to
change one of the code paths, which caused a regression.

Fixed by changing the variable and the format of the variable.

Testing done:
- loaded fake manual entries for a test user
- before the change, got the error
- after the change, did not get the error

This fixes e-mission/e-mission-docs#713
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant