OFBiz › OFBiz - Dev

Re: svn commit: r990007 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order: order/OrderServices.java shoppingcart/CheckOutHelper.java

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

4 messages Options

Adam Heath-2

Re: svn commit: r990007 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order: order/OrderServices.java shoppingcart/CheckOutHelper.java

On 08/26/2010 10:20 PM, [hidden email] wrote:

> Author: jonesde
> Date: Fri Aug 27 03:20:10 2010
> New Revision: 990007
>
> URL: http://svn.apache.org/viewvc?rev=990007&view=rev
> Log:
> Related to 990006 the tax calc address code is made more consistent and supported at a lower level by passing in a facilityId when available; this is the order component part and the other commit was the lower level accounting component part
>
> Modified:
> ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java
> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java
>
> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java?rev=990007&r1=990006&r2=990007&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java (original)
> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java Fri Aug 27 03:20:10 2010
> @@ -1529,6 +1529,7 @@ public class OrderServices {
>
> // if shippingAddress is still null then don't calculate tax; it may be an situation where no tax is applicable, or the data is bad and we don't have a way to find an address to check tax for
> if (shippingAddress == null) {
> + Debug.logWarning("Not calculating tax for Order [" + orderId + "] because there is no shippingAddress, and no address on the origin facility [" + orderHeader.getString("originFacilityId") + "]", module);
> continue;
> }
>
>
> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java?rev=990007&r1=990006&r2=990007&view=diff
> ==============================================================================
> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java (original)
> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java Fri Aug 27 03:20:10 2010
> @@ -56,6 +56,7 @@ import org.ofbiz.order.shoppingcart.prod
> import org.ofbiz.order.shoppingcart.shipping.ShippingEvents;
> import org.ofbiz.order.thirdparty.paypal.ExpressCheckoutEvents;
> import org.ofbiz.party.contact.ContactHelper;
> +import org.ofbiz.party.contact.ContactMechWorker;
> import org.ofbiz.product.store.ProductStoreWorker;
> import org.ofbiz.service.GenericServiceException;
> import org.ofbiz.service.LocalDispatcher;
> @@ -759,7 +760,7 @@ public class CheckOutHelper {
> int shipGroups = this.cart.getShipGroupSize();
> for (int i = 0; i< shipGroups; i++) {
> Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap = new HashMap<Integer, ShoppingCartItem>();
> - Map<String, Object> serviceContext = this.makeTaxContext(i, shipAddress, shoppingCartItemIndexMap);
> + Map<String, Object> serviceContext = this.makeTaxContext(i, shipAddress, shoppingCartItemIndexMap, cart.getFacilityId());
> List<List<? extends Object>> taxReturn = this.getTaxAdjustments(dispatcher, "calcTax", serviceContext);
>
> if (Debug.verboseOn()) Debug.logVerbose("ReturnList: " + taxReturn, module);
> @@ -786,7 +787,7 @@ public class CheckOutHelper {
> }
> }
>
> - private Map<String, Object> makeTaxContext(int shipGroup, GenericValue shipAddress, Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap) {
> + private Map<String, Object> makeTaxContext(int shipGroup, GenericValue shipAddress, Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap, String originFacilityId) {
> ShoppingCart.CartShipInfo csi = cart.getShipInfo(shipGroup);
> int totalItems = csi.shipItemInfo.size();
>
> @@ -835,6 +836,26 @@ public class CheckOutHelper {
> }
> }
>
> + if (shipAddress == null) {
> + // face-to-face order; use the facility address
> + if (originFacilityId != null) {
> + GenericValue facilityContactMech = ContactMechWorker.getFacilityContactMechByPurpose(delegator, originFacilityId, UtilMisc.toList("SHIP_ORIG_LOCATION", "PRIMARY_LOCATION"));
> + if (facilityContactMech != null) {
> + try {
> + shipAddress = delegator.findByPrimaryKey("PostalAddress",
> + UtilMisc.toMap("contactMechId", facilityContactMech.getString("contactMechId")));
> + } catch (GenericEntityException e) {
> + Debug.logError(e, module);
> + }
> + }
> + }
> + }
> +
> + // if shippingAddress is still null then don't calculate tax; it may be an situation where no tax is applicable, or the data is bad and we don't have a way to find an address to check tax for
> + if (shipAddress == null) {
> + Debug.logWarning("Not calculating tax for new order because there is no shipping address, no billing address, and no address on the origin facility [" + originFacilityId + "]", module);
> + }
> +

This is a poor change. Consider the case where cart items are
assigned to ship groups, but the actual contactMechId for the group is
set much much later.

When a web browser first connects, it is not logged in. The client
starts adding products to the cart, and assigning them to
shipgroups(based on a nick name). No contactMechId is set at this
time. However, there *is* a facilityId, as the store has been
configured that way.

So, in this case, this logic will assume that each ship group is a
face-to-face sale, which is definately not the case.

So, maybe you would turn off the tax calculation in such a case. That
would fix it before the user is logged in.

Now, consider a previous user logging in, and only *some* of their
nick-name based shipgroups have pre-existing addresses. Those ship
groups will have a contactMechId auto-set, and the checkout process
will then prompt for the missing PostalAddress destinations. However,
it wants to display correct tax info, so it calculates the taxes. The
shipgroups with a contactMechId set will be correct, but then the new
groups will again fall back on the facility/face-to-face.

Also, what would happen if all items in a shipgroup were digital, and
didn't even need a contactMechId to be set?

> Map<String, Object> serviceContext = UtilMisc.<String, Object>toMap("productStoreId", cart.getProductStoreId());
> serviceContext.put("payToPartyId", cart.getBillFromVendorPartyId());
> serviceContext.put("billToPartyId", cart.getBillToCustomerPartyId());
>
>

David E. Jones-2

Re: svn commit: r990007 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order: order/OrderServices.java shoppingcart/CheckOutHelper.java

Read the code just above, that's where the facility's address is considered.

-David

On Nov 30, 2010, at 4:37 PM, Adam Heath wrote:

> On 08/26/2010 10:20 PM, [hidden email] wrote:
>> Author: jonesde
>> Date: Fri Aug 27 03:20:10 2010
>> New Revision: 990007
>>
>> URL: http://svn.apache.org/viewvc?rev=990007&view=rev
>> Log:
>> Related to 990006 the tax calc address code is made more consistent and supported at a lower level by passing in a facilityId when available; this is the order component part and the other commit was the lower level accounting component part
>>
>> Modified:
>> ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java
>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java?rev=990007&r1=990006&r2=990007&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java Fri Aug 27 03:20:10 2010
>> @@ -1529,6 +1529,7 @@ public class OrderServices {
>>
>> // if shippingAddress is still null then don't calculate tax; it may be an situation where no tax is applicable, or the data is bad and we don't have a way to find an address to check tax for
>> if (shippingAddress == null) {
>> + Debug.logWarning("Not calculating tax for Order [" + orderId + "] because there is no shippingAddress, and no address on the origin facility [" + orderHeader.getString("originFacilityId") + "]", module);
>> continue;
>> }
>>
>>
>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java?rev=990007&r1=990006&r2=990007&view=diff
>> ==============================================================================
>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java (original)
>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java Fri Aug 27 03:20:10 2010
>> @@ -56,6 +56,7 @@ import org.ofbiz.order.shoppingcart.prod
>> import org.ofbiz.order.shoppingcart.shipping.ShippingEvents;
>> import org.ofbiz.order.thirdparty.paypal.ExpressCheckoutEvents;
>> import org.ofbiz.party.contact.ContactHelper;
>> +import org.ofbiz.party.contact.ContactMechWorker;
>> import org.ofbiz.product.store.ProductStoreWorker;
>> import org.ofbiz.service.GenericServiceException;
>> import org.ofbiz.service.LocalDispatcher;
>> @@ -759,7 +760,7 @@ public class CheckOutHelper {
>> int shipGroups = this.cart.getShipGroupSize();
>> for (int i = 0; i< shipGroups; i++) {
>> Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap = new HashMap<Integer, ShoppingCartItem>();
>> - Map<String, Object> serviceContext = this.makeTaxContext(i, shipAddress, shoppingCartItemIndexMap);
>> + Map<String, Object> serviceContext = this.makeTaxContext(i, shipAddress, shoppingCartItemIndexMap, cart.getFacilityId());
>> List<List<? extends Object>> taxReturn = this.getTaxAdjustments(dispatcher, "calcTax", serviceContext);
>>
>> if (Debug.verboseOn()) Debug.logVerbose("ReturnList: " + taxReturn, module);
>> @@ -786,7 +787,7 @@ public class CheckOutHelper {
>> }
>> }
>>
>> - private Map<String, Object> makeTaxContext(int shipGroup, GenericValue shipAddress, Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap) {
>> + private Map<String, Object> makeTaxContext(int shipGroup, GenericValue shipAddress, Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap, String originFacilityId) {
>> ShoppingCart.CartShipInfo csi = cart.getShipInfo(shipGroup);
>> int totalItems = csi.shipItemInfo.size();
>>
>> @@ -835,6 +836,26 @@ public class CheckOutHelper {
>> }
>> }
>>
>> + if (shipAddress == null) {
>> + // face-to-face order; use the facility address
>> + if (originFacilityId != null) {
>> + GenericValue facilityContactMech = ContactMechWorker.getFacilityContactMechByPurpose(delegator, originFacilityId, UtilMisc.toList("SHIP_ORIG_LOCATION", "PRIMARY_LOCATION"));
>> + if (facilityContactMech != null) {
>> + try {
>> + shipAddress = delegator.findByPrimaryKey("PostalAddress",
>> + UtilMisc.toMap("contactMechId", facilityContactMech.getString("contactMechId")));
>> + } catch (GenericEntityException e) {
>> + Debug.logError(e, module);
>> + }
>> + }
>> + }
>> + }
>> +
>> + // if shippingAddress is still null then don't calculate tax; it may be an situation where no tax is applicable, or the data is bad and we don't have a way to find an address to check tax for
>> + if (shipAddress == null) {
>> + Debug.logWarning("Not calculating tax for new order because there is no shipping address, no billing address, and no address on the origin facility [" + originFacilityId + "]", module);
>> + }
>> +
>
> This is a poor change. Consider the case where cart items are assigned to ship groups, but the actual contactMechId for the group is set much much later.
>
> When a web browser first connects, it is not logged in. The client starts adding products to the cart, and assigning them to shipgroups(based on a nick name). No contactMechId is set at this time. However, there *is* a facilityId, as the store has been configured that way.
>
> So, in this case, this logic will assume that each ship group is a face-to-face sale, which is definately not the case.
>
> So, maybe you would turn off the tax calculation in such a case. That would fix it before the user is logged in.
>
> Now, consider a previous user logging in, and only *some* of their nick-name based shipgroups have pre-existing addresses. Those ship groups will have a contactMechId auto-set, and the checkout process will then prompt for the missing PostalAddress destinations. However, it wants to display correct tax info, so it calculates the taxes. The shipgroups with a contactMechId set will be correct, but then the new groups will again fall back on the facility/face-to-face.
>
> Also, what would happen if all items in a shipgroup were digital, and didn't even need a contactMechId to be set?
>
>> Map<String, Object> serviceContext = UtilMisc.<String, Object>toMap("productStoreId", cart.getProductStoreId());
>> serviceContext.put("payToPartyId", cart.getBillFromVendorPartyId());
>> serviceContext.put("billToPartyId", cart.getBillToCustomerPartyId());
>>
>>
>

David E. Jones-2

Re: svn commit: r990007 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order: order/OrderServices.java shoppingcart/CheckOutHelper.java

In reply to this post by Adam Heath-2

Never mind my last comment, I see in your message has the other scenarios you are looking for.

Once a user has an address the code to get it from the facility will no longer apply.

Is there some way you would like to see this operate differently?

-David

On Nov 30, 2010, at 4:37 PM, Adam Heath wrote:

Adam Heath-2

Re: svn commit: r990007 - in /ofbiz/trunk/applications/order/src/org/ofbiz/order: order/OrderServices.java shoppingcart/CheckOutHelper.java

In reply to this post by David E. Jones-2

On 11/30/2010 05:48 PM, David E Jones wrote:
>
> Read the code just above, that's where the facility's address is considered.

Sure. But consider the case where the origin facility has been set(by
partially going thru a checkout), then the user adds more items to the
cart, and assigns them to brand new shipgroups, that have no address.

>
> -David
>
>
> On Nov 30, 2010, at 4:37 PM, Adam Heath wrote:
>
>> On 08/26/2010 10:20 PM, [hidden email] wrote:
>>> Author: jonesde
>>> Date: Fri Aug 27 03:20:10 2010
>>> New Revision: 990007
>>>
>>> URL: http://svn.apache.org/viewvc?rev=990007&view=rev
>>> Log:
>>> Related to 990006 the tax calc address code is made more consistent and supported at a lower level by passing in a facilityId when available; this is the order component part and the other commit was the lower level accounting component part
>>>
>>> Modified:
>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java
>>> ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java
>>>
>>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java?rev=990007&r1=990006&r2=990007&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java (original)
>>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/order/OrderServices.java Fri Aug 27 03:20:10 2010
>>> @@ -1529,6 +1529,7 @@ public class OrderServices {
>>>
>>> // if shippingAddress is still null then don't calculate tax; it may be an situation where no tax is applicable, or the data is bad and we don't have a way to find an address to check tax for
>>> if (shippingAddress == null) {
>>> + Debug.logWarning("Not calculating tax for Order [" + orderId + "] because there is no shippingAddress, and no address on the origin facility [" + orderHeader.getString("originFacilityId") + "]", module);
>>> continue;
>>> }
>>>
>>>
>>> Modified: ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java
>>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java?rev=990007&r1=990006&r2=990007&view=diff
>>> ==============================================================================
>>> --- ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java (original)
>>> +++ ofbiz/trunk/applications/order/src/org/ofbiz/order/shoppingcart/CheckOutHelper.java Fri Aug 27 03:20:10 2010
>>> @@ -56,6 +56,7 @@ import org.ofbiz.order.shoppingcart.prod
>>> import org.ofbiz.order.shoppingcart.shipping.ShippingEvents;
>>> import org.ofbiz.order.thirdparty.paypal.ExpressCheckoutEvents;
>>> import org.ofbiz.party.contact.ContactHelper;
>>> +import org.ofbiz.party.contact.ContactMechWorker;
>>> import org.ofbiz.product.store.ProductStoreWorker;
>>> import org.ofbiz.service.GenericServiceException;
>>> import org.ofbiz.service.LocalDispatcher;
>>> @@ -759,7 +760,7 @@ public class CheckOutHelper {
>>> int shipGroups = this.cart.getShipGroupSize();
>>> for (int i = 0; i< shipGroups; i++) {
>>> Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap = new HashMap<Integer, ShoppingCartItem>();
>>> - Map<String, Object> serviceContext = this.makeTaxContext(i, shipAddress, shoppingCartItemIndexMap);
>>> + Map<String, Object> serviceContext = this.makeTaxContext(i, shipAddress, shoppingCartItemIndexMap, cart.getFacilityId());
>>> List<List<? extends Object>> taxReturn = this.getTaxAdjustments(dispatcher, "calcTax", serviceContext);
>>>
>>> if (Debug.verboseOn()) Debug.logVerbose("ReturnList: " + taxReturn, module);
>>> @@ -786,7 +787,7 @@ public class CheckOutHelper {
>>> }
>>> }
>>>
>>> - private Map<String, Object> makeTaxContext(int shipGroup, GenericValue shipAddress, Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap) {
>>> + private Map<String, Object> makeTaxContext(int shipGroup, GenericValue shipAddress, Map<Integer, ShoppingCartItem> shoppingCartItemIndexMap, String originFacilityId) {
>>> ShoppingCart.CartShipInfo csi = cart.getShipInfo(shipGroup);
>>> int totalItems = csi.shipItemInfo.size();
>>>
>>> @@ -835,6 +836,26 @@ public class CheckOutHelper {
>>> }
>>> }
>>>
>>> + if (shipAddress == null) {
>>> + // face-to-face order; use the facility address
>>> + if (originFacilityId != null) {
>>> + GenericValue facilityContactMech = ContactMechWorker.getFacilityContactMechByPurpose(delegator, originFacilityId, UtilMisc.toList("SHIP_ORIG_LOCATION", "PRIMARY_LOCATION"));
>>> + if (facilityContactMech != null) {
>>> + try {
>>> + shipAddress = delegator.findByPrimaryKey("PostalAddress",
>>> + UtilMisc.toMap("contactMechId", facilityContactMech.getString("contactMechId")));
>>> + } catch (GenericEntityException e) {
>>> + Debug.logError(e, module);
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>> + // if shippingAddress is still null then don't calculate tax; it may be an situation where no tax is applicable, or the data is bad and we don't have a way to find an address to check tax for
>>> + if (shipAddress == null) {
>>> + Debug.logWarning("Not calculating tax for new order because there is no shipping address, no billing address, and no address on the origin facility [" + originFacilityId + "]", module);
>>> + }
>>> +
>>
>> This is a poor change. Consider the case where cart items are assigned to ship groups, but the actual contactMechId for the group is set much much later.
>>
>> When a web browser first connects, it is not logged in. The client starts adding products to the cart, and assigning them to shipgroups(based on a nick name). No contactMechId is set at this time. However, there *is* a facilityId, as the store has been configured that way.
>>
>> So, in this case, this logic will assume that each ship group is a face-to-face sale, which is definately not the case.
>>
>> So, maybe you would turn off the tax calculation in such a case. That would fix it before the user is logged in.
>>
>> Now, consider a previous user logging in, and only *some* of their nick-name based shipgroups have pre-existing addresses. Those ship groups will have a contactMechId auto-set, and the checkout process will then prompt for the missing PostalAddress destinations. However, it wants to display correct tax info, so it calculates the taxes. The shipgroups with a contactMechId set will be correct, but then the new groups will again fall back on the facility/face-to-face.
>>
>> Also, what would happen if all items in a shipgroup were digital, and didn't even need a contactMechId to be set?
>>
>>> Map<String, Object> serviceContext = UtilMisc.<String, Object>toMap("productStoreId", cart.getProductStoreId());
>>> serviceContext.put("payToPartyId", cart.getBillFromVendorPartyId());
>>> serviceContext.put("billToPartyId", cart.getBillToCustomerPartyId());
>>>
>>>
>>
>