Repository navigation
Add promo code support to billing API - #685
alisawavezen12 wants to merge 13 commits into
Conversation
neSpecc
left a comment
neSpecc
left a comment
There was a problem hiding this comment.
Overall, the promo logic itself mostly works and is covered by tests, but a few things need to change before merging.
1. The PR changes the regular payment flow, not only promo. Regular /pay and /recurrent were rewritten (see the inline comments). This is the main risk, so please revert them to the master logic. If regular payments need idempotency or handling of out-of-order callbacks, let's do that in a separate PR with its own tests. The existing integration tests in test/integration/cases/billing pass, but they don't cover subscription replacement and cancellation, so green CI doesn't prove the regular flow is safe.
2. Suggested shape of the feature. A promo code should only change the price and reuse the existing flow:
composePayment: get a quote from a promo code service and putpromo: { ... }into the checksum;/check:expectedAmount = data.promo ? data.promo.finalAmount : plan.monthlyCharge, plus a check that the code can still be used. The business operation is created through the factory as usual;/pay: the regular flow plusif (data.promo) redeem(...)(safe to repeat thanks to the unique index). The receipt uses the amount actually paid;/fail: no changes.
DB access should go through PromoCodesFactory / PromoCodeRedemptionsFactory, like the other models, and domain functions shouldn't receive req / res. This removes finishPromoPayment and its own lease mechanism, which duplicates /pay and /fail (plan change, unblock, notifications, receipt, card saving) and will drift from them with every change.
3. payWithCard ignores the promo code. It charges the full price, so /check rejects the payment with WRONG_AMOUNT. Garage doesn't call it now, but let's explicitly reject promo checksums there.
neSpecc
left a comment
neSpecc
left a comment
There was a problem hiding this comment.
Thanks, all previous comments are addressed. The regular /check, /pay, /fail and /recurrent flow now matches master apart from the separate data.promo blocks, and the promo logic goes through the factories.
A few things left, see inline. The main one is payload.userId in cloudpayments.ts, the rest are naming and descriptions.
| amount: +body.Amount * PENNY_MULTIPLIER, | ||
| currency: body.Currency, | ||
| userId: member._id, | ||
| userId: data.promo ? member.userId : member._id, |
There was a problem hiding this comment.
Now the same field means different things depending on the promo code: for regular payments it's the team membership _id (as in master), for promo payments it's the user id.
In this repo, userId of a plan purchase is read only by the new validatePromoOperation(). I'd keep member._id for all payments and drop the userId comparison there: transactionId, workspace, plan and amount are enough to match the operation, and the redemption already gets the user id from the signed checksum. If we want to store the real user id, let's change it for all payments in a separate PR.
| * | ||
| * @param payment - verified signed quote and provider callback fields | ||
| */ | ||
| public async ensurePaymentAvailable(payment: PromoPayment): Promise<void> { |
There was a problem hiding this comment.
It's hard to tell from the name what "payment available" means, and the description explains how the method works rather than what it's for. Something like ensurePromoCodeStillValid() with "Re-check in /check that the paid amount matches the signed price and the code can still be used for this plan, payer and workspace".
| * Read legacy percentage documents without changing stored data. | ||
| * | ||
| * @param query - indexed code or identifier lookup | ||
| * @param tariffPlanId - selected plan | ||
| */ | ||
| private async findActivePromoCode(query: Filter<PromoCode>, tariffPlanId: ObjectId): Promise<PromoCode> { | ||
| const promoCode = await this.collection.findOne(query); | ||
|
|
||
| ensureActivePromoCode(promoCode, tariffPlanId); | ||
|
|
||
| /* | ||
| * shortcut: support discountPercent until existing documents have been migrated to discount. | ||
| */ | ||
| const legacy = promoCode as PromoCode & { discountPercent?: number }; | ||
|
|
||
| if (promoCode.discount === undefined && typeof legacy.discountPercent === 'number') { | ||
| return { | ||
| ...promoCode, | ||
| discount: { | ||
| type: 'percent', | ||
| value: legacy.discountPercent, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The promoCodes collection is created by this PR, so production has no discountPercent documents. If there are some on stage, let's update them by hand and remove this shim.
Then the description can say what the method is for, e.g. "Find a promo code that can be applied to the selected plan, or throw".
| * @param identity - payer and paid workspace | ||
| * @param transactionId - provider transaction on callback retries | ||
| */ | ||
| public async ensurePromoCodeAvailable( |
There was a problem hiding this comment.
This method has the same name as the pure ensurePromoCodeAvailable() from promoCodes/redemptions.ts that it calls, so at a call site it's unclear which one is used. Maybe rename the pure one, e.g. validatePromoCodeUsage().
Also, the description says what the method doesn't do instead of what it's for. Something like "Throw if this payer or workspace has already used the code, or the code has reached maxUses".
| @@ -0,0 +1,151 @@ | |||
| import { ObjectId } from 'mongodb'; | |||
There was a problem hiding this comment.
After the refactoring this file contains promo payment types and validators, while redemptions live in PromoCodeRedemptionsFactory. A name like promoCodes/validation.ts would match the content better.
| payload.workspaceId.toString() !== payment.workspaceId || | ||
| payload.userId.toString() !== payment.userId || | ||
| payload.tariffPlanId.toString() !== payment.tariffPlanId || | ||
| payload.amount !== payment.paidAmount * 100 || payload.currency !== payment.currency) { |
There was a problem hiding this comment.
Let's use PENNY_MULTIPLIER instead of 100, like cloudpayments.ts does when it creates this operation (it would need to be moved to a shared place).
| /** | ||
| * Retry a promo callback whose successful usage could not be persisted. | ||
| */ | ||
| RETRY = 1, |
There was a problem hiding this comment.
CloudPayments describes only code: 0 for Pay notifications, and the retry is triggered by the non-200 HTTP status, not by the code. Also, PayCodes is shared by all payments, while the description ties this value to promo codes.
I'd drop RETRY and keep res.status(500) with a comment at the call site in /pay.
| promo?: PromoPaymentData; | ||
| /** | ||
| * Subscription start date signed by composePayment for plan purchases and card linking. | ||
| * Kept outside promo because regular payments use it too; promo /check also |
There was a problem hiding this comment.
Regular payments don't read nextPaymentDate: composePayment only signs it into the checksum, and the only reader is validatePromoSubscription(). Let's describe what the field is, e.g. "Start date of the subscription created after this payment", without the claim about regular payments.
Add promo code validation, plan restrictions, and discounted payment amounts. Reserve promo codes for payments and handle successful and failed payment callbacks.
Related changes in Web: codex-team/hawk.garage#927