Conversation
xlisachan
force-pushed
the
FXA-11929
branch
4 times, most recently
from
a6b5467 to
fec3155
Compare
| const unusedAmountTotal = invoice.lines.data | ||
| .filter((line) => line.amount < 0) | ||
| .reduce((sum, line) => sum + line.amount, 0); |
There was a problem hiding this comment.
invoice.lines can contain a bunch of different things, and IMO it's risky to sum the amounts together on an invoice to get the unusedAmountTotal.
If the intention is to calculate the total proration, maybe consider making use of the invoice.lines.data.proration flag.
| let unitAmount; | ||
| if (invoice.lines.data[invoice.lines.data.length - 1]) { | ||
| unitAmount = Number( | ||
| invoice.lines.data[invoice.lines.data.length - 1] | ||
| .unit_amount_excluding_tax | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is it safe to assume the last item in invoice.lines will always provide the correct value?
xlisachan
force-pushed
the
FXA-11929
branch
4 times, most recently
from
833ab93 to
99f4115
Compare
xlisachan
changed the title
xlisachan
force-pushed
the
FXA-11929
branch
10 times, most recently
from
66aef4b to
de1b35b
Compare
| ...links, | ||
| uid, | ||
| email, | ||
| invoiceAmountDue: this._getLocalizedCurrencyString( |
There was a problem hiding this comment.
🎉 This is great, it looks like we're getting a localized version of this variable!
Is it out of the scope for this patch to also fix $productPaymentCycleOld / $productPaymentCycleNew ?
There was a problem hiding this comment.
Unfortunately it is out of scope for this patch. We have a ticket that includes the improvement mentioned, as well as other updates.
| price.id, | ||
| cart.currency | ||
| ); | ||
| assertNotNull(offeringPrice); |
There was a problem hiding this comment.
Switch this over to an explicit error, if you don't mind. Its easier to debug than AssertionError's are
| ...InvoicePreviewFactory(override), | ||
| oneTimeCharge: faker.number.int({ min: 1, max: 1000 }), | ||
| oneTimeChargeSubtotal: faker.number.int({ min: 1, max: 1000 }), | ||
| amountDue: faker.number.int({ min: 0, max: 1000 }), |
There was a problem hiding this comment.
Please update this to have min: 1, rather than min: 0, as an unexpected value of 0 here can have unintended consequences for how we process transactions
david1alvarez
left a comment
david1alvarez
left a comment
There was a problem hiding this comment.
r+wc, looks great! The changes looked good across the board
…ect amount due Because - the list price is being displayed instead of the price of the offering - on Subscription Management page, the total credit balance is deducted from all subscriptions - totals on Success page reflect upcoming invoice - emails are not displaying correct amount charged This pull request - updates the price of the offering from the list price to the unit amount - temporary removes inaccurate upcoming invoice total from Subscription Management - updates charged amount in email Closes: FXA-11929
4 participants
Because
This pull request
Issue that this pull request solves
Closes: FXA-11929, FXA-11077, FXA-11729
Checklist
Put an
xin the boxes that applyOther information (Optional)
Outline of changes