Skip to content

feat: add Plivo SMS service (#322)#326

Open
radaghastly wants to merge 10 commits into
mainfrom
feat/322-plivo-sms
Open

feat: add Plivo SMS service (#322)#326
radaghastly wants to merge 10 commits into
mainfrom
feat/322-plivo-sms

Conversation

@radaghastly
Copy link
Copy Markdown
Collaborator

Adds Plivo SMS as a new messaging service, following the existing service pattern.

Changes

  • src/routes/plivo.js — Route handler with Basic Auth, message listing, single message retrieval, account info, and wildcard GET
  • src/routes/ui/plivo.js — UI setup card (Account Name, Auth ID, Auth Token)
  • src/lib/serviceRegistry.js — Added plivo to SERVICE_REGISTRY, SERVICE_READERS, and new messaging category
  • src/lib/queueExecutor.js — Added plivo to SERVICE_URLS, getAccessToken, buildUrl, buildHeaders
  • src/routes/ui/services.js — Registered plivo UI module and catalog entry
  • src/routes/ui/service-detail.js — Added plivo icon and form fields
  • src/index.js — Mounted plivo routes at /api/plivo
  • src/services/queueService.js — Added plivo to VALID_SERVICES for write proxy
  • tests/plivo.test.js — Jest tests (9 passing)
  • public/icons/plivo.svg — SMS chat bubble icon

Closes #322

Copy link
Copy Markdown
Collaborator

@luthien-m luthien-m left a comment

Choose a reason for hiding this comment

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

Clean implementation, follows existing patterns. LGTM ✅

One merge concern:

Both this PR and #324 (Twilio) independently create a messaging category in serviceRegistry.js. Whichever merges second will conflict. The fix is simple — just combine services: ["twilio", "plivo"] — but whoever merges second needs to handle it.

Minor notes (non-blocking):

  1. No writeGuidelines in serviceInfo — Twilio's PR included guidelines about SMS costing money, verifying phone numbers, etc. Worth adding for agent safety since Plivo also sends paid SMS.

  2. Nice improvement: Auth token field uses type="password" by default in the UI card — better than Twilio's type="text" default. 👍

Tests are thorough (7 cases including network errors). Basic auth pattern is correct. Plivo API path structure with trailing slashes is handled properly.

Copy link
Copy Markdown
Collaborator

@jimleighy jimleighy left a comment

Choose a reason for hiding this comment

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

Solid work, Rad. Follows existing service patterns well:

  • Route handler with proper auth (Basic Auth via authId/authToken) ✅
  • Service registry + categories integration ✅
  • Queue executor support (buildUrl, buildHeaders, getAccessToken) ✅
  • UI setup card with proper escaping ✅
  • 9 passing tests ✅
  • Clean SVG icon ✅

One minor note (non-blocking): the wildcard GET route and the specific message routes overlap — Express will hit the specific ones first due to ordering, so it works fine. LGTM 👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Plivo SMS as a supported service

3 participants