Merge lp:~renatonlima/openerp-fiscal-rules/fiscal-classification into lp:openerp-fiscal-rules

[Renato Lima - http://www.akretion.com]

Changes necessary to adapt views and objects in account_product_fical_classification module for OpenERP v7

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Why is it already merged without any approval?

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi renato,

Thanks for your contribs, but here I completely agree with Guewen, why is this MP merged without any approval.

@Raphaël : We try to put in place some community rules and I know you didn't had time to jump into yet, but please follow them as everyone.

Regards,

Joël

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello Joël and Geween,

well basically, we share Alexis de Lattre's opinion about this: basically, we think that the original module authors should be able to just commit, especially when porting on a new OpenERP version which is the case here. Indeed, there is no risk we break anybody stable installation here.

I'm all for code review when third parties are hacking some module that are claimed to be somewhat stable and thank you for that, but for prototyping new things or for porting to whole new release, we cannot always afford to wait for reviews...

That being said, if you have comments about the code, feel free to share them, we can always take them in consideration afterwards. Again, the thing is that we should be able to shape the code quickly, according to what our business model allows.

What do you think?

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Raphaël,

Well, the OpenERP community is actually taking responsability for the modules in these projects now and if module 'initiators' are allowed to commit unreviewed branches to these modules that can undermine this idea.

I must admit I had to get used to it myself but now I find it incredibly rich to have other people validating my changes even to 'my own' modules and sometimes getting valuable comments on them. So would you please reconsider?

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Hi Raphaël,

Stefan said very well what I was going to say. Just want to add more arguments.

We now have to think and act as a community.
In this role, we have to share the knowledge and responsibilities and
increase the overall quality of our work, as professionally as we are.

1. Community. If authors are allowed to bypass the reviews, I would see the
reviews as a means to 'protect' our modules rather than to increase their
quality and to share knowledge.
Think about this, partners would end up by reviewing only their 'own' modules (if they do).
A lot of my (and other involved reviewers) time is invested on those
reviews. If it doesn't end up with good results and with the born
of a strong community, if the quality of the modules is hazardous, I think I can stop immediately.
It's all or nothing in that concern.
That would be exactly the same situation than before the organization of the community, except
that 'extra-addons' would be split in many branches.
I would not call that a community, but a... share of branches.

2. Authoring. The more the community will be active and efficient, the less
the notion of author will be clear.

3. Fair. Not every partners have the chance to have a community reviewer in
their team.

And yes, you'll need to change your habits, as much as us and the others
ones which have started this adventure. But I'm intimately convinced that if
everybody play by the rules, it will be worth the pain.

By the way, nobody want to push prototypes in production, isn't it?

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Dear Guewen and Stefan,

really I like the community review system and I promise we will submit ourselves to it the more possible. And as you can remember we are the one who spent an absurd amount of time to build the branch extraction scripts that just made the modularization possible...
Now really I think that there should have some exceptions and if you think about it, you'll see even you aren't doing any different.

So first of all Guewen, I don't want to sound harsh, but are you doing any different when you claim to rewrite the Magentoerpconnect project in v7? I'm already working on refactoring the lower level dependencies of it too... Where are these nice code review on your early prototypes? How is that different from the refactor we do in our localization?

Now about the fiscal_classification module: it happens it's a module we built for the Brazilian localization but with the hope it could be useful to other countries and this is the reason it's not built in our localization branch. But to day we are unaware of any thord party using it unfortunately... Still we promise we will do demo videos to explain the module better to get more chances to increase its adoption.

Now our business reality is that while OpenERP SA is selling partnerships to competitors leeching at our code and project image without never contributing a single line of code, just Renato Lima and me are carrying around over 15 000 lines/ 21 modules of localization alone. As you can see: it's a very tough reality that you at CampToCamp would be far from assuming when you want to raise 150k euros just to refactor the Magento connector for v7... So I mean, almost nobody is doing what we do in the conditions we do it and unfortunately it means we have to do it the way it can work for us. And the way it can work for us isn't having to wait for people who never used that code to review every little commit we do in the version; unfortunately.

So depending on your company size, location, market, maturity, realities are VERY different. You could think that only CampToCamp reality is valid. Well I would say that NONE of the larger/more comfortable companies did a tenth of what we did for our localization (or even for the Magento connector until quite recently BTW), so I think this is a clear sign that every business reality is valid and not adapting to these micro scales realities would be a terrible loss for the OpenERP eco-system.

By the way, that code as already been deployed in production in a 400 employees companies last month, so it isn't too experimental either (now the former version was utterly borken for such usage).

So again, I think module authors should be allow to opt'in for review or not if they are refactoring for new OpenERP versions. If that is not possible, well, what would happen is that we would simply move out our module from that reviewed branch (that we had started BTW) and used an unreviewed branch as the main branch. I don't think such an extremity is a good idea at all.

Instead what I think is that it's better to maximize code review but without being fanatic about it. That is: everytime you touch a third party module, you submit yourself to the re...

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Dear Raphaël,

I cannot understand your comments here, I'm really sorry. I cannot admit you're speaking about us this way without saying a word. We're talking about the rules that community agree to follow, not quality of your code nor the fact that you're the author, either not that you're code is stable or not. Don't mixed up everything please.

First of all, we're not talking about Akretion's investment in the OpenERP's world, in term of contributions, effort, time, and more. We both know how much we invest here ! That's why we, Camptocamp and Akretion, work hands-in-hands on various topics, with friendship, peace and at the end a win-win situation.

Then, we know you're making all the effort on Brazilian localization, we know that's hard work. Now, do not talk about what you don't know. Do not says "it's a very tough reality that you at Campcocamp would be far from assuming when you want to raise 150k euros just to refactor the Magento connector for v7...". We also assume the devs of thousands lines of code before you even had a look on OpenERP ! All this work was done without one $ from anybody else but Camptocamp. Most of those devs are now part of the core now-days, everybody benefit from it and that's the fantastic Open Source world.

Regarding the connector devs, could I remind you that we already make workshops, talks, specifications and all the rest, sharing everything with Sebastien Beau from your team months before publishing something !? As far as I remember, we now share specs and how we though to build the new generation with the whole community. We already plan to make a code sprint with you all. How the hell can you just ask Guewen where is his code !? Do you really think we won't have the community to review it ? It doesn't seems to be in our habits if you just look at the merge proposal review list.... Without asking a penny, Camptocamp make most of the reviews , and from far !

Finally, regarding the funding on the connector, we though it'll be fare to ask others to contribute on it as it's not always at Akretion, Camptocamp or some core contributors to fund everything. We already, as you've done, put a lot of money here. It seems fair to ask for some other beneficiary to invest a bit. And your team agree on that (or at least Sebastien did).

I don't want to sound harsh, but do not push the button to far away here, this is something I do not like at all. I can understand you're a bit angry regarding Stefan and Guewen answer on you merge. But that's the rules that everybody agree to respect.

You don't want those constraints (that I more see like a benefit), well, put your module in your own branch as you mentioned. No big deal for us if you feel more comfortable with that. But in the community branch, rules must be followed without exceptions. If you do need a quick review, just write to the reviewer mailing, I'm sure somebody will review it in the same day. Moreover, most of the review are made in 2-3 days, we're not at OpenERP ;) and it'll be even faster if you jump in :) ! If you want to continue the debate or suggest other rules please, post your suggestions on the reviewer mailing list (openerp-community-reviewer@l...

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Dear Joel,

I know very well about CampToCamp efforts from the beginning and never questioned that. The thing is you'are now up for a level of bureaucracy we cannot afford from where we are, just not for everything.

Yes you started at a rough time, may be as rough as it is for us to start in Brazil or even harder. Now, your work methodologies certainly adapted to the changes in the situation that is different now than what it was at the cowboy time. But hey, we are still at the cowboy time here ;-) and my goal wasn't to rant about it, but instead to point how the market is being badly addressed making all of us loosing precious time.
In the same line, when we will have a more mature localization and more air to breath, our goal is certainly to conform to better coding methodology standards (that we certainly know from our past lifes in jobs that pay the bill).

About theses modules being community based or not and about the rules standing over them. I should say I was a bit busy after the creation of the branches extraction scripts so couldn't express myself further. But basically I agree with what Alexis de Lattre said about it, which is more or less what I stated again. Or in other words, no I don't agree that things should always get through the bureaucracy of reviews, specially for things that are still in prototype forms like these modules.

I think the eco-system is very large. There is no point in wanting that some people would be able to review everything of it and specially I think expecting it would be counter-productive for everybody. Of course anyone is free to give his opinion about anybody's code and of course we try to submit ourselves to reviews, but I don't think being systematic about this scales.

You seem to see the world a bit binary: review or not review at all. Well if so we may move again these modules under other groups. Notice that while we created these branch it's not me who putted them under these core-accounting group, so saying I agreed with all the rules is a bit fast...

Really my intention is not to polemic, but I have a different vision: I think some projects like this one have leaders despite receiving appreciated third parties contributions. But in these cases, I think these third parties contributions should submit themselves to review, but not necessarily impose the reverse or we kill productivity. I mean imagine, caricaturing a bit, it would be like asking the kernel developers to review every commit of every program that run on Linux, just because it runs on Linux. Of course it's a caricature, but I think the reason it wouldn't fundamentally scale are the same.

Again, I really consider reviews as good practices, and think they could be mandatory for the stable branches. I just don't think it's productive to make them mandatory in such situations.

If you want groups where reviews are mandatory. We could even do it, but in that case, I think we would need several groups and review only inside a group and not a single golden group. But again, not even sure it would be productive for such a case given nobody else is using or maintaining that code.

About the connector: sorry but I think the situation is...

[Nhomar - Vauxoo (nhomar)]

Hello Raph.

IMHO, rule is rule, you follow the rule or died try it.

It means,

2 options here:

Option A:

1.- Build your own copy of the community branch -
2.- Propose the merge.
3.- Read with open mind.
4.- Mantain Updated your specific local branch until the work is merged.
5.- Fix corrections.
....
6.- It will be merged.
7.- Correct the :parent: in your local branch.

Option B:

1.- Not propose the module for a community branch.
2.- Use it and mantain it.
3.- When you think it will pass and you have the time, make the MP.
4.- Everybody happy.

Raph, imagine for one second, you must hire 20 Q&A, and pay for the job to ensure your customer will have the BEST software in the world, how much it will cost in terms of money to your customer??

Dude!, your customer must pay the job, or you pay for it, but here the discusion is not about Who pay for it or not, and Who win money or not, and Who Find a profitable or not profitable way to do them own Job or not.

Please, exactly in the same way you feel when somebody else that take your job and makes money without giving you back nothing, we feel when somebody else because <whatever reason here> wants to bypass Simple Rules.

I promise i will post a blogpost giving a way about how you must manage opensource developments in OpenERP Community ecosystems to explain by myself better, ok my conclusion,

Respect the rules, >>>1st community's alert to you,
<joke> You are not Chuck Norris, Openerp's community is Chuck Norris </joke> :-)

http://college.oxy.edu/admissionblog/files/2011/10/Thumbs-UP.jpg

review: Abstain

[Nhomar - Vauxoo (nhomar)]

About the code:

Just making:

pep8 . in the folder downloaded:

./account_fiscal_position_rule_sale/__openerp__.py:28:80: E501 line too long (87 > 79 characters)
./account_fiscal_position_rule_sale/__openerp__.py:32:80: E501 line too long (141 > 79 characters)
./account_fiscal_position_rule_sale/__openerp__.py:37:17: E122 continuation line missing indentation or outdented
./account_fiscal_position_rule_sale/sale.py:48:24: E121 continuation line indentation is not a multiple of four
./account_fiscal_position_rule_sale/sale.py:53:24: E123 closing bracket does not match indentation of opening bracket's line
./account_fiscal_position_rule_sale/sale.py:70:24: E121 continuation line indentation is not a multiple of four
./account_fiscal_position_rule_sale/sale.py:75:24: E123 closing bracket does not match indentation of opening bracket's line
./account_fiscal_position_rule_sale/sale.py:93:24: E121 continuation line indentation is not a multiple of four
./account_fiscal_position_rule_sale/sale.py:98:24: E123 closing bracket does not match indentation of opening bracket's line
./account_product_fiscal_classification/__openerp__.py:34:9: E126 continuation line over-indented for hanging indent
./account_product_fiscal_classification/__openerp__.py:35:9: E122 continuation line missing indentation or outdented
./account_product_fiscal_classification/__openerp__.py:24:11: E203 whitespace before ':'
./account_product_fiscal_classification/__openerp__.py:25:14: E203 whitespace before ':'
./account_product_fiscal_classification/__openerp__.py:27:13: E203 whitespace before ':'
./account_product_fiscal_classification/account_product_fiscal_classification.py:25:1: E302 expected 2 blank lines, found 1
./account_product_fiscal_classification/account_product_fiscal_classification.py:29:17: E126 continuation line over-indented for hanging indent
./account_product_fiscal_classification/account_product_fiscal_classification.py:32:18: E121 continuation line indentation is not a multiple of four
./account_product_fiscal_classification/account_product_fiscal_classification.py:33:18: E121 continuation line indentation is not a multiple of four
./account_product_fiscal_classification/account_product_fiscal_classification.py:44:1: W293 blank line contains whitespace
./account_product_fiscal_classification/account_product_fiscal_classification.py:48:23: E701 multiple statements on one line (colon)
./account_product_fiscal_classification/account_product_fiscal_classification.py:57:80: E501 line too long (126 > 79 characters)
./account_product_fiscal_classification/account_product_fiscal_classification.py:71:80: E501 line too long (86 > 79 characters)
./account_product_fiscal_classification/account_product_fiscal_classification.py:72:21: E128 continuation line under-indented for visual indent
./account_product_fiscal_classification/account_product_fiscal_classification.py:73:1: W293 blank line contains whitespace
./account_product_fiscal_classification/account_product_fiscal_classification.py:75:80: E501 line too long (136 > 79 characters)
./account_product_fiscal_classification/account_product_fiscal_classification.py:76:80: E501 line too long (153 > 79 characters)
./accoun...

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

+1 for Nhomar point of view... Rules are rules. They are not mine, but community ones. If you do not agree with them, ask to change them on the mailling. Since there, you follow them or make your own branch.

OpenERP-Magento connector that we're building will be publish AND REVIEW as soon as we'll have something that make sense.

Cheers,

Joël

--

Joël Grand-Guillaume
Division Manager
Business Solutions

Camptocamp SA
PSE A, CH-1015 Lausanne
http://openerp.camptocamp.com/

Phone: [phone]
Office: [phone]
Fax: [phone]
Email: <email address hidden>

[Stefan Rijnhart (Opener) (stefan-opener)]

Would it be a solution if the community reviewers retreated from this project and maybe a (hopefully) small number of other projects that are in this stage of development? We could put up a note on the main page of the other projects declaring that they adhere to the community review policy published in the OpenERP documentation. At least in that case the responsability would be clear.

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

@Nhomar with all the respect, if you want to play the PEP8 hero, you would do a more productive job running them against the core of OpenERP... we are absolutely aware of the PEP8 issues in that branch, but again we had other priorities in the past than building PEP8 compliant code over a sandy OpenERP basis and a muddy business model. That being said, if you look at the evolution of our code, you'll see that the code we migrate is being improved a lot regarding to PEP8. Now less than half of the modules in that branch have been migrated to v7 so again, priorities are certainly others.

@Stefan, I think you are hitting the spot! This branch is actually a good example: this is a group of modules we at least in Brazil tend to use together because our localization depends on all of them (though as I said they may not be localization specific and that's why we moved them out).

Now, among these modules of that branch, some modules have been used by contributing third parties in other contexts such as CampToCamp, SavoirFaireLinux in Canada and the French part of Akretion, in order to address taxes in multi-countries situations (with e-commerce mostly), which is the same problematic as managing taxes in federations of countries with fiscal sovereignty such as what we have in Brazil.
It means, that we should ideally be careful with the modules used by these good willing third parties, while we mostly need to be agile to address our localization needs.

Does it mean we should split these modules into more branches and face an even harder chaos of possibly incompatible branches when it comes down to installing our localization? To give you a better picture, I would say that even with theses grouped branches, around half of the companies being hired as official "partners" in Brazil are unable to install/update the localization. I mean, going more branching chaos is certainly on the table, but at some point it sucks because the guys supposed to represent us, would then have expectations even farther from the hard reality and that certainly ends up...

So what do we do? To sum up we see that with these branches we are largely solving the "modularization" problem the monolithic extra-addons had. Now, Joel and others want to also fix the "quality" part of the community module. I would say that it's two different issues and the quality part is even harder to address as long as we cannot have one module=one branch + a decent way of installing and testing the whole against the explosion of possible combinations (a problem largely fixed in the software industry minus in OpenERP who always try to re-invent the wheel, now with the apps that is in clear business conflict with these quality requirements).

At some point, what I tried to defend is a flexible system where some modules in stables branches could get stronger commit rules, while these rules could be relaxed in some other cases like this one. We can very well say "the rule is the rule and that's it". But my fear is that if we follow that, we will fragment the ecosystem of modules in much more unmanageable branches for the mortals (or make the community unattractive in other words) and have only a very...

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Dear Raphaël,

Why saying that having a review on all community project/branches will be less flexible and/or lead to more chaotic branches !?

Before claiming that, will you just please make a MP and wait for the review once ? I mean, you don't even "play" the game once before saying this won't work for tons of reasons... I perfectly understand your fear my friends, trust me. I know you don't want to rely on others in some situations... I had the same at the beginning. But that's part of the game to build strong module and community. If you really want to be hell quick once, well make an exceptions on your side by using your own branch till the MP is approved...

As MP are reviewed within 1-3 days currently I think it's not blocking anybody in his work. We never really slow down each other this way... If you join the review, we'll be even more quick. As I said, it's not like with OpenERP MP review here, you don't wait months !

What I suggest is that you try working with the rules in place, if it doesn't fit your needs, then well, launch the debate on the community reviewer mailling list. It seems fair to me, what do you think ?

Have a nice week-end,

Joël

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

P.S. After a quick check, all reviews are made in less than a day on community branch currently. Just look in the MP mailing.. I mean, it's really working well and fast, give the current rules a try (and a chance :) !

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

@Joël,

all right, let's say we will try it this way in the future and if it proves to be too bureaucratic, move to another group.

By the way, even if we didn't pass through the official merge proposal ceremony, Renato and me peered on that (yes at 4 AM during a go live last month on the v6.1 branch; that's the reason we cannot afford more bureaucracy) so it's not like uncertain commits in the wild. That's isn't fundamentally different than 2 C2C developers approving each others...

As for the reasons I didn't play the review game yet, it's certainly boils down to the fact that already investing on the branch extraction scripts was a huge investment for a light structure like us and as I said, migrating 15 000 lines of maturing locatization code to version 7.0 with only two people made us busy enough for not having the luxury to touch too much non localization related code these last months. I would happily peer with others for our localization. The issue is almost nobody here is making such serious investment on the localization, so we end up peering with ourselves so far. This situation is certainly related to the way OpenERP SA addresses this market that's why no matter how important the code is I never miss an occasion to give my opinion about the business model part of the project. At least we manage to peer for a few lower level modules we share with other localizations such as the Spanish one.

Also as we respect the modules used in productions by others, you'll notice that we didn't make the offence of committing these things wildly into the 6.1 "stable" branch but rather maintained our own branch here instead https://code.launchpad.net/~akretion-team/openerp-fiscal-rules/6.1-legacy for our customer pool we maintain under that 6.1 transitional branch.

Have a nice week end too and thanks again for the community work.

[Niels Huylebroeck (red15)]

I'd like to point out that the community process is working well for
bugfixing and minor improvements but development of a new module is
something that is probably moving too fast and creating large merges.

So I would suggest new module development is something you would keep on a
different branch, then when it's ready for deployment you can just copy (or
maybe using the extraction tools) the module into a community branch
(commits of development of new a new module is rarely interesting in bzr
history I think).

When the module is merged as a whole this of course will also create a
large merge, I would like to suggest that for easier reviewing and testing
by community that the unittests would be a good place to document features
and explain the workings of the module. Unittests are a good thing to have
anyway we can all agree ?

Considering budget constraints (we all have them) it's possible you didn't
create a unittest for a module that is relatively complex, the only
suggestion I would make is to at least write down in a simple textual
unittest which just has a @unittest2.skip("Needs code implementation")
decorator to show it's not yet implemented.

2013/1/18 Raphaël Valyi - http://www.akretion.com <email address hidden>

> @Joël,
>
> all right, let's say we will try it this way in the future and if it
> proves to be too bureaucratic, move to another group.
>
> By the way, even if we didn't pass through the official merge proposal
> ceremony, Renato and me peered on that (yes at 4 AM during a go live last
> month on the v6.1 branch; that's the reason we cannot afford more
> bureaucracy) so it's not like uncertain commits in the wild. That's isn't
> fundamentally different than 2 C2C developers approving each others...
>
> As for the reasons I didn't play the review game yet, it's certainly boils
> down to the fact that already investing on the branch extraction scripts
> was a huge investment for a light structure like us and as I said,
> migrating 15 000 lines of maturing locatization code to version 7.0 with
> only two people made us busy enough for not having the luxury to touch too
> much non localization related code these last months. I would happily peer
> with others for our localization. The issue is almost nobody here is making
> such serious investment on the localization, so we end up peering with
> ourselves so far. This situation is certainly related to the way OpenERP SA
> addresses this market that's why no matter how important the code is I
> never miss an occasion to give my opinion about the business model part of
> the project. At least we manage to peer for a few lower level modules we
> share with other localizations such as the Spanish one.
>
> Also as we respect the modules used in productions by others, you'll
> notice that we didn't make the offence of committing these things wildly
> into the 6.1 "stable" branch but rather maintained our own branch here
> instead
> https://code.launchpad.net/~akretion-team/openerp-fiscal-rules/6.1-legacyfor our customer pool we maintain under that 6.1 transitional branch.
>
> Have a nice week end too and thanks again for the community work.
> --
>
> https://code.launchpad.net/~ren...

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

Can we please move this discussion to the openerp-community mailing list? Or wherever you like but not here, which is hardly appropriate.

Thanks.

=== modified file 'account_product_fiscal_classification/account_product_fiscal_classification.py'
--- account_product_fiscal_classification/account_product_fiscal_classification.py	2012-10-29 13:41:30 +0000
+++ account_product_fiscal_classification/account_product_fiscal_classification.py	2013-01-15 20:01:26 +0000
@@ -23,174 +23,193 @@
 from osv import fields, osv
 
 class account_product_fiscal_classification(osv.osv):
-
     _name = 'account.product.fiscal.classification'
     _description = 'Product Fiscal Classification'
     _columns = {
                 'name': fields.char('Main code', size=32, required=True),
                 'description': fields.char('Description', size=64),
                 'company_id': fields.many2one('res.company', 'Company'),
-                 #TODO restrict to company_id if company_id set when framework will allow it:
-                'sale_base_tax_ids': fields.many2many('account.tax',
-                                                      'fiscal_classification_sale_tax_rel',
-                                                      'fiscal_classification_id', 'tax_id',
-                                                      'Base Sale Taxes',
-                                                      domain=[('type_tax_use','!=','purchase')]),
-                'purchase_base_tax_ids': fields.many2many('account.tax',
-                                                          'fiscal_classification_purchase_tax_rel',
-                                                          'fiscal_classification_id',
-                                                          'tax_id', 'Base Purchase Taxes',
-                                                          domain=[('type_tax_use','!=','sale')]),
-                }
-
+                 # TODO restrict to company_id if company_id set when
+                 # framework will allow it:
+                'sale_base_tax_ids': fields.many2many(
+                    'account.tax', 'fiscal_classification_sale_tax_rel',
+                    'fiscal_classification_id', 'tax_id', 'Base Sale Taxes',
+                    domain=[('type_tax_use', '!=', 'purchase')]),
+                'purchase_base_tax_ids': fields.many2many(
+                    'account.tax', 'fiscal_classification_purchase_tax_rel',
+                    'fiscal_classification_id', 'tax_id',
+                    'Base Purchase Taxes',
+                    domain=[('type_tax_use', '!=', 'sale')])
+    }
+    
     def button_update_products(self, cr, uid, ids, context=None):
 
         result = True
-        if not context:
-            context = {}
+        if not context: context = {}
 
-        obj_product = self.pool.get('product.product')
+        obj_product = self.pool.get('product.template')
         obj_company = self.pool.get('res.company')
 
         for fiscal_classification in self.browse(cr, uid, ids):
-            current_company_id = self.pool.get('res.users').browse(cr, uid, uid).company_id.id
-
-            product_ids = obj_product.search(cr, uid, [])
-            product_read = obj_product.read(cr, uid, product_ids, ['property_fiscal_classification'])
-            product_ids_new = [x['id'] for x in product_read if x['property_fiscal_classification'] and x['property_fiscal_classification'][0] == fiscal_classification.id]
-
-            for product in obj_product.browse(cr, uid, product_ids_new, context):
-                to_keep_sale_tax_ids = self.pool.get('account.tax').search(cr, uid, [('id', 'in', [x.id for x in product.taxes_id]), ('company_id', '!=', current_company_id)])
-                to_keep_purchase_tax_ids = self.pool.get('account.tax').search(cr, uid, [('id', 'in',[x.id for x in product.supplier_taxes_id]), ('company_id', '!=', current_company_id)])
-
+            property_ids = self.pool.get('ir.property').search(
+                cr, uid, [('name', '=', 'property_fiscal_classification'),
+                          ('res_id', 'ilike', 'product.template,'),
+                          ('value_reference', '=', 'account.product.fiscal.classification,%s' % (fiscal_classification.id,))])
+
+            reads = self.pool.get('ir.property').read(
+                cr, uid, property_ids, ['res_id'])
+            product_ids = [int(l['res_id'].split(',')[1]) for l in reads]
+            current_company_id = self.pool.get('res.users').browse(
+                cr, uid, uid).company_id.id
+
+            for product in obj_product.browse(cr, uid, product_ids, context):
+                to_keep_sale_tax_ids = self.pool.get('account.tax').search(
+                    cr, uid, [('id', 'in', [x.id for x in product.taxes_id]),
+                              ('company_id', '!=', current_company_id)])
+
+                to_keep_purchase_tax_ids = self.pool.get('account.tax').search(
+                    cr, uid, [('id', 'in', [x.id for x in product.supplier_taxes_id]),
+                    ('company_id', '!=', current_company_id)])
+            
                 vals = {
-                        'taxes_id': [(6, 0, to_keep_sale_tax_ids + [x.id for x in fiscal_classification.sale_base_tax_ids])],
-                        'supplier_taxes_id': [(6, 0, to_keep_purchase_tax_ids + [x.id for x in fiscal_classification.purchase_base_tax_ids])],
+                        'taxes_id': [(6, 0, list(set(to_keep_sale_tax_ids + [x.id for x in fiscal_classification.sale_base_tax_ids])))],
+                        'supplier_taxes_id': [(6, 0, list(set(to_keep_purchase_tax_ids + [x.id for x in fiscal_classification.purchase_base_tax_ids])))],
                         }
-
+            
                 obj_product.write(cr, uid, product.id, vals, context)
-
+        
         return result
-
-    def name_search(self, cr, user, name='', args=None, operator='ilike', context=None, limit=80):
-
-        if not args:
-            args = []
-
-        if not context:
-            context = {}
+    
+    def name_search(self, cr, user, name='', args=None, operator='ilike',
+                    context=None, limit=80):
+
+        if not args: args = []
+
+        if not context: context = {}
 
         if name:
-            ids = self.search(cr, user, [('name', '=', name)] + args, limit=limit, context=context)
-            if not len(ids):
-                ids = self.search(cr, user, [('description', '=', name)] + args, limit=limit, context=context)
-            if not len(ids):
-                ids = self.search(cr, user, [('name', operator, name)] + args, limit=limit, context=context)
-                ids += self.search(cr, user, [('description', operator, name)] + args, limit=limit, context=context)
+            ids = self.search(cr, user, [('name', '=', name)] + args,
+                              limit=limit, context=context)
+            if not len(ids):
+                ids = self.search(
+                    cr, user, [('description', '=', name)] + args,
+                    limit=limit, context=context)
+            if not len(ids):
+                ids = self.search(cr, user, [('name', operator, name)] + args,
+                                  limit=limit, context=context)
+                ids += self.search(
+                    cr, user, [('description', operator, name)] + args,
+                    limit=limit, context=context)
         else:
             ids = self.search(cr, user, args, limit=limit, context=context)
-
+        
         return self.name_get(cr, user, ids, context)
 
 account_product_fiscal_classification()
 
+
 class account_product_fiscal_classification_template(osv.osv):
-
     _name = 'account.product.fiscal.classification.template'
     _description = 'Product Fiscal Classification Template'
     _columns = {
                 'name': fields.char('Main code', size=32, required=True),
                 'description': fields.char('Description', size=64),
-                #TODO restrict to company_id if company_id set when framework will allow it:
-                'sale_base_tax_ids': fields.many2many('account.tax.template',
-                                                      'fiscal_classification_template_sale_tax_rel',
-                                                      'fiscal_classification_id', 'tax_id',
-                                                      'Base Sale Taxes',
-                                                      domain=[('type_tax_use','!=','purchase')]),
-                'purchase_base_tax_ids': fields.many2many('account.tax.template',
-                                                          'fiscal_classification_template_purchase_tax_rel',
-                                                          'fiscal_classification_id',
-                                                          'tax_id', 'Base Purchase Taxes',
-                                                          domain=[('type_tax_use','!=','sale')]),
-                }
-
-    def name_search(self, cr, user, name='', args=None, operator='ilike', context=None, limit=80):
-
-        if not args:
-            args = []
-
-        if not context:
-            context = {}
+                # TODO restrict to company_id if company_id set when 
+                # framework will allow it:
+                'sale_base_tax_ids': fields.many2many(
+                    'account.tax.template',
+                    'fiscal_classification_template_sale_tax_rel',
+                    'fiscal_classification_id', 'tax_id',
+                    'Base Sale Taxes',
+                    domain=[('type_tax_use', '!=', 'purchase')]),
+                'purchase_base_tax_ids': fields.many2many(
+                    'account.tax.template',
+                    'fiscal_classification_template_purchase_tax_rel',
+                    'fiscal_classification_id',
+                    'tax_id', 'Base Purchase Taxes',
+                    domain=[('type_tax_use', '!=', 'sale')]),
+    }
+
+    def name_search(self, cr, user, name='', args=None, operator='ilike',
+                    context=None, limit=80):
+
+        if not args: args = []
+
+        if not context: context = {}
 
         if name:
-            ids = self.search(cr, user, [('name', '=', name)] + args, limit=limit, context=context)
-            if not len(ids):
-                ids = self.search(cr, user, [('description', '=', name)] + args, limit=limit, context=context)
-            if not len(ids):
-                ids = self.search(cr, user, [('name', operator, name)] + args, limit=limit, context=context)
-                ids += self.search(cr, user, [('description', operator, name)] + args, limit=limit, context=context)
+            ids = self.search(cr, user, [('name', '=', name)] + args,
+                              limit=limit, context=context)
+            if not len(ids):
+                ids = self.search(
+                    cr, user, [('description', '=', name)] + args,
+                    limit=limit, context=context)
+            if not len(ids):
+                ids = self.search(cr, user, [('name', operator, name)] + args,
+                                  limit=limit, context=context)
+                ids += self.search(
+                    cr, user, [('description', operator, name)] + args,
+                    limit=limit, context=context)
         else:
             ids = self.search(cr, user, args, limit=limit, context=context)
-
+        
         return self.name_get(cr, user, ids, context)
 
 account_product_fiscal_classification_template()
 
+
 class wizard_account_product_fiscal_classification(osv.osv_memory):
-
     _name='wizard.account.product.fiscal.classification'
-
     _columns = {
-                'company_id':fields.many2one('res.company','Company', required=True),
-                }
-
+        'company_id': fields.many2one('res.company','Company', required=True),
+    }
     _defaults = {
-                 'company_id': lambda self, cr, uid, c: self.pool.get('res.users').browse(cr,uid,[uid],c)[0].company_id.id,
-                }
+        'company_id': lambda self, cr, uid, c: self.pool.get('res.users').browse(cr,uid,[uid],c)[0].company_id.id,
+    }
 
     def action_create(self, cr, uid, ids, context=None):
-
         obj_wizard = self.browse(cr,uid,ids[0])
         obj_tax = self.pool.get('account.tax')
         obj_tax_template = self.pool.get('account.tax.template')
-        obj_fiscal_classification = self.pool.get('account.product.fiscal.classification')
-        obj_fiscal_classification_template = self.pool.get('account.product.fiscal.classification.template')
+        obj_fc = self.pool.get('account.product.fiscal.classification')
+        obj_fc_template = self.pool.get('account.product.fiscal.classification.template')
 
         company_id = obj_wizard.company_id.id
         tax_template_ref = {}
-
+        
         tax_ids = obj_tax.search(cr,uid,[('company_id','=',company_id)])
-
+        
         for tax in obj_tax.browse(cr,uid,tax_ids):
             tax_template = obj_tax_template.search(cr,uid,[('name','=',tax.name)])[0]
-
+            
             if tax_template:
                 tax_template_ref[tax_template] = tax.id
-
-        fclass_ids_template = obj_fiscal_classification_template.search(cr, uid, [])
-
-        for fclass_template in obj_fiscal_classification_template.browse(cr, uid, fclass_ids_template):
-
-            fclass_id = obj_fiscal_classification.search(cr, uid, [('name','=',fclass_template.name)])
-
-            if not fclass_id:
+        
+        fclass_ids_template = obj_fc_template.search(cr, uid, [])
+        
+        for fclass_template in obj_fc_template.browse(cr, uid, fclass_ids_template):
+            
+            fclass_id = obj_fc.search(cr, uid, [('name','=',fclass_template.name)])
+            
+            if not fclass_id: 
                 sale_tax_ids = []
                 for tax in fclass_template.sale_base_tax_ids:
                     sale_tax_ids.append(tax_template_ref[tax.id])
-
+                
                 purchase_tax_ids = []
                 for tax in fclass_template.purchase_base_tax_ids:
                     purchase_tax_ids.append(tax_template_ref[tax.id])
-
+                
                 vals = {
-                        'name': fclass_template.name,
-                        'description': fclass_template.description,
-                        'company_id': company_id,
-                        'sale_base_tax_ids': [(6,0, sale_tax_ids)],
-                        'purchase_base_tax_ids': [(6,0, purchase_tax_ids)]
-                        }
-                obj_fiscal_classification.create(cr,uid,vals)
-
+                    'name': fclass_template.name,
+                    'description': fclass_template.description,
+                    'company_id': company_id,
+                    'sale_base_tax_ids': [(6, 0, sale_tax_ids)],
+                    'purchase_base_tax_ids': [(6, 0, purchase_tax_ids)]
+                }
+                obj_fc.create(cr,uid,vals)
+            
         return {}
 
 wizard_account_product_fiscal_classification()
+

=== modified file 'account_product_fiscal_classification/account_product_fiscal_classification_view.xml'
--- account_product_fiscal_classification/account_product_fiscal_classification_view.xml	2012-12-05 19:41:36 +0000
+++ account_product_fiscal_classification/account_product_fiscal_classification_view.xml	2013-01-15 20:01:26 +0000
@@ -5,7 +5,6 @@
         <record model="ir.ui.view" id="fiscal_classification_normal_form_view_form">
             <field name="name">fiscal_classification_normal_form_view_form</field>
             <field name="model">account.product.fiscal.classification</field>
-            <field name="type">form</field>
             <field name="arch" type="xml">
                 <form string="Fiscal Classification">
                         <field name="name"/>
@@ -24,12 +23,11 @@
         <record model="ir.ui.view" id="fiscal_classification_normal_form_view_tree">
             <field name="name">fiscal_classification_normal_form_view_tree</field>
             <field name="model">account.product.fiscal.classification</field>
-            <field name="type">tree</field>
             <field name="arch" type="xml">
                 <tree string="Fiscal Classification">
-                    <field select="1" name="name"/>
+                    <field name="name"/>
                     <field name="description"/>
-                    <field select="1" name="company_id"/>
+                    <field name="company_id"/>
                 </tree>
             </field>
         </record>
@@ -37,7 +35,6 @@
         <record model="ir.ui.view" id="fiscal_classification_template_normal_form_view_form">
             <field name="name">fiscal_classification_template_normal_form_view_form</field>
             <field name="model">account.product.fiscal.classification.template</field>
-            <field name="type">form</field>
             <field name="arch" type="xml">
                 <form string="Fiscal Classification Template">
                         <field name="name"/>
@@ -54,10 +51,9 @@
         <record model="ir.ui.view" id="fiscal_classification_template_normal_form_view_tree">
             <field name="name">fiscal_classification_normal_form_view_tree</field>
             <field name="model">account.product.fiscal.classification.template</field>
-            <field name="type">tree</field>
             <field name="arch" type="xml">
-                <tree string="Fiscal Classification Templates">
-                    <field select="1" name="name"/>
+                <tree string="Fiscal Classification Template">
+                    <field name="name"/>
                     <field name="description"/>
                 </tree>
             </field>
@@ -68,7 +64,6 @@
         <record id="view_wizard_account_product_fiscal_classification" model="ir.ui.view">
             <field name="name">Generate Product Fiscal Classification from Templates</field>
             <field name="model">wizard.account.product.fiscal.classification</field>
-            <field name="type">form</field>
             <field name="arch" type="xml">
                 <form string="Generate Product Fiscal Classification from Templates">
                     <separator col="4" colspan="4" string="Generate Product Fiscal Classification from Templates"/>

=== modified file 'account_product_fiscal_classification/product.py'
--- account_product_fiscal_classification/product.py	2012-10-29 13:41:30 +0000
+++ account_product_fiscal_classification/product.py	2013-01-15 20:01:26 +0000
@@ -22,9 +22,8 @@
 
 from osv import fields, osv
 
-class product_template(osv.osv):
+class product_template(osv.Model):
     _inherit = 'product.template'
-
     _columns = {
                 'property_fiscal_classification': fields.property(
                     'account.product.fiscal.classification',
@@ -34,7 +33,7 @@
                     method=True,
                     view_load=True,
                     help="Company wise (eg localizable) Fiscal Classification"),
-                }
+    }
 
     def fiscal_classification_id_change(self, cr, uid, ids, fiscal_classification_id=False, sale_tax_ids=[[6, 0, []]], purchase_tax_ids=[[6, 0, []]]):
         """We eventually keep the sale and purchase taxes because those are not company wise in OpenERP. So if we choose a different fiscal position
@@ -47,13 +46,14 @@
             to_keep_sale_tax_ids = self.pool.get('account.tax').search(cr, uid, [('id', 'in', sale_tax_ids[0][2]), ('company_id', '!=', current_company_id)])
             to_keep_purchase_tax_ids = self.pool.get('account.tax').search(cr, uid, [('id', 'in', purchase_tax_ids[0][2]), ('company_id', '!=', current_company_id)])
 
-            result['value']['taxes_id'] = to_keep_sale_tax_ids + [x.id for x in fiscal_classification.sale_base_tax_ids]
-            result['value']['supplier_taxes_id'] = to_keep_purchase_tax_ids + [x.id for x in fiscal_classification.purchase_base_tax_ids]
+            result['value']['taxes_id'] = list(set(to_keep_sale_tax_ids + [x.id for x in fiscal_classification.sale_base_tax_ids]))
+            result['value']['supplier_taxes_id'] = list(set(to_keep_purchase_tax_ids + [x.id for x in fiscal_classification.purchase_base_tax_ids]))
         return result
 
 product_template()
 
-class product_product(osv.osv):
+
+class product_product(osv.Model):
     _inherit = "product.product"
 
     def fiscal_classification_id_change(self, cr, uid, ids, fiscal_classification_id=False, sale_tax_ids=[[6, 0, []]], purchase_tax_ids=[[6, 0, []]]):
@@ -64,5 +64,3 @@
         return result
 
 product_product()
-
-# vim:expandtab:smartindent:tabstop=4:softtabstop=4:shiftwidth=4:

=== modified file 'account_product_fiscal_classification/product_view.xml'
--- account_product_fiscal_classification/product_view.xml	2012-12-07 04:37:44 +0000
+++ account_product_fiscal_classification/product_view.xml	2013-01-15 20:01:26 +0000
@@ -8,7 +8,7 @@
             <field name="inherit_id" ref="account.product_normal_form_view"/>
             <field name="arch" type="xml">
                 <field name="property_account_expense" position="after">
-                    <separator string="Fiscal Properties" colspan="4"/>
+                	<separator string="Fiscal Properties" colspan="4"/>
                     <field name="property_fiscal_classification" colspan="4" attrs="{'required': [('type', '!=', 'service')]}" select="2" on_change="fiscal_classification_id_change(property_fiscal_classification, taxes_id, supplier_taxes_id)"/>
                 </field>
             </field>