-
Notifications
You must be signed in to change notification settings - Fork 2
ID: FPCO-3436; DONE: 100; HOURS: 20; extension billing library webhook support #3
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
base: main
Are you sure you want to change the base?
ID: FPCO-3436; DONE: 100; HOURS: 20; extension billing library webhook support #3
Conversation
.vscode/launch.json
Outdated
| @@ -0,0 +1,25 @@ | |||
| { | |||
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.
remove this file.
handlers/webhook-handler.js
Outdated
| } | ||
|
|
||
| const webhookHandler = () => new WebhookHandler(SubscriptionModel); | ||
| const webhookHandler = (model) => new WebhookHandler(model); |
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.
move constructor to setupBilling function itself. We don't want to create singleton instance on memory directly. Export class itself instead.
index.js
Outdated
|
|
||
| const models = require("./models")(config.db_connection, config.collection_name, config.orm_type); | ||
| const { getActivePlans, subscribePlan, getActiveSubscription, updateSubscriptionStatus } = require("./controllers/subscription.helper")(config, models); | ||
| const { webhookHandler } = require("./handlers/webhook-handler"); |
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.
import webhook handler class here and create instance here only.
index.js
Outdated
| getActiveSubscription, | ||
| updateSubscriptionStatus | ||
| updateSubscriptionStatus, | ||
| webhookHandler: webhookHandler(models.subscriptionModel) |
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.
pass all models here instead of subscription model alone. In future multiple db models update will be handled on webhook handlers. Change accordingly on each handler as well.
| @@ -0,0 +1,32 @@ | |||
| module.exports = { | |||
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.
name fixture file to whichever model fixture is here. Payload is too generic to understanding which db collection fixture it is written here.
handlers/webhook-handler.js
Outdated
| success = true; | ||
| message = "Subscription is cancelled by user"; | ||
| } | ||
| return { |
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.
webhook handler will not return anything. remove return from here.
|
|
||
| payloadFixture.status = "declined"; | ||
|
|
||
| const data = await this.fdk_billing_instance.webhookHandler.handleExtensionSubscriptionUpdate("extension/extension-subscription", payloadFixture, subscriptionFixture.company_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.
verification data should be fetched from database post webhook handler is executed. Since webhook handler will update db records. Fetch data from db instead of this. Update for the same on all test cases.
|
|
||
| const data = await this.fdk_billing_instance.webhookHandler.handleExtensionSubscriptionUpdate("extension/extension-subscription", payloadFixture, subscriptionFixture.company_id); | ||
|
|
||
| expect(data.success).toBeFalse() |
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.
Add status check on each test cases. even for success false check status is unchanged.
handlers/webhook-handler.js
Outdated
| const sellerSubscription = await this.model.getSubscriptionByPlatformId(payload._id, companyId); | ||
| const existingSubscription = await this.model.getActiveSubscription(companyId); | ||
| if (!sellerSubscription) { | ||
| return { |
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.
throw exception here if subscription is not found. there will be no return value on webhook handlers.
| const { clearData } = require("../../helpers/setup_db"); | ||
| const subscriptionFixture = require("../../fixtures/subscription"); | ||
| const ObjectId = require("mongoose").Types.ObjectId; | ||
| const payloadFixture = require("../../fixtures/payload"); |
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.
rename fixture variable name here also
handlers/webhook-handler.js
Outdated
| 'use strict'; | ||
| class WebhookHandler { | ||
| constructor(models) { | ||
| this.model = models.subscriptionModel; |
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.
assign models object itself here. and use this.models.subscriptionModel every place in this file.
handlers/webhook-handler.js
Outdated
| if (existingSubscription) { | ||
| await this.model.cancelSubscription(existingSubscription.id); | ||
| } | ||
| success = true; |
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.
lose these variables if not using it anymore later.
index.js
Outdated
| const models = require("./models")(config.db_connection, config.collection_name, config.orm_type); | ||
| const { getActivePlans, subscribePlan, getActiveSubscription, updateSubscriptionStatus } = require("./controllers/subscription.helper")(config, models); | ||
| const { WebhookHandler } = require("./handlers/webhook-handler"); | ||
| const webhookHandler = (models) => new WebhookHandler(models); |
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.
why we need function here? why can't we just create new instance directly? Think properly before adding extra unnecessary steps.
… invoked with instance
extension billing library webhook support