-
Notifications
You must be signed in to change notification settings - Fork 12
feat: Tracking needs for Shipping Presets project #661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1627,6 +1627,7 @@ export interface ClickedPublish { | |
| context_module: ContextModule | ||
| artwork_id: string | ||
| label: string | ||
| shipping_preset_id?: string | ||
| } | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 (?)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's the idea. These are already specific actions ( |
||
|
|
||
| /** | ||
|
|
@@ -1650,6 +1651,7 @@ export interface ClickedSave { | |
| context_module: ContextModule | ||
| artwork_id: string | ||
| label: string | ||
| shipping_preset_id?: string | ||
| } | ||
|
|
||
| /** | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the diff I'd like the most feedback on!
Allowing the
CmsBulkEditProcessingCompletedevent to support optional arrays forlabelsandvaluesThis is needed as all shipping/pickup related fields will be sent over at one time
Ex:
And here's the UI, all domestic, international, and collector pickup are sent over on a single click
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if this raises any red flags on your end @leodmz !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The singular
labelandvaluesingular will continue to fire as expected/untouchedThe array fields are brand new attributes on the event -
labelsandvalueswill be separate optional fields and only be used when we are ready!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see now. No concerns from my side then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 , let me know if we should ticket this to use the array values event type as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)