Initial fix for 1869 #1877
Initial fix for 1869 #1877
Conversation
| @@ -744,7 +744,7 @@ private DateTime alignToNextBCDIfRequired(final Plan curPlan, final PlanPhase cu | |||
| } | |||
|
|
|||
| final BillingPeriod billingPeriod = curPlanPhase.getRecurring() != null ? curPlanPhase.getRecurring().getBillingPeriod() : BillingPeriod.NO_BILLING_PERIOD; | |||
| final LocalDate resultingLocalDate = BillCycleDayCalculator.alignProposedNextBillCycleDate(originalTransitionDate, bcd, billingPeriod, context); | |||
| final LocalDate resultingLocalDate = BillCycleDayCalculator.alignProposedNextBillCycleDate(originalTransitionDate, lastTransitionDate, bcd, billingPeriod, context); | |||
There was a problem hiding this comment.
Unrelated to your change, but the name are so confusing. Would it make sense to rename
originalTransitionDate->curTransitionDate# Really the transition time associated with thePlan#EffectiveDateForExistingSubscriptionslastTransitionDate->prevTransitionDate
And perhaps swap the 2 in the method signature?
| @@ -76,22 +76,27 @@ public static LocalDate alignProposedBillCycleDate(final DateTime proposedDate, | |||
| } | |||
|
|
|||
|
|
|||
| public static LocalDate alignProposedNextBillCycleDate(final LocalDate proposedDate, final int billingCycleDay, final BillingPeriod billingPeriod) { | |||
| public static LocalDate alignProposedNextBillCycleDate(final LocalDate proposedDate, final LocalDate originalDate, final int billingCycleDay, final BillingPeriod billingPeriod) { | |||
There was a problem hiding this comment.
Same thing here - rename and swap:
originalDate->prevTransitionDateproposedDate->curTransitionDate
There was a problem hiding this comment.
What about we also change the method name to alignProposedNextBillCycleDate -> alignToNextBillCycleDate. It would take as a first argument prevTransitionDate, and then curTransitionDate with a comment /* date to be aligned */ ?
| @@ -172,7 +172,7 @@ public void testAlignProposedNextBillCycleDate1() { | |||
| // BCD > proposed day of the month | |||
There was a problem hiding this comment.
Could we add a few unit tests here to cover for non monthly billing periods - at least the one matching the scenario?
|
@reshmabidikar I am merging your PR as-is - optionally we could have another small one to clean things up and address the comments. |
Initial fix for #1869