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][IMP] contract: Big changes in recurrency fields #1080

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

flalexg
Copy link

@flalexg flalexg commented Apr 24, 2024

Modules instaled:

  • contract
  • contract_sale
  • contract_sale_generation

Description of the problem(s):
TO report this problem, I changed to invisible fields to visible in the view in order to see ther error better.
We have made all the screenshots generating invoices, but it also occurs when generating sales.

Problem 1

If you add the products and then you change the recurrency (for exemple Every 1 week), the recurrency fields are not updated correctly.

  1. We add a product.
    image

  2. We change the recurrency
    image

  3. We save and we create 3 invoices
    image

As we can see, the recurrence is every 1 Month and not every 1 Week.

Problem 2

The contract never ends.

  1. We create this contract with a Date End.
    image

  2. We add some products (in order to avoid the problem above).
    image

As we can see, the Date End is not set on the lines. (origin of the error).

  1. We generate infinite invoices.

image

We are able to generate infinite invoices. Maybe the scheduled action controls this, but this should be controlled manually also.

Problem 3

Date end dissapear when changing the recurrency.

  1. We create a contract. It must has a Date End. We save it.
    image

  2. We want to change de recurrency (it can happens). We notice the date end dissapears when we save the changes.
    image

Problem 4

When having 2 or more lines, the last date invoice field doesn't compute correctly.

  1. We create a contract. To see more clear the error, we set the recurrency every 1 week.
    image

  2. We create 1 invoice.

image

As we can see, the field last_date_invoice is mismatching between the 2 lines, when it should be the same because we aren't in a recurrency line level. If we had set a date end, we would be see an error at the last invoices, where only one product would be invoiced due to this error.

After exposing this, I made some several changes in the structure of the module contract.

Problem 5

If we put a Date End and we change the Date Of Next Invoice manually, the date end dissapears

  1. We create a contract with Date End
    image

  2. We manually change the Date Of Next Invoice and we save it.

image

As you can see, the Date End dissapears.

Problem 6

We can't change the Date of Next Invoice in recurrence at line level

  1. We create a contrat with recurrence at line level
    image

  2. We create a line. We only establish a product and we save.
    image

  3. We edit the line and we stablish the 'Date of Next Invoice' on day later. We save and close. The Next Period End is correctly changed.
    image

  4. We open the line again.
    image

As we can see, the Date of Next Invoice is not changed making the next period end beig bad calculated.

@flalexg flalexg changed the title [IMP] Big changes in recurrency fields [16.0][IMP] contract: Big changes in recurrency fields Apr 24, 2024
@flalexg flalexg force-pushed the 16.0-imp-recurrency-fields branch 2 times, most recently from fd40bdf to 4167955 Compare April 24, 2024 16:53
@javierjcf
Copy link

javierjcf commented Jun 12, 2024

Recently i was affected by problem 4, this PR solve it. I think this PR should be merged, it fix a lot of errors. what is happening with runbot building?

@ChristophAbenthungCibex

I ran into similar problems and this PR fixes them. LGTM

@pedrobaeza pedrobaeza added this to the 16.0 milestone Jul 29, 2024
@@ -166,11 +172,9 @@ def _compute_recurring_interval(self):
def _compute_date_start(self):
self._set_recurrence_field("date_start")

# pylint: disable=missing-return
@api.depends("contract_id.recurring_next_date", "contract_id.line_recurrence")
def _compute_recurring_next_date(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing this?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @pedrobaeza. Sorry for the delay for the response.

I removed this because it's redundant. Since we are inheriting from contract.recurrency.mixin.basic in the contract.line, it's redundant having the same on the abstract.contract.line.

Copy link
Member

Choose a reason for hiding this comment

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

But this override was needed AFAIK.

Copy link
Author

Choose a reason for hiding this comment

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

Mmmm i'm going to check it now, but one of the problems is that the contract never ends. This happens because this inherit.
On a first step, I created the _compute_date_end function below. This, trigger a chain of computes (next_period_date_start compute that triggers the recurring_next_date compute).
Then, I checked this function and I concluded that it was redundant and not necessary.

I've been working with this changed since I created the MR and it's working very well.

If u want, I can let function as it was initially since is not having any impact on the code.

@@ -48,6 +48,8 @@ class ContractRecurrencyBasicMixin(models.AbstractModel):
help="Invoice every (Days/Week/Month/Year)",
)
date_start = fields.Date()
date_end = fields.Date()

Copy link
Member

Choose a reason for hiding this comment

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

Don't add empty lines here in the middle.

Copy link
Author

Choose a reason for hiding this comment

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

My bad. Sorry.

@@ -89,7 +94,7 @@ class ContractRecurrencyMixin(models.AbstractModel):
string="Next Period End",
compute="_compute_next_period_date_end",
)
last_date_invoiced = fields.Date(readonly=True, copy=False)
last_date_invoiced = fields.Date(copy=False)
Copy link
Member

Choose a reason for hiding this comment

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

Don't remove readonly, which is unrelated to the goal of this PR.

Copy link
Author

Choose a reason for hiding this comment

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

Changed

invisible="1"
/>
<field name="next_period_date_end" invisible="1" />

Copy link
Member

Choose a reason for hiding this comment

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

Don't add empty lines in the middle.

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

@@ -249,11 +249,17 @@
/>
<field name="recurring_next_date" invisible="1" />
<field name="date_start" invisible="1" />
<field name="date_end" />
<field name="date_end" invisible="1" />
Copy link
Member

Choose a reason for hiding this comment

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

Why making it invisible?

Copy link
Author

Choose a reason for hiding this comment

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

Well, I make it invisible since the other fields like date_start or recurring_next_date are invisible. To be uniform, but I'm letting it visible.

@pedrobaeza
Copy link
Member

FYI we are planning to refactor whole module in #1108

@HaraldPanten
Copy link

FYI we are planning to refactor whole module in #1108

We're not far from creating a PR.

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

Successfully merging this pull request may close these issues.

5 participants