entity primary key change -> BAD: Re: svn commit: r895250 - in /ofbiz/trunk/applications: order/entitydef/ product/entitydef/ product/script/org/ofbiz/shipment/issuance/ product/script/org/ofbiz/shipment/receipt/ product/script/org/ofbiz/shipment/shipment/ product/servicedef/ product/webap...

[Adam Heath-2]

[hidden email] wrote:

> Author: jacopoc
> Date: Sat Jan  2 18:20:19 2010
> New Revision: 895250
>
> URL: http://svn.apache.org/viewvc?rev=895250&view=rev
> Log:
> Pretty important change in the way purchase shipments and purchase-order-to-shipment mapping are modeled:
> * OrderShipment entity (that was mostly unused) has been enhanced to properly associate order items (OrderItemShipGrpAssoc) to shipment items: this entity is now used in place of ItemIssuance (that was improperly used, in po, for the same purpose)
> * as a consequence of the above change, I have updated the purchase order receive screens and business logic triggered by them
>
> Modified:
>     ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>     ofbiz/trunk/applications/product/entitydef/entitymodel_shipment.xml
>     ofbiz/trunk/applications/product/script/org/ofbiz/shipment/issuance/IssuanceServices.xml
>     ofbiz/trunk/applications/product/script/org/ofbiz/shipment/receipt/ShipmentReceiptServices.xml
>     ofbiz/trunk/applications/product/script/org/ofbiz/shipment/shipment/ShipmentServices.xml
>     ofbiz/trunk/applications/product/servicedef/services_shipment.xml
>     ofbiz/trunk/applications/product/webapp/facility/WEB-INF/actions/inventory/ReceiveInventory.groovy
>     ofbiz/trunk/applications/product/webapp/facility/inventory/receiveInventory.ftl
>     ofbiz/trunk/applications/product/webapp/facility/shipment/ShipmentTabBar.ftl
>     ofbiz/trunk/applications/product/widget/facility/ShipmentForms.xml
>     ofbiz/trunk/applications/product/widget/facility/ShipmentScreens.xml
>
> Modified: ofbiz/trunk/applications/order/entitydef/entitymodel.xml
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/entitydef/entitymodel.xml?rev=895250&r1=895249&r2=895250&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/order/entitydef/entitymodel.xml (original)
> +++ ofbiz/trunk/applications/order/entitydef/entitymodel.xml Sat Jan  2 18:20:19 2010
> @@ -1131,11 +1131,13 @@
>              title="Order Shipment Entity">
>        <field name="orderId" type="id-ne"></field>
>        <field name="orderItemSeqId" type="id-ne"></field>
> +      <field name="shipGroupSeqId" type="id-ne"></field>
>        <field name="shipmentId" type="id-ne"></field>
>        <field name="shipmentItemSeqId" type="id-ne"></field>
>        <field name="quantity" type="fixed-point"></field>
>        <prim-key field="orderId"/>
>        <prim-key field="orderItemSeqId"/>
> +      <prim-key field="shipGroupSeqId"/>
>        <prim-key field="shipmentId"/>
>        <prim-key field="shipmentItemSeqId"/>
>        <relation type="one" fk-name="ORDER_SHPMT_OHDR" rel-entity-name="OrderHeader">
> @@ -1152,6 +1154,11 @@
>          <key-map field-name="shipmentId"/>
>          <key-map field-name="shipmentItemSeqId"/>
>        </relation>
> +      <relation type="one-nofk" rel-entity-name="OrderItemShipGroupAssoc">
> +        <key-map field-name="orderId"/>
> +        <key-map field-name="orderItemSeqId"/>
> +        <key-map field-name="shipGroupSeqId"/>
> +      </relation>
>      </entity>
>      <entity entity-name="OrderStatus"
>              package-name="org.ofbiz.order.order"
>

No, NO NO!.

Do not change the primary key of entities.  Period.  There are
procedures for doing this, deprecating the old, making a new.  This
broke my code.

[Jacopo Cappellato-4]

On Mar 17, 2010, at 8:03 PM, Adam Heath wrote:

> [hidden email] wrote:
>> Author: jacopoc
>> Date: Sat Jan  2 18:20:19 2010
>> New Revision: 895250
>>
>> URL: http://svn.apache.org/viewvc?rev=895250&view=rev
>> Log:
>> Pretty important change in the way purchase shipments and purchase-order-to-shipment mapping are modeled:
>> * OrderShipment entity (that was mostly unused) has been enhanced to properly associate order items (OrderItemShipGrpAssoc) to shipment items: this entity is now used in place of ItemIssuance (that was improperly used, in po, for the same purpose)
>> * as a consequence of the above change, I have updated the purchase order receive screens and business logic triggered by them
>>
>> Modified:
>>    ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>>    ofbiz/trunk/applications/product/entitydef/entitymodel_shipment.xml
>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/issuance/IssuanceServices.xml
>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/receipt/ShipmentReceiptServices.xml
>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/shipment/ShipmentServices.xml
>>    ofbiz/trunk/applications/product/servicedef/services_shipment.xml
>>    ofbiz/trunk/applications/product/webapp/facility/WEB-INF/actions/inventory/ReceiveInventory.groovy
>>    ofbiz/trunk/applications/product/webapp/facility/inventory/receiveInventory.ftl
>>    ofbiz/trunk/applications/product/webapp/facility/shipment/ShipmentTabBar.ftl
>>    ofbiz/trunk/applications/product/widget/facility/ShipmentForms.xml
>>    ofbiz/trunk/applications/product/widget/facility/ShipmentScreens.xml
>>
>> Modified: ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/entitydef/entitymodel.xml?rev=895250&r1=895249&r2=895250&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/entitydef/entitymodel.xml (original)
>> +++ ofbiz/trunk/applications/order/entitydef/entitymodel.xml Sat Jan  2 18:20:19 2010
>> @@ -1131,11 +1131,13 @@
>>             title="Order Shipment Entity">
>>       <field name="orderId" type="id-ne"></field>
>>       <field name="orderItemSeqId" type="id-ne"></field>
>> +      <field name="shipGroupSeqId" type="id-ne"></field>
>>       <field name="shipmentId" type="id-ne"></field>
>>       <field name="shipmentItemSeqId" type="id-ne"></field>
>>       <field name="quantity" type="fixed-point"></field>
>>       <prim-key field="orderId"/>
>>       <prim-key field="orderItemSeqId"/>
>> +      <prim-key field="shipGroupSeqId"/>
>>       <prim-key field="shipmentId"/>
>>       <prim-key field="shipmentItemSeqId"/>
>>       <relation type="one" fk-name="ORDER_SHPMT_OHDR" rel-entity-name="OrderHeader">
>> @@ -1152,6 +1154,11 @@
>>         <key-map field-name="shipmentId"/>
>>         <key-map field-name="shipmentItemSeqId"/>
>>       </relation>
>> +      <relation type="one-nofk" rel-entity-name="OrderItemShipGroupAssoc">
>> +        <key-map field-name="orderId"/>
>> +        <key-map field-name="orderItemSeqId"/>
>> +        <key-map field-name="shipGroupSeqId"/>
>> +      </relation>
>>     </entity>
>>     <entity entity-name="OrderStatus"
>>             package-name="org.ofbiz.order.order"
>>
>
> No, NO NO!.
>
> Do not change the primary key of entities.  Period.  There are
> procedures for doing this, deprecating the old, making a new.  This
> broke my code.
>

See if this can help you:

Kind regards,

Jacopo




Author: jacopoc
Date: Mon Jan 11 15:19:03 2010
New Revision: 897897

URL: http://svn.apache.org/viewvc?rev=897897&view=rev
Log:
Implemented upgrade service for ItemIssuance->OrderShipment record migration for purchase shipments.


Modified:
   ofbiz/trunk/applications/order/script/org/ofbiz/order/UpgradeServices.xml
   ofbiz/trunk/applications/order/servicedef/services_upgrade.xml

Modified: ofbiz/trunk/applications/order/script/org/ofbiz/order/UpgradeServices.xml
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/script/org/ofbiz/order/UpgradeServices.xml?rev=897897&r1=897896&r2=897897&view=diff
==============================================================================
--- ofbiz/trunk/applications/order/script/org/ofbiz/order/UpgradeServices.xml (original)
+++ ofbiz/trunk/applications/order/script/org/ofbiz/order/UpgradeServices.xml Mon Jan 11 15:19:03 2010
@@ -48,5 +48,29 @@
            <create-value value-field="custRequestParty"/>
        </iterate>
    </simple-method>
-</simple-methods>

+    <simple-method method-name="migrateOrderShipment" short-description="Migrate data from OldOrderItemAssociation to OrderItemAssoc">
+        <entity-condition entity-name="ItemIssuance" list="itemIssuances">
+            <condition-list combine="and">
+                <condition-expr field-name="inventoryItemId" operator="equals" from-field="nullField"/>
+                <condition-expr field-name="shipmentId" operator="not-equals" from-field="nullField"/>
+                <condition-expr field-name="shipmentItemSeqId" operator="not-equals" from-field="nullField"/>
+            </condition-list>
+        </entity-condition>
+        <iterate list="itemIssuances" entry="itemIssuance">
+            <make-value entity-name="OrderShipment" value-field="orderShipment"/>
+            <set field="orderShipment.orderId" from-field="itemIssuance.orderId"/>
+            <set field="orderShipment.orderItemSeqId" from-field="itemIssuance.orderItemSeqId"/>
+            <set field="orderShipment.shipGroupSeqId" from-field="itemIssuance.shipGroupSeqId"/>
+            <set field="orderShipment.shipmentId" from-field="itemIssuance.shipmentId"/>
+            <set field="orderShipment.shipmentItemSeqId" from-field="itemIssuance.shipmentItemSeqId"/>
+            <set field="orderShipment.quantity" from-field="itemIssuance.quantity"/>
+            <create-value value-field="orderShipment"/>
+            <get-related value-field="itemIssuance" relation-name="ItemIssuanceRole" list="itemIssuanceRoles"/>
+            <iterate entry="itemIssuanceRole" list="itemIssuanceRoles">
+                <remove-value value-field="itemIssuanceRole"/>
+            </iterate>
+            <remove-value value-field="itemIssuance"/>
+        </iterate>
+    </simple-method>
+</simple-methods>

Modified: ofbiz/trunk/applications/order/servicedef/services_upgrade.xml
URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/servicedef/services_upgrade.xml?rev=897897&r1=897896&r2=897897&view=diff
==============================================================================
--- ofbiz/trunk/applications/order/servicedef/services_upgrade.xml (original)
+++ ofbiz/trunk/applications/order/servicedef/services_upgrade.xml Mon Jan 11 15:19:03 2010
@@ -45,4 +45,12 @@
            party/data/PartyTypeData.xml
        </description>
    </service>
+    <service name="migrateOrderShipment" engine="simple"
+            location="component://order/script/org/ofbiz/order/UpgradeServices.xml" invoke="migrateOrderShipment">
+        <description>
+            Since revision 895250 (2010-01-02) the entity OrderShipment is used to record purchase order items that
+            will be received as part of a purchase shipment. Previously ItemIssuance was used with an empty inventoryId.
+            This service will replace ItemIssuaces with OrderShipment records for the required shipments.
+        </description>
+    </service>
</services>

[Adam Heath-2]

Jacopo Cappellato wrote:

> On Mar 17, 2010, at 8:03 PM, Adam Heath wrote:
>
>> [hidden email] wrote:
>>> Author: jacopoc
>>> Date: Sat Jan  2 18:20:19 2010
>>> New Revision: 895250
>>>
>>> URL: http://svn.apache.org/viewvc?rev=895250&view=rev
>>> Log:
>>> Pretty important change in the way purchase shipments and purchase-order-to-shipment mapping are modeled:
>>> * OrderShipment entity (that was mostly unused) has been enhanced to properly associate order items (OrderItemShipGrpAssoc) to shipment items: this entity is now used in place of ItemIssuance (that was improperly used, in po, for the same purpose)
>>> * as a consequence of the above change, I have updated the purchase order receive screens and business logic triggered by them
>>>
>>> Modified:
>>>    ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>>>    ofbiz/trunk/applications/product/entitydef/entitymodel_shipment.xml
>>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/issuance/IssuanceServices.xml
>>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/receipt/ShipmentReceiptServices.xml
>>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/shipment/ShipmentServices.xml
>>>    ofbiz/trunk/applications/product/servicedef/services_shipment.xml
>>>    ofbiz/trunk/applications/product/webapp/facility/WEB-INF/actions/inventory/ReceiveInventory.groovy
>>>    ofbiz/trunk/applications/product/webapp/facility/inventory/receiveInventory.ftl
>>>    ofbiz/trunk/applications/product/webapp/facility/shipment/ShipmentTabBar.ftl
>>>    ofbiz/trunk/applications/product/widget/facility/ShipmentForms.xml
>>>    ofbiz/trunk/applications/product/widget/facility/ShipmentScreens.xml
>>>
>>> Modified: ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/entitydef/entitymodel.xml?rev=895250&r1=895249&r2=895250&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/order/entitydef/entitymodel.xml (original)
>>> +++ ofbiz/trunk/applications/order/entitydef/entitymodel.xml Sat Jan  2 18:20:19 2010
>>> @@ -1131,11 +1131,13 @@
>>>             title="Order Shipment Entity">
>>>       <field name="orderId" type="id-ne"></field>
>>>       <field name="orderItemSeqId" type="id-ne"></field>
>>> +      <field name="shipGroupSeqId" type="id-ne"></field>
>>>       <field name="shipmentId" type="id-ne"></field>
>>>       <field name="shipmentItemSeqId" type="id-ne"></field>
>>>       <field name="quantity" type="fixed-point"></field>
>>>       <prim-key field="orderId"/>
>>>       <prim-key field="orderItemSeqId"/>
>>> +      <prim-key field="shipGroupSeqId"/>
>>>       <prim-key field="shipmentId"/>
>>>       <prim-key field="shipmentItemSeqId"/>
>>>       <relation type="one" fk-name="ORDER_SHPMT_OHDR" rel-entity-name="OrderHeader">
>>> @@ -1152,6 +1154,11 @@
>>>         <key-map field-name="shipmentId"/>
>>>         <key-map field-name="shipmentItemSeqId"/>
>>>       </relation>
>>> +      <relation type="one-nofk" rel-entity-name="OrderItemShipGroupAssoc">
>>> +        <key-map field-name="orderId"/>
>>> +        <key-map field-name="orderItemSeqId"/>
>>> +        <key-map field-name="shipGroupSeqId"/>
>>> +      </relation>
>>>     </entity>
>>>     <entity entity-name="OrderStatus"
>>>             package-name="org.ofbiz.order.order"
>>>
>> No, NO NO!.
>>
>> Do not change the primary key of entities.  Period.  There are
>> procedures for doing this, deprecating the old, making a new.  This
>> broke my code.
>>
>
> See if this can help you:

Sorry, no.  I don't even have to try to know this isn't correct.  You
haven't done proper deprecation on the entity.

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.

[Adam Heath-2]

Adam Heath wrote:

> Jacopo Cappellato wrote:
>> On Mar 17, 2010, at 8:03 PM, Adam Heath wrote:
>>
>>> [hidden email] wrote:
>>>> Author: jacopoc
>>>> Date: Sat Jan  2 18:20:19 2010
>>>> New Revision: 895250
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=895250&view=rev
>>>> Log:
>>>> Pretty important change in the way purchase shipments and purchase-order-to-shipment mapping are modeled:
>>>> * OrderShipment entity (that was mostly unused) has been enhanced to properly associate order items (OrderItemShipGrpAssoc) to shipment items: this entity is now used in place of ItemIssuance (that was improperly used, in po, for the same purpose)
>>>> * as a consequence of the above change, I have updated the purchase order receive screens and business logic triggered by them
>>>>
>>>> Modified:
>>>>    ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>>>>    ofbiz/trunk/applications/product/entitydef/entitymodel_shipment.xml
>>>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/issuance/IssuanceServices.xml
>>>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/receipt/ShipmentReceiptServices.xml
>>>>    ofbiz/trunk/applications/product/script/org/ofbiz/shipment/shipment/ShipmentServices.xml
>>>>    ofbiz/trunk/applications/product/servicedef/services_shipment.xml
>>>>    ofbiz/trunk/applications/product/webapp/facility/WEB-INF/actions/inventory/ReceiveInventory.groovy
>>>>    ofbiz/trunk/applications/product/webapp/facility/inventory/receiveInventory.ftl
>>>>    ofbiz/trunk/applications/product/webapp/facility/shipment/ShipmentTabBar.ftl
>>>>    ofbiz/trunk/applications/product/widget/facility/ShipmentForms.xml
>>>>    ofbiz/trunk/applications/product/widget/facility/ShipmentScreens.xml
>>>>
>>>> Modified: ofbiz/trunk/applications/order/entitydef/entitymodel.xml
>>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/entitydef/entitymodel.xml?rev=895250&r1=895249&r2=895250&view=diff
>>>> ==============================================================================
>>>> --- ofbiz/trunk/applications/order/entitydef/entitymodel.xml (original)
>>>> +++ ofbiz/trunk/applications/order/entitydef/entitymodel.xml Sat Jan  2 18:20:19 2010
>>>> @@ -1131,11 +1131,13 @@
>>>>             title="Order Shipment Entity">
>>>>       <field name="orderId" type="id-ne"></field>
>>>>       <field name="orderItemSeqId" type="id-ne"></field>
>>>> +      <field name="shipGroupSeqId" type="id-ne"></field>
>>>>       <field name="shipmentId" type="id-ne"></field>
>>>>       <field name="shipmentItemSeqId" type="id-ne"></field>
>>>>       <field name="quantity" type="fixed-point"></field>
>>>>       <prim-key field="orderId"/>
>>>>       <prim-key field="orderItemSeqId"/>
>>>> +      <prim-key field="shipGroupSeqId"/>
>>>>       <prim-key field="shipmentId"/>
>>>>       <prim-key field="shipmentItemSeqId"/>
>>>>       <relation type="one" fk-name="ORDER_SHPMT_OHDR" rel-entity-name="OrderHeader">
>>>> @@ -1152,6 +1154,11 @@
>>>>         <key-map field-name="shipmentId"/>
>>>>         <key-map field-name="shipmentItemSeqId"/>
>>>>       </relation>
>>>> +      <relation type="one-nofk" rel-entity-name="OrderItemShipGroupAssoc">
>>>> +        <key-map field-name="orderId"/>
>>>> +        <key-map field-name="orderItemSeqId"/>
>>>> +        <key-map field-name="shipGroupSeqId"/>
>>>> +      </relation>
>>>>     </entity>
>>>>     <entity entity-name="OrderStatus"
>>>>             package-name="org.ofbiz.order.order"
>>>>
>>> No, NO NO!.
>>>
>>> Do not change the primary key of entities.  Period.  There are
>>> procedures for doing this, deprecating the old, making a new.  This
>>> broke my code.
>>>
>> See if this can help you:
>
> Sorry, no.  I don't even have to try to know this isn't correct.  You
> haven't done proper deprecation on the entity.
>
> 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.

You have to deprecate the OrderShipment entity, it has existed since
at least July, 2006.

[Jacopo Cappellato-4]

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.

> 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

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)

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.

Kind regards,

Jacopo

[Adam Heath-2]

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.

[Jacopo Cappellato-4]

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

[Adam Heath-2]

Jacopo Cappellato wrote:

> 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.

Are you discussing your ItemIssuance to OrderShipment change?  If so,
I'm not.

OrderShipment has been in existance for a long long time.  It has been
released into the wild.  It was referenced by lots of places for at
least several years.  It has been used in production environments.
Therefore, any changes to the primary key of that entity have to be
handled with care.  This was not done.

>> 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...

I absolutely do.  I've looked at the svn history.  There's no way to
argue that.  It's available for anyone to see, if they just look in
the past.  I see it referenced in versions that were from released
versions of ofbiz.  It's been used by production systems.  Care must
be taken.

>>> 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.

So, you are admitting that the code has been in use for years.  Yet
you don't see the need to implement proper deprecation of the
OrderShipment entity(not ItemIssuance).

>>> 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.

There are other people out there in this big blue world of ours that
don't follow this mailing list.  They will see a new version of ofbiz
get released, and go: "woo!  time to schedule an upgrade in a year
from now, after we've had a chance to read over all the changes and
digest what they mean."  So when they finally get around to doing this
upgrade, your change will be 1.5 years old, and they themselves will
be working with their original ofbiz that is 2.5 years old(or older).

>
>> You are lucky I discovered this problem before we released.
>
> Lucky?

Lucky, because we need to fix it before we release, not after.


>> 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.

Do you really want to play that card?  Really?  This has nothing to do
with ofbiz application experience.  This is a procedure for all of
ofbiz, not just the applications.

Yes, it is true, that you haven't seen either Ean or I discussing in
public various ofbiz applications.  That doesn't mean we haven't been
using them.  Remember, we have been involved for years.  The first
major site we did used an ofbiz from january, 2003.  And I'm the one
who wrote a major chunk of the view entity cache clearing code.

[Jacopo Cappellato-4]

Adam,

this is going to be a long thread and I don't want it to be.
I already tried to explain why I decided to change the primary key of OrderShipment without following the deprecation mechanism (that I think it is a very good best practice) in this specific situation.
I perfectly know when and how the shipment plans (i.e. the feature for which the OrderShipment entity was introduced) were implemented: I know because I contributed it (and the OrderShipment entity of course).
I also know that it was only partially implemented and so I would be very surprised (especially because no one over the years ever asked or reported bugs etc.. about it - apart from me that was aware of its existence) if it is really used in production by someone. This is simply to explain that I was not irresponsible when I did it.

However you are right if you say that I can't be completely sure if someone somewhere is using the entity. Also, lately the community seems to be more inclined to transform "best practices" (to be applied "cum granu salis") into "policies" to be applied blindly.
In some way this is good because it requires less skills and creativity (but more pragmatism) and also less skilled persons can become good reviewers just by following written rules.
This is probably less interesting but maybe necessary given the grow of the community.

So ok, I am convinced now, we have to deprecate that entity etc.

What does your book of policies says about this: is it something I *have* to do as a punishment for this bad commit or will you take care of it?

Jacopo

PS: BTW, if no one will implement the deprecation pattern in the next few days I will do that, no problem.


On Mar 18, 2010, at 5:10 PM, Adam Heath wrote:

> Jacopo Cappellato wrote:
>> 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.
>
> Are you discussing your ItemIssuance to OrderShipment change?  If so,
> I'm not.
>
> OrderShipment has been in existance for a long long time.  It has been
> released into the wild.  It was referenced by lots of places for at
> least several years.  It has been used in production environments.
> Therefore, any changes to the primary key of that entity have to be
> handled with care.  This was not done.
>
>>> 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...
>
> I absolutely do.  I've looked at the svn history.  There's no way to
> argue that.  It's available for anyone to see, if they just look in
> the past.  I see it referenced in versions that were from released
> versions of ofbiz.  It's been used by production systems.  Care must
> be taken.
>
>>>> 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.
>
> So, you are admitting that the code has been in use for years.  Yet
> you don't see the need to implement proper deprecation of the
> OrderShipment entity(not ItemIssuance).
>
>>>> 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.
>
> There are other people out there in this big blue world of ours that
> don't follow this mailing list.  They will see a new version of ofbiz
> get released, and go: "woo!  time to schedule an upgrade in a year
> from now, after we've had a chance to read over all the changes and
> digest what they mean."  So when they finally get around to doing this
> upgrade, your change will be 1.5 years old, and they themselves will
> be working with their original ofbiz that is 2.5 years old(or older).
>
>>
>>> You are lucky I discovered this problem before we released.
>>
>> Lucky?
>
> Lucky, because we need to fix it before we release, not after.
>
>
>>> 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.
>
> Do you really want to play that card?  Really?  This has nothing to do
> with ofbiz application experience.  This is a procedure for all of
> ofbiz, not just the applications.
>
> Yes, it is true, that you haven't seen either Ean or I discussing in
> public various ofbiz applications.  That doesn't mean we haven't been
> using them.  Remember, we have been involved for years.  The first
> major site we did used an ofbiz from january, 2003.  And I'm the one
> who wrote a major chunk of the view entity cache clearing code.

[Adam Heath-2]

Jacopo Cappellato wrote:

> Adam,
>
> this is going to be a long thread and I don't want it to be.
> I already tried to explain why I decided to change the primary key of OrderShipment without following the deprecation mechanism (that I think it is a very good best practice) in this specific situation.
> I perfectly know when and how the shipment plans (i.e. the feature for which the OrderShipment entity was introduced) were implemented: I know because I contributed it (and the OrderShipment entity of course).
> I also know that it was only partially implemented and so I would be very surprised (especially because no one over the years ever asked or reported bugs etc.. about it - apart from me that was aware of its existence) if it is really used in production by someone. This is simply to explain that I was not irresponsible when I did it.
>
> However you are right if you say that I can't be completely sure if someone somewhere is using the entity. Also, lately the community seems to be more inclined to transform "best practices" (to be applied "cum granu salis") into "policies" to be applied blindly.
> In some way this is good because it requires less skills and creativity (but more pragmatism) and also less skilled persons can become good reviewers just by following written rules.
> This is probably less interesting but maybe necessary given the grow of the community.
>
> So ok, I am convinced now, we have to deprecate that entity etc.
>
> What does your book of policies says about this: is it something I *have* to do as a punishment for this bad commit or will you take care of it?

If I implied that it had to be you to do it, I apoligize.  I didn't
mean it to sound that way.

I was trying to discuss with you about the reasons behind this change,
so that we could all agree as to what the reasons and use cases were.

Now that it seems we are on the same page, we can go about fixing this
as a team.  I'll admit, however, that I have never gone thru an entity
deprecation exercise.  And OrderShipment is a very low-level entity,
that has tons of references, so doing this right could be very
complex.  The reason why I was so heated was that I understood this
would be a lot of work, and I was trying to see if there is a better
way than changing the primary key.

The reason the original committer is generally the best one to fix
their original commit, is because that original committer knows the
most about what they were trying to do.  That original person knows
their own code they wrote, and probably has an understand of the rest
of the files that they were modifying.

However, due to the scope of this deprecation, we need the rest of the
community to help out.  Unless we decide to revert this primary key
change, and due this a different way.

> PS: BTW, if no one will implement the deprecation pattern in the next few days I will do that, no problem.

There's no big hurry.  My previously mentioned importer already had
the shipGroupSeqId available(it was hard-coded to 00001, as it's
somewhat simple), so it wasn't a big change for me personally.

[Scott Gray-2]

On 18/03/2010, at 10:50 AM, Adam Heath wrote:

> Jacopo Cappellato wrote:
>> Adam,
>>
>> this is going to be a long thread and I don't want it to be.
>> I already tried to explain why I decided to change the primary key of OrderShipment without following the deprecation mechanism (that I think it is a very good best practice) in this specific situation.
>> I perfectly know when and how the shipment plans (i.e. the feature for which the OrderShipment entity was introduced) were implemented: I know because I contributed it (and the OrderShipment entity of course).
>> I also know that it was only partially implemented and so I would be very surprised (especially because no one over the years ever asked or reported bugs etc.. about it - apart from me that was aware of its existence) if it is really used in production by someone. This is simply to explain that I was not irresponsible when I did it.
>>
>> However you are right if you say that I can't be completely sure if someone somewhere is using the entity. Also, lately the community seems to be more inclined to transform "best practices" (to be applied "cum granu salis") into "policies" to be applied blindly.
>> In some way this is good because it requires less skills and creativity (but more pragmatism) and also less skilled persons can become good reviewers just by following written rules.
>> This is probably less interesting but maybe necessary given the grow of the community.
>>
>> So ok, I am convinced now, we have to deprecate that entity etc.
>>
>> What does your book of policies says about this: is it something I *have* to do as a punishment for this bad commit or will you take care of it?
>
> If I implied that it had to be you to do it, I apoligize.  I didn't
> mean it to sound that way.
>
> I was trying to discuss with you about the reasons behind this change,
> so that we could all agree as to what the reasons and use cases were.
>
> Now that it seems we are on the same page, we can go about fixing this
> as a team.  I'll admit, however, that I have never gone thru an entity
> deprecation exercise.  And OrderShipment is a very low-level entity,
> that has tons of references, so doing this right could be very
> complex.  The reason why I was so heated was that I understood this
> would be a lot of work, and I was trying to see if there is a better
> way than changing the primary key.
>
> The reason the original committer is generally the best one to fix
> their original commit, is because that original committer knows the
> most about what they were trying to do.  That original person knows
> their own code they wrote, and probably has an understand of the rest
> of the files that they were modifying.
>
> However, due to the scope of this deprecation, we need the rest of the
> community to help out.  Unless we decide to revert this primary key
> change, and due this a different way.
>
>> PS: BTW, if no one will implement the deprecation pattern in the next few days I will do that, no problem.
>
> There's no big hurry.  My previously mentioned importer already had
> the shipGroupSeqId available(it was hard-coded to 00001, as it's
> somewhat simple), so it wasn't a big change for me personally.

I'm yet to form an opinion on whether the approach Jacopo took was a valid one,  I've been looking at the code and I don't yet understand what problems this change may have caused for downstream users.
Adam, would you mind explaining in more detail how the change has effected you negatively?

Thanks
Scott

[Jacopo Cappellato-4]

On Mar 18, 2010, at 5:50 PM, Adam Heath wrote:

> I'll admit, however, that I have never gone thru an entity
> deprecation exercise.

You are not alone... in fact I think I am one of the very few committers that ever implemented the entity deprecation pattern in the past :-)

>  And OrderShipment is a very low-level entity,
> that has tons of references, so doing this right could be very
> complex.  

It shouldn't be too difficult actually, I'll have a look soon when I have a chance.
But if you are having other issues caused by the change I did please let me know because maybe I can suggest a better solution.

Jacopo

[Jacopo Cappellato-4]

On Mar 18, 2010, at 5:50 PM, Adam Heath wrote:

> However, due to the scope of this deprecation, we need the rest of the
> community to help out.  Unless we decide to revert this primary key
> change, and due this a different way.

I don't think I need help apart for the decision about the name of the entity that will deprecate OrderShipment.
Please suggest!

Jacopo

[Adam Heath-2]

Scott Gray wrote:

> On 18/03/2010, at 10:50 AM, Adam Heath wrote:
>
>> Jacopo Cappellato wrote:
>>> Adam,
>>>
>>> this is going to be a long thread and I don't want it to be.
>>> I already tried to explain why I decided to change the primary key of OrderShipment without following the deprecation mechanism (that I think it is a very good best practice) in this specific situation.
>>> I perfectly know when and how the shipment plans (i.e. the feature for which the OrderShipment entity was introduced) were implemented: I know because I contributed it (and the OrderShipment entity of course).
>>> I also know that it was only partially implemented and so I would be very surprised (especially because no one over the years ever asked or reported bugs etc.. about it - apart from me that was aware of its existence) if it is really used in production by someone. This is simply to explain that I was not irresponsible when I did it.
>>>
>>> However you are right if you say that I can't be completely sure if someone somewhere is using the entity. Also, lately the community seems to be more inclined to transform "best practices" (to be applied "cum granu salis") into "policies" to be applied blindly.
>>> In some way this is good because it requires less skills and creativity (but more pragmatism) and also less skilled persons can become good reviewers just by following written rules.
>>> This is probably less interesting but maybe necessary given the grow of the community.
>>>
>>> So ok, I am convinced now, we have to deprecate that entity etc.
>>>
>>> What does your book of policies says about this: is it something I *have* to do as a punishment for this bad commit or will you take care of it?
>> If I implied that it had to be you to do it, I apoligize.  I didn't
>> mean it to sound that way.
>>
>> I was trying to discuss with you about the reasons behind this change,
>> so that we could all agree as to what the reasons and use cases were.
>>
>> Now that it seems we are on the same page, we can go about fixing this
>> as a team.  I'll admit, however, that I have never gone thru an entity
>> deprecation exercise.  And OrderShipment is a very low-level entity,
>> that has tons of references, so doing this right could be very
>> complex.  The reason why I was so heated was that I understood this
>> would be a lot of work, and I was trying to see if there is a better
>> way than changing the primary key.
>>
>> The reason the original committer is generally the best one to fix
>> their original commit, is because that original committer knows the
>> most about what they were trying to do.  That original person knows
>> their own code they wrote, and probably has an understand of the rest
>> of the files that they were modifying.
>>
>> However, due to the scope of this deprecation, we need the rest of the
>> community to help out.  Unless we decide to revert this primary key
>> change, and due this a different way.
>>
>>> PS: BTW, if no one will implement the deprecation pattern in the next few days I will do that, no problem.
>> There's no big hurry.  My previously mentioned importer already had
>> the shipGroupSeqId available(it was hard-coded to 00001, as it's
>> somewhat simple), so it wasn't a big change for me personally.
>
> I'm yet to form an opinion on whether the approach Jacopo took was a valid one,  I've been looking at the code and I don't yet understand what problems this change may have caused for downstream users.
> Adam, would you mind explaining in more detail how the change has effected you negatively?

A new field was added to the OrderShipment entity, and that field was
made a primary key.

My importer, which started from an empty database, thru an error,
because this field was not set, and null is not allowed.  As I said,
the fix for me was simple, as this particular job hasn't gone live
yet, so I don't have to worry so much about migration.

However, if I had an existing set of data, and just tried to upgrade
ofbiz in place, the upgrade itself would have failed, as the entity
engine would not alter the table to add the new primary key field.

Even if I did an xml data dump, then reinstalled ofbiz, the xml data
dump would not import, as the data would be missing this new primary
key field.

[Jacopo Cappellato-4]

On Mar 18, 2010, at 6:04 PM, Adam Heath wrote:

> My importer, which started from an empty database, thru an error,
> because this field was not set, and null is not allowed.

What I find interesting is that your OrderShipment entity was not empty.

Jacopo

[Adam Heath-2]

Jacopo Cappellato wrote:
> On Mar 18, 2010, at 6:04 PM, Adam Heath wrote:
>
>> My importer, which started from an empty database, thru an error,
>> because this field was not set, and null is not allowed.
>
> What I find interesting is that your OrderShipment entity was not empty.

Because my importer was creating completed/approved orders, and that
is how shipments are done.  To get them to show in the backend
ordermgr, you have to create OrderShipment entities.

Or, do you mean that no items were listed in the shipment?

Before this change, I could import old orders into a completed state.
 Approved orders, one that exist in the old system, but haven't been
fully shipped(due to inventory not yet being available, or they just
haven't been fully processed), also worked.  I could do a quick ship
entire order, and everything would function as it should.

It may be that your change isn't really needed.  But I will admit I
don't have a complete knowledge of everything.

[Scott Gray-2]

On 18/03/2010, at 11:04 AM, Adam Heath wrote:

> Scott Gray wrote:
>> On 18/03/2010, at 10:50 AM, Adam Heath wrote:
>>
>>> Jacopo Cappellato wrote:
>>>> Adam,
>>>>
>>>> this is going to be a long thread and I don't want it to be.
>>>> I already tried to explain why I decided to change the primary key of OrderShipment without following the deprecation mechanism (that I think it is a very good best practice) in this specific situation.
>>>> I perfectly know when and how the shipment plans (i.e. the feature for which the OrderShipment entity was introduced) were implemented: I know because I contributed it (and the OrderShipment entity of course).
>>>> I also know that it was only partially implemented and so I would be very surprised (especially because no one over the years ever asked or reported bugs etc.. about it - apart from me that was aware of its existence) if it is really used in production by someone. This is simply to explain that I was not irresponsible when I did it.
>>>>
>>>> However you are right if you say that I can't be completely sure if someone somewhere is using the entity. Also, lately the community seems to be more inclined to transform "best practices" (to be applied "cum granu salis") into "policies" to be applied blindly.
>>>> In some way this is good because it requires less skills and creativity (but more pragmatism) and also less skilled persons can become good reviewers just by following written rules.
>>>> This is probably less interesting but maybe necessary given the grow of the community.
>>>>
>>>> So ok, I am convinced now, we have to deprecate that entity etc.
>>>>
>>>> What does your book of policies says about this: is it something I *have* to do as a punishment for this bad commit or will you take care of it?
>>> If I implied that it had to be you to do it, I apoligize.  I didn't
>>> mean it to sound that way.
>>>
>>> I was trying to discuss with you about the reasons behind this change,
>>> so that we could all agree as to what the reasons and use cases were.
>>>
>>> Now that it seems we are on the same page, we can go about fixing this
>>> as a team.  I'll admit, however, that I have never gone thru an entity
>>> deprecation exercise.  And OrderShipment is a very low-level entity,
>>> that has tons of references, so doing this right could be very
>>> complex.  The reason why I was so heated was that I understood this
>>> would be a lot of work, and I was trying to see if there is a better
>>> way than changing the primary key.
>>>
>>> The reason the original committer is generally the best one to fix
>>> their original commit, is because that original committer knows the
>>> most about what they were trying to do.  That original person knows
>>> their own code they wrote, and probably has an understand of the rest
>>> of the files that they were modifying.
>>>
>>> However, due to the scope of this deprecation, we need the rest of the
>>> community to help out.  Unless we decide to revert this primary key
>>> change, and due this a different way.
>>>
>>>> PS: BTW, if no one will implement the deprecation pattern in the next few days I will do that, no problem.
>>> There's no big hurry.  My previously mentioned importer already had
>>> the shipGroupSeqId available(it was hard-coded to 00001, as it's
>>> somewhat simple), so it wasn't a big change for me personally.
>>
>> I'm yet to form an opinion on whether the approach Jacopo took was a valid one,  I've been looking at the code and I don't yet understand what problems this change may have caused for downstream users.
>> Adam, would you mind explaining in more detail how the change has effected you negatively?
>
> A new field was added to the OrderShipment entity, and that field was
> made a primary key.
>
> My importer, which started from an empty database, thru an error,
> because this field was not set, and null is not allowed.  As I said,
> the fix for me was simple, as this particular job hasn't gone live
> yet, so I don't have to worry so much about migration.

I would argue that in this case the early failure was a good thing.  If the deprecation pattern had been followed it's possible you wouldn't have noticed until much later and then would have had to deal with updating your importer as well as migrating the data from the old table to the new.

> However, if I had an existing set of data, and just tried to upgrade
> ofbiz in place, the upgrade itself would have failed, as the entity
> engine would not alter the table to add the new primary key field.

This is incorrect, OFBiz does not attempt to add new fields to the primary key, it is considered an unsafe operation.  A new field is simply added and that field is unconstrained in terms of allowing null values.
The developer would have been alerted in the logs during startup and still have the ability to populate the field before manually making the field part of the primary key.

>
> Even if I did an xml data dump, then reinstalled ofbiz, the xml data
> dump would not import, as the data would be missing this new primary
> key field.


As above, the import in this case would have been successful I believe.

I'm not arguing against the deprecation pattern but I do believe there can always be exceptions to our "rules".

Regards
Scott

[Adam Heath-2]

Adam Heath wrote:

> Jacopo Cappellato wrote:
>> On Mar 18, 2010, at 6:04 PM, Adam Heath wrote:
>>
>>> My importer, which started from an empty database, thru an error,
>>> because this field was not set, and null is not allowed.
>> What I find interesting is that your OrderShipment entity was not empty.
>
> Because my importer was creating completed/approved orders, and that
> is how shipments are done.  To get them to show in the backend
> ordermgr, you have to create OrderShipment entities.
>
> Or, do you mean that no items were listed in the shipment?
>
> Before this change, I could import old orders into a completed state.
>  Approved orders, one that exist in the old system, but haven't been
> fully shipped(due to inventory not yet being available, or they just
> haven't been fully processed), also worked.  I could do a quick ship
> entire order, and everything would function as it should.
>
> It may be that your change isn't really needed.  But I will admit I
> don't have a complete knowledge of everything.

OrderItemShipGroupAssoc?

[Adam Heath-2]

Scott Gray wrote:

> On 18/03/2010, at 11:04 AM, Adam Heath wrote:
>
>> Scott Gray wrote:
>>> On 18/03/2010, at 10:50 AM, Adam Heath wrote:
>>>
>>>> Jacopo Cappellato wrote:
>>>>> Adam,
>>>>>
>>>>> this is going to be a long thread and I don't want it to be.
>>>>> I already tried to explain why I decided to change the primary key of OrderShipment without following the deprecation mechanism (that I think it is a very good best practice) in this specific situation.
>>>>> I perfectly know when and how the shipment plans (i.e. the feature for which the OrderShipment entity was introduced) were implemented: I know because I contributed it (and the OrderShipment entity of course).
>>>>> I also know that it was only partially implemented and so I would be very surprised (especially because no one over the years ever asked or reported bugs etc.. about it - apart from me that was aware of its existence) if it is really used in production by someone. This is simply to explain that I was not irresponsible when I did it.
>>>>>
>>>>> However you are right if you say that I can't be completely sure if someone somewhere is using the entity. Also, lately the community seems to be more inclined to transform "best practices" (to be applied "cum granu salis") into "policies" to be applied blindly.
>>>>> In some way this is good because it requires less skills and creativity (but more pragmatism) and also less skilled persons can become good reviewers just by following written rules.
>>>>> This is probably less interesting but maybe necessary given the grow of the community.
>>>>>
>>>>> So ok, I am convinced now, we have to deprecate that entity etc.
>>>>>
>>>>> What does your book of policies says about this: is it something I *have* to do as a punishment for this bad commit or will you take care of it?
>>>> If I implied that it had to be you to do it, I apoligize.  I didn't
>>>> mean it to sound that way.
>>>>
>>>> I was trying to discuss with you about the reasons behind this change,
>>>> so that we could all agree as to what the reasons and use cases were.
>>>>
>>>> Now that it seems we are on the same page, we can go about fixing this
>>>> as a team.  I'll admit, however, that I have never gone thru an entity
>>>> deprecation exercise.  And OrderShipment is a very low-level entity,
>>>> that has tons of references, so doing this right could be very
>>>> complex.  The reason why I was so heated was that I understood this
>>>> would be a lot of work, and I was trying to see if there is a better
>>>> way than changing the primary key.
>>>>
>>>> The reason the original committer is generally the best one to fix
>>>> their original commit, is because that original committer knows the
>>>> most about what they were trying to do.  That original person knows
>>>> their own code they wrote, and probably has an understand of the rest
>>>> of the files that they were modifying.
>>>>
>>>> However, due to the scope of this deprecation, we need the rest of the
>>>> community to help out.  Unless we decide to revert this primary key
>>>> change, and due this a different way.
>>>>
>>>>> PS: BTW, if no one will implement the deprecation pattern in the next few days I will do that, no problem.
>>>> There's no big hurry.  My previously mentioned importer already had
>>>> the shipGroupSeqId available(it was hard-coded to 00001, as it's
>>>> somewhat simple), so it wasn't a big change for me personally.
>>> I'm yet to form an opinion on whether the approach Jacopo took was a valid one,  I've been looking at the code and I don't yet understand what problems this change may have caused for downstream users.
>>> Adam, would you mind explaining in more detail how the change has effected you negatively?
>> A new field was added to the OrderShipment entity, and that field was
>> made a primary key.
>>
>> My importer, which started from an empty database, thru an error,
>> because this field was not set, and null is not allowed.  As I said,
>> the fix for me was simple, as this particular job hasn't gone live
>> yet, so I don't have to worry so much about migration.
>
> I would argue that in this case the early failure was a good thing.  If the deprecation pattern had been followed it's possible you wouldn't have noticed until much later and then would have had to deal with updating your importer as well as migrating the data from the old table to the new.

You don't understand how deprecation works.

The entity is renamed.  So there would still be the failure in my
importer, as it tried to update an entity that doesn't exist.

Additionally, if someone upgrading their data does an xml dump, the
old data will be dumped with the entity named OrderShipment.  After
installing the newer ofbiz, that identiy will also not exist, so the
xml import will fail as well, when it can't find the OrderShipment
entity(which was renamed to OldOrderShipment).

[Adam Heath-2]

Jacopo Cappellato wrote:
> On Mar 18, 2010, at 6:04 PM, Adam Heath wrote:
>
>> My importer, which started from an empty database, thru an error,
>> because this field was not set, and null is not allowed.
>
> What I find interesting is that your OrderShipment entity was not empty.

When I set about writing my order importer(which is pulling from a
legacy database that has products in an mysql database, but orders in
an mssql database), I created a non-trivial order thru ecommerce, and
thru the shared demo system.  I then looked at the actual entities in
the database, to see how the fields mapped into what was displayed by
the ordermgr backend.  I then mapped the old databases to what ofbiz
wanted.

I did comparisons of created, approved, and completed orders, with
available inventory, and without, did credit card comparisons of
authorized, captured, not authorized, looked at how tracking numbers
were implemented.  In all such cases, everything that was created thru
ecommerce appeared to work correctly, with my understanding of the
order work flow.

The frontend we have uses ShoppingCart, and CheckOutHelper, and the
data those create seemed to match my importer, so I was happy.
Nothing seemed to not work.

So what was your change attempting to accomplish?  I still haven't
seen that explanation.