openerp-expert-framework team mailing list archive

[Raphael Valyi <rvalyi@xxxxxxxxx>]

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




On Tue, Sep 27, 2011 at 7:35 AM, Raphael Valyi <rvalyi@xxxxxxxxx> wrote:

> Hello Antony, my replies inline:
>
> On Mon, Sep 26, 2011 at 8:06 PM, Antony Lesuisse <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
> > 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
> www.akretion.com
>
>