Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

16.0 mig membership prorate variable period #168

Merged

Conversation

edlopen
Copy link
Member

@edlopen edlopen commented May 2, 2024

Standard migration to 16.0

@moduon MT-1833

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

chienandalu and others added 14 commits May 2, 2024 08:49
Currently translated at 100.0% (3 of 3 strings)

Translation: vertical-association-12.0/vertical-association-12.0-membership_prorate_variable_period
Translate-URL: https://translation.odoo-community.org/projects/vertical-association-12-0/vertical-association-12-0-membership_prorate_variable_period/pt_BR/
Currently translated at 100.0% (3 of 3 strings)

Translation: vertical-association-12.0/vertical-association-12.0-membership_prorate_variable_period
Translate-URL: https://translation.odoo-community.org/projects/vertical-association-12-0/vertical-association-12-0-membership_prorate_variable_period/it/
Currently translated at 100.0% (3 of 3 strings)

Translation: vertical-association-14.0/vertical-association-14.0-membership_prorate_variable_period
Translate-URL: https://translation.odoo-community.org/projects/vertical-association-14-0/vertical-association-14-0-membership_prorate_variable_period/ca/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: vertical-association-14.0/vertical-association-14.0-membership_prorate_variable_period
Translate-URL: https://translation.odoo-community.org/projects/vertical-association-14-0/vertical-association-14-0-membership_prorate_variable_period/
Currently translated at 100.0% (6 of 6 strings)

Translation: vertical-association-14.0/vertical-association-14.0-membership_prorate_variable_period
Translate-URL: https://translation.odoo-community.org/projects/vertical-association-14-0/vertical-association-14-0-membership_prorate_variable_period/it/
@yajo
Copy link
Member

yajo commented May 3, 2024

@chienandalu wanna review this one?

@OCA-git-bot
Copy link
Contributor

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
Copy link
Member Author

edlopen commented May 8, 2024

Can you check it functionally @fcvalgar? Thanks!

Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the calculation of the prorate is not correct because it does not include the current day and the final day. In the memberships both days are usually included.

image

image

image

@edlopen
Copy link
Member Author

edlopen commented May 14, 2024

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 ?

Copy link

@fcvalgar fcvalgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review,

LGTM, thank you @edlopen

@yajo
Copy link
Member

yajo commented May 15, 2024

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

@EmilioPascual
Copy link
Contributor

EmilioPascual commented May 15, 2024

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
Copy link
Member Author

edlopen commented May 16, 2024

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

@rafaelbn
Copy link
Member

/ocabot migration membership_prorate_variable_period

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone May 17, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request May 17, 2024
11 tasks
rafaelbn

This comment was marked as resolved.

@OCA-git-bot
Copy link
Contributor

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
Copy link
Member

@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

This comment was marked as resolved.

@rafaelbn
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

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 OCA-git-bot merged commit e448489 into OCA:16.0 May 17, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.