Add pricing commands - #4
Conversation
![sigit-code[bot]](https://avatars.githubusercontent.com/in/3492899?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a new smbcloud-ascapi-pricing crate that wraps two App Store Connect endpoints — GET /v1/appPriceSchedules/{id} and the manual/automatic prices collections — as extension traits on the shared Client. The core logic is in app_price.rs: it paginates the collection, sends include=appPricePoint,territory on every request, and joins the three JSON:API resources into a flat TerritoryPrice per territory. The app_price_schedule.rs module fetches the schedule's base territory. CLI wires up two subcommands (app-prices schedule and app-prices list) that delegate to these traits. Tests cover the join, unknown-type tolerance, scheduled end-dates, and the URL-stripping helper.
Automated review by siGit Code Review · commit fa697dc
|
|
||
| // `links.next` already carries include/limit/filter and a | ||
| // cursor, so following it must not re-append our own query. | ||
| let Some(next) = doc.links.and_then(|links| links.next) else { |
There was a problem hiding this comment.
warning — When links.next is present but path_and_query returns None (e.g. Apple changes the URL shape), the loop silently drops all remaining pages instead of surfacing an error. The comment acknowledges this but the consequence — silently truncated price lists — is likely worse than returning an error.
| fn join(doc: &PricesDocument) -> Vec<TerritoryPrice> { | ||
| let mut points: HashMap<&str, &PricePointAttributes> = HashMap::new(); | ||
| let mut currencies: HashMap<&str, Option<&str>> = HashMap::new(); | ||
| for resource in &doc.included { |
There was a problem hiding this comment.
warning — join iterates doc.included even when it is empty (the common fast path is fine), but more importantly it borrows string slices from doc via HashMap<&str, …> and then calls collect() on doc.data, so the borrow checker approves — but if included ever contained a resource whose id is not UTF-8 stable across pages, the HashMap key lifetime tied to the local doc is fine only within one call. No actual bug here given ownership, but the IncludedResource variant that carries only #[serde(other)] and no id field means unknown resources never pollute the maps — confirm that serde does not attempt to deserialise the id field for the Unknown arm (it does not, since other absorbs the whole object; this is correct).
| /// better than a panic on a link Apple changed the shape of. | ||
| fn path_and_query(url: &str) -> Option<String> { | ||
| let after_scheme = url.split_once("://")?.1; | ||
| let (_host, rest) = after_scheme.split_once('/')?; |
There was a problem hiding this comment.
warning — path_and_query splits on the first / after the host, so a URL whose path begins with // (double-slash redirect, unlikely but possible) would silently eat the leading slash and produce a malformed path passed to Client::request. A more robust parse would use a proper URL library already available via reqwest's re-export of url::Url.
| .map(str::to_string), | ||
| customer_price: point.and_then(|p| p.customer_price.clone()), | ||
| proceeds: point.and_then(|p| p.proceeds.clone()), | ||
| manual: price.attributes.manual.unwrap_or(false), |
There was a problem hiding this comment.
nit — price.attributes.manual.unwrap_or(false) silently treats a missing manual field (e.g. on an automatic-price row that Apple decides to omit it from) as false; a caller using PriceKind::Manual would then see manual: false on every row, which is misleading. Consider preserving Option<bool> in TerritoryPrice or at least deriving the value from the PriceKind the caller already passed.
| #[serde(rename_all = "camelCase")] | ||
| struct ScheduleRelationships { | ||
| #[serde(default)] | ||
| base_territory: Option<crate::app_price::ToOne>, |
There was a problem hiding this comment.
nit — ScheduleRelationships reaches into crate::app_price::ToOne (marked pub(crate)) rather than defining a local or shared type. This creates a hidden coupling between the two modules; if app_price::ToOne is ever changed or made private the schedule module breaks silently at the module boundary.
1 participant
No description provided.