ENH-296 Giving Tuesday, Metrics and Documentation - #297
ENH-296 Giving Tuesday, Metrics and Documentation#297grant-minor-sntialtech wants to merge 9 commits into
Conversation
|
I believe to correct the prettier stuff I just run: Correct? |
|
@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. |
|
@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. |
|
Reviewed PR #297; Minor, not commented inline
— GLM-5.2 |
|
For the comment around: 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
left a comment
bickelj-agent
left a comment
There was a problem hiding this comment.
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
updateAllhandler body (getChangemakers→ flatMaptaxId→ filter valid/invalid → warn → fetch profiles →getToken→getOrCreateSource→postChangemakerFieldValueBatch→ sequentialpostChangemakerFieldValueWarnOnForbiddenloop with themissingPermissionChangemakerIdsset → final warn) is near-identical tocharityNavigator.tslines 416-489. A shared driver taking{ records, getEin, getGoodAsOf, baseFieldMap, shortCode, label, notes }would collapse both. - The
lookupFromPdchandlers share the same skeleton (getChangemakers→ EIN extraction →validEins/invalidEinsfilter → warn → fetch → write-or-log). JSON_SPACES = 2andHTTP_STATUS_FORBIDDEN = 403are redefined incharityNavigator.ts,givingTuesday.ts, andgetMetrics.ts.
| 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; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
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
| 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', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
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
| const getChangemakerByEin = (ein: string, changemakers: ChangemakerBundle): Changemaker | null => { | ||
| // Make the comparison with hyphens stripped and zero-padded to match the | ||
| // normalized EIN GivingTuesday echoes back in its records. | ||
| const normalized = toGivingTuesdayEin(ein); | ||
| const matches = changemakers.entries.filter((c) => toGivingTuesdayEin(c.taxId) === normalized); | ||
| if (matches.length > 1) { | ||
| logger.warn(`Found multiple changemakers with EIN ${ein}, not returning any.`); | ||
| return null; | ||
| } | ||
| if (matches.length < 1) { | ||
| logger.info(`Found no changemaker with EIN ${ein}`); | ||
| return null; | ||
| } | ||
| if (matches.length === 1 && matches[0] !== undefined) { | ||
| return matches[0]; | ||
| } | ||
| throw new Error('How could this have happened?'); | ||
| }; |
There was a problem hiding this comment.
Structurally identical to charityNavigator.ts's getChangemakerByEin (lines 278-293); only the EIN normalization differs (toGivingTuesdayEin vs replace('-','')). A shared helper taking a normalize function parameter would deduplicate it. — GLM-5.2
There was a problem hiding this comment.
@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)
3 participants
No description provided.