🎀 Final change of the service rewrite #1126
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
in testing the initCustomDatasets function, I was calling it and then setting a timeout to wait for it to run, now I await getting the config, then await initialization double checked the Jest tests and running in the emulator - still going smoothly!
-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
There is no right or wrong here, but it's a bit cleaner and more consistent with the rest of the codebase if we generally follow this: i) for short one-liners, use const and declare an arrow function ii) for longer functions, use the traditional 'function' declaration also replaced some uses of 'var' with 'const' or 'let' added AppConfig type to initCustomDatasetHelper and replaced a console statement with logDebug
- NPM package for GeoJSON types
-In useAppConfig.ts, we can't rely on $ionicPlatform anymore since we won't be using Angular moving forward. So we need an alternate way to ensure Cordova plugins are ready before we try to do stuff with them. I found that Cordova fires the 'deviceready' event once plugins are loaded - so we can just listen for that, wrap it in a promise, and use it in the place of $ionicPlatform.ready(...) -There were a few uses where getAngularService() was still used to access the old Logger, which were easily substituted to instead use the functions exported from logger.ts (logDebug, logWarn, etc) -There were several unused imports left over in files where getAngularService was formerly used and is no longer
I also had to adjust the error coming from notifScheduler because it wasn't helping much with the way it was defined before
The startup logic will now happen not in an Angular context, but with an event listener for 'deviceready' (which is what Cordova uses) And the React initialization will happen from there.
- remove unused imports - use logDebug, logInfo, logWarn, format nicely - replace ionic.Platform.isIOS / isAndroid with cordova.platformId
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
Typescript wants us to be more explicit about what could be null/undefined and what couldn't be. transitionTrip2TripObj should return a Promise that could resolve as an UnprocessedTrip or undefined. (If undefined, it gets filtered later in readUnprocessedTrips) We also need to address that Luxon's DateTime.toISO has string or null as the return type – null is for if the input was invalid. It never should be, but if it was for some reason we can have an error dialog and assert type with an empty string.
mockUnprocessedTrip was unnecessary and not used anywhere mockFilterLocation didn't need all these fake properties and instead could just include the few that are needed and then declare it 'as FilteredLocation'
enketoHelper.test.ts had a lot of the wrong types in the fake responses. They were all strings but I've since updated them so there some strings, some numbers, objects. EnketoResponseData in enketoHelper is correct. - some other type casting (with 'any') in both of these tests to ensure fake inputs satisfy parameters types
this test still had LabelOptions type for the userInput values instead of the UserInputEntry type. It didn't cause the tests to fail because inferFinalLabels and verifiabilityForTrip only check that the values exist - they don't care what properties they have. Anyhow, the types line up now.
-- Part of the fix for e-mission/e-mission-docs#1034 When a user input is recorded, we put it in storage. For snappy performance we also cache it in the timelineLabelMap / timelineNotesMap state. However, if we switch filters or load more trips, this state gets overridden. So, we need to call updateLocalUnprocessedInputs at some point. It is an async function but we can call it without 'await' and just allow it to execute in the background.
-- Part of the fix for e-mission/e-mission-docs#1034 When the yellow checkmark is used to verify a pair of inferred mode+purpose labels, both labels should be verified by a single click. The labels get stored fine; however, the way we update state after doing so prevents both labels from being updated simultaneously and only one of them gets filled in. The resulting UX is that it takes 2 clicks and this is not desired. We could: update state in succession, one after the other (await label A, then proceed label B), OR we could rework these functions to support adding labels for a particular trip as a batch (so the same function call could store mode+purpose at the same time). This was decided as a better and more performant option. It uses Promise.all to store all labels from the batch, then it updates the state with all changes from that batch. When a single label is recorded, it is a batch of 1 - when verifying inferences, it may be a batch of multiple labels.
When selecting date, the following behavior occurs: - If 2+ days, behave as normal (Range of Day1 -> Day2) - If "open Ended" (i.e., only start is picked), fetch a range of days between that day and present (Day1 -> Today) - If 1 day, fetch range for 48 hours from that day TODO: The 1 day range going to 48 hours is meant as a patch, so that this WSOD fix can go to staging. To make a single day only fetch 24 hrs, some changes to the server may be needed.
Instead of mutating an extra property 'displayDt' onto the entries, let's use a get function that returns the displayDt. This should fix issue (4) in e-mission/e-mission-docs#1034. And let's also memoize that function so performance is not impacted
If something was thrown, it could display 'undefined' if 'err' was an error message and not the error object. Using the displayError function is a more proper way to guarantee we see any error messages
- 'key' could be kept in 'data' or 'metadata' depending on if it is a processed or unprocessed entry (this is an inconsistency we may want to revisit later) - handle possible null/ undefined parameters in functions: check for truthy inputs before executing
…-mission-phone into migration-wrapup
…sion-phone into migration-wrapup
This test failed due to a bad merge conflict resolution. This commit reverts notifScheduler.test.ts to the state of 3cbb5f0 after sebastianbarry's fix. The test now passes.
- ReminderSchemeConfig should be ReminderSchemesConfig - a problem that resulted from resolving merge conflicts - Defining types in notifScheduler.ts so that the functions have return types and Typescript type warnings go away
-- With 'strictNullChecks' enabled in Typescript, we get type errors when declaring objects of a type with some of its properties missing.
But for the purpose of testing (ie fake data, potentially invalid inputs) we want to do just that. We can get around this by hard casting with `as unknown as <T>`.
In tests, for blank/invalid inputs being used (such as a blank CompositeTrip object) we can use `{} as any`
This fixes a bunch of type warnings that show up with strictNullChecks being true. Fixes include adding types, disambiguating null and undefined types and handling them more carefully, and disambiguating trip vs place with type guards
With this change, any remaining uses of the `moment` library from datetime operations have been fully replaced by Luxon. All references to moment are removed.
When using AngularJS, jQuery needed to be provided as a global by Webpack. Now, we do not need it at all (except Enketo which still uses jQuery internally, but provides it to itself and is not needed globally.)
🎀 UI Migration Wrap-up
Processed responses for user input surveys were not being matched because they were being looked up under "manual/trip_user_input", while it should have just been "trip_user_input". This is because we were only invoking a function to trim off "manual/" for MULTILABEL user input matching, and it was neglected for ENKETO. Revise the functions in confirmHelper (keeping 2 versions, `removeManualPrefix` which just trims the key, and `inputType2retKey` which looks up the key and then trims), and use `removeManualPrefix` in inputMatcher appropriately Some day we can unify the data model (e-mission/e-mission-docs#1045) and not keep 2 versions of so many things
Adds basic unit tests for mapInputsToTimelineEntries under both MULTILABEL and ENKETO configurations. It considers fake user inputs that are processed, and those that are unprocessed, and ensures that the returned map includes both.
🚑 Fix Processed Trip Survey Responses Not Matching
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1126 +/- ##
=========================================
Coverage ? 77.55%
=========================================
Files ? 28
Lines ? 1702
Branches ? 367
=========================================
Hits ? 1320
Misses ? 382
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Releases have been submitted to both android and iOS stores for review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Includes lots and lots of rewrites including the following:
setup_ios_native.shto support Apple Silicone #1116survey_infobackwards compat + add tests for dynamicConfig #1115