Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎀 UI Migration Wrap-up #1106

Merged
merged 137 commits into from
Feb 1, 2024

Conversation

JGreenlee
Copy link
Collaborator

This PR includes all the remaining rewrite PRs and also adds on the "cleanup tasks" listed in e-mission/e-mission-docs#977

-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
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.
Since these have both been simplified we can merge them together
It was failing because 'commHelper' is no longer directly in '/js' -- it is in 'js/services'
This commit enables strictNullChecks for greater type safety and then fixes the dozens of instances where TS now warns us about values that are potentially null https://www.typescriptlang.org/tsconfig#strictNullChecks

This means adding types on a lot of places where we had not added types before. Typescript is more powerful when it's on a stricter configuration, but this means we have to be more explicit.

In a few of the files, I also cleaned up formatting and modernized syntax while I was making changes there.
This file had a lot of outdated JS syntax and console.log statements. I just did a full sweep of this file and 'modernized' it.
Added some type definitions too.
I previously used the wrong property name here. Now that we have Typescript on a stricter configuration, I found my mistake
We previously had maintainers for the translations in these languages, but they have not been updated in a very long time. Because the app has changed so much since then, the old translations are almost entirely obsolete.
At this point, I think it is better to remove them from usage. If we ever get someone to update the translations we can add them back.
We'll still keep the old translations up on the e-mission-translate repo although they won't be used here.
VSCode was only picking up on our declaration TS files if they were open in the editor. This signals to VSCode and TS to check js/types for .d.ts files.
JGreenlee and others added 13 commits January 25, 2024 01:49
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.
In 4f173ba I bumped the prettier version to 3.1. Whatever version we are using for the project should be the same as the workflow runs with (as we established in e-mission#1117)
-- 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
@JGreenlee
Copy link
Collaborator Author

After a rebase, there are no merge conflicts

@JGreenlee
Copy link
Collaborator Author

I will fix the failing test tomorrow. I probably made a mistake during the rebase, as it was a long and messy process

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
@JGreenlee
Copy link
Collaborator Author

Fixed the test. There is still a lot of cleanup left to do with the merge conflicts and fixing type warnings.
(since this branch is on a stricter Typescript setting than the incoming commits)

-- 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.)
@shankari
Copy link
Contributor

@JGreenlee I am not sure if this is fully done, but I am going to review and merge this tonight since the tests are passing. You can address any review comments and further cleanup in a separate PR.

@shankari
Copy link
Contributor

Update: I have currently reviewed the 100 most straightforward files. Will do the remaining 23, including the enketo and labeltab files, where the changes are not mere boilerplate, tomorrow.

Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of comments, but most of them are small or requests for clarification.
So I am finally merging this (fittingly, on a #22 VTA bus, using the VTA Free WiFi) so that we can be done with the rewrite, and fix these comments in a cleanup PR that is completely in react!!

"humanize-duration": "3.31.0",
"i18next": "^22.5.0",
"humanize-duration": "^3.31.0",
"i18next": "^23.7.6",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: do we actually have any tests for i18n? In general, we rely upon the automated tests to catch regressions during upgrades, so it would be good to ensure that it isn't broken. (we can check this visually/manually) for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we don't specifically test the i18nextInit.ts file, some of the tests do invoke it because those tests depend on i18n

So there is coverage for it downstream

Comment on lines -191 to -195
altitude: 100,
elapsedRealtimeNanos: 10000,
filter: 'time',
fmt_time: '',
heading: 1.0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you remove these? Is it because they are not used in the tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, those properties are not used by the test. If we declare like as FilteredLocation, we don't have to list every single property.
I thought this would be a better strategy for unit tests, where we're quite often testing with 'stub' objects, or declaring invalid inputs.

Comment on lines -47 to +49
CommHelper.fetchUrlCached = jest
.fn()
.mockImplementation(() => JSON.stringify(fakeDefaultLabelOptions));
jest.mock('../js/services/commHelper', () => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the reason for this change? The commit message says that it is due to strictNullChecks, but it is not clear where the null checks are here.

www/js/angular-react-helper.tsx Show resolved Hide resolved
color={colors.onBackground}
iconColor={colors.onBackground}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this change do? It is part of the null checks commit but doesn't seem to be related. Does it change the visual appearance of the icons in any way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the wrong property name, used to be called color but renamed in RN Paper 5.x. Typescript hinted to this.

I should have explained this or put it in a different commit

www/js/control/ProfileSettings.tsx Show resolved Hide resolved
www/js/splash/pushNotifySettings.ts Show resolved Hide resolved
www/js/metrics/metricsHelper.ts Show resolved Hide resolved
dtStartDate.toString() === DateTime.fromJSDate(endDate).startOf('day').toString()
) {
// For when only one day is selected
// NOTE: As written, this technically timestamp will technically fetch _two_ days.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this note now be removed? when we select only one day, the endDate is the end of the startDate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@the-bay-kay can likely provide better insight than I can

Copy link
Contributor

@the-bay-kay the-bay-kay Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing! This comment was a result of the work we did to fix the White-Screen of Death showstopper on the Metrics tab. All the notes are in this PR, if you want to read more!

As written, selecting a single day (endDate being the end of the startDate), actually fetches two days worth of metrics from the server. When we query for any shorter of a range (endDate < the end of the startDate), no days are fetched, and the metrics tab returns empty. (Put another way: if you select 1/17/24, the metrics display would be identical to those displayed in the range 1/17/24-1/18/24)/

I spent a fair bit of time digging through the server, tracing the call all the way back to the DB Query, and couldn't figure out why the query resulted in two days. Furthermore, "open-ended date selection" seems to be an issue with the date-picker plugin we use; on the JS Playground example they give, I noticed that choosing open ended ranges resulted in undefined behavior. I think there's future work to be done (a) sending a PR to the date picker, to change the open-ended behavior, and (b) debugging the server queries to figure out why single-day metrics aren't available.

The actual WSOD was an issue with this query; because the end date was undefined, errors were thrown and the page would crash. After fixing this, we had a few options:

  • Make single day queries appear blank
  • Make single day queries show the "incorrect" 48-hour period (Which, when selecting the present day, is actually correct!)
  • Detect single day selections,and throw a pop-up warning prompting the user to re-select a range of 2+ days.
    • Unfortunately, the Date-Picker plugin does not support this natively. Ideally, we'd stop the user from being able to select a single day, but that'd require a pretty hefty rework of the plugin).

Because our priority was getting this done and dusted for staging, Jack and I agreed the best option was to display the "two-day" period, while documenting the behavior. This felt like the most user-friendly solution; since the metrics tab already shows ballpark values, we figured having it not be exactly the day was sufficient for now. I'd like to wrap back around and fix this once we have time, but the priority was fixing the WSOD error!

Comment on lines -58 to +59
textList.push({ label: label, value: Math.round(userPrevWeek.low) });
textList.push({ label: label, value: `${Math.round(userPrevWeek.low)}` });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that this is part of "cleanup formatting". But can you clarify why formatting this as text right here is better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's because Math.round returns a number and we want a string. So this was just a quick way to cast to a string.
We could alternatively do "" + x or String(x) or x.toString()

@shankari shankari merged commit d8379c2 into e-mission:service_rewrite_2023 Feb 1, 2024
5 checks passed
JGreenlee added a commit to JGreenlee/e-mission-phone that referenced this pull request Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Address code reviews with "future"
Development

Successfully merging this pull request may close these issues.

4 participants