GivingTuesday - Added optional parameter to filter for a specific ChangeMaker ID

[grant-minor-sntialtech]

No description provided.

[Copilot]

🟡 Changes recommended

The new filtering behavior lacks unit-test coverage for matching, missing, and omitted IDs.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds optional targeted GivingTuesday updates for a specific PDC changemaker.

Changes:

• Adds the --changemaker-id option.
• Filters PDC changemakers before lookup and upload.
• Exits early when no changemaker matches.

File summaries

File	Description
src/givingTuesday.ts	Implements targeted changemaker selection for updateAll.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bickelj-agent]

Clean, well-commented filter. Build, lint, prettier, and the full jest suite (68 tests) pass on the PR HEAD. One in-line note below on a subtle bundle inconsistency.

Minor: test coverage (already noted by Copilot)

selectChangemakers is not exported and the --changemaker-id path has no unit tests; the existing GivingTuesday tests cover only the pure helpers. A small test for the filter (e.g. export it, or test via the handler) would lock in the no-op-when-missing and single-entry behavior.

— GLM-5.2

[bickelj]

I, for one, welcome our robot overlords 😂

I see one or two suggestions from the bots. I think either or both would be good.

[grant-minor-sntialtech]

@bickelj Just a heads up that I renamed the script itself at Jonathans request. I'll work on the other suggestions as well.

[bickelj-agent]

Re-review on 27da419: both prior points are resolved — selectChangemakers now sets total: entries.length (my in-line note), and a new selectChangemakers suite adds 4 tests covering omit/match/unknown/no-mutation (the coverage gap). The givingTuesday.ts → gtdc990.ts rename is consistent: the index.ts import and the batch notes string update with it, while the CLI command name stays givingTuesday. tsc, eslint, prettier, and the full jest suite (72 tests) pass on the PR HEAD.

— GLM-5.2

[Copilot]

🔵 Needs a closer look

Targeted mode still fetches the full changemaker collection, and related documentation needs updating.

Review details

Suppressed comments (3)

src/gtdc990.ts:363

• The new public --changemaker-id option is not reflected in docs/givingTuesday.md's updateAll argument/usage documentation, and that report still links to the old src/givingTuesday.ts and src/givingTuesday.unit.test.ts paths while the CLI now imports gtdc990.js. Please update the generated documentation (or regenerate it) so users can discover the option and the links resolve.
  src/gtdc990.ts:366
• This targeted mode still calls getChangemakers, whose implementation requests /changemakers?_count=10000000, and only filters the full response in memory. A run for one ID therefore transfers and deserializes the same potentially huge collection as a full sync, undermining the targeted option and retaining the same timeout/memory risk. When an ID is supplied, fetch that changemaker server-side (or add a single-record API helper) and keep the all-record fetch only for the default path.
  src/index.ts:7
• Renaming the command module to gtdc990 here leaves docs/givingTuesday.md pointing at the removed src/givingTuesday.ts and describing the old module; it also does not document the new --changemaker-id option. Please update that integration document (or preserve the old filename) so the source link and CLI guidance remain usable.

import { givingTuesday } from './gtdc990.js';

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[bickelj]

@grant-minor-sntialtech I wonder if we could go back to the clearer name, or if we need to have an obscure name, have the obscure name throughout. Also whatever Copilot said.