From 58bb3cb6ff2e8bfadb9f628a5b626b5f66f08451 Mon Sep 17 00:00:00 2001 From: pavan bhat Date: Wed, 14 Aug 2024 03:58:02 +0530 Subject: [PATCH] Refactor cronjobs for sync APIs (#64) * fixed and refactored * worte test cases for fireAndForgetApiCaller * renamed functions * refactor * change cron from 11 to 12hrs and 20min to 30min --------- Co-authored-by: Mehul Kiran Chaudhari <55375534+MehulKChaudhari@users.noreply.github.com> Co-authored-by: Yash Raj <56453897+yesyash@users.noreply.github.com> --- src/handlers/scheduledEventHandler.ts | 62 +++------ src/tests/handlers/missedRoleHandler.test.ts | 16 +-- .../handlers/scheduledEventHandler.test.ts | 124 +++--------------- src/tests/utils/apiCaller.test.ts | 60 ++++++++- src/utils/apiCaller.ts | 49 ++++--- src/worker.ts | 36 +---- wrangler.toml | 2 +- 7 files changed, 143 insertions(+), 206 deletions(-) diff --git a/src/handlers/scheduledEventHandler.ts b/src/handlers/scheduledEventHandler.ts index 5dad138..7cdd706 100644 --- a/src/handlers/scheduledEventHandler.ts +++ b/src/handlers/scheduledEventHandler.ts @@ -4,8 +4,8 @@ import config from '../config/config'; import { NAMESPACE_NAME } from '../constants'; import { updateUserRoles } from '../services/discordBotServices'; import { getMissedUpdatesUsers } from '../services/rdsBackendService'; -import { DiscordUserRole, env, NicknameUpdateResponseType, UserStatusResponse } from '../types/global.types'; -import { apiCaller } from '../utils/apiCaller'; +import { DiscordUserRole, env, NicknameUpdateResponseType } from '../types/global.types'; +import { fireAndForgetApiCall } from '../utils/apiCaller'; import { chunks } from '../utils/arrayUtils'; import { generateJwt } from '../utils/generateJwt'; @@ -15,7 +15,7 @@ export async function ping(env: env) { return response; } -export async function callDiscordNicknameBatchUpdate(env: env) { +export async function callDiscordNicknameBatchUpdateHandler(env: env) { const namespace = env[NAMESPACE_NAME] as unknown as KVNamespace; let lastNicknameUpdate: string | null = '0'; try { @@ -64,7 +64,7 @@ export async function callDiscordNicknameBatchUpdate(env: env) { return data; } -export const addMissedUpdatesRole = async (env: env) => { +export const addMissedUpdatesRoleHandler = async (env: env) => { const MAX_ROLE_UPDATE = 25; try { let cursor: string | undefined = undefined; @@ -96,48 +96,22 @@ export const addMissedUpdatesRole = async (env: env) => { } }; -export const syncUsersStatus = async (env: env): Promise => { - await apiCaller(env, 'users/status/update', 'PATCH'); +export const syncApiHandler = async (env: env) => { + const handlers = [ + fireAndForgetApiCall(env, 'users/status/sync', 'PATCH'), + fireAndForgetApiCall(env, 'external-accounts/users?action=discord-users-sync', 'POST'), + fireAndForgetApiCall(env, 'users', 'POST'), + fireAndForgetApiCall(env, 'discord-actions/nicknames/sync?dev=true', 'POST'), + fireAndForgetApiCall(env, 'discord-actions/group-idle-7d', 'PUT'), + fireAndForgetApiCall(env, 'discord-actions/group-onboarding-31d-plus', 'PUT'), + ]; try { - const idleUsersData = (await apiCaller(env, 'users/status?aggregate=true', 'GET')) as UserStatusResponse | undefined; - - if (!idleUsersData?.data?.users || idleUsersData.data.users.length === 0) { - console.error('Error: Users data is not in the expected format or no users found'); - return null; - } - - const response = await apiCaller(env, 'users/status/batch', 'PATCH', { - body: JSON.stringify({ users: idleUsersData.data.users }), - }); - - return response; + await Promise.all(handlers); + console.log( + `Worker for syncing idle users, nicknames, idle 7d users, and onboarding 31d+ users has completed. Worker for syncing user status, external accounts, and unverified users has completed.`, + ); } catch (error) { - console.error('Error during syncUsersStatus:', error); - return null; + console.error('Error occurred during Sync API calls:', error); } }; - -export const syncExternalAccounts = async (env: env) => { - return await apiCaller(env, 'external-accounts/users?action=discord-users-sync', 'POST'); -}; - -export const syncUnverifiedUsers = async (env: env) => { - return await apiCaller(env, 'users', 'POST'); -}; - -export const syncIdleUsers = async (env: env) => { - return await apiCaller(env, 'discord-actions/group-idle', 'PUT'); -}; - -export const syncNickNames = async (env: env) => { - return await apiCaller(env, 'discord-actions/nicknames/sync?dev=true', 'POST'); -}; - -export const syncIdle7dUsers = async (env: env) => { - return await apiCaller(env, 'discord-actions/group-idle-7d', 'PUT'); -}; - -export const syncOnboarding31dPlusUsers = async (env: env) => { - return await apiCaller(env, 'discord-actions/group-onboarding-31d-plus', 'PUT'); -}; diff --git a/src/tests/handlers/missedRoleHandler.test.ts b/src/tests/handlers/missedRoleHandler.test.ts index 0052c37..c8d6fb4 100644 --- a/src/tests/handlers/missedRoleHandler.test.ts +++ b/src/tests/handlers/missedRoleHandler.test.ts @@ -1,4 +1,4 @@ -import { addMissedUpdatesRole } from '../../handlers/scheduledEventHandler'; +import { addMissedUpdatesRoleHandler } from '../../handlers/scheduledEventHandler'; import { updateUserRoles } from '../../services/discordBotServices'; import { getMissedUpdatesUsers } from '../../services/rdsBackendService'; import { @@ -13,7 +13,7 @@ jest.mock('.../../../../services/rdsBackendService', () => ({ jest.mock('.../../../../services/discordBotServices', () => ({ updateUserRoles: jest.fn(), })); -describe('addMissedUpdatesRole', () => { +describe('addMissedUpdatesRoleHandler', () => { beforeEach(() => { jest.resetAllMocks(); }); @@ -25,7 +25,7 @@ describe('addMissedUpdatesRole', () => { (getMissedUpdatesUsers as jest.Mock) .mockResolvedValueOnce(missedUpdatesUsersMock) .mockResolvedValueOnce(missedUpdatesUsersMockWithoutCursor); - await addMissedUpdatesRole({}); + await addMissedUpdatesRoleHandler({}); expect(getMissedUpdatesUsers).toHaveBeenCalledTimes(2); expect(updateUserRoles).toHaveBeenCalledTimes(2); }); @@ -34,7 +34,7 @@ describe('addMissedUpdatesRole', () => { const usersMockData = { ...missedUpdatesUsersMockWithoutCursor }; usersMockData.usersToAddRole = usersMockData.usersToAddRole.slice(0, 1); (getMissedUpdatesUsers as jest.Mock).mockResolvedValueOnce(usersMockData); - await addMissedUpdatesRole({}); + await addMissedUpdatesRoleHandler({}); expect(getMissedUpdatesUsers).toHaveBeenCalledTimes(1); expect(updateUserRoles).toHaveBeenCalledTimes(1); }); @@ -42,7 +42,7 @@ describe('addMissedUpdatesRole', () => { it('should not call updateUserRoles when there are no users to add role', async () => { (getMissedUpdatesUsers as jest.Mock).mockResolvedValueOnce(missedUpdatesUsersMockWithNoUsers); - await addMissedUpdatesRole({}); + await addMissedUpdatesRoleHandler({}); expect(getMissedUpdatesUsers).toHaveBeenCalledTimes(1); expect(updateUserRoles).toHaveBeenCalledTimes(0); }); @@ -51,7 +51,7 @@ describe('addMissedUpdatesRole', () => { const mockValue: any = { ...missedUpdatesUsersMockWithoutCursor, usersToAddRole: new Array(75).fill('id') }; (getMissedUpdatesUsers as jest.Mock).mockResolvedValueOnce(mockValue); - await addMissedUpdatesRole({}); + await addMissedUpdatesRoleHandler({}); expect(getMissedUpdatesUsers).toHaveBeenCalledTimes(1); expect(updateUserRoles).toHaveBeenCalledTimes(3); }); @@ -59,7 +59,7 @@ describe('addMissedUpdatesRole', () => { it('should handle errors', async () => { (getMissedUpdatesUsers as jest.Mock).mockRejectedValueOnce(new Error('Error fetching missed updates users')); const consoleSpy = jest.spyOn(console, 'error'); - await addMissedUpdatesRole({}); + await addMissedUpdatesRoleHandler({}); expect(consoleSpy).toHaveBeenCalledWith('Error while adding missed updates roles'); }); @@ -68,7 +68,7 @@ describe('addMissedUpdatesRole', () => { const consoleSpy = jest.spyOn(console, 'error'); const mockValue: any = { ...missedUpdatesUsersMockWithoutCursor, usersToAddRole: new Array(75).fill('id') }; (getMissedUpdatesUsers as jest.Mock).mockResolvedValueOnce(mockValue); - await addMissedUpdatesRole({}); + await addMissedUpdatesRoleHandler({}); expect(getMissedUpdatesUsers).toHaveBeenCalledTimes(1); expect(consoleSpy).toHaveBeenCalledTimes(1); expect(updateUserRoles).toHaveBeenCalledTimes(3); diff --git a/src/tests/handlers/scheduledEventHandler.test.ts b/src/tests/handlers/scheduledEventHandler.test.ts index 1136bda..426f780 100644 --- a/src/tests/handlers/scheduledEventHandler.test.ts +++ b/src/tests/handlers/scheduledEventHandler.test.ts @@ -1,97 +1,19 @@ -import { - syncExternalAccounts, - syncIdle7dUsers, - syncIdleUsers, - syncNickNames, - syncOnboarding31dPlusUsers, - syncUnverifiedUsers, - syncUsersStatus, -} from '../../handlers/scheduledEventHandler'; +import { syncApiHandler } from '../../handlers/scheduledEventHandler'; import { env } from '../../types/global.types'; import * as apiCallerModule from '../../utils/apiCaller'; -jest.mock('../../utils/apiCaller', () => ({ - apiCaller: jest.fn(), -})); - const consoleErrorMock: jest.SpyInstance = jest.spyOn(console, 'error').mockImplementation(); -const apiCallerFunction = apiCallerModule.apiCaller; +const fireAndForgetApiCallMock = jest.fn(); beforeEach(() => { jest.clearAllMocks(); + jest.spyOn(apiCallerModule, 'fireAndForgetApiCall').mockImplementation(fireAndForgetApiCallMock); }); afterAll(() => { consoleErrorMock.mockRestore(); }); -describe('syncUsersStatus', () => { - const mockEnv: env = { - CURRENT_ENVIRONMENT: { - RDS_BASE_API_URL: 'default', - }, - }; - - beforeEach(() => { - jest.clearAllMocks(); - }); - - it('should successfully sync users status', async () => { - (apiCallerFunction as jest.Mock).mockResolvedValueOnce(undefined); - (apiCallerFunction as jest.Mock).mockResolvedValueOnce({ - data: { - users: [{ userId: 'asdoiuahow212' }], - }, - }); - (apiCallerFunction as jest.Mock).mockResolvedValueOnce({ success: true }); - - await syncUsersStatus(mockEnv); - - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status/update', 'PATCH'); - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status?aggregate=true', 'GET'); - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status/batch', 'PATCH', { - body: JSON.stringify({ users: [{ userId: 'asdoiuahow212' }] }), - }); - expect(apiCallerFunction).toHaveBeenCalledTimes(3); - }); - - it('should handle error during users data retrieval', async () => { - (apiCallerFunction as jest.Mock).mockResolvedValueOnce(undefined); - (apiCallerFunction as jest.Mock).mockRejectedValueOnce(new Error('Error fetching users data')); - - const result = await syncUsersStatus(mockEnv); - - expect(result).toBeNull(); - - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status/update', 'PATCH'); - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status?aggregate=true', 'GET'); - expect(apiCallerFunction).toHaveBeenCalledTimes(2); - - expect(console.error).toHaveBeenCalledWith('Error during syncUsersStatus:', new Error('Error fetching users data')); - expect(console.error).toHaveBeenCalledTimes(1); - }); - - it('should log an error when no users are found or data is not in the expected format', async () => { - (apiCallerFunction as jest.Mock).mockResolvedValueOnce(undefined); - (apiCallerFunction as jest.Mock).mockResolvedValueOnce({ - data: { - users: [], - }, - }); - - const result = await syncUsersStatus(mockEnv); - - expect(result).toBeNull(); - - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status/update', 'PATCH'); - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, 'users/status?aggregate=true', 'GET'); - expect(apiCallerFunction).toHaveBeenCalledTimes(2); - - expect(console.error).toHaveBeenCalledWith('Error: Users data is not in the expected format or no users found'); - expect(console.error).toHaveBeenCalledTimes(1); - }); -}); - describe('sync apis', () => { const mockEnv: env = { CURRENT_ENVIRONMENT: { @@ -99,34 +21,26 @@ describe('sync apis', () => { }, }; - const testSyncFunction = async (syncFunction: Function, endpoint: string, method: string) => { - await syncFunction(mockEnv); + it('should call all sync functions', async () => { + await syncApiHandler(mockEnv); - expect(apiCallerFunction).toHaveBeenCalledWith(mockEnv, endpoint, method); - expect(apiCallerFunction).toHaveBeenCalledTimes(1); - }; - - it('should sync unverified users', async () => { - await testSyncFunction(syncUnverifiedUsers, 'users', 'POST'); - }); - - it('should sync idle users', async () => { - await testSyncFunction(syncIdleUsers, 'discord-actions/group-idle', 'PUT'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledWith(mockEnv, 'users/status/sync', 'PATCH'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledWith(mockEnv, 'external-accounts/users?action=discord-users-sync', 'POST'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledWith(mockEnv, 'users', 'POST'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledWith(mockEnv, 'discord-actions/nicknames/sync?dev=true', 'POST'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledWith(mockEnv, 'discord-actions/group-idle-7d', 'PUT'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledWith(mockEnv, 'discord-actions/group-onboarding-31d-plus', 'PUT'); + expect(apiCallerModule.fireAndForgetApiCall).toHaveBeenCalledTimes(6); }); - it('should sync external accounts', async () => { - await testSyncFunction(syncExternalAccounts, 'external-accounts/users?action=discord-users-sync', 'POST'); - }); - - it('should sync nicknames', async () => { - await testSyncFunction(syncNickNames, 'discord-actions/nicknames/sync?dev=true', 'POST'); - }); + it('should catch errors during API calls', async () => { + const mockError = new Error('API error'); + (apiCallerModule.fireAndForgetApiCall as jest.MockedFunction).mockRejectedValueOnce( + mockError, + ); - it('should sync idle 7d users', async () => { - await testSyncFunction(syncIdle7dUsers, 'discord-actions/group-idle-7d', 'PUT'); - }); + await syncApiHandler(mockEnv); - it('should sync onboarding 31d+ users', async () => { - await testSyncFunction(syncOnboarding31dPlusUsers, 'discord-actions/group-onboarding-31d-plus', 'PUT'); + expect(console.error).toHaveBeenCalledWith('Error occurred during Sync API calls:', mockError); }); }); diff --git a/src/tests/utils/apiCaller.test.ts b/src/tests/utils/apiCaller.test.ts index 7ccb1b6..a1b950b 100644 --- a/src/tests/utils/apiCaller.test.ts +++ b/src/tests/utils/apiCaller.test.ts @@ -1,6 +1,6 @@ import { RDS_BASE_DEVELOPMENT_API_URL } from '../../constants/urls'; import { env } from '../../types/global.types'; -import { apiCaller } from '../../utils/apiCaller'; +import { apiCaller, fireAndForgetApiCall } from '../../utils/apiCaller'; import { generateJwt } from '../../utils/generateJwt'; import * as generateJwtModule from '../../utils/generateJwt'; @@ -76,3 +76,61 @@ describe('apiCaller', () => { await expect(apiCaller(mockEnv, 'someEndpoint', 'GET')).rejects.toThrow('Generate JWT error'); }); }); + +describe('fireAndForgetApiCall', () => { + const mockEnv: env = { + CURRENT_ENVIRONMENT: { + RDS_BASE_API_URL: 'default', + }, + }; + + beforeEach(() => { + jest.clearAllMocks(); + (globalThis as any).fetch = jest.fn().mockResolvedValue({ + ok: true, + }); + }); + + it('should make a fire and forget API call', async () => { + await fireAndForgetApiCall(mockEnv, 'users', 'GET'); + + expect((globalThis as any).fetch).toHaveBeenCalledWith(`${RDS_BASE_DEVELOPMENT_API_URL}/users`, { + method: 'GET', + headers: { + Authorization: 'Bearer mocked-token', + 'Content-Type': 'application/json', + }, + }); + }); + + it('should make a fire and forget POST API call', async () => { + await fireAndForgetApiCall(mockEnv, 'test', 'POST', { + body: JSON.stringify({ data: 'example' }), + }); + + expect((globalThis as any).fetch).toHaveBeenCalledWith(`${RDS_BASE_DEVELOPMENT_API_URL}/test`, { + method: 'POST', + headers: { + Authorization: 'Bearer mocked-token', + 'Content-Type': 'application/json', + }, + body: JSON.stringify({ data: 'example' }), + }); + }); + + it('should log and rethrow error during fetch call failure', async () => { + const mockError = new Error('Network error'); + + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + (globalThis as any).fetch = jest.fn().mockImplementation(() => { + throw mockError; + }); + + await expect(fireAndForgetApiCall({}, 'someEndpoint', 'GET')).rejects.toThrowError(mockError); + expect(consoleErrorSpy).toHaveBeenCalledWith(`Error during fire and forget API call: ${mockError}`); + expect(consoleErrorSpy).toHaveBeenCalledTimes(1); + + consoleErrorSpy.mockRestore(); + }); +}); diff --git a/src/utils/apiCaller.ts b/src/utils/apiCaller.ts index 79f6474..9e14ecd 100644 --- a/src/utils/apiCaller.ts +++ b/src/utils/apiCaller.ts @@ -2,34 +2,49 @@ import config from '../config/config'; import { env } from '../types/global.types'; import { generateJwt } from './generateJwt'; -export const apiCaller = async ( +const createOptions = async (env: env): Promise> => { + try { + const token = await generateJwt(env); + return { + headers: { + Authorization: `Bearer ${token}`, + 'Content-Type': 'application/json', + }, + }; + } catch (error) { + console.error(`Error while creating options: ${error}`); + throw error; + } +}; + +export const fireAndForgetApiCall = async ( env: env, endpoint: string, method: string, options?: Record, -): Promise> => { +): Promise => { const url = config(env).RDS_BASE_API_URL; - let token; try { - token = await generateJwt(env); - } catch (err) { - console.error(`Error while generating JWT token: ${err}`); - throw err; + const requestOptions = await createOptions(env); + return fetch(`${url}/${endpoint}`, { method, ...requestOptions, ...options }); + } catch (error) { + console.error(`Error during fire and forget API call: ${error}`); + throw error; } +}; - const defaultOptions = { - method, - headers: { - Authorization: `Bearer ${token}`, - 'Content-Type': 'application/json', - }, - }; - +export const apiCaller = async ( + env: env, + endpoint: string, + method: string, + options?: Record, +): Promise> => { + const url = config(env).RDS_BASE_API_URL; try { - const response = await fetch(`${url}/${endpoint}`, { ...defaultOptions, ...options }); + const requestOptions = await createOptions(env); + const response = await fetch(`${url}/${endpoint}`, { method, ...requestOptions, ...options }); return await response.json(); } catch (error) { - // TODO: Handle these errors: log to newRelic or any other better approach console.error(`Error during fetch operation: ${error}`); throw error; } diff --git a/src/worker.ts b/src/worker.ts index 38f5c0a..a4a6715 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -1,45 +1,21 @@ -import { - addMissedUpdatesRole, - callDiscordNicknameBatchUpdate, - syncExternalAccounts, - syncIdle7dUsers, - syncIdleUsers, - syncOnboarding31dPlusUsers, - syncUnverifiedUsers, - syncUsersStatus, -} from './handlers/scheduledEventHandler'; +import { addMissedUpdatesRoleHandler, callDiscordNicknameBatchUpdateHandler, syncApiHandler } from './handlers/scheduledEventHandler'; import { env } from './types/global.types'; -const EVERY_6_HOURS = '0 */6 * * *'; -const EVERY_11_HOURS = '0 */11 * * *'; -const EVERY_20_MINUTES = '*/20 * * * *'; +const EVERY_12_HOURS = '0 */12 * * *'; const EVERY_30_MINUTES = '*/30 * * * *'; export default { // eslint-disable-next-line no-unused-vars async scheduled(req: ScheduledController, env: env, ctx: ExecutionContext) { switch (req.cron) { - case EVERY_6_HOURS: { - return await callDiscordNicknameBatchUpdate(env); - } - case EVERY_11_HOURS: { - return await addMissedUpdatesRole(env); - } - - case EVERY_20_MINUTES: { - await syncIdleUsers(env); - // await syncNickNames(env); TODO: Enable it once changes from website-backend is merged - await syncIdle7dUsers(env); - await syncOnboarding31dPlusUsers(env); - console.log('Worker for syncing idle users, nicknames, idle 7d users, and onboarding 31d+ users has completed.'); + case EVERY_12_HOURS: { + await callDiscordNicknameBatchUpdateHandler(env); + await addMissedUpdatesRoleHandler(env); break; } case EVERY_30_MINUTES: { - await syncUsersStatus(env); - await syncExternalAccounts(env); - await syncUnverifiedUsers(env); - console.log('Worker for syncing user status, external accounts, and unverified users has completed.'); + await syncApiHandler(env); break; } diff --git a/wrangler.toml b/wrangler.toml index dc25df9..3470ea5 100644 --- a/wrangler.toml +++ b/wrangler.toml @@ -22,7 +22,7 @@ services = [ ] [triggers] -crons = ["0 */6 * * *","0 */11 * * *","*/20 * * * *","*/30 * * * *" ] +crons = ["0 */11 * * *","*/20 * * * *" ] # # Durable Object binding - For more information: https://developers.cloudflare.com/workers/runtime-apis/durable-objects # [[durable_objects]]