‼️📅 Rewrite - notifScheduler.ts #1092

sebastianbarry · 2023-10-31T20:09:38Z

Translated notifScheduler.js from Angular to React Typescript

commHelper.ts

Added a temporary "any" return type for getUser because the TS needed it

ProfileSettings.jsx

Imported the new useSchedulerHelper function (name in progress - need to find a good name for this hook)
Replaced any instances of the old Angular NotificationScheduler module with the new schedulerHelper hook

notifScheduler.ts

Translated the file from Angular to React TS
This file exports a single function which initializes the appConfig and then returns an object containing the functions that the Notification Scheduler functions need (only ProfileSettings.jsx is using this file currently)
Replaced any instances of moment with Luxon equivalent

Translated notifScheduler.js from Angular to React Typescript commHelper.ts - Added a temporary "any" return type for getUser because the TS needed it ProfileSettings.jsx - Imported the new useSchedulerHelper function (name in progress - need to find a good name for this hook) - Replaced any instances of the old Angular NotificationScheduler module with the new schedulerHelper hook notifScheduler.ts - Translated the file from Angular to React TS - This file exports a single function which initializes the appConfig and then returns an object containing the functions that the Notification Scheduler functions need (only ProfileSettings.jsx is using this file currently) - Replaced any instances of moment with Luxon equivalent

sebastianbarry · 2023-10-31T20:11:12Z

@JGreenlee Added this to your review channel and @ you for visibility

If you have a moment, could you take a look at this at a high level and let me know if the structure needs to be changed at all?

I also have another question: Is notifScheduler at a high level only being used by programs and studies that use reminderSchemes in their config?

For example: https://github.com/e-mission/nrel-openpath-deploy-configs/blob/4470d5e9eec2a4c34dbd9cc0d98f57029ca022fb/configs/dev-emulator-timeuse.nrel-op.json#L113

sebastianbarry · 2023-10-31T20:13:09Z

Sorry, my finger slipped and it accidentally closed the PR 😖

sebastianbarry · 2023-10-31T20:16:50Z

Testing:

Description	Screenshot
Notifications populate in "Developer Zone -> Upcoming Notifications"
Promise rejection errors popping up from outside files now

sebastianbarry · 2023-10-31T20:20:45Z

To-Do:

Address the promise rejection errors
Come up with a good name for the hook?
Add stronger typing for the typescript (for example, the user returned in getUser should be typed)
Consider whether we want to use a local variable for the appConfig instead of passing it as an argument down the chain of functions
Test the notifications for every use case (this is something I'm confused about -- I'm not sure the best way to test this and know that what I've written doesn't have any regressions or issues; does this PR need Jest testing in it as well? How do you go about unit testing phone notifications?)

@JGreenlee can you think of anything else I might want to add to this to-do list?

JGreenlee

I left my suggestions! In short, I think the React hook is unnecessary because ProfileSettings already has access to appConfig, so you can pass its reminderSchemes into the notifScheduler functions.

And here's my answer to that question:

I also have another question: Is notifScheduler at a high level only being used by programs and studies that use reminderSchemes in their config?

Correct- if reminderSchemes is not present, nothing in notifScheduler should run

JGreenlee · 2023-11-01T17:41:20Z

www/js/commHelper.ts

@@ -147,7 +147,7 @@ export function updateUser(updateDoc) {
  });
 }

-export function getUser() {
+export function getUser(): any {


As you said, we'd ideally type this out properly. I understand if need to use any in the short term, but instead of adding an any return type here, I think you should be able to do this at the place where getUser() is being called.

Like so:

const user = getUser() as any;

Let me know if that works!

JGreenlee · 2023-11-01T17:50:33Z

www/js/splash/notifScheduler.ts

+export function useSchedulerHelper() {
+    const appConfig = useAppConfig();
+    const [reminderSchemes, setReminderSchemes] = useState();
+
+    useEffect(() => {
+        if (!appConfig) {
+            logDebug("No reminder schemes found in config, not scheduling notifications");
+            return;
+        }
+        setReminderSchemes(appConfig.reminderSchemes);
+    }, [appConfig]);
+
+    //setUpActions();
+    update(reminderSchemes);
+
+    return {
+        setReminderPrefs: (newPrefs) => setReminderPrefs(newPrefs, reminderSchemes),
+        getReminderPrefs: () => getReminderPrefs(reminderSchemes),
+        getScheduledNotifs: () => getScheduledNotifs(),
+    }
+}


After seeing the code, I actually think this would be simpler and more concise without using a React hook.
There isn't anything about this code that is necessarily React-based or having to do with the UI components. It's really a matter of conditionally formatting data, which we can do just as well in plain TS.

We already have access to the appConfig/uiConfig in ProfileSettings.jsx, so it's a bit redundant to invoke useAppConfig() again in this hook.
And reminderSchemes is just part of appConfig - it doesn't really need to be its own state.

JGreenlee · 2023-11-01T17:59:20Z

www/js/splash/notifScheduler.ts

+}
+
+// determines when notifications are needed, and schedules them if not already scheduled
+const update = async (reminderSchemes) => {


I see you already accept reminderSchemes as an argument to all the functions that need it, which a decision that I think is fine.
So why not export this function and call it directly from ProfileSettings? ProfileSettings has appConfig, which is all you need

JGreenlee · 2023-11-01T18:01:30Z

www/js/control/ProfileSettings.jsx

@@ -25,6 +25,7 @@ import { AppContext } from "../App";
 import { shareQR } from "../components/QrCode";
 import { storageClear } from "../plugin/storage";
 import { getAppVersion } from "../plugin/clientStats";
+import { useSchedulerHelper } from "../splash/notifScheduler";


If you export update, getScheduledNotifs, getReminderPrefs, and setReminderPrefs directly from notifScheduler.ts, you can import them as individual functions instead of useSchedulerHelper

JGreenlee · 2023-11-01T18:06:02Z

www/js/control/ProfileSettings.jsx

@@ -173,13 +175,13 @@ const ProfileSettings = () => {
        const newNotificationSettings ={};

        if (uiConfig?.reminderSchemes) {
-            const prefs = await  NotificationScheduler.getReminderPrefs();
+            const prefs = await schedulerHelper.getReminderPrefs();


This will become await getReminderPrefs(uiConfig.reminderSchemes)

JGreenlee · 2023-11-01T18:09:01Z

www/js/splash/notifScheduler.ts

+    }, [appConfig]);
+
+    //setUpActions();
+    update(reminderSchemes);


If you remove the React hook, you'll need to do this update call somewhere else. You can do it in ProfileSettings somewhere in the whenReady function.

JGreenlee · 2023-11-01T18:10:21Z

www/js/splash/notifScheduler.ts

+    } catch (e) {
+        console.log("ERROR: Could not find reminder scheme for assignment " + reminderSchemes + " - " + reminder_assignment);
+    }


Appreciate the addition of this catch block - but instead of console.log let's displayErrorMsg (from logger.ts)

@jiji14

Following the Prettier changes in @jiji14 commit bb73676

In order to not mess with commHelper.ts as it is outside of the scope of this issue - related to e-mission#1092 (comment)

During the prettier changes, a bunch of the imports got merged. Undoing in this commit: e-mission@1bfb113

Per e-mission#1092 (comment)

www/js/splash/notifScheduler.ts

JGreenlee · 2023-11-07T06:00:59Z

www/js/control/ProfileSettings.jsx

@@ -187,13 +201,13 @@ const ProfileSettings = () => {
    const newNotificationSettings = {};

    if (uiConfig?.reminderSchemes) {
-      const prefs = await NotificationScheduler.getReminderPrefs();
+      const prefs = await getReminderPrefs();


This function takes reminderSchemes as a parameter, doesn't it?

Suggested change

const prefs = await getReminderPrefs();

const prefs = await getReminderPrefs(uiConfig.reminderSchemes);

I did forget this, good catch! It was slipping by in my testing without this, because reminderSchemes is only used in getReminderPrefs() if the user object does not already exist on the server and it needs to do the initialization of notification-scheduling for a new user

JGreenlee

LGTM

sebastianbarry · 2023-11-07T16:22:38Z

@JGreenlee do we also want to add unit testing within our rewrite PRs?

JGreenlee

Yes, I almost forgot!

We should include unit tests for as much of this as possible.
Although unit tests can't actually verify that native notifications will display on a phone, you can verify that the correct notifications are getting scheduled for the right times.

You could have a mock implementation for the native plugin which keeps track of all the scheduled notifications.

sebastianbarry · 2023-11-07T16:33:57Z

I'm also having a significant problem: my initial testing after pushing the last commits was working fine, but now I have 1. Created a new OPcode, and 2. reloaded the app, and now the app is crashing when I so much as open the profile settings screen:

Untitled.mov

@JGreenlee do you have any tips for debugging an issue that causes the app to crash? Maybe a log dump or something that can give me insight into what is happening to make the app crash? The phone serve does not crash, just the app itself

I also had to adjust the error coming from notifScheduler because it wasn't helping much with the way it was defined before

This tests the final functionality and longest path through this function

…-mission-phone into notifScheduler-rewrite

JGreenlee · 2023-11-30T17:31:24Z

Resolved merge conflicts

www/js/splash/notifScheduler.ts

sebastianbarry · 2023-12-01T16:18:41Z

I am getting a bit stuck on the next test and was wondering if anyone had any experience or ideas in terms of changing a Jest mocked function implementation for only one test (I commented the word THIS to show what I'm talking about:

// ...Somewhere earlier in the Jest file (outside of any of the `describes`)
jest.mock('../js/services/commHelper', () => ({
  ...jest.requireActual('../js/services/commHelper'),
  getUser: jest.fn(() =>
    Promise.resolve({
      // These values are **important**...
      //   reminder_assignment: must match a key from the reminder scheme above,
      //   reminder_join_date: must match the first day of the mocked notifs below in the tests,
      //   reminder_time_of_day: must match the defaultTime from the chosen reminder_assignment in the reminder scheme above
      reminder_assignment: 'weekly',
      reminder_join_date: '2023-11-14',
      reminder_time_of_day: '21:00',
    }),
  ),
  updateUser: jest.fn(() => Promise.resolve()),
}));


// ...Inside the test for getReminderPrefs
  it('should resolve with newly initialilzed reminder prefs when user does not exist', async () => {
    // getReminderPrefs arguments
    const reminderSchemes: any = exampleReminderSchemes;
    let isScheduling: boolean = true;
    const setIsScheduling: Function = jest.fn((val: boolean) => (isScheduling = val));
    const scheduledPromise: Promise<any> = Promise.resolve();
    // create the expected result
    const expectedResult = {
      reminder_assignment: 'weekly',
      reminder_join_date: '2023-11-14',
      reminder_time_of_day: '21:00',
    };

    // --------------
    // THIS 
    // --------------
    // mock the return value of getUser (a promise that resolves with empty object {}
    // this way, the getReminderPrefs skips the logic for "user already exists" and tries to set them up for the first time
    jest.doMock('../js/commHelper', () => ({
      ...jest.requireActual('../js/commHelper'),
      getUser: jest.fn(() => Promise.resolve({})),
      updateUser: jest.fn(() => Promise.resolve()),
    }));


    // call the function
    const { reminder_assignment, reminder_join_date, reminder_time_of_day } =
      await getReminderPrefs(reminderSchemes, isScheduling, setIsScheduling, scheduledPromise);

    expect(logDebug).toHaveBeenCalledWith('User just joined, Initializing reminder prefs');
  });

The issue is that when I set a console.log(user) within notifScheduler.ts (the actual file) and run the test, getUser() still returns with the full mocked object instead of my replacement of it {}. It appears that Jest is either ignoring my re-mocking of this, I am not re-mocking correctly, or there is some logic that causes the outer shell of mocking to take precedence over the mocks within the it tests. Does anyone have any thoughts on this?

I looked at this stack overflow discussion but it doesn't appear to have a direct answer (or a few other similar threads), aside from changing the way the mock is implemented entirely and just mocking within each individual test (which I don't think is a good solution because it will make this file HUGE).

Abby-Wheelis · 2023-12-01T16:30:54Z

@sebastianbarry, I think I've worked on similar situations, and I would recommend checking how the user is stored (some things are cached locally, and so even if you overwrite the fetching process, it will use what was fetched in another test). Maybe you'd be able to get some ideas from the tests I wrote in #1063 or the discussion in that PR, I know changing the mocks out was something that Jack and I discussed/worked on in review. And in #1093 with the tests for storeDeviceSettings I think I had to clear storage to reset the user. Maybe your problem in similar to one of those fixes?

the-bay-kay · 2023-12-01T17:04:37Z

It appears that Jest is either ignoring my re-mocking of this, I am not re-mocking correctly, or there is some logic that causes the outer shell of mocking to take precedence over the mocks within the it tests. Does anyone have any thoughts on this?

For re-mocking, you may want to check out the .mockImplementationOnce() function! FWIW, I've personally had some issues making it run smoothly - but if I understand your problem correctly, this may be a solution. You could try something like...

jest.mock('../js/services/commHelper', () => ({
  ...jest.requireActual('../js/services/commHelper'),
  getUser: jest.fn(() =>
    Promise.resolve({ // This is the default mock
      // These values are **important**...
      //   reminder_assignment: must match a key from the reminder scheme above,
      //   reminder_join_date: must match the first day of the mocked notifs below in the tests,
      //   reminder_time_of_day: must match the defaultTime from the chosen reminder_assignment in the reminder scheme above
      reminder_assignment: 'weekly',
      reminder_join_date: '2023-11-14',
      reminder_time_of_day: '21:00',
    }) // The "Once" will get called before the default, so you may need to reorder the tests...
      .mockImplementationOnce(() => {Promise.resolve({})),
  ),
  updateUser: jest.fn(() => Promise.resolve()),

Obviously it doesn't feel the best mocking functions on a one-off basis, but this would let you keep using the original mock at least! If not this, I found a workaround by mocking the higher-order functions, so that different inputs returned different values (The code for those mocks is here for how I got around using mockImplementationOnce if you're curious! I'm not sure it is the exact same issue, but hopefully it helps : ))

…-mission-phone into notifScheduler-rewrite

If there are no reminderSchemes in the appConfig, notifScheduler doesn't need to do anything so let's not invoke it. Also, let's create the type definition for reminderSchemes and use it here in notifScheduler.

JGreenlee · 2023-12-06T19:57:40Z

Just resolved merge conflicts and fixed a bug.

JGreenlee

The rewrite itself looks good to me.

Not every function is tested yet, but the tests we do have are passing. I suggest we merge this as-is to unblock #1106 and then fill in the rest of the tests in a later PR.

codecov · 2023-12-06T20:03:18Z

Codecov Report

Merging #1092 (ec9729d) into service_rewrite_2023 (1006165) will increase coverage by 3.30%.
Report is 43 commits behind head on service_rewrite_2023.
The diff coverage is 100.00%.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           service_rewrite_2023    #1092      +/-   ##
========================================================
+ Coverage                 58.80%   62.11%   +3.30%     
========================================================
  Files                        26       27       +1     
  Lines                      1420     1544     +124     
  Branches                    320      334      +14     
========================================================
+ Hits                        835      959     +124     
  Misses                      585      585

Flag	Coverage Δ
unit	`62.11% <100.00%> (+3.30%)`	⬆️

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

Files	Coverage Δ
www/js/splash/notifScheduler.ts	`100.00% <100.00%> (ø)`

notifScheduler.test.ts - Imported getUser and updateUser so that we can mock them in the tests - Imported addStatReading so we can mock it to do nothing - Removed the old way of mocking the implementation of getUser, and the new way allows us to change them per-test - Added automatic mock for clientStats - added beforeEach statements to mock getUser inside the describe blocks -

- Tests to see if setReminderPrefs works when called in the way that ProfileSettings.jsx calls it

sebastianbarry · 2023-12-12T18:30:25Z

After committing those final tests, I believe this PR is ready for review! 🎉

@JGreenlee the main things I'm interested in is:

Are my tests actually testing the use of the functions? i.e. I'm not expecting the wrong types of things (like testing logDebug arguments too much for what is appropriate)
Are my tests readable?

JGreenlee

The purpose of tests is to verify that when a function is called, the expected result is achieved. It looks to me like after calling updateScheduledNotifs, we don't actually make sure that those notifications were scheduled.

I think we need to revise the approach for testing that function, but I am still going to recommend merging this now and bulking up the tests in a later PR.

JGreenlee · 2023-12-13T17:15:34Z

www/__tests__/notifScheduler.test.ts

+    await updateScheduledNotifs(reminderSchemes, isScheduling, setIsScheduling, scheduledPromise);
+
+    expect(setIsScheduling).toHaveBeenCalledWith(true);
+    expect(logDebug).toHaveBeenCalledWith('After cancelling, there are no scheduled notifications');
+    expect(logDebug).toHaveBeenCalledWith('After scheduling, there are no scheduled notifications');
+    expect(setIsScheduling).toHaveBeenCalledWith(false);


The tests should be checking the functionality of notifScheduler. In this instance we want to see that the appropriate notifications are scheduled. The logDebug statements are good for dev purposes but I don't think they should be the focus of our testing strategy.

After calling updateScheduledNotifs(...) we should verify that the function did what it was supposed to do.
In this case, I think the best way to do that is to then call getScheduledNotifs(...) and check the result.

@JGreenlee I added the test you suggested, to test getScheduledNotifs(...) in ec9729d

Great suggestion, thanks!

JGreenlee · 2023-12-13T17:17:17Z

www/__tests__/notifScheduler.test.ts

+    expect(logDebug).toHaveBeenCalledWith(
+      'ERROR: Already scheduling notifications, not scheduling again',
+    );


In this instance, though, I think checking the logDebug does make sense because we expect updateScheduledNotifs to have quit early and not scheduled any notifications.

I agree with:

The logDebug statements are good for dev purposes but I don't think they should be the focus of our testing strategy.

In a future fix, I would suggest changing the underlying function to return a value that you can check, and then changing the test to check it

JGreenlee · 2023-12-13T17:17:31Z

www/__tests__/notifScheduler.test.ts

+    // call the function
+    await updateScheduledNotifs(reminderSchemes, isScheduling, setIsScheduling, scheduledPromise);
+
+    expect(logDebug).toHaveBeenCalledWith('Error: Reminder scheme not found');


This one is fine too

… scheduled - There were many places where I used to/fromMillis instead of to/fromJSDate, causing a type difference that was causing errors - Added a i18n mock because not having a language code return was messing with the debugScheduledNotifs log outputs - Added a funcitonality for mockNotifs in the test, so that it can be adjusted, cleared, added to, etc. so that we can test with getScheduledNotifs if it successfully scheduled the notifs

shankari

I have some comments/questions, but am going to go ahead and merge this so that we can get the release out and clean this up later, consistent with

I suggest we merge this as-is to unblock #1106 and then fill in the rest of the tests in a later PR.

shankari · 2024-01-20T03:57:28Z

www/js/control/ProfileSettings.jsx

+      promiseList.push(
+        getReminderPrefs(uiConfig.reminderSchemes, isScheduling, setIsScheduling, scheduledPromise),
+      );
+      promiseList.push(getScheduledNotifs(isScheduling, scheduledPromise));
+      let resultList = await Promise.all(promiseList);


nit, future fix: can you add a comment about why we are reading both the values together instead of two separate awaits? It is fine if the answer is "it is cleaner" - I just wanted to understand the reason for the change.

shankari · 2024-01-20T04:01:09Z

www/__mocks__/globalMocks.ts

@@ -1,5 +1,11 @@
 export const mockLogger = () => {
  window['Logger'] = { log: console.log };
+  window.alert = (msg) => {
+    console.log(msg);


If we are keeping the logDebug tests, we should change this from console.log to logDebug so we can test the alerting behavior.

shankari · 2024-01-20T04:18:59Z

www/js/splash/notifScheduler.ts

+  isScheduling: boolean,
+  setIsScheduling: Function,


Where are these actually used? I tried to follow it through, but it looks like a circular invocation:
updateScheduledNotifs -> getReminderPrefs -> setReminderPrefs -> updateScheduledNotifs and getReminderPrefs but I don't see where this is actually invoked!?

shankari · 2024-01-20T05:04:24Z

www/__tests__/notifScheduler.test.ts

+    let reminderSchemes: any = exampleReminderSchemes;
+    delete reminderSchemes.weekly; // delete the weekly reminder scheme, to create a missing reminder scheme error
+    let isScheduling: boolean = false;
+    const setIsScheduling: Function = jest.fn((val: boolean) => (isScheduling = val));
+    const scheduledPromise: Promise<any> = Promise.resolve();
+    // call the function
+    await updateScheduledNotifs(reminderSchemes, isScheduling, setIsScheduling, scheduledPromise);
+
+    expect(logDebug).toHaveBeenCalledWith('Error: Reminder scheme not found');


I don't understand this. you are removing the weekly reminderScheme from the local in-memory variable.
There are no existing reminders, right? So how would updateScheduledNotifs know that there was supposed to have been a weekly scheme?

shankari · 2024-01-20T23:14:11Z

After this merge, notifScheduler tests are failing while running jest for code coverage
https://github.com/e-mission/e-mission-phone/actions/runs/7597279222/job/20691918950?pr=1085
while run as part of testing #1085

@sebastianbarry please fix ASAP

Summary of all failing tests
FAIL www/__tests__/notifScheduler.test.ts
  ● updateScheduledNotifs › should resolve after scheduling notifications

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: "After scheduling, there are 4 scheduled notifications at 21:00 first is November 19, 2023 at 9:00 PM"
    Received
           1: "requesting fetch while still actively scheduling, waiting on scheduledPromise"
           2: "there are no notifications"
           3: "User already has reminder prefs, returning them: {\"reminder_assignment\":\"weekly\",\"reminder_join_date\":\"2023-11-14\",\"reminder_time_of_day\":\"21:00\"}"

    Number of calls: 5

      270 |     expect(setIsScheduling).toHaveBeenCalledWith(true);
      271 |     expect(logDebug).toHaveBeenCalledWith('After cancelling, there are no scheduled notifications');
    > 272 |     expect(logDebug).toHaveBeenCalledWith(
          |                      ^
      273 |       'After scheduling, there are 4 scheduled notifications at 21:00 first is November 19, 2023 at 9:00 PM',
      274 |     );
      275 |     expect(setIsScheduling).toHaveBeenCalledWith(false);

      at toHaveBeenCalledWith (www/__tests__/notifScheduler.test.ts:272:22)
      at call (www/__tests__/notifScheduler.test.ts:2:1)
      at Generator.tryCatch (www/__tests__/notifScheduler.test.ts:2:1)
      at Generator._invoke [as next] (www/__tests__/notifScheduler.test.ts:2:1)
      at asyncGeneratorStep (www/__tests__/notifScheduler.test.ts:2:1)
      at asyncGeneratorStep (www/__tests__/notifScheduler.test.ts:2:1)

sebastianbarry closed this Oct 31, 2023

sebastianbarry reopened this Oct 31, 2023

sebastianbarry marked this pull request as draft October 31, 2023 20:12

sebastianbarry changed the title ~~Add notifScheduler.ts~~ ‼️📅 Rewrite - notifScheduler.ts Oct 31, 2023

JGreenlee requested changes Nov 1, 2023

View reviewed changes

sebastianbarry added 12 commits November 2, 2023 14:24

Merge branch 'service_rewrite_2023' into notifScheduler-rewrite

da87f47

Merge branch 'service_rewrite_2023' into notifScheduler-rewrite

1bfb113

Format notifScheduler.ts

131eca5

Following the Prettier changes in @jiji14 commit bb73676

Merge branch 'service_rewrite_2023' into notifScheduler-rewrite

cc7664f

Temporary any type, handled purely in notifScheduler getReminderPrefs

39986a3

In order to not mess with commHelper.ts as it is outside of the scope of this issue - related to e-mission#1092 (comment)

Undoing imports getting duplicated during prettier changes

58539ca

During the prettier changes, a bunch of the imports got merged. Undoing in this commit: e-mission@1bfb113

Replace console.log with logger.ts's displayErrorMsg

384ffe8

Per e-mission#1092 (comment)

Move scheduledNotifs into the only function that uses it

9d3c718

Remove console.errors that I was using for debugging

a1e035f

Adding Luxon functions

8b09cb3

Update notifscheduler to export functions instead of using hooks

96d1d50

Run prettier on non-pretty code

a6d99e7

JGreenlee reviewed Nov 7, 2023

View reviewed changes

www/js/splash/notifScheduler.ts Show resolved Hide resolved

JGreenlee reviewed Nov 7, 2023

View reviewed changes

sebastianbarry added 2 commits November 7, 2023 09:07

Remove old Angular notifScheduler.js

b0da6e9

Add the reminderSchemes as an argument to called getReminderPrefs

6b9d222

JGreenlee approved these changes Nov 7, 2023

View reviewed changes

JGreenlee requested changes Nov 7, 2023

View reviewed changes

sebastianbarry and others added 5 commits November 21, 2023 09:33

Add reminder scheme missing test for updateScheduledNotifs

87fdae5

I also had to adjust the error coming from notifScheduler because it wasn't helping much with the way it was defined before

Add already scheduled notifs test

214081e

Fixing comments

f5ebb1e

Final overall test for updateScheduledNotifs

6892580

This tests the final functionality and longest path through this function

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

e8b449f

…-mission-phone into notifScheduler-rewrite

Added user exists test for getReminderPrefs

85f0519

sebastianbarry commented Nov 30, 2023

View reviewed changes

www/js/splash/notifScheduler.ts Outdated Show resolved Hide resolved

sebastianbarry and others added 2 commits November 30, 2023 16:05

Remove unnecessary console.log

0528fb0

Fixing moved location of commHelper causing jest tests to fail

2c0ec40

JGreenlee added 2 commits December 6, 2023 14:44

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

35ffc41

…-mission-phone into notifScheduler-rewrite

fix error on configs without reminderSchemes

4e44dba

If there are no reminderSchemes in the appConfig, notifScheduler doesn't need to do anything so let's not invoke it. Also, let's create the type definition for reminderSchemes and use it here in notifScheduler.

JGreenlee approved these changes Dec 6, 2023

View reviewed changes

JGreenlee mentioned this pull request Dec 7, 2023

🎀 UI Migration Wrap-up #1106

Merged

sebastianbarry added 2 commits December 12, 2023 10:59

Add simple setReminderPrefs test

d3750c3

- Tests to see if setReminderPrefs works when called in the way that ProfileSettings.jsx calls it

sebastianbarry marked this pull request as ready for review December 12, 2023 18:30

JGreenlee requested changes Dec 13, 2023

View reviewed changes

shankari approved these changes Jan 20, 2024

View reviewed changes

shankari merged commit 14c748d into e-mission:service_rewrite_2023 Jan 20, 2024
4 checks passed

sebastianbarry mentioned this pull request Jan 22, 2024

✏️📖 Diary Services Rewrite #1085

Merged

shankari mentioned this pull request Feb 3, 2024

🎀 Final change of the service rewrite #1126

Merged

	const prefs = await getReminderPrefs();
	const prefs = await getReminderPrefs(uiConfig.reminderSchemes);

‼️📅 Rewrite - notifScheduler.ts #1092

‼️📅 Rewrite - notifScheduler.ts #1092

Conversation

sebastianbarry commented Oct 31, 2023

sebastianbarry commented Oct 31, 2023 • edited Loading

sebastianbarry commented Oct 31, 2023

sebastianbarry commented Oct 31, 2023 • edited Loading

Testing:

sebastianbarry commented Oct 31, 2023 • edited Loading

To-Do:

JGreenlee 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

Choose a reason for hiding this comment

sebastianbarry Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

JGreenlee left a comment

Choose a reason for hiding this comment

sebastianbarry commented Nov 7, 2023

JGreenlee left a comment • edited Loading

Choose a reason for hiding this comment

sebastianbarry commented Nov 7, 2023

JGreenlee commented Nov 30, 2023

sebastianbarry commented Dec 1, 2023

Abby-Wheelis commented Dec 1, 2023

the-bay-kay commented Dec 1, 2023 • edited Loading

JGreenlee commented Dec 6, 2023

JGreenlee left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2023 • edited Loading

Codecov Report

sebastianbarry commented Dec 12, 2023

JGreenlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianbarry Dec 15, 2023 • edited Loading

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

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

shankari commented Jan 20, 2024 • edited Loading

sebastianbarry commented Oct 31, 2023 •

edited

Loading

sebastianbarry commented Oct 31, 2023 •

edited

Loading

sebastianbarry commented Oct 31, 2023 •

edited

Loading

sebastianbarry Nov 7, 2023 •

edited

Loading

JGreenlee left a comment •

edited

Loading

the-bay-kay commented Dec 1, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading

sebastianbarry Dec 15, 2023 •

edited

Loading

shankari commented Jan 20, 2024 •

edited

Loading