search

OCA / vertical-association

Odoo addons for membership related tasks
GNU Affero General Public License v3.0
38 stars 112 forks source link

16.0 mig membership prorate variable period #168

Closed edlopen closed 4 months ago

edlopen commented 4 months ago

Standard migration to 16.0

@moduon MT-1833

@yajo @EmilioPascual @Shide @rafaelbn review this PR when you can. Thank you!

yajo commented 4 months ago

@chienandalu wanna review this one?

OCA-git-bot commented 4 months ago

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

edlopen commented 4 months ago

Can you check it functionally @fcvalgar? Thanks!

edlopen commented 4 months ago

Hi @fcvalgar , I've reviewed the two day difference that you commented. It is kind of a bummer because it seems that is missing the first and the last day and it lead us to misunderstood of what the problem really is.

I'll try to not enter too much in the details but this is a matter of decimal precision in the quantities.

In your case you are prorrating for the last 236 days in the year. If the quantity of a year is 1, our prorrated quantity should be 0.646575(...) years but in the invoice line will store only 0.64 which is that value rounded to two decimals. That lead us to $24 difference. So a membership that cost $1 a day make us think that we are missing the starting and ending date but, sadly, not.

To sum up, the difference that you notice is a precission error in the prorrate calculation. Because this is a migration I would consider that this PR is ok to merge but the roadmap will be updated to cover this issue later on.

I have a couple of ideas of how make this run propelly but I feel that is out of the scope of this PR.

Also, what do you think @EmilioPascual ?

yajo commented 4 months ago

I think it's worth it to record your discoveries in the module's roadmap/known issues.

EmilioPascual commented 4 months ago

Perhaps error is in the rounding in this line https://github.com/OCA/vertical-association/blob/16.0/membership_prorate/models/account_move_line.py#L37, but it's in another module. Anyway migration is ok.

edlopen commented 4 months ago

@rafaelbn this PR is ok to merge. We've all agreed on this migration to move on. Thank you!

rafaelbn commented 4 months ago

/ocabot migration membership_prorate_variable_period

OCA-git-bot commented 4 months ago

The migration issue (#131) has not been updated to reference the current pull request because a previous pull request (#167) is not closed. Perhaps you should check that there is no duplicate work. CC @edlopen

rafaelbn commented 4 months ago

@EmilioPascual @edlopen about this:

Perhaps error is in the rounding in this line https://github.com/OCA/vertical-association/blob/16.0/membership_prorate/models/account_move_line.py#L37, but it's in another module. Anyway migration is ok.

Please open an issue in githug and a subtas in our side.

thank you! 😄 You are awesome!

rafaelbn commented 4 months ago

/ocabot migration patch

rafaelbn commented 4 months ago

/ocabot merge patch

OCA-git-bot commented 4 months ago

Hey, thanks for contributing! Proceeding to merge this for you. Prepared branch 16.0-ocabot-merge-pr-168-by-rafaelbn-bump-patch, awaiting test results.

OCA-git-bot commented 4 months ago

Congratulations, your PR was merged at aacfeff86fa7f3cb351d453be01a5a8f00b77c9d. Thanks a lot for contributing to OCA. ❤️