Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add to your .package(url: "https://github.com/automattic/wordpress-rs", branch: "pr-build/1289")Built from e022ffa |
| /// Numeric sale price when a coupon applies. | ||
| #[serde(default)] | ||
| #[uniffi(default = None)] | ||
| pub sale_cost: Option<f64>, |
There was a problem hiding this comment.
#1283 has landed, so we could probably use Decimal2 here?
There was a problem hiding this comment.
It hadn't landed yet when I worked on this. Let me make that change.
bfea4c6 to
6b6b53d
Compare
jkmassel
left a comment
There was a problem hiding this comment.
Some suggestions for you to work around the strangeness of this API shape.
| #[uniffi(default = None)] | ||
| pub usage_limit: Option<u32>, | ||
| /// Cost per interval during the offer period. | ||
| pub cost_per_interval: f64, |
There was a problem hiding this comment.
Same point about Decimal2
There was a problem hiding this comment.
Addressed in 900783a.
Note that upon reviewing the API implementation, I've also updated tld_rank to Option<f64> per Claude's suggestion.
| pub struct ProductsParams { | ||
| /// Filter by product type (e.g. `"domains"`). | ||
| #[uniffi(default = None)] | ||
| pub product_type: Option<String>, |
There was a problem hiding this comment.
This endpoint has a bunch of localized fields, so we need to provide the locale we want strings in.
The argument is locale, and it can be WPComLanguage – that should automatically handle serialization into query strings for you.
| pub struct ProductsParams { | ||
| /// Filter by product type (e.g. `"domains"`). | ||
| #[uniffi(default = None)] | ||
| pub product_type: Option<String>, |
There was a problem hiding this comment.
WDYT about introducing a ProductType enum for this?
It would need the Other case to handle any new values, but it'd avoid stringly-typing for an operation like "get all the domain registration products" – the API returns all results instead of no results if something is misspelled.
| /// Absent for non-registration products like domain mapping. | ||
| #[serde(default)] | ||
| #[uniffi(default = None)] | ||
| pub tld: Option<String>, |
There was a problem hiding this comment.
tld, is_privacy_protection_product_purchase_allowed, is_hsts_required, is_dot_gay_notice_required are all only defined on domains.
WDYT about defining DomainProductInfo to hold these?
6b6b53d to
8d88fb4
Compare
| /// Locale for localized product names and descriptions. | ||
| #[uniffi(default = None)] | ||
| pub locale: Option<WPComLanguage>, | ||
| } |
There was a problem hiding this comment.
There's also a currency param that controls which currency is returned.
We should implement it, probably as an enum with USD, EUR, AUD, etc. We can have a catch-all Other for it. It can be a separate PR, but we should update the places where currency is emitted to use that too.
There was a problem hiding this comment.
I'll address this in a separate PR to avoid some merge conflicts.
| pub cost_smallest_unit: u64, | ||
| pub currency_code: CurrencyCode, | ||
| /// Billing period (e.g. `"year"`). | ||
| pub product_term: String, |
There was a problem hiding this comment.
This seems like a constrained set of values – should we make it an enum as well?
| /// Details of an active sale coupon applied to a product. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, uniffi::Record)] | ||
| pub struct SaleCoupon { | ||
| pub start_date: String, |
There was a problem hiding this comment.
Based on the field name, I think I'd expect this to be a Date type?
| /// Domain-specific fields. Fields are populated only for domain | ||
| /// registration products; all will be `None` for other product types. | ||
| #[serde(flatten)] | ||
| pub domain_info: DomainProductInfo, |
There was a problem hiding this comment.
This should probably be optional, because not all products are domains. When it is, all of the fields can be non-optional because they're only populated for domain-shaped objects.
| #[derive(Debug, Clone, Serialize, Deserialize, uniffi::Record)] | ||
| pub struct IntroductoryOffer { | ||
| /// Unit of the offer interval (e.g. `"month"`). | ||
| pub interval_unit: String, |
There was a problem hiding this comment.
This is actually an enum on the server too:
| Value | Meaning |
|---|---|
"day" |
Daily interval |
"week" |
Weekly interval |
"month" |
Monthly interval |
"year" |
Annual interval |
"indefinite" |
No end / indefinite |
Could have Other just in case but it seems nicely shaped.
229811b to
2b257e8
Compare
jkmassel
left a comment
There was a problem hiding this comment.
echoing @crazytonyli's approval
Implement `GET /products` with optional `type` filter parameter. The `Product` type covers all product variants (domain registrations, transfers, Jetpack plans, etc.) with optional fields for domain-specific data (`tld`, `sale_coupon`, HSTS) and non-domain data (`introductory_offer`, price tiers).
- Product.cost: f64 → Decimal2 - IntroductoryOffer.cost_per_interval: f64 → Decimal2 - SaleCoupon.tld_rank: Option<u32> → Option<f64> to match backend
Accept an optional WPComLanguage locale in ProductsParams to get localized product names and descriptions from the API.
Replace stringly-typed product_type filter with a ProductTypeFilter enum (Domains, Jetpack, Other) to prevent typos that silently return all results instead of a filtered set.
Group domain-specific optional fields (tld, is_privacy_protection_- product_purchase_allowed, is_hsts_required, is_dot_gay_notice_required) into a DomainProductInfo struct, flattened for JSON compatibility.
Match the DerivedRequest trait signature which requires &self.
CurrencyCode (in wp_com module) wraps an ISO 4217 string. ProductId (in products module) wraps a u64 with built-in deserialization from both numeric and string representations, since the API is inconsistent about the encoding.
Since tld and is_privacy_protection_product_purchase_allowed are always present for domain products, make them non-optional. With tld as a required String, serde(flatten) correctly produces None for non-domain products and Some for domain products.
Known values from the backend: month, year, two years, three years, hundred years, one time. Includes Other(String) fallback.
Matches the backend's `Time_Span_Unit` enum: day, week, month, year, indefinite. Includes Other(String) fallback.
The existing date deserializer already handles the MySQL format
("2025-01-01 00:00:00") used by the coupon start_date and expires
fields.
ddbbdf7 to
e022ffa
Compare
Implement
GET /productswith optionaltypefilter parameter. TheProducttype covers all product variants (domain registrations, transfers, Jetpack plans, etc.) with optional fields for domain-specific data (tld,sale_coupon, HSTS) and non-domain data (introductory_offer, price tiers).