OFBiz
  1. OFBiz
  2. OFBIZ-4501

Incorrect use of eca for create/updateShipment

    Details

    • Type: Bug Bug
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: Release Branch 11.04, Trunk
    • Fix Version/s: None
    • Component/s: product
    • Labels:
      None

      Description

      createShipment service doesn't populate the facility and order info into shipment. Instead it is handled by eca rules. This is wrong. ECA rules should be used to update other objects or cause other actions and not update the object that is being committed. This makes it difficult to traverse the code. Can also cause bugs that are difficult troubleshoot. e.g: In this case, facilities are populated in shipment by method setShipmentSettingsFromPrimaryOrder, but eca rule checking for originFacilityId gets executed before it is populated. Following eca rules should be removed and instead the code should be added to create/updateshipment methods.

      <!-- if new originFacilityId or destinationFacilityId, get settings from facilities -->
      <eca service="createShipment" event="commit">
      <condition field-name="originFacilityId" operator="is-not-empty"/>
      <action service="setShipmentSettingsFromFacilities" mode="sync"/>
      </eca>
      <eca service="createShipment" event="commit">
      <condition field-name="destinationFacilityId" operator="is-not-empty"/>
      <action service="setShipmentSettingsFromFacilities" mode="sync"/>
      </eca>
      <eca service="updateShipment" event="commit">
      <condition-field field-name="originFacilityId" operator="not-equals" to-field-name="oldOriginFacilityId"/>
      <condition field-name="originFacilityId" operator="is-not-empty"/>
      <action service="setShipmentSettingsFromFacilities" mode="sync"/>
      </eca>
      <eca service="updateShipment" event="commit">
      <condition-field field-name="destinationFacilityId" operator="not-equals" to-field-name="oldDestinationFacilityId"/>
      <condition field-name="destinationFacilityId" operator="is-not-empty"/>
      <action service="setShipmentSettingsFromFacilities" mode="sync"/>
      </eca>

      <!-- if new primaryOrderId, get settings from order -->
      <eca service="createShipment" event="commit">
      <condition field-name="primaryOrderId" operator="is-not-empty"/>
      <action service="setShipmentSettingsFromPrimaryOrder" mode="sync"/>
      </eca>

      <eca service="updateShipment" event="commit">
      <condition-field field-name="primaryOrderId" operator="not-equals" to-field-name="oldPrimaryOrderId"/>
      <condition field-name="primaryOrderId" operator="is-not-empty"/>
      <action service="setShipmentSettingsFromPrimaryOrder" mode="sync"/>
      </eca>

        Activity

        Hide
        Kiran Gawde added a comment -

        Changes are done to 4 files:
        controller.xml, services_shipping.xml, secas_shipment.xml(Removed reference to setShipmentSettingsFromPrimaryOrder, setShipmentSettingsFromFacilities)

        ShipmentServices.xml(Changes look a lot, but most are just renaming of newEntity and lookedUpValue to shipment)

        Show
        Kiran Gawde added a comment - Changes are done to 4 files: controller.xml, services_shipping.xml, secas_shipment.xml(Removed reference to setShipmentSettingsFromPrimaryOrder, setShipmentSettingsFromFacilities) ShipmentServices.xml(Changes look a lot, but most are just renaming of newEntity and lookedUpValue to shipment)
        Hide
        Kiran Gawde added a comment -

        Moved ShipmentService.xml changes in separate file for clarity. Also fixed the facilityId for DROP_SHIPMENT order

        Show
        Kiran Gawde added a comment - Moved ShipmentService.xml changes in separate file for clarity. Also fixed the facilityId for DROP_SHIPMENT order
        Hide
        Kiran Gawde added a comment -

        Replaced tabs with spaces. And added following before calling setShipmentSettingsFromPrimaryOrder:

        <set from-field="shipment.shipmentId" field="parameters.shipmentId"/>

        Show
        Kiran Gawde added a comment - Replaced tabs with spaces. And added following before calling setShipmentSettingsFromPrimaryOrder: <set from-field="shipment.shipmentId" field="parameters.shipmentId"/>
        Hide
        Kiran Gawde added a comment -

        Separated setShipmentPartiesAndContacts & createShipmentRouteSegments.

        Now following sequence is followed while creating/updating the shipment:
        setShipmentSettingsFromPrimaryOrder
        setShipmentSettingsFromFacilities
        setShipmentPartiesAndContacts
        createShipmentRouteSegments

        Show
        Kiran Gawde added a comment - Separated setShipmentPartiesAndContacts & createShipmentRouteSegments. Now following sequence is followed while creating/updating the shipment: setShipmentSettingsFromPrimaryOrder setShipmentSettingsFromFacilities setShipmentPartiesAndContacts createShipmentRouteSegments
        Hide
        Kiran Gawde added a comment -

        Hello Friends,

        Any ETA on getting this integrated on 11.04 branch? I already have this fix in my vendor branch, so I am not dependent. But in case, there are any questions or suggestions, I can alter the code while the change is fresh in mind. Also, it would avoid future merge issues.

        Cheers,
        Kiran

        Show
        Kiran Gawde added a comment - Hello Friends, Any ETA on getting this integrated on 11.04 branch? I already have this fix in my vendor branch, so I am not dependent. But in case, there are any questions or suggestions, I can alter the code while the change is fresh in mind. Also, it would avoid future merge issues. Cheers, Kiran
        Hide
        Adrian Crum added a comment -

        Kiran,

        Thank you for working on this. I agree that the overuse of ECAs causes problems and makes the services difficult to troubleshoot. But removing them is going to be a tough sell because many in the community see it as a feature - they see it as a crude form of a workflow implementation. I have added my vote to this issue because I believe a lot of the ECAs should go away. It might help your cause to start a discussion on the dev mailing list and see if you can rally some more support for ECA removal.

        Show
        Adrian Crum added a comment - Kiran, Thank you for working on this. I agree that the overuse of ECAs causes problems and makes the services difficult to troubleshoot. But removing them is going to be a tough sell because many in the community see it as a feature - they see it as a crude form of a workflow implementation. I have added my vote to this issue because I believe a lot of the ECAs should go away. It might help your cause to start a discussion on the dev mailing list and see if you can rally some more support for ECA removal.
        Hide
        Anil K Patel added a comment -

        Thanks for inputs.
        I like the theme of this thread and plan to spend sometime review the code and test it. Don't know how soon it might be so did not assign task to myself.

        Show
        Anil K Patel added a comment - Thanks for inputs. I like the theme of this thread and plan to spend sometime review the code and test it. Don't know how soon it might be so did not assign task to myself.
        Hide
        Jacques Le Roux added a comment -

        Just FYI: part of this code is pretty old: http://svn.ofbiz.org/viewcvs/trunk/components/product/servicedef/secas_shipment.xml?rev=82&view=markup
        At this time maybe things were far less complicated...

        Show
        Jacques Le Roux added a comment - Just FYI: part of this code is pretty old: http://svn.ofbiz.org/viewcvs/trunk/components/product/servicedef/secas_shipment.xml?rev=82&view=markup At this time maybe things were far less complicated...
        Hide
        Jacques Le Roux added a comment -

        Hi Anil,

        Did you get chances to review and test?

        Show
        Jacques Le Roux added a comment - Hi Anil, Did you get chances to review and test?
        Hide
        Jacques Le Roux added a comment -

        One month, still on it?

        Show
        Jacques Le Roux added a comment - One month, still on it?
        Hide
        Jacques Le Roux added a comment -

        Hi Anil, did you get a chance to review?

        I don't know if it's the case but it was blocking Kiran to contribute more, Kiran?

        Show
        Jacques Le Roux added a comment - Hi Anil, did you get a chance to review? I don't know if it's the case but it was blocking Kiran to contribute more , Kiran?

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Kiran Gawde
          • Votes:
            3 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development