Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration of sale_line_quantity_properties_based #146

Merged
merged 23 commits into from
Feb 17, 2017

Conversation

tafaRU
Copy link
Member

@tafaRU tafaRU commented May 5, 2015

This PR depends on #147 which also depends on #167 (both merged)

@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from 0769b16 to 20f31ca Compare June 19, 2015 09:49
}
try:
exec self.product_id.quantity_formula_id.\
formula_text in localdict
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use safe_eval (from openerp.tools.safe_eval) in exec mode. Not perfect, but limits the code injection risks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gurneyalex, done at tafaRU@bfe4905
Sorry to bother you but #167 needs to be merged asap so that I can unblock this one, #147 and #145

Thanks!

@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from 33724ef to c45977b Compare June 25, 2015 09:57
@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch 3 times, most recently from cb30149 to d8ca31c Compare November 16, 2015 11:39
@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from d8ca31c to 742e860 Compare November 17, 2015 13:50
@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from 742e860 to 2495847 Compare November 25, 2015 10:08
10 [pcs of] (4 m x 0.5 m) shelves = 20 m² of wood

In order to have this function working, it is necessary to have the user
proceeding as follows:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think s/proceeding/proceed/

(but I'm French...)

@tafaRU
Copy link
Member Author

tafaRU commented Feb 12, 2016

@gurneyalex, I addressed your last remarks and the PR now should be ready to be merged.
Please have a look at it.

@@ -35,6 +35,8 @@ env:
- TESTS="1" ODOO_REPO="OCA/OCB" INCLUDE="sale_sourced_by_line_sale_transport_multi_address" LINT_CHECK="0"
- TESTS="1" ODOO_REPO="odoo/odoo" INCLUDE="sale_properties_dynamic_fields" LINT_CHECK="0"
- TESTS="1" ODOO_REPO="OCA/OCB" INCLUDE="sale_properties_dynamic_fields" LINT_CHECK="0"
- TESTS="1" ODOO_REPO="odoo/odoo" INCLUDE="sale__line_quantity_properties_based" LINT_CHECK="0"
- TESTS="1" ODOO_REPO="OCA/OCB" INCLUDE="sale__line_quantity_properties_based" LINT_CHECK="0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many underscores

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think sale_properties_dynamic_fields and sale_line_quantity_properties_based can be tested together if possible

@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch 2 times, most recently from 9efef72 to 212a23e Compare February 18, 2016 08:15
@tafaRU
Copy link
Member Author

tafaRU commented Feb 18, 2016

@eLBati, done, thanks!

@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from 212a23e to 2f93741 Compare February 18, 2016 09:43
@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from 2f93741 to 411769c Compare February 26, 2016 14:09
@eLBati
Copy link
Member

eLBati commented Mar 2, 2016

@tafaRU please check tests

@eLBati
Copy link
Member

eLBati commented Mar 2, 2016

Maybe a rebase can help, after #266

@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch from 411769c to e9c50fa Compare March 3, 2016 08:12
@tafaRU
Copy link
Member Author

tafaRU commented Mar 3, 2016

@eLBati, just rebased, let's see.

@tafaRU
Copy link
Member Author

tafaRU commented Mar 3, 2016

@eLBati travis is still red, further investigation is needed then.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 73.17% when pulling 99aa782 on tafaRU:8.0-sale_line_quantity_properties_based-migr into e3251d6 on OCA:8.0.

@atchuthan
Copy link
Member

@tafaRU In .travis.yml, why do we still test with INCLUDE/EXCLUDE?
d6b210a

@pedrobaeza suggested on removing INCLUDE/EXCLUDE as it was designed for older testing versions in OCA and to adapt our travis for new version test.

@tafaRU
Copy link
Member Author

tafaRU commented Apr 14, 2016

@atchuthan, thanks for your help

Could you please give me some reference about this?

@pedrobaeza suggested on removing INCLUDE/EXCLUDE as it was designed for older testing versions in OCA and to adapt our travis for new version test.

tafaRU and others added 21 commits October 3, 2016 14:45
To do this I apply the following changes:
- depends from sale_properties_dynamic_fields instead of sale_properties_easy_creation
- onchange (v8) only depends by property_ids
- get empty properties and get empty properties dynamic fields in on_change (v7)
…me feedback, rather than silently doing nothing.
@tafaRU tafaRU force-pushed the 8.0-sale_line_quantity_properties_based-migr branch 2 times, most recently from 6f7e19f to dcab80a Compare October 3, 2016 14:23
@gurneyalex
Copy link
Member

👍

@pedrobaeza
Copy link
Member

You finally made it!

I will prefer as you know some commit squashing and module renaming, but after all the work, I approve this.

@eLBati eLBati merged commit 22c2d77 into OCA:8.0 Feb 17, 2017
lmignon pushed a commit to acsone/sale-workflow that referenced this pull request Feb 11, 2021
Signed-off-by simahawk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants