ENH-296 Giving Tuesday, Metrics and Documentation by grant-minor-sntialtech · Pull Request #297 · PhilanthropyDataCommons/data-scripts

grant-minor-sntialtech · 2026-08-14T23:51:20Z

No description provided.

grant-minor-sntialtech · 2026-08-15T00:14:30Z

I believe to correct the prettier stuff I just run:
npm run lint:prettier

Correct?

bickelj · 2026-08-20T18:30:01Z

@grant-minor-sntialtech I don't want to get too pedantic about commit histories but I think we'll want to squash/rebase all the fixes into 1-3 commits before pushing. I asked GLM-5.2 for a review and it usually gives decent feedback.

bickelj · 2026-08-20T18:31:44Z

@grant-minor-sntialtech I think the enormous docs files might be too verbose to maintain. It looks like they were generated as summaries of what can be found in the code, and if they are useful I suppose they should stay. But perhaps they can be condensed to repeat less of what can be easily found when examining the code. I like the little tables showing which commands need PDC auth and which don't, though, that part is very handy.

bickelj-agent · 2026-08-20T18:32:49Z

Reviewed PR #297; npm run lint and the full Jest suite (68 tests) pass locally. Three follow-ups, posted inline: the pdc-bulk-uploader → pdc-metrics default change left the code comment and the docs inconsistent, and src/getMetrics-report.md is referenced but does not exist.

Minor, not commented inline

All three new docs use repo-relative links that resolve wrong from docs/ (e.g. [src/index.ts](src/index.ts) → docs/src/index.ts); consider ../src/.... docs/getMetrics.md line 3 also drops the src/ prefix ([getMetrics.ts](getMetrics.ts)) while the other two docs keep it.
classifyHttpError reports Unexpected HTTP status undefined for transport errors with no response; a distinct note for the no-response case would read more clearly.

— GLM-5.2

grant-minor-sntialtech · 2026-08-20T21:28:01Z

For the comment around:
classifyHttpError reports Unexpected HTTP status undefined for transport errors with no response; a distinct note for the no-response case would read more clearly.

I'm not an expert, but I don't believe there is a status for 'no response'. You still get a header and stuff... Is this to start tracking network timeouts?

bickelj-agent

givingTuesday.ts mirrors large parts of the existing charityNavigator.ts; the three in-band comments cover the clearest copy-pasted helpers, and the handler-level duplication is listed below. — GLM-5.2

Additional duplication not commented in-band

The updateAll handler body (getChangemakers → flatMap taxId → filter valid/invalid → warn → fetch profiles → getToken → getOrCreateSource → postChangemakerFieldValueBatch → sequential postChangemakerFieldValueWarnOnForbidden loop with the missingPermissionChangemakerIds set → final warn) is near-identical to charityNavigator.ts lines 416-489. A shared driver taking { records, getEin, getGoodAsOf, baseFieldMap, shortCode, label, notes } would collapse both.
The lookupFromPdc handlers share the same skeleton (getChangemakers → EIN extraction → validEins/invalidEins filter → warn → fetch → write-or-log).
JSON_SPACES = 2 and HTTP_STATUS_FORBIDDEN = 403 are redefined in charityNavigator.ts, givingTuesday.ts, and getMetrics.ts.

bickelj-agent · 2026-08-24T18:23:20Z

+const postChangemakerFieldValueWarnOnForbidden = async (
+  baseUrl: string,
+  token: AccessTokenSet,
+  data: WritableChangemakerFieldValue,
+  warnedChangemakers: Set<number>, // Mutated! This is for observation/logs, not control!
+): Promise<void> => {
+  try {
+    const fieldValue = await postChangemakerFieldValue(baseUrl, token, data);
+    logger.info(`Added changemaker field value: ${JSON.stringify(fieldValue)}`);
+  } catch (e: unknown) {
+    if (e instanceof AxiosError && e.status === HTTP_STATUS_FORBIDDEN) {
+      logger.warn(`No permission (403) to create ${JSON.stringify(data)}`);
+      warnedChangemakers.add(data.changemakerId);
+    } else {
+      throw e;
+    }
+  }
+};


This function is byte-for-byte identical to charityNavigator.ts (lines 296-313). Extract it to a shared module (e.g. pdc-api.ts) so both givingTuesday and charityNavigator import one copy. — GLM-5.2

My thought was that for a first pass to use 'copy and paste re-use'. Not ideal, but hopefully there are more data sources coming online (I've done initial mappings for techsoup and grantmakers.io so far). I guess my approach was to just get each working individually then merging into a cleaner code base once there was more than a couple.

@grant-minor-sntialtech I'm OK with it as-is for now, you are welcome to rebase-squash then merge, or if you want I can do it.

Oh, and this is an ongoing/general type of thing: usually when we approve that passes the baton back to the person who opened the PR and gives the option to make minor modifications in response to the feedback.

bickelj-agent · 2026-08-24T18:23:20Z

+const getOrCreateSource = async (baseUrl: string, token: AccessTokenSet): Promise<Source> => {
+  const sources = await getSources(baseUrl, token);
+  const filteredSources = sources.entries.filter((s) => s.dataProviderShortCode === GT_SHORT_CODE);
+  if (filteredSources.length === 1 && filteredSources[0] !== undefined) {
+    // Hurray, an existing GivingTuesday Source was found, return it!
+    return filteredSources[0];
+  }
+  // Create the GivingTuesday Source, we expect/require the Data Provider to exist.
+  logger.warn('Have a `pdc-admin` create a source because only administrators may be able.');
+  // The following may not succeed, doesn't succeed as of this writing.
+  return await postSource(baseUrl, token, {
+    dataProviderShortCode: GT_SHORT_CODE,
+    label: 'GivingTuesday',
+  });
+};


This mirrors charityNavigator.ts's getOrCreateSource (lines 383-397); only the short code (GT_SHORT_CODE vs CN_SHORT_CODE) and label differ. A shared getOrCreateSource(baseUrl, token, shortCode, label) would deduplicate it. — GLM-5.2

bickelj

@grant-minor-sntialtech If we already have run with the GT script, I'd be OK with merging as-is and doing a refactoring pass in a separate PR, what do you think?

Looks pretty good.

I still guess the generated docs will get stale, but such is life. If they're useful let's keep them. When something gets stale we could revisit.

Thanks for this! (Oh and squash when merging as you suggested)

bickelj · 2026-09-01T15:24:17Z

@grant-minor-sntialtech Also needs a link back to the issue in the commit message: #296

grant-minor-sntialtech added 4 commits August 14, 2026 18:48

Added Giving Tuesday

dbf9ceb

Added Metrics

a9af451

Added Documents

e283720

Index and packaging updates

985d741

grant-minor-sntialtech requested review from bickelj and bickelj-agent August 14, 2026 23:51

linting cleanups

907c04c

bickelj reviewed Aug 19, 2026

View reviewed changes

Comment thread src/getMetrics.ts Outdated

grant-minor-sntialtech added 2 commits August 19, 2026 22:10

Adjusted DEFAULT_OIDC_CLIENT_ID

2e1a5fb

Added header reporting the API in final report

61730bb

bickelj-agent reviewed Aug 20, 2026

View reviewed changes

Comment thread src/getMetrics.ts Outdated

bickelj-agent reviewed Aug 20, 2026

View reviewed changes

Comment thread docs/getMetrics.md Outdated

grant-minor-sntialtech added 2 commits August 20, 2026 16:32

Cleanup on a number of small items; doc paths, defaults, etc...

6523cea

prettier fix

6e8a684

bickelj-agent reviewed Aug 24, 2026

View reviewed changes

bickelj approved these changes Aug 24, 2026

View reviewed changes

grant-minor-sntialtech linked an issue Sep 1, 2026 that may be closed by this pull request

[Enhancement]: Add Import for Giving Tuesday, a Metric gathering script and documentation for these and Charity Navigator #296

Closed

grant-minor-sntialtech merged commit c79ac74 into main Sep 1, 2026
7 checks passed

grant-minor-sntialtech deleted the ENH-296-GivingTuesday-Metrics-Docs branch September 1, 2026 21:19

Uh oh!

Conversation

grant-minor-sntialtech commented Aug 14, 2026

Uh oh!

grant-minor-sntialtech commented Aug 15, 2026

Uh oh!

Uh oh!

bickelj commented Aug 20, 2026

Uh oh!

bickelj commented Aug 20, 2026

Uh oh!

bickelj-agent commented Aug 20, 2026

Uh oh!

Uh oh!

Uh oh!

grant-minor-sntialtech commented Aug 20, 2026

Uh oh!

bickelj-agent left a comment

Choose a reason for hiding this comment

Uh oh!

bickelj-agent Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

grant-minor-sntialtech Aug 28, 2026

Choose a reason for hiding this comment

Uh oh!

bickelj Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bickelj-agent Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bickelj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bickelj commented Sep 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bickelj Sep 1, 2026 •

edited

Loading

bickelj left a comment •

edited

Loading