🚑️ 🐛 Ensure that we do show the replaced mode when the `mode_of_inter…

[shankari]

…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

[shankari]

On a godogo config	bike does not show replaced mode	double confirmation that there was only one "bike" in the labeloptions	After the fix, it is shown properly

[shankari]

I am going to merge this tomorrow morning and submit for review, given that it is a showstopper.

[shankari]

This is almost certainly already on production, so checking what happens when the fixed code is sent out by installing version 1.7.4 first and then upgrading.

all trips shown as labeled (v1.7.4)	they were e-bike and bike (v1.7.4)	after upgrade, the bike trip showed up as needing to label the replaced mode (phew!)

[shankari]

I looked at the git blame and it looked like this had not been changed since the before the rewrite - it still used userInput.Mode.value which was the old copy-over label value mapping way of accessing the input. So I installed binary versions of the app going back to the rewrite and it has been broken since February!!! We didn't notice, and none of the users noticed either. We haven't heard anything back about it, not even from the current e-bike deployers (DDOT/DOEE/ca-ebike). @JGreenlee am I correct?!

If so, then from #1150 (comment), people might get a whole slew of "To label" trips for trips that they had already labeled. That's good, because our data will be nice again, but we may want to send a heads-up, both as a notification, and to the current deployers in case they get a slew of questions. Maybe we can combine it with the upcoming stacked bar chart change. @JGreenlee can you draft this update?

v1.6.9 is broken	v1.6.6 is broken	v1.6.3 broken	v1.5.6 works

[shankari]

The tests are failing as below. So we actually had a unit test for this, but apparently the test was wrong??!

  ● confirmHelper › returns full labelInputDetails for a labelUserInput which has the mode of study

    expect(received).toEqual(expected) // deep equality

    - Expected  - 6
    + Received  + 0

    @@ -9,12 +9,6 @@
          "choosetext": "diary.choose-purpose",
          "key": "manual/purpose_confirm",
          "labeltext": "diary.purpose",
          "name": "PURPOSE",
        },
    -   "REPLACED_MODE": Object {
    -     "choosetext": "diary.choose-replaced-mode",
    -     "key": "manual/replaced_mode",
    -     "labeltext": "diary.replaces",
    -     "name": "REPLACED_MODE",
    -   },
      }

      83 |     );
      84 |     const fullLabelInputDetails = getLabelInputDetails(fakeAppConfigWithModeOfStudy);
    > 85 |     expect(labelInputDetails).toEqual(fullLabelInputDetails);
         |                               ^
      86 |   });
      87 |
      88 |   it(`converts 'other' text to a label key`, () => {

      at Object.toEqual (www/__tests__/confirmHelper.test.ts:85:31)

[shankari]

Ah, it looks like the text expects that the MODE will be filled in with the entry from the label option

    { value: 'walk', baseMode: 'WALKING', met_equivalent: 'WALKING', kgCo2PerKm: 0 },

But it is in fact filled in with the user input object, with data and metadata

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.42%. Comparing base (8a9d9b3) to head (20b9e1a).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1150   +/-   ##
=======================================
  Coverage   26.42%   26.42%           
=======================================
  Files         114      114           
  Lines        4980     4980           
  Branches     1026     1062   +36     
=======================================
  Hits         1316     1316           
+ Misses       3664     3662    -2     
- Partials        0        2    +2     

Flag	Coverage Δ
unit	26.42% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
www/js/survey/multilabel/confirmHelper.ts	97.11% <100.00%> (ø)

... and 2 files with indirect coverage changes

[JGreenlee]

I am glad you caught this. I believe your investigation is correct; when I unified the format of ENKETO vs MULTILABEL user inputs on the phone (which was the first step of e-mission/e-mission-docs#1045), I missed this bit of code and the corresponding unit test.
Yes, I think all trips that were labeled as e-bike during that time will show back up in "To Label" with this fix.

---

Since this was a pretty serious issue, I want to take a moment and reflect on it.

I think there are a couple things that could have helped prevent this issue from going undetected:

1. better type safety (if labelInputDetailsForTrip had typed arguments this would have been caught by the TS linter)
2. testing REPLACED_MODE in my own day-to-day manual testing
   (I don't own an ebike, so I don't usually test 'replaced mode'. I could fake it for the purpose of testing though)
3. integration + end-to-end testing

Type safety is an ongoing effort; we have been gradually adding types throughout the codebase wherever possible.

As OpenPATH grows to support more deployments with differing configurations, I think it is growing increasingly difficult to validate the full range of features with manual testing. We cover as much as we can with our small team

Integration and end-to-end testing have been long-term goals, but maybe one key takeaway from this is how crucial they are to the long-term success of the project. When we have time, we should really sit down and devote some time to those, or at least form a concrete plan for when and how we can implement them (like what can we do before the cordova migration vs. what should we wait until after to do?)