Merge lp://staging/~therp-nl/openerp-product-attributes/7.0_lp1272282_fixed_price into lp://staging/~product-core-editors/openerp-product-attributes/7.0

Thanks for your work Ronald.

The idea to apply a 100% discount to get a fixed price seems correct to me. I would request a functional review to Lorenzo to check if that covers the use case for the old module.

The 100% discount trick is what we currently do in a non-automatic sort of way, and it looks like this module is just what I expected to develop some day to make it easier.
So we're quite willing to test this, but please allow a few days before we can give a feedback.

From a functional point of view, I approve the -1 trick.

Functional: Worked for me, no problem.
I only wish it was possible to mix fixed price rules and standard ones, so we could get rules like
- product A = 1€
- product B = 3€
- other products = sale price + 10%
But this is something that we can change later, it should not block this MP.

GUI:
The form layout for the fixed price list version is slightly wrong (the number of cols must be wrong I guess).
Would you mind making the list view of pricelist lines editable? That would make it much faster to enter.

Code:
I suggest you merge the content of "model/" in a single file at the upper level directory to make consistent with the other modules.

There are spaces at the end of lots of lines, would you care to remove them please?

Can you please make the docstring for the check_*() functions more explicit that "Raise exception when error found", like maybe "Ensure the fixed prices are in fixed pricelists"?

The docstring states that "_compute_vals()" mutates the argument "vals", so I'd be more comfortable if it was called "_adapt_vals()" or something likely explicit.

In _compute_vals you don't use the context, so please remove "context = context or {}"

In write(),the context is missing from the browse() call.

The python files lack copyright comments, is that OK?

Without demeriting your work, IMHO a better approach would be to include a field calle type_extended or something similar on product.pricelist.item model that includes the other rule types and a new one called "Fixed price". On the on_change of this field, you can change original type and discount value to -1 if "Fixed price" is selected. On interface, you only need to hide corresponding fields when this type is selected, and relace the old field with this new one. I think it's simpler, don't you?

Regards.

I tried to take most remarks into account. I refer to the commit messages.

Two things
- I prefer having a separate file for each model created or modified. One of the big annoyances in working with the code in OpenERP in my experience is how everything is lumped together in monster .py files. grep is our friend ofcourse. But why for example is res.partner modified somewhere after 1800 (!) lines of code in account_invoice.py? The java community has had the convention of "one class per file" for ages, and that is working out perfectly.
- I did not add an editable list view for the moment. This is just for lack of time. Might add it in the future.

Hi all,

my review was just a pointer for Lorenzo because I wanted to see his opinion on this implementation as opposed to his own. I now abstain because I didn't actually review the code.

Thanks to all for your work.

Hi, Ronald, let me then work in my solution and I'll propose it here. Until them, I'll put this on 'Needs fixing' until then.

Regards.

@Pedro I do not quite understand why you put 'need fixing'. As far as I understand, the module now does everything you and others asked for. OK, I make fields read-only instead of completely hiding them. But what else would you like to be different?

Dear Ronald Portier,
Thanks for your work. I tested them and functionally they look just fine.
The UI could be simpler: you could make the fields "hidden" instead of "read-only" when a pricelist lines have a fixed price.
Then the additional menu entries and views wouldn't be needed, you could remove them altogether.
Regarding the many files, usually we group them by module so you could do with a "product.py" with all things product-related. But I'd not block this MP for such a detail.
And other that that it looks just fine to me.

Pedro Manuel Baeza, I humbly think we should accept this MP, and then you can base your own branch on Ronald Portier's work. What do you think?

Pedro Manuel Baeza,
I think I didn't understand your counter-proposal at first, sorry.
What you're proposing may be more powerful but will be more intrusive won't it? Then it might be harder to test. Do you expect to develop advanced price list customizations beyond fixed price in the short term?
Otherwise I think we should accept this and eventually replace it with more complex code like the one you propose, when the actual need arises.

Hi, I have make another MP proposing my approach for this task. Please review it and tell me if you see it more idoneous:

https://code.launchpad.net/~pedro.baeza/openerp-product-attributes/7.0-product_pricelist_fixed_price/+merge/205045

Regards.

I'll disapprove it as https://code.launchpad.net/~pedro.baeza/openerp-product-attributes/7.0-product_pricelist_fixed_price/+merge/205045 superseeds it.

Ronald would be great if you could have a look at Pedro's MP as well.

Hi Ronald,

The alternative [0] proposed by Pedro is about to be merged; it has been approved by 3 people. Can we have your opinion on that, and eventually can you close your one if you think that Pedro's proposal fit with your needs as well?

Thanks.

[0] https://code.launchpad.net/~pedro.baeza/openerp-product-attributes/7.0-product_pricelist_fixed_price/+merge/205045

Ronald having approved Pedro's proposal, I set the merge status of this proposal as rejected. Thanks to everyone for your work.