Hi Adam, On Mar 18, 2010, at 4:18 PM, Adam Heath wrote:
> Jacopo Cappellato wrote: >> Hi Adam, >> >> please see my comments inline: >> >> On Mar 17, 2010, at 9:22 PM, Adam Heath wrote: >> >>> Sorry, no. I don't even have to try to know this isn't correct. You >>> haven't done proper deprecation on the entity. >>> >> >> calm down, you don't even know what you are talking about. > > > http://cwiki.apache.org/confluence/display/OFBTECH/General+Entity+Overview > > Look at deprecated entities. > > ... > When changing an entity significantly, and especially when changing > the primary key, always deprecate the existing entity and create a new > one so that an upgrade path for existing databases is possible. To > facilitate this in addition to deprecating the old entity (prefixing > with "Old") and defining the new one you should always create a > service to move data from the old entity to the new one. The service > doesn't have to figure out when to run itself (which is tough to > figure out and generally not reliable), and generally should be made > to be run manually. > ... Pointing your finger at me and feeling strong because you have the "Holy Book Of Policies" in your hand? I am joking but as you know I know this best practice; in fact I have considered it carefully but then, as I explained in my last email, in this specific situation my judgment told me it was not necessary because the OrderShipment entity was only part of an incomplete process I implemented time before (and could not be used in production as is). I may be wrong, but not because of that generic best practice; we can (politely) discuss specific issues you are having and then I am sure we will find out the best way to go. > > OrderShipment has been used for ages, long before our last release. > And you didn't follow the proper procedure for changing the primary > key. The procedure is to rename the entity when the primary key > fields changed. Again, you don't know what you are talking about, see my comments below... > >>> You have to rename the entity, then set the table of the renamed old >>> entity to the correct table name, then create a completely brand new >>> entity, with whatever changes, then modify the rest of the code to use >>> the new entity. >> >> How would the above strategy prevented you to have to know/deal with the >> change? >> What I did is exactly inline with the above strategy (I will not call it a >> "policy", because it isn't a policy in the meaning that you pretend to give >> it). >> What you suggest above would have still obliged you to: >> 1) fix your custom code >> 2) run the upgrade script that I have provided > > Of course I have to change my code. That's not the point. Ok, at least we are on the same page on this. > >> >> These are exactly the same steps you have to run after the changes I did - >> no extra work is required because of my approach. >> >> Now, in my self-defense (your tone seems to imply that you have setup a >> trial on me, the rope is hanging ready for me), I decided to not use the >> deprecation pattern for two reasons: >> a) no code was using the OrderShipment entity, no code apart from some old >> code (that I wrote) related to manufacturing shipment plans: a process that >> was only partially implemented for manufacturing companies implementing a >> particular type of fulfillment based on order/manufacturing schedules; I >> doubt you or anyone else were using this (and for sure not in production >> because it was not functional complete until my recent changes) >> b) I have decided to keep the entity model cleaner: we can't limit the OFBiz >> grow being stuck on code that others could have built outside of OFBiz and >> kept proprietary; if third parties are doing this then it's fine, they will >> have to fix the code after the upgrade (I am sure they are prepared for >> doing this, even if you are not) > > I see tons of references to OrderShipment. > > For example, > applications/product/webapp/facility/WEB-INF/actions/shipment/EditShipmentPlan.groovy > mentioned OrderShipment, with lines that were modified on 2008-06-13, > revision 667640. This change was a conversion from bsh to groovy, so > this entity was even in use well before that. Yes, this is exactly what I was referring to: that code is used to define a "shipment plan" (see my previous email), I contributed it a lot of time ago. > > >> >> That said, if you follow my notes you will probably resolve most of the >> issues you are experiencing and most of all you will help me to test the >> migration script (in fact I could run it only in a couple of servers so it >> is still experimental). But we have to be collaborative, relaxed: pointing >> fingers, judging others' work and screaming is not the right way to >> interact... at least not the right way to interact with *me*, so please >> avoid doing this from now on, it really annoys me. > > It's not about *me* following procedure. I can do that. I can handle > entity changes. But I'm not the only one using ofbiz. I don't understand your response. > > You are lucky I discovered this problem before we released. Lucky? > This is a change that happened during this development cycle, and *must* be > fixed before our next release. > I am not going to take orders from you but I am very open at accepting suggestions and in fact I would like to get the opinion also from other committers, persons like David, Adrian, Anil, Scott etc... with much more experience than you on the OFBiz applications and on business processes in general. If they also think that we should deprecate the entity and replace it as you suggest I will not object and I promise that, before the next official release containing this code will be released, I will implement the proper code. Kind regards, Jacopo
