OFBiz
  1. OFBiz
  2. OFBIZ-3883

Customer's Shipment Address Not Assigned to Dropship Purchase Orders

    Details

      Description

      When buying drop ship products from the demo store the customer's shipping address is not saved with the purchase order, so the shipping address cannot be provided to the drop ship supplier.

      Steps to Reproduce:

      • From the DropShip Category add "DropShip from BigSupplier" and "DropShip from DemoSupplier" to your cart.
      • Login as admin
      • Use the one page checkout to order the products

      Resulting Order:
      https://demo-trunk.ofbiz.apache.org:8443/ordermgr/control/orderview?orderId=WSCO10002
      The purchase orders WS10003 and WS10004 are associated correctly with the order items, but as the shipping groups don't have addresses, the purchase orders don't have either.

      Expectation:
      All Shipment groups should have the shipping address assigned.

      Actual:
      Only the first shipment group has the shipping address assigned.

      Possibly offending line of Code:
      ShoppingCart.java line 2235:
      this.setShippingContactMechId(0, shippingContactMechId);
      Should probably not add the contactMechId only to the first CartShipInfo. When we tried to change that, though, drop shipping broke somehow completely.

        Activity

        Hide
        Rene Scheibe added a comment - - edited

        While reproducing it on my local machine, I further found out that some shipping data for the 2nd supplier (DemoSupplier) is missing in table order_item_ship_group - see below:

        order_id ship_group_seq_id shipment_method_type_id supplier_party_id carrier_party_id contact_mech_id
        LWS10000 00001 NO_SHIPPING BigSupplier NA 10000
        LWS10000 00002 NULL DemoSupplier NULL NULL
        Show
        Rene Scheibe added a comment - - edited While reproducing it on my local machine, I further found out that some shipping data for the 2nd supplier (DemoSupplier) is missing in table order_item_ship_group - see below: order_id ship_group_seq_id shipment_method_type_id supplier_party_id carrier_party_id contact_mech_id LWS10000 00001 NO_SHIPPING BigSupplier NA 10000 LWS10000 00002 NULL DemoSupplier NULL NULL
        Hide
        John McDonald added a comment -

        I tested this patch and it works for us.

        ShoppingCart.java line 2222
        this.setShippingContactMechId(x, shippingContactMechId);

        Replace with:
        for(int x=0; x<shipInfo.size(); x++)

        { this.setShippingContactMechId(x, shippingContactMechId); }

        This only fixes the address issue for drop shipments, NOT the missing shipping method and carrier details.

        Show
        John McDonald added a comment - I tested this patch and it works for us. ShoppingCart.java line 2222 this.setShippingContactMechId(x, shippingContactMechId); Replace with: for(int x=0; x<shipInfo.size(); x++) { this.setShippingContactMechId(x, shippingContactMechId); } This only fixes the address issue for drop shipments, NOT the missing shipping method and carrier details.
        Hide
        John McDonald added a comment - - edited

        So this issue plagues several other methods in ShoppingCart.java. For each of the methods, the fix is the same: wrap the method content with a loop and change the '0' to 'x'. Example provided in previous comment. Here's a list of all of the methods that need to be changed:

        setShippingContactMechId <- patch shown in previous comment
        setShipmentMethodTypeId
        setShippingInstructions
        setMaySplit
        setIsGift
        setGiftMessage
        setCarrierPartyId
        setProductStoreShipMethId

        This has been tested and works properly by effectively copying all of the shipping details to all of the ship groups.

        Show
        John McDonald added a comment - - edited So this issue plagues several other methods in ShoppingCart.java. For each of the methods, the fix is the same: wrap the method content with a loop and change the '0' to 'x'. Example provided in previous comment. Here's a list of all of the methods that need to be changed: setShippingContactMechId <- patch shown in previous comment setShipmentMethodTypeId setShippingInstructions setMaySplit setIsGift setGiftMessage setCarrierPartyId setProductStoreShipMethId This has been tested and works properly by effectively copying all of the shipping details to all of the ship groups.
        Hide
        Jacques Le Roux added a comment -

        Could you please provide a patch?

        Show
        Jacques Le Roux added a comment - Could you please provide a patch?
        Hide
        John McDonald added a comment - - edited

        Patch attached.

        Show
        John McDonald added a comment - - edited Patch attached.
        Hide
        Paul Foxworthy added a comment -

        Hi John,

        As you know these methods are overloaded and there is a second form where you specify the shipping group.

        I think the current implementation was designed for convenience in the common situation where there is only one shipping group. Your suggested change will change the semantics of the methods.

        What do you think about this:

        • Introduce new methods with names setAllShippingContactMechId etc, which execute a loop as you've suggested.
        • Deprecate the existing setShippingContactMechId etc which only set shipping group zero. Those methods are, as you've observed, dangerous and confusing. The two-parameter forms where a shipping group is specified should be kept as-is, i.e. not deprecated.
        • Think through the right thing to do for each call to the deprecated methods. They should be changed to call the setAll..., or possibly to the two-parameter form to make clear a change to only one shipping group is intended.
        • Eventually we should be able to remove the deprecated methods altogether.

        Cheers

        Paul Foxworthy

        Show
        Paul Foxworthy added a comment - Hi John, As you know these methods are overloaded and there is a second form where you specify the shipping group. I think the current implementation was designed for convenience in the common situation where there is only one shipping group. Your suggested change will change the semantics of the methods. What do you think about this: Introduce new methods with names setAllShippingContactMechId etc, which execute a loop as you've suggested. Deprecate the existing setShippingContactMechId etc which only set shipping group zero. Those methods are, as you've observed, dangerous and confusing. The two-parameter forms where a shipping group is specified should be kept as-is, i.e. not deprecated. Think through the right thing to do for each call to the deprecated methods. They should be changed to call the setAll..., or possibly to the two-parameter form to make clear a change to only one shipping group is intended. Eventually we should be able to remove the deprecated methods altogether. Cheers Paul Foxworthy
        Hide
        John McDonald added a comment -

        Jeff,
        Those are excellent suggestions. I wasn't too concerned about modifying the existing methods as I searched the codebase for all places where it's being called and only found a few. Based on those situations it didn't appear that it would impact the intended functionality so I made the change and then tested the referenced code and didn't immediately uncover any issues. As you know, though, testing is subjective and it's likely that I missed some test cases. lol

        With that being said, I prefer your changes over the ones that I've suggested.

        Show
        John McDonald added a comment - Jeff, Those are excellent suggestions. I wasn't too concerned about modifying the existing methods as I searched the codebase for all places where it's being called and only found a few. Based on those situations it didn't appear that it would impact the intended functionality so I made the change and then tested the referenced code and didn't immediately uncover any issues. As you know, though, testing is subjective and it's likely that I missed some test cases. lol With that being said, I prefer your changes over the ones that I've suggested.
        Hide
        Jacques Le Roux added a comment -

        Paul, John,

        Thanks a bunch for you efforts (notably Paul's review ). I will wait a new patch before reviewing myself

        Show
        Jacques Le Roux added a comment - Paul, John, Thanks a bunch for you efforts (notably Paul's review ). I will wait a new patch before reviewing myself
        Hide
        John McDonald added a comment - - edited

        New patch attached, based on feedback from Paul.

        Show
        John McDonald added a comment - - edited New patch attached, based on feedback from Paul.
        Hide
        Jacques Le Roux added a comment -

        Thanks guys,

        John, your (enhanced) patch is in
        trunk r1407116 + 1407142
        R12.04 r1407117 + 1407143
        R11.04 r1407153
        R10.04 r1407163

        Thanks to Paul's help I rather removed than deprecate the old methods and replaced them where relevant. I also adapted the related tests

        Show
        Jacques Le Roux added a comment - Thanks guys, John, your (enhanced) patch is in trunk r1407116 + 1407142 R12.04 r1407117 + 1407143 R11.04 r1407153 R10.04 r1407163 Thanks to Paul's help I rather removed than deprecate the old methods and replaced them where relevant. I also adapted the related tests
        Hide
        Jacques Le Roux added a comment - - edited

        == TYPO ==
        At David's demand I reverted the branches commmits above and I will commit rather John's patch as I wanted to do intially, but thought that we should better fix the issues in branches also. Then people will be able to decide what to do on their own as John suggested.
        R12.04 reverted at r1407627
        R11.04 reverted at r1407630
        R10.04 reverted at r1407631

        Show
        Jacques Le Roux added a comment - - edited == TYPO == At David's demand I reverted the branches commmits above and I will commit rather John's patch as I wanted to do intially, but thought that we should better fix the issues in branches also. Then people will be able to decide what to do on their own as John suggested. R12.04 reverted at r1407627 R11.04 reverted at r1407630 R10.04 reverted at r1407631
        Hide
        Jacques Le Roux added a comment -

        John's patch committed in
        R12.04 r1407684
        R11.04 r1407696
        R10.04 r1407700

        Show
        Jacques Le Roux added a comment - John's patch committed in R12.04 r1407684 R11.04 r1407696 R10.04 r1407700
        Hide
        Paul Foxworthy added a comment -

        If anyone's interested, the discussion of this issue is here:

        http://ofbiz.135035.n4.nabble.com/Re-svn-commit-r1407163-in-ofbiz-branches-release10-04-applications-accounting-src-org-ofbiz-accounti-td4637370.html

        I can see David's point re the stability of releases.

        Cheers

        Paul Foxworthy

        Show
        Paul Foxworthy added a comment - If anyone's interested, the discussion of this issue is here: http://ofbiz.135035.n4.nabble.com/Re-svn-commit-r1407163-in-ofbiz-branches-release10-04-applications-accounting-src-org-ofbiz-accounti-td4637370.html I can see David's point re the stability of releases. Cheers Paul Foxworthy
        Hide
        Jacques Le Roux added a comment - - edited

        == TYPO ==
        I slept on it and now I wonder if we should not actually change the calls to the deprecated methods but keep them to avoid the issue David reported. Then we have best of both worlds: issue fixed but still backwards compatible, opinions?

        Show
        Jacques Le Roux added a comment - - edited == TYPO == I slept on it and now I wonder if we should not actually change the calls to the deprecated methods but keep them to avoid the issue David reported. Then we have best of both worlds: issue fixed but still backwards compatible, opinions?
        Hide
        Jacques Le Roux added a comment -

        I will do that when I will get a chance...

        Show
        Jacques Le Roux added a comment - I will do that when I will get a chance...
        Hide
        Jacques Le Roux added a comment -

        Actually, I decided while reviewing OFBIZ-5102 was it was good enough in releases!

        Show
        Jacques Le Roux added a comment - Actually, I decided while reviewing OFBIZ-5102 was it was good enough in releases!

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Martin Kreidenweis
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development