Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: added endpoint for product details #20
base: main
Are you sure you want to change the base?
feat: added endpoint for product details #20
Changes from 2 commits
d9ea887
83c257b
4e14a81
b908eb4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@vickywane This doesn't need to be tied to notifications. Let's just say that it:
"Retrieve product and pricing information associated with a premium customer subscription."
In Stripe nomenclature, a subscription is the unique pairing of a customer account with a product entity. Products have a pricing attached to them which can be one time, recurring or some combination.
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.
https://stripe.com/docs/billing/prices-guide
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 text adjustment has been made.
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.
Still reads
"
Retrieve notification 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.
@vickywane similar comment here. It does not have to be tied to notifications. We may add other products in the future and this API should still return the product information given a customer subscription.
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.
An adjustment has been made to the
summary
field in the openapi spec to correct this.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.
Strange. This
index.html
file still shows<div class="operation-summary">Retrieve notification product</div>
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.
@vickywane instead of hardcoding the email product id in the deployment environment, let's just retrieve all information about user's subscription options along with product and price id. For now it's only email notifications, but we plan for that list of options to expand.
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 explained in this comment, the renamed
email-product-id
has no direct relation with end-users. The product is created by the organization either through the Stripe API or Stripe dashboard.It is used to identify the product that a user is subscribed to. The product being retrieved is what stores the data.
To handle multiple product-IDs, they can be stored as environment variables with respective names. However, product IDs are currently not stored in a user's meta_data.
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.
Then this method should take a generic product ID argument and return product information details.
EMAIL_PRODUCT_ID should not be hardcoded in the method implementation, but instead passed in as a
productId
argument from the client using thisgetProductInfo
method.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.
@vickywane in line with previous comments, this function name should be probably renamed to getProductInformation()
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.
An adjustment has been made to the
operationId
field in the openapi spec to correct this.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.
@vickywane the change has not been reflected in the README.md file.
The path still links to
docs/DefaultApi.md#getNotificationProduct
instead ofdocs/DefaultApi.md#getProductInfo
.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.
@vickywane another line that needs cleanup
getNotificationProduct
.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 would be reflected in the new
operation-id
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.
@vickywane
DefaultApi.md
still readsRetrieve notification product
forGET /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.
@vickywane I see a lot of references to
getNotificationProduct
. Is the intention to have a method hardwired to getting notification product information or is the goal to have a more genericgetProductInfo
method?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 was yet to push the new updates from my local repository at the time of your review. I only called your attention to this comment.