-
Notifications
You must be signed in to change notification settings - Fork 573
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
feat(EMI-2044): Add tracking to Auction Signals #10699
feat(EMI-2044): Add tracking to Auction Signals #10699
Conversation
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.
LGTM, nothing blocking here.
...getArtworkSignalTrackingFields( | ||
artwork.collectorSignals, | ||
AREnableAuctionImprovementsSignals | ||
), |
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.
👍
import { isEmpty } from "lodash" | ||
import { DateTime } from "luxon" | ||
|
||
export type CollectorSignals = LargeArtworkRail_artworks$data[0]["collectorSignals"] |
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.
non-blocking: If this file is dependent on this one specific type, should it be located with that file?
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.
not really, since it's not a component, it's a function to get data, but I didn't want to create a type and decided to use relay types for the data
if (!isEmpty(partnerOffer)) { | ||
signal_label = "Limited-Time Offer" | ||
} |
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 might evolve with implementations for the UI as well after artsy/metaphysics#5956 merges.
"signal_label" | "signal_bid_count" | "signal_lot_watcher_count" | ||
> | ||
|
||
export const getArtworkSignalTrackingFields = ( |
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 don't like that we need to restate this auction logic just for the tracking. I was under the assumption that the signal_label
was just to track the "Limited-time offer" "Curators Pick" and "Increased Interest" badges, which is why I pushed for the "Primary Label" field in metaphysics , to easily figure out what is being shown to the user. I see now it's written as an example in the Cohesion PR and I should have caught this sooner, sorry. I don't see the benefit in tracking the "Time left to bid" and "Bidding live now" here. Not going to block this PR as I know it needs to go out soon, but I don't want to recreate this in force. @erikdstock maybe we can discuss this tomorrow.
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.
:ceiling-cat: I agree with Rachel here, this should be 1:1 from the properties we receive and forward, we shouldn't manipulate the event data before sending it to the clients
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 agree as well, but we have so many different places where tracking events are being sent, so I decided instead of adding the functionality in all the places, I extracted it to one place and used it wherever applicable
I'll merge this PR if that's okay and continue on with the release :) |
* feat(EMI-2044): Add tracking to Auction Signals * fix test * update tests * fix test 2.0
This PR resolves EMI-2044
Description
This PR adds tracking for Artworks with auction signals
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.