✍ Angular Services needing rewrite

[JGreenlee]

We have rewritten nearly all of the Angular directives and view as React components. So there is very little view/controller code left. However, this is a good amount of JS code still lingering around in Angular services (aka factories).

Prior to the first PR of the migration (before e-mission/e-mission-phone#974), there were ~78 .js files in the www/js directory.
As of 9/15/23, on the branch where we're the furthest ahead in the migration (on e-mission/e-mission-phone#1018) there are 48 .js files, 16 .ts files, 8 .jsx files, and 47 .tsx files.

Of the 48 .js files, many of them actively being replaced right now - and many more of them will be gradually pruned as being obsolete as we progress with the migration.

But the rest of them, (which I estimate is about half), we'll want to address on a separate timeline for the sake of efficiency. Here are a list of files I think will need to be rewritten:

• js/i18n-utils.js (if we still plan to use i18n-ized filenames. I think it would be preferable to just move fully to keyed i18n) // removed, not needed to rewrite
• js/services.js (contains multiple services incl. CommHelper (@JGreenlee), ControlHelper (@the-bay-kay), UnifiedDataLoader (@the-bay-kay). These should be split into separate files)
• js/config/dynamic_config.js (~350 lines @JGreenlee)
• js/config/imperial.js (was already rewritten into useImperialConfig.ts anyway)
• js/config/server_conn.js (@JGreenlee)
• js/control/emailService.js (@niccolopaganini)
• js/control/uploadService.js (@Abby-Wheelis)
• js/diary/services.js (this file was HUGE but has mostly been rewritten already. ~380 lines remain that should be rewritten into timelineHelper.ts (@the-bay-kay)
• js/plugin/storage.js (@JGreenlee)
• js/splash/customURL.js (@jiji14)
• js/splash/localnotify.js // not needed on account of ❓ LocalNotify is not used anymore (is that ok?) #1028
• js/splash/notifScheduler.js (@sebastianbarry)
• js/splash/pushnotify.js (@Abby-Wheelis)
• js/splash/referral.js (I'm not actually sure what this file does or if we need it) // not needed - https://github.com/e-mission/e-mission-phone/pull/1040/files#r1351501452
• js/splash/remotenotify.js (@Abby-Wheelis)
• js/splash/startprefs.js (@Abby-Wheelis)
• js/splash/storedevicesettings.js (@Abby-Wheelis)
• js/stats/clientstats.js (@JGreenlee)
• js/survey/input-matcher.js (@jiji14)
• js/survey/enketo/answer.js (rewrite into js/survey/enketo/enketoHelper.ts) (@Abby-Wheelis)
• js/survey/enketo/infinite_scroll_filters.js (@sebastianbarry)
• js/survey/multilabel/infinite_scroll_filters.js (@sebastianbarry)
• js/survey/enketo/enketo-add-note-button.js (@JGreenlee)
• js/survey/enketo/enketo-trip-button.js (@JGreenlee)
• js/survey/multilabel/multilabel-ui.js (@JGreenlee)
• js/metrics-factory.js (@Abby-Wheelis)
• js/metrics-mappings.js (@Abby-Wheelis)

---

After all these are rewritten, there will be some cleanup tasks (@JGreenlee):

• rewrite what is still needed of ngApp.js into index.js
• ensure getAngularService is not used anywhere
• delete angular-react-helper.tsx (it served us well 🫡)
• delete any remaining, vestigial code: controllers.js, diary.js, main.js, services.js (should be empty), localnotify.js, referral.js, i18n-utils.js, the Angular service from logger.ts
• remove all references to Angular and Ionic
• remove Angular, Ionic, and all related packages

Also needing to happen:

• update / rewrite OpenSourceLicenses.md

[shankari]

Can we try to set up a test framework for pure functions so that we can:

• get used to writing tests?
• use Test Driven Development (TDD) for the rewrite to ensure that we don't have any regressions in the core functionality represented here

Note that since these are pure functions, any errors may not be as easy to spot as errors in more visual, component-based functionality. So it would be great if we could do TDD for this round. We should anticipate it taking longer for the initial write, but requiring fewer rounds once it is done.

[JGreenlee]

Tips for rewriting Angular services into .ts files:

• Find services that don't depend on other services. If you look at service A and it depends on service B, do service B first.
• If a service uses Logger, substitute the functions exported from www/js/plugin/logger.ts. You can use logDebug instead of Logger.info and use displayError instead of Logger.displayError (but note that the parameter order for displayError was reversed because the second param was made optional)
• If a service uses $window, you can just use the plain window object.
  • commonly, the services interface with Cordova plugins via something like $window.cordova.plugins.BEMDataCollection. If you try to access window.cordova.plugins directly, TypeScript will throw type warnings. Accessing it like window['cordova'].plugins will avoid warnings.
• If a service uses $http, substitute the fetch keyword. fetch is universal JavaScript and you can find plenty of documentation for how to use it.
• Write tests as much as possible! For every .ts file we create, we should also create a corresponding .test.ts file located in the www/__tests__ folder. Every function exported from the .ts file (ie anything that can be used by another file) should get a few test cases to make sure the function performs correctly. Test both expected and unexpected inputs!
  Example of a good test file: https://github.com/e-mission/e-mission-phone/blob/master/www/__tests__/diaryHelper.test.ts

[JGreenlee]

I have identified these as services that don't significantly depend on other services and would be good to start with:

• i18n-utils.js (@sebastianbarry)
• emailService.js (@niccolopaganini)
• customURL.js (@jiji14)
• input-matcher.js
• answer.js
• both of the infinite_scroll_filters.js

---

I have also marked the services that I plan to rewrite myself. There are a few big ones like KVStore, ClientStats, and CommHelper that a lot of the others depend on. It would be ideal to get these done quickly, so I will prioritize them this week + next.

[JGreenlee]

@shankari Do you have any guidance on how to test functions that use Cordova plugins?

Should we mock them? I can find some resources online of how to do that, but is there anywhere else in the project that we mock for testing that I could look at?
For example, I'm rewriting KVStore into a new file storage.ts. KVStore heavily uses both local storage and the BEMUserCache plugin. Would it be appropriate to mock both storage solutions? And then, maybe in the tests I could simulate what would happen if one or the other got spontaneously cleared. (We would expect to see the preserved value get filled in from the store that wasn't cleared).

[shankari]

I think that there are a few server tests that use mocking. We haven't had phone tests before, so they don't use mocking.
So you should use the internet tutorials as your template.

For my own edification, is it correct that we can't just call the BEMUserCache plugin because you are running this a javascript environment without access to the cordova plugins?

But even in that case, I don't think you need to mock the localStorage plugin. Can't you just simulate the localStorage being cleared by actually calling clear between storing a variable and trying to access it?

[JGreenlee]

For my own edification, is it correct that we can't just call the BEMUserCache plugin because you are running this a javascript environment without access to the cordova plugins?

Correct. In the context that Jest runs the test in, Cordova does not exist.

I don't think you need to mock the localStorage plugin

localStorage does not exist in that context either.

localStorage exists on all browsers, but Jest doesn't run in a browser. I am guessing it's a Node process, so we can't use browser APIs

---

I see some people saying that Jest automatically mocks localStorage for us. However, I tried it myself and still got ReferenceError: localStorage is not defined

---

I had to scroll down further to find the answer: https://stackoverflow.com/a/74309873
Yes, Jest can mock localStorage for us, but we need to configure it

[sebastianbarry]

After discussion with @shankari , I am going to be changing ConfirmHelper to reproduce the functionality of i18nUtils because we decided that it is no longer needed - more specifically I am going to be making trip_confirm_options populate in the similar way to the dynamic-labels in https://github.com/e-mission/nrel-openpath-deploy-configs/tree/main/label_options

[sebastianbarry]

Per my conversation today with @shankari , I am going to start working on both of the infinite_scroll_filters.js rewrites

[Abby-Wheelis]

What about splash/startprefs.js? It looks like the onboarding process does still use this angular service, and I believe js/splash/localnotify.js, js/splash/pushnotify.js, and js/splash/remotenotify.js all depend on it.

I'd be happy to start on splash/startprefs.js next, else I could work on js/splash/notifScheduler.js which does not seem to depend on any more angular services.

[shankari]

@Abby-Wheelis I wonder if this is the service that is trying to call the notification API during onboarding.
#1006

It is in splash...

[JGreenlee]

What about splash/startprefs.js? It looks like the onboarding process does still use this angular service, and I believe js/splash/localnotify.js, js/splash/pushnotify.js, and js/splash/remotenotify.js all depend on it.

I'd be happy to start on splash/startprefs.js next, else I could work on js/splash/notifScheduler.js which does not seem to depend on any more angular services.

Yes, that will need to be rewritten. Good find!
I think the only reason I left it off the original list is that I was expecting that it would need to be fully rewritten along with the new onboarding. But I left it only partially rewritten because we wanted the onboarding accessibility fixes ASAP.

It ties in closely with onboardingHelper.ts.

[Abby-Wheelis]

I wonder if this is the service that is trying to call the notification API during onboarding.

maybe? I'll certainly keep an eye on it.

This is may be the place to reconsider how we handle consent, since there's several functions here that deal with gathering and saving consent. e-mission/e-mission-phone#1009 (comment) I can get an issue started to discuss if we want to make this change now / what we want to change to.

[Abby-Wheelis]

I'm not 100% if splash/storedevicesettings would need to be rewritten, it is similar in use to startprefs, but when I went to start the re-write I can see that it is not called by any other files. However, the device settings are stored into the user profile with a call to updateUser from commHelper. I did not see a getUser call in the phone code that seemed to correspond to this data. Is this data used in the phone code anywhere, or maybe in the server? I don't want to remove this service unless it's actually unneeded, but I haven't found yet how it is used.

[jiji14]

Same question as Abby! I don't think localNotify is called anywhere, so I'm wondering if it still needs to be rewritten.

[JGreenlee]

@jiji14 Yes LocalNotify is needed. @Abby-Wheelis I am not sure about StoreDeviceSettings yet.

Be careful with these services -- even if none of their functions are explicitly called somewhere else in the codebase, they might have initialization logic that automatically runs when the Angular module is registered.

E.g. with LocalNotify you see line 102:

$ionicPlatform.ready().then(function() {
  localNotify.registerRedirectHandler();
  Logger.log("finished registering handlers, about to fire queued events");
  $window.cordova.plugins.notification.local.fireQueuedEvents();
});

Anything inside $ionicPlatform.ready().then(...) is initialization logic and still needs to happen for our notifications mechanism to work properly.

[the-bay-kay]

Since both of my PR's are pending at the moment, I figured I'd start on the last services.js rewrite! I'm working on moving diary/services.js into timelineHelper.js. I also just noticed that diaryHelper.ts still relies on Moment.js - since I'm already working with the diary directory, should I go ahead and update that first?

[Abby-Wheelis]

I think I'm at a point to start on a new service rewrite (startprefs is advancing through review and I'm waiting to add a few more tests to enketoHelper). It looks like pushnotify might be a good one to start next, and I can continue with the 'afterConsent' and 'afterIntro' changes I had in startprefs.

[JGreenlee]

Just updated the list adding js/metrics-factory.js and js/metrics-mappings.js. Evidently, these 2 flew under the radar, as we did not rewrite them when we made the new dashboard -- so there's a bit more work left to do.

• metrics-factory.js contains FootprintHelper and CalorieCal.
• metrics.mappings.js contains CarbonDatasetHelper, METDatasetHelper, and CustomDatasetHelper.

Technically, none of the calorie and MET code is used right now, but we will want to re-implement something like it later. (Instead of counting the calories burned from active travel (like walking or biking), we want to breakdown how many minutes were spent in high-intensity / moderate intensity / low-intensity active travel. Higher MET = higher intensity. It would also be cool to show MET over time for each active trip.)

Open to discussion about how to organize the rewrites. I think we should definitely break them apart and place them in metrics/. We could extract the standard/base datasets (which are hardcoded in the DatasetHelpers) to separate files which each just export one object. Then we could have one metrics/datasetHelper.ts file to handle toggling/choosing between datasets.
metrics/footprintHelper.ts would be a straightforward rewrite, and something like metrics/metHelper.ts could replace CalorieCal (or as much of it as we want to preserve)

What do others think (especially @Abby-Wheelis who has worked the most closely with the dataset code)?

[Abby-Wheelis]

I agree with breaking these apart into their own files - are you saying that CustomDatasetHelper would become most of datasetHelper.ts (with maybe some of the initialization code from carbon and met)? Because that makes sense to me.

From what I remember, the custom dataset code is used somewhat subtly, but it's very important, especially as more deployments are customizing their mode options, so we just need to make sure that we're still initializing and using the custom dataset when appropriate. discussion about old/new dash mismatch

Overall, I agree with with your plan, that looks like it would make a total of 5 ts files (carbon, met, dataset helper, footprint helper, and met helper) which accounts for the 5 angular modules that currently make up those two files. I think breaking it down is better, and will mean things are more readable when we look back on them later.

[Abby-Wheelis]

We talked about this some more today, and there are a few questions we need to figure out:

• what to do about MET (should we reinstate height/weight/age gathering?
  • pre-migration, we used the MET calculations to get as-accurate-as-possible estimates of calories burned in active transit, now we show active minutes, with no regard for intensity
  • during migration, and again today, we discussed potentially classifying activity into low-med-high intensity. If/when we do this, how accurate do we need the METS to be?
  • if coarse accuracy is OK, I'd propose we base the MET just on mode&speed to bin the intensity
• what to do about the "fallback" carbon datasets?
  • do we use these anymore? This data object shows METs for a set of modes in different countries, and you can change the country in the profile. However, is this still used even after the recent changes to labels?
  • My impression is that projects will either have their own custom labels or fall back to one of our samples. If this is now the case, can we take out the fallback country-specific numbers?

@shankari what do you think about these MET and Carbon considerations?

[shankari]

if coarse accuracy is OK, I'd propose we base the MET just on mode&speed to bin the intensity

1. I think this is fine for now. If/when we reinstate the calorie calculations as an optional "card", we can revisit gathering age/height/weight. Since it is only relevant to that tab, saving those entries be part of that tab.
2. yeah we can take out the fallback country-specific numbers for now. After we finish the rewrite, as part of the big external deliverable, we should rethink the CO2/energy calculations and unify them across the app and the public dashboard.

[JGreenlee]

The rewrite is done and merged to master 🎉