[12.0][FIX] account_asset_management: asset table wrong leapyear calc

[luc-demeyer]

This PR fixes the following 'leap year' calculation error:

Consider an asset of 5000 EUR, depreciated of 5 years, pro-rata temporis, linear depreciation.
Fiscal year terminates at june 30.
Asset start date is 2015-01-28.
The pro-rata temporis calc should result in the following depreciation amounts:
2015-06-30: 421.92 EUR
2016-06-30 up to 20106-30 : 1000 EUR
2020-06-30: 578,08 EUR

But is goes wrong (without this patch) because 2016 is a leap year.

[luc-demeyer]

One more remark:
I'll add a unit test for this scenario in one of the coming days.

[pedrobaeza]

@JordiBForgeFlow I think you changed this algorithm, isn't it?

[luc-demeyer]

This part of the code have not been touched by the changes of @JordiBForgeFlow
Imho it's a scenario that has always been wrong.

[luc-demeyer]

All unit tests are green and new unit test added for deviating fiscal years with leap year in the table.
I think this one is ready for merge.

[mmotahar]

cy_days = calendar.isleap(fy) and 366 or 365

[mmotahar]

[mmotahar]

Build wasn't successful in Odoo.sh. See the error.

[mmotahar]

[luc-demeyer]

@mromdhane
can you give me the exact setup (fiscal year settings, asset definitions, ... ) that cause this error so that I can reproduce on my local test environment ?

[luc-demeyer]

@mmotahar-ia
It looks like you are not running with this patch. The code linenumbers of the traceback do not correspond with the code base of this PR.

[hioppolo-ia]

Monthly depreciation is calculating correctly, with the exception of the first entry on some assets which is showing as negative.

From my test cases, this happens when fiscal year is set to 1 Jul – 30 June and the start date of the asset is on the 31st of any month.

[luc-demeyer]

@hioppolo-ia
This is now fixed.
The error came from the way the first period amount was calculated.
Old calc: number of full periods - FY amount
New calc: line_days * days_amount (same calc as asset with 'days_calc' set)

When you start your fiscal year at july 1st you have 184 in your first half FY and only 181 in the second half. The old calculation resulted in a negative amount when there was only one day in the first depreciation period.

The new calculation in this PR is better but leads of course to a different result for the first period in case of 'prorata temporis' without days_calc. The FY amount remains untouched, only the spreading over the FY periods is different. As a consequence I needed to adapt 'test_02_prorata_basic'.

Concretely:
new calc:

old calc:

@kittiu @JordiBForgeFlow @pedrobaeza
For this PR I took advantage of the 'days_calc' logic that has been added by your common efforts.
I did a small refactoring of the approach (without changing the business logic):

• move of line_days calc to _compute_line_dates method
• change of "if entry.get('day_amount') > 0' into 'if self.days_calc'
  This last change is required since the day_amount is now used for both prorata and days_calc.
  The check on a positive amount is also a 'functional bug' since it does not allow to depreciate negative amount assets (which some of my customers use as a workaround for the fact that we do not (yet) support asset devaluations in a proper way).

[hioppolo-ia]

@luc-demeyer
Thank you for updating this, the fix looks like it is working on your end but i haven't been able to reproduce it successfully.

The first and end period calcs in the first year ONLY are not calculating correctly. I noticed on your screenshot above that period 1 calculates correctly (can't see period 12), but this doesn't seem to be the same in my instance. I tested this with Jan-Dec fiscal year and this did not work either.

Example 1:

Example 2:

Example 3:

This is happening for both linear and degressive assets and i haven't been able to isolate it to any particular configuration. I will email you for access to our test DB so you can investigate and see if there is any difference in the config to help isolate the issue.

[luc-demeyer]

@hioppolo-ia
Thanks to give me access to your test db.
I moved the Fiscal Year settings back to july 1st - june 30th and entered two assets with linear depreciations over 5 years (cf assets named test*). These do calculate correctly now. Hence it looks like we fixed that problem.

The first period calc is wrong for asset 74421. This one is degressive. Can you doublecheck is there are still some problems in case of linear depreciation ? And if so, let me know the asset reference on your test db so that I can check.

[luc-demeyer]

@hioppolo-ia
I have now fixed the first period calculation for the degressive depreciation. Please have a look.

[hioppolo-ia]

@luc-demeyer

First period is fixed, however final period in first year is still facing some issues.

For degressive
-First period for the first year calculates correctly, this is fixed.
-The last period for the first year (june period, y1) is still calculating incorrectly.

For Linear
-First period for the first year calculates correctly, this is fixed.
-The last period for the first year (june period, y1) is still calculating incorrectly.

I will send you our test DB by email again.

[luc-demeyer]

@hioppolo-ia
I think first need to explain how the fiscal year depreciation works:

If you have and asset of 3000 EUR that is depreciated over 3 years than there is a depreciation of 1000 EUR per year. This is what we typically see with customers who work with an external account to maintain the asset calculation. Those accountants usually make year end accounting entry with in this example 1000 EUR of depreciation.
In order to have the cost spread over the year the asset management module takes the annual amount, calculates the first period depreciation based upon the number of days in that period (in case of pro-rata temporis). The following months are fixed monthly amounts. But in order to have the correct amount depreciated at the end of the fiscal year we need to work away rounding errors at the last fiscal period of the year.

[pedrobaeza]

@luc-demeyer isn't interesting to include such explanation of the computation in the module's README?

[hioppolo-ia]

@luc-demeyer
Understood, but i don't think the last period in the first year is a rounding issue. The differences are significant in the examples above compared to the rounding i have seen for other years which is just a few cents usually.

Is the module working in a way that the prorata amount from the first period is being applied to the last period of the first year instead of the last period of the whole calculation?

For example using the above for asset 74426 and assuming the asset life was 3 years, the depreciation amounts should be as follows:

Fiscal Year 1
aug 203.58 prorata
sep 562.92
oct 562.92
nov 562.92
dec 562.92
jan 562.92
feb 562.92
mar 562.92
apr 562.92
may 562.92
jun 562.92

Year2
jul 562.92
aug 562.92
sep 562.92
oct 562.92
nov 562.92
dec 562.92
jan 562.92
feb 562.92
mar 562.92
apr 562.92
may 562.92
jun 562.92

Year3
jul 562.92
aug 562.92
sep 562.92
oct 562.92
nov 562.92
dec 562.92
jan 562.92
feb 562.92
mar 562.92
apr 562.92
may 562.92
jun 562.92

Year4
jul 359.34 prorata (562.92 [monthly amount] - 203.58 [first period amount])

it looks like some of this prorata amount is being applied to the last period in the first year instead of the last period in the whole calculation.

What are your thoughts?

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[rafaelbn]

Related issue to this PR opened in #1712

/ocabot rebase

[OCA-git-bot]

@rafaelbn The rebase process failed, because command git rebase origin/12.0 failed with output:

First, rewinding head to replay your work on top of it...
Applying: [12.0][FIX]asset table wrong leapyear calc
Using index info to reconstruct a base tree...
M	account_asset_management/models/account_asset.py
Falling back to patching base and 3-way merge...
Auto-merging account_asset_management/models/account_asset.py
CONFLICT (content): Merge conflict in account_asset_management/models/account_asset.py
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 [12.0][FIX]asset table wrong leapyear calc
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".

[rafaelbn]

@luc-demeyer could you please rebase?

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.