Hello Olivier,
could you please review my proposal again:
https://code.launchpad.net/~akretion-team/openobject-addons/addons-sale-extensible-action-ship-create/+merge/76609
Quick summary:
* still provides overridability on generated business objects
(procurement, move and picking) and BEFORE they are created in the database
* still provide a way to eventually partition the shipments over several
pickings, for that one should override
the _create_pickings_an_procurements method and filter the lines before
calling super. Again this is important for long term projects, drop
shipping, renting and other business flows.
* I have no more empty methods. 3 of the 4 new methods don't bring a lot
of business logic, but at least they are not empty, if you think it's
better I can agree with that.
* should provide enough freedom to customize the date_planned as
Numerigraphe wanted in his branch.
Also, please see my comments around the first "if order.state ==
'shipping_except':" I think this code doesn't work well. I just copied/pasted
it, so it shouldn't break anything, but as I commented in the code, I think it
doesn't work well and it's worth a little review by those who made it.
If you can merge that I can do the same for purchase pickings and a few other
places too.
Finally, I'll post more about this, but on Friday we have seen that store
fields computation methods tend to be called at every write or call. Because
of the API before that kind of refactoring, for some business logic or worst,
for some localizations like ours, we currently have no choice than re-browsing
and re-writing into the records. This trigger calls to store fields methods
and totally kill the performance (we had a case where creating 150 order lines
would take 30 minutes instead of 30 seconds because of unecessary calls to
those functions). So I insist, I think it's very important for the perceived
performance that the API give a chance to override before writing into the
database, so I hope you consider this merge seriously.
Regards.
--
Raphaël Valyi
Founder and consultant
http://twitter.com/rvalyi <http://twitter.com/#!/rvalyi>
+55 21 2516 2954
www.akretion.com <http://www.akretion.com/>
On Tue, Sep 27, 2011 at 7:35 AM, Raphael Valyi <rvalyi@xxxxxxxxx
<mailto:rvalyi@xxxxxxxxx>> wrote:
Hello Antony, my replies inline:
On Mon, Sep 26, 2011 at 8:06 PM, Antony Lesuisse <al@xxxxxxxxxxx
<mailto:al@xxxxxxxxxxx>> wrote:
>
> As usual the signal to noise ratio is low. So let me TLDR it: You want
to customize the business logic of openerp models more easily. OK.
I hope I'll prove you should read more carefully instead of being arrogant
with your community, read later.
>
>
> The mechanism we use in openerp is subclassing the model and overriding
methods (and we call super). Some existing methods should be split and we
will be happy to merge such kind refactoring.
>
> But, instead of that, what you propose is: not to refactor and to
introduce hooks everywhere and i really think this is insane. 'Towards a
cleaner a api' and linking your merge prop, to me, sounds like an oxymoron.
I hope I'll show you how wrong you are instead. And that's a pitty that
you seem so self assured and don't respect debating alternatives.
>
>
> example about the creation of a picking when needed:
>
> this is ok
>
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-sale-action-ship-create-al/+merge/77062
>
> this is not
>
https://code.launchpad.net/~akretion-team/openobject-addons/addons-sale-extensible-action-ship-create/+merge/76609
I'm sorry but I don't agree at all with your proposition, here is why:
So suppose we use your action_ship_create_picking in
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-sale-action-ship-create-al/+merge/77062
If I need to override the picking creation for a customization. A real
simple case fro he Brazilian localization: I need to propagate a m2o
category, say a "fiscal operation" to the picking.
How do I do that with your proposal?
So I override action_ship_create_picking. Then what? I see 2 options:
1) I call super to reuse your action_ship_create_picking:
then it sucks because your action_ship_create_picking already created the
record into the database! I will need to re-read it and re-write my field
into the database... If everybody does that as it is too often the case in
OpenERP addons, we end up doing 3x more requests than necessary to the
database, no wonder this is slow in the real life.
Not too say that may by I don't want to create a picking at all (in
base_project_costing I don't want) yet.
This may seems hacky but there are dozens of such real life situation
where OpenERP methods create a record we should delete then, very smart
database optimization indeed...
Let's take an other different concrete example to see where this reasoning
leads us: in Brazil delivery cost cannot be added as an order line, it
needs to be a global tax (eg deleted by overrider). The kind of code we
have in OpenERP is prety much like you suggest: it create the record first
and thinks later. I'm sorry to maintain need something smarter.
2) or I override action_ship_create_picking and I don't call super:
Suppose 2 modules do that: those modules are then incompatible unless you
create a third module to make the 3 compatibles (until you need one more
module), this is a very common situation with today's addons unfortunately.
Instead my solution suffer none of those pitfalls.
You said: "The mechanism we use in openerp is subclassing the model and
overriding methods (and we call super)."
Well read the merge, my solution follows this rule.
A bad non OOP hook would have been: value = self.get_value_from_hook(...)
And process value
This is not what I'm proposing, except for the key generation which I have
reasons for.
I'm sorry, but the only credible alternative that fulfills the
requirements (wise database usage and extensibility) is the proposal by
Olivier to create a 'prepare_picking' method that would prepare the data
in the memory but without hitting the database.
I hesitated between the two solutions and this is acceptable to me as
well. I'm not too sure if that's so much better, but I will try a new
merge proposal in that direction.
The reason I did what I did what I did instead of what Olivier proposed is
because I find it a reasonably "structuring" API. Instead, Olivier
suggestion is more of a woodstock/python solution where everyone get the
whole databag at the end of the method and can do what they want. To me It
means more freedom but also more risk of modules doing incompatible things
because of possible implicit and wrong assumptions they do over the
databag the receive from the overrider chain. Example: in my example I can
partition a picking over say different project phases. If I just get a
databag, chances are overriders wrongly assume implicitly there is only
one picking.
So as you see, this is a profound debate with several possibilities with
strucurant and non structurant API's. I totally respect Olivier's poposal,
but I'm sorry I fail to accept your arrogance associated to your flawed
solution.
Here you introduce one method and only fix 50% the issue with the picking.
Specifically I also introduce one method for the picking and at least I
meet both the extensibility and database optimization requirements. So who
is wrong?
More on this: you said "introduce hooks everywhere and i really think this
is insane"
Yes there is a method explosion and this is a mess, and when we will
modularize the code the apparent mess of the core will increase (through
overall mess will decrease and this is my point).
But who is the culprit?
IMHO the culprit is not those asking for a more fine grained API to meet
their real life requirements where the sale module isn't enough (probably
80% of the guys that really work as OpenERP professionals). The culprit is
the lack of abstraction in OpenERP. Methods would better belong to
specific reusable organized scopes like modules (not sure how you do that
in Python, but I do it a lot in Ruby and it's clean; look at OOOR code if
you want to see some clean code from me).
I'm sorry, but the class model of OpenERP seems to fall down to wrapping
collection of records into a browse object that get their
method overrided as modules are loaded. Even the sale orders and invoices
product_id_change methods are not able to mutualize any common logic/code.
Tell me this is smart OOP...
I'm sorry but much better can be achieved. This is not because that is
better than average ERP systems that this is good code.
Now this is easy to put the guilt on the community. But this is normal,
third parties are focused to integration and 300 of us give you money
instead to take care of the framework. We don't have commit rights and
cannot afford working weeks without getting merges, so yes, we do more
superficial things in the core and real work elsewhere. You OpenERP SA are
instead expected to drive the debate about the non superficial things. You
can accuse me of communicating me too much, but sorry, doing the things
alone in the dark and sending the resulting invoice to your partners at
the end for code that don't meet their need is certainly not what we
expect from an open source project either.
Yes, as I said, we can also in the future fix those things by elegant AOP
kind of code. BUT you don't propose a better alternative and you also
propose to do nothing. Well, what I say is: let's face it, the world is
not perfect. Today, the real code that is being effectively deployed (eg
not just server and addons) is a real mess yes. But we need a solution to
be better than that. If perfection was an option I would take it too. You
don' propose perfect thing either at all. I propose action and convergence
toward better things release after release.
That being said, I will investigate Olivier's proposal.
I fear you guys don't understand our requirement because unlike us you
don't spend your time integrating OpenERP in the real life for countries
other than Belgium or France. Please take a step back and see how
schizophrenical this is too spend your time making a nice batch API
orientation in the virgin world of the server while real life addons spend
their time rebrowsing the same record one by one over and over and
re-writting them after customization.
Regards.
>
>
>
>
> On 09/26/2011 05:14 PM, Raphael Valyi wrote:
>>
>> Dear framework experts,
>>
>> like many others real life OpenERP integrators, at Akretion our average
>> OpenERP is with around 15 extra addons overriding natural behavior to
bring
>> value added features or localization.
>> So it's extremely important the normal addons modules can be overrided
in a
>> decent way.
>> Otherwise, features are not competitive to create, modules are not
compatible
>> between them (it's very serious, in Brazil module creating a
>> purchase/sale/invoice order line or picking should be overriden with a
>> localization module at this stage, imagine how hard it is for the new
comers,
>> many non trivial localizations are in the same situation...).
>> An other consequence is that the overall SQL activity is inefficient, no
>> matter how perfect the server layer is going to be.
>>
>> Let's take a concrete example: the sale module and the action_ship_create
>> method that generate the shipments and the procurements.
>> Today it's a massive single bloc of code and customization can only happen
>> once the method has been executed (or before but then there is no
reuse and
>> compatibility at all). That means that we should re-search the created
items,
>> re-browse them. And depending how we change them later, other modules
doing
>> that are probably not compatible.
>>
>> *Instead, I propose to apply 2 general principles:*
>> *1) any method creating some important
>> business object (specially something localizable) should give a
chance to a
>> potential module overrider to customize it BEFORE hitting the database
with
>> the create method. That creation method should provide all the creation
>> context (like the order, the product line for a shipment creation) so the
>> overrider can take that into account.*
>> *
>> *
>> *2) when there is a what I would call a "cardinalitity mismatch", that is
>> stock move lines belong to less (often 1) picking, account move lines
belong
>> to 1 move... Then there should be a key lookup in a hash to know if the
>> container object can be reused or need a new instance instead. The
generation
>> of hat key should be overridable in modules.*
>>
>>
>> *I proposed a little piece of such work that is waiting for a merge,
please
>> comment on here:*
>>
https://code.launchpad.net/~akretion-team/openobject-addons/addons-sale-extensible-action-ship-create/+merge/76609
>>
>>
>> In the future, that behavior could even be abstracted away in some
declarative
>> and customization fashion. However, let me warn against that. I
suggest to do
>> that in a declarative style only if perfect, else it's much worse than
python
>> code. Take the server actions for instance or the methods attached to
workflow
>> nodes. It's declarative but very little used (python methods are instead)
>> because it was far from perfect).
>>
>> And again, *let me advocate for a working solution right now for 6.1*.
Change
>> can be manage over the time. This has never been done with OpenERP,
but things
>> can be deprecated 6 months in advance as other framework do. We need a
>> solution to increase OpenERP adoption. Taking a concrete example, today in
>> Brazil, considering that kind of catches less than 5 people are able
to put
>> OpenERP to work. This is not an ideal solution, neither for us,
neither for
>> OpenERP SA..
>> So I kindly request an evalution of this merge. If modules start using
that
>> modular API instead of hugly mono-bloc overrides, at least in the
future they
>> will have made a step toward a declarative refactoring, so this is not
a loss,
>> this is a win. I insist this refactoring is totally backward
compatible and
>> breaks no API, I was just warning here against the perfect being the
enemy of
>> the good that too often lead to inaction. The world is not perfect, third
>> parties contributions have limited resources and still we should converge
>> toward a better solution.
>>
>>
>> If this merge is accepted, then count on us to provide other such
little life
>> saver refactorings. This is no surprise that we have a divergence on the
>> relicense, well I propose to let this aside for now as we need a
decent 6.1
>> release for the benefit of everybody. You will later be able to claim that
>> those merges are under the dual license, at least now the dual license
is known.
>>
>>
>> Waiting for the merge evaluation.
>> (BTW we and many Brazilian partners and users are also waiting for a
merge of
>> l10n_br with correct chart of account more than 4 months now).
>
>
> _______________________________________________
> Mailing list: https://launchpad.net/~openerp-expert-framework
> Post to : openerp-expert-framework@xxxxxxxxxxxxxxxxxxx
<mailto:openerp-expert-framework@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe : https://launchpad.net/~openerp-expert-framework
> More help : https://help.launchpad.net/ListHelp
--
Raphaël Valyi
Founder and consultant
http://twitter.com/rvalyi
+55 21 2516 2954 <tel:%2B55%2021%202516%202954>
www.akretion.com <http://www.akretion.com>
_______________________________________________
Mailing list: https://launchpad.net/~openerp-expert-framework
Post to : openerp-expert-framework@xxxxxxxxxxxxxxxxxxx
Unsubscribe : https://launchpad.net/~openerp-expert-framework
More help : https://help.launchpad.net/ListHelp
