-
-
Notifications
You must be signed in to change notification settings - Fork 523
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
[MIG] contract_sale_generation: Migration to 15.0 #847
[MIG] contract_sale_generation: Migration to 15.0 #847
Conversation
self.contract_line._onchange_is_auto_renew() | ||
contracts = self.contract2 | ||
for _i in range(10): | ||
contracts |= self.contract.copy({"contract_type": "sale"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently tests fail at this one line. Error:
contract/models/contract_line.py", line 452, in _check_recurring_next_date_start_date raise ValidationError( odoo.exceptions.ValidationError: You can't have a date of next invoice anterior to the start of the contract line 'Test Contract Template'
Will need to check this more. @NL66278 @thomaspaulb if you have any ideas, please shoot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ntsirintanis I tried to follow what happens, and somehow it's a problem with the order of the evaluation of computations.
- The test contract line is set to certain values, among which
date_start = 2020-01-01
- The contract including this line is copied 10 times
- During copying of the contract,
recurring_next_date
of the lines gets recomputed correctly (2020-01-15) - Something happens and
recurring_next_date
is now2020-01-01
, which is lower than 2020-01-15 (maybe recomputation, or goes back to cached value) - The computed values are checked and it finds 2020-01-01 < 2020-01-15 and borks out
I find that this stops happening when you access self.contract_line.recurring_next_date
(by printing, or just like that) before copying, so between line 98 and 99.
Somehow equivalently, @rousseldenis found in this PR that he had to call _compute_recurring_next_date()
, however that specifically does not work for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another failing test on import odoo.odoo.tests
in another module, I don't know how that got through CI, but it's clearly from and should be odoo.tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaspaulb We need first to fix the tests import errors, then, check which test(s) are failing. @ntsirintanis
e0490fe
to
02a5564
Compare
/ocabot migration contract_sale_generation |
@ntsirintanis Thanks for this. Don't forget to make your PR's title a little bit understandable (e.g.: like your last commit) |
02a5564
to
fc4bf5f
Compare
@@ -123,13 +123,10 @@ def _recurring_create_sale(self, date_ref=False): | |||
self._compute_recurring_next_date() | |||
return sale_orders | |||
|
|||
@api.model | |||
def _get_recurring_create_func(self, create_type="invoice"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ntsirintanis Why removing this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it was removed from contract module in version 15.0, along with the generation_type field and all related methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmh, that's because migration PR for 15 was created before those changes.
I'll do forwardport of changes in contract since. Wait for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arf, already done.
Could you review #776 ?
fc4bf5f
to
cb75840
Compare
…ata (OCA#130) * [FIX+IMP] contract: Improve usability and don't fail on wrong data * Cron create invoices masked for avoiding silent errors * New constraints for assuring data consistency * UI helps for entering consistent data * Spanish translation * Remove double company_id field on form * [FIX] contract_sale_generation: Adapt tests to upstream contract
* Change the method called in the view * Complete the create_invoice method * Bump version + authoring * Correct bad call of method Small Documentation * Add super call in python test * FIX bad field names causing bad quantities in sale.order.line
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
cb75840
to
0fb4b16
Compare
# then it maintains it's 'old' value. | ||
# TODO: Research that | ||
recurring_next_date = self.contract_line.recurring_next_date | ||
self.assertGreaterEqual(recurring_next_date, self.contract_line.date_start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added these 2 lines in order to make unit tests work, and pre-commit to not complain. Need to see why this is happening though.
0fb4b16
to
a2d53c1
Compare
@@ -96,6 +96,11 @@ def test_cron_recurring_create_sale(self): | |||
self.contract_line.recurring_invoicing_type = "post-paid" | |||
self.contract_line.date_end = "2020-03-15" | |||
self.contract_line._onchange_is_auto_renew() | |||
# If we do not recompute recurring_next_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, IMHO there are still some cases that should be solved after theses changes : cd086dd
@ntsirintanis I think if a test is added for a contract with payment terms and a fiscal position, the code will be nearly completely covered and you will get rid of the last two red crosses. |
dates = self._get_period_to_invoice( | ||
self.last_date_invoiced, self.recurring_next_date | ||
) | ||
sale_line_vals = self._prepare_sale_line_vals(dates, order_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call keyword parm with keyword: order_id=order_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually order_id
is nowhere passed to _prepare_sale_line
as far as I can see. So logic and argument can maybe removed from this function and from _prepare_sale_line_vals
. Can anyoint point out a use for thus argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is intended in order to override and force an existing order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you add sale lines to an existing sale order, through a write on that order, order_id will be filled automagically. Or if somehow some code wants to use the prepared values to create the order lines, the order_id can be inserted in that code. On the other hand this is existing code and we should be carefull with changing any method signature. I was just wondering about the usecase...
from odoo.tests import Form | ||
|
||
|
||
def to_date(date): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is not uses and can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but should be in each test line that uses fields.Date.to_date(
7da94d7
to
c06d773
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
c06d773
to
5b9bf0f
Compare
This PR is as green as it gets. I added some superfluous unit tests to satisfy codecov, but can't do much beyond that. Please review. |
@ntsirintanis I already approved the PR. In the CodeCov there are still three red lines:
I do not know how the yellow lines are counted in the percentage. It might be very difficult to get rid of those. |
Agreed. I've been already working on points one and two. Will finish them, push, and hope for absolute greenness. |
5b9bf0f
to
dc07e6d
Compare
@ntsirintanis Is this ready ? |
yes, finally, we are green! |
/ocabot merge patch |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at cb00e34. Thanks a lot for contributing to OCA. ❤️ |
@ntsirintanis Could you check at this ? #841 I think this is related to your problem with recurring_next_date value. |
|
No description provided.