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

‼️📅 Rewrite - notifScheduler.ts #1092

Merged
Merged
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
4abe86d
Add notifScheduler.ts
sebastianbarry Oct 31, 2023
da87f47
Merge branch 'service_rewrite_2023' into notifScheduler-rewrite
sebastianbarry Nov 2, 2023
1bfb113
Merge branch 'service_rewrite_2023' into notifScheduler-rewrite
sebastianbarry Nov 2, 2023
131eca5
Format notifScheduler.ts
sebastianbarry Nov 2, 2023
cc7664f
Merge branch 'service_rewrite_2023' into notifScheduler-rewrite
sebastianbarry Nov 2, 2023
39986a3
Temporary any type, handled purely in notifScheduler getReminderPrefs
sebastianbarry Nov 2, 2023
58539ca
Undoing imports getting duplicated during prettier changes
sebastianbarry Nov 2, 2023
384ffe8
Replace console.log with logger.ts's displayErrorMsg
sebastianbarry Nov 2, 2023
9d3c718
Move scheduledNotifs into the only function that uses it
sebastianbarry Nov 3, 2023
a1e035f
Remove console.errors that I was using for debugging
sebastianbarry Nov 3, 2023
8b09cb3
Adding Luxon functions
sebastianbarry Nov 3, 2023
96d1d50
Update notifscheduler to export functions instead of using hooks
sebastianbarry Nov 3, 2023
a6d99e7
Run prettier on non-pretty code
sebastianbarry Nov 3, 2023
b0da6e9
Remove old Angular notifScheduler.js
sebastianbarry Nov 7, 2023
6b9d222
Add the reminderSchemes as an argument to called getReminderPrefs
sebastianbarry Nov 7, 2023
97571ee
Added typing for User object in getReminderPrefs
sebastianbarry Nov 7, 2023
a29705d
Restructure promise format and replaced moment with Luxon
sebastianbarry Nov 7, 2023
9b4b1d0
Merge branch 'service_rewrite_2023' of https://github.com/e-mission/e…
JGreenlee Nov 9, 2023
ba239e6
Resolve the app crashing errors
sebastianbarry Nov 9, 2023
2f75de9
Remove unnecessary imports
sebastianbarry Nov 10, 2023
5e572d4
notif-scheduling-state variables kept in ProfileSettings.jsx and hand…
sebastianbarry Nov 14, 2023
db91c8c
Remove empty objects from notifs, fixing the "n.trigger.at does not e…
sebastianbarry Nov 14, 2023
e4817da
Added removeEmptyObjects check to the other cordova plugin getSchedul…
sebastianbarry Nov 14, 2023
0ee468a
Added sorting to list of notifications right before they get scheduled
sebastianbarry Nov 16, 2023
e209f10
Change static variable isScheduling to a useState
sebastianbarry Nov 16, 2023
b8bf1cf
Replace console.logs with logDebugs
sebastianbarry Nov 16, 2023
b27ae9a
Add in more type declarations
sebastianbarry Nov 16, 2023
abbe618
Add Jest Testing - getScheduledNotifs
sebastianbarry Nov 16, 2023
fbd1b62
Add first updateScheduledNotifs test
sebastianbarry Nov 17, 2023
87fdae5
Add reminder scheme missing test for updateScheduledNotifs
sebastianbarry Nov 21, 2023
214081e
Add already scheduled notifs test
sebastianbarry Nov 22, 2023
f5ebb1e
Fixing comments
sebastianbarry Nov 22, 2023
6892580
Final overall test for updateScheduledNotifs
sebastianbarry Nov 24, 2023
e8b449f
Merge branch 'service_rewrite_2023' of https://github.com/e-mission/e…
JGreenlee Nov 30, 2023
85f0519
Added user exists test for getReminderPrefs
sebastianbarry Nov 30, 2023
0528fb0
Remove unnecessary console.log
sebastianbarry Nov 30, 2023
2c0ec40
Fixing moved location of commHelper causing jest tests to fail
sebastianbarry Nov 30, 2023
35ffc41
Merge branch 'service_rewrite_2023' of https://github.com/e-mission/e…
JGreenlee Dec 6, 2023
4e44dba
fix error on configs without reminderSchemes
JGreenlee Dec 6, 2023
5ee4d64
Add getReminderPrefs test for when user does not exist
sebastianbarry Dec 12, 2023
d3750c3
Add simple setReminderPrefs test
sebastianbarry Dec 12, 2023
ec9729d
Add test for updateScheduledNotifs to test if notifs successfully get…
sebastianbarry Dec 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions www/__mocks__/cordovaMocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ export const mockCordova = () => {
window['cordova'].plugins ||= {};
};

export const mockReminders = () => {
window['cordova'] ||= {};
window['cordova'].plugins ||= {};
window['cordova'].plugins.notification ||= {};
window['cordova'].plugins.notification.local ||= {};
window['cordova'].plugins.notification.local.getScheduled ||= () => [];
window['cordova'].plugins.notification.local.cancelAll ||= () => {};
window['cordova'].plugins.notification.local.schedule ||= () => {};
};

export const mockDevice = () => {
window['device'] ||= {};
window['device'].platform ||= 'ios';
Expand Down
6 changes: 6 additions & 0 deletions www/__mocks__/globalMocks.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
export const mockLogger = () => {
window['Logger'] = { log: console.log };
window.alert = (msg) => {
console.log(msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

};
console.error = (msg) => {
console.log(msg);
};
};

let alerts = [];
Expand Down
333 changes: 333 additions & 0 deletions www/__tests__/notifScheduler.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,333 @@
import { mockReminders } from '../__mocks__/cordovaMocks';
import { mockLogger } from '../__mocks__/globalMocks';
import { logDebug } from '../js/plugin/logger';
import { DateTime } from 'luxon';
import {
getScheduledNotifs,
updateScheduledNotifs,
getReminderPrefs,
setReminderPrefs,
} from '../js/splash/notifScheduler';

const exampleReminderSchemes = {
weekly: {
title: {
en: 'Please take a moment to label your trips',
es: 'Por favor, tómese un momento para etiquetar sus viajes',
},
text: {
en: 'Click to open the app and view unlabeled trips',
es: 'Haga clic para abrir la aplicación y ver los viajes sin etiquetar',
},
schedule: [
{ start: 0, end: 1, intervalInDays: 1 },
{ start: 3, end: 5, intervalInDays: 2 },
],
defaultTime: '21:00',
},
'week-quarterly': {
title: {
en: 'Please take a moment to label your trips',
es: 'Por favor, tómese un momento para etiquetar sus viajes',
},
text: {
en: 'Click to open the app and view unlabeled trips',
es: 'Haga clic para abrir la aplicación y ver los viajes sin etiquetar',
},
schedule: [
{ start: 0, end: 1, intervalInDays: 1 },
{ start: 3, end: 5, intervalInDays: 2 },
],
defaultTime: '22:00',
},
passive: {
title: {
en: 'Please take a moment to label your trips',
es: 'Por favor, tómese un momento para etiquetar sus viajes',
},
text: {
en: 'Click to open the app and view unlabeled trips',
es: 'Haga clic para abrir la aplicación y ver los viajes sin etiquetar',
},
schedule: [
{ start: 0, end: 1, intervalInDays: 1 },
{ start: 3, end: 5, intervalInDays: 2 },
],
defaultTime: '23:00',
},
};

mockLogger();
mockReminders();

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()),
}));

jest.mock('../js/plugin/clientStats', () => ({
...jest.requireActual('../js/plugin/clientStats'),
addStatReading: jest.fn(),
}));

jest.mock('../js/plugin/logger', () => ({
...jest.requireActual('../js/plugin/logger'),
logDebug: jest.fn(),
}));

jest.mock('../js/splash/notifScheduler', () => ({
...jest.requireActual('../js/splash/notifScheduler'),
// for getScheduledNotifs
getNotifs: jest.fn(),
// for updateScheduledNotifs
calcNotifTimes: jest.fn(),
removeEmptyObjects: jest.fn(),
areAlreadyScheduled: jest.fn(),
scheduleNotifs: jest.fn(),
}));

describe('getScheduledNotifs', () => {
it('should resolve with notifications while not actively scheduling', async () => {
// getScheduledNotifs arguments
const isScheduling = false;
const scheduledPromise = Promise.resolve();
// create the mock notifs from cordova plugin
const mockNotifs = [{ trigger: { at: DateTime.now().toMillis() } }];
// create the expected result
const expectedResult = [
{
key: DateTime.fromMillis(mockNotifs[0].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[0].trigger.at).toFormat('t'),
},
];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
// call the function
const scheduledNotifs = await getScheduledNotifs(isScheduling, Promise.resolve());

expect(scheduledNotifs).toEqual(expectedResult);
});

it('should resolve with notifications if actively scheduling', async () => {
// getScheduledNotifs arguments
const isScheduling = true;
const scheduledPromise = Promise.resolve();
// create the mock notifs from cordova plugin
const mockNotifs = [{ trigger: { at: DateTime.now().toMillis() } }];
// create the expected result
const expectedResult = [
{
key: DateTime.fromMillis(mockNotifs[0].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[0].trigger.at).toFormat('t'),
},
];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
// call the funciton
const scheduledNotifs = await getScheduledNotifs(isScheduling, scheduledPromise);

expect(scheduledNotifs).toEqual(expectedResult);
});

it('should handle case where no notifications are present', async () => {
// getScheduledNotifs arguments
const isScheduling = false;
const scheduledPromise = Promise.resolve();
// create the mock notifs from cordova plugin
const mockNotifs = [];
// create the expected result
const expectedResult = [];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
// call the funciton
const scheduledNotifs = await getScheduledNotifs(isScheduling, Promise.resolve());

expect(scheduledNotifs).toEqual(expectedResult);
});

it('should handle the case where greater than 5 notifications are present', async () => {
// getScheduledNotifs arguments
const isScheduling = false;
const scheduledPromise = Promise.resolve();
// create the mock notifs from cordova plugin (greater than 5 notifications)
const mockNotifs = [
{ trigger: { at: DateTime.now().toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 1 }).toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 2 }).toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 3 }).toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 4 }).toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 5 }).toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 6 }).toMillis() } },
{ trigger: { at: DateTime.now().plus({ weeks: 7 }).toMillis() } },
];
// create the expected result (only the first 5 notifications)
const expectedResult = [
{
key: DateTime.fromMillis(mockNotifs[0].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[0].trigger.at).toFormat('t'),
},
{
key: DateTime.fromMillis(mockNotifs[1].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[1].trigger.at).toFormat('t'),
},
{
key: DateTime.fromMillis(mockNotifs[2].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[2].trigger.at).toFormat('t'),
},
{
key: DateTime.fromMillis(mockNotifs[3].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[3].trigger.at).toFormat('t'),
},
{
key: DateTime.fromMillis(mockNotifs[4].trigger.at).toFormat('DDD'),
val: DateTime.fromMillis(mockNotifs[4].trigger.at).toFormat('t'),
},
];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
// call the funciton
const scheduledNotifs = await getScheduledNotifs(isScheduling, Promise.resolve());

expect(scheduledNotifs).toEqual(expectedResult);
});
});

describe('updateScheduledNotifs', () => {
afterEach(() => {
jest.restoreAllMocks(); // Restore mocked functions after each test
});

it('should resolve after scheduling notifications', async () => {
// updateScheduleNotifs arguments
const reminderSchemes: any = exampleReminderSchemes;
let isScheduling: boolean = false;
const setIsScheduling: Function = jest.fn((val: boolean) => (isScheduling = val));
const scheduledPromise: Promise<any> = Promise.resolve();
// create an empty array of mock notifs from cordova plugin
const mockNotifs = [];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
jest
.spyOn(window['cordova'].plugins.notification.local, 'cancelAll')
.mockImplementation((callback) => callback());
jest
.spyOn(window['cordova'].plugins.notification.local, 'schedule')
.mockImplementation((arg, callback) => callback(arg));
// call the function
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sebastianbarry sebastianbarry Dec 15, 2023

Choose a reason for hiding this comment

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

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

Great suggestion, thanks!

});

it('should resolve without scheduling if notifications are already scheduled', async () => {
// updateScheduleNotifs arguments
const reminderSchemes: any = exampleReminderSchemes;
let isScheduling: boolean = false;
const setIsScheduling: Function = jest.fn((val: boolean) => (isScheduling = val));
const scheduledPromise: Promise<any> = Promise.resolve();
// create the mock notifs from cordova plugin (must match the notifs that will generate from the reminder scheme above...
// in this case: exampleReminderSchemes.weekly, because getUser is mocked to return reminder_assignment: 'weekly')
const mockNotifs = [
{ trigger: { at: DateTime.fromFormat('2023-11-14 21:00', 'yyyy-MM-dd HH:mm').toMillis() } },
{ trigger: { at: DateTime.fromFormat('2023-11-15 21:00', 'yyyy-MM-dd HH:mm').toMillis() } },
{ trigger: { at: DateTime.fromFormat('2023-11-17 21:00', 'yyyy-MM-dd HH:mm').toMillis() } },
{ trigger: { at: DateTime.fromFormat('2023-11-19 21:00', 'yyyy-MM-dd HH:mm').toMillis() } },
];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
// call the function
await updateScheduledNotifs(reminderSchemes, isScheduling, setIsScheduling, scheduledPromise);

expect(logDebug).toHaveBeenCalledWith('Already scheduled, not scheduling again');
});

it('should wait for the previous scheduling to finish if isScheduling is true', async () => {
// updateScheduleNotifs 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 an empty array of mock notifs from cordova plugin
const mockNotifs = [];

// mock the cordova plugin
jest
.spyOn(window['cordova'].plugins.notification.local, 'getScheduled')
.mockImplementation((callback) => callback(mockNotifs));
// call the function
await updateScheduledNotifs(reminderSchemes, isScheduling, setIsScheduling, scheduledPromise);

expect(logDebug).toHaveBeenCalledWith(
'ERROR: Already scheduling notifications, not scheduling again',
);
Comment on lines +320 to +322
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

});

it('should log an error message if the reminder scheme is missing', async () => {
// updateScheduleNotifs arguments
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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is fine too

Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines +327 to +335
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

});
});

describe('getReminderPrefs', () => {
it('should resolve with reminder prefs when user exists', 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',
};

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

expect(reminder_assignment).toEqual(expectedResult.reminder_assignment);
expect(reminder_join_date).toEqual(expectedResult.reminder_join_date);
expect(reminder_time_of_day).toEqual(expectedResult.reminder_time_of_day);
});
});
1 change: 0 additions & 1 deletion www/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import 'leaflet/dist/leaflet.css';
import './js/ngApp.js';
import './js/splash/referral.js';
import './js/splash/localnotify.js';
import './js/splash/notifScheduler.js';
import './js/controllers.js';
import './js/services.js';
import './js/i18n-utils.js';
Expand Down
Loading
Loading