tryton-env: Add purchase_blanket_agreement and sale_blanket_agreement modules

tryton-env: Add purchase_blanket_agreement and sale_blanket_agreement modules

issue10962

[reviewbot, 2021-11-17 13:58:18]

flake8 OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2021-11-17 15:22:16]

flake8 OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2021-11-19 14:52:06]

flake8 OK
URL: https://codereview.tryton.org/391151002

[pokoli, 2021-11-23 09:09:45]

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/blanket_order.py (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:19: 'Purchase blanket order'
blanket -> Blanket

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:61: 'readonly': Eval('state') !=
'draft',
We need to add a domain that from_date < from_date when both are set.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:66: states={
We need to add a domain that to_date is greather than from_date when both are
set.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:109: '/tree',
Probably it can be written in a  single line.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:115: @staticmethod
Please use classmethod for default methods.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:160: super(PurchaseBlanketOrder,
cls).__setup__()
can be written as super()

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:204: return
super(PurchaseBlanketOrder, cls).copy(
idem about super()

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:228: today =
Pool().get('ir.date').today()
please use call Pool() at the start of the method.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:229: if self.to_date < today:
to_date can be empty, so you need to test it first.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:607: purchase = Purchase(
I think we should separate the purchase creation in a method, so third party
modules can extend the defined fields.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:621: for sequence, line in
enumerate(self.start.lines):
There is no need to set sequence by default, order will be preserved if all
invoice have an empty sequence.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:625: purchase_line =
PurchaseLine(
Idem about purchase_line separation.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/configuration.py (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/configuration.py:53: return
ModelData.get_id('purchase_blanket_order',
We usually break after ( so 'purchase_blanket_order' should be on a new line.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/locale/en.po (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/locale/en.po:1: #
locale files should not be part of the review. They will be generated latter by
the translation process. 

P.D: If you have them already translated you can save them in another location
and upload them to pootle once the module is commited to tryton. This way the
translations will be autoamtically upladed.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/purchase.py (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:36: def check_party(cls, line):
as it is checking a single line, this can be a instance method: 

def check_party(self):
   if line.blanket_order_line:

Also it should be named check_blanket_order_party to avoid collision with other
modules.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:47: def check_product(cls, line):
Idem about instance method and naming. 


Also, I'm wondering if it won't be better to have a single method that checks
all blanked_order properties.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/purchase.xml (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.xml:14: <field
name="name">Purchase</field>
Won't be better named "Purchases" ?

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.xml:20: </record>
Where is such action used? I will prefer to keep together the action definition
and the keyword where it is used.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/tests/scenario_purchase_blanket_order.rst
(right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/tests/scenario_purchase_blanket_order.rst:104:
>>> blanket_order_line.remaining_quantity
Do we need to test that the remaining quantity ignores cancelled purchases?

[mrichez, 2021-11-23 17:13:15]

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/blanket_order.py (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:19: 'Purchase blanket order'
On 2021/11/23 09:09:44, pokoli wrote:
> blanket -> Blanket

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:61: 'readonly': Eval('state') !=
'draft',
On 2021/11/23 09:09:44, pokoli wrote:
> We need to add a domain that from_date < from_date when both are set.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:66: states={
On 2021/11/23 09:09:44, pokoli wrote:
> We need to add a domain that to_date is greather than from_date when both are
> set.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:109: '/tree',
On 2021/11/23 09:09:44, pokoli wrote:
> Probably it can be written in a  single line.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:115: @staticmethod
On 2021/11/23 09:09:44, pokoli wrote:
> Please use classmethod for default methods.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:160: super(PurchaseBlanketOrder,
cls).__setup__()
On 2021/11/23 09:09:44, pokoli wrote:
> can be written as super()

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:204: return
super(PurchaseBlanketOrder, cls).copy(
On 2021/11/23 09:09:44, pokoli wrote:
> idem about super()

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:228: today =
Pool().get('ir.date').today()
On 2021/11/23 09:09:44, pokoli wrote:
> please use call Pool() at the start of the method.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:229: if self.to_date < today:
On 2021/11/23 09:09:44, pokoli wrote:
> to_date can be empty, so you need to test it first.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:607: purchase = Purchase(
On 2021/11/23 09:09:44, pokoli wrote:
> I think we should separate the purchase creation in a method, so third party
> modules can extend the defined fields.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:621: for sequence, line in
enumerate(self.start.lines):
On 2021/11/23 09:09:44, pokoli wrote:
> There is no need to set sequence by default, order will be preserved if all
> invoice have an empty sequence. 

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:625: purchase_line =
PurchaseLine(
On 2021/11/23 09:09:44, pokoli wrote:
> Idem about purchase_line separation.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/configuration.py (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/configuration.py:53: return
ModelData.get_id('purchase_blanket_order',
On 2021/11/23 09:09:44, pokoli wrote:
> We usually break after ( so 'purchase_blanket_order' should be on a new line.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/locale/en.po (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/locale/en.po:1: #
On 2021/11/23 09:09:44, pokoli wrote:
> locale files should not be part of the review. They will be generated latter
by
> the translation process. 
> 
> P.D: If you have them already translated you can save them in another location
> and upload them to pootle once the module is commited to tryton. This way the
> translations will be autoamtically upladed.

Acknowledged.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/purchase.py (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:36: def check_party(cls, line):
On 2021/11/23 09:09:44, pokoli wrote:
> as it is checking a single line, this can be a instance method: 
> 
> def check_party(self):
>    if line.blanket_order_line:
> 
> Also it should be named check_blanket_order_party to avoid collision with
other
> modules. 

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:47: def check_product(cls, line):
On 2021/11/23 09:09:44, pokoli wrote:
> Idem about instance method and naming. 
> 
> 
> Also, I'm wondering if it won't be better to have a single method that checks
> all blanked_order properties.

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/purchase.xml (right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.xml:14: <field
name="name">Purchase</field>
On 2021/11/23 09:09:44, pokoli wrote:
> Won't be better named "Purchases" ? 

Done.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/purchase.xml:20: </record>
On 2021/11/23 09:09:45, pokoli wrote:
> Where is such action used? I will prefer to keep together the action
definition
> and the keyword where it is used.

Those are used by links in the purchase_blanket_order_form view. I'll add them
in blanket_order.xml and add relate too.

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
File modules/purchase_blanket_order/tests/scenario_purchase_blanket_order.rst
(right):

https://codereview.tryton.org/391151002/diff/370261002/modules/purchase_blank...
modules/purchase_blanket_order/tests/scenario_purchase_blanket_order.rst:104:
>>> blanket_order_line.remaining_quantity
On 2021/11/23 09:09:45, pokoli wrote:
> Do we need to test that the remaining quantity ignores cancelled purchases? 

And maybe i should not take care of cancelled purchases for remaining quantity.

[reviewbot, 2021-11-23 17:27:31]

flake8 OK
URL: https://codereview.tryton.org/391151002

[ced, 2021-11-24 12:23:17]

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
File modules/purchase_blanket_order/blanket_order.py (right):

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:23: __name__ =
'purchase.blanket_order'
I think I would name it:

purchase.blanket_agreement

and put it in purchase.py

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:103: ('done', "Done"),
I think Closed is better word.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:170: def __setup__(cls):
define __setup__ before the on_change/getter

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:178: ('running', 'done'),
We should allow to reset to draft to allow editing.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:260: def delete(cls, orders):
Missing super call.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:277:
where=purchase_blanket_order_line.order.in_(order_ids))
Could overflow the query size.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:292: order=order.rec_name))
I think all those checks can be solved by setting the proper ondelete CASCADE
and RESTRICT.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:358: description =
fields.Text("Description", size=None,
size=None is useless.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:371: 'product.uom', "Unit",
ondelete='RESTRICT',
Missing a domain to use the same as product.
But it should take care of secondary unit like in purchase requisition.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:564: order =
fields.Many2One('purchase.blanket_order.to.purchase', "Order")
For me it is useless.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:566: "Purchase Blanket Order
Line")
strange indent.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:569: description =
fields.Text("Description", size=None)
For me summary and description are not needed.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:572: digits=(16,
Eval('unit_digits', 2)),
Use digits='unit'

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:575: unit =
fields.Many2One('product.uom', "Unit", readonly=True)
It should be editable with a domain on category.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:585: order =
fields.Many2One('purchase.blanket_order', "Order")
For me it is useless.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:587:
'purchase.blanket_order.line.to.purchase.line', 'order', "Lines")
target field could be None.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:636: purchase_line.unit_price =
line.unit_price
If it is empty use the one on the blanket order line.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
File modules/purchase_blanket_order/purchase.py (right):

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:25: 'purchase.blanket_order.line',
"Purchase Blanket Order Line",
Should be ondelete RESTRICT.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:27: ('order.state', '=', 'running'),
It should include done and cancel.
Indeed the domain should be applied only for draft or quotation purchase.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:29: Eval('_parent_purchase',
{}).get('party'),
I think the clause should always be applied even if party is empty.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:33: ),
There should be a clause on product.
But only for draft and quotation state because amendment could change it later.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:38: if
self.blanket_order_line.order.supplier != self.purchase.party:
I think it should be done with a domain.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:45: if self.product !=
self.blanket_order_line.product:
I think it should be done with a domain.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:59: 
There should be an on_change_product to remove the blanket order if it is no
more compatible with the product (and the party).

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:71: self.taxes =
self.compute_taxes(self.purchase.party)
I think the onchange should only set the unit price from the blanket order.
And the quantity and the description if they were not set by the user.

[mrichez, 2021-11-30 09:48:13]

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
File modules/purchase_blanket_order/blanket_order.py (right):

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:23: __name__ =
'purchase.blanket_order'
On 2021/11/24 12:23:16, ced wrote:
> I think I would name it:
> 
> purchase.blanket_agreement
> 
> and put it in purchase.py

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:103: ('done', "Done"),
On 2021/11/24 12:23:17, ced wrote:
> I think Closed is better word.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:170: def __setup__(cls):
On 2021/11/24 12:23:17, ced wrote:
> define __setup__ before the on_change/getter

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:178: ('running', 'done'),
On 2021/11/24 12:23:17, ced wrote:
> We should allow to reset to draft to allow editing.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:260: def delete(cls, orders):
On 2021/11/24 12:23:16, ced wrote:
> Missing super call.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:277:
where=purchase_blanket_order_line.order.in_(order_ids))
On 2021/11/24 12:23:16, ced wrote:
> Could overflow the query size.

Acknowledged.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:292: order=order.rec_name))
On 2021/11/24 12:23:17, ced wrote:
> I think all those checks can be solved by setting the proper ondelete CASCADE
> and RESTRICT.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:358: description =
fields.Text("Description", size=None,
On 2021/11/24 12:23:16, ced wrote:
> size=None is useless.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:371: 'product.uom', "Unit",
ondelete='RESTRICT',
On 2021/11/24 12:23:17, ced wrote:
> Missing a domain to use the same as product.
> But it should take care of secondary unit like in purchase requisition.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:564: order =
fields.Many2One('purchase.blanket_order.to.purchase', "Order")
On 2021/11/24 12:23:16, ced wrote:
> For me it is useless.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:566: "Purchase Blanket Order
Line")
On 2021/11/24 12:23:17, ced wrote:
> strange indent.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:569: description =
fields.Text("Description", size=None)
On 2021/11/24 12:23:16, ced wrote:
> For me summary and description are not needed.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:572: digits=(16,
Eval('unit_digits', 2)),
On 2021/11/24 12:23:16, ced wrote:
> Use digits='unit'

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:575: unit =
fields.Many2One('product.uom', "Unit", readonly=True)
On 2021/11/24 12:23:16, ced wrote:
> It should be editable with a domain on category.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:585: order =
fields.Many2One('purchase.blanket_order', "Order")
On 2021/11/24 12:23:17, ced wrote:
> For me it is useless.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:587:
'purchase.blanket_order.line.to.purchase.line', 'order', "Lines")
On 2021/11/24 12:23:17, ced wrote:
> target field could be None.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/blanket_order.py:636: purchase_line.unit_price =
line.unit_price
On 2021/11/24 12:23:16, ced wrote:
> If it is empty use the one on the blanket order line.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
File modules/purchase_blanket_order/purchase.py (right):

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:25: 'purchase.blanket_order.line',
"Purchase Blanket Order Line",
On 2021/11/24 12:23:17, ced wrote:
> Should be ondelete RESTRICT.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:27: ('order.state', '=', 'running'),
On 2021/11/24 12:23:17, ced wrote:
> It should include done and cancel.
> Indeed the domain should be applied only for draft or quotation purchase.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:29: Eval('_parent_purchase',
{}).get('party'),
On 2021/11/24 12:23:17, ced wrote:
> I think the clause should always be applied even if party is empty.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:33: ),
On 2021/11/24 12:23:17, ced wrote:
> There should be a clause on product.
> But only for draft and quotation state because amendment could change it
later.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:38: if
self.blanket_order_line.order.supplier != self.purchase.party:
On 2021/11/24 12:23:17, ced wrote:
> I think it should be done with a domain.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:45: if self.product !=
self.blanket_order_line.product:
On 2021/11/24 12:23:17, ced wrote:
> I think it should be done with a domain.

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:59: 
On 2021/11/24 12:23:17, ced wrote:
> There should be an on_change_product to remove the blanket order if it is no
> more compatible with the product (and the party).

Done.

https://codereview.tryton.org/391151002/diff/383651075/modules/purchase_blank...
modules/purchase_blanket_order/purchase.py:71: self.taxes =
self.compute_taxes(self.purchase.party)
On 2021/11/24 12:23:17, ced wrote:
> I think the onchange should only set the unit price from the blanket order.
> And the quantity and the description if they were not set by the user.

Done.

[reviewbot, 2021-11-30 10:01:40]

flake8 OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2021-11-30 16:01:33]

flake8 OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2021-12-15 15:07:24]

ERROR: /tmp/reviewbot-2zzsen8g/modules/purchase_blanket_agreement/__init__.py
Imports are incorrectly sorted and/or formatted.
ERROR:
/tmp/reviewbot-2zzsen8g/modules/purchase_blanket_agreement/configuration.py
Imports are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/purchase_blanket_agreement/purchase.py
Imports are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/purchase_blanket_agreement/setup.py
Imports are incorrectly sorted and/or formatted.
ERROR:
/tmp/reviewbot-2zzsen8g/modules/purchase_blanket_agreement/tests/__init__.py
Imports are incorrectly sorted and/or formatted.
ERROR:
/tmp/reviewbot-2zzsen8g/modules/purchase_blanket_agreement/tests/test_purchase_blanket_agreement.py
Imports are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/sale_blanket_agreement/__init__.py
Imports are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/sale_blanket_agreement/configuration.py
Imports are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/sale_blanket_agreement/sale.py Imports
are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/sale_blanket_agreement/setup.py Imports
are incorrectly sorted and/or formatted.
ERROR: /tmp/reviewbot-2zzsen8g/modules/sale_blanket_agreement/tests/__init__.py
Imports are incorrectly sorted and/or formatted.
ERROR:
/tmp/reviewbot-2zzsen8g/modules/sale_blanket_agreement/tests/test_sale_blanket_agreement.py
Imports are incorrectly sorted and/or formatted.

URL: https://codereview.tryton.org/391151002

[reviewbot, 2021-12-16 08:32:03]

checks OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2022-01-18 15:54:41]

checks OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2022-02-14 14:53:06]

checks OK
URL: https://codereview.tryton.org/391151002

[reviewbot, 2022-03-18 16:09:15]

checks OK
URL: https://codereview.tryton.org/391151002

[ced, 2022-03-30 12:17:57]

I reviewed only the purchase part but normally my comments should apply also on
the sale part which is mainly similar.

https://codereview.tryton.org/391151002/diff/380561002/.hgsubstate
File .hgsubstate (right):

https://codereview.tryton.org/391151002/diff/380561002/.hgsubstate#newcode1
.hgsubstate:1: 1ddf373597a28566f47b8817cc3edce96fd0b446 modules/account
This file should not be modified.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/__init__.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/__init__.py:11:
configuration.ConfigurationSequence,
Both should be defined in purchase as their names are purchase.configuration

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/__init__.py:16:
purchase.PurchaseBlanketAgreementLineToPurchaseLine,
All should drop the extra 'Purchase' in the name as they are in the purchase
submodule.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/configuration.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:13: 'ir.sequence', "Purchase
Blanket Agreement Sequence",
should be indented of 8 spaces as there is two opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:17: [Eval('context',
{}).get('company', -1), None]),
indentation should be increased.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:19:
Id('purchase_blanket_agreement',
idem

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:35:
).default_purchase_blanket_agreement_sequence()
indentation should be increased

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:47:
'sequence_type_purchase_blanket_agreement')),
idem for indentation.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:53: ModelData =
Pool().get('ir.model.data')
instantiate pool separately.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/design.rst (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/design.rst:4: 
This does not follow the style of design documentation. Look for example at
purchase documentation.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/index.rst (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:26: 
Introduction should be shorter.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/usage.rst (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/usage.rst:3: *****
If there is no usage documentation, file should be deleted.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:25: ('agreement.state', 'in',
['done', 'cancelled', 'running']),
I think it should be only allowed to select running.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:29: Eval('_parent_purchase',
{}).get('party'),
no need for trailing coma at the end of If statement.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:31: ],
should be indented.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:34:
@fields.depends('blanket_agreement_line', '_parent_purchase.party',
It is probably cleaner to put each one on a new line.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:40: 'purchase',
methods=['compute_taxes'])
better to put the purchase and _parent_purchase together.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:43: self.product =
self.blanket_agreement_line.product
What is the goal? The domain already ensure they are the same.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:46: self.unit =
self.blanket_agreement_line.unit
I think unit should set only if it is empty.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:48: self.unit_price =
self.blanket_agreement_line.unit_price
I think that it should be calling in compute_unit_price and that
'blanket_agreement_line' should be added in the '_get_context_purchase_price'
and that Product.get_purchase_price use it to return the blanket price.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:49: if not self.quantity:
I think the quantity must always be set to remaining quantity.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:50: self.quantity =
self.blanket_agreement_line.remaining_quantity
The quantity must be converted into the self.unit.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:61: 
I think that on quotation and confirm, we should raise a warning if the
remaining quantity of an agreement line goes below 0.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:74: },
wrong indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:77: reference =
fields.Char("Reference", select=True,
better to break line at opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:81: )
wrong indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:82: supplier =
fields.Many2One('party.party', "Supplier", required=True,
idem breaking line at opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:85: 'readonly': ((Eval('state')
!= 'draft')
better to break line at opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:86: | (Eval('lines', [0]) &
Eval('supplier'))),
wrong indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:87: },
wrong indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:90: },
intent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:107: ('from_date', '=', None),
It is simpler to use:

If(Eval('to_date'),
   ('from_date', '<=', Eval('to_Date')),
   ())

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:118: ('to_date', '=', None),
idem

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:122: 'required':
~Eval('state').in_(['draft', 'cancelled']),
I'm not sure that all blanket agreement will have an end date so we should not
make it required and let it editable when running.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:139: "Amount",
currency='currency', digits='currency'),
indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:159: def view_attributes(cls):
usually defined just before the CRWD functions.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:166: cls._order = [
Maybe we could insert the from_date DESC NULLS FIRST.
It will look less random to the user.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:168: ]
indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:173: ('running', 'closed'),
I think we should allow to come back from mistaken close to running (but only by
purchase admin).

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:175: ))
indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:212: Company =
Pool().get('company.company')
instantiate pool separately.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:235: Decimal(0))
It will be better to use a SQL query with SUM to avoid reading all the lines.
And so we loose the on_change behavior but I do not think it is a real problem
as we do not have it neither on purchase.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:242: return
self.on_change_with_amount()
Why not use directly the on_change as getter.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:245: def copy(cls,
blanket_agreements, default=None):
maybe it will be simpler to always use "agreements" for the list of records.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:249: default = default.copy()
could be a one-liner like:

default = default.copy() if default is not None else {}

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:251: default.setdefault('state',
'draft')
No need to reset state, Workflow does it.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:265: values['number'] =
config.get_multivalue(
I think it will be better to set a number on running transition.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:270: def get_visual(self, *a):
getter methods are defined because CRWD methods.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:276: today =
pool.get('ir.date').today()
get the date only once

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:278: return 'danger'
I think danger is a little bit too high for a late closing, I guess warning is
better.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:293: purchase.on_change_party()
Calling on_change_party may change the currency.
So it will be better to set explicitly the methods and payment term etc.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:300: pass  # TODO
why is there a TODO?

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:312: pass  # TODO
could set the from_date to day for agreements without one.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:317: def
call_wizard_create_purchase(cls, blanket_agreements):
Probably better named 'create_purchase_wizard'

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:324: pass  # TODO
Should probably fill the to_date with today if it is not yet filled.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:324: pass  # TODO
Probably need to raise a warning if closing an agreement with a to_date after
today and there is remaining lines.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:332:
'purchase.blanket_agreement', "Purchase Blanket Agreement",
I think it can be just labeled "Agreement" like the name.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:341: product =
fields.Many2One('product.product', "Product",
break line on opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:342: ondelete='RESTRICT',
I think the product should be required.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:348: ],
indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:350: 'readonly':
((Eval('purchase_blanket_agreement_state') != 'draft')
break line on opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:360: 'quantity':
Eval('quantity'),
I'm not sure we need to the quantity when selecting the product because such
agreement is for the long term.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:380: | (Eval('purchase_lines',
[0]) & Eval('product_supplier')))
I do not think we need this test because there can be only purchase lines for
running agreement.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:389: | (Eval('purchase_lines',
[0]) & Eval('description')))
idem

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:393: quantity =
fields.Float("Quantity", required=True,
breaking line at opening (

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:401: 'product.uom', "Unit",
ondelete='RESTRICT',
I think unit should always be required.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:417: "Amount", digits='currency',
currency='currency'),
indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:422: processed_quantity =
fields.Function(
I do not think we need such computation but only remaining quantity is enough.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:430:
purchase_blanket_agreement_state = fields.Function(
could be named agreement_state because the Many2One is named agreement.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:516: def
compute_unit_price(self):
I'm not sure we should compute a price. The blanket agreement is specially about
agreeing on a price.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:551: for line in
self.purchase_lines:
It will be better to use a SQL query to sum the actual quantity of the purchase
lines.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:552: if line.purchase.state not
in self._purchase_states():
I do not think we need to care about the purchase state but only about the
actual quantity.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:558: quantity =
line.actual_quantity or line.quantity or 0.
Should distinct between actual_quantity being 0 and None.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:614: PurchaseLine =
Pool().get('purchase.line')
instantiate pool separately.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:620: )
indent

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:623:
purchase_line.on_change_product()
I think we should set the unit price after calling on_change_product.

Indeed it should be done similar to compute_purchase_line of
purchase.request.create_purchase (but without the grouping key).

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:631: default = default.copy()
purchase_lines must be cleared.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:631: default = default.copy()
could be oneliner

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:634: def get_rec_name(self,
name):
should be declared with the other getter before the CRWD methods.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:636: f'{self.agreement.rec_name}
- {self.product.rec_name} [{self.id}]')
I do not see the point to display the id.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:641: __name__ =
'purchase.blanket_agreement.line.to.purchase.line'
I think the name should be purchase.blanket_agreement.create_purchase.line

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:650: quantity =
fields.Float("Quantity", required=True, digits='unit')
I think quantity must be limited to remaining quantity.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:678: __name__ =
'purchase.blanket_agreement.to.purchase'
I think the name could be:

purchase.blanket_agreement.create_purchase

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:684: class
PurchaseBlanketAgreementCreatePurchase(Wizard):
Easier to read if the wizard if defined before the ModelView.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:703: 'unit_price':
line.unit_price}
It will be good to construct this dict in a separate method so it can be
extended easily.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:722: lines.append(purchase_line)
I think this will be better defined in the create_purchase method on the
agreement which takes optional argument a list of lines, then updating here the
quantity.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:26: <record model="ir.ui.view"
id="purchase_blanket_agreement_view_tree">
Better to use _list suffix when it is not a tree.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:35: <field
name="search_value"></field>
no need to specify empty field.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:37: <record
model="ir.action.act_window.view" id="act_purchase_blanket_agreement_tree_view">
Better to use _view1 as suffix.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:61: <record
model="ir.action.act_window.domain"
id="act_purchase_blanket_agreement_domain_closed">
I do not think we need a tab for closed as it is final state.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:68: <record
model="ir.action.act_window.domain"
id="act_purchase_blanket_agreement_domain_cancelled">
I do not think we need a tab for cancelled.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:86: sequence="15"
We use only multiple of 10 in std module. So for me here it should be 20 as it
is a main entry but not the principal.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:89: <record
model="ir.ui.menu-res.group"
id="menu_purchase_blanket_agreement_group_purchase_blanket_agreement">
For me other user can see the menu.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:96: <field
name="string">Cancel</field>
final state button should have a confirm

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:100: <record
model="ir.model.button-res.group"
id="purchase_blanket_agreement_cancel_button_group_purchase_blanket_agreement">
For me any one who has write access to agreement can use the button. So there is
no need to define group for buttons.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:111: <record
model="ir.model.button-res.group"
id="purchase_blanket_agreement_draft_button_group_purchase_blanket_agreement">
idem for all buttons.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:140: <field
name="string">Close</field>
final state button should have a confirm.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:155: <record model="ir.ui.view"
id="purchase_blanket_agreement_line_view_tree">
_line

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:183:
eval="[If(Eval('active_ids', []) == [Eval('active_id')],
('lines.blanket_agreement_line.agreement.id', '=', Eval('active_id')),
('lines.blanket_agreement_line.agreement.id', 'in', Eval('active_ids')))]"
I do not think we need to change operator to '=' when a single agreement is
selected because it will have no impact on the domain inversion because it is
using the One2Many.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:189: <field
name="model">purchase.purchase,-1</field>
It should be purchase agreement model.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:203: <field
name="model">purchase.line,-1</field>
It should be purchase agreement model.
But indeed I think the relate would be better on the agreement line.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:207: <record
model="ir.rule.group" id="rule_group_purchase_blanket_agreement_companies">
better to define rule after the access.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:217: <record
model="ir.model.access" id="access_purchase_blanket_agreement_all_user">
better to define access after the menuitem

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:217: <record
model="ir.model.access" id="access_purchase_blanket_agreement_all_user">
we do not suffix with '_all_user'.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:224: <record
model="ir.model.access" id="access_purchase_blanket_agreement_line_all_user">
No need for access for line as it inherit from agreement.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:230: </record>
I think we should give read access to purchase group.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:239: <record
model="ir.model.access"
id="access_purchase_blanket_agreement_line_for_group_purchase_blanket_agreement">
no need access for line.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/setup.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/setup.py:140: 'Programming Language :: Python
:: 3.6',
drop 3.6

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/setup.py:143: 'Programming Language :: Python
:: 3.9',
add 3.10

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/setup.py:149: python_requires='>=3.6',
3.7

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:23:
... ['purchase_blanket_agreement', 'purchase_secondary_unit'])
I do not think it is correct to test purchase_secondary_unit behavior here.
It should be tested in the module itself.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:24:

better to get all Models here.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:35:
>>> revenue = accounts['revenue']
I think it is better to not define variable for each kind of account but to
always use accounts dict as it is more explicit that it is account than just
'expense' or 'revenue'.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:40:
>>> supplier = Party(name='Supplier')
would be good to use ""

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:50:
>>> account_category.save()
not sure that you need account category as you do not create invoice.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:80:
>>> blanket_agreement.company = company
no need to set company.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:97:
>>> create_purchase.form.lines[0].quantity = 5.0
maybe good to test there is only 1 line.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:100:
>>> purchase, = Purchase.find([('state', '=', 'draft')])
You could use create_purchase.actions to get and test the purchase created.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:108:
>>> blanket_agreement, = PurchaseBlanketAgreement.find([('state', '=',
'running')])
I think it is better to reload the agreement.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:112:

I think it will be good to try to create a purchase manually and link it to an
agreement.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:113:
Create another purchase blanket agreement with secondary unit::
this should be for second unit module.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/tests/test_purchase_blanket_agreement.py
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/test_purchase_blanket_agreement.py:12:
extras = ['purchase_secondary_unit']
it should not be in extras because it is not an extras_depend.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/tox.ini (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tox.ini:2: envlist =
{py36,py37,py38,py39}-{sqlite,postgresql},pypy3-{sqlite,postgresql}
update to remove py36 and add py310

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/tryton.cfg (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tryton.cfg:5: purchase
should also depend on company, product, party, currency.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tryton.cfg:7: configuration.xml
content should be moved in purchase.xml as it is all related to purchase.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/configuration_form.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/configuration_form.xml:5: <xpath
expr="/form/field[@name='purchase_sequence']" position="after">
I do not think it is good to put it next to purchase sequence because it will
put invoice method on a new line.

I think the best is to use insert in /form with a starting new line.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/create_purchase_form.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/create_purchase_form.xml:4: <form>
maybe col="1"

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:20:
<field name="to_date"/>
I think the dates will be better in the page.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:24:
<page string="Purchase Blanket Agreement" id="purchase_blanket_agreement">
Maybe just "Agreement"

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:27:
<group id="states" yfill="1">
I do not think we need a group.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:31:
<group id="amount" yfill="1">
I do not think we need a group.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:40:
<field name="currency"/>
I think currency should be in the same page as lines and amount.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:4:
<form cursor="product">
I do not think we need a cursor.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:9:
<field name="product" view_ids="purchase.product_view_list_purchase_line"/>
I'm wondering if we should not have a dedicated view similar to the purchase one
but without the quantities.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:14:
<field name="quantity" symbol="unit"/>
no need for symbol as unit is next to it.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml:4:
<tree sequence="sequence">
we need one view without sequence as default and one with sequence (which is
used on the agreement form).

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml:7:
<field name="product_supplier"/>
could be optional and expanded.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml:8:
<field name="summary" expand="1"/>
could be optional

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:5:
<field name="number" expand="1"/>
optional

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:6:
<field name="reference" expand="1"/>
optional=0

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:8:
<field name="state"/>
I think state should come after the dates.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:10:
<field name="to_date" expand="1"/>
dates could be optional=0

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:11:
<field name="visual" tree_invisible="1"/>
should added by view_attributes.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_line_form.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_line_form.xml:5: <xpath
expr="/form/notebook/page[@id='general']/group[@id='delivery_date']"
position="after">
For me it should be added after the product/product_supplier line with an ending
newline.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_line_form.xml:5: <xpath
expr="/form/notebook/page[@id='general']/group[@id='delivery_date']"
position="after">
for xpath expr better to use less deterministic form like:
//field[@name='product_supplier']

[mrichez, 2022-05-04 09:17:12]

https://codereview.tryton.org/391151002/diff/380561002/.hgsubstate
File .hgsubstate (right):

https://codereview.tryton.org/391151002/diff/380561002/.hgsubstate#newcode1
.hgsubstate:1: 1ddf373597a28566f47b8817cc3edce96fd0b446 modules/account
On 2022/03/30 12:17:49, ced wrote:
> This file should not be modified.

Acknowledged.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/__init__.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/__init__.py:11:
configuration.ConfigurationSequence,
On 2022/03/30 12:17:49, ced wrote:
> Both should be defined in purchase as their names are purchase.configuration

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/__init__.py:16:
purchase.PurchaseBlanketAgreementLineToPurchaseLine,
On 2022/03/30 12:17:49, ced wrote:
> All should drop the extra 'Purchase' in the name as they are in the purchase
> submodule.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/configuration.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:13: 'ir.sequence', "Purchase
Blanket Agreement Sequence",
On 2022/03/30 12:17:49, ced wrote:
> should be indented of 8 spaces as there is two opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:17: [Eval('context',
{}).get('company', -1), None]),
On 2022/03/30 12:17:49, ced wrote:
> indentation should be increased.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:19:
Id('purchase_blanket_agreement',
On 2022/03/30 12:17:49, ced wrote:
> idem

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:35:
).default_purchase_blanket_agreement_sequence()
On 2022/03/30 12:17:49, ced wrote:
> indentation should be increased

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:47:
'sequence_type_purchase_blanket_agreement')),
On 2022/03/30 12:17:49, ced wrote:
> idem for indentation.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/configuration.py:53: ModelData =
Pool().get('ir.model.data')
On 2022/03/30 12:17:49, ced wrote:
> instantiate pool separately.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/design.rst (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/design.rst:4: 
On 2022/03/30 12:17:49, ced wrote:
> This does not follow the style of design documentation. Look for example at
> purchase documentation.

Acknowledged.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/index.rst (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:26: 
On 2022/03/30 12:17:49, ced wrote:
> Introduction should be shorter.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/usage.rst (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/usage.rst:3: *****
On 2022/03/30 12:17:49, ced wrote:
> If there is no usage documentation, file should be deleted.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:25: ('agreement.state', 'in',
['done', 'cancelled', 'running']),
On 2022/03/30 12:17:50, ced wrote:
> I think it should be only allowed to select running.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:29: Eval('_parent_purchase',
{}).get('party'),
On 2022/03/30 12:17:50, ced wrote:
> no need for trailing coma at the end of If statement.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:31: ],
On 2022/03/30 12:17:53, ced wrote:
> should be indented.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:34:
@fields.depends('blanket_agreement_line', '_parent_purchase.party',
On 2022/03/30 12:17:50, ced wrote:
> It is probably cleaner to put each one on a new line.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:34:
@fields.depends('blanket_agreement_line', '_parent_purchase.party',
On 2022/03/30 12:17:50, ced wrote:
> It is probably cleaner to put each one on a new line.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:40: 'purchase',
methods=['compute_taxes'])
On 2022/03/30 12:17:51, ced wrote:
> better to put the purchase and _parent_purchase together.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:43: self.product =
self.blanket_agreement_line.product
On 2022/03/30 12:17:52, ced wrote:
> What is the goal? The domain already ensure they are the same.

Acknowledged.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:46: self.unit =
self.blanket_agreement_line.unit
On 2022/03/30 12:17:52, ced wrote:
> I think unit should set only if it is empty.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:48: self.unit_price =
self.blanket_agreement_line.unit_price
On 2022/03/30 12:17:51, ced wrote:
> I think that it should be calling in compute_unit_price and that
> 'blanket_agreement_line' should be added in the '_get_context_purchase_price'
> and that Product.get_purchase_price use it to return the blanket price.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:49: if not self.quantity:
On 2022/03/30 12:17:51, ced wrote:
> I think the quantity must always be set to remaining quantity.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:50: self.quantity =
self.blanket_agreement_line.remaining_quantity
On 2022/03/30 12:17:52, ced wrote:
> The quantity must be converted into the self.unit.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:61: 
On 2022/03/30 12:17:50, ced wrote:
> I think that on quotation and confirm, we should raise a warning if the
> remaining quantity of an agreement line goes below 0.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:74: },
On 2022/03/30 12:17:53, ced wrote:
> wrong indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:77: reference =
fields.Char("Reference", select=True,
On 2022/03/30 12:17:53, ced wrote:
> better to break line at opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:81: )
On 2022/03/30 12:17:52, ced wrote:
> wrong indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:82: supplier =
fields.Many2One('party.party', "Supplier", required=True,
On 2022/03/30 12:17:51, ced wrote:
> idem breaking line at opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:85: 'readonly': ((Eval('state')
!= 'draft')
On 2022/03/30 12:17:50, ced wrote:
> better to break line at opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:86: | (Eval('lines', [0]) &
Eval('supplier'))),
On 2022/03/30 12:17:52, ced wrote:
> wrong indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:87: },
On 2022/03/30 12:17:49, ced wrote:
> wrong indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:90: },
On 2022/03/30 12:17:50, ced wrote:
> intent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:107: ('from_date', '=', None),
On 2022/03/30 12:17:52, ced wrote:
> It is simpler to use:
> 
> If(Eval('to_date'),
>    ('from_date', '<=', Eval('to_Date')),
>    ())

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:118: ('to_date', '=', None),
On 2022/03/30 12:17:50, ced wrote:
> idem

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:122: 'required':
~Eval('state').in_(['draft', 'cancelled']),
On 2022/03/30 12:17:50, ced wrote:
> I'm not sure that all blanket agreement will have an end date so we should not
> make it required and let it editable when running.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:139: "Amount",
currency='currency', digits='currency'),
On 2022/03/30 12:17:50, ced wrote:
> indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:159: def view_attributes(cls):
On 2022/03/30 12:17:51, ced wrote:
> usually defined just before the CRWD functions.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:166: cls._order = [
On 2022/03/30 12:17:50, ced wrote:
> Maybe we could insert the from_date DESC NULLS FIRST.
> It will look less random to the user.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:168: ]
On 2022/03/30 12:17:51, ced wrote:
> indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:173: ('running', 'closed'),
On 2022/03/30 12:17:51, ced wrote:
> I think we should allow to come back from mistaken close to running (but only
by
> purchase admin).

Done.
How to do such thing? 
I define conditions on invisible state of button.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:175: ))
On 2022/03/30 12:17:51, ced wrote:
> indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:212: Company =
Pool().get('company.company')
On 2022/03/30 12:17:52, ced wrote:
> instantiate pool separately.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:235: Decimal(0))
On 2022/03/30 12:17:53, ced wrote:
> It will be better to use a SQL query with SUM to avoid reading all the lines.
> And so we loose the on_change behavior but I do not think it is a real problem
> as we do not have it neither on purchase.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:242: return
self.on_change_with_amount()
On 2022/03/30 12:17:52, ced wrote:
> Why not use directly the on_change as getter.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:245: def copy(cls,
blanket_agreements, default=None):
On 2022/03/30 12:17:51, ced wrote:
> maybe it will be simpler to always use "agreements" for the list of records.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:249: default = default.copy()
On 2022/03/30 12:17:53, ced wrote:
> could be a one-liner like:
> 
> default = default.copy() if default is not None else {}

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:251: default.setdefault('state',
'draft')
On 2022/03/30 12:17:49, ced wrote:
> No need to reset state, Workflow does it.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:265: values['number'] =
config.get_multivalue(
On 2022/03/30 12:17:52, ced wrote:
> I think it will be better to set a number on running transition.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:270: def get_visual(self, *a):
On 2022/03/30 12:17:50, ced wrote:
> getter methods are defined because CRWD methods.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:276: today =
pool.get('ir.date').today()
On 2022/03/30 12:17:52, ced wrote:
> get the date only once

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:278: return 'danger'
On 2022/03/30 12:17:53, ced wrote:
> I think danger is a little bit too high for a late closing, I guess warning is
> better.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:293: purchase.on_change_party()
On 2022/03/30 12:17:52, ced wrote:
> Calling on_change_party may change the currency.
> So it will be better to set explicitly the methods and payment term etc.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:300: pass  # TODO
On 2022/03/30 12:17:51, ced wrote:
> why is there a TODO?

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:312: pass  # TODO
On 2022/03/30 12:17:52, ced wrote:
> could set the from_date to day for agreements without one.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:317: def
call_wizard_create_purchase(cls, blanket_agreements):
On 2022/03/30 12:17:51, ced wrote:
> Probably better named 'create_purchase_wizard'

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:324: pass  # TODO
On 2022/03/30 12:17:53, ced wrote:
> Should probably fill the to_date with today if it is not yet filled.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:332:
'purchase.blanket_agreement', "Purchase Blanket Agreement",
On 2022/03/30 12:17:51, ced wrote:
> I think it can be just labeled "Agreement" like the name.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:341: product =
fields.Many2One('product.product', "Product",
On 2022/03/30 12:17:52, ced wrote:
> break line on opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:342: ondelete='RESTRICT',
On 2022/03/30 12:17:53, ced wrote:
> I think the product should be required.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:348: ],
On 2022/03/30 12:17:53, ced wrote:
> indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:350: 'readonly':
((Eval('purchase_blanket_agreement_state') != 'draft')
On 2022/03/30 12:17:52, ced wrote:
> break line on opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:360: 'quantity':
Eval('quantity'),
On 2022/03/30 12:17:51, ced wrote:
> I'm not sure we need to the quantity when selecting the product because such
> agreement is for the long term.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:380: | (Eval('purchase_lines',
[0]) & Eval('product_supplier')))
On 2022/03/30 12:17:49, ced wrote:
> I do not think we need this test because there can be only purchase lines for
> running agreement.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:389: | (Eval('purchase_lines',
[0]) & Eval('description')))
On 2022/03/30 12:17:53, ced wrote:
> idem

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:393: quantity =
fields.Float("Quantity", required=True,
On 2022/03/30 12:17:52, ced wrote:
> breaking line at opening (

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:401: 'product.uom', "Unit",
ondelete='RESTRICT',
On 2022/03/30 12:17:53, ced wrote:
> I think unit should always be required.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:417: "Amount", digits='currency',
currency='currency'),
On 2022/03/30 12:17:51, ced wrote:
> indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:422: processed_quantity =
fields.Function(
On 2022/03/30 12:17:50, ced wrote:
> I do not think we need such computation but only remaining quantity is enough.

We use it to calculate remaining quantity

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:430:
purchase_blanket_agreement_state = fields.Function(
On 2022/03/30 12:17:52, ced wrote:
> could be named agreement_state because the Many2One is named agreement.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:516: def
compute_unit_price(self):
On 2022/03/30 12:17:52, ced wrote:
> I'm not sure we should compute a price. The blanket agreement is specially
about
> agreeing on a price.

Acknowledged.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:551: for line in
self.purchase_lines:
On 2022/03/30 12:17:53, ced wrote:
> It will be better to use a SQL query to sum the actual quantity of the
purchase
> lines.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:552: if line.purchase.state not
in self._purchase_states():
On 2022/03/30 12:17:51, ced wrote:
> I do not think we need to care about the purchase state but only about the
> actual quantity.
Actual quantity is only updated on purchase process, so we must care about
cancelled purchases.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:558: quantity =
line.actual_quantity or line.quantity or 0.
On 2022/03/30 12:17:52, ced wrote:
> Should distinct between actual_quantity being 0 and None.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:614: PurchaseLine =
Pool().get('purchase.line')
On 2022/03/30 12:17:50, ced wrote:
> instantiate pool separately.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:620: )
On 2022/03/30 12:17:53, ced wrote:
> indent

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:623:
purchase_line.on_change_product()
On 2022/03/30 12:17:50, ced wrote:
> I think we should set the unit price after calling on_change_product.
> 
> Indeed it should be done similar to compute_purchase_line of
> purchase.request.create_purchase (but without the grouping key).

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:631: default = default.copy()
On 2022/03/30 12:17:49, ced wrote:
> could be oneliner

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:631: default = default.copy()
On 2022/03/30 12:17:53, ced wrote:
> purchase_lines must be cleared.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:634: def get_rec_name(self,
name):
On 2022/03/30 12:17:51, ced wrote:
> should be declared with the other getter before the CRWD methods.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:636: f'{self.agreement.rec_name}
- {self.product.rec_name} [{self.id}]')
On 2022/03/30 12:17:50, ced wrote:
> I do not see the point to display the id.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:641: __name__ =
'purchase.blanket_agreement.line.to.purchase.line'
On 2022/03/30 12:17:51, ced wrote:
> I think the name should be purchase.blanket_agreement.create_purchase.line

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:650: quantity =
fields.Float("Quantity", required=True, digits='unit')
On 2022/03/30 12:17:52, ced wrote:
> I think quantity must be limited to remaining quantity.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:678: __name__ =
'purchase.blanket_agreement.to.purchase'
On 2022/03/30 12:17:49, ced wrote:
> I think the name could be:
> 
> purchase.blanket_agreement.create_purchase

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:684: class
PurchaseBlanketAgreementCreatePurchase(Wizard):
On 2022/03/30 12:17:52, ced wrote:
> Easier to read if the wizard if defined before the ModelView.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:703: 'unit_price':
line.unit_price}
On 2022/03/30 12:17:53, ced wrote:
> It will be good to construct this dict in a separate method so it can be
> extended easily.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:722: lines.append(purchase_line)
On 2022/03/30 12:17:50, ced wrote:
> I think this will be better defined in the create_purchase method on the
> agreement which takes optional argument a list of lines, then updating here
the
> quantity.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:26: <record model="ir.ui.view"
id="purchase_blanket_agreement_view_tree">
On 2022/03/30 12:17:54, ced wrote:
> Better to use _list suffix when it is not a tree.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:35: <field
name="search_value"></field>
On 2022/03/30 12:17:54, ced wrote:
> no need to specify empty field.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:37: <record
model="ir.action.act_window.view" id="act_purchase_blanket_agreement_tree_view">
On 2022/03/30 12:17:54, ced wrote:
> Better to use _view1 as suffix.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:61: <record
model="ir.action.act_window.domain"
id="act_purchase_blanket_agreement_domain_closed">
On 2022/03/30 12:17:54, ced wrote:
> I do not think we need a tab for closed as it is final state.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:68: <record
model="ir.action.act_window.domain"
id="act_purchase_blanket_agreement_domain_cancelled">
On 2022/03/30 12:17:54, ced wrote:
> I do not think we need a tab for cancelled.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:86: sequence="15"
On 2022/03/30 12:17:54, ced wrote:
> We use only multiple of 10 in std module. So for me here it should be 20 as it
> is a main entry but not the principal.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:89: <record
model="ir.ui.menu-res.group"
id="menu_purchase_blanket_agreement_group_purchase_blanket_agreement">
On 2022/03/30 12:17:54, ced wrote:
> For me other user can see the menu.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:96: <field
name="string">Cancel</field>
On 2022/03/30 12:17:54, ced wrote:
> final state button should have a confirm

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:100: <record
model="ir.model.button-res.group"
id="purchase_blanket_agreement_cancel_button_group_purchase_blanket_agreement">
On 2022/03/30 12:17:54, ced wrote:
> For me any one who has write access to agreement can use the button. So there
is
> no need to define group for buttons.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:111: <record
model="ir.model.button-res.group"
id="purchase_blanket_agreement_draft_button_group_purchase_blanket_agreement">
On 2022/03/30 12:17:54, ced wrote:
> idem for all buttons.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:140: <field
name="string">Close</field>
On 2022/03/30 12:17:54, ced wrote:
> final state button should have a confirm.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:155: <record model="ir.ui.view"
id="purchase_blanket_agreement_line_view_tree">
On 2022/03/30 12:17:54, ced wrote:
> _line

?

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:183:
eval="[If(Eval('active_ids', []) == [Eval('active_id')],
('lines.blanket_agreement_line.agreement.id', '=', Eval('active_id')),
('lines.blanket_agreement_line.agreement.id', 'in', Eval('active_ids')))]"
On 2022/03/30 12:17:55, ced wrote:
> I do not think we need to change operator to '=' when a single agreement is
> selected because it will have no impact on the domain inversion because it is
> using the One2Many.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:189: <field
name="model">purchase.purchase,-1</field>
On 2022/03/30 12:17:54, ced wrote:
> It should be purchase agreement model.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:203: <field
name="model">purchase.line,-1</field>
On 2022/03/30 12:17:54, ced wrote:
> It should be purchase agreement model.
> But indeed I think the relate would be better on the agreement line.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:207: <record
model="ir.rule.group" id="rule_group_purchase_blanket_agreement_companies">
On 2022/03/30 12:17:54, ced wrote:
> better to define rule after the access.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:217: <record
model="ir.model.access" id="access_purchase_blanket_agreement_all_user">
On 2022/03/30 12:17:54, ced wrote:
> better to define access after the menuitem

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:217: <record
model="ir.model.access" id="access_purchase_blanket_agreement_all_user">
On 2022/03/30 12:17:54, ced wrote:
> we do not suffix with '_all_user'.

Acknowledged.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:224: <record
model="ir.model.access" id="access_purchase_blanket_agreement_line_all_user">
On 2022/03/30 12:17:55, ced wrote:
> No need for access for line as it inherit from agreement.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:230: </record>
On 2022/03/30 12:17:55, ced wrote:
> I think we should give read access to purchase group.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:239: <record
model="ir.model.access"
id="access_purchase_blanket_agreement_line_for_group_purchase_blanket_agreement">
On 2022/03/30 12:17:53, ced wrote:
> no need access for line.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/setup.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/setup.py:140: 'Programming Language :: Python
:: 3.6',
On 2022/03/30 12:17:55, ced wrote:
> drop 3.6

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/setup.py:143: 'Programming Language :: Python
:: 3.9',
On 2022/03/30 12:17:55, ced wrote:
> add 3.10

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/setup.py:149: python_requires='>=3.6',
On 2022/03/30 12:17:55, ced wrote:
> 3.7

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:23:
... ['purchase_blanket_agreement', 'purchase_secondary_unit'])
On 2022/03/30 12:17:56, ced wrote:
> I do not think it is correct to test purchase_secondary_unit behavior here.
> It should be tested in the module itself.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:24:

On 2022/03/30 12:17:55, ced wrote:
> better to get all Models here.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:35:
>>> revenue = accounts['revenue']
On 2022/03/30 12:17:56, ced wrote:
> I think it is better to not define variable for each kind of account but to
> always use accounts dict as it is more explicit that it is account than just
> 'expense' or 'revenue'.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:40:
>>> supplier = Party(name='Supplier')
On 2022/03/30 12:17:55, ced wrote:
> would be good to use ""

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:50:
>>> account_category.save()
On 2022/03/30 12:17:55, ced wrote:
> not sure that you need account category as you do not create invoice.

Needed when calling compute_taxes on purchase_line

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:80:
>>> blanket_agreement.company = company
On 2022/03/30 12:17:55, ced wrote:
> no need to set company.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:97:
>>> create_purchase.form.lines[0].quantity = 5.0
On 2022/03/30 12:17:55, ced wrote:
> maybe good to test there is only 1 line.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:100:
>>> purchase, = Purchase.find([('state', '=', 'draft')])
On 2022/03/30 12:17:55, ced wrote:
> You could use create_purchase.actions to get and test the purchase created.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:108:
>>> blanket_agreement, = PurchaseBlanketAgreement.find([('state', '=',
'running')])
On 2022/03/30 12:17:55, ced wrote:
> I think it is better to reload the agreement.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:112:

On 2022/03/30 12:17:55, ced wrote:
> I think it will be good to try to create a purchase manually and link it to an
> agreement.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:113:
Create another purchase blanket agreement with secondary unit::
On 2022/03/30 12:17:55, ced wrote:
> this should be for second unit module.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/tests/test_purchase_blanket_agreement.py
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/test_purchase_blanket_agreement.py:12:
extras = ['purchase_secondary_unit']
On 2022/03/30 12:17:56, ced wrote:
> it should not be in extras because it is not an extras_depend.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/tox.ini (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tox.ini:2: envlist =
{py36,py37,py38,py39}-{sqlite,postgresql},pypy3-{sqlite,postgresql}
On 2022/03/30 12:17:56, ced wrote:
> update to remove py36 and add py310

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/tryton.cfg (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tryton.cfg:5: purchase
On 2022/03/30 12:17:56, ced wrote:
> should also depend on company, product, party, currency.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/tryton.cfg:7: configuration.xml
On 2022/03/30 12:17:56, ced wrote:
> content should be moved in purchase.xml as it is all related to purchase.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/configuration_form.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/configuration_form.xml:5: <xpath
expr="/form/field[@name='purchase_sequence']" position="after">
On 2022/03/30 12:17:56, ced wrote:
> I do not think it is good to put it next to purchase sequence because it will
> put invoice method on a new line.
> 
> I think the best is to use insert in /form with a starting new line.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/create_purchase_form.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/create_purchase_form.xml:4: <form>
On 2022/03/30 12:17:56, ced wrote:
> maybe col="1"

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:20:
<field name="to_date"/>
On 2022/03/30 12:17:56, ced wrote:
> I think the dates will be better in the page.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:24:
<page string="Purchase Blanket Agreement" id="purchase_blanket_agreement">
On 2022/03/30 12:17:56, ced wrote:
> Maybe just "Agreement"

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:27:
<group id="states" yfill="1">
On 2022/03/30 12:17:56, ced wrote:
> I do not think we need a group.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:31:
<group id="amount" yfill="1">
On 2022/03/30 12:17:56, ced wrote:
> I do not think we need a group.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:40:
<field name="currency"/>
On 2022/03/30 12:17:56, ced wrote:
> I think currency should be in the same page as lines and amount.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:4:
<form cursor="product">
On 2022/03/30 12:17:56, ced wrote:
> I do not think we need a cursor.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:9:
<field name="product" view_ids="purchase.product_view_list_purchase_line"/>
On 2022/03/30 12:17:56, ced wrote:
> I'm wondering if we should not have a dedicated view similar to the purchase
one
> but without the quantities.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:14:
<field name="quantity" symbol="unit"/>
On 2022/03/30 12:17:56, ced wrote:
> no need for symbol as unit is next to it.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml:4:
<tree sequence="sequence">
On 2022/03/30 12:17:56, ced wrote:
> we need one view without sequence as default and one with sequence (which is
> used on the agreement form).

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml:7:
<field name="product_supplier"/>
On 2022/03/30 12:17:56, ced wrote:
> could be optional and expanded.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_tree.xml:8:
<field name="summary" expand="1"/>
On 2022/03/30 12:17:56, ced wrote:
> could be optional

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml
(right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:5:
<field name="number" expand="1"/>
On 2022/03/30 12:17:57, ced wrote:
> optional

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:6:
<field name="reference" expand="1"/>
On 2022/03/30 12:17:57, ced wrote:
> optional=0

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:8:
<field name="state"/>
On 2022/03/30 12:17:57, ced wrote:
> I think state should come after the dates.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:10:
<field name="to_date" expand="1"/>
On 2022/03/30 12:17:56, ced wrote:
> dates could be optional=0

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_tree.xml:11:
<field name="visual" tree_invisible="1"/>
On 2022/03/30 12:17:57, ced wrote:
> should added by view_attributes.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_line_form.xml (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_line_form.xml:5: <xpath
expr="/form/notebook/page[@id='general']/group[@id='delivery_date']"
position="after">
On 2022/03/30 12:17:57, ced wrote:
> For me it should be added after the product/product_supplier line with an
ending
> newline.

Done.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_line_form.xml:5: <xpath
expr="/form/notebook/page[@id='general']/group[@id='delivery_date']"
position="after">
On 2022/03/30 12:17:57, ced wrote:
> for xpath expr better to use less deterministic form like:
> //field[@name='product_supplier']

Done.

[reviewbot, 2022-05-04 10:00:55]

https://codereview.tryton.org/391151002/diff/435081003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:395:
continuation line missing indentation or outdented

https://codereview.tryton.org/391151002/diff/435081003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:406:
continuation line missing indentation or outdented

https://codereview.tryton.org/391151002/diff/435081003/modules/sale_blanket_a...
modules/sale_blanket_agreement/sale.py:384:
continuation line missing indentation or outdented

https://codereview.tryton.org/391151002/diff/435081003/modules/sale_blanket_a...
modules/sale_blanket_agreement/sale.py:395:
continuation line missing indentation or outdented


URL: https://codereview.tryton.org/391151002

[reviewbot, 2022-06-20 13:41:44]

checks OK
URL: https://codereview.tryton.org/391151002

[ced, 2022-07-26 11:51:15]

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.py (right):

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:173: ('running', 'closed'),
On 2022/05/04 09:17:07, mrichez wrote:
> On 2022/03/30 12:17:51, ced wrote:
> > I think we should allow to come back from mistaken close to running (but
only
> by
> > purchase admin).
> 
> Done.
> How to do such thing? 
> I define conditions on invisible state of button.

I think it is enough because it is mainly to avoid mistakes not really about
permissions.

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:235: Decimal(0))
On 2022/03/30 12:17:53, ced wrote:
> It will be better to use a SQL query with SUM to avoid reading all the lines.
> And so we loose the on_change behavior but I do not think it is a real problem
> as we do not have it neither on purchase.

Indeed we have on_change_lines on purchase which update the total.
So we should keep the on_change_with_amount (as it is the only field to update
from the lines).

https://codereview.tryton.org/391151002/diff/380561002/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:552: if line.purchase.state not
in self._purchase_states():
On 2022/05/04 09:17:08, mrichez wrote:
> On 2022/03/30 12:17:51, ced wrote:
> > I do not think we need to care about the purchase state but only about the
> > actual quantity.
> Actual quantity is only updated on purchase process, so we must care about
> cancelled purchases.

Acknowledged.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/.drone.yml (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/.drone.yml:31: - twine check dist/*
Missing check_doc

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/.drone.yml:45: - IMAGE: python:3.6
3.6 has been dropped.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/.drone.yml:68: DATABASE: postgresql
Missing 3.10

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/__init__.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/__init__.py:1: from trytond.pool import Pool
Missing copyright header

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/__init__.py:24:
module='purchase_blanket_agreement', type_='report')
Could be removed

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/doc/index.rst (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:5: A blanket agreement is a
long-term agreement between a company and its suppliers.
80 cols

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:6: It can also be named: Frame
contracts. The *Purchase Blanket Agreement Module*
trailing space

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:6: It can also be named: Frame
contracts. The *Purchase Blanket Agreement Module*
One line per sentence.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:8: of products and to request
partial delivery over a period.
trailing space

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/doc/index.rst:10: purchases for needed
quantities of products following the contract.
Missing toctree with at least design but we could probably have also a usage

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/exceptions.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/exceptions.py:7: class
AgreementQuantityWarning(UserWarning):
I think it is more consistant to name it BlanketAgreementQuantityWarning

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/product.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/product.py:16: context =
Transaction().context
usually we get context in the same block as pool.get

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/product.py:20: line = None
I do not see the point

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/product.py:23:
context['blanket_agreement_line'])
I think it can fit on one line.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/product.py:24: if line:
For me this test is always True

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:41:
'.msg_agreement_missing_remaining_quantity'))
I think we must include in the warning message: the record name of the purchase,
the 5 first agreement lines without remaining quantity.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:51:
purchase_blanket_agreement_sequence = fields.MultiValue(fields.Many2One(
For me we do not need to prefix with purchase_

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:60: ]))
wrong indentation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:87: ])
idem

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:103: 
I think it will be good to have a computed field with the blanket_agreements.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:123:
'purchase.blanket_agreement.line', "Purchase Blanket Agreement Line",
Blanket Agreement Line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:134:
@fields.depends('blanket_agreement_line',
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:136:
'_parent_blanket_agreement_line.unit')
I do not understand the depends on product and unit

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:143:
@fields.depends('blanket_agreement_line',
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:162: if self.unit.category ==
self.blanket_agreement_line.unit.category:
I think we must set quantity only if remaining_quantity is > 0

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:166: self.taxes =
self.compute_taxes(self.purchase.party)
For me we do not need to compute the taxes as the product has not changed nor
the party.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:168:
@fields.depends('blanket_agreement_line', 'product',
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:194: 'readonly': Eval('state') !=
'draft',
For me we should always be allowed to change the reference.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:207: contact =
fields.Many2One('party.contact_mechanism', "Contact",
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:210: },
wrong indentation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:213: },
idem

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:216: },
idem

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:218: from_date =
fields.Date("From date",
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:227: })
wrong indentation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:235: 'readonly':
~Eval('state').in_(['draft', 'running']),
also cancelled

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:236: })
idem

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:237: currency =
fields.Many2One('currency.currency', "Currency", required=True,
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:242: })
idem

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:257: lines =
fields.One2Many('purchase.blanket_agreement.line', 'agreement',
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:262: | ~Bool(Eval('supplier')))
no need for Bool()

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:263: })
wrong indentation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:272: ('id', 'DESC'),
Better to insert new clause and keep the existing default id clause.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:283: 'cancel': {
wrong indentation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:298: 'readonly': ~Eval('lines'),
I think it should also be readonly if from_date is in the future.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:301: 'create_purchase_wizard': {
I think it should be named just: create_purchase

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:310: cls._states_cached =
['closed', 'cancelled']
not used

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:324: company =
Transaction().context.get('company')
Better to call default_company

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:342: def get_amount(self,
name=None):
It should be a classmethod to take benefit from the sql query otherwise it has
the same load as looping of the lines.
So I think it is simpler to have only a on_change_with_amount. I do not expect
to have too much agreements at the same time that it will become a performance
issue.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:371: default.setdefault('number',
None)
I think from/to date should also be reset.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:411: })
Both methods have the same code, so they could be factorized to take the field
name as parameter.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:419: def get_visual(self, *a):
*a -> name

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:423: if self.state ==
'cancelled':
also for closed

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:431: def _get_purchase(self,
lines=None):
I think it should be public because it is called by the wizard.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:442: )
identation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:445: for line in lines:
It may be good to assert that the line is linked to self

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:447: purchase_line.unit_price =
(line.unit_price if line.unit_price
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:450: self.supplier)
Both lines could be done in get_purchase_line if we pass the extra arguments.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:491: and any(l.remaining_quantity
> 0 for l in agreement.lines)):
indentation

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:499:
agreement=agreement.rec_name))
display also the 5 first line with remaining quantities

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:505: 
I think we could have a common _states defined here.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:509: select=True, required=True,
could be on a single line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:533: 'uom': Eval('unit'),
We could also add quantity

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:535: depends=['company'])
I think we should add also unit and quantity

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:538: ondelete='RESTRICT',
could be on a single line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:564: digits='unit',
could be on a single line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:571: 'required':
Bool(Eval('product')),
not needed

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:576: required=True,
could be on a single line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:592: purchase_lines =
fields.One2Many('purchase.line', 'blanket_agreement_line',
break line at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:597: "Agreement State"),
could be on a single line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:613: 
It will be good to order methods for fields using the same order as their
definitions.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:648: def _purchase_states(self):
The name of the method must be more explicit like:
_excluded_purchase_states_processed

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:651: @fields.depends('quantity',
'unit_price', 'unit', 'agreement',
I do not see why you need unit

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:662: def
on_change_with_processed_quantity(self, name=None):
Can not use SQL in on_change. But indeed there is no benefit in using on_change
because purchase lines are not added from the UI.
Also to benefit from the SQL, it must be a classmethod.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:688: return processed_quantity
The quantity must be rounded with the unit.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:725: self.amount =
self.on_change_with_amount()
I see no change that requires to recompute the amount.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:758: f'{self.agreement.rec_name}
- {self.product.rec_name}')
usually we use the format: self.product.rec_name @ self.agreement.rec_name

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:758: f'{self.agreement.rec_name}
- {self.product.rec_name}')
It will even be better to include the quantity with format_number_symbol

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:767: class
BlanketAgreementCreatePurchaseLine(ModelView):
better to declare it after BlanketAgreementCreatePurchase

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:768: 'Create Purchase line'
Use " for docstring

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:769: __name__ =
'purchase.blanket_agreement.create_purchase.line'
I'm wondering if we should rename line into selection.
This will be less ambigus with the agreement line.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:771: line =
fields.Many2One('purchase.blanket_agreement.line',
break at opening (

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:781: ('quantity', '<=',
Eval('remaining_quantity', 0))])
Not sur we need such domain because user may create multiple lines for the same
agreement line.
He will have the warning when validate the purchase.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:805: 
I think we need a on_change_line to fill the readonly fields if the user create
a line himself.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:815: ])
wrong indent

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:819: lines = [self.get_line(line)
break line at opening [

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:821: if line.remaining_quantity >
0.
useless trailing .

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:822: ]
wrong indent

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:841:
purchase_line.on_change_quantity()
this should be done in get_purchase.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:844: action['views'].reverse()
Should remove the domain tabs.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.xml (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:18: <record model="res.group"
id="group_purchase_blanket_agreement">
define group first.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:19: <field name="name">Purchases
Blanket Agreement</field>
Purchase Blanket Agreement

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:19: <field name="name">Purchases
Blanket Agreement</field>
I think it should have group_purchase as parent.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:77: <field name="confirm">Are
you sure you want to cancel the agreement?</field>
I do not think we need confirmation as we can go back to draft.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:106: <record model="ir.ui.view"
id="purchase_blanket_agreement_line_view_form">
define lines after all the agreement stuffs.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:112: <record model="ir.ui.view"
id="purchase_blanket_agreement_line_view_tree">
_list

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:116: <field
name="name">purchase_blanket_agreement_line_tree</field>
_list

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:123: <field
name="name">purchase_blanket_agreement_line_tree_sequence</field>
_list_sequence

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:126: <record model="ir.ui.view"
id="product_view_list_agreement_line">
better to define it in product.xml

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:142: <field
name="name">create_purchase_line_tree</field>
_list

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:175: <field
name="model">purchase.blanket_agreement.line,-1</field>
I think the relate should be on agreement because relate on line is not easily
accessible.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.xml:181:
action="act_purchase_blanket_agreement"
define menuitem just after the action definition.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst
(right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:90:
...     'purchase.blanket_agreement.create_purchase_wizard',
[blanket_agreement])
80 cols

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:104:
>>> blanket_agreement.reload()
normally it is not needed as the wizard was run on it.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:109:
Create Purchase, link line to agreement and raise a warning
trailing space.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/scenario_purchase_blanket_agreement.rst:110:
because purchased quantity is more than agreement's remaining quantity::
I think you can run the wizard a second time and check quantity but increase it.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/tests/test_module.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/tests/test_module.py:8: 'Test Purchase
Blanket Agreement module'
double quote

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/tryton.cfg (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/tryton.cfg:13: 
empty line

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/create_purchase_form.xml (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/create_purchase_form.xml:5: <field
colspan="4" name="lines" mode="tree" readonly="1"/>
I think we can have form mode also and it can be not readonly

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/create_purchase_line_tree.xml
(right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/create_purchase_line_tree.xml:4: <tree
editable="1" >
I think we should display the agreement line because we could have multiple
lines for the same product.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml
(right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:26:
<field name="state"/>
I think it will be better to put the state outside the notebook so it is always
visible.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_form.xml:29:
<field name="currency"/>
currency should be before the lines.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml
(right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_line_form.xml:24:
<field name="description" colspan="4"/>
missing note (which should be another page).

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/view/purchase_blanket_agreement_list.xml
(right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_list.xml:4:
<tree>
missing optional company

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/view/purchase_blanket_agreement_list.xml:9:
<field name="to_date" expand="1" optional="0"/>
I do not think dates should be expanded.

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
File modules/purchase_secondary_unit/purchase.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
modules/purchase_secondary_unit/purchase.py:273: return self.quantity -
(processed_quantity or 0.)
better to assign: quantity = self.quantity…

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
modules/purchase_secondary_unit/purchase.py:311:
super().on_change_blanket_agreement_line()
always call super

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
File
modules/purchase_secondary_unit/tests/scenario_purchase_blanket_agreement.rst
(right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
modules/purchase_secondary_unit/tests/scenario_purchase_blanket_agreement.rst:1:
===================================
purchase_blanket_agreement must be added as test requires

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
modules/purchase_secondary_unit/tests/scenario_purchase_blanket_agreement.rst:98:
...     'purchase.blanket_agreement.create_purchase_wizard',
[blanket_agreement])
80 cols

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
modules/purchase_secondary_unit/tests/scenario_purchase_blanket_agreement.rst:125:
because purchased quantity is more than agreement's remaining quantity::
I do not think it is needed because it is not related to the second unit

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
File modules/purchase_secondary_unit/tests/test_module.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_secon...
modules/purchase_secondary_unit/tests/test_module.py:10: extras =
['account_invoice_secondary_unit', 'stock_secondary_unit',
break line at opening [

[ced, 2022-08-03 14:28:02]

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
File modules/purchase_blanket_agreement/purchase.py (right):

https://codereview.tryton.org/391151002/diff/435391003/modules/purchase_blank...
modules/purchase_blanket_agreement/purchase.py:777: unit_price =
fields.Numeric("Unit Price", digits=price_digits)
Use Monetary