[19.0][ADD] contract_invoice_offset - #1366
Conversation
4dc2053 to
00d86c8
Compare
8b9a854 to
8c8b9c7
Compare
bosd
changed the title
e6f8429 to
65eeb15
Compare
a9fc61a to
731c908
Compare
ab08faa to
6348343
Compare
778a5bd to
114f847
Compare
114f847 to
0fa76e3
Compare
OCA-git-bot
added
series:19.0
mod:contract_invoice_offset
Fix _get_period_to_invoice returning (date, False, date) instead of (False, False, False) when get_next_period_date_end returns False for ended contracts. The guard `if last_date_invoiced and ...` short-circuited on False; changed to `if not last_date_invoiced or ...`.
0fa76e3 to
3fb11cc
Compare
anthonissen-a
left a comment
anthonissen-a
left a comment
There was a problem hiding this comment.
Very useful module! My main concern is compatibility with other modules, given the number of overwrites.
| "license": "AGPL-3", | ||
| "depends": [ | ||
| "contract", | ||
| "contract_invoice_align_start", |
There was a problem hiding this comment.
IMO this dependency should be removed to be more generic.
There was a problem hiding this comment.
Done — contract_invoice_align_start is off the depends list, and the align_billing_cycle plumbing (the _get_offset_kwargs() entry and the api.depends on it) is gone with it. _get_offset_kwargs() stays as the extension point, so a glue module can add the alignment back for installations that want both.
Side effect: this was the last unmerged dependency in the chain (it pulled in contract_variable_qty_prorated too), so the [DO NOT MERGE] commit with the test-requirements.txt pins is dropped and the PR only needs contract now.
There was a problem hiding this comment.
Reopening this one — I tried it and had to back it out.
Dropping contract_invoice_align_start from depends is not just a manifest change: align_billing_cycle is that module the offset chain currently rides on. Without it installed, the contract falls back to min(line.recurring_next_date) and the line-level pre-paid/post-paid values win over the contract-level ones, so test_postpaid_delayed_one_month and test_days_fallback both shift by a full period. The offset behaviour is deliberately crafted here, so I would rather not change it as a side effect of a dependency cleanup.
Happy to make it generic, but it needs to be its own piece of work: the alignment kwarg has to move out of _get_offset_kwargs() and into a glue module with tests covering both the with- and without-alignment cases. Do you want that as a follow-up PR, or folded into the refactor you are doing in #1498?
3fb11cc to
6caf79f
Compare
- Name the offset units after the recurrence rule types already used in `contract` (daily/weekly/monthly/yearly), so the offset can be turned into a `relativedelta` with the existing `get_relative_delta` helper instead of a hand written if/elif chain. - `get_next_invoice_date` and `get_next_period_date_end` now call `super()` and only apply (respectively reverse) the flexible offset, instead of reimplementing the base logic. This drops the duplicated billing cycle alignment branch, which `contract_invoice_align_start` already implements in its own override of the same method. - Collect the extra keyword arguments in `_get_offset_kwargs()`, an extension point for modules adding their own arguments to the recurrence helpers. - Say "overwrite" where the base method is replaced rather than extended. - Cover the remaining branches with tests: header level offset, weekly and yearly units, next period end, empty period, line level offset precedence and the missing next invoice date.
6caf79f to
b1c6fc1
Compare
cda53eb to
b1c6fc1
Compare
`contract` leaves `line_recurrence` False by default, which makes a line take its recurrence settings from the contract header. The tests relied on that default to check that a header level offset reaches the lines. `contract_line_successor`, split out of `contract` and merged since, redefines the field with `default=True`. Installed alongside, it flips the default, the lines keep their own pre-paid settings and `test_postpaid_delayed_one_month` and `test_days_fallback` shift by a full period. Set `line_recurrence` explicitly in the fixture so the tests state the configuration they exercise instead of depending on which sibling modules happen to be installed. No behaviour change in the module itself.
b1c6fc1 to
4035deb
Compare
3 participants
depends on #1372 and #1365
(#1312 and #1364 are merged.)