[MIG][10.0] product_dimension #195
Conversation
gurneyalex
force-pushed
the
10.0-product_dimension
branch
2 times, most recently
from
ed082ab
to
c553c37
Compare
product_dimension/models/product.py
Outdated
| @@ -8,7 +8,7 @@ | |||
| from openerp import api | |||
gurneyalex
force-pushed
the
10.0-product_dimension
branch
from
c553c37
to
cb8d950
Compare
| <field name="model">product.template</field> | ||
| <field name="inherit_id" ref="product.product_template_only_form_view"/> | ||
| <field name="inherit_id" ref="stock.view_template_property_form"/> |
There was a problem hiding this comment.
Changing dependency for only a convenient view might be too much. Can't you place this in another place?
There was a problem hiding this comment.
product is not an app (in the odoo manifest sense), so IMO is not a big deal having modules depend on "real apps" (like sale, purchase, or stock). If you install only the product module, do you even have a menuitem to access them? I'm not sure!
There was a problem hiding this comment.
Yeah, I agree, but depending only on product allows you to decide which app you want to install. You can install sale without stock and need to manage dimensions in product, but with this, you are forced to have the stock module installed. The same if you decide to depend on sale and you only need purchase. So the best option is to rely only on a view in product module (and it's reasonable to do it I think).
|
👍 Good |
|
What is the status of this migration? |
|
@gurneyalex please note this module has a nasty little bug. Unfortunately underscore.js does not allow the use of the field name 'length' - see #210 I expect if we close and reopen this PR it will fail travis tests. |
gurneyalex
force-pushed
the
10.0-product_dimension
branch
from
cb8d950
to
1656b13
Compare
|
@gdgellatly rebased and pushed. |
|
@gurneyalex Guess travis wasn't updated as indicated. This bug drove me mental for literally years until by some fluke someone solved it for me. When you give a field the name length, and use it an embedded one2many list view, bad things happen. This is because underscore.js uses the length attribute of the 'object' and if the odoo record has an attribute called length it takes precedence. Guess its more an FYI than blocking seeing this module never embeds it anywhere. Certainly my modules now have lngth fields when talking about a products dimensions. |
|
@gdgellatly do you think it is worth a field rename ? I don't really understand the issue since other standard addons in odoo have a field called 'length' already (e.g. delivery). So surely it cannot be that bad? @pedrobaeza you said in #210 that pylint should flag this, yet the rebased PR is green. Should we worry about this? |
|
It's flagged, but as beta warning for now: https://travis-ci.org/OCA/product-attribute/jobs/233559765#L516 You should rename it for avoiding problems if you embed the view. Odoo core is not always the best place to look at for extensibility problems. |
|
@gurneyalex this was the test addon I used for reporting to Odoo https://github.com/gdgellatly/odoo_bug_demos/tree/master/testo2m . The symptoms are whenever an embedded one2many displays a field named length, no text displays, no records are updated, new lines are blank etc. But saving does update them. Now on one hand it is extremely unlikely this module would ever suffer from it as its more form view based, on the other if it ever happens because someone really needs to edit in an embedded o2m, noone knows why it doesn't work. Its related to a change made to form.js in v7 circa December 2015, strangely enough to solve another _.js problem related to many2many tags. |
| @@ -24,11 +23,7 @@ def onchange_calculate_volume(self): | |||
|
|
|||
| def convert_to_meters(self, measure, dimensional_uom): | |||
| uom_meters = self.env['product.uom'].search([('name', '=', 'm')]) | |||
There was a problem hiding this comment.
Why not use the XML ID ?
uom_meters = self.env.ref('product.product_uom_meter')
There was a problem hiding this comment.
I you use Odoo in an another language it doens't work
There was a problem hiding this comment.
@gurneyalex I created a PR with these two modification. Do what you want with this PR :p
There was a problem hiding this comment.
I think it is good to merge.
Yet, I think that both comments could be taken into account. Possibly in a next version, it could be added to the roadmap in the README if no-one has time to take into account now:
- use xml_id in convert_to_meters
- depend only on product
|
Do you think that we can merge (if @gurneyalex accept my PR) this PR rapidly ? |
|
There's still the problem on depending on stock. |
gurneyalex
force-pushed
the
10.0-product_dimension
branch
from
d855c59
to
9dbaa50
Compare
|
Hi everyone. regards |
|
Hi @gurneyalex , @pedrobaeza , Who is responsible of resolving such conflicts? |
|
@flotho Normally the author of the PR is the responsible for fixing his PR, but you can help. |
|
Superseeded by #491 |
No description provided.