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. ... 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. >> 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. > > 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. > > 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. You are lucky I discovered this problem before we released. This is a change that happened during this development cycle, and *must* be fixed before our next release.
