Skip to content

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

Closed
sbejaoui wants to merge 2 commits into
OCA:12.0from
acsone:12.0-next_period_end_date-sbj
Closed

[12.0][IMP] - Contract: Add a new field to compute the next period end date#431
sbejaoui wants to merge 2 commits into
OCA:12.0from
acsone:12.0-next_period_end_date-sbj

Conversation

@sbejaoui

@sbejaoui sbejaoui commented Nov 28, 2019

Copy link
Copy Markdown
Contributor

An alternative solution for #428 is to make the invoicing period independent from the invoice date. Original idea of @pedrobaeza (see)

This PR add a new filed to store the next period end date. With the field last_date_invoiced the user can easily see and change the invoicing period.

A migration script was added to initialize the new field.

What we can add to this PR:

  • add an integer field offset to shift the invoice date
  • make the next invoice date a computed field based on
    • invoicing_type: pre-paid/post-paid
    • first and last period dates
    • offset field
  • make the monthlylastday a check box to be used to compute the next invoice date

CC/ @pedrobaeza , @sbidoul , @gva-acsone, @carlosdauden

…d date

make the invoicing period independent from the invoice date
@sbejaoui
sbejaoui requested a review from pedrobaeza November 28, 2019 17:13
@sbejaoui
sbejaoui force-pushed the 12.0-next_period_end_date-sbj branch from 54d92d7 to 6509e9e Compare November 28, 2019 17:27
else:
rec.last_date_invoiced = last_date_invoiced
rec.recurring_next_date = recurring_next_date
rec.next_period_end_date = next_period_end_date

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you can continue simplifying by doing _update_next_period_date instead of _update_recurring_next_date and make recurring_next_date a computed field that depends on next_period_date and recurring_invoicing_type.

)
next_period_end_date = fields.Date(
string='Next Period End Date', copy=False
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd suggest adding a next_period_start_date too (computed = last_date_invoiced + 1). That will simplify the code a bit. And you can display to the user instead of last_date_invoiced, it will improve the UX.

if self.date_end and self.date_end < last_date_invoiced:
if not self.next_period_end_date and self.date_end:
last_date_invoiced = self.date_end
return first_date_invoiced, last_date_invoiced, recurring_next_date

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When you have done _update_next_period this _get_period_to_invoice can become very dumb: return next_period_start_date, next_period_end_date

@sbidoul

sbidoul commented Nov 28, 2019

Copy link
Copy Markdown
Member

So yes 👍 , this PR will be a refactoring of the logic by computing the next period instead of the next invoice date, and making the next invoice date depending on the next period. It will become much easier to reason about the code when this is done, and the UX should be better too by clearly showing what will be the next period to invoice and the invoice date (instead of leaving it to some interpretation of the user as it is now).

And it will pave the way for new features such as:

  • end first period at the end of the month in prepaid mode too (similar to monthlylastday which is post-paid now)
  • add an offset for the invoice date based on the period to allow, e.g. invoicing a few days before the end of the period
  • always invoice at the end of the month even if the period overlaps a month boundary
  • etc...

@rafaelbn rafaelbn added this to the 12.0 milestone Nov 28, 2019
@sbidoul

sbidoul commented Nov 29, 2019

Copy link
Copy Markdown
Member

As discussed, in a first step, let's make recurring_next_date computed, but with an inverse method for backward compatibility (the inverse method logging a deprecation warning).

@pedrobaeza

Copy link
Copy Markdown
Member

Is this outdated with #434?

@sbidoul

sbidoul commented Dec 6, 2019

Copy link
Copy Markdown
Member

Is this outdated with #434?

Probably yes. My idea here to make recurring_next_date computed from the next period is raising some backward compatibility issue. I still think it would be easier for the user but there are some edge cases that are not reproducible with this approach.

In any case #434 improves maintainability and readability (IMO), and is fully compatible (except two minor differences that you can see in the test changes in #434).

@sbidoul

sbidoul commented Dec 8, 2019

Copy link
Copy Markdown
Member

Closing in favor of #434

@sbidoul sbidoul closed this Dec 8, 2019
@sbidoul
sbidoul deleted the 12.0-next_period_end_date-sbj branch December 8, 2019 11:41
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.

4 participants