Skip to content
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

WIP: Feat/Stripe: Added more object of the stripe products categorie #101

Closed
wants to merge 14 commits into from

Conversation

nielsbrakel
Copy link

@nielsbrakel nielsbrakel commented Jun 2, 2023

NOTE: WIP Just looking for some feedback :)

What kind of change does this PR introduce?

  1. Added new product objects: coupons, promotion_codes, tax_codes, tax_rates and shipping rates

What is the current behavior?

Added more object to make the wrapper more complete

What is the new behavior?

You can now access more information from stripe

Additional context

This is still a WIP so will place some comments on my own code to check some things.

@@ -97,6 +97,8 @@ fn body_to_rows(
.and_then(|v| match *col_type {
"bool" => v.as_bool().map(Cell::Bool),
"i64" => v.as_i64().map(Cell::I64),
"f64" => v.as_f64().map(Cell::F64),
"json" => v.as_i64().map(Cell::Json),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is correct... maybe I should first learn how rust works...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is correct for f64, but for json I think it is better to use string convert in between.

@@ -156,6 +161,8 @@ fn row_to_body(row: &Row) -> JsonValue {
if let Some(m) = v.0.clone().as_object_mut() {
map.append(m)
}
} else {
map.insert(col_name, v);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious if someone can shed some light on this code if this is the correct implementation for normal json values

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks correct, but that requires the col_name matches with Stripe json field, may need have some testing on this.

@burmecia
Copy link
Member

burmecia commented Jun 5, 2023

Thanks for the PR! Happy to see more objects to be added to this FDW, let us know once it is ready.

@burmecia burmecia added the enhancement New feature or request label Jun 5, 2023
@psteinroe
Copy link

hey @nielsbrakel, are you still working on this? I would like to add subscription_items and their usage record summaries to the list of supported objects and am wondering if I should start a new pr, or collaborate on this one?

@nielsbrakel
Copy link
Author

@psteinroe I'm planning to continue working on it, but can not give a date or time.

@olirice
Copy link
Collaborator

olirice commented Jul 30, 2024

closing due to inactivity. please re-open when ready

@olirice olirice closed this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants