Skip to content

Add update_professional_contact tool to CRM extension#133

Open
swhitt wants to merge 3 commits intoNateBJones-Projects:mainfrom
swhitt:add-update-professional-contact
Open

Add update_professional_contact tool to CRM extension#133
swhitt wants to merge 3 commits intoNateBJones-Projects:mainfrom
swhitt:add-update-professional-contact

Conversation

@swhitt
Copy link
Copy Markdown
Contributor

@swhitt swhitt commented Mar 29, 2026

Contribution Type

  • Repo improvement (docs, CI, templates)

What does this do?

Adds the missing update_professional_contact tool to Extension 5 (Professional CRM), as proposed in #93 but never implemented. Accepts contact_id plus any combination of optional fields (name, company, title, email, phone, linkedin_url, how_we_met, tags, notes, follow_up_date). Only provided fields are updated. Also fixes follow_up_date being unwritable despite get_follow_ups_due querying it.

Requirements

Nothing new.

Checklist

  • I've read CONTRIBUTING.md
  • My contribution has a README.md with prerequisites, step-by-step instructions, and expected outcome
  • My metadata.json has all required fields
  • I tested this on my own Open Brain instance
  • No credentials, API keys, or secrets are included

Adds the ability to update existing contact fields (name, company,
title, email, phone, tags, notes, follow_up_date, etc.) which was
proposed in NateBJones-Projects#93 but never implemented. Only provided fields are
updated, and the existing updated_at trigger handles timestamping.
@matthallett1
Copy link
Copy Markdown
Collaborator

@claude review this PR

1 similar comment
@matthallett1
Copy link
Copy Markdown
Collaborator

@claude review this PR

@github-actions
Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@matthallett1
Copy link
Copy Markdown
Collaborator

@claude review this PR

@github-actions
Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@matthallett1
Copy link
Copy Markdown
Collaborator

@claude review this PR

@github-actions
Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@matthallett1
Copy link
Copy Markdown
Collaborator

@claude review this PR

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 30, 2026

Claude finished @matthallett1's task in 1m 34s —— View job


PR Review: Add update_professional_contact tool

  • Gather context
  • Read changed files
  • Review alignment, safety, and design patterns
  • Post findings

Overall: This is a clean, well-scoped addition that fills a real gap. Approving with a few minor notes.


What the PR Does

Adds Tool 7 (update_professional_contact) to the Professional CRM extension, plus a one-line header comment update. The tool accepts contact_id plus any combination of optional fields and only writes the fields that are actually provided.


Positives

  • Correct partial-update pattern (index.ts:384–387): Using Object.entries(fields) to build a updates object and skipping undefined values is the right approach for a patch-style tool. No risk of accidentally nulling fields the user didn't mention.
  • Empty-update guard (index.ts:389–391): Throws if no fields are provided, preventing a no-op DB roundtrip.
  • User-scoped update (index.ts:396–397): .eq("id", contact_id).eq("user_id", userId) ensures users can only update their own contacts. This is critical and it's done correctly.
  • Returns updated record (index.ts:398–399): .select().single() confirms the row was found and returns fresh data, so the caller knows the update took effect.
  • follow_up_date fix: The schema has this column and get_follow_ups_due queries it, but there was no way to write it before. This unblocks a core workflow.

Minor Notes

1. notes and tags are destructive replacements (index.ts:379–380)

The descriptions already say "Replace…" which is good. However, users who use link_thought_to_contact (which appends to notes) may be surprised if they later call update_professional_contact with a new notes value and wipe those linked thoughts. No action required — just worth documenting in the README if it isn't already.

2. add_professional_contact still lacks follow_up_date (index.ts:64–114)

Pre-existing gap — the create tool has no follow_up_date parameter even though the schema supports it. This PR's update tool provides a workaround (create then immediately update), but a future PR could add follow_up_date to add_professional_contact for completeness. Not a blocker.

3. No date format validation on follow_up_date

z.string() accepts anything. The description says "YYYY-MM-DD" but there's no regex check. This is consistent with how expected_close_date is handled in create_opportunity, so not a regression — just worth noting if someone wants to harden the inputs later. Fix this →


Safety & Design Checklist

Check Status
No credentials or hardcoded secrets
No destructive SQL
Remote Edge Function (not local Node.js server)
User-scoped queries (user_id filter on update)
Core thoughts table untouched
Consistent with existing tool patterns in this file

This is a solid contribution. The implementation is correct and the gap it fills is real. The minor notes above are non-blocking.

@github-actions github-actions bot added the extension Contribution: curated learning path build label Mar 30, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 30, 2026

Claude finished @matthallett1's task in 1m 1s —— View job


PR Review: Add update_professional_contact tool

  • Gather context
  • Read changed files
  • Review alignment, safety, and design patterns
  • Post findings

Overall: Clean, well-targeted addition. Approving with minor notes.

This PR fills a genuine gap — the CRM extension had no way to update contact fields or set follow_up_date, despite get_follow_ups_due relying on that column. The implementation is correct and consistent with the existing tool patterns.


What Changed

  • index.ts:366–416: New Tool 7 update_professional_contact — patch-style update accepting contact_id + any combination of optional fields
  • index.ts:4: Header comment updated to reflect add/update/search capability
  • Tool previously numbered 7 (link_thought_to_contact) renumbered to Tool 8

Positives

  • Correct partial-update pattern (index.ts:384–387): Iterating Object.entries(fields) and skipping undefined values is the right approach — no risk of accidentally nulling unmentioned fields.
  • Empty-update guard (index.ts:389–391): Throws before hitting the database if no fields are provided. Good.
  • User-scoped write (index.ts:396–397): .eq("id", contact_id).eq("user_id", userId) — users can only update their own contacts. Critical and correctly implemented.
  • Returns updated record (index.ts:398–399): .select().single() confirms the row exists and was updated, giving the caller fresh data.
  • follow_up_date unblocked: get_follow_ups_due (Tool 6) queries this column but there was previously no write path for it. This PR closes that loop.

Minor Notes

1. notes replacement may surprise users who used link_thought_to_contact

link_thought_to_contact (Tool 8) appends to notes. If a user later calls update_professional_contact with a new notes value, those linked thought references get overwritten. The description already says "Replace notes" — worth a one-liner in the README reinforcing this, but not a blocker.

2. add_professional_contact still lacks follow_up_date (index.ts:64–114)

Pre-existing gap. This PR's update tool provides a workaround (create, then update), but a follow-up PR adding follow_up_date to the create tool would complete the picture. Not a blocker for this PR.

3. No date format validation on follow_up_date (index.ts:381)

z.string() accepts any string. The description says "YYYY-MM-DD" but there's no enforcement. This is consistent with how expected_close_date is handled elsewhere in the file, so it's not a regression — just worth hardening later. Fix this →


Safety & Design Checklist

Check Status
No credentials or hardcoded secrets
No destructive SQL
Remote Edge Function (not local Node.js)
User-scoped update (user_id filter)
Core thoughts table untouched
Follows existing tool patterns in this file
No new dependencies or standalone deployment

The implementation is correct, safe, and fills a real functional gap in the CRM extension. The minor notes above are non-blocking. Good contribution.

Copy link
Copy Markdown
Collaborator

@matthallett1 matthallett1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work, Steve — the partial-update pattern is clean and the user-scoping is correct.

One issue to address before merging: follow_up_date can be set but never cleared. The update loop drops undefined values, and sending an empty string fails against the DATE column. This means once a follow-up is set, the contact is permanently stuck in get_follow_ups_due (which filters on follow_up_date IS NOT NULL).

Fix: Accept null or empty string for follow_up_date and follow_up_notes, and write null to the DB when clearing:

// In the update loop, handle null explicitly for clearable fields
if (key === 'follow_up_date' && (value === null || value === '')) {
  updates[key] = null;
} else if (value !== undefined) {
  updates[key] = value;
}

This unblocks the 'follow-up completed' workflow where users want to clear reminders after acting on them.

Fixes the case where a follow-up date, once set, could never be
cleared — leaving contacts permanently stuck in get_follow_ups_due.
@swhitt
Copy link
Copy Markdown
Contributor Author

swhitt commented Mar 31, 2026

Good catch, thanks; fixed. follow_up_date now accepts null or empty string, writing null to the DB so contacts can be cleared from get_follow_ups_due.

@swhitt swhitt requested a review from matthallett1 March 31, 2026 19:57
@alanshurafa
Copy link
Copy Markdown
Contributor

Review feedback from @matthallett1 has been addressed in commit b598fef:

  • follow_up_date clearing: Now accepts null or empty string to clear follow_up_date and follow_up_notes, unblocking the "follow-up completed" workflow.

Ready for re-review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extension Contribution: curated learning path build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants