✏️📖 Diary Services Rewrite #1085

the-bay-kay · 2023-10-26T17:46:39Z

Before rewriting www/js/diary/services.js, I took some time to update diaryHelper.ts so that it no longer requires the legacy moment.js library.

The plan, as with the other rewrites, is to convert the Angular services into typescript. These functions will be moved from services.js into timelineHelper.ts.

diaryHelper now only uses luxon, and does not rely on the momentjs library.

…-mission-phone into diaryServices-rewrite Keeping code up to date, enable Prettier

- moved functions to timelineHelper - updated momentJS code to luxon

the-bay-kay · 2023-11-03T22:53:23Z

Update: All of the service rewrites seem to be working correctly, I'm now working on writing tests!

The current issue I've run into is that timelineHelper still rely on the $ionicLoading service. After doing some searching, it seems that Angular's TestBed is compatible with Jest, and can be used to mock services. For the time being, I'll continue to work on this testing framework!

That being said, should I be focusing on replacing the $ionicLoading service instead? @JGreenlee , I'd appreciate your two cents! I know the goal of the rewrites was to move away from angular, so working on mocking it seems counterproductive.

Edit: Theoretically, we should be able to use the Ionic Loader without Angular (docs): with some quick tests, this doesn't seem to be working. Is this because the version we use with angular is Ionic v1.3.3? I'm mostly thinking out loud in this edit, I'll keep looking into using the loader in react!

JGreenlee · 2023-11-04T03:10:48Z

Yes! Replace if possible! We want to be done with Angular and move on to new things

(for the record we are using Angular 1.6.7 I believe, which is not what Ionic ships with but it was necessary to upgrade to be able to interop with React)

JGreenlee · 2023-11-04T03:22:30Z

I would suggest handling the "loading" popup in the thing that calls timelineHelper (which I believe is just the LabelTab component).

timelineHelper is just plain Typescript and its basic purpose is just to reformat data into something the React components can use. It doesn't know anything about what's in the React DOM, so I think it's ok to decouple it from the user-facing loading popup.

At least for now until we (potentially) implement a 'global' popup mechanism

- Behavior remains unchanged when removing this component - labelTab and other parent components handle loading, so the ionic service is not needed.

- Added test for each diaryService rewrite

Updated for changes

www/js/diary/timelineHelper.ts

the-bay-kay · 2023-11-09T19:05:08Z

Tests and mocks are done - currently brainstorming more effective ways to test beyond expect().resolves.not.toThrow(). Half of this file is test data - it seems cumbersome to hardcode in another 100+ lines of data to check with .toEqual(), but this also seems the most foolproof / futureproof way of testing. Will try this approach first, just to set up some robust tests before moving forward.

www/js/diary/timelineHelper.ts

- borrowed the windowAlert mocks from PR e-mission#1093, to allow error checking - added mockCheck, updated some tests to use to `toEqual()`

the-bay-kay · 2023-11-09T23:05:40Z

I had two questions about this PR's current unit tests:

Currently, one of the tests timelineHelper.test.ts relies on a pretty large readAllCheckOne object, defined in the mock. If I were to do this for the other 3 unit tests that require checks, I would need to add 200+ lines to the mock. Is there a less bulky way to go about these checks? I'm not sure how to generate the "check object" more efficiently, without effectively re-writing the method that's being tested. I could use jest.spyOn() to ensure each helper function is called, though that doesn't confirm the function is returning correct data.
Currently, I've only written tests for the functions moved from /www/diary/services.js. While I'm already editing it, should I go ahead and design tests for the remaining functions in timelineHelper.ts?

- wrap the tests related to unprocessed inputs in a describe(...) - on the other mocks in this file, fallback to the original implementation - ensure both updateAllUnprocessedInputs and updateLocalUnprocessedInputs are used - tidy up the mock configs

codecov · 2023-12-06T18:24:26Z

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Comparison is base (6e3af4f) 58.83% compared to head (f54257b) 73.95%.
Report is 59 commits behind head on service_rewrite_2023.

Additional details and impacted files

@@                    Coverage Diff                    @@
##           service_rewrite_2023    #1085       +/-   ##
=========================================================
+ Coverage                 58.83%   73.95%   +15.12%     
=========================================================
  Files                        26       27        +1     
  Lines                      1421     1701      +280     
  Branches                    320      356       +36     
=========================================================
+ Hits                        836     1258      +422     
+ Misses                      585      443      -142

Flag	Coverage Δ
unit	`73.95% <92.15%> (+15.12%)`	⬆️

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

Files	Coverage Δ
www/js/services/commHelper.ts	`25.00% <100.00%> (+1.27%)`	⬆️
www/js/services/unifiedDataLoader.ts	`87.87% <100.00%> (ø)`
www/js/splash/notifScheduler.ts	`100.00% <100.00%> (ø)`
www/js/survey/enketo/enketoHelper.ts	`73.39% <100.00%> (+1.57%)`	⬆️
www/js/survey/multilabel/confirmHelper.ts	`94.94% <ø> (ø)`
www/js/diary/diaryHelper.ts	`73.46% <58.33%> (+0.55%)`	⬆️
www/js/config/dynamicConfig.ts	`84.67% <86.66%> (+66.35%)`	⬆️
www/js/diary/timelineHelper.ts	`88.93% <89.30%> (+81.62%)`	⬆️

... and 1 file with indirect coverage changes

This was caused by missing parentheses - instead of passing the result of toISO(), we were passing the toISO function itself!

shankari · 2024-01-20T03:02:45Z

@the-bay-kay this now has a merge conflict, can you please resolve?

shankari · 2024-01-20T22:44:51Z

I just merged the changes, but ended up losing this change since it was overridden by the switch to luxon.
@Abby-Wheelis @the-bay-kay can you confirm that the new luxon code works properly here
6e3af4f

shankari

Almost there, just some high-level questions and cleanup

www/js/types/diaryTypes.ts

www/js/survey/enketo/enketoHelper.ts

www/__mocks__/timelineHelperMocks.ts

www/js/diary/timelineHelper.ts

www/__tests__/timelineHelper.test.ts

shankari · 2024-01-21T03:39:09Z

Given that the tests are failing, here's my proposed plan of action for finishing the merges:

@sebastianbarry fixes the notifScheduler tests
@the-bay-kay then pulls that change, addresses review comments, fixes prettier and updates
I review the hopefully minor changes and merge
@JGreenlee pulls these changes into the UI wrapup change and resolves merge conflicts
- Hopefully, the remaining cleanup changes are fairly minor
I review and merge the UI wrapup change

sebastianbarry · 2024-01-22T15:35:22Z

@shankari I am not sure how to proceed with fixing my bug; the jest test passes on my local machine, and it passed the github action tests while it was just my PR #1092, but it fails in a strange unexpected way on this PR https://github.com/e-mission/e-mission-phone/actions/runs/7597279222/job/20691918950?pr=1085

It appears that the function to schedule the notifications works as expected, scheduling the 4 correct notifications. The issue is that after, I'm checking to see if the scheduledNotifs actually got scheduled by calling getScheduledNotifs. In here, the async behavior in getScheduledNotifs believes that the notifications are still in the process of being scheduled (which does not reproduce on my local machine), and then when it finishes scheduling, the notifications are not scheduled (however they are at this point on my local machine).

Do you have any ideas for how I can fix this? Or perhaps test it in the github actions codecov environment so I at least have somewhere to go off of? At this point I'm not sure what the problem is and I don't know where to go from here.


Image of it working on my local machine

shankari · 2024-01-22T17:20:53Z

@sebastianbarry have you pulled from service_rewrite_2023 after your PR was merged? I merged several other PRs at the same time, and one of them might have affected your tests.

I would:

pull and test locally
if it passes, test in the GH environment by creating a PR that you push additional logs to and wait for the run to complete. (we can squash merge this so it won't mess up the commit history)

There is no way to run GH actions locally.

jiji14 · 2024-01-22T18:13:05Z

@sebastianbarry have you pulled from service_rewrite_2023 after your PR was merged? I merged several other PRs at the same time, and one of them might have affected your tests.

I would:

pull and test locally

if it passes, test in the GH environment by creating a PR that you push additional logs to and wait for the run to complete. (we can squash merge this so it won't mess up the commit history)

There is no way to run GH actions locally.

@sebastianbarry

Also check if there is any difference between local environment and GH environment. For example, different version might cause the issue. It happened to me when I worked with GA so wanted to let you know!

shankari · 2024-01-22T19:05:46Z

Also check if there is any difference between local environment and GH environment. For example, different version might cause the issue. It happened to me when I worked with GA so wanted to let you know!

Was this "mismatched version on GA" on OpenPATH? This should not happen with OpenPATH because we have locked down our dependencies. When you run setup and activate, you use the version of the tools that is checked in to OpenPATH, and since we call setup and activate on GH (and we should in case we don't), it should run with the same environment.

The reason for locking down the dependencies was exactly this - we wanted to have a consistent build and test process, so that a test failure was meaningful

jiji14 · 2024-01-22T20:21:56Z

Also check if there is any difference between local environment and GH environment. For example, different version might cause the issue. It happened to me when I worked with GA so wanted to let you know!

Was this "mismatched version on GA" on OpenPATH? This should not happen with OpenPATH because we have locked down our dependencies. When you run setup and activate, you use the version of the tools that is checked in to OpenPATH, and since we call setup and activate on GH (and we should in case we don't), it should run with the same environment.

The reason for locking down the dependencies was exactly this - we wanted to have a consistent build and test process, so that a test failure was meaningful

It happened when setting up prettier, but it is fixed now!

sebastianbarry · 2024-01-22T23:55:52Z

The updates are in my pull request #1123

- Expanded types for several function parameters - Changed exported functions of `() => {}` format to `function () {}`

This better reflects the origin of this type: both BEMServerComm and BEMUserCache return data of this structure.

Test now compares the unpacked object map to original object.

Was running on `js/`, but not mocks; fixed formatting in mocks and tests!

shankari · 2024-01-25T03:36:29Z

@the-bay-kay I am merging this now to unblock @JGreenlee but it would be good if you could resolve the other comments as well when you have a chance.

the-bay-kay added 11 commits October 26, 2023 10:34

Updated diaryHelper.ts, now uses luxon

6c23b2d

diaryHelper now only uses luxon, and does not rely on the momentjs library.

Removed moment from timelineHelper

062828e

Updated readAllCompositeTrips function

5ade8b7

Merge branch 'unifiedDataLoader-rewrite' into diaryServices-rewrite

dd05c89

Merge branch 'service_rewrite_2023' of https://github.com/e-mission/e…

545d407

…-mission-phone into diaryServices-rewrite Keeping code up to date, enable Prettier

Rewrote readUnprocessedTrips, helper functions

7a4b6fb

- moved functions to timelineHelper - updated momentJS code to luxon

Fully removed angular service

b5736a1

Merge branch 'type_interfaces' into diaryServices-rewrite

dd238e6

Merge branch 'service_rewrite_2023' into diaryServices-rewrite

8e2022f

Fixed error in prettier merge

7c7f4d8

Ran prettier on remaining files

356dfcf

the-bay-kay added 7 commits November 6, 2023 15:46

Removed $ionicLoading

6dcf1ab

- Behavior remains unchanged when removing this component - labelTab and other parent components handle loading, so the ionic service is not needed.

Merge branch 'type_interfaces' into diaryServices-rewrite

651876b

Added basic timelineHelper tests

2bcdac8

- Added test for each diaryService rewrite

Updated issue with index.html

ae5ae95

Merge branch 'type_interfaces' into diaryServices-rewrite

1326940

Updated for changes

Improved mocks, added unit tests

efa96e0

Updated mocks, added tests

13206e9

JGreenlee reviewed Nov 9, 2023

View reviewed changes

www/js/diary/timelineHelper.ts Outdated Show resolved Hide resolved

JGreenlee reviewed Nov 9, 2023

View reviewed changes

www/js/diary/timelineHelper.ts Outdated Show resolved Hide resolved

the-bay-kay added 5 commits November 9, 2023 11:40

Moved mockData to separate file for legibility

f031d33

Deepcopy wasn't necessary, removed Parse/stringify

2e1a402

Added clarifying comment, adjusted DateTime format

adab677

Expanded mocks, sured up tests

2d0d321

- borrowed the windowAlert mocks from PR e-mission#1093, to allow error checking - added mockCheck, updated some tests to use to `toEqual()`

Minor adjustment to tests

bf91301

jiji14 mentioned this pull request Dec 5, 2023

Run Codecov Workflow only when PR is ready #1114

Merged

fix up timelineHelper tests

4d664ad

- wrap the tests related to unprocessed inputs in a describe(...) - on the other mocks in this file, fallback to the original implementation - ensure both updateAllUnprocessedInputs and updateLocalUnprocessedInputs are used - tidy up the mock configs

fix broken start time on draft trips

aa8cbe4

This was caused by missing parentheses - instead of passing the result of toISO(), we were passing the toISO function itself!

JGreenlee approved these changes Dec 6, 2023

View reviewed changes

JGreenlee mentioned this pull request Dec 7, 2023

🎀 UI Migration Wrap-up #1106

Merged

Merge branch 'service_rewrite_2023' into diaryServices-rewrite

dfbee1f

Fix formatting error introduced by merge code

1fb7c95

shankari mentioned this pull request Jan 20, 2024

‼️📅 Rewrite - notifScheduler.ts #1092

Merged

shankari requested changes Jan 21, 2024

View reviewed changes

the-bay-kay added 7 commits January 23, 2024 08:28

Merge branch 'service_rewrite_2023' into diaryServices-rewrite

e69138b

Update func param types, fix func formatting

a89f356

- Expanded types for several function parameters - Changed exported functions of `() => {}` format to `function () {}`

Renamed ServerData to BEMData

d54ce29

This better reflects the origin of this type: both BEMServerComm and BEMUserCache return data of this structure.

Imporved test in timelineHelper.test.ts

cdb7138

Test now compares the unpacked object map to original object.

Ran prettier from www/

95d6255

Was running on `js/`, but not mocks; fixed formatting in mocks and tests!

Updated diaryTypes to use GeoJSON Point

9f73f74

Prettier after Point change

f54257b

shankari approved these changes Jan 25, 2024

View reviewed changes

shankari merged commit ee32008 into e-mission:service_rewrite_2023 Jan 25, 2024
5 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

✏️📖 Diary Services Rewrite #1085

✏️📖 Diary Services Rewrite #1085

the-bay-kay commented Oct 26, 2023

the-bay-kay commented Nov 3, 2023 •

edited

Loading

JGreenlee commented Nov 4, 2023

JGreenlee commented Nov 4, 2023

the-bay-kay commented Nov 9, 2023

the-bay-kay commented Nov 9, 2023

codecov bot commented Dec 6, 2023 •

edited

Loading

shankari commented Jan 20, 2024

shankari commented Jan 20, 2024

shankari left a comment

shankari commented Jan 21, 2024

sebastianbarry commented Jan 22, 2024

shankari commented Jan 22, 2024

jiji14 commented Jan 22, 2024

shankari commented Jan 22, 2024

jiji14 commented Jan 22, 2024

sebastianbarry commented Jan 22, 2024 •

edited

Loading

shankari commented Jan 25, 2024

✏️📖 Diary Services Rewrite #1085

✏️📖 Diary Services Rewrite #1085

Conversation

the-bay-kay commented Oct 26, 2023

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

JGreenlee commented Nov 4, 2023

JGreenlee commented Nov 4, 2023

the-bay-kay commented Nov 9, 2023

the-bay-kay commented Nov 9, 2023

codecov bot commented Dec 6, 2023 • edited Loading

Codecov Report

shankari commented Jan 20, 2024

shankari commented Jan 20, 2024

shankari left a comment

Choose a reason for hiding this comment

shankari commented Jan 21, 2024

sebastianbarry commented Jan 22, 2024

shankari commented Jan 22, 2024

jiji14 commented Jan 22, 2024

shankari commented Jan 22, 2024

jiji14 commented Jan 22, 2024

sebastianbarry commented Jan 22, 2024 • edited Loading

shankari commented Jan 25, 2024

the-bay-kay commented Nov 3, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading

sebastianbarry commented Jan 22, 2024 •

edited

Loading