[12.0] contract: some refactoring and a UX improvement by sbidoul · Pull Request #434 · OCA/contract

sbidoul · 2019-12-06T10:13:38Z

Some simple refactoring of the contract module. The main idea is to split the computation of the next invoice date in two parts: the computation of the next period, followed by the computation of the next invoice date (aka recurring_next_date) based on the next period dates.

This is best reviewed one commit at a time to understand individual changes.

There is also a UX improvement: display the next period start and end date on the contract line form, so the user does not need to deduce it from other parameters.

Finally, monthlylastday now supports pre-paid/post-paid. This could be a breaking change
if there is code that creates monthlylastday contract lines without setting them to post-paid mode.

fixes: #428

First compute the next period end date, then derive the next invoice date from the next period stard and end date.

This concentrates all next date calculation logic in one place, and will allow further simplifications.

Add two computed field showing the next period start and end date. This improve the UX and will enable further simplifications in the code.

Move the part of the logic that compute the next period depending on the chosen next invoice date to _get_next_period_date_end.

Reuse the logic that is now fully located in _get_recurring_next_date.

For backward compatibility

sbejaoui · 2019-12-06T15:39:17Z

        )
        self.contract.recurring_create_invoice()
        self.assertEqual(
-            self.acct_line.recurring_next_date, to_date('2018-04-01')


The current version assure that a customer is billed the same day each month.

Assume this example:

date start 01/01

date end 15/03

the customer will receive an invoices at:

for pre-paid: 01/01, 01/02 and 01/03

for monthlylastday: 31/01, 28/02 and 31/03

for post-paid: 01/02, 01/03 and 01/04

for me it make more sens to say post-paid mode invoice date is the last day of the invoicing period (or the last day of the invoicing period + one day).

Let make a RFC and see what other think about this change

get_relative_delta now works the same for all recurring rules. Move the special case handling to _init_last_date_invoiced which is used only for migration.

fixed

sbejaoui

Fantastic

Thank you @sbidoul for this great work.

For me the most important thing in this PR is the fact that it fix a logic issue we had for a long time. Now the recurring_invoicing_type is no longer implicit in the monthlylastday option and we can configure a invoicing process to stop at the end of the month and be payed at the first day of the period (even for an interval of several months see).

Adding to this fix, this PR open the way for new features (invoicing offset, computed next invoice date,.. ) by simplifying the code and improve the UX.

cc/ @gva-acsone

sbidoul · 2019-12-09T11:48:19Z

@@ -669,7 +669,7 @@ def _get_period_to_invoice(
        self.ensure_one()
        first_date_invoiced = False


This line is useless now

monthlylastday is (almost) not a special case anymore \o/. montlylastday is simply a montly period where the periods are aligned on month boundaries. The last bit of special casing is that postpaid generates invoice the day after the last dasy of the period, except for monthlylastday where the invoice is generated on the last day of the period. This last exception will disappear when we put the offset under user control. This is a breaking change because the post-paid/pre-paid mode becomes relevant for monthlylastday invoicing. The field becomes visible in the UI. Code that generate monthlylastday contract lines must now correctly set the pre-paid/post-paid mode too. Some tests have had to be adapted to reflect that.

sbidoul · 2019-12-09T13:23:57Z

I added the offset field in the contract line form, to further help the user understand what is going on.

@sbejaoui will do some more testing with other depending modules (OCA and customer specific), but we don't expect issue. So this is ready to review. Reviewing individual commits is highly recommended to understand the logical evolution of the code base.

…thlylastday

rafaelbn

After a first quick functional review it looks OK 👍

OCA-git-bot · 2019-12-12T08:43:36Z

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). 🤖

pedrobaeza · 2019-12-12T09:07:04Z

It's difficult to predict side effects with the great amount of changes, but let's trust in your good work and extra tests also makes this more confidence. Trying it out in real will tell us any possible problem.

/ocabot merge major

OCA-git-bot · 2019-12-12T09:07:20Z

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-434-by-pedrobaeza-bump-major, awaiting test results.

Signed-off-by pedrobaeza

sbidoul · 2019-12-12T09:30:09Z

To summarize breaking changes:

Contract lines and products using monthlylastday MUST now specify pre-paid / post-paid. There is a migration script for that. The UI has been adapted. Code that creates such lines must be adapted or will default to pre-paid whereas post-paid was implied before.
get_relative_delta() now works uniformly for all period types (it had an exception for monthlylastday before). Code calling it directly must be adapted.
In post-paid mode, the invoice date is now always the first day after the end of the period being invoiced. There was an exception before for the last period of the contract.

OCA-git-bot · 2019-12-12T09:46:22Z

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

use the new methods for next period calculation result of the refactoring in OCA#434 to consider the recurring_invoicing_type for monthlylastday mode and clean code

sbidoul added 4 commits December 6, 2019 09:19

[REF] contract: rename misnamed methods

5ee13d4

[REF] contract: clarify _get_recurring_next_date

5bc8959

First compute the next period end date, then derive the next invoice date from the next period stard and end date.

[REF] contract: handle max_date_end in _get_recurring_next_date

e6b9ea9

This concentrates all next date calculation logic in one place, and will allow further simplifications.

[IMP] contract: add next period start/end fields

edf6833

Add two computed field showing the next period start and end date. This improve the UX and will enable further simplifications in the code.

sbidoul added the work in progress label Dec 6, 2019

sbidoul added this to the 12.0 milestone Dec 6, 2019

sbidoul requested a review from sbejaoui December 6, 2019 10:13

sbidoul added 3 commits December 6, 2019 12:44

[REF] contract: refactor _get_period_to_invoice

83cb7c1

Move the part of the logic that compute the next period depending on the chosen next invoice date to _get_next_period_date_end.

[REF] contract: refactor _update_recurring_next_date

aff3781

Reuse the logic that is now fully located in _get_recurring_next_date.

[REF] contract: re-add _compute_first_recurring_next_date

7c3b9bc

For backward compatibility

sbidoul force-pushed the 12.0-contract-refactor-sbi branch from cf8e0e2 to 7c3b9bc Compare December 6, 2019 13:26

[FIX] contract: add missing dependency in computed field

dd3f3a6

pedrobaeza mentioned this pull request Dec 6, 2019

[12.0][IMP] - Contract: Add a new field to compute the next period end date #431

Closed

sbejaoui reviewed Dec 6, 2019

View reviewed changes

[REF] contract: remove one monthlylastday special case

d53f01d

get_relative_delta now works the same for all recurring rules. Move the special case handling to _init_last_date_invoiced which is used only for migration.

sbidoul added needs review and removed work in progress labels Dec 8, 2019

sbejaoui previously requested changes Dec 9, 2019

View reviewed changes

Comment thread contract/migrations/12.0.5.0.0/pre-migration.py Outdated

sbejaoui self-requested a review December 9, 2019 09:14

sbidoul commented Dec 9, 2019

View reviewed changes

Comment thread contract/models/contract_line.py Outdated

sbejaoui force-pushed the 12.0-contract-refactor-sbi branch from e5b1ff4 to eef75b7 Compare December 9, 2019 11:11

sbejaoui approved these changes Dec 9, 2019

View reviewed changes

sbidoul commented Dec 9, 2019

View reviewed changes

sbidoul force-pushed the 12.0-contract-refactor-sbi branch from 42acf47 to 1b2f8db Compare December 9, 2019 12:16

sbidoul and others added 2 commits December 9, 2019 13:17

[FIX] contract: Improve unit tests

a90e18b

sbejaoui and others added 2 commits December 9, 2019 13:17

[REF] contract: simplify _get_period_to_invoice

7212fc4

[IMP] contract: display invoicing offset

d9bd822

sbidoul force-pushed the 12.0-contract-refactor-sbi branch from 1b2f8db to d9bd822 Compare December 9, 2019 12:17

[IMP] - Set recurring_invoicing_type for existing products set to mon…

ab4ed31

…thlylastday

rafaelbn approved these changes Dec 12, 2019

View reviewed changes

OCA-git-bot added approved ready to merge labels Dec 12, 2019

OCA-git-bot added a commit that referenced this pull request Dec 12, 2019

Merge PR #434 into 12.0

755a035

Signed-off-by pedrobaeza

OCA-git-bot merged commit ab4ed31 into OCA:12.0 Dec 12, 2019

OCA-git-bot added the merged 🎉 label Dec 12, 2019

sbidoul deleted the 12.0-contract-refactor-sbi branch December 12, 2019 10:00

sbejaoui mentioned this pull request Dec 18, 2019

[12.0][FIX] use the new methods for next period calculation result of the refactoring in #434 to consider the recurring_invoicing_type for monthlylastday mode and clean code #439

Merged

sbejaoui mentioned this pull request Jan 7, 2020

[CONTRACT] The recurring rule type monthly-last-day #426

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[12.0] contract: some refactoring and a UX improvement - #434

[12.0] contract: some refactoring and a UX improvement#434
OCA-git-bot merged 18 commits into
OCA:12.0from
acsone:12.0-contract-refactor-sbi

sbidoul commented Dec 6, 2019 •

edited by sbejaoui

Loading

Uh oh!

sbejaoui Dec 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbejaoui left a comment

Uh oh!

sbidoul Dec 9, 2019

Uh oh!

sbidoul commented Dec 9, 2019

Uh oh!

rafaelbn left a comment

Uh oh!

OCA-git-bot commented Dec 12, 2019

Uh oh!

pedrobaeza commented Dec 12, 2019

Uh oh!

OCA-git-bot commented Dec 12, 2019

Uh oh!

sbidoul commented Dec 12, 2019

Uh oh!

OCA-git-bot commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -669,7 +669,7 @@ def _get_period_to_invoice(
		self.ensure_one()
		first_date_invoiced = False

Uh oh!

Uh oh!

Conversation

sbidoul commented Dec 6, 2019 • edited by sbejaoui Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbejaoui Dec 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbejaoui left a comment

Choose a reason for hiding this comment

Uh oh!

sbidoul Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

sbidoul commented Dec 9, 2019

Uh oh!

rafaelbn left a comment

Choose a reason for hiding this comment

Uh oh!

OCA-git-bot commented Dec 12, 2019

Uh oh!

pedrobaeza commented Dec 12, 2019

Uh oh!

OCA-git-bot commented Dec 12, 2019

Uh oh!

sbidoul commented Dec 12, 2019

Uh oh!

OCA-git-bot commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sbidoul commented Dec 6, 2019 •

edited by sbejaoui

Loading