Rewrite answer.js into enketoHelper.ts #1063

Abby-Wheelis · 2023-10-11T21:24:08Z

This PR is based on Jack's rewrite PR so that I can work off his mocks and hopefully minimize merge conflicts once that branch can be merged.

Addresses one of the rewrites in: ✍ Angular Services needing rewrite

…to expand-enketo-helper

moving the methods form answer.js into enketoHelper as a part of the services migration

When testing, I was getting an error from Jest about duplicate modules, one of which was in platforms. This change resolves that error

Abby-Wheelis · 2023-10-11T21:27:22Z

I have discovered an issue here, when I try to add an activity to a trip or a place in the emulator, that activity does not show up in a list on the card as expected. Hopefully I can find the issue and resolve in the re-write and test process.

additional test for filterByNameAndVersion fake answers have been constructed to be filtered

completely remove answer.js and all references to it, replace references with references to enketoHelper.ts

Abby-Wheelis · 2023-10-12T15:27:59Z

One of the things that's popping up with testing, is the need for a fake survey. I had been copying part of the document from somewhere like trip-end-survey.json and trying to fill in answers, but I'm not sure that's really working the way I think it is. I wonder if there's a way for me to download the xml for a filled-out survey from a tool like kobotoolbox? I've tried looking at log statements, but it cuts the printing of the xml off in a console.log statement, which makes sense given how long the files can be.

resolveTimestamps is a helper function to saveResponse, but still contains a fair amount of its own logic. Testing the edge cases for this function ensures that it will behave as expected within the larger context

Abby-Wheelis · 2023-10-12T17:46:42Z

One of the things that's popping up with testing, is the need for a fake survey.

I was able to clear this for the test I was working on in that moment by just copying a part of the survey from the logs.

Now, I'm facing an issue with loading the config, in the process of trying to test resolveLabel. I think my next step is to test _lazyLoadConfig independently to make sure that is working, with the web of calls to storage that I might need to mock (some of them are already mocked). What's odd is that I have tests for filterByNameAndVersion already running, and those depend on _lazyLoadConfig so my problem might be elsewhere.

adding a basic test for _loadLazyConfig, in order to ensure that my mock setup for that works, before moving into testing functions that depend on it

Abby-Wheelis · 2023-10-12T18:34:06Z

test _lazyLoadConfig independently to make sure that is working, with the web of calls to storage that I might need to mock (some of them are already mocked).

I did this by mocking getDocument in the cordova mocks in order to return a hard-coded of a config with just enough information to have one survey. This works for now, and can be expanded later as needed.

Abby-Wheelis · 2023-10-12T21:30:29Z

I've moved on try trying to test resolveLabels and to do that I need i18next and MessageFormat. I have been trying to find a way to mock these and allow the tests to access the mocks, but so far have been unsuccessful. Here's some information I found about Manual Mocks in Jest. It seemed like a good place to start, but I have not been able to have the code running in the tests access the mocks, it just seems to fail as undefined for some reason.

currently struggling with i18next and MessageFormat, as I can't get either of those mocked and working

Abby-Wheelis · 2023-10-12T23:07:15Z

I have discovered an issue here, when I try to add an activity to a trip or a place in the emulator, that activity does not show up in a list on the card as expected. Hopefully I can find the issue and resolve in the re-write and test process.

I have not found a way to fix this yet. I did decide to check on master, and it works there. The other difference between this branch and master is that the demographics survey does not save between logins on this branch.

On service_rewrite_2023, the activities show up after I input them and persist after refreshing the label screen, but don't stay after logging out and back in. The demographic survey responses don't show up after a new login either.

I also checked out JGreenlee:rewrite-services-sept2023, since those changes are in this branch too. Not saving between logins is observed here, but when I input activities they show up, so there are no regressions between service_rewrite_2023 and the end of Jack's changes.

To summarize:

the demographic survey no longer saves between logins due to changes between master and service_rewrite_2023
in my migration I broke saving the activity survey responses and having them show on the label screen

I can investigate further tomorrow, but I have a better idea now of when the regressions took place.

the message format plugin moved! https://github.com/messageformat/messageformat/tree/main/packages/core

JGreenlee · 2023-10-13T15:38:25Z

On service_rewrite_2023, the activities show up after I input them and persist after refreshing the label screen, but don't stay after logging out and back in. The demographic survey responses don't show up after a new login either.

After talking, we think this is actually the expected behavior. The log out button should clear everything. If we do not "force sync" before logging out, any unsynced data is lost. This is why we have the "Are you sure?" popup explaining this to users before they log out

Abby-Wheelis · 2023-10-13T15:46:22Z

Some updates after meeting with @JGreenlee this morning to discuss the problems I've been having:

Instead of trying to mock i18next and messageformat, it is better to use the actual packages. To do this we configured i18next in the test file, which should work after I incorporate the changes from React Component Testing Setup. Jack also helped me figure out that we were using a different messageformat package than we thought, and after updating to the new @messageformat/core the test is progressing further.

Next steps:

incorporate the changes from React Component Testing Setup #1049
update my mock config to resolve testing error with messageformat
finish writing tests
see if forcing sync causes the demographic survey to save
investigate typings to make sure they are accurate and in typescript
debug the activities not saving to restore

setting __DEV__ to false in globals, so that it can be used throught the testing suit calling the i18n setup in the tests should work once we incorporate the React testing changes

remove old i18n code, update types, comment out broken tests

from log statements, these answers have data and metadata, no labels

ensured that tests still pass

with the changes from e-mission#1049, we are now able to test using i18n, no need to mock!

introduced i18next for the tests updated config in mock, and test of loading config to be accurate to what is expected (missing some '{' ) adjust formatting of function indentation

now that I understand how this function works, I got the xml (filled and unfilled) directly from console.log statements. The tests are now accurate, and cover each of the cases.

Abby-Wheelis · 2023-10-13T21:31:22Z

I just learned something helpful for testing -- there were a couple of universal packages that were throwing errors in the tests. When I moused over them, they described, for example, that URL was an alias for require('url').URL. I was able to configure testing and get this to work by adding the following to my tests:

global.URL = require('url').URL;
global.Blob = require('node:buffer').Blob;

It was a quick and easy fix (which lots of testing configuration has not been) so documenting for future reference.

testing for saving the response, both when it works and when the timestamps are invalid, resulting in an error

www/__mocks__/cordovaMocks.ts

JGreenlee · 2023-11-17T18:11:24Z

www/__tests__/enketoHelper.test.ts

+global.URL = require('url').URL;
+global.Blob = require('node:buffer').Blob;


I'm curious about this - why was it needed?
Do Node environments not have these as globals but browser environments do?

I remember getting an error about this, I don't remember exactly what the error was, but I found that the best way to fix it was to set it like this. comment about this. I think the issue had something to do with the testing environemnt

www/__tests__/enketoHelper.test.ts

www/__mocks__/cordovaMocks.ts

www/__tests__/enketoHelper.test.ts

www/js/survey/enketo/enketoHelper.ts

JGreenlee · 2023-11-17T18:58:47Z

www/js/survey/enketo/enketoHelper.ts

+/** @type {EnketoSurveyConfig} _config */
+let _config: EnketoSurveyConfig;
+
+/**
+ * _lazyLoadConfig load enketo survey config. If already loaded, return the cached config
+ * @returns {Promise<EnketoSurveyConfig>} enketo survey config
+ */
+export function _lazyLoadConfig() {
+  if (_config !== undefined) {
+    return Promise.resolve(_config);
+  }
+  return getConfig().then((newConfig) => {
+    logInfo('Resolved UI_CONFIG_READY promise in enketoHelper, filling in templates');
+    _config = newConfig.survey_info.surveys;
+    return _config;
+  });
+}


After analyzing a bit further, I think this function is pointless.
getConfig() is already cached/lazyloaded, so we could just call it directly. The only difference is that getConfig() returns the whole config obj while this function only returns the survey_info.surveys sub-field of the config obj.

I think it would be clearer to remove this function. Anywhere it was used, call getConfig() and access the subfields from there.
Then move your EnketoSurveyConfig type def to js/types/appConfigTypes.ts (it doesn't exist yet on this branch but I started it on your other PR #1098)
The reasoning is that eventually we're going to want a single source of truth for the entire config – not scattered typings for each sub-field of the config.

I made those changes in d123b4f, including moving the type. I did not work the type into the existing appConfig type yet though

we also want to test that the timestamps follow the "minute" accuracy convention we implemented, so that if the survey and timelineEntry match within the minute, we use the timelineEntry timestamps, else use the timestamps from the survey e-mission#1063 (comment)

this form is invalide because of the start and end times mismatching

the function was not all that necessary, so I removed it Also moved my survey config type into the appropriate place e-mission#1063 (comment)

adding a response that should get filtered out because the version is too low e-mission#1063 (comment)

Because of the new parameter, I was able to add more test cases to resolve Labels I needed to also clear the locally stored config out of dynamicConfig.ts

www/__mocks__/cordovaMocks.ts

www/__tests__/clientStats.test.ts

JGreenlee · 2023-11-19T05:53:07Z

www/js/config/dynamicConfig.ts

+//used test multiple configs, not used outside of test
+export const resetStoredConfig = function () {
+  storedConfig = null;
+};


It's kind of a workaround, but ok for now

nit: you may want to prefix with _test_ or something similar to highlight that it is an internal function to be used for testing only

www/js/types/appConfigTypes.ts

@JGreenlee

from @JGreenlee's suggestion in review Co-authored-by: Jack Greenlee <[email protected]>

not all (in fact many) of the tests don't need this config at all, so the parameter should be optional to clean things up Co-authored-by: Jack Greenlee <[email protected]>

follow-on to making the config an optional parameter to mockBEMUserCache Instead of passing it in EVERY TIME, it is now a fallback, and the config only needs to be specified and re-specified in the enketoHelper tests added fallback and removed specification from tests that didn't need it

-add types for the parameters of resolveTimestamps -update doc of this function -cast 'timelineEntry' to trip or place where needed -add a few fields that were missing from type defs of ConfirmedPlace and EnketoSurveyConfig

JGreenlee

LGTM!

if the survey doesn't use start and end times, timestamps will be null. This is fine and we can use optional chaining to prevent throwing an error here

…s/e-mission-phone into expand-enketo-helper

codecov · 2023-12-03T20:26:28Z

Codecov Report

Merging #1063 (b900d1e) into service_rewrite_2023 (7121ec6) will increase coverage by 3.77%.
Report is 1 commits behind head on service_rewrite_2023.
The diff coverage is 74.68%.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           service_rewrite_2023    #1063      +/-   ##
========================================================
+ Coverage                 52.67%   56.44%   +3.77%     
========================================================
  Files                        21       22       +1     
  Lines                      1198     1311     +113     
  Branches                    266      296      +30     
========================================================
+ Hits                        631      740     +109     
- Misses                      567      571       +4

Flag	Coverage Δ
unit	`56.44% <74.68%> (+3.77%)`	⬆️

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

Files	Coverage Δ
www/js/config/dynamicConfig.ts	`18.32% <100.00%> (+9.79%)`	⬆️
www/js/diary/timelineHelper.ts	`7.31% <0.00%> (-0.10%)`	⬇️
www/js/survey/enketo/enketoHelper.ts	`71.81% <76.00%> (ø)`

... and 2 files with indirect coverage changes

shankari · 2023-12-03T20:52:00Z

Oddly, "end trip force sync" seems to work, whereas just "force sync" doesn't seem to work. The only difference between the two should be that "end trip force sync" briefly starts and ends the trip before calling forceSync.

This is a long story, probably best summarized in e-mission/e-mission-docs#640
tl;dr: We only push locally cached data to the server after a trip is complete. This allows us to run the local trip detection algorithm without worrying about missing data that has been pushed up to the server. That is why you need to end the trip before force syncing to see the modified survey on the server.

Since this PR is open now and it deals with Enketo, it seems like a good opportunity to add support for client-side XML -> JSON transforms (which we've been wanting to do for awhile now!)

I noticed this in the dependencies. Exciting!
It will make it so much easier to not have to have Jack run the transformation on his personal windows laptop 😄

@Abby-Wheelis in the next round of intake form improvements, you may be able to allow participants to specify the XML file directly, either in the form (not sure if upload is supported), or by adding to the issue in a new comment or by adding to the PR directly. That should cut down significantly on the amount of busy work you have to do!

shankari

I am going to merge this now so I can get a release out to staging, but please fix the issues outlined here in a future fix:

comments from me
codecoverage results (thanks to Jiji!!) about the lines that are not covered with the current tests.

shankari · 2023-12-03T21:04:48Z

www/js/config/dynamicConfig.ts

+//used test multiple configs, not used outside of test
+export const resetStoredConfig = function () {
+  storedConfig = null;
+};


nit: you may want to prefix with _test_ or something similar to highlight that it is an internal function to be used for testing only

www/js/survey/enketo/enketoHelper.ts

shankari · 2023-12-04T03:26:56Z

www/js/survey/enketo/enketoHelper.ts

  );
 }
+
+export async function fetchSurvey(url: string) {
+  const responseText = await fetchUrlCached(url);


is this the right thing to do? Is it possible for the cache to be out of date - for example, right after the user has edited the survey.

This function fetches the survey, not the response (ie this is what is handling the xml/json differences) that @JGreenlee added. Our previous response function [loadPreviousResponseForSurvey] does re-fetch every time.

www/js/survey/enketo/enketoHelper.ts

shankari · 2023-12-04T03:50:34Z

www/__tests__/enketoHelper.test.ts

+  expect(getInstanceStr(null, opts)).toBe(null);
+  //if there is a prefilled survey, return it
+  expect(getInstanceStr(xmlModel, opts)).toBe(filled);


shouldn't you also check with an xmlResponse that does not have the start/end datetime?

So, one that is already filled, but does not have the timestamps filled?

I'm a little confused what you mean, but maybe as I comb through the codecov report I can figure out where the gap you're referring to is.

e-mission#1063 (comment)

Abby Wheelis added 4 commits October 11, 2023 14:50

Merge remote-tracking branch 'JGreenlee/rewrite-services-sept2023' in…

5d6fb5a

…to expand-enketo-helper

draft enketoHelper tests

0815334

convert answer.js into enketoHelper

43b8386

moving the methods form answer.js into enketoHelper as a part of the services migration

exclude platforms

9840b5a

When testing, I was getting an error from Jest about duplicate modules, one of which was in platforms. This change resolves that error

Abby Wheelis added 2 commits October 11, 2023 15:58

add more tests

1eaf8b8

additional test for filterByNameAndVersion fake answers have been constructed to be filtered

remove answer.js

74b8471

completely remove answer.js and all references to it, replace references with references to enketoHelper.ts

adding tests for resolveTimestamps

d259799

resolveTimestamps is a helper function to saveResponse, but still contains a fair amount of its own logic. Testing the edge cases for this function ensures that it will behave as expected within the larger context

testing for _lazyLoadConfig

11a3df6

adding a basic test for _loadLazyConfig, in order to ensure that my mock setup for that works, before moving into testing functions that depend on it

attempting to add more tests

bc42845

currently struggling with i18next and MessageFormat, as I can't get either of those mocked and working

update messageformat plugin

608d97d

the message format plugin moved! https://github.com/messageformat/messageformat/tree/main/packages/core

Abby Wheelis added 9 commits October 13, 2023 10:12

start to configure i18next for tests

8b901f6

setting __DEV__ to false in globals, so that it can be used throught the testing suit calling the i18n setup in the tests should work once we incorporate the React testing changes

update the message format import

cea96dd

updates to testing

b7f6d68

updates to tests

75a0371

remove old i18n code, update types, comment out broken tests

update types

09f21bc

from log statements, these answers have data and metadata, no labels

merge changes from e-mission#1049

496eeb5

ensured that tests still pass

don't mock i18n, use the real thing

e7fe7a8

with the changes from e-mission#1049, we are now able to test using i18n, no need to mock!

set up baseline testing for resolveLabels

70f98cd

introduced i18next for the tests updated config in mock, and test of loading config to be accurate to what is expected (missing some '{' ) adjust formatting of function indentation

rework getInstanceStr tests

719403a

now that I understand how this function works, I got the xml (filled and unfilled) directly from console.log statements. The tests are now accurate, and cover each of the cases.

Abby Wheelis added 2 commits October 13, 2023 16:25

tests for saveResponse

252e42f

testing for saving the response, both when it works and when the timestamps are invalid, resulting in an error

take out old console.log

9dc9384

Abby-Wheelis closed this Nov 13, 2023

Abby-Wheelis reopened this Nov 13, 2023

JGreenlee requested changes Nov 17, 2023

View reviewed changes

Abby Wheelis added 7 commits November 17, 2023 16:45

add a test case

8f7c674

we also want to test that the timestamps follow the "minute" accuracy convention we implemented, so that if the survey and timelineEntry match within the minute, we use the timelineEntry timestamps, else use the timestamps from the survey e-mission#1063 (comment)

note the issue

7ec575e

this form is invalide because of the start and end times mismatching

check for key before resolving with this information

8b3ab76

remove _lazyLoadConfig, move types

d123b4f

the function was not all that necessary, so I removed it Also moved my survey config type into the appropriate place e-mission#1063 (comment)

test the version

100eefb

adding a response that should get filtered out because the version is too low e-mission#1063 (comment)

make config a parameter to mockBEMUserCache

22decbf

more test cases!!

80aa5ea

Because of the new parameter, I was able to add more test cases to resolve Labels I needed to also clear the locally stored config out of dynamicConfig.ts

JGreenlee requested changes Nov 19, 2023

View reviewed changes

Abby-Wheelis and others added 5 commits November 19, 2023 08:33

add survey_info to the AppConfig Type

0ef1db8

from @JGreenlee's suggestion in review Co-authored-by: Jack Greenlee <[email protected]>

make config optional param

10b9546

not all (in fact many) of the tests don't need this config at all, so the parameter should be optional to clean things up Co-authored-by: Jack Greenlee <[email protected]>

prettier types

83a773e

enketoHelper: expand typings for resolveTimestamps

becc4af

-add types for the parameters of resolveTimestamps -update doc of this function -cast 'timelineEntry' to trip or place where needed -add a few fields that were missing from type defs of ConfirmedPlace and EnketoSurveyConfig

JGreenlee approved these changes Nov 20, 2023

View reviewed changes

JGreenlee and others added 3 commits November 21, 2023 11:55

enketoHelper: fix error if timestamps are null

828ee12

if the survey doesn't use start and end times, timestamps will be null. This is fine and we can use optional chaining to prevent throwing an error here

Merge branch 'expand-enketo-helper' of https://github.com/Abby-Wheeli…

e414c5c

…s/e-mission-phone into expand-enketo-helper

Merge branch 'service_rewrite_2023' into expand-enketo-helper

aee8738

Abby-Wheelis mentioned this pull request Dec 1, 2023

‼️📅 Rewrite - notifScheduler.ts #1092

Merged

Merge branch 'service_rewrite_2023' into expand-enketo-helper

b900d1e

shankari mentioned this pull request Dec 3, 2023

Add codecov report on Github Action #1108

Merged

shankari approved these changes Dec 4, 2023

View reviewed changes

shankari merged commit 9a321b7 into e-mission:service_rewrite_2023 Dec 4, 2023
4 checks passed

Abby-Wheelis pushed a commit to Abby-Wheelis/e-mission-phone that referenced this pull request Dec 4, 2023

restore line

d083d3e

e-mission#1063 (comment)

Abby-Wheelis mentioned this pull request Dec 4, 2023

🧹 Enketo cleanup #1113

Open

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

Rewrite answer.js into enketoHelper.ts #1063

Rewrite answer.js into enketoHelper.ts #1063

Abby-Wheelis commented Oct 11, 2023

Abby-Wheelis commented Oct 11, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

JGreenlee commented Oct 13, 2023 •

edited

Loading

Abby-Wheelis commented Oct 13, 2023 •

edited

Loading

Abby-Wheelis commented Oct 13, 2023

JGreenlee Nov 17, 2023

Abby-Wheelis Nov 17, 2023

JGreenlee Nov 17, 2023

Abby-Wheelis Nov 18, 2023

JGreenlee Nov 19, 2023

shankari Dec 3, 2023

JGreenlee left a comment

codecov bot commented Dec 3, 2023

shankari commented Dec 3, 2023

shankari left a comment

shankari Dec 3, 2023

shankari Dec 4, 2023

Abby-Wheelis Dec 5, 2023

shankari Dec 4, 2023

Abby-Wheelis Dec 4, 2023

		global.URL = require('url').URL;
		global.Blob = require('node:buffer').Blob;

Rewrite answer.js into enketoHelper.ts #1063

Rewrite answer.js into enketoHelper.ts #1063

Conversation

Abby-Wheelis commented Oct 11, 2023

Abby-Wheelis commented Oct 11, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

Abby-Wheelis commented Oct 12, 2023

JGreenlee commented Oct 13, 2023 • edited Loading

Abby-Wheelis commented Oct 13, 2023 • edited Loading

Abby-Wheelis commented Oct 13, 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

JGreenlee left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 3, 2023

Codecov Report

shankari commented Dec 3, 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

JGreenlee commented Oct 13, 2023 •

edited

Loading

Abby-Wheelis commented Oct 13, 2023 •

edited

Loading