Sales - Workflow and Organization

Merge lp://staging/~camptocamp/sale-wkfl/7.0-add-sale-default-discount-lep into lp://staging/~sale-core-editors/sale-wkfl/7.0

7.0-add-sale-default-discount-lep
Merge into 7.0

Proposed by Leonardo Pistone on 2014-07-17

Status:	Needs review
Proposed branch:	lp://staging/~camptocamp/sale-wkfl/7.0-add-sale-default-discount-lep
Merge into:	lp://staging/~sale-core-editors/sale-wkfl/7.0
Diff against target:	335 lines (+287/-0) 9 files modified sale_default_discount/__init__.py (+22/-0) sale_default_discount/__openerp__.py (+56/-0) sale_default_discount/model/__init__.py (+23/-0) sale_default_discount/model/partner.py (+34/-0) sale_default_discount/model/sale.py (+47/-0) sale_default_discount/test/no_default_discount.yml (+21/-0) sale_default_discount/test/partner_default_discount.yml (+30/-0) sale_default_discount/view/partner.xml (+20/-0) sale_default_discount/view/sale.xml (+34/-0)
To merge this branch:	bzr merge lp://staging/~camptocamp/sale-wkfl/7.0-add-sale-default-discount-lep
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Pedro Manuel Baeza		2014-07-17	Needs Resubmitting on 2014-08-08
Review via email: mp+227214@code.staging.launchpad.net

Revision history for this message

Pedro Manuel Baeza (pedro.baeza) wrote on 2014-07-17:

Hi, Leonardo,

Thanks for the MP.

I'm seeing the code, and I feel more comfortable having the field called as default_sale_discount, or at least sale_discount, not only discount, because partners can be also suppliers.

About the functionality itself, I'm not seeing in the code the part that applies this default value to sale order lines. Where it is?

Regards.

review: Needs Information (code review)

lp://staging/~camptocamp/sale-wkfl/7.0-add-sale-default-discount-lep updated on 2014-07-18

50. By Leonardo Pistone on 2014-07-18

rename field to sale_default_discount

As suggested by Pedro

51. By Leonardo Pistone on 2014-07-18

add tests to sale_default_discount

As discussed in the yaml file itself, I hit a situation where manual testing
works, but a default value does not show in the order line when in a yaml
test.

I prefer not to write extra production code just to make already working code
pass yaml tests. If YAML code are meant to be full-stack integrated tests (I
got the impression they are, since for example they run onchange methods),
then this looks like a bug in the yaml processor.

Revision history for this message

Leonardo Pistone (lepistone) wrote on 2014-07-18:

Pedro,

I renamed the field as you suggested.

The default in the line works because of the attribute "context" in sale.xml. Thanks to Romain Deheele for showing me that.

I also added two tests, and got a bit frustrated when I realized that the YAML test does not get the same behaviour of manual testing (all is OK when manual testing). More details in the commit message and the yaml file.

A broader discussion we could continue later on the community list is: what are yaml tests supposed to test?

Thanks

Revision history for this message

Yann Papouin (yann-papouin) wrote on 2014-07-18:

Just for the record, the almost same result can be achieved with a product.pricelist by checking the visible_discount field and setting this pricelist as the sale one in the partner form.

Revision history for this message

Leonardo Pistone (lepistone) wrote on 2014-07-18:

Yann,

you are right. Still, this module came out from a situation where pricelists were already used for other things, and a simple default discount was useful anyway.

Revision history for this message

Pedro Manuel Baeza (pedro.baeza) wrote on 2014-08-08:

This project is now hosted on https://github.com/OCA/sale-workflow. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

review: Needs Resubmitting

Revision history for this message

Leonardo Pistone (lepistone) wrote on 2014-11-26:

migrated to https://github.com/OCA/sale-workflow/pull/74

Unmerged revisions

51. By Leonardo Pistone on 2014-07-18

add tests to sale_default_discount

As discussed in the yaml file itself, I hit a situation where manual testing
works, but a default value does not show in the order line when in a yaml
test.

50. By Leonardo Pistone on 2014-07-18