✏️ Rewrite KVStore, ClientStats, and CommHelper #1040

JGreenlee · 2023-09-27T22:42:29Z

As part of the ongoing effort in e-mission/e-mission-docs#977, these are a few of the services that I have rewritten upfront.

These services are widely used throughout the codebase. It's preferable to rewrite these first before attempting the majority of the other services because so many of the other services depend on these 3.

KVStore is fairly long and complex because we use 2 storage solutions for redundancy in case one fails (and now there are tests to verify that it works!)
ClientStats is not complex, but it's used all over the codebase (and we should be able to use it more reliably now that we are using React for tab navigation)
CommHelper is fairly long and critical to the function of the app

This PR also adds tests for the rewritten files. In doing so, I created mocks for a few of the cordova plugins that are used throughout the codebase - this will benefit others in writing their own tests.

storage.ts is replacing storage.js which had the KVStore service inside it. storage.ts will provide a set of functions performing the same duties as KVStorage's functions did. - `get` -> `storageGet` - `set` -> `storageSet` - `remove` -> `storageRemove` - `getDirect` -> `storageGetDirect` - `syncAllWebAndNativeValues` -> `storageSyncLocalAndNative` storage.ts will still use the "BEMUserCache" Cordova plugin in exactly the same way that KVStore did. However, instead of using the `angular-local-storage` package as a wrapper around localStorage, we will use it directly. A couple functions were added ('localStorageSet` and `localStorageGet`) to facilitate using localStorage directly - localStorage requires us to stringify objects before storing, and parse them on retrieval. Other than these substitutions, and being rewritten in modern JS with some typings, the logic is exactly the same as it was in KVStore. --To facilitate this change, storage.js is temporarily renamed to ngStorage.js so that it doesn't conflict with the storage.ts filename. ngStorage.js will be removed soon after it is not used anymore.

storage.ts replaces KVStore, so we can replace all uses of KVStore throughout the codebase, substituting the new functions from storage.ts as follows: - `get` -> `storageGet` - `set` -> `storageSet` - `remove` -> `storageRemove` - `getDirect` -> `storageGetDirect` - `syncAllWebAndNativeValues` -> `storageSyncLocalAndNative`

Rewritten into storage.ts, this Angular service is not needed anymore. We can remove the file, remove it from index.js, and remove it as a dependency from all of the Angular modules that previously used it.

Followed the advice of https://stackoverflow.com/a/74309873 so that we can have a mocked out localStorage to use in tests

To test storage.ts we need to be able to mock the usercache cordova plugin, as well as fill in the missing window.Logger. I created mocks for both of these which are kept in the __mocks__ folder and simply called at the start of storage.test.ts. Because storage.ts calls getAngularService, angular-react-helper.tsx was included in the process. That file imported PaperProvider, which did not work with jest. Because we are not angularizing components anymore, we do not actually need much of the code in angular-react-helper anyway. Let's just take it out so we can safely implicate getAngularService. As for the storage tests themselves, they check whether the storage functions are able to store and retrieve data, and crucially they ensure that local and native storage can compensate for each other if one of them gets cleared.

js/plugin/clientStats.ts will replace js/stats/clientstats.js - camelcase filename convention - we don't need a dedicated folder for stats because clientstats.js was the only thing in it Nothing significant has changed here. I just refactored into modern JS and added type definitions for each function's parameters. mappings of old/new exported methods: - getStatKeys -> statKeys (now a variable, not a function) - getAppVersion -> getAppVersion - getStatsEvent -> getStatsEvent - addReading -> addStatReading - addEvent -> addStatEvent - addError -> addStatError

ClientStats was rewritten into clientStats.ts. This commit simply substitutes in the new methods for the old ones, everywhere that client stats are recorded, as follows: - getStatKeys -> statKeys (now a variable, not a function) - getAppVersion -> getAppVersion - getStatsEvent -> getStatsEvent - addReading -> addStatReading - addEvent -> addStatEvent

Rewritten into a new file clientStats.ts, this old file is not needed anymore. We can remove the file, remove it from index.js, and remove it as a dependency from all of the Angular modules that previously used it.

This is used in ProfileSettings is for the "App Version" row at the very bottom of the Profile page. getAppVersion() was not working correctly - the correct function to call is `getVersionNumber` which returns a promise. In a .then block, we can memoize this value in the local let appVersion. Since it is memoized, we don't need a dedicated state value in ProfileSettings for appVersion. We can just call getAppVersion() directly where it's used in the SettingRow.

There are a few other 'window' variables that are provided in a Cordova/Ionic app and are expected in various few places throughout the codebase. To test, we need to be able to mock these too. mockCordova and mockDevice just provide fake platform/version info. mockGetAppVersion mocks the cordova-plugin-app-version, which is a third-party plugin we use to get the version of the app. This is used in clientStats.ts, so we will need to use this mock when we test that. The mock returns info for a "Mock App", version 1.2.3.

getAppVersion() calls cordova-plugin-app-version's getVersionNumber, which is asynchronous. So getAppVersion should be asynchronous too. I changed it to return a promise. We handle this in clientStats by adopting async/await syntax. We handle this in ProfileSettings by storing it as a ref once the promise is resolved. Then we just access it as appVersion.current.

Tests the methods exported from clientStats.ts, including getAppVersion, addStatReading, addStatEvent, and addStatError. The tests record these stats and then query the usercache for the messages, expecting to see a new entry filled in with the fake data. putMessage and getAllMessages needed to be added to the mockBEMUserCache, since the client stats are recorded as 'messages' rather than 'key-value' pairs

CommHelper from js/services.js is being rewritten into commHelper.ts. This commit moves over the functions from CommHelper that we need. So now the following functions are exported: - fetchUrlCached (was already in commHelper.ts) - getRawEntries - getPipelineRangeTs - getPipelineCompleteTs - getMetrics - getAggregateData - registerUser - updateUser - getUser The following functions were not moved over to the new commHelper.ts because they are not currently used in the codebase and I don't anticipate them being used in the foreseeable future: - putOne - getTimelineForDay - habiticaRegister - habiticaProxy - moment2Localdate - moment2Timestamp - getRawEntriesForLocalDate

CommHelper from js/services.js was rewritten into js/commHelper.ts. This commit switches all functions calls to the new commHelper.ts. All of the functions are named the same and perform the same logic.

Rewritten into commHelper.ts, this Angular service is not needed anymore. We can remove the 'factory' declaration from js/services.js and remove it as a dependency from all of the Angular modules that previously used it.

JGreenlee · 2023-09-28T20:31:13Z

I rewrote KVStore, ClientStats, and CommHelper.
I wrote tests for KVStore and ClientStats, which required creating mocks for many of the external plugins we use.

TODO: write tests for CommHelper

…JGreenlee/e-mission-phone into rewrite-services-sept2023

-Added one test for `fetchUrlCached` - uses a mocked 'fetch' to simulate retreieving data from a URL. It ensures that the second time this URL is queried, the data comes back faster (because it gets cached in the localStorage after the first call) Note at the bottom explains why other functions were not tested.

JGreenlee · 2023-09-29T17:13:44Z

All of the functions in CommHelper, except one, are basically just wrappers around the Cordova plugin (BEMServerComm).

So to test these at all, we would have to mock the entire BEMServerComm, which seems like it would be of limited value. Because at that point the tests would be pretty far removed from reality - we're just testing the mock and nothing else. It would be much better, if possible, to actually test the native code.

For now I am going to test the one function I can (even this requires mocking the fetch API), and leave a note explaining why the others are not tested.

@shankari Is this acceptable to you?

shankari · 2023-09-30T04:34:57Z

@JGreenlee ah I finally see what you were referring to at the team meeting. I agree that unit testing CommHelper is not useful.

However, fully testing timelineHelper (and other services related to the label screen) will need you to return values from the calls to CommHelper to work properly. You might want to mock CommHelper to assist in that, OR we can implement integration testing with a server running using docker-compose.

LMK if that is clear.

JGreenlee · 2023-10-03T16:33:37Z

LMK if that is clear.

The approach is clear.

Although, I don't understand how the integration testing would be implemented. To actually make a call to the server, we would need to make native calls to the BEM server comm plugin, which then would query the server. How would we interface with Cordova plugins through a Github workflow?

And as a more general question, why do we make server calls through native code anyway? Couldn't we do it more easily in JS?

shankari · 2023-10-06T21:57:39Z

Quick reply to document what we discussed in the meeting:

the official cordova plugins have tests that invoke them using javascript (https://github.com/apache/cordova-plugin-camera), which they are able to run using GitHub actions
- note that we already build android and iOS, so launching the emulator itself should be fairly straightforward
- launching the tests in the emulator is likely to be the tricky part
if we can set up a similar environment, we should also be able to call the plugins from our tests
- this will also test the plugins, so we might want to have a discussion around tests that focus on the plugins and tests that use the plugins to test the UI
- or maybe it will turn out that this is an arbitrary distinction, and once you looking at the interaction between the UI and the native code, the modules are so intertwined that it makes sense to just write joint tests

since this involves native code, @louisg1337 might be interested/want to help as well.

shankari

Very clean PR, most changes can be deferred until later.

www/__mocks__/cordovaMocks.ts

shankari · 2023-10-09T21:52:10Z

www/__mocks__/cordovaMocks.ts

+  const mockBEMUserCache = {
+    getLocalStorage: (key: string, isSecure: boolean) => {
+      return new Promise((rs, rj) =>
+        setTimeout(() => {


I assume that the setTimeout is to delay the response.

future fix: you could pass in the timeout as a parameter to the mock (assuming that something like mockBEMUserCache(delay) works and then we could test what happens if the plugins are slow.

www/__mocks__/cordovaMocks.ts

www/__tests__/clientStats.test.ts

www/__tests__/storage.test.ts

www/js/splash/referral.js

www/js/plugin/storage.ts

shankari · 2023-10-10T05:32:09Z

www/js/plugin/storage.ts

+  return unmungeValue(key, localStorageGet(key));
+}
+
+function findMissing(fromKeys, toKeys) {


future fix: It would be good to add a unit test for findMissing as well - it is covered in storageSyncLocalAndNative, but sometimes you can test more extensively by calling the lower function directly (e.g. call it with a blank list, with null etc)

per https://github.com/e-mission/e-mission-phone/pull/1040/files#r1351511049, removes the backwards compat to munge when filling in native values from local storage. Also removes the backwards-compat comment, replacing it with other comments describing how we fill in missing local or native values

A user-facing popup here is likely to just annoy or confuse users. Let's set these log statements at the "WARN" level so it is more visible than other log statements if we do need to debug it, but not intrusive or detrimental to UX

It is fairly self-explanatory that {key: value} was the chosen approach

Per https://github.com/e-mission/e-mission-phone/pull/1040/files#r1351477569, we'll show an error message here if the BEMUserCache 'db' is not defined. We'll also ensure that if the Logger plugin is undefined, we do not try to call it as this would cause an error.

A couple of the functions that were excluded from the rewrite were requested to be added back: e-mission#1040 (comment) The logic is the same, but with modernized syntax, and using Luxon instead of Moment to deal with timestamps

Appending "user-friendy" descriptions to error popups is already handled in logger.ts (see `displayErrorMsg`), so it is unnecessary in this file.

- Add the 2 functions to the list that were re-implemented after the initial rewrite - More accurately describe how we would ideally test server comm

Per e-mission#1040 (comment)

shankari

I think this is now done. I am merging to unblock the others...

JGreenlee added 3 commits September 27, 2023 16:03

remove KVStore / ngStorage.js

6ffbf18

Rewritten into storage.ts, this Angular service is not needed anymore. We can remove the file, remove it from index.js, and remove it as a dependency from all of the Angular modules that previously used it.

JGreenlee marked this pull request as draft September 27, 2023 22:43

JGreenlee changed the base branch from master to onboarding_routing_sept_2023 September 27, 2023 22:43

JGreenlee added 12 commits September 27, 2023 20:54

use JSDOM as jest env so localStorage works

cdabde4

Followed the advice of https://stackoverflow.com/a/74309873 so that we can have a mocked out localStorage to use in tests

remove the old ClientStats / clientstats.js

25c7077

Rewritten into a new file clientStats.ts, this old file is not needed anymore. We can remove the file, remove it from index.js, and remove it as a dependency from all of the Angular modules that previously used it.

use commHelper.ts everywhere, not old CommHelper

1dc7451

CommHelper from js/services.js was rewritten into js/commHelper.ts. This commit switches all functions calls to the new commHelper.ts. All of the functions are named the same and perform the same logic.

remove the old CommHelper service

f78dab1

Rewritten into commHelper.ts, this Angular service is not needed anymore. We can remove the 'factory' declaration from js/services.js and remove it as a dependency from all of the Angular modules that previously used it.

JGreenlee added 2 commits September 29, 2023 00:55

Merge branch 'react_navigation_new_onboarding' of https://github.com/…

5826213

…JGreenlee/e-mission-phone into rewrite-services-sept2023

JGreenlee marked this pull request as ready for review September 29, 2023 17:47

JGreenlee changed the base branch from onboarding_routing_sept_2023 to service_rewrite_2023 October 3, 2023 16:24

JGreenlee mentioned this pull request Oct 3, 2023

Rewrite angular service - customURL #1042

Merged

JGreenlee changed the base branch from service_rewrite_2023 to onboarding_routing_sept_2023 October 4, 2023 16:25

JGreenlee changed the base branch from onboarding_routing_sept_2023 to service_rewrite_2023 October 4, 2023 16:25

Abby-Wheelis mentioned this pull request Oct 6, 2023

⬆️ ☁️ Upload Service -- rewrite and testing #1053

Merged

JGreenlee mentioned this pull request Oct 9, 2023

🧪 Considerations for Integration and End-to-End Testing Strategies e-mission/e-mission-docs#1005

Open

shankari requested changes Oct 10, 2023

View reviewed changes

JGreenlee added 9 commits October 11, 2023 16:42

don't displayErrorMsg in storage.ts

40ce229

A user-facing popup here is likely to just annoy or confuse users. Let's set these log statements at the "WARN" level so it is more visible than other log statements if we do need to debug it, but not intrusive or detrimental to UX

remove unneeded comment

37ac065

It is fairly self-explanatory that {key: value} was the chosen approach

re-implement functions of commHelper

ea2b8c5

A couple of the functions that were excluded from the rewrite were requested to be added back: e-mission#1040 (comment) The logic is the same, but with modernized syntax, and using Luxon instead of Moment to deal with timestamps

don't "processErrorMessages" in commHelper

641e8aa

Appending "user-friendy" descriptions to error popups is already handled in logger.ts (see `displayErrorMsg`), so it is unnecessary in this file.

update comment in commHelper.test.ts

03dc94e

- Add the 2 functions to the list that were re-implemented after the initial rewrite - More accurately describe how we would ideally test server comm

cordovaMocks: use cordova-ios version from json

acb36aa

commHelper: promisify 'registerUser'

e546d73

Per e-mission#1040 (comment)

shankari approved these changes Oct 13, 2023

View reviewed changes

shankari merged commit 5f530f5 into e-mission:service_rewrite_2023 Oct 13, 2023

This was referenced Oct 13, 2023

🎛️ ControlHelper Rewrite #1052

Merged

🔖 [PR] New default label options and translations #1065

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✏️ Rewrite KVStore, ClientStats, and CommHelper #1040

✏️ Rewrite KVStore, ClientStats, and CommHelper #1040

JGreenlee commented Sep 27, 2023 •

edited

Loading

JGreenlee commented Sep 28, 2023

JGreenlee commented Sep 29, 2023 •

edited

Loading

shankari commented Sep 30, 2023

JGreenlee commented Oct 3, 2023

shankari commented Oct 6, 2023

shankari left a comment

shankari Oct 9, 2023

shankari Oct 10, 2023 •

edited

Loading

shankari left a comment

✏️ Rewrite KVStore, ClientStats, and CommHelper #1040

✏️ Rewrite KVStore, ClientStats, and CommHelper #1040

Conversation

JGreenlee commented Sep 27, 2023 • edited Loading

JGreenlee commented Sep 28, 2023

JGreenlee commented Sep 29, 2023 • edited Loading

shankari commented Sep 30, 2023

JGreenlee commented Oct 3, 2023

shankari commented Oct 6, 2023

shankari left a comment

Choose a reason for hiding this comment

shankari Oct 9, 2023

Choose a reason for hiding this comment

shankari Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

shankari left a comment

Choose a reason for hiding this comment

JGreenlee commented Sep 27, 2023 •

edited

Loading

JGreenlee commented Sep 29, 2023 •

edited

Loading

shankari Oct 10, 2023 •

edited

Loading