search

e-mission / e-mission-docs

Repository for docs and issues. If you need help, please file an issue here. Public conversations are better for open source projects than private email.
https://e-mission.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
15 stars 34 forks source link

✍ Angular Services needing rewrite #977

Closed JGreenlee closed 9 months ago

JGreenlee commented 1 year ago

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 https://github.com/e-mission/e-mission-phone/pull/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 https://github.com/e-mission/e-mission-phone/pull/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:

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

Also needing to happen:

shankari commented 1 year ago

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

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 commented 1 year ago

Tips for rewriting Angular services into .ts files:

JGreenlee commented 1 year ago

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

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Abby-Wheelis commented 1 year ago

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 commented 1 year ago

@Abby-Wheelis I wonder if this is the service that is trying to call the notification API during onboarding. https://github.com/e-mission/e-mission-docs/issues/1006

It is in splash...

JGreenlee commented 1 year ago

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 commented 1 year ago

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. https://github.com/e-mission/e-mission-phone/pull/1009#discussion_r1320486300 I can get an issue started to discuss if we want to make this change now / what we want to change to.

Abby-Wheelis commented 1 year ago

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 commented 1 year ago

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

JGreenlee commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

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 commented 1 year ago

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 commented 1 year ago

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

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

shankari commented 1 year ago

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 commented 9 months ago

The rewrite is done and merged to master 🎉