🏗️🗃️ UnifiedDataLoader rewrite #1064

the-bay-kay · 2023-10-12T18:10:01Z

UnifiedDataLoader is the last remaining service in /www/services.js to be rewritten, as discussed in Issue 977. The remaining services in this file (Chats and ReferHelper) are no longer being used, so my plan is to remove these and then convert services.js into unifiedDataLoader.ts.

I did have an open question about the services.js rewrite - once all of the rewritten services are together, (commHelper, controlHelper, and unifiedData), should these be relocated into a directory other than /www/js/? I'm not sure whether these would be moved into a new services/ directory, sorted into the current folders, or kept as they are. I figured it was worth thinking about how we wanted to organize these going forward, since the /www/js/ has expanded a bit!

The 'Chats' and 'ReferHelper' services, alongside their derivative services, have not been used for quite some time. These were removed in preparation for the UnifiedDataLoader-rewrite.

the-bay-kay · 2023-10-12T23:09:24Z

Just a quick observation about the behavior or getUnifiedMessagesForInterval: When called by /www/js/survey/enketo/enketoHelper.ts, the return value (serverResponse.phone_data) always seems to return blank. I.e., the following snippet:

var remotePromise = getRawEntries([key], tq.startTs, tq.endTs)
          .then(function(serverResponse: serverData) {
            console.log('-->', JSON.stringify(serverResponse.phone_data));
            return serverResponse.phone_data;
          });

Results in the output:

[0] [phonegap] [console.log] --> []

Should this always be empty? I've tried this on a few different emulators, timelines, and surveys: all of which had the same result. If this isn't intentional, I can look into it further - regardless, I'll be committing and pushing my rewrite soon!

- Moved UnifiedDataLoader to a separate typescript file. - The method is no longer an angular service, and instead exports two 'getUnified' methods. - Updated other files that rely on this service. - Tested getUnifiedMessagesForoInterval, as used in enketoHelper.

the-bay-kay · 2023-10-12T23:36:47Z

From the manual tests I've done, the rewrite seems to be performing well! I did have two more questions:

The older code mentions refactoring the two getUnified methods into one generalized method. Is it worth trying to do this, while I'm already re-writing the functions? I'll go ahead and try to rewrite these tomorrow morning.
Under what context is getUnifiedSensorDataForInterval() called? From what I can tell, it's used in timelineHelper.ts for the getAllUnprocessedInputs() method, which in turn is called in the LabelTab components. After some time experimenting with different emulators and timelines, I could never get hit the breakpoints I put on these methods. Any advice on testing these functions would be greatly appreciated!

Edit: I may be wrong, but I believe these are called during the initial loading and refresh processes! I've also gone ahead and rewritten getUnified, in 36cfff6

the-bay-kay · 2023-10-13T15:16:54Z

As discussed in PR #1052, switching the branches cleans the git logs to only contain my edits - I also updated the title, since I accidentally used a duplicate in icons : )

- Generalized getUnified functions into one method that takes a higher order function as input, and uses that to fetch required data. - Updated calls to the getUnified functions - Added an interface for the combineWithDedup method, because it requires objects with the 'metadata.writets' property to run - Updated other unifiedDataLoader functions so the parameters were appropriately typed

the-bay-kay · 2023-10-13T22:12:09Z

Quick Update: Now that the unifiedData functions have been generalized, I'm working on generalizing combinePromise so that it can iterate over a list of promises (Like the original To-Do comment suggested).

My general approach has been to recursively build up the promises. For example, if we had an array of promises [p1, p2, p3], they would be combined in the order (p3+p2) + p1. Since this function takes two promises as input, and returns a promise, I think this is a relatively intuitive change - that being said, I've had some difficulty formatting the base case.

I'm pretty sure I'm making some jumps in my logic and missing something obvious - I'll spend some more time on the recursive approach, and switch gears if that doesn't go anywhere.
(Oh, and a note on the force-push above - that was a git commit --amend to fix a type on in my commit title!)

- Updated combinePromise to now take a list of one or more promises as input, and recursively combine them to a single promise. - Added parameter explanations to each function - Cleaned up debuging console.log statements

- While the types themselves may be subject to change, the goal is to set up a good foundation for strict typing in the future! As such, I added some to a basic version of Jiji's diaryTypes (see PR e-mission#1061), and will be updating those regularly.

Many thanks to Jiji for doing the heavy lifting on this one in PR 1061! - Fixed indents - Switched ServerData from an interface to a Generic Type. After doing some further reading, the [TS Docs](https://www.typescriptlang.org/docs/handbook/2/objects.html#interfaces-vs-intersections) seem to suggest that this is a better way of handling container-style types. - Added basic ServerReponse type - this is often how data is wrapped when returned from functions like `getRawData()`

- Minor changes to unifiedDataLoader: adjusted logInfo, exports - Wrote test cases for combinedPromises

the-bay-kay · 2023-10-24T22:49:14Z

All tests have been passed successfully! Will be testing on Android before marking as Ready for Review.

- Updated types to match PR e-mission#1073 - Part of the motivaiton for this is that `/dairy/services.js` also relies on unifiedDataLoader; if this is written using those types, then that rewrite can build off of this rewrite without any additional merge conflicts down the road.

shankari · 2023-10-27T02:42:44Z

I did have an open question about the services.js rewrite - once all of the rewritten services are together, (commHelper, controlHelper, and unifiedData), should these be relocated into a directory other than /www/js/? I'm not sure whether these would be moved into a new services/ directory, sorted into the current folders, or kept as they are. I figured it was worth thinking about how we wanted to organize these going forward, since the /www/js/ has expanded a bit!

Good thought! I vote for moving them into a new services/ directory. I think that the directories are within www/js (e.g. www/js/diary) though

shankari · 2023-10-27T02:48:04Z

When called by /www/js/survey/enketo/enketoHelper.ts, the return value (serverResponse.phone_data) always seems to return blank. I.e., the following snippet:

What is the key that the enketoHelper is trying to read? If it is the demographic result, it is not included in the fake data that we load, so it will always be empty. To check if it is non-empty, you need to:

save the onboarding survey and "end trip and force sync" OR
use a stage/production token

If the key is a trip or place confirm or an addition, it may just be that your data has not been pushed to the server. Pick a stage token that is related to time use (I think I have one in the wiki) and test; you should see some results.

Please report the results of this testing before I review further.

shankari · 2023-10-27T02:49:44Z

Edit: I may be wrong, but I believe these are called during the initial loading and refresh processes! I've also gone ahead and rewritten getUnified, in 36cfff6

Initial loading of the label screen, yes.

the-bay-kay · 2023-11-03T21:21:58Z

At the time of writing, this does not pass the 'prettier' check because of index.html - please see PR #1096 for more info!

www/js/services.js

www/js/services/unifiedDataLoader.ts

JGreenlee · 2023-11-06T15:51:32Z

www/js/services/unifiedDataLoader.ts

+ * @param combiner a function that takes two arrays and joins them
+ * @returns A promise which evaluates to a combined list of values or errors
+ */
+export const combinedPromises = function (


I was unsure why we have this big function just to join a list of promises, when Promise.all already serves that purpose. I read back in the commit history and saw this approach was taken because Promise.all fails if any one of the promises fails 34c5658 -- and we want local retrieval to work even when network is unavailable. That makes sense.

But now in 2023, I think we could use Promise.allSettled (which probably was not supported by major browsers in 2017) and greatly simplify this.

We would still need to dedup -- I think after the Promise.allSettled call we would just merge with dedup

I don't think it's necessary to do this before merging this PR, but it might be good to keep around as a 'future change'

I didn't know Promise.allSettled was an option, cool! I'll spend a bit of time seeing if I can get that to work : )

Alas, this is still a pretty large function. I think it can be maybe be simplified without using a for loop, so unresolving this for a future cleanup while still merging.

I'm thinking something like map((result) => result.status != 'fulfilled').reduce((a, b) => a && b);

www/js/types/serverData.ts

Co-authored-by: Jack Greenlee <[email protected]>

- combinedPromises now uses `Promise.allSettled()` - rewrote tests to reflect change

JGreenlee · 2023-11-07T00:17:07Z

www/js/services/unifiedDataLoader.ts

+ * @param list An array of values from a BEMUserCache promise
+ * @returns an array with duplicate values removed
+ */
+export const removeDup = function (list: Array<ServerData<any>>) {


Note to self: we do the same deduplication here as we do in https://github.com/e-mission/e-mission-phone/pull/1086/files#diff-00cfb43594908885f4c2e9121a2224b2c080d9b96bff2e09ec6d4f8d8c6b0645R102-R104

Once both are merged it would be nice to use a common function in both places

Leaving this open as a future cleanup

www/js/services/unifiedDataLoader.ts

Co-authored-by: Jack Greenlee <[email protected]>

the-bay-kay · 2023-11-07T17:02:14Z

I'm still unsure why the LoadMoreButton tests are failing in the github action - fwiw, the tests pass on my local machine!

JGreenlee

LGTM

shankari · 2023-11-10T03:55:37Z

@the-bay-kay this now has merge conflicts again. I am going to try to resolve them to avoid an infinite cycle of changes.

shankari · 2023-11-10T04:09:10Z

Ah, it failed because of prettier and I can't see anything obviously wrong with the file that it is flagging...
@the-bay-kay can you fix this tomorrow? I will review and merge...

JGreenlee · 2023-11-10T04:47:42Z

I would fix it tonight if I could, but I don't have write access to this one

JGreenlee · 2023-11-10T04:52:49Z

@shankari Looks like the only file not passing Prettier is diaryTypes.ts. So if you wish to merge this tonight anyway, I can take care of diaryTypes.ts tomorrow in #1086 and it will all even out

the-bay-kay · 2023-11-10T16:21:46Z

Looks like the only file not passing Prettier is diaryTypes.ts. So if you wish to merge this tonight anyway, I can take care of diaryTypes.ts tomorrow in #1086 and it will all even out

~~If you'd like, I can fetch the current code, pull from service_rewrites_2023 to resolve the merge conflicts, and then run prettier on the whole thing before re-pushing the code! ~~
Merged, prettier'd, and pushed!

shankari · 2023-11-10T17:21:22Z

@the-bay-kay as you can see, there is a failing test and it is not LoadMoreButton. Can you please fix before I review?

the-bay-kay · 2023-11-10T17:50:42Z

@the-bay-kay as you can see, there is a failing test and it is not LoadMoreButton. Can you please fix before I review?

Fixed! I missed a couple imports in the merge, all tests are passing now.

- Fixed imports

shankari

This touches 46 files, so I am merging it now. But I expect that this will be cleaned up in your next PR.

shankari · 2023-11-11T03:24:16Z

www/js/metrics/MetricsTab.tsx

@@ -72,7 +72,7 @@ const MetricsTab = () => {
        setAggMetrics(metrics as MetricsData);
      }
    } catch (e) {
-      displayError(e, t('errors.while-loading-metrics'));
+      logWarn(e + t('errors.while-loading-metrics')); // replace with displayErr


Flagging this as hack that we need to revert during cleanup

shankari · 2023-11-11T03:25:50Z

www/js/services/unifiedDataLoader.ts

+ * @param list An array of values from a BEMUserCache promise
+ * @returns an array with duplicate values removed
+ */
+export const removeDup = function (list: Array<ServerData<any>>) {


Leaving this open as a future cleanup

shankari · 2023-11-11T03:32:17Z

www/js/diary/services.js

+            const tripProps = points2TripProps(filteredLocationList);
+
+            return {
+              ...tripProps,


nit: I thought that prettier would ensure that we wouldn't have this extraneous whitespace.
Is this related to the fact that prettier changes the function to one line and so the indentation changes.

function ( $http, $ionicLoading, $ionicPlatform, $window, $rootScope, UnifiedDataLoader, Logger, $injector, ) { ------ function ($http, $ionicLoading, $ionicPlatform, $window, $rootScope, Logger, $injector) {

shankari · 2023-11-11T03:36:09Z

www/js/services/unifiedDataLoader.ts

+ * @param combiner a function that takes two arrays and joins them
+ * @returns A promise which evaluates to a combined list of values or errors
+ */
+export const combinedPromises = function (


Alas, this is still a pretty large function. I think it can be maybe be simplified without using a for loop, so unresolving this for a future cleanup while still merging.

shankari · 2023-11-11T03:45:22Z

www/js/services/unifiedDataLoader.ts

+ * @param combiner a function that takes two arrays and joins them
+ * @returns A promise which evaluates to a combined list of values or errors
+ */
+export const combinedPromises = function (


I'm thinking something like map((result) => result.status != 'fulfilled').reduce((a, b) => a && b);

shankari · 2023-11-11T03:48:18Z

www/js/types/serverData.ts

+  write_local_dt: LocalDt;
+};
+
+export type LocalDt = {


aren't these also defined elsewhere? I seem to remember merging the inputMatcher with similarly defined new types. Why is there a second copy of them?

shankari · 2023-11-11T03:49:10Z

www/js/types/fileShareTypes.ts

+
+export type TimeStampData = ServerData<RawTimelineData>;
+
+export type RawTimelineData = {


where is this used? The data we get from the server should be ServerData with both metadata and data

Removed stale code

83a4939

The 'Chats' and 'ReferHelper' services, alongside their derivative services, have not been used for quite some time. These were removed in preparation for the UnifiedDataLoader-rewrite.

the-bay-kay changed the title ~~UnifiedDataLoader rewrite~~ 🗃️ UnifiedDataLoader rewrite Oct 12, 2023

the-bay-kay changed the base branch from service_rewrite_2023 to master October 13, 2023 15:15

the-bay-kay changed the base branch from master to service_rewrite_2023 October 13, 2023 15:15

the-bay-kay changed the title ~~🗃️ UnifiedDataLoader rewrite~~ 🏗️🗃️ UnifiedDataLoader rewrite Oct 13, 2023

the-bay-kay force-pushed the UnifiedDataLoader-rewrite branch from 6988d17 to 36cfff6 Compare October 13, 2023 18:32

Generalized combinePromises, improved TS

b7abd98

- Updated combinePromise to now take a list of one or more promises as input, and recursively combine them to a single promise. - Added parameter explanations to each function - Cleaned up debuging console.log statements

JGreenlee mentioned this pull request Oct 17, 2023

Rewrite answer.js into enketoHelper.ts #1063

Merged

the-bay-kay added 8 commits October 19, 2023 14:40

Added types directory, updated TS

e6abae8

- While the types themselves may be subject to change, the goal is to set up a good foundation for strict typing in the future! As such, I added some to a basic version of Jiji's diaryTypes (see PR e-mission#1061), and will be updating those regularly.

Added first unifiedDataLoader test

1674d46

Added rudimentary type interfaces for server data

8d765d0

Added types for controlHelper.ts

49e1757

Added tests for combinedPromises

7dd8ffb

- Minor changes to unifiedDataLoader: adjusted logInfo, exports - Wrote test cases for combinedPromises

Updated tests, formatted test file using Prettier

1907966

Code format changes to unifiedDataLoader.ts

46c1429

the-bay-kay marked this pull request as ready for review October 24, 2023 22:52

shankari changed the base branch from service_rewrite_2023 to master October 25, 2023 04:14

shankari changed the base branch from master to service_rewrite_2023 October 25, 2023 04:14

Ran prettier on remaining files

d9285d3

JGreenlee requested changes Nov 6, 2023

View reviewed changes

the-bay-kay and others added 5 commits November 6, 2023 08:38

Update www/js/services/unifiedDataLoader.ts

59503e6

Co-authored-by: Jack Greenlee <[email protected]>

Removed old UnifiedDataLoader

9a9cd92

Merge gitHub change to local repo

2a04319

Refactored combinedPromises

000f08d

- combinedPromises now uses `Promise.allSettled()` - rewrote tests to reflect change

Prettier cleanup

71bee6f

JGreenlee reviewed Nov 7, 2023

View reviewed changes

www/js/services/unifiedDataLoader.ts Outdated Show resolved Hide resolved

Update www/js/services/unifiedDataLoader.ts

bafdb47

Co-authored-by: Jack Greenlee <[email protected]>

JGreenlee approved these changes Nov 7, 2023

View reviewed changes

Merge branch 'service_rewrite_2023' into UnifiedDataLoader-rewrite

99534bf

the-bay-kay added 2 commits November 10, 2023 08:33

Merge branch 'service_rewrite_2023' into UnifiedDataLoader-rewrite

808ab2e

Ran Prettier after merge

25f7adf

JGreenlee mentioned this pull request Nov 10, 2023

📊 🦶 Metrics Services Rewrites #1098

Merged

Fixed imports

fa95f97

the-bay-kay added 2 commits November 10, 2023 15:02

Merge branch 'service_rewrite_2023' into UnifiedDataLoader-rewrite

c2bb442

Merge cleanup

7e8f07b

- Fixed imports

shankari approved these changes Nov 11, 2023

View reviewed changes

shankari merged commit cf9eee0 into e-mission:service_rewrite_2023 Nov 11, 2023
2 checks passed

shankari mentioned this pull request Feb 3, 2024

🎀 Final change of the service rewrite #1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🏗️🗃️ UnifiedDataLoader rewrite #1064

🏗️🗃️ UnifiedDataLoader rewrite #1064

the-bay-kay commented Oct 12, 2023

the-bay-kay commented Oct 12, 2023

the-bay-kay commented Oct 12, 2023 •

edited

Loading

the-bay-kay commented Oct 13, 2023

the-bay-kay commented Oct 13, 2023 •

edited

Loading

the-bay-kay commented Oct 24, 2023 •

edited

Loading

shankari commented Oct 27, 2023

shankari commented Oct 27, 2023

shankari commented Oct 27, 2023

the-bay-kay commented Nov 3, 2023

JGreenlee Nov 6, 2023

JGreenlee Nov 6, 2023

JGreenlee Nov 6, 2023

the-bay-kay Nov 6, 2023

shankari Nov 11, 2023

shankari Nov 11, 2023

JGreenlee Nov 7, 2023

shankari Nov 11, 2023

the-bay-kay commented Nov 7, 2023

JGreenlee left a comment

shankari commented Nov 10, 2023

shankari commented Nov 10, 2023 •

edited

Loading

JGreenlee commented Nov 10, 2023

JGreenlee commented Nov 10, 2023

the-bay-kay commented Nov 10, 2023 •

edited

Loading

shankari commented Nov 10, 2023

the-bay-kay commented Nov 10, 2023

shankari left a comment

shankari Nov 11, 2023

shankari Nov 11, 2023

shankari Nov 11, 2023

shankari Nov 11, 2023

shankari Nov 11, 2023

shankari Nov 11, 2023

shankari Nov 11, 2023


		export type TimeStampData = ServerData<RawTimelineData>;

		export type RawTimelineData = {

🏗️🗃️ UnifiedDataLoader rewrite #1064

🏗️🗃️ UnifiedDataLoader rewrite #1064

Conversation

the-bay-kay commented Oct 12, 2023

the-bay-kay commented Oct 12, 2023

the-bay-kay commented Oct 12, 2023 • edited Loading

the-bay-kay commented Oct 13, 2023

the-bay-kay commented Oct 13, 2023 • edited Loading

the-bay-kay commented Oct 24, 2023 • edited Loading

shankari commented Oct 27, 2023

shankari commented Oct 27, 2023

shankari commented Oct 27, 2023

the-bay-kay commented Nov 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-bay-kay commented Nov 7, 2023

JGreenlee left a comment

Choose a reason for hiding this comment

shankari commented Nov 10, 2023

shankari commented Nov 10, 2023 • edited Loading

JGreenlee commented Nov 10, 2023

JGreenlee commented Nov 10, 2023

the-bay-kay commented Nov 10, 2023 • edited Loading

shankari commented Nov 10, 2023

the-bay-kay commented Nov 10, 2023

shankari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

the-bay-kay commented Oct 12, 2023 •

edited

Loading

the-bay-kay commented Oct 13, 2023 •

edited

Loading

the-bay-kay commented Oct 24, 2023 •

edited

Loading

shankari commented Nov 10, 2023 •

edited

Loading

the-bay-kay commented Nov 10, 2023 •

edited

Loading