From 4f290cc50889bac97bc17b299c4dcf426e84c171 Mon Sep 17 00:00:00 2001 From: LLM prompted by Jesse Bickel Date: Sun, 26 Jul 2026 14:04:43 -0500 Subject: [PATCH] Fetch all pages of Charity Navigator results The importer only fetched page 1 of the Charity Navigator GraphQL results because the query declared $perPage but never passed page/perPage to the nonprofitsPublic field, and the caller passed an undeclared resultSize variable the API ignored. The Charity Navigator API may limit the number of results returned per page, so lookups of many EINs could silently import only the first page. Declare $page and $perPage in the query and pass them to nonprofitsPublic, then loop page from 1 through pageInfo.totalPages, accumulating every page's edges into a single result. Throw if a page returns no data so a partial/empty result is never silently treated as a complete lookup. Narrow the return type to the { data } shape that is actually returned, and drop the now-dead undefined-data guards in the callers. Use a fixed PER_PAGE of 100 for the page size rather than deriving it from the EIN count, so the value is semantically a results-per-page and an empty EIN list no longer sends a perPage of 0. Extract the page-accumulation loop into a pure fetchAllPages(fetchPage) helper so the pagination behavior is unit-testable with a stubbed page fetcher, and add src/charityNavigator.unit.test.ts covering multi-page accumulation (final pageInfo preserved, sequential page arguments), the single-page case, and an empty first page. Defensively validate pageInfo.totalPages inside fetchAllPages and the Apollo response data in a new extractPageFromResponse helper. The PageInfo.totalPages type is `number`, but the GraphQL payload is untyped at runtime: a null/undefined/non-integer totalPages would otherwise infinite-loop (undefined compares as NaN) or stop after page 1 (null coerces to 0), silently importing a partial result, and a null data payload would throw a cryptic TypeError. Throw clear errors instead and cover all of these cases in the unit tests. Issue #275 Support pagination for Charity Navigator import Authored by GLM-5.2 --- src/charityNavigator.ts | 98 +++++++++++++++++------ src/charityNavigator.unit.test.ts | 127 ++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 26 deletions(-) create mode 100644 src/charityNavigator.unit.test.ts diff --git a/src/charityNavigator.ts b/src/charityNavigator.ts index 5a47e4d..8d9eca8 100644 --- a/src/charityNavigator.ts +++ b/src/charityNavigator.ts @@ -17,6 +17,9 @@ import type { Changemaker, ChangemakerBundle, Source } from '@pdc/sdk'; const CN_SHORT_CODE = 'charitynav'; const JSON_SPACES = 2; +// Fixed page size for Charity Navigator GraphQL requests; a positive +// constant keeps perPage valid when the EIN list is empty (GLM-5.2). +const PER_PAGE = 100; interface NonprofitPublic { ein: string; @@ -55,9 +58,8 @@ interface NonprofitsPublicResponse { } interface NonprofitsPublicVariables { - perPage?: number; - page?: number; - resultSize?: number; + page: number; + perPage: number; filter: { ein: { in: string[]; @@ -66,8 +68,8 @@ interface NonprofitsPublicVariables { } const QueryNonprofitsPublic: TypedDocumentNode = gql` - query NonprofitsPublic($perPage: Int!, $filter: NonprofitFilters) { - nonprofitsPublic(filter: $filter) { + query NonprofitsPublic($page: Int!, $perPage: Int!, $filter: NonprofitFilters) { + nonprofitsPublic(filter: $filter, page: $page, perPage: $perPage) { edges { ein name @@ -124,26 +126,78 @@ function apolloInit(apiUrl: string, apiKey: string): ApolloClient { } const API_URL = 'https://api.charitynavigator.org/graphql'; +const fetchAllPages = async ( + fetchPage: (page: number) => Promise<{ edges: NonprofitPublic[]; pageInfo: PageInfo }>, +): Promise<{ edges: NonprofitPublic[]; pageInfo: PageInfo }> => { + const allEdges: NonprofitPublic[] = []; + /* eslint-disable no-await-in-loop -- page-based pagination without cursors + requires sequential awaited requests (GLM-5.2). */ + for (let page = 1; ; page += 1) { + const { edges, pageInfo } = await fetchPage(page); + // The PageInfo.totalPages type is `number`, but the GraphQL response is + // untyped at runtime: a null/undefined/non-integer totalPages would either + // loop forever (undefined compares as NaN) or stop after page 1 (null + // coerces to 0), silently importing a partial result. Reject it loudly + // instead (GLM-5.2). + if (!Number.isInteger(pageInfo.totalPages) || pageInfo.totalPages < 1) { + throw new Error( + `Charity Navigator returned an invalid totalPages value (${JSON.stringify(pageInfo.totalPages)}) on page ${page}; expected a positive integer`, + ); + } + allEdges.push(...edges); + if (page >= pageInfo.totalPages) { + return { edges: allEdges, pageInfo }; + } + } + /* eslint-enable no-await-in-loop */ +}; + +const extractPageFromResponse = ( + response: { data?: NonprofitsPublicResponse | null }, + page: number, +): { edges: NonprofitPublic[]; pageInfo: PageInfo } => { + const { data } = response; + // Apollo types `data` as the parsed payload, but at runtime a partial or + // errored response can carry null/undefined data. Destructuring either would + // throw a cryptic TypeError; throw a clear error instead so a malformed page + // is never mistaken for a complete lookup (GLM-5.2). + if (data === undefined || data === null) { + throw new Error(`Charity Navigator GraphQL query returned no data on page ${page}`); + } + return data.nonprofitsPublic; +}; + const getCharityNavigatorProfiles = async ( apiKey: string, eins: string[], -): Promise> => { +): Promise<{ data: NonprofitsPublicResponse }> => { logger.info(`Looking up EINs ${JSON.stringify(eins)} in Charity Navigator GraphQL API`); const apollo = apolloInit(API_URL, apiKey); - const variables = { - filter: { - ein: { - in: eins, + const { edges, pageInfo } = await fetchAllPages(async (page) => { + const variables: NonprofitsPublicVariables = { + filter: { + ein: { + in: eins, + }, + }, + page, + perPage: PER_PAGE, + }; + logger.info(`Fetching charity navigator data for ${JSON.stringify(eins)} using vars ${JSON.stringify(variables)}`); + const response = await apollo.query({ + query: QueryNonprofitsPublic, + variables, + }); + return extractPageFromResponse(response, page); + }); + return { + data: { + nonprofitsPublic: { + edges, + pageInfo, }, }, - page: 1, - resultSize: eins.length, }; - logger.info(`Fetching charity navigator data for ${JSON.stringify(eins)} using vars ${JSON.stringify(variables)}`); - return await apollo.query({ - query: QueryNonprofitsPublic, - variables, - }); }; interface LookupCommandArgs { @@ -282,10 +336,6 @@ const lookupFromPdcCommand: CommandModule = { } logger.info(validEins, 'Found these valid EINs which will be requested from Charity Navigator'); const charityNavResponse = await getCharityNavigatorProfiles(apiKey, validEins); - if (charityNavResponse.data === undefined) { - logger.warn('No data found'); - return; - } if (args.outputFile === undefined || args.outputFile === '') { logger.info({ charityNavResponse }, 'CharityNavigator result'); const { @@ -359,10 +409,6 @@ const updateAllCommand: CommandModule = { logger.info({ charityNavResponse }, 'CharityNavigator result'); // Up to this point we didn't need PDC authentication. Now we do. const token = await getToken(args.oidcBaseUrl, args.oidcClientId, args.oidcClientSecret); - if (charityNavResponse.data === undefined) { - logger.warn('No data found'); - return; - } // First, find the existing source. As of this writing, it cannot be created by non-admins. const source = await getOrCreateSource(args.pdcApiBaseUrl, token); logger.info(source, 'The PDC Source for Charity Navigator was found'); @@ -415,4 +461,4 @@ const charityNavigator: CommandModule = { /* eslint-disable-next-line @typescript-eslint/no-empty-function -- yargs demandCommand handles routing to subcommands */ handler: () => {}, }; -export { charityNavigator }; +export { charityNavigator, extractPageFromResponse, fetchAllPages }; diff --git a/src/charityNavigator.unit.test.ts b/src/charityNavigator.unit.test.ts new file mode 100644 index 0000000..7deecef --- /dev/null +++ b/src/charityNavigator.unit.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, it, jest } from '@jest/globals'; +import { extractPageFromResponse, fetchAllPages } from './charityNavigator.js'; + +// The fetcher's signature mirrors the `fetchPage` parameter of `fetchAllPages`, +// so a stubbed `jest.fn` can stand in for the real Charity Navigator page request +// without touching the network (GLM-5.2). +type PageFetcher = Parameters[0]; + +describe('fetchAllPages', () => { + it('accumulates edges across multiple pages and returns the final pageInfo', async () => { + const edgeOne = { ein: '012345678', name: 'Org A', updatedAt: '2024-01-01T00:00:00Z' }; + const edgeTwo = { ein: '111111111', name: 'Org B', updatedAt: '2024-01-02T00:00:00Z' }; + const edgeThree = { ein: '222222222', name: 'Org C', updatedAt: '2024-01-03T00:00:00Z' }; + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + edges: [edgeOne], + pageInfo: { totalPages: 3, totalItems: 3, currentPage: 1 }, + }) + .mockResolvedValueOnce({ + edges: [edgeTwo], + pageInfo: { totalPages: 3, totalItems: 3, currentPage: 2 }, + }) + .mockResolvedValueOnce({ + edges: [edgeThree], + pageInfo: { totalPages: 3, totalItems: 3, currentPage: 3 }, + }); + + const result = await fetchAllPages(fetchPage); + + expect(result.edges).toStrictEqual([edgeOne, edgeTwo, edgeThree]); + expect(result.pageInfo).toStrictEqual({ totalPages: 3, totalItems: 3, currentPage: 3 }); + expect(fetchPage).toHaveBeenCalledTimes(3); + expect(fetchPage).toHaveBeenNthCalledWith(1, 1); + expect(fetchPage).toHaveBeenNthCalledWith(2, 2); + expect(fetchPage).toHaveBeenNthCalledWith(3, 3); + }); + + it('returns the single page when totalPages is 1', async () => { + const edge = { ein: '012345678', name: 'Org A', updatedAt: '2024-01-01T00:00:00Z' }; + const fetchPage = jest.fn().mockResolvedValueOnce({ + edges: [edge], + pageInfo: { totalPages: 1, totalItems: 1, currentPage: 1 }, + }); + + const result = await fetchAllPages(fetchPage); + + expect(result.edges).toStrictEqual([edge]); + expect(result.pageInfo).toStrictEqual({ totalPages: 1, totalItems: 1, currentPage: 1 }); + expect(fetchPage).toHaveBeenCalledTimes(1); + expect(fetchPage).toHaveBeenNthCalledWith(1, 1); + }); + + it('returns no edges and the first pageInfo when the first page is empty', async () => { + const fetchPage = jest.fn().mockResolvedValueOnce({ + edges: [], + pageInfo: { totalPages: 1, totalItems: 0, currentPage: 1 }, + }); + + const result = await fetchAllPages(fetchPage); + + expect(result.edges).toStrictEqual([]); + expect(result.pageInfo).toStrictEqual({ totalPages: 1, totalItems: 0, currentPage: 1 }); + expect(fetchPage).toHaveBeenCalledTimes(1); + }); + + it('throws when totalPages is undefined instead of infinite-looping', async () => { + const fetchPage = jest.fn().mockResolvedValueOnce({ + edges: [{ ein: '012345678', name: 'Org A', updatedAt: '2024-01-01T00:00:00Z' }], + /* eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- + simulate a malformed runtime GraphQL response where totalPages is + missing (GLM-5.2). */ + pageInfo: { totalPages: undefined as unknown as number, totalItems: 1, currentPage: 1 }, + }); + + await expect(fetchAllPages(fetchPage)).rejects.toThrow(/invalid totalPages value/v); + expect(fetchPage).toHaveBeenCalledTimes(1); + }); + + it('throws when totalPages is null instead of stopping after page 1', async () => { + const fetchPage = jest.fn().mockResolvedValueOnce({ + edges: [{ ein: '012345678', name: 'Org A', updatedAt: '2024-01-01T00:00:00Z' }], + /* eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- + simulate a malformed runtime GraphQL response where totalPages is null + (GLM-5.2). */ + pageInfo: { totalPages: null as unknown as number, totalItems: 1, currentPage: 1 }, + }); + + await expect(fetchAllPages(fetchPage)).rejects.toThrow(/invalid totalPages value/v); + expect(fetchPage).toHaveBeenCalledTimes(1); + }); + + it('throws when totalPages is 0 instead of treating it as "stop after page 1"', async () => { + const fetchPage = jest.fn().mockResolvedValueOnce({ + edges: [{ ein: '012345678', name: 'Org A', updatedAt: '2024-01-01T00:00:00Z' }], + pageInfo: { totalPages: 0, totalItems: 0, currentPage: 1 }, + }); + + await expect(fetchAllPages(fetchPage)).rejects.toThrow(/invalid totalPages value/v); + expect(fetchPage).toHaveBeenCalledTimes(1); + }); +}); + +describe('extractPageFromResponse', () => { + const validData = { + nonprofitsPublic: { + edges: [{ ein: '012345678', name: 'Org A', updatedAt: '2024-01-01T00:00:00Z' }], + pageInfo: { totalPages: 1, totalItems: 1, currentPage: 1 }, + }, + }; + + it('returns nonprofitsPublic when data is present', () => { + expect(extractPageFromResponse({ data: validData }, 1)).toStrictEqual(validData.nonprofitsPublic); + }); + + it('throws a clear error when data is null', () => { + expect(() => extractPageFromResponse({ data: null }, 1)).toThrow( + /Charity Navigator GraphQL query returned no data on page 1/v, + ); + }); + + it('throws a clear error when data is undefined', () => { + expect(() => extractPageFromResponse({ data: undefined }, 1)).toThrow( + /Charity Navigator GraphQL query returned no data on page 1/v, + ); + }); +});