-
Notifications
You must be signed in to change notification settings - Fork 2k
A4A > Marketplace: Implement term pricing for Hosting, Products & Cart #107867
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
A4A > Marketplace: Implement term pricing for Hosting, Products & Cart #107867
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~108 bytes added 📈 [gzipped]) DetailsCommon code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~48 bytes added 📈 [gzipped]) DetailsSections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
|
@yashwin, I tested it, and it works just fine. My main concern is that we are doing a lot of conditional rendering. Do you think we can just calculate the value (discount and actual cost) within |
| const isFree = actualCost === 0; | ||
|
|
||
| const renderPrice = () => { | ||
| if ( isTermPricingEnabled ) { |
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 can see that we are doing conditional rendering with the prices based on the term selected. I wonder if we could just handle the calculation from getProductPricingInfo so we don't have to do conditional rendering here and other places. wdyt?
288500f to
21a9dbb
Compare
|
Thanks for the review, @jkguidaven! I have made the changes as per your suggestions. Could you please take another look? |
21a9dbb to
1b74cb2
Compare
|
this PR needs a rebase |
1b74cb2 to
8142181
Compare
8142181 to
c1f39f6
Compare
| import { getActiveAgency } from 'calypso/state/a8c-for-agencies/agency/selectors'; | ||
| import { TitanOrder } from 'calypso/state/a8c-for-agencies/types'; | ||
| import { APIProductFamilyProduct } from 'calypso/state/partner-portal/types'; | ||
| import type { APIProductFamilyProduct } from 'calypso/a8c-for-agencies/types/products'; |
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 created a new types file on A4A as we are introducing new fields.
| } ); | ||
| } | ||
|
|
||
| export function usePublicProductsQuery(): UseQueryResult< APIProductFamilyProduct[], unknown > { |
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 are not using this anywhere.
| const getProductsQueryKey = ( agencyId?: number ) => [ 'a4a', 'marketplace', 'products', agencyId ]; | ||
|
|
||
| export default function useProductsQuery( | ||
| isPublicFacing = false, |
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.
Removed support for the public-facing API as we no longer need it.
| productSearchQuery, | ||
| usePublicQuery = false, | ||
| }: Props ) { | ||
| const { data, isLoading: isLoadingProducts } = useProductsQuery( usePublicQuery ); |
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.
usePublicQuery is not used anywhere.
| export function useProductCategories( product: APIProductFamilyProduct ): string[] { | ||
| const translate = useTranslate(); | ||
| const { family_slug } = product; | ||
| const { family_slug, slug } = product; |
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 are going to change how we group families. You can see the response here: 199438-ghe-Automattic/wpcom
To support that, we are going to use the slug also so that the current implementation doesn't break.
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.
It's ok to repeat the same return here as we will get rid of the logic completely when we enable BD for all the users.
| </> | ||
| ); | ||
| }; | ||
|
|
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.
Fixing some TS warnings
| return 0; | ||
| }; | ||
|
|
||
| export const calculateCommissions = ( referral: Referral, products: APIProductFamilyProduct[] ) => { |
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 being used anymore
|
|
||
| export const getProductCommissionPercentage = ( slug?: string ) => { | ||
| if ( ! slug || ! isProductEligibleForCommission( slug ) ) { | ||
| export const getProductCommissionPercentage = ( slug?: string, familySlug?: 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.
As mentioned earlier, we will change the family structure. This is to correctly calculate the commissions.
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.
since familySlug was added do we plan to get rid of slug or something? Or are they both needed?
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, both are needed
0208ea9 to
029f8f3
Compare
|
I tested it, using the new endpoint, it seems that it works well, but my checkout is configured to use EUR, so in the marketplace the prices are in USD, and in the checkout it is EUR. |
|
|
||
| const isTermPricingEnabled = isEnabled( 'a4a-bd-term-pricing' ) && isEnabled( 'a4a-bd-checkout' ); | ||
|
|
||
| const priceInterval = () => { |
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 a way we could make this reusable? This seems to be repeated 3 times in different places. could be part of getProductPricingInfo?
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 tried doing it. The thing is, this will be removed soon, so we will only have term pricing going forward. So, I would leave this as-is for now. Also, it is repeated in 2 places only, the text is different in another place.
da7bb97 to
31110d8
Compare
|
Thanks for the review, @cleacos & @jkguidaven! I found the filters weren't working properly. I have fixed it now: 31110d8
@cleacos This is based on the endpoint, not the UI. UI is configured to work with the product's currency. I have now updated the endpoint to take the current based on the agency. So, this should work fine now. |
dc80259 to
e6d9019
Compare
|
@yashwin I'm still facing this issue for WPCOM plans:
I added the WPCOM hosting plan to the cart, then I went to the checkout page, and it failed. |
|
@cleacos: Could you please try again? I tested it locally for both monthly and yearly products and it works fine. Please ensure to get the latest trunk and restart the server when you switch to this branch.
|
57d4684 to
7b72516
Compare
| product_id: number; | ||
| monthly_product_id?: number; | ||
| yearly_product_id?: number; | ||
| alternative_product_id?: number; |
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.
Missed fields?
monthly_alternative_product_idyearly_alternative_product_id
and remove alternative_product_id?
I had this comment from the previous version
| @@ -1,9 +1,8 @@ | |||
| export const SECURITY_PRODUCT_SLUGS = [ | |||
| 'jetpack-backup', | |||
| 'jetpack-backup-t1', | |||
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 that Jetpack Backup products could be T1 or T2.
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.
No, we only show T1, right?
a8439bf to
5711f86
Compare
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
|
There is a mismatch between the WPCOM Hosting prices. For example, the base monthly price in EUR should be 22 EUR and the endpoint returns 40 |
5711f86 to
f2b10d3
Compare
travisw
left a comment
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 didn't inspect every line of code but in general the actual changes in functionality seem reasonable. And in testing it seemed ok.
I also left a couple comment on the back pr about incorrect prices and using different product ids in the endpoint (200442-ghe-Automattic/wpcom).
|
|
||
| export const getProductCommissionPercentage = ( slug?: string ) => { | ||
| if ( ! slug || ! isProductEligibleForCommission( slug ) ) { | ||
| export const getProductCommissionPercentage = ( slug?: string, familySlug?: 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.
since familySlug was added do we plan to get rid of slug or something? Or are they both needed?
client/a8c-for-agencies/sections/marketplace/hooks/use-total-invoice-value.ts
Show resolved
Hide resolved
...-overview/hosting-content/standard-agency-hosting/wpcom-plan-section/wpcom-plan-selector.tsx
Show resolved
Hide resolved
To clarify, non-BD agencies should be unchanged, right? No term pricing, prices still using the old endpoint, ... |
|
Thanks for the review, @travisw!
Yes, both are needed for the filters to work properly.
Yes, correct. No term pricing, no BD checkout.
I'll address that separately. |
I'll fix this in another PR |
travisw
left a comment
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.
Seems good to ship
|
Closing this in favour of #108280 |





Part of https://linear.app/a8c/issue/A4A-1926/marketplace-show-monthlyearly-pricing-for-hosting-and-products
Resolves https://linear.app/a8c/issue/A4A-1929/products-display-monthlyyearly-pricing-on-cards
Resolves https://linear.app/a8c/issue/A4A-1930/hosting-display-monthlyyearly-pricing-on-cards
Resolves https://linear.app/a8c/issue/A4A-1961/cart-display-monthlyyearly-pricing
Resolves https://linear.app/a8c/issue/A4A-1962/display-term-based-pricing-on-product-details-modal-lightbox
Proposed Changes
This PR implements term pricing for Hosting, Products & Cart.
Why are these changes being made?
Testing Instructions
Note: There might be some pricing difference when the feature flag is enabled. Please ignore it now, as we will fix it later.
?flags=-a4a-bd-term-pricingVerify that all the filters work as expected on the Products page
Patch 199438-ghe-Automattic/wpcom/
Append the URL with
?flags=a4a-bd-term-pricing> Test all the scenarios mentioned below > Verify the UI looks as displayed below for yearly and monthly billing.Billing term: Monthly
Billing term: Yearly
Pre-merge Checklist