Changes for fixed proration #1942
Conversation
sbrossie
left a comment
There was a problem hiding this comment.
Looks good, but I based on your TODO, I think we need to double check about the Item class.
| final Item[] result = new Item[2]; | ||
|
|
||
| final BigDecimal amount0 = InvoiceDateUtils.calculateProrationBetweenDates(startDate, splitDate, Days.daysBetween(startDate, endDate).getDays()).multiply(amount); | ||
| final BigDecimal amount0 = InvoiceDateUtils.calculateProrationBetweenDates(startDate, splitDate, Days.daysBetween(startDate, endDate).getDays(), 0).multiply(amount); //TODO_fixed_proration - setting to 0, revisit |
There was a problem hiding this comment.
I think we will need to implement it here as well. I would suggest to write 2 beatrix tests:
- One that mimic the current problem being reported, i.e a use case of leading pro-ration (I think you already had it); it should work
- One that implements a use case of trailing pro-ration (i.e cancelation before the EOT); I suspect this is where it may break until we fix
Item.
sbrossie
left a comment
There was a problem hiding this comment.
This is great, I just have a few comments around code organization and specifically passing or not the InvoiceConfig. Take a look and let me know what you think.
| import org.killbill.billing.util.currency.KillBillMoney; | ||
| import org.killbill.commons.utils.annotation.VisibleForTesting; | ||
|
|
||
| public class InvoiceDateUtils { |
There was a problem hiding this comment.
This class currently only offers static method, because there is no state that is kept. However, now that we need the config (or at least one param of the config), this makes less sense. It would be better to have a real instance - perhaps injected with guice, that takes a CTOR with the InvoiceConfig, in such a way that we don't mix request parameters (startDate, ...) and static state from the system (config).
- Could you experiment to see if this is not too much trouble doing this - I think the result would be much cleaner
- If this proves difficult, then at least let's not pass the
InvoiceConfig, but instead just the Integer for theprorationFixedDays
There was a problem hiding this comment.
As discussed, I've updated the methods in InvoiceDateUtils to accept prorationFixedDays instead of invoiceConfig.
| return daysBetween; | ||
| } | ||
| final int lastDayOfMonth = startDate.dayOfMonth().withMaximumValue().getDayOfMonth(); | ||
| if (lastDayOfMonth == fixedDaysInMonth) { |
There was a problem hiding this comment.
The if is not even needed because your else would return daysBetween in this case.
There was a problem hiding this comment.
Sorry, but I do not follow this comment, which if statement are you referring to?
There was a problem hiding this comment.
The one on line 99 above my comment. What I am saying is that:
if (lastDayOfMonth == fixedDaysInMonth) {
return daysBetween;
} else {
return daysBetween - (lastDayOfMonth - fixedDaysInMonth);
}
Is the same as: return daysBetween - (lastDayOfMonth - fixedDaysInMonth); so we could simplify - no need for if ... else
| if (lastDayOfMonth == fixedDaysInMonth) { | ||
| return daysBetween; | ||
| } else { | ||
| return daysBetween - (lastDayOfMonth - fixedDaysInMonth); |
There was a problem hiding this comment.
It's 🎉 that it works for both when lastDayOfMonth > fixedDaysInMonth or the reverse (and even the equality has commented above).
| this.isBuilt = false; | ||
| this.allExistingItems = new LinkedList<InvoiceItem>(); | ||
| this.pendingItemAdj = new LinkedList<InvoiceItem>(); | ||
| this.invoiceConfig = invoiceConfig; |
There was a problem hiding this comment.
Similar remark as for the class InvoiceDateUtils. Here we don't offer static method, but these objects are created on the fly with request parameters, and I think it would be best to mix the InvoiceConfig - not that most methods would not be available anyways because they need a context, which we don't pass. So here, instead I suggest to pass the integer for prorationFixedDays.
There was a problem hiding this comment.
As discussed, I've updated AccountItemTree (and the other related classes) to accept prorationFixedDays instead of invoiceConfig.
| final InvoicePruner invoicePruner = new InvoicePruner(existingInvoices); | ||
| final Set<UUID> toBeIgnored = invoicePruner.getFullyRepairedItemsClosure(); | ||
| final AccountItemTree accountItemTree = new AccountItemTree(account.getId(), invoiceId); | ||
| final AccountItemTree accountItemTree = new AccountItemTree(account.getId(), invoiceId, config.getProrationFixedDays()); |
There was a problem hiding this comment.
src/main files - for tests, it's ok to use the other form.
Changes for fixed proration:
org.killbill.invoice.proration.fixed.daysTestInvoiceDateUtilsWithFixedProrationandTestInvoiceDateUtilsWithoutFixedProrationNeed your input on the following:
The Item class also invokes the
InvoiceDateUtils.calculateProrationBetweenDateshere. Since this class does not have access toInvoiceConfig, I have passed the value0corresponding tofixedDaysInMonth. See this comment. I did not spend much time investigating what this class does and whether it requires reading the config parameter but if you think this is important, I'll look into it. Let me know.