-
Notifications
You must be signed in to change notification settings - Fork 11
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:add support for cpsv #956
Conversation
9ce557d
to
1c43185
Compare
80328bc
to
864bbf0
Compare
6bf2d47
to
b885283
Compare
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 think we need to simplify and add tests. There's too many cases where the desired outcome is not transparently clear from reading the code.
.required("Price is a required field"), | ||
billingType: string() | ||
.label("Pricing Type") | ||
.oneOf(["cpm", "cpc"]) | ||
.oneOf(["cpm", "cpc", "cpsv"]) | ||
.required("Pricing type is a required field"), | ||
adSets: array() |
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.
We need some tests around this schema, please. This is too complex to validate behaviour by reading.
</Typography> | ||
) : ( | ||
<Stack direction="column" spacing={2} alignItems="flex-start"> | ||
<Stack direction="column" spacing={2} alignItems="flex-start"> |
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.
Is there any way we can simplify this, either by splitting into smaller components or moving some of the logic into (tested) functions? It's really quite hard to read, and the prices.find
logic should not be duplicated.
(p) => | ||
p.format === values.format && p.billingType === billingType, | ||
); | ||
if (price) campaignPrice.setValue(price?.billingModelPrice); |
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.
nit:
if (price) campaignPrice.setValue(price?.billingModelPrice); | |
if (price) campaignPrice.setValue(price.billingModelPrice); |
You've qualified that price is defined, so you don't need to nullability check in the body of the if, typescript will infer this.
)} | ||
) : ( | ||
<Typography variant="body2"> | ||
<strong>{_.upperCase(values.billingType)}</strong>{" "} |
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.
<strong>{_.upperCase(values.billingType)}</strong>{" "} | |
<strong>{uiLabelsForBillingType(values.billingType).<whichever one you want>}</strong>{" "} |
p.billingType === bMeta.value.toUpperCase() | ||
); | ||
}); | ||
p.billingType === billingType.value.toUpperCase(), |
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.
Another prices.find
with a billingType.toUpperCase
... let's factor this logic out.
if (props.format === CampaignFormat.NewsDisplayAd) { | ||
price.setValue(found?.billingModelPrice ?? "10"); | ||
billing.setValue("cpm"); | ||
if (billingType.value === "cpc") helper.setValue("cpm"); |
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 looks wrong?
Oh. If the billing type was cpc before the format switched to news, then force the billing type to cpm
is I think the behaviour. But that doesn't entirely make sense to me: surely if the price changes the billing model also has to change, since the two are linked - a CPC price doesn't make sense for CPM.
billingType: BillingType, | ||
defaultPrice: string, | ||
schema: StringSchema<string | undefined, AnyObject, undefined, "">, | ||
) { | ||
const found = prices.find( | ||
(p) => p.format === format && p.billingType === billingType, | ||
(p) => formats.includes(p.format) && p.billingType === billingType, |
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.
Interesting, this one doesn't uppercase the billing type ;)
Going to close for now, and re-open when I tackle structured pricing |
Part of #955
Screenshare.-.2023-11-08.4_14_53.PM.mp4