search

OCA / vertical-travel

GNU Affero General Public License v3.0
33 stars 75 forks source link

Fix cases where Category name is translated and domain breaks #12

Closed bwrsandman closed 9 years ago

bwrsandman commented 9 years ago

In non-english interfaces the domain="[('category_id.name', '=', 'Weight')]" returns nothing since category_id.name == 'Poids'

There does not seem to be any way to refer to model datas within arch of a view, so product_uom_categ_kgm_ref was created to only point at the ID of product.product_uom_categ_kgm

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-1.83%) when pulling 5a31150fd6cbba7ec37dd1c67de60e317a49286e on savoirfairelinux:product_uom_categ into 552b843dcfc28878480cbf4159c5b941c89fdf6d on OCA:7.0.

pedrobaeza commented 9 years ago

Why don't you use ref('module.xml_id') to refer to an specific id?

bwrsandman commented 9 years ago

@pedrobaeza I tried that, it doesn't work inside of arch / inside of domains

pedrobaeza commented 9 years ago

No? It's strange, because I remembered to do something similar. Put the domain at field level then:

'baggage_weight': fields.float('Baggage Weight', help='Weight of baggage.', domain="[('category_id', '=', ref('product.product_uom_categ_kgm')]"),
bwrsandman commented 9 years ago

@pedrobaeza well that's in the .py code and you would need to define ref. It would only be run at initiation which may or may not have the model data declared already

It's an idea worth considering, thanks!

pedrobaeza commented 9 years ago

No, if you enclose it into ", the evaluation is done where is needed. It's a trick I am using a lot in OdooMRP to perform some advanced filters. See an example here: https://github.com/odoomrp/odoomrp-wip/blob/8.0/sale_product_variants/models/sale_order.py#L51

About ref, it that doesn't work (I'm not sure if you have got that function declared on view environment), then use self.ref.

bwrsandman commented 9 years ago

@pedrobaeza I will try it, let you know what I find.

bwrsandman commented 9 years ago

@pedrobaeza still have NameError: name 'ref' is not defined

This workaround is the only thing that works so far...

pedrobaeza commented 9 years ago

I can't find a way to do it also, so I must resign and admit the hack you're doing having no better solution. I have restart Travis builds, because I'm finding an strange error with related selection fields, to see if it's due to the new Odoo revision.

yvaucher commented 9 years ago

@bwrsandman I think in such case, you need to use %(xml_id)d in domain instead of ref():

domain="[('category_id', '=', %(product.product_uom_categ_kgm)d)]"/>
bwrsandman commented 9 years ago

Thanks for the correction @yvaucher

ehdem commented 9 years ago

:+1:

pedrobaeza commented 9 years ago

:+1:

yvaucher commented 9 years ago

@pedrobaeza @ehdem It was not fixed ...

pedrobaeza commented 9 years ago

No? Are you sure? I checked it and works for me

yvaucher commented 9 years ago

@pedrobaeza I'm talking about my previous comment. We don't need that extra field here.

https://github.com/OCA/vertical-travel/pull/12#issuecomment-71009572

pedrobaeza commented 9 years ago

I tried that also and got the same error. Have you tried?

bwrsandman commented 9 years ago

@pedrobaeza this wasn't properly reviewed and didn't follow the community standards

  1. Two community reviewer approves requires 5 calendar / 3 working day wait period after the reviews before a merge, this was merged nearly instantly after you approved.
  2. You're a community reviewer but @ehdem is not
  3. The one who merges should not be one who has voted, unless more than 3 reviewers have approved or the wait period has been lifted.

https://pad.odoo.com/p/community-review

pedrobaeza commented 9 years ago

@bwrsandman, please don't come with this periodically argued claim again.

Anyways, this doesn't break anything, so where's the problem? And it so, we can fast-track a revert.

With these kind of things, you are discouraging me a lot to review your work...

bwrsandman commented 9 years ago

The problem is that the PR did not need to be rushed and needed fixing. I didn't add the needs fixing tag and you missed @yvaucher's comment.

The reason why I stress these rules is because I want to be reviewed by not only by one person, but the rest of the community. This is why we need multiple community reviewers on a Pull Request so that the better practice can be implemented and real discussion can be had.

  1. I always saw it as a wait period after the second approve, otherwise it makes no sense: I could open a pull request for 2 weeks, then add a needs review tag and have it merged by someone the same day.
  2. It's nice to have external reviewers, but we have to keep in mind that they might miss a few things and in that case, there should be at least a minimum of community reviewers as well.
  3. There's a huge issue with this. The more eyes, the better. PRs being stagnant is a real issue, but immediately merging things is even worse. This is an example of such a case.

1 approve from the reviewer team and 1 external approve could have waited a little before merging.

As for the fixup, who is going to do it?

yvaucher commented 9 years ago

@bwrsandman Just did it @ #27

I might have needed to put this in need fixing it would have been more clear that we shouldn't merge this yet.