Feature meta capi - #88
Conversation
maxakuru
left a comment
There was a problem hiding this comment.
Blocking issues
-
No filtering for completed/terminal orders — will fire Purchase for cancelled/incomplete orders
src/actions/meta-capi/index.js:40-46
getJournalEntriesreturns all journal entries in the window, not just completed purchases. The siblingebs-sync/sync.js:81-83explicitly filters toTERMINAL_EVENTS = ['payment_completed', 'payment_cancelled']before acting. Here there's no event-type filter at all, andacc[entry.orderId] ??= entrykeeps the first entry seen per orderId — likely the earliest lifecycle event (e.g.order_created), notpayment_completed. Net effect: this can send a Meta "Purchase" conversion for an order that was never actually paid, or that was later cancelled. -
Meta API errors are silently treated as success
src/actions/meta-capi/index.js:301-308(sendToMeta) never throws on a non-2xx response — it just returns{status: response.status, ...}inside the resolved value. Back in the caller,index.js:58-60unconditionally callscompleteOrderand logs "Successfully fired" regardless of what Meta actually returned. A rejected event (bad token, bad pixel id, malformed payload) gets markedPROCESSEDforever with no retry and no visible failure signal. -
event_timesent as an ISO string, not a Unix timestamp
src/actions/meta-capi/index.js:198—event_time: new Date().toISOString(). Meta's Conversions API requiresevent_timeas an integer Unix timestamp in seconds; an ISO string will very likely be rejected by Meta on every call. Combined with issue #2, this failure mode would be invisible. Also worth fixing: it uses "now" rather than the order's actual purchase time, which matters for a job that scans up to 24h back. -
A single failed order aborts the entire batch
src/actions/meta-capi/index.js:61-67— thecatchinside the per-order loop doesreturn jsonResponse(500, ...)instead ofcontinue. One bad order (e.g. a 404 fromgetOrder, or issue #7 below) stops processing of every other order queued in that invocation, rather than skipping and moving to the next. -
Web-exposed action has no request authentication
app.config.yamlsetsweb: 'yes'/require-adobe-auth: falseformeta-capi, andrequireAuth(index.js:85-100) only checks that server-configured params are present — it validates nothing supplied by the caller. Compare toebs-sync/index.js:46-54, which validates aSYNC_STATUS_TOKENbearer header for its web-exposed paths. As written, anyone who finds the action URL can trigger a full journal scan + Meta CAPI send with no credential. Since this is driven entirely by themeta-capi-schedulealarm trigger, consider droppingweb: 'yes'entirely, or adding the same bearer-token checkebs-syncuses.
Should fix
-
Truncated order suffix used as both the lock key and the Meta
event_id
index.js:52(orderValue = orderNumber.split('-').pop()) andindex.js:199(event_id: eventId.split('-').pop()). If two orders from different stores/prefixes share the same numeric suffix, they'll collide on the state lock key (one order silently treated as alreadyPROCESSED) and on the Metaevent_id. Since the whole point of this PR is dedup,event_idshould also be verified against whatevereventIDthe client-side Pixel actually sends for the same purchase — a mismatch here means Meta can't dedupe at all, defeating the ticket's purpose. -
Unsafe property access breaks the defensive pattern used two lines above
index.js:235—price: parseFloat(item.price.final)throws ifitem.priceis missing/undefined, while the adjacentid/quantityfields use?.. Given issue #4, one malformed line item kills the rest of the run. -
Unprefixed state keys share a namespace-wide KV store with
ebs-sync
@adobe/aio-lib-state'sinit()returns a client scoped to the whole Adobe I/O Runtime namespace, not to an individual action — all actions in the namespace share the same keyspace and quota.ebs-sync/state.js:16-17prefixes its keys (ebs-sync:state,ebs-sync:lock) specifically to avoid collisions.meta-capi'sclaimOrder/completeOrder/failOrder(index.js:113, 150, 170) instead key on bare, unprefixed order IDs. No live collision today (ebs-syncnever uses a raw order ID as a top-level key), but it breaks the established convention and creates risk for any future action, plus adds unbounded high-cardinality keys to a shared per-namespace quota. Prefix these keys, e.g.`meta-capi:${orderValue}`.
Nitpicks
index.js:20— leftover commented-out line (//return jsonResponse(202, ...)), remove before merge.README.mddescribes accepting__ow_body/ raw JSON invocation, andexample-event.jsonuses a camelCase schema (eventName,userData,customData) — neither matches the actual implementation, which is cron-driven only and builds snake_case (event_name,user_data,custom_data) fields. Docs look copied from an earlier design; update or drop them.index.js:132-137—claimOrder's state TTL is 600s (10 min) butlockedUntilimplies a 15-min lock; the key can vanish and be reclaimed before the code's own stated lock window elapses. Align the two.Core.Logger('meta-capi', ...)is re-instantiated inmain,requireAuth, andsendToMeta(lines 28, 88, 274) — could be created once and threaded through.- Phone numbers are hashed with only
.trim().toLowerCase()(index.js:192) — Meta's spec wants phone digits-only with country code before hashing; low priority but affects match quality.
|
Thank you, @maxakuru, for taking the time to thoroughly review my PR and provide detailed feedback. Below are my comments on each of the points raised. The requested changes have been addressed and pushed to this PR.
|
maxakuru
left a comment
There was a problem hiding this comment.
1. 🔴 Blocking: action is web: 'yes' with no HTTP API, causing the new auth check to 401 out the cron trigger
app.config.yaml:70-76 and src/actions/meta-capi/index.js:32-34
This action has no HTTP status/trigger endpoint (unlike ebs-sync, which exposes GET/POST handlers and only calls requireAuth for those web-invoked paths). meta-capi is purely cron-driven via meta-capi-rule → meta-capi-schedule. Making it web: 'yes' was the actual mistake — it's not needed since there's no HTTP API to protect, and it's what created the need for the bearer-token check in the first place.
Since requireAuth (index.js:109-117) runs unconditionally in main(), and alarm-triggered invocations never carry an Authorization header, every scheduled run now returns 401 before doing any work — the action can never fire.
Suggested fix: set web: 'yes' → web: 'no' (or omit it) in app.config.yaml for meta-capi, drop the require-adobe-auth annotation, and remove requireAuth() / the SYNC_STATUS_TOKEN input from index.js entirely. A non-web action invoked only by a rule doesn't need a bearer-token gate — there's no public URL to protect.
2. 🟡 item.price.final still unsafe — and no fallback like ebs-sync uses
src/actions/meta-capi/index.js:258
price: parseFloat(item?.price.final)item?.price only guards against item being nullish — if item.price itself is undefined, this still throws.
ebs-sync handles this same shape defensively with a fallback to regular (price: { final: string; currency: string; regular?: string } per ebs-sync/types.d.ts:149):
// ebs-sync/ebs.js:707
const unitCents = moneyToCents(item.price?.final || item.price?.regular || '0.00');Suggest the same pattern here:
price: parseFloat(item?.price?.final || item?.price?.regular || '0')3. 🟡 Error message lost in the per-order failure log
src/actions/meta-capi/index.js:89
log.error('Error occurred while processing order', { orderId: orderValue, error: JSON.stringify(error) });Error.message is non-enumerable, so JSON.stringify(error) serializes to "{}" — the actual failure reason won't appear in this log line. Use error: error.message, consistent with the top-level catch at line 95.
4. 🟡 README is out of sync with the actual event_id / event_time extraction logic
Since event_id matching the client-side Pixel eventID is called out as required for dedup to work at all, these need to be accurate:
README.md:42saysevent_idis extracted viaorderId.split('-').pop(), but the code (index.js:202,222) now extracts it viaorderValue.split("Z-")[1]. Update the doc to match.README.md:30says the state key uses the fullorderId, butindex.js:78,130still uses the truncated suffix (order.split('-').pop()). Update the doc to match.README.md:90saysevent_timecomes fromorder.timestamp, but there's no such field — it's parsed out of theorderIdstring itself via the"Z-"split. Update the doc to match.example-event.json:6still showsevent_timeas an ISO string, contradicting both the real payload and the README's own "Payload structure" example (README.md:55, which correctly shows an integer). Fix the sample to an integer timestamp.
Also worth confirming as a follow-up (not necessarily a code change, but flag it): buildMetaRequestPayload assumes every orderId has the shape {ISO8601-timestamp}Z-{realOrderId}. If an order's ID doesn't contain "Z-", orderId destructures to undefined, and since JSON.stringify drops undefined properties, the outgoing Meta event would silently have no event_id at all, breaking dedup with no error. Consider a guard/log for that case, or confirm the format is guaranteed for all orders.
5. 🟢 Leftover dead code
src/actions/meta-capi/index.js:204
//eventId.split('-').pop()Leftover comment from the old extraction approach — remove before merge.
This is to implement Meta deduplication for Purchase event.
Ticket # VITA-803 Meta Deduplication Integration for Purchase event