feat: Tracking needs for Shipping Presets project #661
Conversation
…rtwork form clicks
artsy-peril
bot
added
the
Jira Synced
| artwork_id: string | ||
| label: string | ||
| shipping_preset_id?: string | ||
| } |
There was a problem hiding this comment.
these ones I want to look into more - maybe its ok to have such a specific optional field on a very generic artwork click event (?)
There was a problem hiding this comment.
Yeah, that's the idea. These are already specific actions (ClickedPublish, ClickedSave). They only get triggered in CMS when hitting the CTAs, so by adding this we can always capture the shipping preset id on saving and publishing.
| values?: string[] | ||
| artwork_ids: string[] | ||
| shipping_preset_id?: string | ||
| } |
There was a problem hiding this comment.
This is probably the diff I'd like the most feedback on!
Allowing the CmsBulkEditProcessingCompleted event to support optional arrays for labels and values
This is needed as all shipping/pickup related fields will be sent over at one time
Ex:
{
action: "processingCompleted",
context_module: "Artworks - bulk edit",
labels: ["change domesticShipping", "change internationalShipping", "change collectorPickup"],
values: ["2000", "3000", "1000"],
artwork_ids: ["artwork1", "artwork2"],
shipping_preset_id: "preset123"
}
And here's the UI, all domestic, international, and collector pickup are sent over on a single click
There was a problem hiding this comment.
Let me know if this raises any red flags on your end @leodmz !
There was a problem hiding this comment.
the optional array bit is fine.
I'm a bit concerned with amending an attribute on an existing event though. We've had issues in the past when trying to change the type of an attribute after it already started being fired.
This is a bit different but we should try firing some examples in prod' to see what it looks like in redshift
There was a problem hiding this comment.
The singular label and value singular will continue to fire as expected/untouched
The array fields are brand new attributes on the event - labelsand values will be separate optional fields and only be used when we are ready!
There was a problem hiding this comment.
ah I see now. No concerns from my side then :)
There was a problem hiding this comment.
Ohh interesting this wasn't already the case.
This probably means we're not capturing all of them in case of signature then (signature section on the drawer already provides multiple fields).
It shouldn't be a big deal as I believe @leodmz is relying on the actual db tables for most of the tracking events related to the updated fields, but just something to be aware of.
There was a problem hiding this comment.
Yes you are right @lidimayra, looks like we never send over the text field value when a user updates a signature in batch edits
I've updated this example to signature: hand signed and notes of "bottom left hand" and notice we only send the first value
There was a problem hiding this comment.
🐛 , let me know if we should ticket this to use the array values event type as well!
There was a problem hiding this comment.
I'd say only if @leodmz thinks it might be useful somehow (if we never had it and we never lacked it, it might not even be needed, given our analysis are usually on top of the data provided by Gravity db tables)
jpotts244
changed the title
jpotts244
changed the title
| values?: string[] | ||
| artwork_ids: string[] | ||
| shipping_preset_id?: string | ||
| } |
There was a problem hiding this comment.
Ohh interesting this wasn't already the case.
This probably means we're not capturing all of them in case of signature then (signature section on the drawer already provides multiple fields).
It shouldn't be a big deal as I believe @leodmz is relying on the actual db tables for most of the tracking events related to the updated fields, but just something to be aware of.
|
🚀 PR was released in |
The type of this PR is: FEAT
This PR resolves Shipping-Preset-Project-tracking
Description
Adjusting and adding some new tracking details for the shipping presets flows
See thoughts inline
PR Checklist (tick all before merging)
index.tsclickandtap(platform is inferred by the DB storing events)cc @artsy/amber-devs