New module sale_rebate for 2.2

[yangoon, 2011-07-17 22:55:57]

http://codereview.tryton.org/66001/diff/1/INSTALL
File INSTALL (right):

http://codereview.tryton.org/66001/diff/1/INSTALL#newcode7
INSTALL:7: * Python 2.5 or later (http://www.python.org/)
2.6 for trunk

http://codereview.tryton.org/66001/diff/1/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/1/sale.py#newcode64
sale.py:64: result[line.id] = self._compute_list_price(line.product,
line.sale.currency,
80 cols

http://codereview.tryton.org/66001/diff/1/sale.py#newcode74
sale.py:74: unit_price = (1 - (rebate / 100) ) * list_price
handling of spaces different from following line:
100) ) -> 100))

http://codereview.tryton.org/66001/diff/1/sale.py#newcode75
sale.py:75: unit_price.quantize(Decimal(str(10**-self.unit_price.digits[1])))
unit_price.quantize(Decimal(str(10** - self.unit_price.digits[1])))

http://codereview.tryton.org/66001/diff/1/sale.py#newcode86
sale.py:86: return value.quantize(Decimal(str(10**-self.rebate.digits[1])))
dto

http://codereview.tryton.org/66001/diff/1/sale.py#newcode91
sale.py:91: result[line.id] = self._compute_rebate(line.list_price,
line.unit_price)
80 cols

http://codereview.tryton.org/66001/diff/1/setup.py
File setup.py (right):

http://codereview.tryton.org/66001/diff/1/setup.py#newcode36
setup.py:36: 'trytond.modules.sale': info.get('xml', []) \
trytond.modules.sale_rebate

http://codereview.tryton.org/66001/diff/1/setup.py#newcode48
setup.py:48: 'Programming Language :: Python :: 2.5',
trunk?

[ced, 2011-07-18 08:08:00]

http://codereview.tryton.org/66001/diff/1/INSTALL
File INSTALL (right):

http://codereview.tryton.org/66001/diff/1/INSTALL#newcode7
INSTALL:7: * Python 2.5 or later (http://www.python.org/)
On 2011/07/17 22:55:57, yangoon wrote:
> 2.6 for trunk

It is for 2.0

http://codereview.tryton.org/66001/diff/1/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/1/sale.py#newcode64
sale.py:64: result[line.id] = self._compute_list_price(line.product,
line.sale.currency,
On 2011/07/17 22:55:57, yangoon wrote:
> 80 cols

Done.

http://codereview.tryton.org/66001/diff/1/sale.py#newcode74
sale.py:74: unit_price = (1 - (rebate / 100) ) * list_price
On 2011/07/17 22:55:57, yangoon wrote:
> handling of spaces different from following line:
> 100) ) -> 100))

Done.

http://codereview.tryton.org/66001/diff/1/sale.py#newcode75
sale.py:75: unit_price.quantize(Decimal(str(10**-self.unit_price.digits[1])))
On 2011/07/17 22:55:57, yangoon wrote:
> unit_price.quantize(Decimal(str(10** - self.unit_price.digits[1])))

- is not an operator here. We do like that in many other places.

http://codereview.tryton.org/66001/diff/1/sale.py#newcode91
sale.py:91: result[line.id] = self._compute_rebate(line.list_price,
line.unit_price)
On 2011/07/17 22:55:57, yangoon wrote:
> 80 cols

Done.

http://codereview.tryton.org/66001/diff/1/setup.py
File setup.py (right):

http://codereview.tryton.org/66001/diff/1/setup.py#newcode36
setup.py:36: 'trytond.modules.sale': info.get('xml', []) \
On 2011/07/17 22:55:57, yangoon wrote:
> trytond.modules.sale_rebate

Done.

[yangoon, 2011-07-19 13:53:57]

http://codereview.tryton.org/66001/diff/17/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/17/sale.py#newcode74
sale.py:74: unit_price = (1 - (rebate / 100)) * list_price
I still don't understand why you are using here
(1 - (rebate / 100))
instead of 
(1-(rebate/100))
like in the following line. Or the other way round: why not using spaces in the
line below?
Even if the expression in the following line is a string it is meant to be
interpreted as code.

[ced, 2011-07-19 14:21:50]

http://codereview.tryton.org/66001/diff/17/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/17/sale.py#newcode74
sale.py:74: unit_price = (1 - (rebate / 100)) * list_price
On 2011/07/19 13:53:57, yangoon wrote:
> I still don't understand why you are using here
> (1 - (rebate / 100))
> instead of 
> (1-(rebate/100))
> like in the following line. Or the other way round: why not using spaces in
the
> line below?
> Even if the expression in the following line is a string it is meant to be
> interpreted as code.

Because the other one is just the symbol for a number.

[plantian, 2011-07-21 18:23:38]

Suggestion for doc.

http://codereview.tryton.org/66001/diff/17/doc/index.rst
File doc/index.rst (right):

http://codereview.tryton.org/66001/diff/17/doc/index.rst#newcode9
doc/index.rst:9: - *Rebate*: The rebate computed between *List Price* and *Unit
Price*
This documentation seems redundant with the source.  What about documenting
intent?  Such as:

The sale_rebate module has two purposes:
1. Convey the percentage that the *Unit Price* is of the *List Price* to the
customer on each *Sale Line* via the *Rebate* field.
2. Add the ability to recompute the *Unit Price* as a percentage reduction of
the *List Price*.  This can be be done by entering a new percentage in the
*Rebate* field of a *Sale Line*.

[oscaralvarez, 2011-07-25 12:38:19]

Please review the comments for to fix something bugs, in sale.xml

http://codereview.tryton.org/66001/diff/17/sale.xml
File sale.xml (right):

http://codereview.tryton.org/66001/diff/17/sale.xml#newcode27
sale.xml:27: <field name="inherit" ref="sale.sale_view_form"/>
I think that this is wrong and to generate a bug: 

             ref="sale.sale_view_form" 

instead this work it: 

             ref="sale.sale_line_view_form"

http://codereview.tryton.org/66001/diff/17/sale.xml#newcode31
sale.xml:31: <xpath
expr="/form/notebook/page[@name='general']/label[@name='unit_price']"
I think that this is wrong and to generate a bug:
              
      page[@name='general'] 

instead this work it:
      page[@id='general']

[ced, 2011-07-25 13:43:36]

http://codereview.tryton.org/66001/diff/17/sale.xml
File sale.xml (right):

http://codereview.tryton.org/66001/diff/17/sale.xml#newcode27
sale.xml:27: <field name="inherit" ref="sale.sale_view_form"/>
On 2011/07/25 12:38:20, oscar.alvarez.montero wrote:
> I think that this is wrong and to generate a bug: 
> 
>              ref="sale.sale_view_form" 
> 
> instead this work it: 
> 
>              ref="sale.sale_line_view_form"

Done.

http://codereview.tryton.org/66001/diff/17/sale.xml#newcode31
sale.xml:31: <xpath
expr="/form/notebook/page[@name='general']/label[@name='unit_price']"
On 2011/07/25 12:38:20, oscar.alvarez.montero wrote:
> I think that this is wrong and to generate a bug:
>               
>       page[@name='general'] 
> 
> instead this work it:
>       page[@id='general']

Done.

[yangoon, 2011-11-27 11:19:08]

http://codereview.tryton.org/66001/diff/18002/INSTALL
File INSTALL (right):

http://codereview.tryton.org/66001/diff/18002/INSTALL#newcode7
INSTALL:7: * Python 2.5 or later (http://www.python.org/)
Python version is defined in setup.py

http://codereview.tryton.org/66001/diff/18002/MANIFEST.in
File MANIFEST.in (right):

http://codereview.tryton.org/66001/diff/18002/MANIFEST.in#newcode9
MANIFEST.in:9: include *.csv
po files in 2.2

http://codereview.tryton.org/66001/diff/18002/setup.py
File setup.py (right):

http://codereview.tryton.org/66001/diff/18002/setup.py#newcode41
setup.py:41: 'Environment :: Plugins',
Missing Framework classifier

http://codereview.tryton.org/66001/diff/18002/setup.py#newcode48
setup.py:48: 'Programming Language :: Python :: 2.5',
not in 2.2

[yangoon, 2011-11-27 18:22:07]

Two comments, otherwise LGTM

http://codereview.tryton.org/66001/diff/16006/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/16006/sale.py#newcode19
sale.py:19: 'get_list_price')
Hmm, this mode of indentation looks strange to me.

http://codereview.tryton.org/66001/diff/16006/sale.py#newcode21
sale.py:21: help='Rebate in percentage',
in percent or as percentage

[ced, 2011-11-27 19:17:49]

http://codereview.tryton.org/66001/diff/16006/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/16006/sale.py#newcode19
sale.py:19: 'get_list_price')
On 2011/11/27 18:22:08, yangoon wrote:
> Hmm, this mode of indentation looks strange to me.

1 open parenthesis so 4 spaces

http://codereview.tryton.org/66001/diff/16006/sale.py#newcode21
sale.py:21: help='Rebate in percentage',
On 2011/11/27 18:22:08, yangoon wrote:
> in percent or as percentage

It seems that "in percentage" is more used than "as percentage"

[ced, 2011-12-05 13:51:09]

http://codereview.tryton.org/66001/diff/16006/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/16006/sale.py#newcode14
sale.py:14: list_price = fields.Function(fields.Numeric('List Price',
digits=(16, 4),
I'm wondering if we should not store this value?

[udono2, 2011-12-27 17:30:56]

http://codereview.tryton.org/66001/diff/27018/INSTALL
File INSTALL (right):

http://codereview.tryton.org/66001/diff/27018/INSTALL#newcode15
INSTALL:15: Once you've downloaded and unpacked the trytond_sale_rebate source
release, enter the
<80cols

http://codereview.tryton.org/66001/diff/27018/doc/index.rst
File doc/index.rst (right):

http://codereview.tryton.org/66001/diff/27018/doc/index.rst#newcode9
doc/index.rst:9: - *Rebate*: The rebate computed between *List Price* and *Unit
Price*
*Unit Price*_._

http://codereview.tryton.org/66001/diff/27018/sale.xml
File sale.xml (right):

http://codereview.tryton.org/66001/diff/27018/sale.xml#newcode6
sale.xml:6: 
unneeded empty line

http://codereview.tryton.org/66001/diff/27018/sale.xml#newcode59
sale.xml:59: 
unneeded empty line

http://codereview.tryton.org/66001/diff/27018/setup.py
File setup.py (right):

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode17
setup.py:17: (dep, major_version, minor_version, major_version,
indent

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode20
setup.py:20: (major_version, minor_version, major_version, minor_version + 1))
indent

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode29
setup.py:29: info.get('version', '0.0.1').rsplit('.', 1)[0] + '/',
indent

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode37
setup.py:37: + info.get('translation', []),
indent

[ced, 2012-01-02 13:21:58]

http://codereview.tryton.org/66001/diff/27018/doc/index.rst
File doc/index.rst (right):

http://codereview.tryton.org/66001/diff/27018/doc/index.rst#newcode9
doc/index.rst:9: - *Rebate*: The rebate computed between *List Price* and *Unit
Price*
On 2011/12/27 17:30:56, udono wrote:
> *Unit Price*_._

Done.

http://codereview.tryton.org/66001/diff/27018/setup.py
File setup.py (right):

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode17
setup.py:17: (dep, major_version, minor_version, major_version,
On 2011/12/27 17:30:56, udono wrote:
> indent

Done.

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode20
setup.py:20: (major_version, minor_version, major_version, minor_version + 1))
On 2011/12/27 17:30:56, udono wrote:
> indent

Done.

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode29
setup.py:29: info.get('version', '0.0.1').rsplit('.', 1)[0] + '/',
On 2011/12/27 17:30:56, udono wrote:
> indent

Done.

http://codereview.tryton.org/66001/diff/27018/setup.py#newcode37
setup.py:37: + info.get('translation', []),
On 2011/12/27 17:30:56, udono wrote:
> indent

Done.

[yangoon, 2012-01-02 17:45:17]

http://codereview.tryton.org/66001/diff/27020/__tryton__.py
File __tryton__.py (right):

http://codereview.tryton.org/66001/diff/27020/__tryton__.py#newcode1
__tryton__.py:1: #This file is part of Tryton.  The COPYRIGHT file at the top
level of
Missing encoding # -*- coding: utf-8 -*-

[jc.michel, 2012-01-22 20:26:01]

Hi,

The rebate is rightly displayed now.
But I get this trace and error when validating the move (I invoice On shipment):
 File "/trytond/protocols/jsonrpc.py", line 108, in _marshaled_dispatch
    response['result'] = dispatch_method(method, params)
  File "/trytond/protocols/jsonrpc.py", line 147, in _dispatch
    res = dispatch(*args)
  File "/trytond/protocols/dispatcher.py", line 155, in dispatch
    res = getattr(obj, method)(*args_without_context, **kargs)
  File "/trytond/model/modelworkflow.py", line 113, in workflow_trigger_validate
    instance_obj.validate(instance, signal=signal)
  File "/trytond/workflow/workflow.py", line 222, in validate
    force_running=force_running)
  File "/trytond/workflow/workflow.py", line 334, in process
    res = self._split_test(workitem, activity.split_mode, signal)
  File "/trytond/workflow/workflow.py", line 467, in _split_test
    self._join_test(transition, instance)
  File "/trytond/workflow/workflow.py", line 479, in _join_test
    'state': 'active',
  File "/trytond/workflow/workflow.py", line 313, in create
    self.process(workitem)
  File "/trytond/workflow/workflow.py", line 330, in process
    if not self._execute(workitem, activity):
  File "/trytond/workflow/workflow.py", line 381, in _execute
    id_new = self._execute_action(workitem, activity)
  File "/trytond/workflow/workflow.py", line 417, in _execute_action
    return wkf_action(model_obj.browse(workitem.instance.res_id))
  File "/trytond/modules/stock/shipment.py", line 958, in wkf_done
    'effective_date': date_obj.today(),
  File "/trytond/modules/sale/sale.py", line 1672, in write
    sale_obj.workflow_trigger_validate(sale_ids, 'shipment_update')
  File "/trytond/model/modelworkflow.py", line 113, in workflow_trigger_validate
    instance_obj.validate(instance, signal=signal)
  File "/trytond/workflow/workflow.py", line 222, in validate
    force_running=force_running)
  File "/trytond/workflow/workflow.py", line 334, in process
    res = self._split_test(workitem, activity.split_mode, signal)
  File "/trytond/workflow/workflow.py", line 467, in _split_test
    self._join_test(transition, instance)
  File "/trytond/workflow/workflow.py", line 479, in _join_test
    'state': 'active',
  File "/trytond/workflow/workflow.py", line 313, in create
    self.process(workitem)
  File "/trytond/workflow/workflow.py", line 334, in process
    res = self._split_test(workitem, activity.split_mode, signal)
  File "/trytond/workflow/workflow.py", line 467, in _split_test
    self._join_test(transition, instance)
  File "/trytond/workflow/workflow.py", line 479, in _join_test
    'state': 'active',
  File "/trytond/workflow/workflow.py", line 313, in create
    self.process(workitem)
  File "/trytond/workflow/workflow.py", line 330, in process
    if not self._execute(workitem, activity):
  File "/trytond/workflow/workflow.py", line 381, in _execute
    id_new = self._execute_action(workitem, activity)
  File "/trytond/workflow/workflow.py", line 417, in _execute_action
    return wkf_action(model_obj.browse(workitem.instance.res_id))
  File "/trytond/modules/sale/sale.py", line 893, in wkf_invoice_shipment
    self.create_invoice(sale.id)
  File "/trytond/modules/sale_shipment_cost/sale.py", line 217, in
create_invoice
    invoice_id = super(Sale, self).create_invoice(sale_id)
  File "/trytond/modules/sale/sale.py", line 747, in create_invoice
    invoice_lines = self._get_invoice_line_sale_line(sale)
  File "/trytond/modules/sale_shipment_cost/sale.py", line 204, in
_get_invoice_line_sale_line
    result = super(Sale, self)._get_invoice_line_sale_line(sale)
  File "/trytond/modules/sale/sale.py", line 693, in _get_invoice_line_sale_line
    val = line_obj.get_invoice_line(line)
  File "/trytond/modules/sale_rebate/sale.py", line 137, in get_invoice_line
    result['list_price'] = line.list_price
TypeError: list indices must be integers, not str

[udono2, 2012-08-18 07:13:40]

The module looks good so far. I like the possibility to enter negative
percentages. When using a negative percentage it is no longer a
'rebate' but a 'surcharge'.

In this context I stuck a little with the not so generic naming
'rebate'.

I put some ideas about the naming inline.
IMHO at least the description should mention that 'surcharges' are
possible, too.

http://codereview.tryton.org/66001/diff/38004/__tryton__.py
File __tryton__.py (right):

http://codereview.tryton.org/66001/diff/38004/__tryton__.py#newcode5
__tryton__.py:5: 'name': 'Sale Rebate',
For the module name I prefer sale_rebate_surcharge.

http://codereview.tryton.org/66001/diff/38004/__tryton__.py#newcode10
__tryton__.py:10: 'description': '''Add rebate on sale line.''',
and negative rebate as surcharge.

http://codereview.tryton.org/66001/diff/38004/sale.py
File sale.py (right):

http://codereview.tryton.org/66001/diff/38004/sale.py#newcode20
sale.py:20: rebate = fields.Function(fields.Numeric('Rebate', digits=(16, 2),
string could be 'Rebate/Surcharge'

http://codereview.tryton.org/66001/diff/38004/sale.py#newcode21
sale.py:21: help='Rebate in percentage',
Surcharge in negative percentage.

http://codereview.tryton.org/66001/diff/38004/sale.xml
File sale.xml (right):

http://codereview.tryton.org/66001/diff/38004/sale.xml#newcode22
sale.xml:22: </record>
above record must be removed for trytond > 2.2