-
-
Notifications
You must be signed in to change notification settings - Fork 519
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] product_contract #959
Conversation
84dcd76
to
54fad5f
Compare
/ocabot migration product_contract |
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.
Code review
@rousseldenis there is a typo in sale_order.py:
It should have been _check_contRact_is_not_terminated Should that be fixed? If so, should it be fixed in migration or in separate PR? |
) | ||
return date_end | ||
|
||
@api.depends("product_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.
@rousseldenis, @sbejaoui, @pedrobaeza, @sbidoul maybe you could help me here please. I am trying to use this module in e-shop. However when I purchase the product, Contract is created but all the relevant fields are empty. What I found out is that this method _compute_auto_renew
is not called in such scenario. I'm scratching my head on why is it never called and how to solve this in some good and elegant way. Any help / advice is really appreciated.
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.
Hi @Rad0van ,
I think the issue is that all the fields computed by this method have a default value.
What happens at record creation is that get assigned their default value so they don't need to be computed;
The computation method triggers-in, correctly, next time the observed field (product_id
) changes.
This issue doesn't happen when a user manually creates a Sale Order because the computation method is immediately triggered when product_id
changes (as the computation method is also part of the onchange
workflow).
Besides causing this issue, a default value on a computed field doesn't make much sense in my opinion. I suggest you remove all defaults from field definition and maybe add them in the computation method itself.
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.
Hi @espo-tony ,
not really sure I follow what you suggest here because the issue is as you point out that when Sale Order is created directly, the computation method is not called. I have created a hacky solution by creating glue module between product_contract and website_sale where I basically reproduce the whole _compute_auto_renew
method except for setting the quantity. It goes like this:
class SaleOrder(models.Model):
_inherit = "sale.order"
def _cart_update_order_line(self, product_id, quantity, order_line, **kwargs):
"""
This ensures that when contract product is put into basket in e-commerce
all necessary values are set
"""
order_line = super()._cart_update_order_line(
product_id, quantity, order_line, **kwargs
)
"""
This is (apart from commented out line) 1:1 copy of _compute_auto_renew method
from product_contract module to ensure we get the defaults configured
"""
if order_line.product_id.is_contract:
# order_line.product_uom_qty = order_line.product_id.default_qty
order_line.recurring_rule_type = order_line.product_id.recurring_rule_type
order_line.recurring_invoicing_type = (
order_line.product_id.recurring_invoicing_type
)
order_line.date_start = order_line.date_start or fields.Date.today()
order_line.date_end = order_line._get_date_end()
order_line.is_auto_renew = order_line.product_id.is_auto_renew
if order_line.is_auto_renew:
order_line.auto_renew_interval = (
order_line.product_id.auto_renew_interval
)
order_line.auto_renew_rule_type = (
order_line.product_id.auto_renew_rule_type
)
return order_line
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.
Hi @Rad0van
When a Sale Order is created directly, the computation method is not called because of the default values. If you remove them, the computation method will be called even when a Sale Order is created directly. I tested this solution locally and it worked 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.
Hi @Rad0van When a Sale Order is created directly, the computation method is not called because of the default values. If you remove them, the computation method will be called even when a Sale Order is created directly. I tested this solution locally and it worked for me.
Oh I see now! But that is quite a big change which I think should be approved by more senior people.
Also I would be interested to get to know the inner mechanics of this. I.e. why the @api.depends is called when the default values are not present? It only depends on product_id Does anyone know the explanation?
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.
* Add contract functionality to `product.templates` * Add logic to create contracts from `sale.order` that contains contract products.
* 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
- On Sale Order confirmation, a contract is created for each contract template used on sale order lines - A not finished contract can be mentioned on sale order line - A sale order line linked to a contract will update it and don't create a new one if it had the same template
recurring_next_date should be computed by contract line to get default value
- Sale order line for contract product pass to nothing to invoice on order confirmation - Contract Invoices are linked to sale order line
@remi-filament seems @Abranes is right. In the commit #1019 the analytic_account_id was indeed removed... |
Ah yes, my bad I checked contract code on your branch and not on upstream when your branch is outdated for that topic... You should then rebase this PR on OCA v16 branch and squash your 3 last commits in one before this PR can be merged. |
contract = contract_model.create( | ||
rec._prepare_contract_value(contract_template) | ||
) | ||
contracts.append(contract) |
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.
When performing a create(), the created object 'contract.contract(xxx,)' is returned. When appending to the list named 'contracts', you should indicate performing a contract.id to create a list of contract identifiers. If this is not done, when performing a contract_model.browse(contracts), it returns contract.contract(contract.contract(xxx,),), which is incorrect. From what I see, it should return contract.contract(xxx,).
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.
@Rad0van can you answer this comment?
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!
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.
Review to cover all tests. Regards.
"payment_term_id": self.payment_term_id.id, | ||
"fiscal_position_id": self.fiscal_position_id.id, | ||
"invoice_partner_id": self.partner_invoice_id.id, | ||
"line_recurrence": self.partner_invoice_id.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.
Why the line_recurrence is the self.partner_invoice_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.
Why the line_recurrence is the self.partner_invoice_id?
Funny, it's the same as in original module:
contract/product_contract/models/sale_order.py
Lines 76 to 79 in aca1183
"payment_term_id": self.payment_term_id.id, | |
"fiscal_position_id": self.fiscal_position_id.id, | |
"invoice_partner_id": self.partner_invoice_id.id, | |
"line_recurrence": self.partner_invoice_id.id, |
But probably needs fixing as well.
line_to_update_contract = rec.order_line.filtered( | ||
lambda r: r.contract_id | ||
and r.product_id.is_contract | ||
and r |
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.
You should check that r exists at the beggining of the function, no? Otherwise, when performing r.contract_id it will fail.
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.
You should check that r exists at the beggining of the function, no? Otherwise, when performing r.contract_id it will fail.
It's just formatting that's misleading you. Check the following line ;-)
"line_recurrence": self.partner_invoice_id.id, | ||
} | ||
|
||
def action_create_contract(self): |
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 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.
Functional test ok!
Hello @Rad0van, I just wanted to ask if you were planning to continue with the PR. I see that you have some unanswered comments, and I was wondering if I can proceed with the PR of the product_contract module. If it's not a problem! |
Was quite busy but planning to finish this one soon. |
@rousseldenis can you update your review? |
I have carried over changes done in #1083 so now these are 1:1 but also fix a typo in constrain method name. |
5b01c29
to
a41cafe
Compare
<xpath expr="//div[@name='options']" position="inside"> | ||
<div attrs="{'invisible': [('type', '!=', 'service')],}"> | ||
<field name="is_contract" /> | ||
<label for="is_contract" /> | ||
</div> | ||
</xpath> |
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.
<xpath expr="//div[@name='options']" position="inside"> | |
<div attrs="{'invisible': [('type', '!=', 'service')],}"> | |
<field name="is_contract" /> | |
<label for="is_contract" /> | |
</div> | |
</xpath> | |
<div name="options" position="inside"> | |
<span class="d-inline-block" attrs="{'invisible': [('type', '!=', 'service')]}"> | |
<field name="is_contract"/> | |
<label for="is_contract"/> | |
</span> | |
</div> |
This looks better
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 this can be done in another PR.
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.
Code review. Changes seems legit
).format(order_line.product_id.name, rec.company_id.name) | ||
"template for '%(product_name)s' product " | ||
"in '%(company_name)s' company." | ||
) |
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.
Prefer using translation method attributes.
_("You must specify (...) %(product_name)s (...)", product_name=(...), company_name=(...))
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.
As this is PR is identical to the v17 branch, I think the rest can be done in followup PRs.
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.
We can move forward and do improvements in further PR's
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 9ad0022. Thanks a lot for contributing to OCA. ❤️ |
Fairly standard, had only to update one compute method name.