feat: added endpoint for product details #20

vickywane · 2021-06-14T10:32:58Z

This pull request closes this issue.

This PR introduces a GET operation into the /notification endpoint to fetch the email notification product created on Stripe. This operation would be further used by the Premium services feature within the Ambianic PWA.

commit-lint · 2021-06-14T10:33:01Z

Features

added endpoint for product details (d9ea887)

Update

renamed GET /notification to /product (83c257b)
adjusted /product documentation (b908eb4)

Bug Fixes

fixed merge conflicts from previous branch (4e14a81)

Contributors

vickywane

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

@Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
@Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
@Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
@Commit-Lint merge disable will desactivate merge dependabot PR
@Commit-Lint review will approve dependabot PR
@Commit-Lint stop review will stop approve dependabot PR

netlify · 2021-06-14T10:33:04Z

✔️ Deploy Preview for hopeful-bell-0b6f9f ready!

🔨 Explore the source changes: b908eb4

🔍 Inspect the deploy log: https://app.netlify.com/sites/hopeful-bell-0b6f9f/deploys/60d07480f84f1d00070a01d9

😎 Browse the preview: https://deploy-preview-20--hopeful-bell-0b6f9f.netlify.app

ivelin

@vickywane should the name of the endpoint be /product to better represent the intended use. It is confusing to think of POST /notification as a way to send an individual notification message and at the same time GET /notification fetches metadata about the notification product.

A more intuitive use for GET /notification would be to fetch the latest sent notification if we ever needed that historical data access.

vickywane · 2021-06-14T18:26:03Z

A more intuitive use for GET /notification would be to fetch the latest sent notification if we ever needed that historical data access.

@ivelin Good catch!

This same thought ran through my mind while creating the endpoint today. I however wanted to know your thoughts on it also. I would make the change and push back.

lgtm-com · 2021-06-14T22:54:37Z

This pull request fixes 1 alert when merging 83c257b into 6067d3d - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

ivelin

@vickywane a few semantic naming comments.

README.md

ivelin · 2021-06-15T14:50:17Z

docs/index.html

+                            <a href="#operation--product-get">GET</a>
+                        </td>
+                        <td>
+                            <p>Retrieve notification product</p>


@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.

https://stripe.com/docs/billing/prices-guide

This text adjustment has been made.

Still reads
"
Retrieve notification product
"

ivelin · 2021-06-15T14:51:37Z

docs/index.html

+        <span id="path--product"></span>
+            <div id="operation--product-get" class="swagger--panel-operation-get panel">
+                <div class="panel-heading">
+                    <div class="operation-summary">Retrieve notification product</div>


@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.

An adjustment has been made to the summary field in the openapi spec to correct this.

Strange. This index.html file still shows <div class="operation-summary">Retrieve notification product</div>

netlify/functions/notification.js

ivelin · 2021-06-15T15:00:07Z

netlify/functions/product.js

+  } else if (event.httpMethod === "GET") {
+
+    try {
+      const product = await stripe.products.retrieve(process.env.EMAIL_PRODUCT_ID);


@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.

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.

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 this getProductInfo method.

ivelin · 2021-06-15T18:31:46Z

openapi-docs/README.md

@@ -127,17 +127,21 @@ Class | Method | HTTP request | Description
 ------------ | ------------- | ------------- | -------------
 *AmbianicCloudApiCollection.DefaultApi* | [**createSubscription**](docs/DefaultApi.md#createSubscription) | **POST** /subscription | Subscribe a user to Ambianic&#39;s Premium Services
 *AmbianicCloudApiCollection.DefaultApi* | [**deleteSubscription**](docs/DefaultApi.md#deleteSubscription) | **DELETE** /subscription | Delete an Ambianic&#39;s user subscription
+*AmbianicCloudApiCollection.DefaultApi* | [**getNotificationProduct**](docs/DefaultApi.md#getNotificationProduct) | **GET** /product | Retrieve notification product


@vickywane in line with previous comments, this function name should be probably renamed to getProductInformation()

An adjustment has been made to the operationId field in the openapi spec to correct this.

@vickywane the change has not been reflected in the README.md file.
The path still links to docs/DefaultApi.md#getNotificationProduct instead of docs/DefaultApi.md#getProductInfo.

lgtm-com · 2021-06-17T04:47:42Z

This pull request fixes 1 alert when merging 6f40ab2 into a31218f - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

ivelin · 2021-06-17T20:28:43Z

docs/index.html

+                            <a href="#operation--product-get">GET</a>
+                        </td>
+                        <td>
+                            <p>Retrieve notification product</p>


Still reads
"
Retrieve notification product
"

ivelin · 2021-06-17T20:29:29Z

docs/index.html

+        <span id="path--product"></span>
+            <div id="operation--product-get" class="swagger--panel-operation-get panel">
+                <div class="panel-heading">
+                    <div class="operation-summary">Retrieve notification product</div>


Strange. This index.html file still shows <div class="operation-summary">Retrieve notification product</div>

ivelin · 2021-06-17T20:34:01Z

netlify/functions/product.js

+  } else if (event.httpMethod === "GET") {
+
+    try {
+      const product = await stripe.products.retrieve(process.env.EMAIL_PRODUCT_ID);


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 this getProductInfo method.

vickywane · 2021-06-18T07:26:16Z

@vickywane In my most recent commit, i updated the bootprint docs alongside the /product route to receive a productId parameter and use it in fetching a specific product.

lgtm-com · 2021-06-18T07:26:54Z

This pull request fixes 1 alert when merging 11d303f into a31218f - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

ivelin

Needs clean up.

ivelin · 2021-06-18T14:54:57Z

README.md

@@ -23,7 +23,7 @@ STRIPE_KEY=STRIPE_KEY

 # The ID of the email price product used in billing subscribers.
 EMAIL_PRODUCT_PRICE_ID=EMAIL_PRODUCT_PRICE_ID


@vickywane is this doc line still necessary? We do not hardcode product ID in the API.

For now, yes it is.

The EMAIL_PRODUCT_PRICE_ID that is used to retrieve only the pricing entity is different from the EMAIL_PRODUCT_ID.

The EMAIL_PRODUCT_PRICE_ID is used in the /subscription function when creating a subscription. The product ID response retrieved using the /product function does not include the ID of the pricing entity.

See subscription and price docs.

I would further rephrase the description of the EMAIL_PRODUCT_PRICE_ID to better reflect it's use

docs/index.html

ivelin · 2021-06-18T14:59:10Z

docs/index.html

+                            <a href="#operation--product-get">GET</a>
+                        </td>
+                        <td>
+                            <p>Retrieve product and pricing information associated with a premium customer subscription.</p>


@vickywane this description is misleading. If the method returns information associated with a subscription, then the arguments to the method should be user id or subscription id. Needs clean up.

Alright.

You however requested me to use that description here.

Yes, I thought at the time that we would pass user subscription ID as an argument, but later I realized your intention is to pass product ID as an argument to this GET /product method.

docs/index.html

ivelin · 2021-06-20T14:49:38Z

docs/index.html

+                            <a href="#operation--product-get">GET</a>
+                        </td>
+                        <td>
+                            <p>Retrieve product and pricing information associated with a premium customer subscription.</p>


Yes, I thought at the time that we would pass user subscription ID as an argument, but later I realized your intention is to pass product ID as an argument to this GET /product method.

ivelin · 2021-06-20T14:51:44Z

docs/index.html

+                </div>
+                <div class="panel-body">
+                    <section class="sw-operation-description">
+                        <p>An endpoint to retrieve details about Ambianic product.</p>


@vickywane let's clarify here:
"An endpoint to retrieve details about an Ambianic premium subscription product."

Alright. I made this adjustment

ivelin · 2021-06-20T14:52:27Z

openapi-docs/README.md

@@ -127,17 +127,21 @@ Class | Method | HTTP request | Description
 ------------ | ------------- | ------------- | -------------
 *AmbianicCloudApiCollection.DefaultApi* | [**createSubscription**](docs/DefaultApi.md#createSubscription) | **POST** /subscription | Subscribe a user to Ambianic&#39;s Premium Services
 *AmbianicCloudApiCollection.DefaultApi* | [**deleteSubscription**](docs/DefaultApi.md#deleteSubscription) | **DELETE** /subscription | Delete an Ambianic&#39;s user subscription
+*AmbianicCloudApiCollection.DefaultApi* | [**getNotificationProduct**](docs/DefaultApi.md#getNotificationProduct) | **GET** /product | Retrieve notification product


@vickywane the change has not been reflected in the README.md file.
The path still links to docs/DefaultApi.md#getNotificationProduct instead of docs/DefaultApi.md#getProductInfo.

ivelin · 2021-06-20T14:53:53Z

openapi-docs/docs/DefaultApi.md

@@ -6,6 +6,7 @@ Method | HTTP request | Description
 ------------- | ------------- | -------------
 [**createSubscription**](DefaultApi.md#createSubscription) | **POST** /subscription | Subscribe a user to Ambianic&#39;s Premium Services
 [**deleteSubscription**](DefaultApi.md#deleteSubscription) | **DELETE** /subscription | Delete an Ambianic&#39;s user subscription
+[**getNotificationProduct**](DefaultApi.md#getNotificationProduct) | **GET** /product | Retrieve notification product


@vickywane another line that needs cleanup getNotificationProduct.

This would be reflected in the new operation-id

ivelin · 2021-06-20T14:55:31Z

openapi-docs/docs/DefaultApi.md

@@ -117,6 +118,55 @@ No authorization required
 - **Accept**: application/json


+## getNotificationProduct


@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 generic getProductInfo method?

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.

ivelin · 2021-06-20T14:58:48Z

netlify/functions/product.js

+  "Content-Type": "application/json",
+};
+
+exports.handler = async ({ httpMethod, queryStringParameters }, context, callback) => {


@vickywane I see that the implementation is now generic and works for any valid Product ID. Therefore the JS function name should match that behavior and be named getProductInfo as opposed to the more narrow getNotificationProduct naming.

Alright.

I have adjusted the operation-id to effect this change.

ivelin · 2021-06-20T14:59:39Z

openapi-docs/docs/DefaultApi.md

@@ -6,6 +6,7 @@ Method | HTTP request | Description
 ------------- | ------------- | -------------
 [**createSubscription**](DefaultApi.md#createSubscription) | **POST** /subscription | Subscribe a user to Ambianic&#39;s Premium Services
 [**deleteSubscription**](DefaultApi.md#deleteSubscription) | **DELETE** /subscription | Delete an Ambianic&#39;s user subscription
+[**getNotificationProduct**](DefaultApi.md#getNotificationProduct) | **GET** /product | Retrieve notification product


@vickywane DefaultApi.md still reads Retrieve notification product for GET /product.

lgtm-com · 2021-06-21T11:17:07Z

This pull request fixes 1 alert when merging b908eb4 into a31218f - view on LGTM.com

fixed alerts:

1 for Unused variable, import, function or class

feat: added endpoint for product details

d9ea887

vickywane self-assigned this Jun 14, 2021

vickywane requested a review from ivelin June 14, 2021 10:33

ivelin requested changes Jun 14, 2021

View reviewed changes

update: renamed GET /notification to /product

83c257b

pull-request-size bot added the size/XXL label Jun 14, 2021

ivelin requested changes Jun 15, 2021

View reviewed changes

fix: fixed merge conflicts from previous branch

4e14a81

vickywane requested a review from ivelin June 17, 2021 04:44

ivelin requested changes Jun 17, 2021

View reviewed changes

vickywane requested a review from ivelin June 18, 2021 14:38

ivelin requested changes Jun 18, 2021

View reviewed changes

ivelin requested changes Jun 20, 2021

View reviewed changes

update: adjusted /product documentation

b908eb4

vickywane force-pushed the feat/product-detail branch from 11d303f to b908eb4 Compare June 21, 2021 11:14

		@@ -23,7 +23,7 @@ STRIPE_KEY=STRIPE_KEY

		# The ID of the email price product used in billing subscribers.
		EMAIL_PRODUCT_PRICE_ID=EMAIL_PRODUCT_PRICE_ID

		@@ -117,6 +118,55 @@ No authorization required
		- Accept: application/json


		## getNotificationProduct

feat: added endpoint for product details #20

Are you sure you want to change the base?

feat: added endpoint for product details #20

Conversation

vickywane commented Jun 14, 2021

commit-lint bot commented Jun 14, 2021 • edited Loading

Features

Update

Bug Fixes

Contributors

netlify bot commented Jun 14, 2021 • edited Loading

ivelin left a comment

Choose a reason for hiding this comment

vickywane commented Jun 14, 2021

lgtm-com bot commented Jun 14, 2021

ivelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vickywane Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vickywane commented Jun 18, 2021

lgtm-com bot commented Jun 18, 2021

ivelin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 21, 2021

commit-lint bot commented Jun 14, 2021 •

edited

Loading

netlify bot commented Jun 14, 2021 •

edited

Loading

vickywane Jun 17, 2021 •

edited

Loading