Conversation
|
Hi @sbejaoui, |
cpemiguelhendrickfmallari-lgtm
left a comment
There was a problem hiding this comment.
Code review.
Both ports check out — #1446's total-value method correctly adds discount and iterates period-by-period (fixes #1429's 3 issues), with tests covering the basic and edge cases. #1415 is a small, low-risk tooltip/help-text fix. Spot-checked the blacklist, including the two flagged edge cases (#1019, #1152) — reasoning is sound. All 6 checks are passing on the current commit.
Branch has conflicts with base that need resolving before merge, so flagging that as the blocker.
Previous implementation had 3 problems: * Discounts were not taken into account * When combined with 'contract_variable_quantity', '_get_quantity_to_invoice()' returns 1 for fixed quantities * When combined with 'contract_variable_quantity', it is wrong to assume that the quantity to invoice of one big period is equal to the quantity to invoice of the sum of smaller chunks. Because of the two last problems, we are forced to iterate over each periods and sum the subtotals. (cherry picked from commit b5ca4cd)
Ports the wording from OCA#1415, which replaced the bare label on this setting with a help text. The setting moved out of `contract` and into this module when the successor code was split off, so the change lands here rather than where oca-port reports it.
The 16.0/17.0/18.0 sweep produced 27 candidates for this addon; one was a genuine gap (OCA#1446, ported here) and one moved module (OCA#1415, ported into contract_line_successor). The rest would be re-proposed on every future run and re-investigated from scratch. Record them where the tool reads them, with the reason attached. Each verdict was checked against the 19.0 code rather than taken from the PR title. OCA#1019 is the one worth noting: it is a clean cherry-pick, but it merges `analytic_account_id` into `analytic_distribution` and that field no longer exists on contract.line, so applying it would be meaningless. real on 19.0 and OCA#1416 is the 18.0 fix for it, currently under review.
37668ce to
7cfa2ce
Compare
|
Thanks @cpemiguelhendrickfmallari-lgtm Conflicts are resolved. |
cpemiguelhendrickfmallari-lgtm
left a comment
There was a problem hiding this comment.
Code and functional review.
Code: both ports look faithful to the originals with correct 19.0 adaptations (self.env._()).
Functional: tested on runboat. The help text shows under "Create new line at contract line renew" and the setting saves correctly. Screenshot below.
LGTM.
|
This PR has the |
Completes the
oca-portsweep of 19.0 against 16.0, 17.0 and 18.0. Companion to #1541, which covered every other addon.Ports
_get_contract_line_total_value()oncontract.line, with its tests. Authorship preserved. Conflicts were the imports and the insertion point only; adapted to 19.0 by usingself.env._()instead of_().contract_line_successor, because that setting moved there when the successor code was split off.oca-portwill keep reporting it undercontract, which is why it is also in the blacklist with that explanation.Blacklist
The sweep produced 27 candidates for
contract. Two were real, both above. The other 25 would be re-proposed on every future run.Each verdict was checked against the 19.0 code:
Two worth singling out:
analytic_account_idintoanalytic_distribution, andanalytic_account_idno longer exists oncontract.line, so applying it would be meaningless. Clean does not mean wanted.message_postcalls now live incontract_line_successorand already wrap the body inMarkup, so the fix arrived through the module split rather than through a port.**#1152 is listed with a pointer, not a flat rejection.
** The defect is real on 19.0
res_partner_view.xmlstill hasinvisible="customer_rank == 0", so contacts created outside Sales cannot reach their contracts and #1416 is the 18.0 fix for it, currently under review.Port it once #1416 settles rather than duplicating a fix that is still being discussed.